Repository navigation
Conversation
…pattern clear-rate-limit built its DELETE pattern by concatenating RATE_LIMIT_DOMAIN into "EMAIL#%@<domain>#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 LeanerCloud#1173, and its retention rule is left for the maintainer to decide (see the PR body). Refs LeanerCloud#357 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #357 lists two findings. I re-checked both against current main (
a14dc8f2):cmd/cleanup-lambdaDELETE has no status guard, so onceexpires_atis set it could hard-deletepending/scheduled/runningrows47bf1dc3, "fix(cleanup): only delete settled executions when their TTL lapses", the fix for #998). BothdeleteExpiredanddryRunCountnow useexpires_at < $1 AND status = ANY(TerminalExecutionStatuses) AND status <> ALL(HealthScoredExecutionStatuses). Real-Postgres tests incmd/cleanup-lambda/main_integration_test.gocover every status and a concurrent writer.cmd/cleanup-lambdacan never delete an execution because no writer setsexpires_at, so it reportsexecutionsDeleted: 0foreverRATE_LIMIT_DOMAINis put into aLIKEpattern without escaping%and_Finding 2: root cause and fix
cmd/lambda/clear-rate-limit/main.gobuilt its DELETE pattern as"EMAIL#%@" + domain + "#ENDPOINT#forgot_password". The query was parameterized, so this was never SQL injection. But%and_in the domain are LIKE wildcards, and so is the_inforgot_password. As a result:RATE_LIMIT_DOMAIN=%cleared the forgot_password rate limits of every domain, as the issue describes._, such asexample_test.com, also clearedexampleQtest.com._also matched other endpoint names such asforgotXpassword.Fix:
forgotPasswordPatternescapes\,%and_in the domain and the endpoint, and the DELETE now runsid LIKE $1 ESCAPE '\'. Only the email's local part is still a wildcard, which is the intended scope. I moved the query intoclearForgotPasswordLimits(ctx, db, domain)so a real-Postgres test can call it.clearRateLimitstill opens the connection and calls that function, so the Lambda's behavior and response are otherwise the same.I chose escaping over rejecting the value. Escaping keeps exact-match semantics for any input, so a bad value deletes nothing instead of returning an error. A typo now clears too little instead of too much.
Finding 1b: why it is deferred
The status-guard half is already on main (see the table). The remaining problem is that the job's only execution predicate is the row's TTL, and no current writer sets one. Fixing that means choosing a retention rule for this binary. I don't think that should be decided in a cleanup PR:
docs/DEPLOYMENT.mdsays neitherterraform/modules/compute/aws/cleanup-lambda/norgcp/cleanup-function/is used by any environment. The issue's "today" scenario (the cron logging 0 deleted forever) therefore doesn't happen in a real deployment. fix(iac/gcp): cleanup-function module points at an AWS Lambda handler and a deprecated go121 runtime #365, the GCP module's broken entry point, was released earlier for the same reason: where cleanup should run is still undecided.cleanuptask, which callsCleanupOldExecutions(DefaultExecutionTTLDays). That task deletes completed rows byscheduled_dateand canceled rows byupdated_at. A second, hand-written rule in this Lambda would be a copy that can drift from it.CleanupOldExecutions. That means refactoringCleanupOldExecutionsso its predicate can also be counted. Branchfix/issue-47, not opened yet, rewrites that function's query and doc comment. It also editscmd/cleanup-lambda/main_integration_test.go. Separately, fix(cleanup): only delete settled executions when their TTL lapses #1173 noted thatCleanupOldExecutionsbranch 3 has no live-status guard. Calling it from the Lambda as it stands would undo the guard fix(cleanup): only delete settled executions when their TTL lapses #1173 added.Options for the maintainer: (a) make
cmd/cleanup-lambdacall a shared, live-guarded version ofCleanupOldExecutionsafterfix/issue-47lands; (b) deletecmd/cleanup-lambdaand its two unused modules if cleanup stays in the server task (#712); or (c) setPurchaseExecution.TTLwhen executions are created. Option (c) conflicts with the ramp-retention trigger from migration 000124 and with the health-score windows, so I'd avoid it.Refs #357
Verification
All checks ran locally on the branch head. The DB evidence comes from a real Postgres testcontainer with every migration applied. No Lambda was deployed or invoked, and no AWS API was called. All commands ran with
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true.New tests:
cmd/lambda/clear-rate-limit/main_test.go, unit test (runs in plaingo test):TestForgotPasswordPatternEscapesLikeMetacharacterspins the escaped pattern forexample.com,%,example_test.comand a backslash domain.cmd/lambda/clear-rate-limit/main_integration_test.go(//go:build integration):TestClearForgotPasswordLimitsDeletesOnlyTheNamedDomainseeds forgot_password rows for several domains, including a literal%domain,example_test.comandexampleQtest.com, plus aforgotXpasswordrow, aloginrow and an IP row. It runs the real DELETE forexample.com,%,example_test.comand an unused domain, then checks the exact surviving rows and both response counts.Both test files are force-added: the
.gitignoreentryclear-rate-limit, meant for the binary, also matches the package directory.main.gowas already tracked the same way.Commands:
main.go's old pattern and query swapped back in:expected "EMAIL#%@\%#ENDPOINT#forgot\_password", actual"EMAIL#%@%#ENDPOINT#forgot_password").%fails: the DELETE also removed theexample.com(alice and bob),other.org,example_test.comandexampleQtest.comrows, plus theforgotXpasswordrow.example_test.comfails: it also removedexampleQtest.com.example.comfails: it also removedforgotXpassword.nobody.invalidpasses, as expected.ESCAPE '\'clause is not caught. Postgres's default LIKE escape character is already\, so the clause only makes the behavior explicit and the result is the same. Escaping only the domain but not the endpoint is caught by both tests.Other checks:
go build ./...,go vet ./...,go vet -tags integration ./...: cleango test -count=1 ./internal/mocks/: okrun --allow-parallel-runners ./cmd/lambda/clear-rate-limit/..., with and without--build-tags integration: 0 issuesgocyclo -over 10 -ignore "_test\.go" cmd/lambda/clear-rate-limit/: silentgofmt -l: cleanCombined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope / notes
clear-rate-limitstill has no dry-run mode, and itsdefaultDomainis still hard-coded. The 2026-09-02 audit (A10-022) mentions both, but this issue doesn't ask for them.CleanupOldExecutionsbranch 3's missing live-status guard (raised in fix(cleanup): only delete settled executions when their TTL lapses #1173) is still open. It touches the same code asfix/issue-47.Maintainer: please mirror the issue's triage labels onto this PR:
triaged,priority/p3,severity/low,urgency/eventually,impact/internal,effort/l,type/chore.🤖 Generated with Claude Code