sec(auth): constant-time token validation via subtle.ConstantTimeCompare (closes #392) by cristim · Pull Request #837 · LeanerCloud/cloud-commitments-cli

cristim · 2026-05-28T19:38:37Z

Summary

ValidateUserAPIKey: add subtle.ConstantTimeCompare of the derived SHA-256/base64 hash against the stored key_hash after the DB row is fetched, closing the SQL equality timing oracle on the user API key path
validateResetToken + ResetTokenStatus: same pattern for password-reset tokens, comparing user.PasswordResetToken against the locally derived tokenHash
No schema changes; no migration needed -- the stored hash format (SHA-256/hex for reset tokens, SHA-256/base64 for API keys) is unchanged
Fix test fixtures that used literal stub values ("hashed-token") to use hashSessionToken(token) so the new constant-time check passes; add two mismatch test cases to assert the guard rejects tampered rows

The admin API key path in middleware.go already used subtle.ConstantTimeCompare; this PR brings the user API key and password-reset token paths to parity.

Test plan

go test ./internal/auth/... passes (500 tests, 0 failures)
go build ./... succeeds
New test reject when stored hash does not match derived hash confirms the API key guard fires
New test constant-time mismatch rejects token even when DB row exists confirms the reset-token guard fires in ConfirmPasswordReset
New test constant-time mismatch returns used even if DB row exists confirms the guard fires in ResetTokenStatus

coderabbitai · 2026-05-28T19:38:44Z

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e0e2402c-e6a3-41b3-b216-3325aa3a3995

📥 Commits

Reviewing files that changed from the base of the PR and between f692085 and a770254.

📒 Files selected for processing (8)

internal/auth/service.go
internal/auth/service_apikeys.go
internal/auth/service_apikeys_test.go
internal/auth/service_password.go
internal/auth/service_password_test.go
internal/auth/service_security_test.go
internal/auth/service_test.go
internal/server/adapter_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/392-wave12

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-05-28T19:38:44Z

@coderabbitai review

coderabbitai · 2026-05-28T19:38:49Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T10:22:09Z

@coderabbitai full review

coderabbitai · 2026-05-30T10:22:14Z

✅ Actions performed

Full review triggered.

cristim · 2026-06-04T14:03:54Z

@coderabbitai review

coderabbitai · 2026-06-04T14:04:08Z

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 27 seconds before sending another message.

ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures.

cristim · 2026-06-06T07:02:27Z

Adversarial review (post-fix)

Walked the risk surfaces in the brief; one in-scope gap fixed as 11f088884:

Session token validation (internal/auth/service.go:271 ValidateSession): had the same SQL-equality timing oracle as the API-key and reset-token paths fixed in this PR -- GetSession runs WHERE token = $1 in PostgreSQL, which short-circuits on first mismatching byte. This is the hottest auth surface in the app (every authenticated request hits it via internal/api/handler.go:196), so the oracle is more exploitable here than on the user API-key path. Added the same subtle.ConstantTimeCompare(session.Token, hashedToken) guard, with parallel "constant-time mismatch rejects session even when DB row exists" test plus fixture updates in internal/server/adapter_test.go (two GetSession stubs used literal "hashed-token" strings that no longer match after the guard).

Other surfaces walked and verdicts:

All other token compare sites are already constant-time: CSRF (service.go:323), MFA (service_mfa.go:64), Azure scheduledauth bearer (validator.go:325), purchase approvals + messages (approvals.go:40,248, messages.go:203), RI exchange approvals (handler_ri_exchange.go:1050,1158), admin API key middleware (middleware.go:68,182). Grep for == / strings.Compare on token-shaped fields turned up no remaining defects.
Length-equality concern: ConstantTimeCompare is non-constant-time across lengths, but on every callsite in this PR both sides are derived from the same sha256.Sum256 (fixed-length 43-char base64 for API keys, 64-char hex for reset/session tokens). No attacker-controllable length variability.
Logs: no raw tokens or PII in any log site touched by the PR. KeyPrefix logging is hash-prefix style (safe).
Test coverage: positive (existing valid session/successfully validate API key/etc.) AND negative (the three new "constant-time mismatch rejects" cells) both covered.

Verification: go test ./internal/... -- 4223 passed, 0 failures. go build ./... clean.

@coderabbitai review

coderabbitai · 2026-06-06T07:02:52Z

@cristim Excellent adversarial analysis — the session-token path (ValidateSession) was indeed the highest-risk omission given its call frequency, and closing it in the same PR keeps the timing-oracle fix atomically complete across all three surfaces (user API key, reset token, session). A few notes on your documented verdicts:

Length safety: The argument is airtight — sha256.Sum256 produces a fixed 32-byte digest; base64 encoding of that is always 43 chars and hex is always 64 chars, so ConstantTimeCompare never leaks length information on these paths.
Surface coverage: The grep-verified list (middleware.go, service_mfa.go, validator.go, approvals.go, messages.go, handler_ri_exchange.go) is a solid audit trail for reviewers.
Test completeness: Having both matching and mismatching fixture cases for all three guards gives confident regression coverage.

Triggering the full review now.

🐇✨

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures.

cristim · 2026-06-19T21:13:31Z

@coderabbitai review

coderabbitai · 2026-06-19T21:13:37Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-07-09T21:21:23Z

@coderabbitai review

coderabbitai · 2026-07-09T21:21:30Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures.

cristim · 2026-07-10T15:04:04Z

@coderabbitai review

coderabbitai · 2026-07-10T15:04:11Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures.

cristim · 2026-07-17T07:57:22Z

@coderabbitai full review

cristim · 2026-07-17T07:57:23Z

Merge-prep: rebased onto origin/main (27fdb06). One conflict in service_apikeys.go resolved: kept constant-time compare (PR security intent) + main's refactored err= style. go build/vet/lint/gocyclo all exit 0; auth tests pass (count=1); scheduledauth -count=5 clean.

coderabbitai · 2026-07-17T07:57:48Z

✅ Action performed

Full review finished.

You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 8 minutes.

ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures.

cristim · 2026-07-17T11:13:21Z

Rebased onto origin/main (89658a3) to pick up the #1437 tflint-cache and #1438 trivy-skip fixes. New tip: dd1cc4e. Resolved potential conflict in internal/auth/service_apikeys_test.go (main added TestService_HasAPIKeyPermissionAPI at line 797; PR adds a test case inside TestService_ValidateUserAPIKey at line 651 - separate chunks, clean merge). Local gates: go build/vet exit=0, gocyclo -over 10 on touched files exit=0, golangci-lint ./... exit=0 (no issues), go test ./internal/auth/... 595 passed, go test ./internal/server/... 410 passed.

SQL equality in PostgreSQL is not constant-time; an attacker on the same VPC can use response-time differences to learn bytes of a SHA-256 hash. The admin API key path already uses subtle.ConstantTimeCompare; align the user API key and password-reset token paths to match. No schema changes: the stored hash format is unchanged. After the DB row is fetched, compare the expected hash against the stored hash via crypto/subtle.ConstantTimeCompare in ValidateUserAPIKey, validateResetToken, and ResetTokenStatus. A mismatch returns the same error as a missing row to avoid timing oracles via differing code paths. Update all test fixtures that set PasswordResetToken to literal stub values to instead use hashSessionToken(token) so they match the hash the service now verifies. Add targeted mismatch test cases to both the API-key and reset-token validation paths.

ValidateSession had the same SQL-equality timing oracle as the user API key and password-reset token paths fixed in this PR: GetSession runs `WHERE token = $1` in PostgreSQL, which short-circuits on first mismatching byte. Session tokens are the hottest auth surface (every authenticated API request hits ValidateSession via handler.go), so the oracle is even more exploitable here than on the API key path. Add a `subtle.ConstantTimeCompare` of `session.Token` against the locally-derived `hashedToken` immediately after the store fetch, returning the same "session not found" error to keep the response indistinguishable from a missing-row outcome. `crypto/subtle` is already imported in service.go. Tests: - New `TestService_ValidateSession/constant-time mismatch rejects session even when DB row exists` asserts the guard fires when the stored token differs from the derived hash. - Fix `internal/server/adapter_test.go` fixtures that used literal stub tokens (`"hashed-token"`, `"hashed-session-token"`) to use a local `hashSessionTokenForTest` helper mirroring auth.hashSessionToken (private), so the new guard passes for the positive cases. `go test ./internal/...` -- 4223 passed, 0 failures.

Session.Role was removed from the base branch; the constant-time mismatch test in service_test.go still referenced it, breaking go vet. Remove the field from the struct literal; the test logic is unaffected (it verifies token-hash rejection, not role checks).

ValidateUserAPIKey now runs subtle.ConstantTimeCompare(key.KeyHash, keyHash) after the DB fetch. The TestValidateUserAPIKey_LastUsedSingleflight fixture omitted KeyHash, so the empty stored hash failed the compare and the function returned early before the background UpdateAPIKeyLastUsed ran, breaking the mock's Once expectation. Real DB rows always carry KeyHash, so the fixture must set it to represent the actual validated path.

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/security Security finding labels May 28, 2026

cristim force-pushed the fix/392-wave12 branch from 11f0888 to eaeb33f Compare June 7, 2026 21:44

cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:44

cristim force-pushed the fix/392-wave12 branch from eaeb33f to a1e1518 Compare June 19, 2026 21:13

cristim force-pushed the fix/392-wave12 branch from a1e1518 to 546955c Compare July 10, 2026 15:03

cristim force-pushed the fix/392-wave12 branch from 546955c to 1c30951 Compare July 17, 2026 07:57

cristim force-pushed the fix/392-wave12 branch from 1c30951 to dd1cc4e Compare July 17, 2026 11:12

cristim added 4 commits July 17, 2026 15:25

cristim force-pushed the fix/392-wave12 branch from dd1cc4e to a770254 Compare July 17, 2026 12:32

cristim merged commit ea8f266 into main Jul 17, 2026
19 checks passed

cristim mentioned this pull request Jul 17, 2026

Adversarial review of all high-stakes merged PRs — findings tracker #1448

Closed

30 tasks

cristim deleted the fix/392-wave12 branch July 27, 2026 11:09

Conversation

cristim commented May 28, 2026

Summary

Test plan

Uh oh!

coderabbitai Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented May 28, 2026

Uh oh!

coderabbitai Bot commented May 28, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026

Rate Limit Exceeded

Uh oh!

cristim commented Jun 6, 2026

Adversarial review (post-fix)

Uh oh!

coderabbitai Bot commented Jun 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 19, 2026

Uh oh!

coderabbitai Bot commented Jun 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 9, 2026

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 17, 2026

Uh oh!

cristim commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented May 28, 2026 •

edited

Loading

coderabbitai Bot commented Jun 6, 2026 •

edited

Loading

coderabbitai Bot commented Jun 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading