From ed49ed6165f33426064eb3e1d04f59b93eec0419 Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 17 May 2026 20:11:44 -0400 Subject: [PATCH] fix(http): strip zstd from Accept-Encoding so compression sets Content-Encoding (#1092) compression@1.8.1's negotiator silently fails to set the Content-Encoding response header when Accept-Encoding carries an unknown token like zstd, even though the body is still compressed with brotli. Chromium 123+ sends "Accept-Encoding: zstd, gzip, br" by default, including Playwright's bundled Chromium over plain HTTP, so any homelab Sencho viewed without a TLS terminator triggered ERR_CONTENT_DECODING_FAILED in the browser and rendered as a blank page. Insert a tiny middleware (step 4 of the canonical pipeline) that drops zstd tokens from Accept-Encoding before compression's negotiator runs. The negotiator then picks br or gzip and writes the matching header. The fix is no-op when zstd is absent. If only zstd was offered, the header falls back to identity so the response is served uncompressed. Renumber the canonical-order JSDoc in app.ts from 17 to 18 steps and update the step-number references in hub-only-guard and proxy-mount-order test docstrings to match. 11 new tests (8 unit + 3 supertest integration against real compression with a 3000-byte body) lock in the behavior: zstd-stripping across casing and q-values, surviving tokens preserved verbatim, identity fallback when only zstd was offered, Content-Encoding header always written when at least one supported codec remains. --- backend/src/__tests__/hub-only-guard.test.ts | 4 +- .../normalize-accept-encoding.test.ts | 120 ++++++++++++++++++ .../src/__tests__/proxy-mount-order.test.ts | 2 +- backend/src/app.ts | 51 ++++---- .../src/middleware/normalizeAcceptEncoding.ts | 24 ++++ 5 files changed, 176 insertions(+), 25 deletions(-) create mode 100644 backend/src/__tests__/normalize-accept-encoding.test.ts create mode 100644 backend/src/middleware/normalizeAcceptEncoding.ts diff --git a/backend/src/__tests__/hub-only-guard.test.ts b/backend/src/__tests__/hub-only-guard.test.ts index e236b051..caee6e5c 100644 --- a/backend/src/__tests__/hub-only-guard.test.ts +++ b/backend/src/__tests__/hub-only-guard.test.ts @@ -9,8 +9,8 @@ * on a remote instance, crossing a node-authority boundary that the UI * promises will not happen. * - * The guard sits between `enforceApiTokenScope` (step 12) and - * `createRemoteProxyMiddleware` (step 14). + * The guard sits between `enforceApiTokenScope` (step 13) and + * `createRemoteProxyMiddleware` (step 15). */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import request from 'supertest'; diff --git a/backend/src/__tests__/normalize-accept-encoding.test.ts b/backend/src/__tests__/normalize-accept-encoding.test.ts new file mode 100644 index 00000000..a24dda51 --- /dev/null +++ b/backend/src/__tests__/normalize-accept-encoding.test.ts @@ -0,0 +1,120 @@ +/** + * Regression guard for `normalizeAcceptEncoding`. + * + * Background: `compression@1.8.1` (via `negotiator@1.0.0`) misbehaves when + * `Accept-Encoding` carries an unknown token such as `zstd`. The library + * compresses the response body but fails to set a `Content-Encoding` header, + * so the browser decode-fails with `ERR_CONTENT_DECODING_FAILED` and the + * page renders blank. Chromium 123+ sends `Accept-Encoding: zstd, gzip, br` + * by default, including Playwright's bundled Chromium even on plain HTTP. + * + * The middleware drops `zstd` tokens before `compression` negotiates so the + * negotiator falls back to `br` or `gzip` and writes the matching response + * header. These tests cover the unit-level transform and an end-to-end pass + * through `compression` + a route that returns a 2 KB body (above the + * compression threshold). + */ +import { describe, it, expect } from 'vitest'; +import express, { type Request, type Response } from 'express'; +import compression from 'compression'; +import request from 'supertest'; +import { normalizeAcceptEncoding } from '../middleware/normalizeAcceptEncoding'; + +function runNormalize(accept: string | undefined): string | undefined { + const headers: Record = {}; + if (accept !== undefined) headers['accept-encoding'] = accept; + const req = { headers } as unknown as Parameters[0]; + const res = {} as Parameters[1]; + normalizeAcceptEncoding(req, res, () => { /* noop */ }); + const after = req.headers['accept-encoding']; + return typeof after === 'string' ? after : undefined; +} + +function makeApp(): express.Express { + const app = express(); + app.use(normalizeAcceptEncoding); + app.use(compression()); + // Payload large enough to be compressed (default threshold is 1 KB). + const body = 'abcdefghij'.repeat(300); + app.get('/text', (_req: Request, res: Response) => { + res.type('text/plain').send(body); + }); + return app; +} + +describe('normalizeAcceptEncoding (unit)', () => { + it('passes through when Accept-Encoding is missing', () => { + expect(runNormalize(undefined)).toBeUndefined(); + }); + + it('passes through gzip/deflate/br untouched', () => { + expect(runNormalize('gzip')).toBe('gzip'); + expect(runNormalize('gzip, deflate, br')).toBe('gzip, deflate, br'); + expect(runNormalize('br;q=0.5, gzip;q=1.0')).toBe('br;q=0.5, gzip;q=1.0'); + }); + + it('strips a bare zstd token while preserving order', () => { + expect(runNormalize('zstd, gzip, br')).toBe('gzip, br'); + expect(runNormalize('gzip, zstd, br')).toBe('gzip, br'); + expect(runNormalize('gzip, br, zstd')).toBe('gzip, br'); + }); + + it('strips zstd with a q-value', () => { + expect(runNormalize('zstd;q=1.0, gzip;q=0.8')).toBe('gzip;q=0.8'); + expect(runNormalize('gzip, zstd;q=0.5, br')).toBe('gzip, br'); + }); + + it('strips zstd case-insensitively', () => { + expect(runNormalize('ZSTD, gzip')).toBe('gzip'); + expect(runNormalize('Zstd;q=1.0, br')).toBe('br'); + }); + + it('falls back to identity when only zstd was offered', () => { + expect(runNormalize('zstd')).toBe('identity'); + expect(runNormalize('zstd;q=1.0')).toBe('identity'); + }); + + it('leaves the wildcard token alone', () => { + expect(runNormalize('*')).toBe('*'); + }); + + it('does not strip tokens that merely contain "zstd" as a substring', () => { + // Should not match the zstd-only regex even though it starts with "zstd-". + expect(runNormalize('zstd-future, gzip')).toBe('zstd-future, gzip'); + }); +}); + +describe('normalizeAcceptEncoding (integration with compression)', () => { + // supertest (via superagent) auto-decompresses gzip/br response bodies, so + // these tests assert the Content-Encoding response header and that the + // decoded body matches the original payload. The header is what the browser + // checks to decide whether to decompress; the F-3 bug was specifically that + // it was absent. + + it('sets Content-Encoding (br or gzip) when zstd, gzip, br is offered', async () => { + const res = await request(makeApp()) + .get('/text') + .set('Accept-Encoding', 'zstd, gzip, br'); + expect(res.status).toBe(200); + expect(['br', 'gzip']).toContain(res.headers['content-encoding']); + expect(res.text.startsWith('abcdefghij')).toBe(true); + }); + + it('still serves gzip when only gzip is offered (regression baseline)', async () => { + const res = await request(makeApp()) + .get('/text') + .set('Accept-Encoding', 'gzip'); + expect(res.status).toBe(200); + expect(res.headers['content-encoding']).toBe('gzip'); + expect(res.text.startsWith('abcdefghij')).toBe(true); + }); + + it('omits Content-Encoding when only zstd is offered (identity fallback)', async () => { + const res = await request(makeApp()) + .get('/text') + .set('Accept-Encoding', 'zstd'); + expect(res.status).toBe(200); + expect(res.headers['content-encoding']).toBeUndefined(); + expect(res.text.startsWith('abcdefghij')).toBe(true); + }); +}); diff --git a/backend/src/__tests__/proxy-mount-order.test.ts b/backend/src/__tests__/proxy-mount-order.test.ts index 28684bb7..804b5e87 100644 --- a/backend/src/__tests__/proxy-mount-order.test.ts +++ b/backend/src/__tests__/proxy-mount-order.test.ts @@ -2,7 +2,7 @@ * Regression guard for `createRemoteProxyMiddleware` mount order. * * `index.ts` mounts every local `/api/` router before the remote proxy - * at step 13 of the canonical middleware order. A remote-`nodeId` request + * at step 15 of the canonical middleware order. A remote-`nodeId` request * must short-circuit into the proxy rather than match a local router. If the * order were reversed, a GET `/api/stacks` for a remote node would return the * control instance's local stack list instead of the remote's. diff --git a/backend/src/app.ts b/backend/src/app.ts index 808354a9..d0ca05e5 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -6,35 +6,37 @@ import helmet from 'helmet'; import { globalApiLimiter, pollingLimiter } from './middleware/rateLimiters'; import { conditionalJsonParser } from './middleware/jsonParser'; import { nodeContextMiddleware } from './middleware/nodeContext'; +import { normalizeAcceptEncoding } from './middleware/normalizeAcceptEncoding'; import './types/express'; /** * Build an Express app with the full middleware pipeline installed. * - * Canonical middleware order (17 steps). Do not reorder without re-running the + * Canonical middleware order (18 steps). Do not reorder without re-running the * regression checklist in `docs/internal/architecture/middleware-order.md`. * * 1. trust proxy * 2. helmet * 3. cors - * 4. compression - * 5. cookieParser - * 6. globalApiLimiter (at /api) - * 7. pollingLimiter (at /api) - * 8. conditionalJsonParser - * 9. nodeContextMiddleware - * 10. authGate (at /api) -- registered in index.ts - * 11. auditLog (at /api) -- registered in index.ts - * 12. enforceApiTokenScope (at /api) -- registered in index.ts - * 13. hubOnlyGuard (at /api) -- middleware/hubOnlyGuard.ts, registered in index.ts - * 14. createRemoteProxyMiddleware -- proxy/remoteNodeProxy.ts, registered in index.ts - * 15. routes -- registered in index.ts from routes/* - * 16. static serving + SPA fallback -- registered in index.ts - * 17. errorHandler -- registered in index.ts + * 4. normalizeAcceptEncoding + * 5. compression + * 6. cookieParser + * 7. globalApiLimiter (at /api) + * 8. pollingLimiter (at /api) + * 9. conditionalJsonParser + * 10. nodeContextMiddleware + * 11. authGate (at /api) -- registered in index.ts + * 12. auditLog (at /api) -- registered in index.ts + * 13. enforceApiTokenScope (at /api) -- registered in index.ts + * 14. hubOnlyGuard (at /api) -- middleware/hubOnlyGuard.ts, registered in index.ts + * 15. createRemoteProxyMiddleware -- proxy/remoteNodeProxy.ts, registered in index.ts + * 16. routes -- registered in index.ts from routes/* + * 17. static serving + SPA fallback -- registered in index.ts + * 18. errorHandler -- registered in index.ts * - * Steps 10 to 13 and 15 must run after the public auth routers (meta, auth, + * Steps 11 to 14 and 16 must run after the public auth routers (meta, auth, * mfa, sso) are registered so those routes stay reachable without a session - * cookie. index.ts mounts those public routers before step 10 to preserve + * cookie. index.ts mounts those public routers before step 11 to preserve * that invariant. */ export function createApp(): express.Express { @@ -99,7 +101,12 @@ export function createApp(): express.Express { credentials: true, })); - // 4. Compression. SSE streams (Content-Type: text/event-stream) MUST NOT be + // 4. Drop unknown Accept-Encoding tokens (e.g. `zstd` from Chromium 123+) + // before compression negotiates. See middleware/normalizeAcceptEncoding.ts + // for the symptom this prevents. + app.use(normalizeAcceptEncoding); + + // 5. Compression. SSE streams (Content-Type: text/event-stream) MUST NOT be // compressed because compression buffers output and would delay event delivery // until a flush, breaking live log and status streams. app.use(compression({ @@ -112,18 +119,18 @@ export function createApp(): express.Express { }, })); - // 5. Cookie parser must run before the rate limiters so the hybrid key + // 6. Cookie parser must run before the rate limiters so the hybrid key // generator can read req.cookies for per-user rate limit bucketing. app.use(cookieParser()); - // 6-7. Tiered rate limiting (see middleware/rateLimiters.ts for the model). + // 7-8. Tiered rate limiting (see middleware/rateLimiters.ts for the model). app.use('/api/', globalApiLimiter); app.use('/api/', pollingLimiter); - // 8. Parse JSON on local requests; preserve the raw stream for remote proxy. + // 9. Parse JSON on local requests; preserve the raw stream for remote proxy. app.use(conditionalJsonParser); - // 9. Resolve req.nodeId and short-circuit requests to deleted nodes. + // 10. Resolve req.nodeId and short-circuit requests to deleted nodes. app.use(nodeContextMiddleware); return app; diff --git a/backend/src/middleware/normalizeAcceptEncoding.ts b/backend/src/middleware/normalizeAcceptEncoding.ts new file mode 100644 index 00000000..6ec6ffda --- /dev/null +++ b/backend/src/middleware/normalizeAcceptEncoding.ts @@ -0,0 +1,24 @@ +import type { RequestHandler } from 'express'; + +// compression@1.8.1's encoding negotiation (via negotiator@1.0.0) silently +// breaks when Accept-Encoding carries an unknown token such as `zstd`, which +// Chromium 123+ sends by default and which Playwright's bundled Chromium sends +// even over plain HTTP. In that path the response body is still compressed +// (brotli) but the Content-Encoding response header is never set, so the +// browser decode-fails with ERR_CONTENT_DECODING_FAILED and the Sencho UI +// renders blank. Drop the offending token before compression sees it so the +// negotiator falls back to a supported encoding and the header is written. +export const normalizeAcceptEncoding: RequestHandler = (req, _res, next) => { + const raw = req.headers['accept-encoding']; + if (typeof raw !== 'string' || !/\bzstd\b/i.test(raw)) { + next(); + return; + } + const filtered = raw + .split(',') + .map((s) => s.trim()) + .filter((s) => s.length > 0 && !/^zstd(\s*;\s*q\s*=.*)?$/i.test(s)) + .join(', '); + req.headers['accept-encoding'] = filtered || 'identity'; + next(); +};