Skip to content

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.

Sign up for GitHub

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

Open
Alexbeav wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: OAuth/OIDC SSO flow and cookie handling for public suffix domains #1090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebf9ffc
fix: OAuth/OIDC SSO flow for generic providers
Alexbeav Dec 3, 2025
550a104
fix: cookie domain for public suffix domains (synology.me, duckdns.or…
Alexbeav Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions apps/backend/src/services/auth/providers/oauth.provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ export class OauthProvider implements ProvidersInterface {
}

generateLink(): string {
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,
Comment on lines +52 to +58
Copy link

Copilot AI Dec 3, 2025

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:

  1. Generated and stored (e.g., in Redis or a session)
  2. Sent to the OAuth provider
  3. 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.

Copilot uses AI. Check for mistakes.
});

return `${this.authUrl}?${params.toString()}`;
Expand All @@ -71,7 +73,7 @@ export class OauthProvider implements ProvidersInterface {
client_id: this.clientId,
client_secret: this.clientSecret,
code,
redirect_uri: `${this.frontendUrl}/settings`,
redirect_uri: `${this.frontendUrl}/auth?provider=GENERIC`,
}),
});

Expand Down
24 changes: 20 additions & 4 deletions apps/frontend/src/components/auth/register.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { useFetch } from '@gitroom/helpers/utils/custom.fetch';
import Link from 'next/link';
import { Button } from '@gitroom/react/form/button';
import { Input } from '@gitroom/react/form/input';
import { useCallback, useEffect, useMemo, useState } from 'react';
import { useCallback, useEffect, useMemo, useState, useRef } from 'react';
import { classValidatorResolver } from '@hookform/resolvers/class-validator';
import { CreateOrgUserDto } from '@gitroom/nestjs-libraries/dtos/auth/create.org.user.dto';
import { GithubProvider } from '@gitroom/frontend/components/auth/providers/github.provider';
Expand Down Expand Up @@ -39,28 +39,44 @@ type Inputs = {
export function Register() {
const getQuery = useSearchParams();
const fetch = useFetch();
const router = useRouter();
const [provider] = useState(getQuery?.get('provider')?.toUpperCase());
const [code, setCode] = useState(getQuery?.get('code') || '');
const [show, setShow] = useState(false);
// Prevent double-execution in React StrictMode
const loadingRef = useRef(false);

useEffect(() => {
if (provider && code) {
if (provider && code && !loadingRef.current) {
loadingRef.current = true;
load();
}
}, []);
Copy link

Copilot AI Dec 3, 2025

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:

  1. Add load to the dependency array: [load]
  2. Or if you want the effect to run only once, move the condition inside and use getQuery?.get('provider') and getQuery?.get('code') directly instead of relying on state
Suggested change
}, []);
}, [load]);

Copilot uses AI. Check for mistakes.

const load = useCallback(async () => {
const { token } = await (
const { token, login } = await (
await fetch(`/auth/oauth/${provider?.toUpperCase() || 'LOCAL'}/exists`, {
method: 'POST',
body: JSON.stringify({
code,
}),
})
).json();

// Handle existing user login - backend already set auth cookie
// Use window.location.href instead of router.push to do a full page reload
// This clears the OAuth code from the URL and ensures cookies are properly read
if (login) {
window.location.href = '/';
return;
}

if (token) {
setCode(token);
setShow(true);
}
}, [provider, code]);
}, [provider, code, router]);
Copy link

Copilot AI Dec 3, 2025

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]);
Suggested change
}, [provider, code, router]);
}, [provider, code]);

Copilot uses AI. Check for mistakes.

if (!code && !provider) {
return <RegisterAfter token="" provider="LOCAL" />;
}
Expand Down
30 changes: 28 additions & 2 deletions libraries/helpers/src/subdomain/subdomain.management.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,32 @@
import { parse } from 'tldts';

export function getCookieUrlFromDomain(domain: string) {
export function getCookieUrlFromDomain(domain: string): string | undefined {
const url = parse(domain);
return url.domain! ? '.' + url.domain! : url.hostname!;

// If the domain is a public suffix (like synology.me, duckdns.org, ngrok.io, etc.),
// don't set an explicit cookie domain - let the browser use host-only cookies.
// This follows the same approach as Portainer and Sonarr/Radarr.
// Setting domain to .synology.me would be rejected by browsers as a "supercookie"
// because public suffixes are on the PSL (Public Suffix List).
//
// For postiz.alexbeav.synology.me:
// - hostname = "postiz.alexbeav.synology.me"
// - domain = "synology.me" (the registrable domain)
// - publicSuffix = "me"
// - isIcann = true (synology.me is on the ICANN section of PSL)
//
// Setting cookie domain to .synology.me would fail because browsers won't allow
// cookies that could affect all *.synology.me sites.
//
// By returning undefined, Express won't set a Domain attribute, and the browser
// will create a host-only cookie bound to the exact hostname.
if (url.isIcann && url.publicSuffix !== url.domain) {
// The domain's parent is on the public suffix list
Comment on lines +23 to +24
Copy link

Copilot AI Dec 3, 2025

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 like example.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.

Suggested change
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

Copilot uses AI. Check for mistakes.
// Return undefined to use host-only cookies (like Portainer does)
Comment on lines +23 to +25
Copy link

@sentry sentry bot Dec 3, 2025

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

return undefined;
}

// For regular domains like postiz.example.com, return .example.com
// This allows cookie sharing across subdomains when using a private domain
return url.domain ? '.' + url.domain : undefined;
}