fix(oauth): name the offending redirect URI when client validation fails - #1628
Conversation
Agent-connect OAuth is broken in production for every user, and the log line
that should have identified it in seconds gave a bare count instead.
What is wrong in prod: the `PSD OpenClaw` OIDC client
(7e8646f4-4091-4a34-a6b9-0d3721e8a126) carries three redirect URIs — two
correct production/dev HTTPS callbacks and one dev-only
`http://localhost:3000/agent-connect-aistudio/callback`. Its application_type is
`native`, and validateNativeUri rejects an HTTP redirect whose host is not a
literal loopback address: LOOPBACK_HOSTS is {"127.0.0.1", "[::1]"} and
`localhost` is not in it (RFC 8252). Confirmed by running the real validator
against the three stored URIs:
VALID: false
ERRORS: ["http://localhost:3000/agent-connect-aistudio/callback:
native HTTP redirect URIs must use literal 127.0.0.1 or [::1]"]
Validation is all-or-nothing, so that single entry makes loadClient return
undefined and oidc-provider answer `invalid_client` — the two valid URIs never
get a chance, and the flow is dead for everyone, not just the user who reported
it. That user's three attempts are the three errors logged at 19:35:28,
19:36:25 and 19:39:43 UTC on 2026-08-10.
This commit does NOT fix the bad data — the redirect URI is a prod DB row and
has to change through the admin client UI (to `http://127.0.0.1:3000/...`, or
by dropping the localhost entry). What it fixes is why that took a full
investigation to find.
The error logged `redirectErrorCount: 1` and nothing else. A count cannot be
told apart from a missing client, a disabled client, or a PKCE/auth-method
problem, and the caller only ever sees the generic `invalid_client`, so there
was no way to get from the symptom to the URI without reproducing the validator
by hand. `redirectValidation.errors` already held the exact message with the
offending URI in it — it simply was not being written down. It is now.
Redirect URIs are registration metadata, not credentials; these errors carry no
token, code, or secret, so logging them leaks nothing.
Test changes: the logger mock built a fresh object per createLogger() call, so
no test could ever assert on what was logged — which is why the missing detail
went unnoticed. It is now a single shared spy. Added a regression case that
reproduces the exact prod client (one bad localhost entry alongside a valid
production URI) and asserts the client fails closed AND that the log names the
offending URI without blaming the valid one. Verified it fails against the
previous code and passes here.
Verification: 68 tests pass across tests/unit/lib/oauth/; eslint clean on both
touched files and repo-wide. Repo typecheck reports the same 20 pre-existing
AWS SDK type errors before and after this change.
ReviewSmall, well-scoped fix — good root-cause diagnosis in the PR description (loopback vs. Code quality
BugsNone found. Logic is untouched; only the log payload grows. Security
PerformanceNegligible — a small, bounded string array added to a structured log call on an already-slow-path (client validation failure). Test coverage
Overall: solid, minimal diagnostic fix with a legitimate regression test. Nothing here blocks merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad0458a562
ℹ️ 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".
| clientId, | ||
| applicationType: client.applicationType, | ||
| redirectErrorCount: redirectValidation.errors.length, | ||
| redirectErrors: redirectValidation.errors, |
There was a problem hiding this comment.
When a stored redirect is rejected for containing URL userinfo, such as https://alice:hunter2@example.com/callback, validateCommon() rejects it but includes the original URI in redirectValidation.errors; logging that array therefore writes the username and password to CloudWatch. The logger's generic filtering does not recognize URL userinfo, and malformed or invalid-policy URIs may likewise contain sensitive query values, so redact userinfo and sensitive query data while retaining enough origin/path detail to identify the offending entry.
AGENTS.md reference: AGENTS.md:L293-L293
Useful? React with 👍 / 👎.
…re logging
Addresses the P2 on this PR, which was correct and contradicts what the
previous commit message asserted. I wrote that these errors "carry no token,
code, or secret". They can.
`validateCommon` rejects a redirect URI *because* it contains userinfo, and
`validateOAuthRedirectUris` builds its message as `${uri}: ${reason}` with the
original string interpolated. So a stored
`https://alice:hunter2@example.com/callback` produces the error
"https://alice:hunter2@example.com/callback: must not contain userinfo", and
logging that array writes the password to CloudWatch. The logger's generic
field filtering matches key names and does not recognise URL userinfo, so
nothing downstream would have caught it. Invalid-policy and malformed entries
can carry sensitive query values the same way.
The whole point of this PR is that a rejected redirect URI is untrusted input,
so it gets redacted at the point of logging rather than trusted because it came
from our own table. `redactUrisForLog` drops userinfo, query and fragment and
keeps scheme, host, port and path — enough to pick the offending entry out of
the client's stored list, which is all the diagnostic needs.
Two details worth stating:
- Redaction happens at the LOG site, not inside redirect-uri-policy. The admin
client UI surfaces these same errors to the person editing the URI, and there
the full string is correct: they typed it, and truncating it would make the
form unusable.
- The token filter deliberately accepts more than well-formed URIs. Requiring a
legal scheme would let `ht!tp://alice:hunter2@host` through untouched purely
because `ht!tp` is not a valid scheme, and a malformed entry is exactly the
kind most likely to be carrying junk. Anything containing `://`, `@` or `?`
is treated as sensitive; an unparseable candidate is replaced wholesale with
`<unparseable redirect URI>` rather than echoed. Ordinary prose, bare IPs like
`127.0.0.1`, and words such as "URI:" match neither test and pass through, so
the messages stay readable.
Tests: 5 new cases covering userinfo, query+fragment, the second
"Invalid redirect URI: <uri>" message shape, a malformed-scheme URI that still
carries a password, and a prose/localhost case asserting messages stay legible.
All five fail against the previous commit. 73 tests pass across
tests/unit/lib/oauth/; eslint clean; typecheck clean.
Review: fix(oauth) — name the offending redirect URI when client validation failsGood root-cause work: tracing I focused most of the review on the new Bug: mangles the literal "URI:" label in one of the two documented message shapesThe JSDoc says errors come in two shapes — Trace for
Result: The existing test ( Possible gap: opaque (no-
|
Agent-connect OAuth is broken in prod for every user
The
PSD OpenClawOIDC client (7e8646f4…) carries three redirect URIs — two correct HTTPS callbacks and one dev-onlyhttp://localhost:3000/agent-connect-aistudio/callback.Its
application_typeisnative, andvalidateNativeUrirejects an HTTP redirect whose host isn't a literal loopback address —LOOPBACK_HOSTSis{"127.0.0.1", "[::1]"}, andlocalhostisn't in it (RFC 8252).Confirmed by running the real validator against the three stored URIs:
Validation is all-or-nothing, so that single entry makes
loadClientreturnundefinedandoidc-provideranswerinvalid_client. The two valid URIs never get a chance. One user's three attempts are the errors logged at19:35:28,19:36:25,19:39:43UTC on 2026-08-10 — but this affects everyone.The redirect URI is a prod DB row and must change through the admin client UI — to
http://127.0.0.1:3000/..., or by dropping the localhost entry. The flow stays broken until that happens.What this PR fixes: why it took an investigation to find
The error logged
redirectErrorCount: 1and nothing else. A count can't be distinguished from a missing client, a disabled client, or a PKCE/auth-method problem — and the caller only ever sees the genericinvalid_client. There was no path from symptom to URI without reproducing the validator by hand.redirectValidation.errorsalready held the exact message with the offending URI. It just wasn't being written down. Now it is.Redirect URIs are registration metadata, not credentials — these errors carry no token, code, or secret.
Test changes
The logger mock built a fresh object per
createLogger()call, so no test could assert on what was logged — which is why the missing detail went unnoticed. It's now a single shared spy.Added a regression case reproducing the exact prod client (one bad localhost entry beside a valid production URI), asserting the client fails closed and that the log names the offending URI without blaming the valid one. Verified it fails against the previous code and passes here.
Verification
tests/unit/lib/oauth/eslintclean on both touched files and repo-wide