Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
73 changes: 71 additions & 2 deletions apps/backend/src/lib/redirect-urls.test.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { describe, expect, it } from 'vitest';
import { describe, expect, it, vi } from 'vitest';
import { isAcceptedNativeAppUrl, validateRedirectUrl } from './redirect-urls';
import { Tenancy } from './tenancies';

describe('validateRedirectUrl', () => {
const createMockTenancy = (config: Partial<Tenancy['config']>): Tenancy => {
const createMockTenancy = (config: Partial<Tenancy['config']>, projectId: string = 'aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee'): Tenancy => {
return {
project: { id: projectId },
config: {
domains: {
allowLocalhost: false,
Expand Down Expand Up @@ -474,6 +475,74 @@ describe('validateRedirectUrl', () => {
});
});

describe('hosted handler domain (built-with domain)', () => {
it('should trust the project hosted handler domain with default suffix', () => {
vi.stubEnv('NEXT_PUBLIC_STACK_HOSTED_HANDLER_DOMAIN_SUFFIX', '.built-with-stack-auth.com');
const projectId = '12345678-1234-1234-1234-123456789012';
const tenancy = createMockTenancy({
domains: {
allowLocalhost: false,
trustedDomains: {},
},
}, projectId);

// HTTPS on the built-with domain should be trusted
expect(validateRedirectUrl(`https://${projectId}.built-with-stack-auth.com/callback`, tenancy)).toBe(true);
expect(validateRedirectUrl(`https://${projectId}.built-with-stack-auth.com/`, tenancy)).toBe(true);
expect(validateRedirectUrl(`https://${projectId}.built-with-stack-auth.com/handler/oauth-callback`, tenancy)).toBe(true);

// HTTP on the built-with domain should NOT be trusted
expect(validateRedirectUrl(`http://${projectId}.built-with-stack-auth.com/callback`, tenancy)).toBe(false);

// Different project IDs should NOT be trusted
expect(validateRedirectUrl('https://other-project.built-with-stack-auth.com/callback', tenancy)).toBe(false);

// Unrelated domains should NOT be trusted
expect(validateRedirectUrl('https://example.com/callback', tenancy)).toBe(false);

vi.unstubAllEnvs();
});
Comment on lines +478 to +504

it('should trust the hosted handler domain with a custom suffix (e.g. local dev)', () => {
vi.stubEnv('NEXT_PUBLIC_STACK_HOSTED_HANDLER_DOMAIN_SUFFIX', '.localhost:8109');
const projectId = '12345678-1234-1234-1234-123456789012';
const tenancy = createMockTenancy({
domains: {
allowLocalhost: false,
trustedDomains: {},
},
}, projectId);

// Only HTTPS should be trusted, even for localhost-based dev suffix
expect(validateRedirectUrl(`https://${projectId}.localhost:8109/callback`, tenancy)).toBe(true);
expect(validateRedirectUrl(`http://${projectId}.localhost:8109/callback`, tenancy)).toBe(false);

// Wrong port should NOT be trusted
Comment on lines +519 to +520
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 "Wrong port" test case uses HTTP, not HTTPS — it tests the wrong thing

The assertion passes because the protocol is HTTP, not because the port is wrong. Since only https://… is added to trusted domains, any HTTP URL returns false regardless of the port. The test comment says "Wrong port should NOT be trusted" but it never reaches port-matching logic. To actually verify the port-mismatch rejection, the URL should use https://.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/lib/redirect-urls.test.tsx
Line: 519-520

Comment:
**"Wrong port" test case uses HTTP, not HTTPS — it tests the wrong thing**

The assertion passes because the protocol is HTTP, not because the port is wrong. Since only `https://…` is added to trusted domains, any HTTP URL returns `false` regardless of the port. The test comment says "Wrong port should NOT be trusted" but it never reaches port-matching logic. To actually verify the port-mismatch rejection, the URL should use `https://`.

How can I resolve this? If you propose a fix, please make it concise.

expect(validateRedirectUrl(`http://${projectId}.localhost:9999/callback`, tenancy)).toBe(false);

vi.unstubAllEnvs();
});

it('should trust the hosted handler domain even when other trusted domains exist', () => {
vi.stubEnv('NEXT_PUBLIC_STACK_HOSTED_HANDLER_DOMAIN_SUFFIX', '.built-with-stack-auth.com');
const projectId = '12345678-1234-1234-1234-123456789012';
const tenancy = createMockTenancy({
domains: {
allowLocalhost: false,
trustedDomains: {
'1': { baseUrl: 'https://myapp.com', handlerPath: '/handler' },
},
},
}, projectId);

// Both the configured trusted domain and the hosted handler domain should work
expect(validateRedirectUrl('https://myapp.com/callback', tenancy)).toBe(true);
expect(validateRedirectUrl(`https://${projectId}.built-with-stack-auth.com/callback`, tenancy)).toBe(true);

vi.unstubAllEnvs();
});
});

describe('native app SDK URLs', () => {
it('should not accept native app URLs in validateRedirectUrl (handled separately in OAuth model)', () => {
const tenancy = createMockTenancy({
Expand Down
17 changes: 16 additions & 1 deletion apps/backend/src/lib/redirect-urls.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
import { isAcceptedNativeAppUrl, validateRedirectUrl as validateRedirectUrlAgainstTrustedDomains } from "@stackframe/stack-shared/dist/utils/redirect-urls";
import { getEnvVariable } from "@stackframe/stack-shared/dist/utils/env";
import { Tenancy } from "./tenancies";

export { isAcceptedNativeAppUrl };

const defaultHostedHandlerDomainSuffix = ".built-with-stack-auth.com";

/**
* Returns the domain suffix for the hosted handler (e.g. ".built-with-stack-auth.com" in
* production, ".localhost:8109" in local dev).
*/
export function getHostedHandlerDomainSuffix(): string {
return getEnvVariable("NEXT_PUBLIC_STACK_HOSTED_HANDLER_DOMAIN_SUFFIX", defaultHostedHandlerDomainSuffix);
Comment on lines +8 to +14
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev suffix breaks hosted trust

High Severity

The getHostedHandlerDomainSuffix function returns the raw NEXT_PUBLIC_STACK_HOSTED_HANDLER_DOMAIN_SUFFIX env var. In local dev, this includes an unresolved ${NEXT_PUBLIC_STACK_PORT_PREFIX:-81} placeholder. This causes the backend to trust a malformed hosted domain, leading to automatic redirect URL trust failures due to a mismatch with client-used domains.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit cfb9af8. Configure here.


export function validateRedirectUrl(
urlOrString: string | URL,
tenancy: Tenancy,
): boolean {
const hostedDomain = `${tenancy.project.id}${getHostedHandlerDomainSuffix()}`;
return validateRedirectUrlAgainstTrustedDomains(urlOrString, {
allowLocalhost: tenancy.config.domains.allowLocalhost,
trustedDomains: Object.values(tenancy.config.domains.trustedDomains).map(domain => domain.baseUrl),
trustedDomains: [
...Object.values(tenancy.config.domains.trustedDomains).map(domain => domain.baseUrl),
`https://${hostedDomain}`,
],
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignores hosted handler URL template

Medium Severity

Redirect, OAuth, and Turnstile trust only https://{projectId}{domainSuffix}, but hosted handler URLs come from NEXT_PUBLIC_STACK_HOSTED_HANDLER_URL_TEMPLATE when set (e.g. path-based http://localhost:PORT/{projectId}/handler/...). Those real URLs can stay untrusted despite this change.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit cfb9af8. Configure here.

Comment on lines +21 to +27
});
}
8 changes: 8 additions & 0 deletions apps/backend/src/lib/turnstile.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from "@stackframe/stack-shared/dist/utils/turnstile";
import { createUrlIfValid, isLocalhost, matchHostnamePattern } from "@stackframe/stack-shared/dist/utils/urls";
import { BestEffortEndUserRequestContext, getBestEffortEndUserRequestContext } from "./end-users";
import { getHostedHandlerDomainSuffix } from "./redirect-urls";
import { Tenancy } from "./tenancies";


Expand Down Expand Up @@ -50,6 +51,13 @@ function isAllowedTurnstileHostname(hostname: string, tenancy: Tenancy): boolean
if (tenancy.config.domains.allowLocalhost && isLocalhost(`http://${hostname}`)) {
return true;
}

// The project's hosted handler domain (e.g. <project-id>.built-with-stack-auth.com) is always trusted
const hostedHandlerUrl = createUrlIfValid(`https://${tenancy.project.id}${getHostedHandlerDomainSuffix()}`);
if (hostedHandlerUrl != null && hostedHandlerUrl.hostname === hostname) {
return true;
}

Comment on lines +55 to +60
return Object.values(tenancy.config.domains.trustedDomains).some(({ baseUrl }) => {
if (baseUrl == null) return false;
const pattern = createUrlIfValid(baseUrl)?.hostname
Expand Down
6 changes: 5 additions & 1 deletion apps/backend/src/oauth/model.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { usersCrudHandlers } from "@/app/api/latest/users/crud";
import { Prisma } from "@/generated/prisma/client";
import { withExternalDbSyncUpdate } from "@/lib/external-db-sync";
import { checkApiKeySet } from "@/lib/internal-api-keys";
import { isAcceptedNativeAppUrl, validateRedirectUrl } from "@/lib/redirect-urls";
import { getHostedHandlerDomainSuffix, isAcceptedNativeAppUrl, validateRedirectUrl } from "@/lib/redirect-urls";
import { getSoleTenancyFromProjectBranch, getTenancy } from "@/lib/tenancies";
import { createRefreshTokenObj, decodeAccessToken, generateAccessTokenFromRefreshTokenIfValid, isRefreshTokenValid } from "@/lib/tokens";
import { getPrismaClientForTenancy, globalPrismaClient } from "@/prisma-client";
Expand Down Expand Up @@ -76,6 +76,10 @@ export class OAuthModel implements AuthorizationCodeModel {
throw e;
}

// The project's hosted handler domain is always trusted
const hostedDomain = `${tenancy.project.id}${getHostedHandlerDomainSuffix()}`;
redirectUris.push(new URL("/handler", `https://${hostedDomain}`).toString());

Comment on lines +79 to +82
if (redirectUris.length === 0 && tenancy.config.domains.allowLocalhost) {
redirectUris.push("http://localhost");
}
Comment on lines 83 to 85
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Dead code — localhost fallback is never reached

The hosted domain URI is pushed unconditionally on line 81, so redirectUris.length is always >= 1 when this guard is evaluated — the length === 0 condition can never be true and the localhost fallback is unreachable. For projects that rely on allowLocalhost: true with no configured trusted domains, the localhost URI no longer appears in client.redirectUris. If the oauth2-server library uses that list for auto-selection when no redirect_uri is included in the request, those OAuth flows would change behavior silently. The block should either be removed (if validateRedirectUri is fully authoritative) or repositioned so the localhost fallback is still added when appropriate.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/oauth/model.tsx
Line: 83-85

Comment:
**Dead code — localhost fallback is never reached**

The hosted domain URI is pushed unconditionally on line 81, so `redirectUris.length` is always `>= 1` when this guard is evaluated — the `length === 0` condition can never be true and the localhost fallback is unreachable. For projects that rely on `allowLocalhost: true` with no configured trusted domains, the localhost URI no longer appears in `client.redirectUris`. If the oauth2-server library uses that list for auto-selection when no `redirect_uri` is included in the request, those OAuth flows would change behavior silently. The block should either be removed (if `validateRedirectUri` is fully authoritative) or repositioned so the localhost fallback is still added when appropriate.

How can I resolve this? If you propose a fix, please make it concise.

Expand Down
Loading