fix(oauth): repair the OpenClaw client registration and make redirect URIs editable - #1629
Conversation
… URIs editable
Agent-connect has been broken in prod for every user, and the admin surface had
no way to fix it. This closes both halves.
Migration 176 — repair the row.
The PSD OpenClaw client (7e8646f4-4091-4a34-a6b9-0d3721e8a126) carried a
dev-only `http://localhost:3000/agent-connect-aistudio/callback` alongside two
correct production URIs. Its application_type is `native`, and
lib/oauth/redirect-uri-policy.ts 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 section 7.3). Redirect validation in
DrizzleOidcAdapter is all-or-nothing, so that single entry made loadClient()
return undefined and oidc-provider answer `invalid_client`; the two valid URIs
never got a chance. Prod errors 2026-08-10 19:35:28 / 19:36:25 / 19:39:43 UTC
are one user's three attempts, but the client was dead for everyone.
The row was also revoked at 23:18 UTC — reasonably, since revoke was the only
control the admin UI offered for a client that plainly needed changing. The
migration restores is_active and corrects the URI to 127.0.0.1, guarded on the
exact current redirect array and the public-PKCE native profile so an
administrator-modified or unrelated client is never overwritten.
Verified against a throwaway postgres before committing: applies once
(UPDATE 1), re-runs as a clean no-op (UPDATE 0), and the resulting three-URI
array passes validateOAuthRedirectUris("native", …) with VALID: true, ERRORS: [].
Also verified the client had 0 refresh tokens, 0 access tokens and 0
authorization codes, so nothing was ever issued against it and reactivating
grants nothing retroactively.
updateOAuthClientRedirectUris — remove the dead end.
The admin actions were create/list/revoke only, and the table's actions column
rendered nothing at all once isActive went false. A client revoked *because*
its redirect list was wrong was therefore unrecoverable without a migration and
a deploy, which is exactly the situation this migration exists to undo.
The new action is deliberately narrow: redirect URIs and active state, nothing
else. applicationType, tokenEndpointAuthMethod, requirePkce, the secret and the
scopes define the client's security profile, and changing those on a live
registration silently alters what an already-issued grant means — those still
require registering a new client. New URIs are validated against the STORED
application type, never a caller-supplied one, so a native client cannot be
relaxed into web rules by passing a type alongside the URIs. A zero-row update
fails rather than reporting a false success, matching revokeOAuthClient
(REV-COR-055).
The dialog stays available on a revoked client and offers reactivation in the
same save. Validation failures are surfaced verbatim to the admin: they typed
the URI and cannot correct it from a generic error. That is the opposite of the
adapter's LOG path, which redacts — the difference is that this text goes to an
authenticated administrator looking at their own input, not to CloudWatch.
Tests: 5 action cases (admin-role required before any query, the real localhost
repair with reactivation, a rejected native-forbidden URI that must not reach
the UPDATE, validation against the stored type while ignoring a caller-supplied
one, and no-false-success on zero rows) and 4 dialog cases (present and
pre-checked on a revoked client, corrected list plus reactivation in one call,
verbatim error display, blank-line trimming). Also mocked the new dialog in
oauth-clients-admin-trust.test.tsx, which renders the page client and does not
mock @/components/ui/dialog — same treatment it already gives ClientFormSheet.
112 tests pass across the OAuth suites; eslint clean; typecheck clean.
ReviewReviewed the diff ( SummarySolid, well-scoped fix. The narrow "redirect URIs + active state only" surface, validating against the stored Minor / non-blocking
SecurityNo concerns. Admin-role gate is present, redirect URI validation is applied server-side against the trusted stored value (not a client-supplied Test coverageGood — the 5 action tests and 4 dialog tests cover the meaningful branches (auth gate, the actual repair scenario, native-forbidden URI rejection, stored-vs-supplied-type isolation, zero-row failure, revoked-client availability, reactivation, verbatim errors, blank-line trimming). Nothing major missing. 🤖 Generated with Claude Code |
![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: 2c6606f0c4
ℹ️ 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".
| {/* Editing stays available on a REVOKED client. This column used to | ||
| render nothing once isActive went false, which left a client that | ||
| was revoked because its redirect list was wrong with no way back. */} | ||
| <EditRedirectUrisDialog client={client} onSaved={onSaved} /> |
There was a problem hiding this comment.
This introduces a new admin-facing edit/reactivation workflow, but the commit adds only unit tests with the dialog or server-action dependencies mocked and adds no spec under tests/e2e. An authenticated Playwright test should exercise opening this control for a revoked client, submitting the real server action, and observing the refreshed status so role enforcement, action serialization, dialog wiring, and reactivation are covered together.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
|
|
||
| UPDATE oauth_clients | ||
| SET | ||
| redirect_uris = '["http://127.0.0.1:3000/agent-connect-aistudio/callback", "https://dev.aistudio.psd401.ai/agent-connect-aistudio/callback", "https://aistudio.psd401.ai/agent-connect-aistudio/callback"]'::jsonb, |
There was a problem hiding this comment.
After a local db:reset, migration 139 inserts this client and migration 176 changes its only local callback to http://127.0.0.1:3000/..., while .env.example:2 still sets AUTH_URL=http://localhost:3000 and actions/agent-aistudio.actions.ts:22-23 constructs the authorization and token-exchange redirect_uri from that issuer. OAuth redirect matching is exact, so the supported local agent-connect flow will now submit the unregistered localhost URI and be rejected; either generate the loopback-literal URI locally or align the local issuer configuration.
AGENTS.md reference: AGENTS.md:L191-L193
Useful? React with 👍 / 👎.
| throw ErrorFactories.invalidInput( | ||
| "redirectUris", | ||
| validated.redirectUris, | ||
| uriValidation.errors.join("; ") | ||
| ) |
There was a problem hiding this comment.
When an administrator submits an invalid URI, ErrorFactories.invalidInput stores this joined validator text only as the field's constraint; its userMessage is always "The provided input is invalid", and handleError returns that generic value as result.message. The dialog reads only result.message, so the real server action never provides the offending URI or reason that the new UI says it will show, leaving administrators unable to diagnose failures such as the localhost case; set a safe explicit userMessage or return the validation errors in the action contract.
Useful? React with 👍 / 👎.
dev took 176 for the OpenClaw OAuth client redirect repair (#1629) while this branch was open. Renumbers the migration file and every reference to it so the numbering stays collision-free and migrationFiles can hold both.
Agent-connect has been broken in prod for every user, and the admin surface had no way to fix it. This closes both halves.
Migration 176 — repair the row
The
PSD OpenClawclient (7e8646f4…) carried a dev-onlyhttp://localhost:3000/agent-connect-aistudio/callbackalongside two correct production URIs. Itsapplication_typeisnative, and the policy rejects an HTTP redirect whose host isn't a literal loopback address —LOOPBACK_HOSTSis{"127.0.0.1", "[::1]"}, andlocalhostisn't in it (RFC 8252 §7.3).Redirect validation is all-or-nothing, so that single entry made
loadClient()returnundefinedandoidc-provideranswerinvalid_client. The two valid URIs never got a chance. The prod errors at19:35:28 / 19:36:25 / 19:39:43UTC are one user's three attempts — but the client was dead for everyone.The row was also revoked at 23:18 UTC — reasonably, since revoke was the only control the UI offered for a client that plainly needed changing. The migration restores
is_activeand corrects the URI, guarded on the exact current redirect array and the public-PKCE native profile so an administrator-modified or unrelated client is never overwritten.Verified before committing, against a throwaway postgres:
UPDATE 1UPDATE 0VALID: true, ERRORS: []updateOAuthClientRedirectUris— remove the dead endAdmin actions were create/list/revoke only, and the actions column rendered nothing once
isActivewent false. A client revoked because its redirect list was wrong was unrecoverable without a migration and a deploy — exactly the situation migration 176 exists to undo.The action is deliberately narrow: redirect URIs and active state, nothing else.
applicationType,tokenEndpointAuthMethod,requirePkce, the secret and the scopes define the security profile, and changing those on a live registration silently alters what an already-issued grant means — those still require a new client.New URIs are validated against the stored application type, never a caller-supplied one, so a native client can't be relaxed into web rules by passing a type alongside the URIs. A zero-row update fails rather than reporting a false success, matching
revokeOAuthClient(REV-COR-055).The dialog stays available on a revoked client and offers reactivation in the same save. Validation failures are shown verbatim — the admin typed the URI and can't correct it from a generic error. That's the opposite of the adapter's log path (#1628), which redacts: this text goes to an authenticated administrator looking at their own input, not to CloudWatch.
Tests
Action (5): admin role required before any query · the real localhost repair with reactivation · a native-forbidden URI that must not reach the UPDATE · validation against the stored type while ignoring a caller-supplied one · no-false-success on zero rows
Dialog (4): present and pre-checked on a revoked client · corrected list + reactivation in one call · verbatim error display · blank-line trimming
Also mocked the new dialog in
oauth-clients-admin-trust.test.tsx, which renders the page client and doesn't mock@/components/ui/dialog— the same treatment it already givesClientFormSheet.112 tests pass across the OAuth suites; eslint clean; typecheck clean.
After deploy
The migration fixes the row automatically. No manual admin step needed — and if a redirect URI ever goes wrong again, it's now editable in the UI without a deploy.