From 53d9e58d3e1e952eef257ae4135abf60764dbfd1 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 05:24:20 +0000 Subject: [PATCH] fix(clear-rate-limit): match RATE_LIMIT_DOMAIN literally in the LIKE pattern clear-rate-limit built its DELETE pattern by concatenating RATE_LIMIT_DOMAIN into "EMAIL#%@#ENDPOINT#forgot_password". The query was parameterized, but % and _ inside the domain (and the _ in "forgot_password") are LIKE wildcards, so RATE_LIMIT_DOMAIN=% cleared the forgot_password rate limits of every domain, and a domain with an _ matched any character in that position. The domain and endpoint are now escaped and the DELETE uses ESCAPE '\', so only the email's local part is a wildcard. The query moved into clearForgotPasswordLimits so a real-Postgres test can call it. The cleanup-lambda half of the same review finding is not changed here: its status guard already landed in #1173, and its retention rule is left for the maintainer to decide (see the PR body). Refs #357 Co-Authored-By: Claude Opus 5.5 --- cmd/lambda/clear-rate-limit/main.go | 30 ++++- .../clear-rate-limit/main_integration_test.go | 119 ++++++++++++++++++ cmd/lambda/clear-rate-limit/main_test.go | 23 ++++ 3 files changed, 168 insertions(+), 4 deletions(-) create mode 100644 cmd/lambda/clear-rate-limit/main_integration_test.go create mode 100644 cmd/lambda/clear-rate-limit/main_test.go diff --git a/cmd/lambda/clear-rate-limit/main.go b/cmd/lambda/clear-rate-limit/main.go index 93238d1fc..4723a711a 100644 --- a/cmd/lambda/clear-rate-limit/main.go +++ b/cmd/lambda/clear-rate-limit/main.go @@ -5,6 +5,7 @@ import ( "fmt" "log" "os" + "strings" "github.com/LeanerCloud/cloud-commitments-platform/internal/database" "github.com/aws/aws-lambda-go/lambda" @@ -12,6 +13,23 @@ import ( const defaultDomain = "leanercloud.com" +// forgotPasswordEndpoint is the rate-limit endpoint whose per-email rows this +// function clears (see internal/api/rate_limiter.go). +const forgotPasswordEndpoint = "forgot_password" + +// likeEscaper escapes the LIKE metacharacters (and the escape character +// itself) so a value matches only itself inside a pattern that is used with +// ESCAPE '\'. +var likeEscaper = strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`) + +// forgotPasswordPattern is the LIKE pattern for every forgot_password row of +// one email domain. Only the email's local part is a wildcard: the domain and +// endpoint are escaped, so RATE_LIMIT_DOMAIN=% matches a literal "%" domain +// instead of every domain, and an "_" matches only "_" (issue #357). +func forgotPasswordPattern(domain string) string { + return "EMAIL#%@" + likeEscaper.Replace(domain) + "#ENDPOINT#" + likeEscaper.Replace(forgotPasswordEndpoint) +} + func getDomain() string { if domain := os.Getenv("RATE_LIMIT_DOMAIN"); domain != "" { return domain @@ -35,11 +53,15 @@ func clearRateLimit(ctx context.Context) (Response, error) { } defer db.Close() - // Clear rate limits for forgot_password endpoint - domain := getDomain() + return clearForgotPasswordLimits(ctx, db, getDomain()) +} + +// clearForgotPasswordLimits deletes the forgot_password rate-limit rows of +// every email at domain and reports how many rate-limit rows remain. +func clearForgotPasswordLimits(ctx context.Context, db *database.Connection, domain string) (Response, error) { tag, err := db.Exec(ctx, - "DELETE FROM rate_limits WHERE id LIKE $1", - "EMAIL#%@"+domain+"#ENDPOINT#forgot_password") + `DELETE FROM rate_limits WHERE id LIKE $1 ESCAPE '\'`, + forgotPasswordPattern(domain)) if err != nil { return Response{}, fmt.Errorf("failed to delete rate limits: %w", err) } diff --git a/cmd/lambda/clear-rate-limit/main_integration_test.go b/cmd/lambda/clear-rate-limit/main_integration_test.go new file mode 100644 index 000000000..3c95e18ab --- /dev/null +++ b/cmd/lambda/clear-rate-limit/main_integration_test.go @@ -0,0 +1,119 @@ +//go:build integration + +package main + +// Real PostgreSQL tests for the clear-rate-limit DELETE (issue #357). The +// migrated rate_limits table and the DELETE are real; nothing is deployed and +// no Lambda is invoked. + +import ( + "context" + "path/filepath" + "runtime" + "sort" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/require" +) + +// rateLimitRows covers the domain the tests clear, other domains, a literal +// "%" domain, and rows of other endpoints and key kinds. The exampleQtest.com +// and forgotXpassword rows match only while "_" is a wildcard. +var rateLimitRows = []string{ + "EMAIL#alice@example.com#ENDPOINT#forgot_password", + "EMAIL#bob@example.com#ENDPOINT#forgot_password", + "EMAIL#dave@other.org#ENDPOINT#forgot_password", + "EMAIL#erin@%#ENDPOINT#forgot_password", + "EMAIL#frank@example_test.com#ENDPOINT#forgot_password", + "EMAIL#grace@exampleQtest.com#ENDPOINT#forgot_password", + "EMAIL#alice@example.com#ENDPOINT#forgotXpassword", + "EMAIL#alice@example.com#ENDPOINT#login", + "IP#192.0.2.1#ENDPOINT#forgot_password_ip", +} + +func rateLimitTestDB(ctx context.Context, t *testing.T) *database.Connection { + t.Helper() + c := testhelpers.RequirePostgresContainer(ctx, t) + t.Cleanup(func() { _ = c.Cleanup(context.Background()) }) + _, file, _, _ := runtime.Caller(0) + path := filepath.Join(filepath.Dir(file), "..", "..", "..", "internal", "database", "postgres", "migrations") + require.NoError(t, migrations.RunMigrations(ctx, c.DB.Pool(), path, "", "")) + return c.DB +} + +func seedRateLimits(ctx context.Context, t *testing.T, db *database.Connection) { + t.Helper() + _, err := db.Exec(ctx, `DELETE FROM rate_limits`) + require.NoError(t, err) + for _, id := range rateLimitRows { + _, err := db.Exec(ctx, `INSERT INTO rate_limits (id, count, reset_time) VALUES ($1, 1, NOW() + INTERVAL '1 hour')`, id) + require.NoError(t, err, "insert %s", id) + } +} + +func remainingRateLimits(ctx context.Context, t *testing.T, db *database.Connection) []string { + t.Helper() + rows, err := db.Query(ctx, `SELECT id FROM rate_limits`) + require.NoError(t, err) + defer rows.Close() + var out []string + for rows.Next() { + var id string + require.NoError(t, rows.Scan(&id)) + out = append(out, id) + } + require.NoError(t, rows.Err()) + sort.Strings(out) + return out +} + +// without returns rateLimitRows minus the given ids, sorted. +func without(deleted ...string) []string { + gone := map[string]bool{} + for _, id := range deleted { + gone[id] = true + } + var out []string + for _, id := range rateLimitRows { + if !gone[id] { + out = append(out, id) + } + } + sort.Strings(out) + return out +} + +// TestClearForgotPasswordLimitsDeletesOnlyTheNamedDomain is the issue's +// failure scenario: RATE_LIMIT_DOMAIN=% used to clear the forgot_password rows +// of every domain, and an "_" in the domain matched any one character. +func TestClearForgotPasswordLimitsDeletesOnlyTheNamedDomain(t *testing.T) { + ctx := context.Background() + db := rateLimitTestDB(ctx, t) + + cases := []struct { + domain string + deleted []string + }{ + {"example.com", []string{ + "EMAIL#alice@example.com#ENDPOINT#forgot_password", + "EMAIL#bob@example.com#ENDPOINT#forgot_password", + }}, + {"%", []string{"EMAIL#erin@%#ENDPOINT#forgot_password"}}, + {"example_test.com", []string{"EMAIL#frank@example_test.com#ENDPOINT#forgot_password"}}, + {"nobody.invalid", nil}, + } + for _, tc := range cases { + t.Run(tc.domain, func(t *testing.T) { + seedRateLimits(ctx, t, db) + resp, err := clearForgotPasswordLimits(ctx, db, tc.domain) + require.NoError(t, err) + want := without(tc.deleted...) + require.Equal(t, want, remainingRateLimits(ctx, t, db)) + require.Equal(t, len(tc.deleted), resp.DeletedCount) + require.Equal(t, len(want), resp.RemainingCount) + }) + } +} diff --git a/cmd/lambda/clear-rate-limit/main_test.go b/cmd/lambda/clear-rate-limit/main_test.go new file mode 100644 index 000000000..1503fa1d0 --- /dev/null +++ b/cmd/lambda/clear-rate-limit/main_test.go @@ -0,0 +1,23 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestForgotPasswordPatternEscapesLikeMetacharacters pins the pattern the +// DELETE runs with ESCAPE '\': only the email's local part is a wildcard +// (issue #357). The real-Postgres test in main_integration_test.go checks +// which rows the pattern actually deletes. +func TestForgotPasswordPatternEscapesLikeMetacharacters(t *testing.T) { + cases := map[string]string{ + "example.com": `EMAIL#%@example.com#ENDPOINT#forgot\_password`, + "%": `EMAIL#%@\%#ENDPOINT#forgot\_password`, + "example_test.com": `EMAIL#%@example\_test.com#ENDPOINT#forgot\_password`, + `ex\ample.com`: `EMAIL#%@ex\\ample.com#ENDPOINT#forgot\_password`, + } + for domain, want := range cases { + assert.Equal(t, want, forgotPasswordPattern(domain), "domain %q", domain) + } +}