feat: invalid domain warning#332
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRemoves the CLI flag Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Browser
participant Layout
participant DomainWarning
participant App
User->>Browser: Load app
Browser->>Layout: Render with appUrl/rootDomain
alt origin != configured appUrl AND no session ignore
Layout->>DomainWarning: Show warning (appUrl, currentUrl)
User-->>DomainWarning: Click "Ignore"
DomainWarning->>Layout: onClick() -> sessionStorage.setItem + update state
Layout->>App: Render content
else
Layout->>App: Render content
end
sequenceDiagram
autonumber
actor User
participant Browser
participant ContinuePage
User->>ContinuePage: Navigate with redirect_uri
ContinuePage->>ContinuePage: Parse redirect_uri (URL) & validate proto/host/rootDomain/https downgrade
alt valid & trusted
Note over ContinuePage: schedule auto-redirect (100ms)
ContinuePage->>Browser: window.location.replace(redirect) (auto)
else if untrusted or insecure
ContinuePage->>User: Render alert card (continueUntrusted/insecure)
User->>ContinuePage: Click proceed
ContinuePage->>Browser: window.location.assign(redirect) (manual)
end
Note over ContinuePage: reveal manual button after 1s if auto pending
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (6)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #332 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 32 32
Lines 2378 2380 +2
=====================================
- Misses 2378 2380 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/src/pages/continue-page.tsx (2)
20-28: Hooks called conditionally; restructure to top-level.useAppContext, useLocation, useState, useTranslation, useNavigate, and useEffect are placed after early returns, violating Hooks rules and triggering Biome errors. Move all Hooks to the top and gate side-effects logically.
Apply:
export const ContinuePage = () => { - const { isLoggedIn } = useUserContext(); - - if (!isLoggedIn) { - return <Navigate to="/login" />; - } - - const { rootDomain } = useAppContext(); - const { search } = useLocation(); - const [loading, setLoading] = useState(false); - const [showRedirectButton, setShowRedirectButton] = useState(false); + const { isLoggedIn } = useUserContext(); + const { rootDomain } = useAppContext(); + const { search } = useLocation(); + const [loading, setLoading] = useState(false); + const [showRedirectButton, setShowRedirectButton] = useState(false); + const { t } = useTranslation(); + const navigate = useNavigate(); - const { t } = useTranslation(); - const navigate = useNavigate(); + const searchParams = new URLSearchParams(search); + const redirectURI = searchParams.get("redirect_uri"); + const sanitizedRedirectURI = redirectURI ? DOMPurify.sanitize(redirectURI) : ""; + const isRedirectUriValid = !!redirectURI && isValidUrl(sanitizedRedirectURI); + const redirectURLObj = isRedirectUriValid ? new URL(sanitizedRedirectURI) : null; + const normalizeHost = (h: string) => h.toLowerCase().replace(/\.$/, ""); + const rd = normalizeHost(rootDomain ?? ""); + const host = redirectURLObj ? normalizeHost(redirectURLObj.hostname) : ""; + const isTrusted = + redirectURLObj ? host === rd || host.endsWith(`.${rd}`) : false; + const isHttpsDowngrade = + redirectURLObj + ? redirectURLObj.protocol === "http:" && window.location.protocol === "https:" + : false; - const handleRedirect = () => { + const handleRedirect = () => { setLoading(true); - window.location.href = DOMPurify.sanitize(redirectURI); + // replace() avoids polluting history with the interstitial page + window.location.replace(sanitizedRedirectURI); }; - const redirectURLObj = new URL(redirectURI); + // Auto-redirect only when safe and applicable + useEffect(() => { + if (!isLoggedIn || !isRedirectUriValid || !isTrusted || isHttpsDowngrade) { + return; + } + const t1 = setTimeout(() => { + handleRedirect(); + }, 100); + const t2 = setTimeout(() => { + setLoading(false); + setShowRedirectButton(true); + }, 1000); + return () => { + clearTimeout(t1); + clearTimeout(t2); + }; + }, [isLoggedIn, isRedirectUriValid, isTrusted, isHttpsDowngrade]); + // Early returns after hooks are declared + if (!isLoggedIn) { + return <Navigate to="/login" />; + } - const searchParams = new URLSearchParams(search); - const redirectURI = searchParams.get("redirect_uri"); - if (!redirectURI) { return <Navigate to="/logout" />; } - if (!isValidUrl(DOMPurify.sanitize(redirectURI))) { + if (!isRedirectUriValid) { return <Navigate to="/logout" />; }Also applies to: 40-41, 127-135
75-78: Button label key exists?These buttons use t("continueTitle") but that key was removed from locales in this PR. Either re-add the key (preferred, see locale comments) or swap to an existing key. Re-adding keeps copy concise (“Continue” vs “Redirect me manually”).
Also applies to: 112-114
🧹 Nitpick comments (9)
frontend/src/index.css (1)
158-160: Prefer break-words over break-all to avoid mid-token breaks in code.break-all can split URLs/tokens awkwardly. break-words wraps long strings without breaking every character.
-code { - @apply relative rounded bg-muted px-[0.2rem] py-[0.1rem] font-mono text-sm font-semibold break-all; -} +code { + @apply relative rounded bg-muted px-[0.2rem] py-[0.1rem] font-mono text-sm font-semibold break-words; +}frontend/src/components/ui/button.tsx (1)
24-26: Dark-mode warning button may lose contrast; consider restoring dark:bg.Without a dark:bg token, bg-amber-500 is used in dark too; white text may be borderline. Suggest adding dark:bg-amber-600 (and optional dark:hover).
- warning: - "bg-amber-500 text-white shadow-xs hover:bg-amber-400 focus-visible:ring-amber-200/20 dark:focus-visible:ring-amber-400/40", + warning: + "bg-amber-500 text-white shadow-xs hover:bg-amber-400 focus-visible:ring-amber-200/20 dark:focus-visible:ring-amber-400/40 dark:bg-amber-600 dark:hover:bg-amber-500",frontend/src/schemas/app-context-schema.ts (1)
7-8: Tighten validation: appUrl should be a URL; rootDomain should be a bare domain.Prevents malformed values from breaking the domain-warning gate.
- appUrl: z.string(), - rootDomain: z.string(), + appUrl: z.string().url(), + rootDomain: z + .string() + .transform((s) => s.trim().toLowerCase()) + .regex( + /^(?:[a-z0-9](?:[a-z0-9-]{0,61}[a-z0-9])?\.)+[a-z]{2,}$/i, + "rootDomain must be a domain like example.com", + ),internal/controller/context_controller.go (1)
35-45: Remove unusedDisableContinuefield fromContextControllerConfig.internal/controller/context_controller.go type ContextControllerConfig struct { - DisableContinue bool }internal/bootstrap/app_bootstrap.go (1)
47-56: Optional: sanity-check AppURL host vs derived domain at startup.Prevents misconfig where AppURL host doesn’t end with RootDomain.
// Get domain domain, err := utils.GetUpperDomain(app.Config.AppURL) if err != nil { return err } + if host, herr := utils.GetHost(app.Config.AppURL); herr == nil && !strings.HasSuffix(host, domain) { + return fmt.Errorf("app-url host %q does not match derived root domain %q", host, domain) + }frontend/src/pages/continue-page.tsx (1)
112-121: Severity style consistency.Consider variant="warning" (not "destructive") for the untrusted-redirect confirm to match the insecure-redirect confirm and convey caution rather than irreversible action.
- <Button - onClick={handleRedirect} - loading={loading} - variant="destructive" - > + <Button onClick={handleRedirect} loading={loading} variant="warning"> {t("continueTitle")} </Button>frontend/src/components/domain-warning/domain-warning.tsx (2)
22-22: Add alert semantics for accessibilityExpose the warning to assistive tech.
Apply this diff:
- <Card className="min-w-xs sm:min-w-sm"> + <Card role="alert" aria-live="assertive" className="min-w-xs sm:min-w-sm">
34-38: Offer a “Go to the correct domain” primary actionImproves UX by giving a safe path in addition to “ignore”.
Apply this diff (assumes i18n key goToCorrectDomainTitle exists):
<CardFooter className="flex flex-col items-stretch"> <Button onClick={onClick} variant="warning"> {t("ignoreTitle")} </Button> + <Button onClick={() => window.location.assign(appUrl)} className="mt-2"> + {t("goToCorrectDomainTitle")} + </Button> </CardFooter>frontend/src/components/layout/layout.tsx (1)
36-40: Consider matching by registrable root domain (eTLD+1) if subdomains are legitimateIf your deployment spans subdomains (e.g., auth.example.com vs. app.example.com), origin mismatch may be too strict.
Options:
- Use the provided rootDomain from context to allow any subdomain of that root.
- Otherwise, parse with URL and compare .hostname suffix against rootDomain.
- Keep the stricter origin check behind a feature flag if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
cmd/root.go(0 hunks)frontend/src/components/domain-warning/domain-warning.tsx(1 hunks)frontend/src/components/layout/layout.tsx(2 hunks)frontend/src/components/ui/button.tsx(1 hunks)frontend/src/index.css(1 hunks)frontend/src/lib/i18n/locales/en-US.json(2 hunks)frontend/src/lib/i18n/locales/en.json(2 hunks)frontend/src/pages/continue-page.tsx(5 hunks)frontend/src/schemas/app-context-schema.ts(1 hunks)internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(0 hunks)internal/controller/context_controller.go(4 hunks)
💤 Files with no reviewable changes (2)
- internal/config/config.go
- cmd/root.go
🧰 Additional context used
🧬 Code graph analysis (7)
frontend/src/components/domain-warning/domain-warning.tsx (1)
frontend/src/components/ui/card.tsx (10)
Card(85-85)CardHeader(86-86)CardTitle(88-88)CardDescription(90-90)CardFooter(87-87)CardTitle(31-39)CardDescription(41-49)Card(5-16)CardHeader(18-29)CardAction(51-62)
internal/bootstrap/app_bootstrap.go (4)
internal/config/config.go (1)
Config(17-56)internal/controller/user_controller.go (2)
Domain(24-26)Config(28-32)internal/controller/proxy_controller.go (1)
AppURL(20-22)internal/middleware/context_middleware.go (1)
Domain(14-16)
frontend/src/components/layout/layout.tsx (2)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/components/domain-warning/domain-warning.tsx (1)
DomainWarning(17-41)
frontend/src/pages/continue-page.tsx (2)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/pages/totp-page.tsx (1)
window(42-46)
frontend/src/lib/i18n/locales/en.json (1)
frontend/src/pages/totp-page.tsx (1)
window(42-46)
internal/controller/context_controller.go (3)
internal/config/config.go (1)
Config(17-56)internal/controller/oauth_controller.go (3)
Config(29-34)CSRFCookieName(21-27)controller(93-210)internal/controller/user_controller.go (2)
Config(28-32)Domain(24-26)
frontend/src/lib/i18n/locales/en-US.json (1)
frontend/src/pages/totp-page.tsx (2)
window(42-46)toast(37-47)
🪛 Biome (2.1.2)
frontend/src/pages/continue-page.tsx
[error] 24-24: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 26-26: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 27-27: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 40-40: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 41-41: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 127-127: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
internal/controller/context_controller.go (2)
18-20: Field rename to OAuth looks good and keeps json:"oauth".No API change for clients; internal naming aligns with common casing.
92-105: App context response matches frontend schema (appUrl/rootDomain).Good alignment with the new schema and UI gate.
internal/bootstrap/app_bootstrap.go (1)
182-184: Correctly wiring AppURL and RootDomain.RootDomain derived from GetUpperDomain and passed consistently.
frontend/src/lib/i18n/locales/en-US.json (1)
54-56: Domain warning copy LGTM.
The new domainWarningTitle/Subtitles and ignoreTitle read clearly and match the new DomainWarning flow.frontend/src/lib/i18n/locales/en.json (1)
54-56: LGTM on new keys.
Copies align with DomainWarning and untrusted-redirect flows.frontend/src/pages/continue-page.tsx (1)
145-151: Timeout cleanup and manual fallback LGTM.The manual button appears after 1s; with the gating/useEffect cleanup above this avoids stray timers on alternate branches.
frontend/src/components/domain-warning/domain-warning.tsx (2)
21-33: Solid i18n usage with Trans and code componentsReads well and keeps URLs safely wrapped.
35-37: No action needed: Button supports variant="warning"
ThebuttonVariantsdefinition inbutton.tsxincludes awarningvariant, so the fallback todestructiveis unnecessary.frontend/src/components/layout/layout.tsx (1)
7-23: BaseLayout extraction looks goodKeeps language selector and background concerns isolated.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/pages/continue-page.tsx (1)
36-39: Optional: normalize hostnames for UX (case/trailing dot)Lowercasing and trimming a trailing dot avoids false negatives for legitimate hosts (e.g.,
EXAMPLE.COM,example.com.) showing as untrusted.- ? redirectUriObj.hostname === rootDomain || - redirectUriObj.hostname.endsWith(`.${rootDomain}`) + ? normalizeHost(redirectUriObj.hostname) === normalizeHost(rootDomain) || + normalizeHost(redirectUriObj.hostname).endsWith(`.${normalizeHost(rootDomain)}`) : false; + function normalizeHost(host: string) { + return host.toLowerCase().replace(/\.$/, ""); + }
🧹 Nitpick comments (3)
frontend/src/pages/login-page.tsx (1)
94-105: Autoredirect safety + top-level redirect guard LGTM (minor hooks nit)
- Skipping OAuth autoredirect when already logged in and routing logged-in users to
/logoutis consistent with the flow.- Minor: consider adding dependencies to the
useEffector an explicit comment to silence exhaustive-deps if intentional.- useEffect(() => { + useEffect(() => { if (isMounted()) { if ( oauthConfigured && configuredProviders.includes(oauthAutoRedirect) && !isLoggedIn && redirectUri ) { oauthMutation.mutate(oauthAutoRedirect); } } - }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []);Also applies to: 107-109
frontend/src/pages/continue-page.tsx (2)
30-34: Minor: build URL from the same sanitized string you validatedKeeps validation and parsing in lockstep and avoids surprises if sanitization alters the string.
- const isValidRedirectUri = - redirectUri !== null ? isValidUrl(DOMPurify.sanitize(redirectUri)) : false; - const redirectUriObj = isValidRedirectUri - ? new URL(redirectUri as string) - : null; + const sanitizedRedirectUri = + redirectUri !== null ? DOMPurify.sanitize(redirectUri) : null; + const isValidRedirectUri = + sanitizedRedirectUri !== null ? isValidUrl(sanitizedRedirectUri) : false; + const redirectUriObj = isValidRedirectUri + ? new URL(sanitizedRedirectUri as string) + : null;
99-104: Tone of primary action in untrusted flowConsider
variant="warning"instead ofdestructiveto align with the insecure-redirect screen and differentiate danger from confirmation.- <Button - onClick={handleRedirect} - loading={loading} - variant="destructive" - > + <Button onClick={handleRedirect} loading={loading} variant="warning">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
frontend/src/components/layout/layout.tsx(2 hunks)frontend/src/lib/i18n/locales/en-US.json(2 hunks)frontend/src/lib/i18n/locales/en.json(2 hunks)frontend/src/pages/continue-page.tsx(4 hunks)frontend/src/pages/login-page.tsx(6 hunks)frontend/src/pages/logout-page.tsx(2 hunks)frontend/src/pages/totp-page.tsx(1 hunks)frontend/src/pages/unauthorized-page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/lib/i18n/locales/en.json
- frontend/src/components/layout/layout.tsx
- frontend/src/lib/i18n/locales/en-US.json
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/src/pages/login-page.tsx (1)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)
frontend/src/pages/continue-page.tsx (3)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/lib/utils.ts (1)
isValidUrl(8-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
frontend/src/pages/totp-page.tsx (1)
50-52: Guard placement LGTMMoving the redirect guard after mutation creation is fine and keeps the component consistent with the rest of the auth flow.
frontend/src/pages/logout-page.tsx (2)
31-31: Non-async timeout is appropriateDropping the unnecessary async wrapper is correct here.
42-44: Late redirect guard LGTMDeferring the
isLoggedIncheck until after the mutation definition matches the new pattern across pages.frontend/src/pages/login-page.tsx (2)
52-54: Uselocation.replaceto avoid extra history entry — good callPrevents users from navigating back into a transient state.
130-133: Per-provider loading state is clear and correctUsing
oauthMutation.variablesto scope the spinner to the active provider improves UX.Also applies to: 144-147, 156-159
frontend/src/pages/unauthorized-page.tsx (2)
15-19: Hook consolidation/readability LGTMCo-locating
t,navigate, and loading state simplifies the component.
31-33: Redirect guard order LGTMEarly exit when both
usernameandipare absent avoids rendering unnecessary UI.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/pages/continue-page.tsx (1)
55-64: LGTM: auto-redirect timers with cleanup.Thanks for adding proper cleanup; this addresses earlier feedback about clearing timeouts.
Also applies to: 66-69, 70-73, 75-79
🧹 Nitpick comments (3)
frontend/src/pages/logout-page.tsx (1)
31-36: Don't return a cleanup from onSuccess; React Query ignores it.The returned function is never called by React Query. Either drop the return or manage the timer with a ref + component unmount cleanup.
Apply minimal fix (remove unused cleanup):
- const redirect = setTimeout(() => { + const redirect = window.setTimeout(() => { window.location.replace("/login"); }, 500); - - return () => clearTimeout(redirect);Robust alternative (outside this hunk): store the ID and clear on unmount.
// add near component top const logoutRedirectTimer = useRef<number | null>(null); // inside onSuccess logoutRedirectTimer.current = window.setTimeout(() => { window.location.replace("/login"); }, 500); // add once in component useEffect(() => () => { if (logoutRedirectTimer.current) clearTimeout(logoutRedirectTimer.current); }, []);frontend/src/pages/continue-page.tsx (2)
34-38: Normalize hostnames to avoid false warnings (case, trailing dot).Hostnames are case-insensitive and may include a trailing dot; normalize both sides before comparison to reduce noise for valid redirects.
- const isTrustedRedirectUri = - redirectUriObj !== null - ? redirectUriObj.hostname === rootDomain || - redirectUriObj.hostname.endsWith(`.${rootDomain}`) - : false; + const isTrustedRedirectUri = + redirectUriObj !== null + ? (() => { + const host = redirectUriObj.hostname.toLowerCase().replace(/\.$/, ""); + const root = rootDomain.toLowerCase().replace(/\.$/, ""); + return host === root || host.endsWith(`.${root}`); + })() + : false;
52-53: Remove non-null assertion with a guard (minor).Safer and clearer to guard before use.
- const handleRedirect = () => { - setLoading(true); - window.location.replace(redirectUriObj!.toString()); - }; + const handleRedirect = () => { + if (!redirectUriObj) return; + setLoading(true); + window.location.replace(redirectUriObj.toString()); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
frontend/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
frontend/package.json(0 hunks)frontend/src/pages/continue-page.tsx(4 hunks)frontend/src/pages/login-page.tsx(7 hunks)frontend/src/pages/logout-page.tsx(2 hunks)frontend/src/pages/totp-page.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/pages/totp-page.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T13:58:41.884Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#332
File: frontend/src/pages/continue-page.tsx:30-45
Timestamp: 2025-09-01T13:58:41.884Z
Learning: For redirect URI validation in React applications where the URI is only used for navigation (not DOM rendering), DOMPurify sanitization is unnecessary since React handles DOM sanitization and the URI isn't rendered. The critical security measure is explicit protocol validation to prevent javascript: and data: URL attacks.
Applied to files:
frontend/src/pages/continue-page.tsx
📚 Learning: 2025-09-01T13:52:04.446Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#332
File: frontend/src/pages/continue-page.tsx:30-45
Timestamp: 2025-09-01T13:52:04.446Z
Learning: The URL constructor in JavaScript does not reject dangerous schemes like `javascript:` or `data:` - it successfully parses them into URL objects with the corresponding protocol property. Additional scheme validation is required to prevent XSS attacks through malicious redirect URIs.
Applied to files:
frontend/src/pages/continue-page.tsx
🧬 Code graph analysis (2)
frontend/src/pages/login-page.tsx (1)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)
frontend/src/pages/continue-page.tsx (3)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/lib/utils.ts (1)
isValidUrl(8-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
frontend/src/pages/logout-page.tsx (1)
44-46: LGTM: early redirect guard.Redirecting unauthenticated users to /login here is correct and avoids rendering the page unnecessarily.
frontend/src/pages/login-page.tsx (4)
27-28: LGTM: context destructure changes.
103-104: LGTM: guard oauth auto-redirect when already logged in.
111-113: LGTM: delayed isLoggedIn redirect.Moving this guard avoids immediate short-circuit before effects are set up.
134-137: LGTM: provider-specific loading state.Correctly scopes loading to the active provider.
Also applies to: 148-150, 161-163
frontend/src/pages/continue-page.tsx (6)
39-43: LGTM: explicit http/https allowlist for redirects.Mandatory check; good defense against javascript:/data: schemes.
44-48: LGTM: HTTPS→HTTP downgrade detection.Clear, user-visible warning path.
81-83: LGTM: auth guard to /login.
85-87: LGTM: invalid/blocked scheme → /logout.
163-167: i18n keys verified across all locales
continueRedirectingTitle and continueRedirectingSubtitle are present in every JSON under frontend/src/lib/i18n/locales.
127-156: variant="warning" is supported by Button. ThebuttonVariantsdefinition infrontend/src/components/ui/button.tsxincludes awarningkey, so this usage is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/pages/logout-page.tsx (1)
14-14: Good fix: timers are now scoped and cleaned up.Introducing redirectTimer with a proper unmount cleanup resolves the earlier leak risk and makes redirects predictable.
Also applies to: 24-25, 34-37, 45-50
frontend/src/pages/login-page.tsx (1)
20-20: Timer leak fixed as previously requested — nice follow-through.Import/useRef + unmount cleanup aligns with the earlier recommendation.
Also applies to: 33-34, 109-115
🧹 Nitpick comments (9)
frontend/src/pages/logout-page.tsx (3)
34-37: Extract the 500ms delay to a shared constant.Avoid magic numbers and keep redirect timings consistent across pages.
- redirectTimer.current = window.setTimeout(() => { + const REDIRECT_DELAY_MS = 500; + redirectTimer.current = window.setTimeout(() => { window.location.replace("/login"); - }, 500); + }, REDIRECT_DELAY_MS);Optionally centralize in a constants module and reuse in login/totp pages.
52-54: Use replace to avoid history bloat.Small UX nit: add replace so back button won’t return to a dead page.
- return <Navigate to="/login" />; + return <Navigate to="/login" replace />;
90-92: Prevent double-submits explicitly.If doesn’t disable clicks, add disabled to block repeated mutations.
- <Button - loading={logoutMutation.isPending} - onClick={() => logoutMutation.mutate()} - > + <Button + loading={logoutMutation.isPending} + disabled={logoutMutation.isPending} + onClick={() => logoutMutation.mutate()} + >Please confirm whether loading already disables the button.
frontend/src/pages/totp-page.tsx (2)
39-44: Graceful fallback when redirectUri is missing.Routing to /continue with an empty redirect_uri can cascade into a logout (per continue-page behavior). Prefer a sane default.
- redirectTimer.current = window.setTimeout(() => { - window.location.replace( - `/continue?redirect_uri=${encodeURIComponent(redirectUri ?? "")}`, - ); - }, 500); + const nextHref = redirectUri + ? `/continue?redirect_uri=${encodeURIComponent(redirectUri)}` + : "/"; + const REDIRECT_DELAY_MS = 500; + redirectTimer.current = window.setTimeout(() => { + window.location.replace(nextHref); + }, REDIRECT_DELAY_MS);Verify that continue-page still handles the valid-URI path as expected.
59-61: Navigate with replace.Minor UX polish for non-pending visits.
- if (!totpPending) { - return <Navigate to="/" />; - } + if (!totpPending) { + return <Navigate to="/" replace />; + }frontend/src/pages/login-page.tsx (4)
54-56: Deduplicate magic delay and handle missing redirectUri.
- Extract the 500ms delay to a constant (shared if possible).
- If redirectUri is absent, avoid calling OAuth/continue flows that assume it.
- redirectTimer.current = window.setTimeout(() => { - window.location.replace(data.data.url); - }, 500); + const REDIRECT_DELAY_MS = 500; + redirectTimer.current = window.setTimeout(() => { + window.location.replace(data.data.url); + }, REDIRECT_DELAY_MS);- redirectTimer.current = window.setTimeout(() => { - window.location.replace( - `/continue?redirect_uri=${encodeURIComponent(redirectUri ?? "")}`, - ); - }, 500); + const nextHref = redirectUri + ? `/continue?redirect_uri=${encodeURIComponent(redirectUri)}` + : "/"; + const REDIRECT_DELAY_MS = 500; + redirectTimer.current = window.setTimeout(() => { + window.location.replace(nextHref); + }, REDIRECT_DELAY_MS);Confirm backend tolerates empty redirect_uri for OAuth; if not, gate buttons when redirectUri is missing (see next comment).
Also applies to: 80-85
96-107: Auto-redirect effect dependencies.Running once is fine here, but add a comment to silence hooks-lint or explicitly disable the rule to avoid churn.
Example:
// eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => { /* ... */ }, []);
116-118: Use replace on the login guard.Prevents stacking an extra history entry.
- if (isLoggedIn) { - return <Navigate to="/logout" />; - } + if (isLoggedIn) { + return <Navigate to="/logout" replace />; + }
139-142: Disable OAuth buttons when redirectUri is absent.Prevents GET /api/oauth/url/... with redirect_uri=, if the server expects a value.
- <OAuthButton + <OAuthButton title="Google" icon={<GoogleIcon />} className="w-full" onClick={() => oauthMutation.mutate("google")} loading={ oauthMutation.isPending && oauthMutation.variables === "google" } - disabled={oauthMutation.isPending || loginMutation.isPending} + disabled={ + oauthMutation.isPending || + loginMutation.isPending || + !redirectUri + } />Apply the same disabled guard to Github/Generic buttons.
Also applies to: 153-155, 166-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
frontend/src/pages/login-page.tsx(7 hunks)frontend/src/pages/logout-page.tsx(3 hunks)frontend/src/pages/totp-page.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/pages/logout-page.tsx (2)
frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/context/app-context.tsx (1)
useAppContext(36-44)
frontend/src/pages/totp-page.tsx (1)
frontend/src/context/user-context.tsx (1)
useUserContext(38-48)
frontend/src/pages/login-page.tsx (3)
frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/lib/hooks/use-is-mounted.ts (1)
useIsMounted(3-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
frontend/src/pages/totp-page.tsx (1)
15-15: Consistent timer pattern with cleanup — looks good.Matches the login/logout pattern and prevents stray redirects.
Also applies to: 26-27, 39-44, 52-57
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/pages/continue-page.tsx (1)
34-38: Normalize hostnames (lowercase, strip trailing dot) to avoid false warningsTreat EXAMPLE.COM and example.com. as trusted equivalents of example.com. Improves UX without weakening security.
Apply:
+ const normalizeHost = (h: string) => h.toLowerCase().replace(/\.$/, ""); + const trustedBase = normalizeHost(rootDomain); + const redirectHost = redirectUriObj !== null ? normalizeHost(redirectUriObj.hostname) : ""; - const isTrustedRedirectUri = - redirectUriObj !== null - ? redirectUriObj.hostname === rootDomain || - redirectUriObj.hostname.endsWith(`.${rootDomain}`) - : false; + const isTrustedRedirectUri = + redirectUriObj !== null + ? redirectHost === trustedBase || redirectHost.endsWith(`.${trustedBase}`) + : false;
🧹 Nitpick comments (14)
frontend/src/pages/logout-page.tsx (3)
24-25: Harden timer typing for cross-env compatibility.
Use ReturnType to be safe across DOM/Node typings.- const redirectTimer = useRef<number | null>(null); + const redirectTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
45-51: Good cleanup; tiny null check tweak.
Explicit null check avoids accidental skip if 0 is ever returned.- if (redirectTimer.current) clearTimeout(redirectTimer.current); + if (redirectTimer.current !== null) clearTimeout(redirectTimer.current);
34-36: Replace assign with replace in logout and continue pages
For consistency and to prevent stale history entries, update inlogout-page.tsxandcontinue-page.tsx:frontend/src/pages/logout-page.tsx - window.location.assign("/login"); + window.location.replace("/login");Also in
frontend/src/pages/continue-page.tsx, changewindow.location.assign(redirectUriObj!.toString());to
window.location.replace(redirectUriObj!.toString());frontend/src/pages/totp-page.tsx (3)
26-27: Use ReturnType for robust typing.
Keeps types correct in both browser and Node typings.- const redirectTimer = useRef<number | null>(null); + const redirectTimer = useRef<ReturnType<typeof setTimeout> | null>(null);
52-58: Cleanup effect LGTM; minor null check nit.- if (redirectTimer.current) clearTimeout(redirectTimer.current); + if (redirectTimer.current !== null) clearTimeout(redirectTimer.current);
39-44: Standardize redirect behavior to use replace() across auth flows.
logout-page.tsx (line 35) and continue-page.tsx (line 52) currently call window.location.assign(…); update to window.location.replace(…) to match login-page.tsx and totp-page.tsx.frontend/src/components/domain-warning/domain-warning.tsx (3)
12-16: Props shape is clear; minor naming nit.
currentUrl appears to be an origin; consider currentOrigin for precision in a future pass (no change required now).
27-27: Slight ARIA enhancement for assertive alert.
Add aria-atomic to ensure screen readers announce a coherent message.- <Card role="alert" aria-live="assertive" className="min-w-xs sm:min-w-sm"> + <Card role="alert" aria-live="assertive" aria-atomic="true" className="min-w-xs sm:min-w-sm">
44-48: Build target URL with URL API to avoid edge cases.
Avoid manual string concat and double slashes; easier param handling.- onClick={() => - window.location.assign( - `${appUrl}/login?redirect_uri=${encodeURIComponent(redirectUri || "")}`, - ) - } + onClick={() => { + const url = new URL("/login", appUrl); + if (redirectUri) url.searchParams.set("redirect_uri", redirectUri); + window.location.assign(url.toString()); + }}internal/controller/context_controller.go (1)
94-95: Handle parse failures defensively (even if validated upstream).
Low-cost guard to prevent panics/malformed responses if startup validation changes.- appUrl, _ := url.Parse(controller.Config.AppURL) // no need to check error, validated on startup + appUrl, err := url.Parse(controller.Config.AppURL) // validated on startup; still guard + if err != nil || appUrl.Scheme == "" || appUrl.Host == "" { + log.Error().Err(err).Str("appURL", controller.Config.AppURL).Msg("Invalid AppURL") + c.JSON(200, AppContextResponse{ + Status: 200, + Message: "Success", + ConfiguredProviders: controller.Config.ConfiguredProviders, + Title: controller.Config.Title, + GenericName: controller.Config.GenericName, + AppURL: controller.Config.AppURL, + RootDomain: controller.Config.RootDomain, + ForgotPasswordMessage: controller.Config.ForgotPasswordMessage, + BackgroundImage: controller.Config.BackgroundImage, + OAuthAutoRedirect: controller.Config.OAuthAutoRedirect, + }) + return + }frontend/src/pages/continue-page.tsx (4)
50-53: Prefer replace() and guard redirectUriObjReplace avoids polluting history/back-loop; add a minimal null guard.
- const handleRedirect = () => { - setLoading(true); - window.location.assign(redirectUriObj!.toString()); - }; + const handleRedirect = () => { + if (!redirectUriObj) return; + setLoading(true); + window.location.replace(redirectUriObj.toString()); + };
55-79: Effect deps: either annotate intent or make it lint-cleanIf react-hooks/exhaustive-deps is enabled, consider wrapping handleRedirect in useCallback and listing deps, or add an inline disable with rationale to keep it “run-once”.
Example:
- useEffect(() => { + // Intentional: run-once auto-redirect gate + // eslint-disable-next-line react-hooks/exhaustive-deps + useEffect(() => { ... - }, []); + }, []);
120-124: Use replace: true when cancelling to logoutAvoids returning to the warning page via Back.
- <Button - onClick={() => navigate("/logout")} + <Button + onClick={() => navigate("/logout", { replace: true })} variant="outline" disabled={loading} >
154-157: Same here: replace on logoutConsistent UX for cancel action.
- <Button - onClick={() => navigate("/logout")} + <Button + onClick={() => navigate("/logout", { replace: true })} variant="outline" disabled={loading} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
frontend/src/App.tsx(1 hunks)frontend/src/components/domain-warning/domain-warning.tsx(1 hunks)frontend/src/lib/i18n/locales/en-US.json(2 hunks)frontend/src/lib/i18n/locales/en.json(2 hunks)frontend/src/pages/continue-page.tsx(4 hunks)frontend/src/pages/login-page.tsx(7 hunks)frontend/src/pages/logout-page.tsx(3 hunks)frontend/src/pages/totp-page.tsx(3 hunks)internal/controller/context_controller.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/lib/i18n/locales/en.json
- frontend/src/pages/login-page.tsx
- frontend/src/lib/i18n/locales/en-US.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T13:58:41.884Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#332
File: frontend/src/pages/continue-page.tsx:30-45
Timestamp: 2025-09-01T13:58:41.884Z
Learning: For redirect URI validation in React applications where the URI is only used for navigation (not DOM rendering), DOMPurify sanitization is unnecessary since React handles DOM sanitization and the URI isn't rendered. The critical security measure is explicit protocol validation to prevent javascript: and data: URL attacks.
Applied to files:
frontend/src/pages/continue-page.tsx
📚 Learning: 2025-09-01T13:52:04.446Z
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#332
File: frontend/src/pages/continue-page.tsx:30-45
Timestamp: 2025-09-01T13:52:04.446Z
Learning: The URL constructor in JavaScript does not reject dangerous schemes like `javascript:` or `data:` - it successfully parses them into URL objects with the corresponding protocol property. Additional scheme validation is required to prevent XSS attacks through malicious redirect URIs.
Applied to files:
frontend/src/pages/continue-page.tsx
🧬 Code graph analysis (5)
frontend/src/components/domain-warning/domain-warning.tsx (1)
frontend/src/components/ui/card.tsx (6)
Card(85-85)CardTitle(88-88)CardDescription(90-90)CardTitle(31-39)CardDescription(41-49)Card(5-16)
frontend/src/pages/logout-page.tsx (2)
frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/context/app-context.tsx (1)
useAppContext(36-44)
frontend/src/pages/continue-page.tsx (3)
frontend/src/context/app-context.tsx (1)
useAppContext(36-44)frontend/src/context/user-context.tsx (1)
useUserContext(38-48)frontend/src/lib/utils.ts (1)
isValidUrl(8-15)
frontend/src/pages/totp-page.tsx (1)
frontend/src/context/user-context.tsx (1)
useUserContext(38-48)
internal/controller/context_controller.go (2)
internal/config/config.go (1)
Config(17-56)internal/controller/oauth_controller.go (5)
Config(29-34)controller(93-210)controller(45-49)CSRFCookieName(21-27)controller(51-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
frontend/src/pages/logout-page.tsx (2)
14-14: Import change looks good.
Adds necessary hooks for timer cleanup.
52-54: Late guard placement LGTM.
Prevents premature unmount before toasts/mutation wiring; replace avoids extra history.frontend/src/pages/totp-page.tsx (2)
15-15: Import change looks good.
Brings in hooks needed for timer handling.
59-61: Guard relocation LGTM.
Ensures mutation/toast setup before redirecting away.frontend/src/components/domain-warning/domain-warning.tsx (3)
23-25: URL param extraction LGTM.
Safe parsing with URLSearchParams; encode later is correct.
26-37: Accessible copy rendering LGTM.
Trans usage with code components and i18n keys is clean.
40-42: Ignore button wiring LGTM.
Variant and handler align with session-dismiss flow.internal/controller/context_controller.go (5)
4-5: Imports align with new behavior.
fmt and net/url are appropriate for URL normalization.
20-20: Field rename preserves API contract.
Go field OAuth with json:"oauth" keeps wire format stable.
30-31: AppURL/RootDomain exposure LGTM.
Matches frontend needs for domain warning and routing.
41-42: Config struct updates LGTM.
Carries app URL and root domain through to controller cleanly.
77-77: Context field usage LGTM.
Assumes utils.Context has OAuth bool; consistent naming reduces confusion.frontend/src/App.tsx (1)
8-8: Replace-based redirects LGTM.
Prevents redundant history entries for root routing; consistent with page-level guards.Also applies to: 11-11
frontend/src/pages/continue-page.tsx (5)
39-43: Good: explicit http/https allowlistSolid defense; blocks javascript:, data:, file:, etc.
44-48: Good: HTTPS→HTTP downgrade detectionClear, correct check using window.location.protocol.
66-78: Nice: timers are cleared on unmountPrevents leaks/late state updates.
81-88: Login redirect looks goodQuery param is safely encoded; replace prevents back-nav oddities.
90-92: Early reject of invalid/scheme is correctCleanly exits flows that shouldn’t proceed.
1 participant
Summary by CodeRabbit