diff --git a/lib/__tests__/proxy-security-headers.test.ts b/lib/__tests__/proxy-security-headers.test.ts index ef99e435c..c5acf3844 100644 --- a/lib/__tests__/proxy-security-headers.test.ts +++ b/lib/__tests__/proxy-security-headers.test.ts @@ -191,6 +191,44 @@ describe('frame-ancestors of the mail UI', () => { }); }); +// A same-site sibling host can plant the unprefixed Sidebar Apps cookie; only +// the __Host- one is trusted over HTTPS. Behind a TLS proxy that sends no +// X-Forwarded-Proto the request looks like plain HTTP, so COOKIE_SECURE=true +// has to count as HTTPS here too (GHSA-cjm9-m826-mh78). +describe('Sidebar Apps frame-src cookie', () => { + afterEach(() => vi.unstubAllEnvs()); + + const frameSrcWith = async (url: string, cookie: string, extra: Record = {}) => { + const res = await proxy(new NextRequest(url + '/en/mail', { + headers: { 'sec-fetch-dest': 'document', 'sec-fetch-mode': 'navigate', 'sec-fetch-site': 'none', cookie, ...extra }, + })); + return /frame-src [^;]*/.exec(res.headers.get('content-security-policy') ?? '')?.[0]; + }; + const planted = 'bulwark_app_frame_origins=https%3A%2F%2Fevil.example'; + + it('trusts the unprefixed cookie on plain HTTP', async () => { + expect(await frameSrcWith('http://localhost:3000', planted)).toContain('https://evil.example'); + }); + + it('ignores the unprefixed cookie when COOKIE_SECURE=true, even without X-Forwarded-Proto', async () => { + vi.stubEnv('COOKIE_SECURE', 'true'); + expect(await frameSrcWith('http://webmail:3000', planted)).not.toContain('evil.example'); + expect(await frameSrcWith('http://webmail:3000', '__Host-bulwark_app_frame_origins=https%3A%2F%2Fboard.example')) + .toContain('https://board.example'); + }); + + it('ignores the unprefixed cookie behind a chain of proxies that starts with https', async () => { + expect(await frameSrcWith('http://webmail:3000', planted, { 'x-forwarded-proto': 'https, http' })) + .not.toContain('evil.example'); + }); + + it('sends HSTS when COOKIE_SECURE=true says the site is served over TLS', async () => { + vi.stubEnv('COOKIE_SECURE', 'true'); + const res = await load('/en/mail'); + expect(res.headers.get('strict-transport-security')).toBe('max-age=63072000'); + }); +}); + describe('isStaticAssetPath', () => { it('matches only root-level files and the public/ asset folders', () => { for (const path of STATIC_ASSETS) expect(isStaticAssetPath(path), path).toBe(true); diff --git a/lib/__tests__/setup-session-cookie.test.ts b/lib/__tests__/setup-session-cookie.test.ts new file mode 100644 index 000000000..4b81d9f10 --- /dev/null +++ b/lib/__tests__/setup-session-cookie.test.ts @@ -0,0 +1,25 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { NextRequest } from 'next/server'; +import { buildSessionCookieAttributes } from '@/lib/setup/session'; + +// The wizard cookie carries the setup token. Like the admin cookie, its Secure +// flag follows the request unless COOKIE_SECURE says otherwise: behind a TLS +// proxy that sends no X-Forwarded-Proto the request looks like plain HTTP. +describe('setup wizard cookie Secure flag', () => { + afterEach(() => vi.unstubAllEnvs()); + + const secureFor = (url: string, headers: Record = {}) => + buildSessionCookieAttributes(new NextRequest(url, { headers })).secure; + + it('follows the request', () => { + expect(secureFor('http://app:3000/api/setup/token', { 'x-forwarded-proto': 'https' })).toBe(true); + expect(secureFor('http://app:3000/api/setup/token')).toBe(false); + }); + + it('lets COOKIE_SECURE win over the request', () => { + vi.stubEnv('COOKIE_SECURE', 'true'); + expect(secureFor('http://app:3000/api/setup/token')).toBe(true); + vi.stubEnv('COOKIE_SECURE', 'false'); + expect(secureFor('http://app:3000/api/setup/token', { 'x-forwarded-proto': 'https' })).toBe(false); + }); +}); diff --git a/lib/admin/session.ts b/lib/admin/session.ts index 1e65b5fcd..5f4bd0536 100644 --- a/lib/admin/session.ts +++ b/lib/admin/session.ts @@ -1,5 +1,5 @@ import { cookies } from 'next/headers'; -import { isHttpsRequest } from '@/lib/security/request-protocol'; +import { secureCookieFlag } from '@/lib/security/request-protocol'; import { NextResponse } from 'next/server'; import { createCipheriv, createDecipheriv, randomBytes, createHash } from 'node:crypto'; import { getSessionSecret } from '@/lib/auth/session-secret'; @@ -130,9 +130,7 @@ export async function setAdminSessionCookie(request?: Request): Promise { const cookieStore = await cookies(); cookieStore.set(ADMIN_SESSION_COOKIE, token, { httpOnly: true, - secure: process.env.COOKIE_SECURE !== undefined - ? process.env.COOKIE_SECURE === 'true' - : request ? isHttpsRequest(request) : process.env.NODE_ENV === 'production', + secure: secureCookieFlag(request), sameSite: 'lax', path: '/', maxAge: getSessionTTL(), diff --git a/lib/security/request-protocol.ts b/lib/security/request-protocol.ts index 7a8f3bf0b..f771a7c8a 100644 --- a/lib/security/request-protocol.ts +++ b/lib/security/request-protocol.ts @@ -18,3 +18,24 @@ export function isHttpsRequest(request: Request): boolean { return false; } } + +/** + * Whether the site is served over HTTPS, for decisions a missing proxy header + * must not weaken: which cookie name to trust, whether to send HSTS. A TLS + * proxy that sends no X-Forwarded-Proto makes every request look like plain + * HTTP, so COOKIE_SECURE=true counts as the operator saying it is HTTPS. + */ +export function isServedOverHttps(request: Request): boolean { + return process.env.COOKIE_SECURE === 'true' || isHttpsRequest(request); +} + +/** + * The `Secure` flag for a cookie the server sets. An explicit COOKIE_SECURE + * wins, as it does for the mailbox cookies; otherwise it follows the request, + * because the browser drops Secure cookies on plain HTTP. Without a request + * the production default stands. + */ +export function secureCookieFlag(request?: Request): boolean { + if (process.env.COOKIE_SECURE !== undefined) return process.env.COOKIE_SECURE === 'true'; + return request ? isHttpsRequest(request) : process.env.NODE_ENV === 'production'; +} diff --git a/lib/setup/session.ts b/lib/setup/session.ts index c63df439c..091a995e0 100644 --- a/lib/setup/session.ts +++ b/lib/setup/session.ts @@ -1,7 +1,7 @@ import { cookies } from 'next/headers'; import type { NextRequest } from 'next/server'; import { verifySetupToken } from './token'; -import { isHttpsRequest } from '@/lib/security/request-protocol'; +import { secureCookieFlag } from '@/lib/security/request-protocol'; export const SETUP_COOKIE = 'bulwark_setup_token'; const COOKIE_MAX_AGE = 60 * 60; // 1 hour, matches token TTL @@ -28,12 +28,12 @@ export function buildSessionCookieAttributes(request?: NextRequest) { // on plain HTTP, so unconditionally setting Secure in production breaks // setup over HTTP - the operator gets "Wizard session required" on every // step. The wizard surfaces a cleartext-credentials warning in the UI when - // HTTPS isn't in use. + // HTTPS isn't in use. COOKIE_SECURE wins, as for the admin cookie. return { name: SETUP_COOKIE, httpOnly: true, sameSite: 'lax' as const, - secure: request ? isHttpsRequest(request) : process.env.NODE_ENV === 'production', + secure: secureCookieFlag(request), path: '/', maxAge: COOKIE_MAX_AGE, }; diff --git a/proxy.ts b/proxy.ts index fce4ef7b9..0eb28ab76 100644 --- a/proxy.ts +++ b/proxy.ts @@ -12,6 +12,7 @@ import { import { configManager } from "./lib/admin/config-manager"; import { getWopiEditorOrigins } from "./lib/wopi/discovery"; import { detectSetupState } from "./lib/setup/state"; +import { isServedOverHttps } from "./lib/security/request-protocol"; const intlMiddleware = createIntlMiddleware(routing); @@ -307,7 +308,7 @@ export async function proxy(request: NextRequest) { const appFrameOrigins = sidebarAppsEnabled ? parseAppFrameOrigins(pickAppFrameOriginsCookie( (name) => request.cookies.get(name)?.value, - request.nextUrl.protocol === "https:" || request.headers.get("x-forwarded-proto") === "https", + isServedOverHttps(request), )) : []; @@ -412,11 +413,12 @@ export async function proxy(request: NextRequest) { response.headers.set("Referrer-Policy", "strict-origin-when-cross-origin"); response.headers.set("X-XSS-Protection", "0"); - // Only on a request that arrived over TLS (directly or via the reverse - // proxy's X-Forwarded-Proto): a plain-http dev or LAN install must not be - // pinned to https by a header it cannot honour. No includeSubDomains, which - // would speak for the operator's other hosts. - if (request.nextUrl.protocol === "https:" || request.headers.get("x-forwarded-proto") === "https") { + // Only on a request that arrived over TLS (directly, via the reverse + // proxy's X-Forwarded-Proto, or as COOKIE_SECURE=true declares): a + // plain-http dev or LAN install must not be pinned to https by a header it + // cannot honour. No includeSubDomains, which would speak for the operator's + // other hosts. + if (isServedOverHttps(request)) { response.headers.set("Strict-Transport-Security", "max-age=63072000"); } response.headers.set(