diff --git a/gateway/src/proxy.ts b/gateway/src/proxy.ts index a160779..e5c528e 100644 --- a/gateway/src/proxy.ts +++ b/gateway/src/proxy.ts @@ -103,6 +103,26 @@ function rejectSocket(socket: stream.Duplex, status: number, blockStatus: string socket.end(); } +// Node valideert request-opties synchroon in de ClientRequest-constructor +// (bv. ERR_UNESCAPED_CHARACTERS bij een ongeldige request-target). Zo'n throw +// belandt anders in de uncaughtException-handler die het hele proces — en dus +// élke huddle — neerhaalt. Fail per request (400), niet per proces. +function tryCreateUpstreamRequest( + create: () => http.ClientRequest, + res: http.ServerResponse, + complete: (resStatus: number | null) => void, +): http.ClientRequest | null { + try { + return create(); + } catch (err: any) { + const body = JSON.stringify({ error: 'bad_request', message: `cannot forward request: ${err.message}` }); + res.writeHead(400, { 'content-type': 'application/json', 'content-length': Buffer.byteLength(body) }); + res.end(body); + complete(400); + return null; + } +} + // Buffers en scrubt de OAuth token-exchange response zodat het echte access_token // nooit in de audit-log terechtkomt. Stuurt de gescrubde response naar innerRes // en roept complete aan met de veilige audit-body als derde argument. @@ -126,8 +146,7 @@ function handleTokenExchangeResponse( // Bind de placeholder aan de aanvragende container (finding #12); geen // 'unknown'-fallback meer. Een null container levert een niet-inwissel- // bare placeholder op (fail-closed). - const placeholder = storeTokenExchange(containerId, json.access_token as string); - json.access_token = placeholder; + json.access_token = storeTokenExchange(containerId, json.access_token as string); console.log(`[token-exchange] placeholder issued voor container ${containerId}`); outBuf = Buffer.from(JSON.stringify(json)); delete outHeaders['content-encoding']; @@ -156,7 +175,9 @@ function handleTokenExchangeResponse( }); } -export function createProxyServer(): http.Server { +// `port` is standaard de vaste proxypoort; tests binden op 0 (een vrije +// efemere poort) zodat het pad-forwardgedrag hermetisch getest kan worden. +export function createProxyServer(port: number = PROXY_PORT): http.Server { const server = http.createServer(); server.on('request', async (req, res) => { @@ -184,9 +205,14 @@ export function createProxyServer(): http.Server { return; } - // Normaliseer het pad naar de vorm die de upstream zal interpreteren en - // forward exact dat pad. `new URL` heeft `../` al weggevouwen; normalizePathname - // dekt daarnaast `%2f`-getruceerde traversal (finding #7) en weigert fail-closed. + // Beslis op de gedecodeerde vorm (normalizePathname, finding #7) maar + // forward de originele encoded bytes van de URL-parser. De gedecodeerde + // vorm is geen geldige request-target: rauwe spaties/UTF-8 laten + // http.request synchroon gooien (ERR_UNESCAPED_CHARACTERS → proces-crash), + // en de upstream zou hem een twééde keer decoden (double-decode- + // differential; verminkt bovendien legitieme %2F/%20). `new URL` heeft + // `../` al weggevouwen; normalizePathname dekt `%2f`-getruceerde traversal + // en weigert fail-closed — er bereiken dus nooit `..`-bytes de upstream. const normPath = normalizePathname(target.pathname); if (normPath === null) { logAudit({ @@ -196,7 +222,7 @@ export function createProxyServer(): http.Server { send403(res, host, 'deny', containerId); return; } - const forwardPath = `${normPath}${target.search}`; + const forwardPath = `${target.pathname}${target.search}`; let ruleId: number | null; if (host === 'huddle') { @@ -270,7 +296,7 @@ export function createProxyServer(): http.Server { // MCP-verkeer naar huddle altijd via de API-poort (3000), niet de proxypoort (80). const upstreamPort = target.port || 80; - const upstream = http.request( + const upstream = tryCreateUpstreamRequest(() => http.request( { hostname: host, port: upstreamPort, @@ -293,7 +319,8 @@ export function createProxyServer(): http.Server { complete(0, upstreamRes.headers); }); } - ); + ), res, complete); + if (!upstream) return; upstream.on('error', (err) => { if (!res.headersSent) send502(res, err.message); @@ -435,20 +462,24 @@ export function createProxyServer(): http.Server { // De CONNECT stond de host al toe (pad was toen versleuteld). Nu de TLS // getermineerd is kennen we het pad: pas padbeleid alsnog toe per request. // - // Normaliseer het pad één keer naar de vorm die de upstream zal - // interpreteren en forward EXACT dat pad — zo kunnen de gecontroleerde en - // de verstuurde bytes niet divergeren (finding #7). Traversal (`../`, - // `..%2f`) of kapotte encoding → fail closed (403), nooit doorsturen. + // Beslis op de gedecodeerde vorm (finding #7): traversal (`../`, `..%2f`) + // of kapotte encoding → fail closed (403), nooit doorsturen. Geforward + // worden daarna de originele encoded bytes (zie rules.ts): de gedecodeerde + // vorm is geen geldige request-target — rauwe spaties/UTF-8 (bv. een + // `%20` in een Azure DevOps-projectnaam) laten https.request synchroon + // gooien (ERR_UNESCAPED_CHARACTERS → proces-crash) — en de upstream zou + // hem een twééde keer decoden, waarmee `%252e%252e` alsnog tot `..` + // vervalt en legitieme %2F/%20 verminkt raken. const rawUrl = innerReq.url ?? '/'; const qi = rawUrl.indexOf('?'); const rawPathPart = qi === -1 ? rawUrl : rawUrl.slice(0, qi); const query = qi === -1 ? '' : rawUrl.slice(qi); const normPath = normalizePathname(rawPathPart); - const forwardUrl = normPath === null ? null : `${normPath}${query}`; + const checkUrl = normPath === null ? null : `${normPath}${query}`; const pathResult = normPath === null ? { status: 'deny' as const, ruleId: null } - : checkRule(hostname, containerId, forwardUrl); + : checkRule(hostname, containerId, checkUrl); // Alles behalve 'allow' blokkeren: een 'deny'-padregel, maar ook een nog // niet beoordeeld subpad ('requested') van een pad-allowlist-domein — // fail-closed tot de operator het pad expliciet toestaat. @@ -543,14 +574,14 @@ export function createProxyServer(): http.Server { }); }; - const upstreamReq = https.request( + const upstreamReq = tryCreateUpstreamRequest(() => https.request( { hostname, port, method: innerReq.method, - // Forward het genormaliseerde pad dat we ook gecontroleerd hebben, niet - // de rauwe (mogelijk traversal-getruceerde) innerReq.url (finding #7). - path: forwardUrl ?? innerReq.url, + // De originele encoded bytes; de gedecodeerde checkUrl is alleen de + // beslisvorm. Traversal is hierboven al fail-closed geweigerd. + path: rawUrl, headers: upstreamHeaders, servername: hostname, }, @@ -573,7 +604,8 @@ export function createProxyServer(): http.Server { }); } }, - ); + ), innerRes, complete); + if (!upstreamReq) return; upstreamReq.on('error', (err) => { if (!innerRes.headersSent) { @@ -599,8 +631,8 @@ export function createProxyServer(): http.Server { clientSocket.on('close', () => { try { innerTls.destroy(); } catch {} }); }); - server.listen(PROXY_PORT, () => { - console.log(`[proxy] listening on :${PROXY_PORT}`); + server.listen(port, () => { + console.log(`[proxy] listening on :${port}`); }); return server; diff --git a/gateway/test/e2e/boundary.e2e.ts b/gateway/test/e2e/boundary.e2e.ts index 84aeca8..cd93e92 100644 --- a/gateway/test/e2e/boundary.e2e.ts +++ b/gateway/test/e2e/boundary.e2e.ts @@ -224,6 +224,41 @@ describe.skipIf(!E2E_ENABLED)('live security boundary', () => { expect(curlStatusIn(E2E_NAME, 'https://example.org/foo/../secret', '--path-as-is')).toBe('403'); expect(curlStatusIn(E2E_NAME, 'https://example.org/foo/..%2f..%2fadmin', '--path-as-is')).toBe('403'); }); + + // Regressie op de finding #7-fix: de beslissing valt op de gedecodeerde + // vorm, maar geforward worden de originele encoded bytes. Werd de + // gedecodeerde vorm geforward, dan gooide http(s).request synchroon + // ERR_UNESCAPED_CHARACTERS op de rauwe spatie (bv. een Azure DevOps- + // projectnaam met %20). De fix heeft een herkenbare handtekening: een + // wélgevormde upstream-status die géén '000', '403' of '400' is — + // • '000' = de gateway ging neer óf de CONNECT werd geweigerd (pre-fix + // crash, vóór de 400-guard bestond), + // • '403' = door Huddle's pad-policy geblokkeerd (mag hier niet: /foo/* + // matcht op de gedecodeerde vorm), + // • '400' = de bad_request-guard vuurt — precies wat er gebeurt als de + // gedecodeerde (rauwe-spatie) vorm wél geforward zou worden en + // http(s).request synchroon gooit. + // Zo pinnen we de fix vast zonder afhankelijk te zijn van de exacte + // upstream-status van example.org (die mag 2xx/3xx/4xx zijn). + const forwardedOk = (status: string) => { + expect(status).toMatch(/^[1-5]\d\d$/); // een echte upstream-HTTP-status, geen '000' + expect(status).not.toBe('403'); // niet door Huddle geblokkeerd + expect(status).not.toBe('400'); // niet de forward-guard: encoded bytes gooien nooit + }; + it('%-encoded pad (%20) wordt geforward en crasht de gateway niet', async () => { + await clearRulesForDomain('example.org'); + const denyId = await createRule('example.org', 'deny'); + await enablePathMode(denyId); + await createRule('example.org', 'allow', { path_pattern: '/foo/*' }); + await sleep(1000); + // MITM-pad (https): matcht /foo/* op de gedecodeerde vorm, bereikt upstream. + forwardedOk(curlStatusIn(E2E_NAME, 'https://example.org/foo/a%20b', '--path-as-is')); + // Plain-HTTP-pad: zelfde invariant. + forwardedOk(curlStatusIn(E2E_NAME, 'http://example.org/foo/a%20b', '--path-as-is')); + // De gateway leeft nog: een gewoon toegestaan subpad krijgt weer een + // wélgevormde upstream-status (geen '000'). + expect(curlStatusIn(E2E_NAME, 'https://example.org/foo/', '--path-as-is')).toMatch(/^[1-5]\d\d$/); + }); }); // ── Huddle self-traffic via de proxy ────────────────────────────────────── diff --git a/gateway/test/proxy-forward-path.test.ts b/gateway/test/proxy-forward-path.test.ts new file mode 100644 index 0000000..de69249 --- /dev/null +++ b/gateway/test/proxy-forward-path.test.ts @@ -0,0 +1,123 @@ +import { describe, it, expect, beforeAll, afterAll, beforeEach, vi } from 'vitest'; +import http from 'http'; +import type { AddressInfo } from 'net'; + +// ── Regressie op de finding #7-fix (#67) ───────────────────────────────────── +// De proxy BESLIST op de gedecodeerde vorm (normalizePathname), maar FORWARDT de +// originele encoded bytes. Werd de gedecodeerde vorm geforward, dan zag de +// upstream '/foo/a b' i.p.v. '/foo/a%20b' — en http.request gooide op de rauwe +// spatie synchroon ERR_UNESCAPED_CHARACTERS, wat vóór de 400-guard het hele +// gateway-proces neerhaalde. Deze suite pint het contract vast zónder Docker of +// een live container: een echte lokale upstream noteert de request-target die +// hij daadwerkelijk terugkrijgt. +// +// better-sqlite3 is een native module; in een DMZ-devcontainer zonder gebouwde +// binding slaan we de suite over (zie rules.test.ts). Probe vóór de db-import. +let sqliteAvailable = true; +try { + const mod = await import('better-sqlite3'); + new mod.default(':memory:').close(); +} catch (e) { + sqliteAvailable = false; + console.warn( + `[proxy-forward-path.test] SKIPPED — better-sqlite3 binding niet bruikbaar: ${(e as Error).message}` + ); +} + +let db: typeof import('../src/db').db; + +let upstream: http.Server; +let upstreamPort = 0; +let lastUpstreamUrl: string | null = null; + +let proxy: http.Server; +let proxyPort = 0; + +// Stuur één request door de proxy als forward-proxy-client: over plain HTTP is +// de request-target absoluut (`GET http://host/pad`). Resolvet met de status die +// de CLIENT ziet — een antwoord bewijst dat de gateway nog leeft. +function proxyGet(pathAndQuery: string): Promise { + return new Promise((resolve, reject) => { + const req = http.request( + { + host: '127.0.0.1', + port: proxyPort, + method: 'GET', + path: `http://127.0.0.1:${upstreamPort}${pathAndQuery}`, + headers: { host: `127.0.0.1:${upstreamPort}` }, + }, + (res) => { + res.resume(); + res.on('end', () => resolve(res.statusCode ?? 0)); + } + ); + req.on('error', reject); + req.end(); + }); +} + +describe.skipIf(!sqliteAvailable)('proxy forwards the original encoded request-path', () => { + beforeAll(async () => { + const dbMod = await import('../src/db'); + db = dbMod.db; + dbMod.initDb(); + + // Geen Docker in de unit-omgeving: de client-IP hoeft niet naar een + // container te resolven — een globale allow-regel volstaat. + const dockerMod = await import('../src/docker'); + vi.spyOn(dockerMod, 'resolveContainerByIp').mockResolvedValue(null); + + upstream = http.createServer((req, res) => { + lastUpstreamUrl = req.url ?? null; + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('ok'); + }); + await new Promise((r) => upstream.listen(0, '127.0.0.1', () => r())); + upstreamPort = (upstream.address() as AddressInfo).port; + + // createProxyServer bindt zelf (poort 0 = vrije efemere poort); wacht op + // 'listening' i.p.v. zelf nog eens listen() aan te roepen. + const { createProxyServer } = await import('../src/proxy'); + proxy = createProxyServer(0); + await new Promise((r) => proxy.once('listening', () => r())); + proxyPort = (proxy.address() as AddressInfo).port; + }); + + afterAll(async () => { + await new Promise((r) => (proxy ? proxy.close(() => r()) : r())); + await new Promise((r) => (upstream ? upstream.close(() => r()) : r())); + }); + + beforeEach(() => { + db.exec('DELETE FROM rules'); + lastUpstreamUrl = null; + // Host-only allow voor de upstream-host: matcht elk pad, zodat de test het + // pad-forwardgedrag isoleert (niet de rule-matching). + db.prepare(`INSERT INTO rules (domain, container_id, status) VALUES ('127.0.0.1', NULL, 'allow')`).run(); + }); + + it('%20 blijft encoded richting upstream (geen rauwe spatie, geen crash)', async () => { + const status = await proxyGet('/foo/a%20b'); + expect(status).toBe(200); + // Cruciaal: de encoded bytes, NIET de gedecodeerde '/foo/a b'. + expect(lastUpstreamUrl).toBe('/foo/a%20b'); + }); + + it('non-ASCII UTF-8 (%E2%9C%93) blijft encoded richting upstream', async () => { + const status = await proxyGet('/foo/%E2%9C%93'); + expect(status).toBe(200); + expect(lastUpstreamUrl).toBe('/foo/%E2%9C%93'); + }); + + it('de query-string wordt behouden en niet gedecodeerd', async () => { + const status = await proxyGet('/foo/bar?q=a%20b&x=1'); + expect(status).toBe(200); + expect(lastUpstreamUrl).toBe('/foo/bar?q=a%20b&x=1'); + }); + + it('traversal (%2f-getruceerd) wordt fail-closed geweigerd en nooit geforward', async () => { + const status = await proxyGet('/foo/..%2f..%2fadmin'); + expect(status).toBe(403); + expect(lastUpstreamUrl).toBeNull(); + }); +});