fix(gateway): allow password fallback for trusted-proxy auth mode#64122
fix(gateway): allow password fallback for trusted-proxy auth mode#64122jetd1 wants to merge 28 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR fixes a real breakage where
Confidence Score: 4/5Safe to merge after addressing the missing rate-limit pre-check in the trusted-proxy password fallback path. The fix is architecturally sound and the credential-planner, credentials, and call.ts changes are all consistent and well-tested. One P1 gap: the rate-limiter lockout is never queried before the password comparison in the trusted-proxy fallback block, leaving the fallback path open to unlimited password retries even after the IP has been locked out. The password itself has high entropy (48-char hex), which reduces practical risk, but the missing check contradicts the PR's own stated security guarantee and should be addressed. src/gateway/auth.ts — the trusted-proxy password fallback block (around line 561) needs a limiter.check() pre-check before the password comparison.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62bf4e7935
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
62bf4e7 to
3749ccb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3749ccb588
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
83236e2 to
a570e01
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a570e01b87
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
a570e01 to
a67d8ca
Compare
a67d8ca to
cccb1ff
Compare
cccb1ff to
6358423
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63584239a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // same-host reverse proxy — which hits `trusted_proxy_loopback_source` | ||
| // while forwarding external traffic — cannot bypass proxy-header | ||
| // checks just by presenting the password. | ||
| if (isNonProxyFailure && localDirect && auth.password && connectAuth?.password) { |
There was a problem hiding this comment.
Harden trusted-proxy fallback against loopback proxy traffic
This fallback gate relies on localDirect, which only means loopback socket + no forwarded headers; it does not prove the request originated on-host. A same-host reverse proxy that forwards external traffic without Forwarded/X-Forwarded-* headers will still satisfy localDirect, and because trusted_proxy_loopback_source is treated as a non-proxy failure, this branch can accept password auth and bypass trusted-proxy header/user policy. In publicly exposed same-host proxy deployments, that broadens password fallback from local-only to remotely reachable traffic.
Useful? React with 👍 / 👎.
|
I think this should be a first-class service identity flow, not a trusted-proxy fallback. In trusted-proxy mode, internal OpenClaw calls (CLI/tools/subagents/backend) should authenticate as a service principal (service account), with explicit validation in the gateway. That’s clearer and more secure than inferring trust from loopback behaviour or missing proxy headers. If proxy-derived identity is required for these calls, the WS/RPC path should use the configured proxy URL so auth behaviour stays consistent with the rest of trusted-proxy mode. I appreciate the security hardening in #54536. But this PR + that work still feels like incremental exception handling. A dedicated service-account auth path would be cleaner, easier to reason about, and less fragile long-term. And to remove friction using the service accoutn token, the CLI coul still accept a e.g. Thoughts? |
(cherry picked from commit fcc86f0)
(cherry picked from commit f350bb4)
…dit (openclaw#69581) * fix(codex): exclude codex-app-server synthetic apiKey from secrets audit The Codex extension uses the literal string "codex-app-server" as a hardcoded placeholder apiKey in provider.ts, since the real authentication is managed by the app-server transport itself. The secrets audit currently reports this as a real plaintext leak (PLAINTEXT_FOUND), producing a false positive for any user who has configured the Codex harness. Declare it as a plugin-owned non-secret marker in the Codex plugin manifest, so it flows through the standard `listKnownNonSecretApiKeyMarkers()` path alongside `ollama-local`, `lmstudio-local`, `gcp-vertex-credentials`, and `minimax-oauth`. Also extends the existing `model auth markers` unit tests to lock in the behavior. Fixes openclaw#69511 * ci: retrigger checks (no-op) (cherry picked from commit 081da17)
openclaw#69338) When ingestDelta returns null (first empty/commentary delta or unchanged content), the handler returned early, skipping setActivityStatus and armStreamingWatchdog. If all subsequent deltas were also null (e.g. due to phase filtering), the watchdog was never armed and the status bar stayed stale as "idle" while a run was live. Move setActivityStatus("streaming") and armStreamingWatchdog before the null-displayText guard so they fire on every received delta event. Fixes openclaw#34513, openclaw#40824 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit 89b6d02)
6358423 to
c883dd9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e0c530fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (params.mode === "trusted-proxy") { | ||
| return !isTokenPath; | ||
| } |
There was a problem hiding this comment.
Avoid forcing trusted-proxy password SecretRef resolution
This new trusted-proxy branch makes password SecretRefs mandatory to resolve even when password auth is only an optional fallback, which breaks flows that should still work without a resolved password. In particular, resolvePairingSetupFromConfig() calls materializeGatewayAuthSecretRefs() before choosing the auth label, so a config like gateway.auth.mode="trusted-proxy" plus an unavailable gateway.auth.password SecretRef now throws instead of allowing token-labeled pairing setup. That is a regression from optional fallback semantics and can block pairing/setup commands in trusted-proxy deployments with unresolved password refs.
Useful? React with 👍 / 👎.
13e0c53 to
cbe92c1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbe92c146d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (params.mode === "trusted-proxy") { | ||
| return !isTokenPath; |
There was a problem hiding this comment.
Keep trusted-proxy password SecretRefs optional
shouldResolveGatewayAuthSecretRef now resolves gateway.auth.password for every trusted-proxy call (return !isTokenPath), so unresolved password SecretRefs become hard failures even when password is only an optional fallback. This regresses flows that can proceed without a password, e.g. resolvePairingSetupFromConfig() materializes auth SecretRefs before selecting the trusted-proxy auth label, so an unavailable password ref can abort pairing setup instead of cleanly continuing with token/bootstrap behavior.
Useful? React with 👍 / 👎.
|
Thanks @jetd1. Closing this as superseded by the landed fix in #73034, which closes #17761. The landed path keeps trusted-proxy credentials first, allows configured gateway password fallback only for local/same-host callers, keeps bearer-token fallback rejected, rate-limits fallback attempts, and includes docs/changelog plus regression coverage for the gateway and secret-surface behavior. Canonical merge: 7975305 |
Problem
When
gateway.auth.modeis"trusted-proxy", the gateway WebSocket auth handler rejects all non-proxy connections (loopback, untrusted source) without checking password credentials. This breaks local clients — subagents, browser extensions, and CLI — that connect directly and cannot go through the reverse proxy.PR #63280 (
b1724f8b5f) auto-generates a browser control password fortrusted-proxymode and persists it togateway.auth.password. However, the credential resolution path (call.ts,credential-planner.ts,credentials.ts) still blocks password reading fortrusted-proxymode, and the gateway auth dispatcher (auth.ts) still early-returns on trusted-proxy failure without attempting password verification.Result:
callGateway()never reads the password from config, and even if it did, the server would never check it. Subagent spawns, browser extension connections, and local CLI calls all fail withunauthorized.This is reported in #17761, #26007, and #43300.
Root Cause
Two commits introduced conflicting assumptions:
25252ab5ab(Mar 7, auth hardening)localAuthModeAllowsGatewaySecretInputPath()returnfalsefortrusted-proxy, blocking all secret resolutionb1724f8b5f(Apr 9, browser auto-token)gateway.auth.passwordin trusted-proxy modeThe server-side
authorizeGatewayConnectCoretreats auth modes as mutually exclusive: trusted-proxy early-returns on failure, never falling back to password.Solution
Make
trusted-proxy+passwordwork as complementary channels serving different client types, rather than mutually exclusive modes:Changes
1.
src/gateway/auth.ts—authorizeGatewayConnectCoreAfter the trusted-proxy path fails (untrusted source, loopback, missing header), attempt password verification if both the client and config provide one. The proxy path is tried first and remains preferred; password is only a second channel for local clients.
2.
src/gateway/call.ts—localAuthModeAllowsGatewaySecretInputPathAllow password (but not token) secret input resolution for
trusted-proxymode. Token remains mutually exclusive with trusted-proxy; password does not.3.
src/gateway/credential-planner.ts—passwordCanWinAdd
authMode === "trusted-proxy"to thepasswordCanWincondition so the credential planner considers password a viable credential.4.
src/gateway/credentials.ts—localPasswordCanWinAdd
params.plan.authMode === "trusted-proxy"to thelocalPasswordCanWincondition so local password resolution is allowed.Security Properties Preserved
openclaw.json. Any attacker who can read it already has the same access level as a password-holding client.password_mismatchrecords a rate-limit failure, preventing brute-force attacks.Test Coverage
auth.test.ts: password accepted from loopback, password accepted from untrusted source, wrong password rejected, no client password rejected, no gateway password rejected, trusted-proxy identity still preferred over passwordcall.test.ts: splitit.each(["none", "trusted-proxy"])into separate cases —nonestill ignores password,trusted-proxynow resolves itcredentials.test.ts:trusted-proxywith unresolved password ref now throws instead of silently returningundefinedAll 168 tests across
auth.test.ts,call.test.ts,credentials.test.ts, andconnection-auth.test.tspass.Closes #17761, #26007, #43300.