Skip to content

fix(runtime)!: enforce registered redirect_uri on /authorize - #6330

Merged
viktormarinho merged 1 commit into
mainfrom
t3code/fix-oauth-redirect-validation
Aug 20, 2026
Merged

fix(runtime)!: enforce registered redirect_uri on /authorize#6330
viktormarinho merged 1 commit into
mainfrom
t3code/fix-oauth-redirect-validation

Conversation

@viktormarinho

@viktormarinho viktormarinho commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

What is this contribution about?

The OAuth authorization endpoint in @decocms/runtime accepted any redirect_uri that parsed as https, without comparing it to the requesting client.

Registered redirect URIs were write-only. handleClientRegistration persisted them through saveClient, but getClient had no callers anywhere in the package, so what a client declared at registration never constrained anything. The request's client_id was read and then discarded. That is an open redirect on the authorization endpoint, and this runtime encodes the upstream provider token into the authorization code, so the consequence is worse than a normal code leak. PKCE does not help, because whoever builds the /authorize URL also chooses the challenge.

The fix adds one gate, checkRedirectUri, called from both /authorize and /oauth/callback:

  • client_id is required, resolved through persistence.getClient, and redirect_uri must match one of that client's registered URIs by exact string comparison (RFC 6749 3.1.2.2). The only relaxation is a variable port on loopback URIs for native clients (RFC 8252 7.3).
  • A new optional allowedRedirectHosts applies in addition, matched on a label boundary so a suffix cannot be spoofed by a longer host. http is accepted only for loopback. This is what a stateless deployment uses, where there is no client store to resolve.
  • With neither configured, the gate rejects everything. The previous effective policy, any https URL, is not a policy.
  • Rejections render a 400 instead of redirecting to the URI under scrutiny (RFC 6749 4.1.2.1).

The callback runs the same gate on the redirect URI it decodes from the state, before any of its three redirect paths. state was unsigned and upstream providers permit extra query params on their registered callback, so a state this server never issued could otherwise reach /oauth/callback and steer a real token off-origin. Fixing /authorize alone would not have closed that.

Also adds an optional stateSecret. When set, the state and the authorization code are AES-GCM sealed. The code previously carried the upstream access and refresh tokens as plain base64url JSON, so a leaked code was a leaked token rather than something PKCE and the token endpoint could still contain.

How did you verify your code works?

40 tests pass across packages/runtime/src/oauth.test.ts and the new packages/runtime/src/redirect-uri.test.ts.

New coverage in oauth.test.ts: unregistered redirect_uri rejected with no upstream redirect; missing client_id rejected; unknown client_id rejected as invalid_client; registered URI differing only by a trailing slash or an extra query param rejected; exact match accepted; loopback port relaxation accepted while a differing path is not; every redirect_uri rejected when neither policy is configured; the allowedRedirectHosts suffix cases including the two near-miss hosts and http on an allowed host; a forged state at /oauth/callback returning 400 with no location header on both the success and the upstream-error path; and the stateSecret round trip, including that the sealed code does not contain the upstream token and that a plaintext state is refused once a secret is configured.

redirect-uri.test.ts covers the matchers directly, including path traversal and embedded-URL shapes.

I inverted the existing test that asserted the old behaviour (accepts an https redirect_uri and redirects to the upstream authorization URL) rather than appending a new one, since it encoded exactly this bug.

bun run --cwd packages/runtime check, bun run fmt:check, bun run lint and knip are clean. Note that bun test packages/runtime has a pre-existing failure in triggers.test.ts when the whole package runs at once; it fails identically on a stashed tree and passes when that file runs alone.

How to Test

  1. bun test packages/runtime/src/oauth.test.ts packages/runtime/src/redirect-uri.test.ts
  2. Configure a server with oauth.persistence, register a client through /register, then call /authorize with a redirect_uri that is not the registered one.
  3. Expected: 400 with invalid_request and no location header, instead of a 302 to the upstream provider.

Migration Notes

This is a breaking change for consumers of @decocms/runtime, which is why the title carries !. It bumps only packages/runtime/package.json; the Studio app release line is untouched (scripts/release-changes.ts returns packageManifests: ["packages/runtime/package.json"] for this diff).

Nothing in this repo configures oauth, so there is no internal impact. Every downstream consumer is pinned to runtime 1.x, so nothing picks this up automatically.

Consumers need, in the same deploy that takes the new version:

  • Stateless servers must set oauth.allowedRedirectHosts, or /authorize rejects every request by design.
  • Servers with persistence need no config, but should audit stored redirect_uris first, since matching is now exact and anything registered with a trailing slash or stray query param will start failing.
  • In-flight authorizations do not survive the upgrade. A state issued by the previous version carries no client_id, so the new callback gate rejects it. Users retry and it works.
  • stateSecret is opt-in and should be a separate, later deploy. Mid-rollout, an instance holding the secret cannot unseal a state written by one without it.

