Fix extension stuck on login screen after sign-in#264
Merged
Conversation
After clicking "Sign in" on the extension auth page, the OAuth callback always redirected to "/" instead of back to /extension-auth. This meant the content script never ran on the auth page post-login, so the JWT token was never relayed to the extension — leaving it stuck on the login screen. Store a returnTo parameter in the session during /api/login and use it in the OAuth callback redirect. Only relative paths are accepted to prevent open-redirect attacks. https://claude.ai/code/session_011668oYNRLot8LEzJF1wqjA
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughClient adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppServer
participant SessionStore
participant OIDCProvider
Client->>AppServer: GET /api/login?returnTo=/extension-auth
AppServer->>AppServer: sanitizeReturnTo(req.query.returnTo)
AppServer->>SessionStore: store session.returnTo (and save)
AppServer->>OIDCProvider: redirect to OIDC auth (Passport)
OIDCProvider-->>Client: OIDC consent -> redirect to /api/callback
Client->>AppServer: GET /api/callback (auth code)
AppServer->>OIDCProvider: exchange code, verify user
AppServer->>SessionStore: persist authenticated session
AppServer->>SessionStore: read & delete session.returnTo
AppServer-->>Client: redirect to session.returnTo || /
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/pages/ExtensionAuth.tsx (2)
38-38:⚠️ Potential issue | 🟡 MinorMissing
errorin useEffect dependency array.The effect checks
errorin its guard condition (line 12) buterroris not included in the dependency array. Iferroris set and then cleared externally, the effect won't re-run to retry the token fetch.🛠️ Proposed fix
- }, [user, tokenSent]); + }, [user, tokenSent, error]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/ExtensionAuth.tsx` at line 38, The useEffect in ExtensionAuth.tsx references the `error` variable in its guard but the dependency array only contains `[user, tokenSent]`; update the effect's dependency array to include `error` (i.e., `[user, tokenSent, error]`) so the effect re-runs when error changes and retries the token fetch logic (the effect that checks `error`, `user`, and `tokenSent` and invokes the token fetch/handler).
61-61: 🧹 Nitpick | 🔵 TrivialUse semantic color token instead of hardcoded emerald.
Per coding guidelines, use semantic Tailwind color tokens. Consider using
text-successor similar semantic token if defined, ortext-primaryfor consistency.♻️ Proposed change
- <CheckCircle2 className="h-12 w-12 text-emerald-500 dark:text-emerald-400 mx-auto" /> + <CheckCircle2 className="h-12 w-12 text-primary mx-auto" />As per coding guidelines: "Use semantic Tailwind color tokens (text-primary, text-muted-foreground, bg-secondary, bg-background, border-border, etc.) instead of hardcoded hex, RGB, or HSL values in JSX or class names"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/ExtensionAuth.tsx` at line 61, The CheckCircle2 icon in ExtensionAuth is using hardcoded color classes (text-emerald-500 and dark:text-emerald-400); update its className to use the project's semantic Tailwind tokens (for example replace those with text-success and dark:text-success-foreground or text-primary and dark:text-primary-foreground depending on the token defined) so the icon follows semantic theming; modify the className on the CheckCircle2 element accordingly (keeping other utility classes like sizing and mx-auto intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/replit_integrations/auth/replitAuth.ts`:
- Around line 118-122: The returnTo assignment allows bypass via backslashes
(e.g. "/\\evil.com" or "/%5Cevil.com"); add a validation helper (e.g.
isValidReturnTo) and use it when setting returnTo to ensure the value is a
string that starts with "/" but not "//", contains no backslash characters (\"),
and contains no percent-encoded backslashes ("%5C" or "%5c"); optionally
decodeURIComponent first and/or validate with URL parsing against a fixed base
to ensure the path stays local. Apply this helper where returnTo is computed to
replace the current inline checks.
---
Outside diff comments:
In `@client/src/pages/ExtensionAuth.tsx`:
- Line 38: The useEffect in ExtensionAuth.tsx references the `error` variable in
its guard but the dependency array only contains `[user, tokenSent]`; update the
effect's dependency array to include `error` (i.e., `[user, tokenSent, error]`)
so the effect re-runs when error changes and retries the token fetch logic (the
effect that checks `error`, `user`, and `tokenSent` and invokes the token
fetch/handler).
- Line 61: The CheckCircle2 icon in ExtensionAuth is using hardcoded color
classes (text-emerald-500 and dark:text-emerald-400); update its className to
use the project's semantic Tailwind tokens (for example replace those with
text-success and dark:text-success-foreground or text-primary and
dark:text-primary-foreground depending on the token defined) so the icon follows
semantic theming; modify the className on the CheckCircle2 element accordingly
(keeping other utility classes like sizing and mx-auto intact).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0df0ed01-a784-4055-be2c-454d8ca2db5f
📒 Files selected for processing (2)
client/src/pages/ExtensionAuth.tsxserver/replit_integrations/auth/replitAuth.ts
Extract sanitizeReturnTo() as a testable pure function with defense- in-depth against CRLF injection (literal and percent-encoded) and unbounded input length. Add 16 test cases covering open redirect, CRLF, length limits, and type coercion edge cases. https://claude.ai/code/session_011668oYNRLot8LEzJF1wqjA
- Broaden control character rejection from \r\n to all \x00-\x1f (including null bytes and tabs) and their percent-encoded forms - Add explicit req.session.save() before OAuth redirect to ensure returnTo persists to the session store across the identity provider round-trip - Add test cases for null byte and tab character injection https://claude.ai/code/session_011668oYNRLot8LEzJF1wqjA
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/replit_integrations/auth/replitAuth.ts (1)
16-23:⚠️ Potential issue | 🔴 CriticalCritical open-redirect bypass remains via backslash variants.
sanitizeReturnTostill allows payloads like/\evil.com(and encoded%5Cvariants), which can be normalized into protocol-relative redirects by some clients. This keeps an open-redirect path alive.🔒 Suggested hardening
export function sanitizeReturnTo(value: unknown): string | undefined { if (typeof value !== "string") return undefined; if (value.length > 2048) return undefined; if (!value.startsWith("/")) return undefined; if (value.startsWith("//")) return undefined; + if (value.includes("\\") || /%5c/i.test(value)) return undefined; if (/[\x00-\x1f]/.test(value) || /%[01][0-9a-f]/i.test(value)) return undefined; return value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/replit_integrations/auth/replitAuth.ts` around lines 16 - 23, sanitizeReturnTo currently permits backslash variants (e.g., "/\evil.com" or "%5C") that can be normalized into protocol-relative redirects; update sanitizeReturnTo to reject any value containing backslash characters or their percent-encoded forms by adding checks for "\\" and for percent-encoded backslash sequences (case-insensitive "%5C"), and return undefined if either is present so that sanitizeReturnTo reliably blocks these open-redirect vectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/replit_integrations/auth/replitAuth.test.ts`:
- Around line 85-87: The current test for sanitizeReturnTo only checks a leading
backslash variant; add explicit cases for the exploitable normalized shapes by
asserting sanitizeReturnTo("/\\evil.com") and sanitizeReturnTo("/%5Cevil.com")
also return undefined so both the raw backslash and its percent-encoded form are
rejected; update the test block in replitAuth.test.ts (the "rejects
backslash-prefixed paths" it) to include these two expect(...) assertions
referencing sanitizeReturnTo.
In `@server/replit_integrations/auth/replitAuth.ts`:
- Around line 130-133: The code sets (req.session as any).returnTo only when
sanitizeReturnTo(req.query.returnTo) returns a value but never clears a
previously stored value; update the logic in replitAuth.ts so that after calling
sanitizeReturnTo(req.query.returnTo) you explicitly remove or clear (req.session
as any).returnTo (e.g., delete or set undefined) when sanitizeReturnTo returns
falsy, ensuring stale returnTo values are not reused across logins while
continuing to set it when valid; reference the sanitizeReturnTo call and
(req.session as any).returnTo in your change.
---
Duplicate comments:
In `@server/replit_integrations/auth/replitAuth.ts`:
- Around line 16-23: sanitizeReturnTo currently permits backslash variants
(e.g., "/\evil.com" or "%5C") that can be normalized into protocol-relative
redirects; update sanitizeReturnTo to reject any value containing backslash
characters or their percent-encoded forms by adding checks for "\\" and for
percent-encoded backslash sequences (case-insensitive "%5C"), and return
undefined if either is present so that sanitizeReturnTo reliably blocks these
open-redirect vectors.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3daa5c72-d0ab-4473-89e8-78810372a522
📒 Files selected for processing (2)
server/replit_integrations/auth/replitAuth.test.tsserver/replit_integrations/auth/replitAuth.ts
Address CodeRabbit review comments: - Reject backslash characters and %5C in sanitizeReturnTo to prevent browser normalization of /\evil.com into //evil.com - Clear stale returnTo from session when login has no/invalid value - Add test cases for /\evil.com, /%5Cevil.com, /%5cevil.com https://claude.ai/code/session_011668oYNRLot8LEzJF1wqjA
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
2 participants
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
/instead of back to/extension-auth. The content script never ran post-login, so the JWT token was never relayed to the extension — leaving it stuck on the "Connect your account" screen.returnToquery parameter in the session during/api/loginand use it in the OAuth callback redirect. The ExtensionAuth page now passesreturnTo=/extension-authwhen linking to login.sanitizeReturnTo()with defense-in-depth against open redirects (protocol-relative, absolute URLs), CRLF injection (literal + percent-encoded), null bytes, all control characters (\x00-\x1f), and unbounded input length (2048 cap). 18 test cases.req.session.save()before OAuth redirect ensuresreturnTopersists to the session store across the identity provider round-trip.Test plan
/extension-authafter OAuth (not/)/returnTowith absolute URLs (e.g.https://evil.com) is rejectedreturnTowith protocol-relative URLs (e.g.//evil.com) is rejectedreturnTowith CRLF injection attempts is rejectedreturnTolonger than 2048 characters is rejectedhttps://claude.ai/code/session_011668oYNRLot8LEzJF1wqjA
Summary by CodeRabbit