chore(web): guard OAuth API routes against 307/308 redirects#1163
chore(web): guard OAuth API routes against 307/308 redirects#1163brendan-kellam merged 2 commits intomainfrom
Conversation
Adds `oauthApiHandler`, a thin wrapper around `apiHandler` that throws if the wrapped handler ever returns an HTTP 307 or 308 response. Per RFC 9700 §4.12, an OAuth authorization server must not use 307/308 on redirects that could carry user credentials, since those status codes preserve the request method and body. Wires `oauthApiHandler` into all five authorization-server route handlers — the token, register, and revoke endpoints under `/api/ee/oauth/*`, plus the two RFC 8414 / RFC 9728 discovery endpoints under `/api/ee/.well-known/*` — so any future change that accidentally introduces a 307/308 from these routes throws at request time rather than silently shipping. Includes unit tests verifying the wrapper passes through 200/302/303/400 responses unchanged and throws on 307 and 308. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
WalkthroughThis PR introduces a runtime guard for OAuth authorization-server route handlers that rejects HTTP 307 and 308 redirect responses in accordance with RFC 9700 §4.12. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 14 minutes and 4 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 23: The changelog entry "Guarded all OAuth authorization-server route
handlers with a runtime assertion that rejects HTTP 307 and 308 responses, per
RFC 9700 §4.12. [`#1163`](https://github.com/sourcebot-dev/sourcebot/pull/1163)"
is missing the enterprise-only prefix; update that line to prepend "[EE]" to the
entry (so it reads starting with "[EE] Guarded all OAuth authorization-server
route handlers...") to comply with the changelog rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70b33b5d-68e3-41db-b58e-c2a6a3a88fc1
📒 Files selected for processing (8)
CHANGELOG.mdpackages/web/src/app/api/(server)/ee/.well-known/oauth-authorization-server/route.tspackages/web/src/app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.tspackages/web/src/app/api/(server)/ee/oauth/register/route.tspackages/web/src/app/api/(server)/ee/oauth/revoke/route.tspackages/web/src/app/api/(server)/ee/oauth/token/route.tspackages/web/src/ee/features/oauth/apiHandler.test.tspackages/web/src/ee/features/oauth/apiHandler.ts
Fixes SOU-945
Summary
packages/web/src/ee/features/oauth/apiHandler.ts— a thin wrapper aroundapiHandlerthat throws if the wrapped handler ever returns HTTP 307 or 308. Per RFC 9700 §4.12, the OAuth authorization server must not use 307/308 on redirects carrying user credentials, since those statuses preserve the request method and body. The error message cites the spec.apiHandler→oauthApiHandlerin every authorization-server route handler:app/api/(server)/ee/oauth/token/route.tsapp/api/(server)/ee/oauth/register/route.tsapp/api/(server)/ee/oauth/revoke/route.tsapp/api/(server)/ee/.well-known/oauth-authorization-server/route.ts(RFC 8414)app/api/(server)/ee/.well-known/oauth-protected-resource/[...path]/route.ts(RFC 9728)This is defense-in-depth on top of the existing posture: no authorization-server endpoint emits 307 or 308 today, but this wrapper makes regressions fail at request time rather than ship silently.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests