-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix: OAuth/OIDC SSO flow and cookie handling for public suffix domains #1090
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
Fixes three bugs preventing successful SSO integration with OIDC providers
like Authelia, Keycloak, etc:
1. Changed redirect_uri from /settings to /auth?provider=GENERIC
The frontend OAuth callback handler requires the provider query param
to trigger the token exchange
2. Added useRef guard to prevent React StrictMode double-execution
This prevents "authorization code already used" errors
3. Added handling for existing user login response
When backend returns { login: true }, redirect to dashboard instead
of staying stuck on loading screen
Also added state parameter for CSRF protection per OAuth spec.
…g, etc) When deploying Postiz on dynamic DNS services or NAS platforms that use domains on the Public Suffix List (PSL), browsers reject cookies set with a domain like .synology.me because it could affect all *.synology.me sites (a "supercookie" attack vector). This fix detects when the domain's registrable part is itself a public suffix (e.g., synology.me is on the ICANN section of PSL) and returns undefined instead of .synology.me. This causes Express to omit the Domain attribute, creating a "host-only" cookie bound to the exact hostname (e.g., postiz.alexbeav.synology.me). This approach follows the same pattern used by Portainer and Sonarr/Radarr for handling authentication on public suffix domains. Affected services: synology.me, duckdns.org, ngrok.io, hopto.org, etc.
|
@Alexbeav is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
| if (url.isIcann && url.publicSuffix !== url.domain) { | ||
| // The domain's parent is on the public suffix list | ||
| // Return undefined to use host-only cookies (like Portainer does) |
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.
Bug: The condition url.publicSuffix !== url.domain is inverted, causing getCookieUrlFromDomain to return undefined for normal domains.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The condition url.isIcann && url.publicSuffix !== url.domain on line 23 of libraries/helpers/src/subdomain/subdomain.management.ts is logically inverted. For normal domains like example.com, url.publicSuffix ("com") is different from url.domain ("example.com"), causing the condition to evaluate to true and undefined to be returned. This incorrectly breaks cross-subdomain cookie sharing for regular domains, localhost, and IP-based deployments.
💡 Suggested Fix
The condition url.isIcann && url.publicSuffix !== url.domain should be re-evaluated. It likely needs to check if url.domain itself is a public suffix, rather than if url.publicSuffix is different from url.domain.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: libraries/helpers/src/subdomain/subdomain.management.ts#L23-L25
Potential issue: The condition `url.isIcann && url.publicSuffix !== url.domain` on line
23 of `libraries/helpers/src/subdomain/subdomain.management.ts` is logically inverted.
For normal domains like `example.com`, `url.publicSuffix` ("com") is different from
`url.domain` ("example.com"), causing the condition to evaluate to true and `undefined`
to be returned. This incorrectly breaks cross-subdomain cookie sharing for regular
domains, localhost, and IP-based deployments.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 284087
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 multiple OAuth/OIDC SSO integration bugs including incorrect redirect URIs, React StrictMode double-execution, existing user redirect issues, and cookie domain handling for public suffix domains. However, the cookie domain logic appears to have a critical bug that inverts the intended behavior, and the state parameter implementation introduces a CSRF vulnerability.
Key Changes
- Updated OAuth redirect_uri from
/settingsto/auth?provider=GENERICto ensure proper frontend callback handling - Added useRef guard to prevent double token exchange in React StrictMode
- Implemented redirect logic for existing SSO users to dashboard instead of staying on loading screen
- Modified cookie domain function to detect and handle public suffix list domains
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libraries/helpers/src/subdomain/subdomain.management.ts | Updated getCookieUrlFromDomain to detect public suffix domains and return undefined for host-only cookies; logic appears inverted |
| apps/frontend/src/components/auth/register.tsx | Added useRef to prevent double OAuth code exchange; added redirect handling for existing users; missing dependency in useEffect |
| apps/backend/src/services/auth/providers/oauth.provider.ts | Changed redirect_uri to /auth?provider=GENERIC; added state parameter without validation (CSRF vulnerability) |
| const state = Math.random().toString(36).substring(2, 15); | ||
| const params = new URLSearchParams({ | ||
| client_id: this.clientId, | ||
| scope: 'openid profile email', | ||
| response_type: 'code', | ||
| redirect_uri: `${this.frontendUrl}/settings`, | ||
| redirect_uri: `${this.frontendUrl}/auth?provider=GENERIC`, | ||
| state, |
Copilot
AI
Dec 3, 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.
The state parameter is generated but never validated during the OAuth callback. This creates a CSRF vulnerability where an attacker could trick a user into authenticating with the attacker's OAuth code, potentially allowing account takeover.
The state should be:
- Generated and stored (e.g., in Redis or a session)
- Sent to the OAuth provider
- Validated when the callback is received to ensure it matches the stored value
Similar to how the integrations controller uses Redis to store and validate state (login:${state}), this OAuth flow should implement the same pattern.
| loadingRef.current = true; | ||
| load(); | ||
| } | ||
| }, []); |
Copilot
AI
Dec 3, 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.
The useEffect dependency array is empty but the effect calls load() which depends on provider and code state variables. This could cause stale closure issues where the function uses outdated values.
Either:
- Add
loadto the dependency array:[load] - Or if you want the effect to run only once, move the condition inside and use
getQuery?.get('provider')andgetQuery?.get('code')directly instead of relying on state
| }, []); | |
| }, [load]); |
| if (url.isIcann && url.publicSuffix !== url.domain) { | ||
| // The domain's parent is on the public suffix list |
Copilot
AI
Dec 3, 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.
The logic for detecting public suffix domains is inverted. The condition url.publicSuffix !== url.domain will be true for normal domains (like example.com where publicSuffix is com) and false for public suffix domains (like postiz.synology.me where both would be synology.me).
This means the code currently:
- Returns
undefined(host-only cookies) for normal domains likeexample.com❌ - Sets domain cookies for public suffix domains like
synology.me❌
The condition should be:
if (url.isIcann && url.publicSuffix === url.domain) {
// The domain is a public suffix - use host-only cookies
return undefined;
}This would correctly return undefined only when the domain itself is on the Public Suffix List.
| if (url.isIcann && url.publicSuffix !== url.domain) { | |
| // The domain's parent is on the public suffix list | |
| if (url.isIcann && url.publicSuffix === url.domain) { | |
| // The domain itself is on the public suffix list |
| setShow(true); | ||
| } | ||
| }, [provider, code]); | ||
| }, [provider, code, router]); |
Copilot
AI
Dec 3, 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.
The router variable is added to the dependencies array but is not actually used in the load function. The Next.js router object is stable and doesn't need to be in the dependency array.
Remove router from the dependencies:
}, [provider, code]);| }, [provider, code, router]); | |
| }, [provider, code]); |
Summary
Fixes multiple bugs preventing successful SSO integration with OIDC providers (Authelia, Keycloak, etc.) and authentication on public suffix domains (synology.me, duckdns.org, ngrok.io, etc.).
Issues Fixed
Incorrect redirect_uri - Changed from
/settingsto/auth?provider=GENERICso the frontend OAuth callback handler receives the provider query param needed to trigger token exchangeReact StrictMode double token exchange - Added
useRefguard to prevent the authorization code from being exchanged twice (causing "code already used" errors)Existing user stuck on loading - When backend returns
{ login: true }for existing SSO users, now properly redirects to dashboard usingwindow.location.hrefinstead of staying on the loading screenCookie domain rejected on public suffixes - When deploying on domains like
postiz.example.synology.me, browsers reject cookies set with domain.synology.mebecause it's on the Public Suffix List (PSL). The fix detects PSL domains and returnsundefinedto create host-only cookies bound to the exact hostname.Technical Details
Cookie Domain Fix (Issue 4)
The
getCookieUrlFromDomain()function now uses thetldtslibrary to detect when a domain's registrable part is itself a public suffix:This follows the same approach used by Portainer and Sonarr/Radarr for handling authentication on dynamic DNS/NAS domains.
Affected Platforms
Test Plan
/auth?provider=GENERIC)*.synology.medomains