Follow-up in decocms/mcps: pass the existing ALLOWED_REDIRECT_HOST_SUFFIXES into the runtime config and delete github/server/lib/redirect-allowlist.ts, whose assertAllowedRedirectUri() currently only ever sees the server's own origin and so cannot reject anything.

Review Checklist

  • PR title is clear and descriptive
  • Changes are tested and working
  • Documentation is updated (if needed)
  • No breaking changes

Summary by cubic

Closes an open redirect in @decocms/runtime OAuth by enforcing exact redirect_uri validation. Previously /authorize accepted any https URL; now it requires client_id and only redirects to a URI registered for that client, preventing token leakage.

  • Implements checkRedirectUri used in both /authorize and /oauth/callback.
  • Exact match against the client’s registered URIs (RFC 6749 §3.1.2.2); only relaxation is varying the port on loopback URIs for native apps (RFC 8252 §7.3).
  • Adds optional allowedRedirectHosts as an additional allowlist (label-boundary match). http is only allowed for loopback. With neither persistence nor allowedRedirectHosts, all requests are rejected. Rejections return 400 instead of redirecting (RFC 6749 §4.1.2.1).
  • Validates the decoded redirect_uri at /oauth/callback to block forged state from steering tokens off-origin.
  • Adds optional stateSecret to AES-GCM seal state and our authorization code, removing upstream tokens from plaintext codes and making state tamper-evident.

Migration

  • Breaking: /authorize now requires client_id and a validated redirect_uri.
  • Stateless servers must set oauth.allowedRedirectHosts, or every request is rejected.
  • Servers with persistence should audit stored redirect_uris for exact matches; trailing slashes or extra query params will now fail.
  • In-flight authorizations from the old version will fail; users must retry.
  • stateSecret is optional; roll out separately. Mixed instances (with/without it) cannot read each other’s state.

Written for commit 29443ce. Summary will update on new commits.

Review in cubic

The OAuth authorization endpoint accepted any redirect_uri that parsed as
https, without comparing it to the requesting client. Registered redirect
URIs were write-only: handleClientRegistration persisted them via
saveClient, but getClient had no callers anywhere in the package, so the
values a client declared at registration never constrained anything. The
request's client_id was read and discarded.

Add a single gate, checkRedirectUri, used by both /authorize and
/oauth/callback:

- client_id is required, resolved through persistence.getClient, and the
  redirect_uri must match one of that client's registered URIs by exact
  string comparison (RFC 6749 3.1.2.2). The only relaxation is a variable
  port on loopback URIs for native clients (RFC 8252 7.3).
- A new optional allowedRedirectHosts config applies in addition, matching
  on a label boundary so a suffix cannot be spoofed by a longer host. http
  is accepted only for loopback. This is what a stateless deployment uses,
  where there is no client store to resolve.
- With neither configured the gate rejects everything. The previous
  effective policy, any https URL, is not a policy.
- Rejections render a 400 rather than redirecting to the URI under
  scrutiny (RFC 6749 4.1.2.1).

The callback runs the same gate on the redirect URI it decodes from the
state, before any of its three redirect paths. state was unsigned, and the
upstream provider permits extra query params on its registered callback, so
a state this server never issued could otherwise arrive at /oauth/callback
and steer a real token off-origin. Fixing /authorize alone would not have
closed that.

Also add an optional stateSecret. When set, the state and the authorization
code are AES-GCM sealed. The code previously carried the upstream provider's
access and refresh tokens as plain base64url JSON, so a leaked code was a
leaked token rather than something PKCE and the token endpoint could still
contain.

BREAKING CHANGE: /authorize now requires client_id and rejects every
redirect_uri unless the server configures oauth.persistence or
oauth.allowedRedirectHosts. Stateless deployments must set
allowedRedirectHosts in the same deploy that picks up this version.
In-flight authorizations do not survive the upgrade, since a state issued by
the previous version carries no client_id.
@viktormarinho
viktormarinho merged commit 939a0f9 into main Aug 20, 2026
34 checks passed
@viktormarinho
viktormarinho deleted the t3code/fix-oauth-redirect-validation branch August 20, 2026 14:28
decocms Bot pushed a commit that referenced this pull request Aug 20, 2026
PR: #6330 fix(runtime)!: enforce registered redirect_uri on /authorize
Bump type: major

- @decocms/runtime (packages/runtime/package.json): 2.4.2 -> 3.0.0

Deploy-Scope: both
pedrofrxncx added a commit that referenced this pull request Aug 20, 2026
…tration (#6351)

isValidRedirectUri (used by both /authorize and dynamic client registration)
only special-cased 127.0.0.1/localhost/.localhost as non-https loopback
exceptions, while the sibling isLoopbackHost() in redirect-uri.ts (added by
#6330 for the /authorize matching path) also covers [::1]/::1 per RFC 8252
§7.3. A native client binding an ephemeral IPv6 loopback port would get its
redirect_uri rejected at DCR (400 invalid_redirect_uri) even though the same
URI would be accepted as a match against a registered client.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant