Repository navigation
Conversation
…, key rate limits on the client address First slice of the 2026-07-28 auth review's medium/low findings: - credentials.LoadKey memoized a failed secret fetch for the process lifetime via sync.Once, so one throttled fetch (or a first caller with a canceled context) broke credential decryption until the container recycled, even though ensureDB retries. Only a successful load is memoized now; loads are serialized so concurrent callers queue behind one fetch. - DBRateLimiter.Allow returned (true, nil) with no pool, silently disabling the strict credential-endpoint limits. It now returns ErrRateLimiterUnavailable, so checkRateLimitStrict answers 503. - The HTTP adapter keyed rate limits on net/http's "ip:port" RemoteAddr when no X-Forwarded-For was present, giving every connection a fresh bucket. The port is now dropped (also from an "ip:port" rightmost XFF entry), and an unparseable rightmost entry falls back to the peer. Refs LeanerCloud#104 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The combined-tree run with fix/issue-326 (which changes httpToLambdaRequest to take a ResponseWriter and return an error) failed to compile this test, which called httpToLambdaRequest(req). Test requestSourceIP directly and pin that httpToLambdaRequest derives SourceIP from it with a go/parser check, so the test builds whichever of the two branches merges first. Removing the wiring still fails the new check. Refs LeanerCloud#104 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
effort/l
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.
First slice of #104 (medium/low findings from the 2026-07-28 auth review). I re-checked every finding against current
main(a14dc8f2). This PR fixes three of them. The rest are either deferred to an in-flight branch that edits the same code, or left for a later slice, with the reason given for each in the table below.Refs #104
Per-finding status
maina14dc8f2resolveOnce/sync.Onceininternal/oidc/aws_signer.goandazure_signer.goresolveOncewith apublicKeyCachein the same lines. Rotation (a TTL, or keying on the resolved version) should be built on that cache once #1152 merges.GetPublicKeypoisons the signer, GCP included (medium)resetRecentlyIssued,service_password.go:298ip:portRemoteAddrfallback (MEDIUM)httpToLambdaRequest,internal/server/http.goRemoteAddr(and from anip:portrightmost XFF entry), and an unparseable rightmost entry falls back to the peer address. Deferred: a configurable trusted-proxy hop count. It needs a per-deployment setting (ingress topology per cloud, plus Terraform/CFN wiring), so the owner should decide the setting's name and default.enable_cdn = true(medium)httpclient.New()drops proxy support (MEDIUM), plus A08b-003 (otherDefaultTransportdefaults lost)pkg/httpclientmoved toLeanerCloud/cloud-commitments-go(pkg/httpclient/httpclient.go:169on itsmainstill builds a bare&http.Transport{}).internal/secrets/azure_resolver.gonow just callshttpclient.New().go.modbump here.LoadKeymemoizes a transient secret-fetch failure for the process lifetime (LOW)internal/credentials/cipher.gosync.OncecachedcachedKeyErr. This is reachable:ensureDBretriesreinitializeAfterConnecton the next request, but that retry got the cached error back.lower(email)unique index orcitext) plus a backfill, and the owner has to decide what happens to existing case-variant duplicate rows. The service-layer normalization would also touchservice_user.golines that fork branchesfix/issue-313andfix/issue-312rewrite.checkRateLimitStrictfails open when the limiter has no pool (latent, LOW)DBRateLimiter.Allownow returnsErrRateLimiterUnavailablewith no pool (or a nil receiver), so the strict path answers 503. Deferred: a nilHandler.rateLimitertreated as 503. I tried it, and it breaks 34 existing handler tests that build&Handler{}with no limiter (login, setup-admin, reset, change-password, MFA, router coverage). Each would need a permissive limiter fixture, which is a test sweep larger than this slice. Production always installs a limiter (app.go:523,:834).RecordUsageAsync(service_apikeys.go:466) runs once per request, but it still starts a goroutine per request before thesingleflightdedupe.fix/issue-313rewrites the adjacent code inservice_apikeys.go, and the fix (start a flusher only when none is in flight for the key) changes concurrency-sensitive accounting, so it deserves its own PR.awsCfgOnceloads the AWS config with the request's context (low)awsconfig.LoadDefaultConfigon an already-canceled context returnsnilerror (checked with a throwaway probe underAWS_CONFIG_FILE=/dev/null), because credentials and region resolve lazily.handler_accounts_hostcheck_test.go, which fork branchfix/issue-559also edits.#313 correction to #104's "Verified sound" section: #313 (A03-020) points out that the post-fetch
subtle.ConstantTimeComparecalls listed as a positive in #104 are inert. Each one compares a row's hash with the same hash the row was selected by. So that bullet should be read as "store-integrity check", not "closes a timing oracle". Fork branchfix/issue-313rewrites those comments. This PR leaves them alone.Root cause and fix
credentials.LoadKey(internal/credentials/cipher.go):sync.Oncestored the error alongside the key. Now a mutex-guarded cache memoizes only a successful load. A failed load returns its error, and the next call retries. The mutex is held across the load, so concurrent callers queue behind one fetch instead of all hitting Secrets Manager / Key Vault at once (no retry stampede). One caller's canceled context fails only that caller's load.DBRateLimiter.Allow(internal/api/db_rate_limiter.go): a nil pool or nil receiver returned(true, nil). It now returns(false, ErrRateLimiterUnavailable).checkRateLimitStrictturns that into 503 on login, setup-admin, reset-password, change-password and MFA. The ordinarycheckRateLimitkeeps its documented fail-open behavior and logs a warning.internal/server/source_ip.go, called fromhttpToLambdaRequest): the port is stripped fromRemoteAddrand from the rightmost XFF entry (some proxies, for example Azure Application Gateway, appendip:port). If the rightmost entry is not an IP after that, the peer address is used instead of an arbitrary string. The helper lives in its own file becausehttp.gois already over the 500-line guideline.SourceIPis only consumed by the rate limiter and a request log line, so dropping the port changes nothing else.Verification
All evidence below is unit-level (mocks, fakes and
httptest). Nothing here is DB-level: the limiter change only affects the no-pool branch, so I wrote no Postgres integration test. The concurrent case is theLoadKeystampede test, run under-race.Environment for all runs:
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true, Go 1.26.9.Fail-before: each new test was run against the unfixed code first.
go test -count=1 -run 'TestLoadKey_FailedLoadIsRetried|TestLoadKey_ConcurrentCallers' ./internal/credentials/failed both tests. The second call got the cachedcontext canceled. With 32 concurrent callers, all 32 failed (expected 1), and the resolver was called once (expected 2).go test -count=1 -run 'TestDBRateLimiter|TestHandler_checkRateLimitStrict' ./internal/api/failedTestDBRateLimiter_Allow_NilLimiter, all 4 subtests ofTestDBRateLimiter_NilPool_ReportsUnavailable, and both subtests ofTestHandler_checkRateLimitStrict_NilPoolFailsClosed, becauseAllowreturned(true, nil)and the strict check let the request through. Note: the old*_NilPooltests pinned the fail-open behavior. They are replaced, not deleted.go test -count=1 -run TestHTTPToLambdaRequest_SourceIP ./internal/server/failed 7 of 10 subcases (203.0.113.7:51234,[2001:db8::1]:443, the XFFip:portforms, and the unparseable/empty rightmost entry). The 3 cases that already behaved correctly passed.Pass-after, on head
29abc168:go build ./...,go vet ./...,go vet -tags integration ./...: clean.go test -count=1on./internal/credentials/,./internal/server/,./internal/api/,./internal/mocks/and./cmd/rekey/: allok.go test -race -count=1 -run 'TestLoadKey|TestDBRateLimiter|TestHandler_checkRateLimit' ./internal/credentials/ ./internal/api/:ok.golangci-lintv2.10.1run --allow-parallel-runnerson./internal/credentials/... ./internal/server/ ./internal/api/: 0 issues.gocyclo -over 10 -ignore "_test\.go"on the changed non-test files: no output.Combined-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 / follow-ups
Handler.rateLimitertest sweep, the per-request goroutine inRecordUsageAsync, and the httpclient transport fix in cloud-commitments-go.internal/server/http.gois 530 lines after this change (was 537), still over the 500-line guideline.httpToLambdaRequestreads only the firstX-Forwarded-Forheader line (Header.Get). Whether to join repeated header lines depends on how each ingress appends, so I left it unchanged here and it should be handled with the hop-count work.Could a maintainer mirror the issue's triage labels onto this PR:
effort/l,triaged,priority/p3,severity/low,urgency/eventually,impact/many,type/security.🤖 Generated with Claude Code