fix: allow trusted-proxy local password fallback#73034
Conversation
PR SummaryMedium Risk Overview Updates credential planning/SecretRef resolution so Reviewed by Cursor Bugbot for commit df2428e. Bugbot is set up for automated code reviews on this repo. Configure here. |
2000897 to
f6b756c
Compare
Greptile SummaryThis PR introduces a local password fallback for Confidence Score: 4/5Safe to merge; the fallback is correctly scoped to local-direct callers and guarded by rate limiting. No P0 or P1 issues found. Two P2 style observations: the outer guard makes the password_missing_config/password_missing branches inside authorizePasswordAuth unreachable from the trusted-proxy path (intentional, but undocumented in code), and probing with no password credential bypasses rate-limit counting (consistent with the normal password-mode behaviour). All other changes are logically consistent and well-covered. src/gateway/auth.ts around the trusted-proxy password fallback block (lines 464-484) Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/auth.ts
Line: 464-483
Comment:
**Outer guard makes `authorizePasswordAuth` dead paths unreachable**
The outer condition `localDirect && auth.password && connectAuth?.password` guards the block, so `authorizePasswordAuth` is always called with non-nullish `authPassword` and `connectPassword`. Its internal `password_missing_config` and `password_missing` branches can never fire from this path — the code falls through to `result.reason` instead. This is likely intentional information-hiding (don't reveal whether a password is configured on proxy failure), but the divergence from the normal `auth.mode === "password"` flow is worth a brief comment to make the intent explicit.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/gateway/auth.ts
Line: 464
Comment:
**Rate limiter not checked when `connectAuth?.password` is absent**
When a local-direct caller supplies no password at all (`connectAuth?.password` is falsy), the condition short-circuits and we return `result.reason` without ever touching the rate limiter. An attacker that repeatedly probes with no password credential can avoid incrementing the rate limit counter indefinitely. The normal `auth.mode === "password"` path has a similar property (returns `password_missing` without recording failure), so this is consistent — but it means the rate limiter only protects against wrong-password attacks, not credential-probing without a credential. Worth a comment acknowledging the deliberate choice.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: allow trusted-proxy local password ..." | Re-trigger Greptile |
| if (localDirect && auth.password && connectAuth?.password) { | ||
| if (limiter) { | ||
| const rlCheck: RateLimitCheckResult = limiter.check(ip, rateLimitScope); | ||
| if (!rlCheck.allowed) { | ||
| return { | ||
| ok: false, | ||
| reason: "rate_limited", | ||
| rateLimited: true, | ||
| retryAfterMs: rlCheck.retryAfterMs, | ||
| }; | ||
| } | ||
| } | ||
| return authorizePasswordAuth({ | ||
| authPassword: auth.password, | ||
| connectPassword: connectAuth.password, | ||
| limiter, | ||
| ip, | ||
| rateLimitScope, | ||
| }); | ||
| } |
There was a problem hiding this comment.
authorizePasswordAuth dead paths unreachable
The outer condition localDirect && auth.password && connectAuth?.password guards the block, so authorizePasswordAuth is always called with non-nullish authPassword and connectPassword. Its internal password_missing_config and password_missing branches can never fire from this path — the code falls through to result.reason instead. This is likely intentional information-hiding (don't reveal whether a password is configured on proxy failure), but the divergence from the normal auth.mode === "password" flow is worth a brief comment to make the intent explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/auth.ts
Line: 464-483
Comment:
**Outer guard makes `authorizePasswordAuth` dead paths unreachable**
The outer condition `localDirect && auth.password && connectAuth?.password` guards the block, so `authorizePasswordAuth` is always called with non-nullish `authPassword` and `connectPassword`. Its internal `password_missing_config` and `password_missing` branches can never fire from this path — the code falls through to `result.reason` instead. This is likely intentional information-hiding (don't reveal whether a password is configured on proxy failure), but the divergence from the normal `auth.mode === "password"` flow is worth a brief comment to make the intent explicit.
How can I resolve this? If you propose a fix, please make it concise.| @@ -439,6 +461,26 @@ async function authorizeGatewayConnectCore( | |||
| } | |||
| return { ok: true, method: "trusted-proxy", user: result.user }; | |||
| } | |||
| if (localDirect && auth.password && connectAuth?.password) { | |||
There was a problem hiding this comment.
connectAuth?.password is absent
When a local-direct caller supplies no password at all (connectAuth?.password is falsy), the condition short-circuits and we return result.reason without ever touching the rate limiter. An attacker that repeatedly probes with no password credential can avoid incrementing the rate limit counter indefinitely. The normal auth.mode === "password" path has a similar property (returns password_missing without recording failure), so this is consistent — but it means the rate limiter only protects against wrong-password attacks, not credential-probing without a credential. Worth a comment acknowledging the deliberate choice.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/auth.ts
Line: 464
Comment:
**Rate limiter not checked when `connectAuth?.password` is absent**
When a local-direct caller supplies no password at all (`connectAuth?.password` is falsy), the condition short-circuits and we return `result.reason` without ever touching the rate limiter. An attacker that repeatedly probes with no password credential can avoid incrementing the rate limit counter indefinitely. The normal `auth.mode === "password"` path has a similar property (returns `password_missing` without recording failure), so this is consistent — but it means the rate limiter only protects against wrong-password attacks, not credential-probing without a credential. Worth a comment acknowledging the deliberate choice.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2000897cee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (authMode === "trusted-proxy") { | ||
| return !isTokenGatewaySecretInputPath(path); |
There was a problem hiding this comment.
Exclude remote password refs from trusted-proxy local auth
In local gateway.auth.mode: "trusted-proxy", this branch now treats every non-token secret path as eligible, which includes gateway.remote.password. Because gatewaySecretInputPathCanWin scans all gateway secret paths, a resolvable remote password ref can be selected and forwarded as the local shared secret, even though the fallback is documented/implemented as gateway.auth.password-based. This can make local direct auth use the wrong secret (and hit shared-secret rate limiting) when remote credentials differ from local trusted-proxy fallback credentials.
Useful? React with 👍 / 👎.
b90447e to
3133dee
Compare
9f47678 to
861d2b2
Compare
8b63c60 to
df2428e
Compare
|
Landed via rebase onto
Thanks @steipete. |
Summary
Closes #17761.
Supersedes the narrow trusted-proxy password-fallback slice of #64122 with rate-limit coverage and without the broader unrelated changes.
Tests
pnpm test src/gateway/auth.test.ts src/gateway/call.test.ts src/gateway/credentials.test.tspnpm check:changed