diff --git a/.changeset/csrf-localhost-trio-dev-only.md b/.changeset/csrf-localhost-trio-dev-only.md new file mode 100644 index 0000000000..1ee559580c --- /dev/null +++ b/.changeset/csrf-localhost-trio-dev-only.md @@ -0,0 +1,45 @@ +--- +"@objectstack/plugin-auth": patch +--- + +Gate the localhost trusted-origin substitution to non-production (#10366). + +`AuthManager`'s `trustedOrigins` block substituted a localhost wildcard trio +(`http://localhost:*`, `http://*.localhost:*`, `https://*.localhost:*`) whenever +the resolved trusted-origin list came out empty and `OS_CORS_ORIGIN` was unset +or `*`. Its own comment described this as a development convenience, but the +condition tested only emptiness — it carried no `NODE_ENV` term, no dev-mode +term, nothing. A production deployment that reached it with an empty list +CSRF-trusted every `localhost` and `*.localhost` origin. The declared boundary +and the enforced boundary disagreed, and only the declared one was visible in +the file. + +The substitution is now gated on `NODE_ENV !== 'production'`, the same dev +signal already used by the fallback auth secret and by the dev `Origin` +synthesis in the same file. The property enforced: **a development convenience +exists only outside production.** + +**What production receives instead.** With the trio gated off and the list +empty, the block's tail omits `trustedOrigins` from the better-auth config +entirely. That is not an absent policy. Measured against the installed +better-auth 1.7.1: `getTrustedOrigins` +(`dist/context/helpers.mjs`) unconditionally seeds the trusted set from the +resolved `baseURL` origin and treats `options.trustedOrigins` as purely +**additive**, so an omitted key and an empty array are equivalent — both leave +exactly the deployment's own origin trusted, and `validateOrigin` +(`dist/api/middlewares/origin-check.mjs`) refuses everything else with +`403 INVALID_ORIGIN`. + +**Who is affected.** Deployments with an explicitly configured `trustedOrigins`, +or one derived from `OS_CORS_ORIGIN`, are unchanged in production — the +substitution never fired for them. Non-production behaviour is unchanged, +including under `NODE_ENV=test` and when `NODE_ENV` is unset. A production +deployment that was relying on the substitution to reach its own login page +now receives a loud `403` rather than silent over-trust; the remedy is to set +`OS_TRUSTED_ORIGINS`, or to fix the base URL that resolved unusable (PR #10369's +boot diagnostic already names that condition at startup). + +Both existing pins keep their dev-only assertions verbatim; new pins cover the +production omission, the non-production legs, the SSO per-request-function +shape, and — load-bearing — that explicitly configured and `OS_CORS_ORIGIN`-derived +trust survives in production. diff --git a/packages/plugins/plugin-auth/src/auth-manager.test.ts b/packages/plugins/plugin-auth/src/auth-manager.test.ts index e50da10dd1..8db4c64313 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.test.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.test.ts @@ -1315,6 +1315,146 @@ describe('AuthManager', () => { }); }); + // #10366 — the localhost-wildcard trio is a DEVELOPMENT convenience and is + // gated on `NODE_ENV !== 'production'`. Before the gate the condition tested + // only emptiness, so a production deployment whose trusted-origin list + // resolved empty silently CSRF-trusted every `localhost` / `*.localhost` + // origin. These pins enforce the boundary in BOTH directions: production + // must not substitute, and non-production must still substitute. + describe('trustedOrigins localhost substitution is non-production only (#10366)', () => { + const TRIO = ['http://localhost:*', 'http://*.localhost:*', 'https://*.localhost:*']; + + const ENV_KEYS = ['NODE_ENV', 'OS_CORS_ORIGIN', 'CORS_ORIGIN', 'OS_SSO_ENABLED'] as const; + let saved: Record; + + beforeEach(() => { + saved = Object.fromEntries(ENV_KEYS.map(k => [k, process.env[k]])); + for (const k of ENV_KEYS) delete process.env[k]; + }); + + afterEach(() => { + for (const k of ENV_KEYS) { + if (saved[k] === undefined) delete process.env[k]; + else process.env[k] = saved[k]!; + } + }); + + async function captureConfig(config: Record): Promise { + let capturedConfig: any; + (betterAuth as any).mockImplementation((c: any) => { + capturedConfig = c; + return { handler: vi.fn(), api: {} }; + }); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const manager = new AuthManager({ + secret: 'test-secret-at-least-32-chars-long', + baseUrl: 'https://app.example.com', + ...config, + } as any); + await manager.getAuthInstance(); + warnSpy.mockRestore(); + return capturedConfig; + } + + describe('production does not substitute', () => { + it('omits the trustedOrigins key entirely when none is provided', async () => { + process.env.NODE_ENV = 'production'; + const cfg = await captureConfig({}); + + expect(cfg.trustedOrigins).toBeUndefined(); + // Absent, not merely empty — the key must not appear at all, which is + // the shape better-auth receives. + expect('trustedOrigins' in cfg).toBe(false); + }); + + it('omits the trustedOrigins key entirely when an empty array is provided', async () => { + process.env.NODE_ENV = 'production'; + const cfg = await captureConfig({ trustedOrigins: [] }); + + expect(cfg.trustedOrigins).toBeUndefined(); + expect('trustedOrigins' in cfg).toBe(false); + }); + }); + + describe('non-production still substitutes', () => { + it("substitutes the trio under NODE_ENV='development'", async () => { + process.env.NODE_ENV = 'development'; + const cfg = await captureConfig({}); + + expect(cfg.trustedOrigins).toEqual(TRIO); + }); + + // Guards against "hardening" the predicate to `=== 'development'`, which + // would be a STRICTER boundary than ruled and would break `test` and + // unset-NODE_ENV development flows. + it("substitutes the trio under NODE_ENV='test'", async () => { + process.env.NODE_ENV = 'test'; + const cfg = await captureConfig({}); + + expect(cfg.trustedOrigins).toEqual(TRIO); + }); + + it('substitutes the trio when NODE_ENV is unset', async () => { + delete process.env.NODE_ENV; + const cfg = await captureConfig({}); + + expect(cfg.trustedOrigins).toEqual(TRIO); + }); + }); + + // LOAD-BEARING: without these two legs, a change that broke ALL origin + // trust in production would still pass a substitution-only suite. + describe('production leaves real configured trust intact', () => { + it('forwards an explicitly configured trustedOrigins list unchanged', async () => { + process.env.NODE_ENV = 'production'; + const cfg = await captureConfig({ + trustedOrigins: ['https://app.example.com', 'https://*.example.com'], + }); + + expect(cfg.trustedOrigins).toEqual([ + 'https://app.example.com', + 'https://*.example.com', + ]); + }); + + it('forwards an OS_CORS_ORIGIN-derived list unchanged', async () => { + process.env.NODE_ENV = 'production'; + process.env.OS_CORS_ORIGIN = 'https://app.example.com,https://admin.example.com'; + const cfg = await captureConfig({}); + + expect(cfg.trustedOrigins).toEqual([ + 'https://app.example.com', + 'https://admin.example.com', + ]); + }); + }); + + // The SSO branch returns `trustedOrigins` as a per-request FUNCTION built + // from a copy of the same `origins` array, so gating the push at the source + // covers this shape too. That is true today and is exactly the kind of + // coupling a future refactor breaks silently — pin it. + describe('SSO per-request function shape', () => { + it('production: the resolved list contains no localhost wildcard', async () => { + process.env.NODE_ENV = 'production'; + const cfg = await captureConfig({ plugins: { sso: true } }); + + expect(typeof cfg.trustedOrigins).toBe('function'); + const resolved: string[] = await cfg.trustedOrigins(undefined); + for (const entry of TRIO) expect(resolved).not.toContain(entry); + expect(resolved.some(o => o.includes('localhost'))).toBe(false); + }); + + it('non-production: the resolved list still contains the trio', async () => { + process.env.NODE_ENV = 'development'; + const cfg = await captureConfig({ plugins: { sso: true } }); + + expect(typeof cfg.trustedOrigins).toBe('function'); + const resolved: string[] = await cfg.trustedOrigins(undefined); + for (const entry of TRIO) expect(resolved).toContain(entry); + }); + }); + }); + describe('setRuntimeBaseUrl', () => { it('should update baseURL before auth instance is created', async () => { let capturedConfig: any; diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 2e63bcada1..d6bdba962a 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -1905,7 +1905,28 @@ export class AuthManager { // `*.localhost` subdomains so per-project tenant subdomains (the dev // default root domain — see project-provisioning.ts) pass CSRF checks // without operators having to configure trustedOrigins manually. - if (!origins.length && (!corsOrigin || corsOrigin === '*')) { + // + // NON-PRODUCTION ONLY (#10366). This substitution is a development + // convenience and is now gated on the same `NODE_ENV` dev signal used + // by the fallback auth secret and the dev Origin synthesis below, so + // the boundary this comment claims is the boundary that is enforced. + // Previously the condition tested only emptiness, so a production + // deployment whose trusted-origin list resolved empty silently + // CSRF-trusted every `localhost` / `*.localhost` origin. + // + // What production gets instead: `trustedOrigins` is omitted from the + // better-auth config entirely (see the tail of this block). That is + // NOT an absent policy — better-auth seeds its trusted set from the + // resolved `baseURL` origin and treats `trustedOrigins` as purely + // ADDITIVE, so an empty list and an omitted key are equivalent and + // both leave exactly the deployment's own origin trusted; every other + // origin is refused with `403 INVALID_ORIGIN`. Measured against + // better-auth 1.7.1 (`getTrustedOrigins` in `dist/context/helpers.mjs`, + // `validateOrigin` in `dist/api/middlewares/origin-check.mjs`). + if ( + process.env.NODE_ENV !== 'production' && + !origins.length && (!corsOrigin || corsOrigin === '*') + ) { origins.push('http://localhost:*'); origins.push('http://*.localhost:*'); origins.push('https://*.localhost:*');