-
Notifications
You must be signed in to change notification settings - Fork 89
fix(auth): correctly handle non-fragment callbacks #496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes OIDC authentication callback handling to comply with RFC 6749, which prohibits fragment components in OAuth 2.0 redirect URIs. The changes replace fragment-based callbacks (/#callback, /#silent-callback) with path-based routes (/auth, /silent-auth) to support OIDC providers that enforce this standard.
Key Changes:
- Updated redirect URIs from fragment-based to path-based endpoints
- Added new Next.js page routes to handle authentication callbacks
- Enhanced callback success handler to manage redirects for the new
/authroute
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/config.ts | Changed default redirect URIs from fragment-based to path-based |
| src/auth/OIDCProvider.tsx | Added redirect logic for /auth route with query parameter preservation |
| src/auth/CallbackPage.tsx | New component providing fallback for OIDC callback routes |
| src/app/auth/page.tsx | New page route for standard authentication callback |
| src/app/silent-auth/page.tsx | New page route for silent authentication callback |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| queryParams = `?${stored}`; | ||
| localStorage.removeItem("netbird-query-params"); | ||
| } | ||
| } catch (e) { } |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch block silently swallows errors. Consider logging the error or adding a comment explaining why it's safe to ignore.
| } catch (e) { } | |
| } catch (e) { | |
| if (process.env.NODE_ENV !== "production") { | |
| console.error("Error accessing localStorage in CallBackSuccess:", e); | |
| } | |
| } |
| useEffect(() => { | ||
| if (!errorParam && currentPath === "/auth") { | ||
| // Redirect to /peers after a brief delay to ensure tokens are stored | ||
| const timer = setTimeout(() => { |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 100ms delay lacks explanation. Add a comment or constant explaining why this delay is necessary for token storage.
| export default function CallbackPage() { | ||
| const router = useRouter(); | ||
|
|
||
| useEffect(() => { | ||
| // Fallback: if this component renders directly, redirect to /peers | ||
| const timer = setTimeout(() => { | ||
| router.replace("/peers"); | ||
| }, 100); |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number 100ms delay lacks explanation. Add a comment or constant explaining the purpose of this delay.
| export default function CallbackPage() { | |
| const router = useRouter(); | |
| useEffect(() => { | |
| // Fallback: if this component renders directly, redirect to /peers | |
| const timer = setTimeout(() => { | |
| router.replace("/peers"); | |
| }, 100); | |
| // Delay before redirecting to allow any pending UI updates or transitions to complete. | |
| const REDIRECT_DELAY_MS = 100; | |
| export default function CallbackPage() { | |
| const router = useRouter(); | |
| useEffect(() => { | |
| // Fallback: if this component renders directly, redirect to /peers | |
| // The 100ms delay ensures the UI has time to render before navigation, | |
| // which can help avoid race conditions or abrupt transitions. | |
| const timer = setTimeout(() => { | |
| router.replace("/peers"); | |
| }, REDIRECT_DELAY_MS); |
According to https://datatracker.ietf.org/doc/html/rfc6749#section-3.1.2 OIDC callback URLs must not contain a fragment component. Thus, some OIDC providers (at least mine) don't support this, but setting the callback to a path results in 404 from the web server because no actual page exists there.
This PR adds two pages
/authand/silent-authto allow error-free handling of the callback.It should thereby resolve #398.