mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-09-30 12:39:42 +02:00
Merge pull request #224 from christianhaberl/fix/raw-writehead-drops-security-headers
fix(http): raw writeHead routes drop every header the security hook set
This commit is contained in:
@@ -0,0 +1,16 @@
|
||||
---
|
||||
'aicodeman': patch
|
||||
---
|
||||
|
||||
Routes that answer with `reply.raw.writeHead()` no longer drop the headers the
|
||||
security hook set.
|
||||
|
||||
`writeHead` writes straight to the Node response and bypasses Fastify's header
|
||||
store, so everything the `onRequest` hook granted was silently lost — including the
|
||||
`Access-Control-Allow-Origin` it emits for localhost origins, and the
|
||||
`X-Content-Type-Options` / `X-Frame-Options` / CSP headers. A localhost page could
|
||||
therefore call every other `/api` endpoint cross-origin while its EventSource
|
||||
failed CORS.
|
||||
|
||||
Affects `GET /api/events` and the three raw-writing routes in `file-routes.ts`
|
||||
(`file-raw`, `tail-file`, `download`).
|
||||
@@ -640,6 +640,25 @@ async function buildExternalAttachmentRouteItem(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Headers Fastify already put on the reply, in a shape `writeHead` accepts.
|
||||
*
|
||||
* `reply.raw.writeHead()` writes straight to the Node response and bypasses
|
||||
* Fastify's header store, so anything the security `onRequest` hook granted — CORS
|
||||
* for localhost origins, nosniff, frame-options, CSP — is silently dropped on every
|
||||
* route that answers this way. Spread this first and let the route's own headers
|
||||
* win over it.
|
||||
*/
|
||||
function inheritedHeaders(reply: {
|
||||
getHeaders(): NodeJS.Dict<number | string | string[]>;
|
||||
}): Record<string, number | string | string[]> {
|
||||
const out: Record<string, number | string | string[]> = {};
|
||||
for (const [name, value] of Object.entries(reply.getHeaders())) {
|
||||
if (value !== undefined) out[name] = value;
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & EventPort & ConfigPort): void {
|
||||
// Lazy filesystem listing for the Link Existing and mobile input path pickers.
|
||||
app.get('/api/filesystem/browse', async (req, reply): Promise<ApiResponse<FilesystemBrowseData>> => {
|
||||
@@ -1337,6 +1356,7 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
|
||||
const basename = rawBasename.replace(/["\\\r\n]/g, '_');
|
||||
if (download === 'true' || ext === 'svg') {
|
||||
reply.raw.writeHead(200, {
|
||||
...inheritedHeaders(reply),
|
||||
'Content-Type': ext === 'svg' ? 'application/octet-stream' : mimeTypes[ext] || 'application/octet-stream',
|
||||
'Content-Disposition': `attachment; filename="${basename}"`,
|
||||
'Content-Length': content.length,
|
||||
@@ -1576,6 +1596,7 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
|
||||
|
||||
// Set up SSE headers
|
||||
reply.raw.writeHead(200, {
|
||||
...inheritedHeaders(reply),
|
||||
'Content-Type': 'text/event-stream',
|
||||
'Cache-Control': 'no-cache',
|
||||
Connection: 'keep-alive',
|
||||
@@ -1709,6 +1730,7 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even
|
||||
const content = await fs.readFile(resolvedPath);
|
||||
// Bypass Fastify compression — write directly to raw response
|
||||
reply.raw.writeHead(200, {
|
||||
...inheritedHeaders(reply),
|
||||
'Content-Type': mimeTypes[ext] || 'application/octet-stream',
|
||||
'Content-Disposition': `attachment; filename="${filename}"`,
|
||||
'Content-Length': content.length,
|
||||
|
||||
@@ -805,7 +805,24 @@ export class WebServer extends EventEmitter {
|
||||
const clientId =
|
||||
typeof query.clientId === 'string' && SSE_CLIENT_ID_RE.test(query.clientId) ? query.clientId : undefined;
|
||||
|
||||
// Carry over the headers the security hook already set on this reply.
|
||||
//
|
||||
// writeHead goes straight to the Node response and bypasses Fastify's header
|
||||
// store, so everything the onRequest hook granted is silently dropped —
|
||||
// including the Access-Control-Allow-Origin it emits for localhost origins.
|
||||
// The result is an internal contradiction: a localhost page may call every
|
||||
// /api endpoint cross-origin, but its EventSource fails CORS. The security
|
||||
// headers (nosniff, frame-options, CSP) were lost the same way.
|
||||
//
|
||||
// The other raw-writeHead routes live in file-routes.ts and share a helper;
|
||||
// this one keeps its own copy so the server does not import from a route
|
||||
// module it registers.
|
||||
const inherited: Record<string, number | string | string[]> = {};
|
||||
for (const [name, value] of Object.entries(reply.getHeaders())) {
|
||||
if (value !== undefined) inherited[name] = value;
|
||||
}
|
||||
reply.raw.writeHead(200, {
|
||||
...inherited,
|
||||
'Content-Type': 'text/event-stream',
|
||||
'Cache-Control': 'no-cache',
|
||||
Connection: 'keep-alive',
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
/**
|
||||
* @fileoverview `/api/events` must not lose the headers the security hook set.
|
||||
*
|
||||
* The SSE route answers with `reply.raw.writeHead()`, which writes straight to the
|
||||
* Node response and bypasses Fastify's header store. Everything the `onRequest`
|
||||
* security hook had granted was therefore dropped — including the
|
||||
* `Access-Control-Allow-Origin` it emits for localhost origins. The contradiction is
|
||||
* visible from a browser: a localhost page may call every other `/api` endpoint
|
||||
* cross-origin, but its EventSource fails CORS.
|
||||
*
|
||||
* These tests drive a REAL WebServer. An earlier version asserted against an inline
|
||||
* copy of the hook and the handler, which proved nothing: reverting the fix in
|
||||
* `server.ts` left every test green.
|
||||
*/
|
||||
|
||||
import { afterAll, beforeAll, describe, expect, it } from 'vitest';
|
||||
|
||||
import { WebServer } from '../src/web/server.js';
|
||||
|
||||
const TEST_PORT = 3119;
|
||||
const LOCAL_ORIGIN = 'http://localhost:5173';
|
||||
|
||||
/** Open /api/events, read the response headers, then abort — it never ends on its own. */
|
||||
async function eventsHeaders(baseUrl: string, origin?: string): Promise<Headers> {
|
||||
const controller = new AbortController();
|
||||
const timeout = setTimeout(() => controller.abort(), 2000);
|
||||
try {
|
||||
const res = await fetch(`${baseUrl}/api/events`, {
|
||||
signal: controller.signal,
|
||||
headers: origin ? { Origin: origin } : undefined,
|
||||
});
|
||||
const headers = res.headers;
|
||||
controller.abort(); // stop consuming the stream
|
||||
return headers;
|
||||
} finally {
|
||||
clearTimeout(timeout);
|
||||
}
|
||||
}
|
||||
|
||||
describe('GET /api/events header inheritance', () => {
|
||||
let server: WebServer;
|
||||
let baseUrl: string;
|
||||
|
||||
beforeAll(async () => {
|
||||
server = new WebServer(TEST_PORT, false, true);
|
||||
await server.start();
|
||||
baseUrl = `http://localhost:${TEST_PORT}`;
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await server.stop();
|
||||
}, 60000);
|
||||
|
||||
it('keeps the CORS header the security hook granted a localhost origin', async () => {
|
||||
// The regression: this header is set on the Fastify reply and was then thrown
|
||||
// away by writeHead, so an EventSource from a localhost dev server failed CORS
|
||||
// while every other endpoint worked.
|
||||
const headers = await eventsHeaders(baseUrl, LOCAL_ORIGIN);
|
||||
expect(headers.get('access-control-allow-origin')).toBe(LOCAL_ORIGIN);
|
||||
});
|
||||
|
||||
it('keeps the security headers the hook set', async () => {
|
||||
const headers = await eventsHeaders(baseUrl);
|
||||
expect(headers.get('x-content-type-options')).toBe('nosniff');
|
||||
expect(headers.get('x-frame-options')).toBe('SAMEORIGIN');
|
||||
expect(headers.get('content-security-policy')).toBeTruthy();
|
||||
});
|
||||
|
||||
it('still sets the SSE headers, and they win over anything inherited', async () => {
|
||||
const headers = await eventsHeaders(baseUrl);
|
||||
expect(headers.get('content-type')).toBe('text/event-stream');
|
||||
expect(headers.get('cache-control')).toBe('no-cache');
|
||||
expect(headers.get('x-accel-buffering')).toBe('no');
|
||||
});
|
||||
|
||||
it('grants nothing to a non-localhost origin — the hook decides, not this route', async () => {
|
||||
const headers = await eventsHeaders(baseUrl, 'https://evil.example');
|
||||
expect(headers.get('access-control-allow-origin')).toBeNull();
|
||||
});
|
||||
|
||||
it('matches what a normal JSON endpoint returns for the same origin', async () => {
|
||||
// The point of the fix: /api/events stops being the odd one out.
|
||||
const json = await fetch(`${baseUrl}/api/status`, { headers: { Origin: LOCAL_ORIGIN } });
|
||||
const sse = await eventsHeaders(baseUrl, LOCAL_ORIGIN);
|
||||
|
||||
expect(sse.get('access-control-allow-origin')).toBe(json.headers.get('access-control-allow-origin'));
|
||||
expect(sse.get('x-content-type-options')).toBe(json.headers.get('x-content-type-options'));
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user