fix(proxy): forward original encoded request-path instead of decoded …

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;

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 ──────────────────────────────────────

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<number> {
+  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<void>((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<void>((r) => proxy.once('listening', () => r()));
+    proxyPort = (proxy.address() as AddressInfo).port;
+  });
+
+  afterAll(async () => {
+    await new Promise<void>((r) => (proxy ? proxy.close(() => r()) : r()));
+    await new Promise<void>((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();
+  });
+});