[codex] fix OAuth redirect contract#1393
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a CLI auth confirmation handler and updates OAuth redirect flows: helpers now return URLs, client redirects use a configurable redirectMethod via new internals, signInWithOAuth supports an optional returnTo routed through the OAuth callback, and related tests/docs updated. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as Browser Client
participant AppInternals as Redirect Manager
participant AuthProvider as OAuth Provider
participant Callback as OAuth Callback Handler
User->>Client: signInWithOAuth(provider, { returnTo? })
Client->>Client: getNewOAuthProviderOrScopeUrl(provider, afterCallbackRedirectUrl)
Client-->>AppInternals: _redirectTo(oauthAuthorizeUrl)
AppInternals->>AppInternals: getRedirectMethod()
alt redirectMethod == "window"
AppInternals->>Client: window.location.assign(oauthAuthorizeUrl)
else redirectMethod == "navigate"
AppInternals->>Client: useNavigate(oauthAuthorizeUrl)
else redirectMethod == "none"
AppInternals-->>Client: queue redirectTarget (render fallback)
end
Client->>AuthProvider: User authorizes (browser flow)
AuthProvider->>Callback: Redirect back to oauthCallback
Callback->>Callback: Exchange code -> token
Callback->>AppInternals: _redirectTo(afterCallbackRedirectUrl)
AppInternals->>Client: Navigate to return destination (per redirectMethod)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
Contributor
Greptile SummaryThis PR fixes the OAuth redirect contract by decoupling URL construction from navigation: Confidence Score: 5/5Safe to merge; only P2 findings, no runtime breakage identified. No P0 or P1 issues found. The two P2 findings (unvalidated returnTo URL and silent second-redirect drop) are edge cases that don't affect the primary changed paths in normal use. The core fix is structurally sound and well-tested. packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts (returnTo validation) and packages/template/src/components-page/stack-handler-client.tsx (redirectTargets priority)
|
| Filename | Overview |
|---|---|
| packages/template/src/lib/auth.ts | Renamed addNewOAuthProviderOrScope → getNewOAuthProviderOrScopeUrl; navigation removed and delegated to callers. |
| packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts | All OAuth redirect sites now use _redirectTo(); returnTo correctly becomes afterCallbackRedirectUrl; P2: returnTo lacks trusted-domain validation. |
| packages/template/src/components-page/stack-handler-client.tsx | React platform queues redirects into a local array; navigate called via useRef+useEffect; 'none' method gets a fallback MessageCard. |
| packages/template/src/components-page/oauth-callback.tsx | Error redirects now go through redirectToUrl; 'none' redirect method shows a fallback link via redirectUrl state. |
| apps/e2e/tests/js/oauth.test.ts | New e2e test verifies signInWithOAuth never settles after custom navigate fires; 5000ms sentinel gives comfortable margin. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[signInWithOAuth called] --> B[getNewOAuthProviderOrScopeUrl]
B --> C[getOAuthUrl via Stack API]
C --> D[Returns authorization URL]
D --> E{redirectMethod?}
E -->|window| F[window.location.assign]
E -->|custom object| G[navigate fn called]
E -->|nextjs| H[router.push]
E -->|none| I[returns immediately]
F --> J[neverResolve - stays pending]
G --> J
H --> J
I --> J
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts
Line: 2801-2807
Comment:
**`returnTo` not validated against trusted domains**
`constructRedirectUrl` resolves the URL but does not check whether it is same-origin or otherwise trusted. An absolute `returnTo` such as `"https://evil.com"` passes through unchanged and is forwarded to the server as `afterCallbackRedirectUrl`, which is later used for the post-OAuth redirect. Adding a trusted-origin check (similar to `_isTrusted`) here would provide defence-in-depth and improve debugging of misconfigured values, consistent with the team's validation policy for redirect URLs.
**Rule Used:** Always validate redirect URLs for security reasons... ([source](https://app.greptile.com/review/custom-context?memory=45e85dee-0c65-4d46-911a-297e806c23cd))
**Learned From**
[stack-auth/stack-auth#923](https://github.com/stack-auth/stack-auth/pull/923)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/template/src/components-page/stack-handler-client.tsx
Line: 240
Comment:
**`redirectTargets` push during render may be ignored when multiple redirects occur**
`redirectTargets` is a plain array initialised to `[]` on every render. Both `redirectIfNotHandler` (called inside `renderComponent`) and the `result.redirect` branch can push an entry, but only index `0` is ever consumed. In the old code `result.redirect` overwrote `window.location.href` last, so it "won"; with the new code the `redirectIfNotHandler` entry is always index `0` and the `result.redirect` entry is silently dropped. If both code paths can genuinely fire on the same render, the second redirect is lost without any error. Consider asserting that `redirectTargets.length <= 1` after rendering to surface this early.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "Enhance OAuth callback handling and redi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the browser OAuth flow in the JS template SDK to consistently use the configured redirectMethod (instead of direct window.location navigation), and clarifies the redirect contract so the OAuth callback URL remains stable while returnTo controls post-callback destination.
Changes:
- Route browser OAuth navigation through
redirectMethod, and keep redirecting APIs pending once navigation begins. - Add
cliAuthConfirmto handler URL metadata and custom-page prompt guidance. - Update SDK spec wording and add/extend tests to cover the new redirect contract and non-resolving behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdks/spec/src/apps/client-app.spec.md | Documents returnTo vs callback URL behavior and redirectMethod-based navigation in browser OAuth flows. |
| packages/template/src/lib/stack-app/url-targets.ts | Adds cliAuthConfirm hosted-path mapping + URL resolution support. |
| packages/template/src/lib/stack-app/apps/implementations/client-app-impl.ts | Uses URL-returning OAuth helper + _redirectTo instead of hardcoded redirects; adds redirectToCliAuthConfirm; fixes returnTo contract in OAuth authorize call. |
| packages/template/src/lib/auth.ts | Replaces “redirecting helper” with getNewOAuthProviderOrScopeUrl that returns the URL without navigating. |
| packages/template/src/lib/auth.test.ts | Adds unit coverage ensuring OAuth URL construction does not perform navigation. |
| packages/template/src/components-page/stack-handler-client.tsx | Routes handler-page redirects through useNavigate (React platform) instead of direct window.location writes. |
| packages/template/src/components-page/oauth-callback.tsx | Routes error redirects through configured navigation instead of window.location.replace. |
| packages/stack-shared/src/interface/page-component-versions.ts | Adds cliAuthConfirm custom-page prompt metadata and example. |
| packages/stack-shared/src/interface/handler-urls.ts | Extends handler URL type to include cliAuthConfirm. |
| apps/e2e/tests/js/oauth.test.ts | Adjusts/extends e2e coverage for redirectMethod behavior and “promise stays pending after navigation starts”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated the OAuthCallback component to manage redirects more effectively by introducing a new `redirectToError` function, which handles error redirects based on the current redirect method. - Adjusted the timeout duration in the OAuth test to ensure proper handling of asynchronous operations. - Improved the StackHandlerClient to utilize a reference for navigation, allowing for more reliable redirect handling based on the current redirect method. These changes aim to streamline the OAuth flow and improve error handling during redirects.
BilalG1
approved these changes
Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
redirectMethodinstead of hardcodedwindow.locationcalls.cliAuthConfirmhandler URL metadata and custom-page prompt coverage.returnTobehavior.Root Cause
OAuth helpers previously combined URL construction with direct browser navigation. That bypassed configured redirect methods and made it too easy for public redirect APIs to resolve after navigation started.
Impact
Browser SDK consumers get consistent redirect behavior across built-in and custom navigation methods.
returnTois handled as the post-callback destination while the OAuth callback URL remains fixed to the configured handler route.Validation
pnpm test run packages/template/src/lib/auth.test.tspnpm test run apps/e2e/tests/js/oauth.test.tspnpm -C packages/template lintpnpm -C apps/e2e lintpnpm -C packages/template typecheckpnpm -C apps/e2e typecheckSummary by CodeRabbit
New Features
Bug Fixes
Tests