From 01e9929a1a2152160c48d185d4b19e4ca4561319 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 24 Jul 2026 12:17:00 +0000 Subject: [PATCH] fix: stop double-wrapping Couch Location responses Rewrite absolute same-origin Location headers only in toClientResponse/toClientResponseFromCouch. fetchFromCouch now returns the raw upstream Response so undici no longer intermittently throws "body disturbed or locked" on successful PUT/COPY (client 500). Co-authored-by: Peter Baker --- src/proxy/forward.ts | 61 +++++++++--------- src/routes/actors.ts | 67 ++++++++++---------- test/integration/acl.test.ts | 23 +++++++ test/unit/forward-location.test.ts | 99 ++++++++++++++++++++++++++++++ 4 files changed, 183 insertions(+), 67 deletions(-) create mode 100644 test/unit/forward-location.test.ts diff --git a/src/proxy/forward.ts b/src/proxy/forward.ts index 3fb3fbd..8aa693d 100644 --- a/src/proxy/forward.ts +++ b/src/proxy/forward.ts @@ -77,12 +77,9 @@ export async function forwardToCouch( ): Promise { try { const upstream = await fetchFromCouch(c, config, options); - return toClientResponse(upstream, { + return toClientResponseFromCouch(upstream, config, { keepEncoding: options?.keepEncoding, stripHeaders: options?.stripResponseHeaders, - rewriteLocation: { - fromOrigin: new URL(config.couch.url).origin, - }, }); } catch (err) { if (err instanceof BodyTooLargeError) { @@ -175,41 +172,41 @@ export async function fetchFromCouch( status: response.status, }); } - const location = response.headers.get("location"); - if (!location) return response; + // Return the raw upstream Response. Location rewriting belongs only in + // toClientResponse / toClientResponseFromCouch — wrapping here and again + // there double-attaches the body stream and can throw + // "Response body object should not be disturbed or locked". + return response; +} - // Couch commonly emits absolute redirects using its private upstream - // origin. Exposing that URL can let a client leave the ACL proxy when Couch - // is also reachable on an internal or development network. Preserve - // same-origin redirects as origin-relative client locations. - try { - const target = new URL(location, url); - if (target.origin !== couchBase.origin) return response; - const headers = new Headers(response.headers); - headers.set("Location", `${target.pathname}${target.search}${target.hash}`); - return new Response(response.body, { - status: response.status, - statusText: response.statusText, - headers, - }); - } catch { - return response; - } +export type ClientResponseOptions = { + keepEncoding?: boolean; + body?: ReadableStream | string | null; + stripHeaders?: string[]; + rewriteLocation?: { fromOrigin: string }; +}; + +/** + * Convert an upstream Couch Response into a client Response, rewriting + * absolute same-origin Location headers to path-only form so the private + * Couch origin is never advertised. + */ +export function toClientResponseFromCouch( + upstream: Response, + config: AppConfig, + options?: Omit, +): Response { + return toClientResponse(upstream, { + ...options, + rewriteLocation: { fromOrigin: new URL(config.couch.url).origin }, + }); } /** * Convert an upstream Response into a client Response, stripping hop-by-hop * headers and (by default) content-encoding so Node can re-encode if needed. */ -export function toClientResponse( - upstream: Response, - options?: { - keepEncoding?: boolean; - body?: ReadableStream | string | null; - stripHeaders?: string[]; - rewriteLocation?: { fromOrigin: string }; - }, -): Response { +export function toClientResponse(upstream: Response, options?: ClientResponseOptions): Response { const responseHeaders = new Headers(); const stripHeaders = new Set((options?.stripHeaders ?? []).map((header) => header.toLowerCase())); const decoded = !options?.keepEncoding && upstream.headers.has("content-encoding"); diff --git a/src/routes/actors.ts b/src/routes/actors.ts index 57bcec6..7dbc2b6 100644 --- a/src/routes/actors.ts +++ b/src/routes/actors.ts @@ -22,7 +22,7 @@ import { fetchFromCouch, forwardToCouch, jsonResponse, - toClientResponse, + toClientResponseFromCouch, } from "../proxy/forward.js"; import { filterBulkGet, filterRows, type RowsResponse } from "../proxy/filterRows.js"; import { @@ -610,19 +610,13 @@ export const actors: Record = { newEditsFalse, }); } - return toClientResponse(upstream, { - rewriteLocation: { - fromOrigin: new URL(config.couch.url).origin, - }, - }); + return toClientResponseFromCouch(upstream, config); } if (principal.admin && directDocumentWrite && id) { const config = c.get("config"); const upstream = await fetchFromCouch(c, config); if (upstream.ok) await refreshWrittenDoc(c, state, id, true); - return toClientResponse(upstream, { - rewriteLocation: { fromOrigin: new URL(config.couch.url).origin }, - }); + return toClientResponseFromCouch(upstream, config); } await next(); }, @@ -662,9 +656,10 @@ export const actors: Record = { if (!allowed) { return couchError("forbidden", "ACL", 403); } - const upstream = await fetchFromCouch(c, c.get("config")); + const config = c.get("config"); + const upstream = await fetchFromCouch(c, config); if (upstream.ok) await refreshWrittenDoc(c, state, id, true); - return toClientResponse(upstream); + return toClientResponseFromCouch(upstream, config); }, /** @@ -722,9 +717,10 @@ export const actors: Record = { user: principal.name, flags, }); - const upstream = await fetchFromCouch(c, c.get("config")); + const config = c.get("config"); + const upstream = await fetchFromCouch(c, config); if (upstream.ok) await refreshWrittenDoc(c, state, id, true); - return toClientResponse(upstream); + return toClientResponseFromCouch(upstream, config); }, /** @@ -808,9 +804,7 @@ export const actors: Record = { const config = c.get("config"); const upstream = await fetchFromCouch(c, config); if (upstream.ok) await refreshWrittenDoc(c, state, destId, false); - return toClientResponse(upstream, { - rewriteLocation: { fromOrigin: new URL(config.couch.url).origin }, - }); + return toClientResponseFromCouch(upstream, config); }, /** Proxy then filter `_all_docs` / view rows by read ACL. */ @@ -921,7 +915,7 @@ export const actors: Record = { ...(query != null ? { query } : {}), ...(forwardBody !== undefined ? { body: forwardBody } : {}), }); - if (!upstream.ok) return toClientResponse(upstream); + if (!upstream.ok) return toClientResponseFromCouch(upstream, config); let body: RowsResponse; try { @@ -955,7 +949,7 @@ export const actors: Record = { filteredRows: filtered.rows.length, preserveDenied, }); - const response = toClientResponse(upstream, { + const response = toClientResponseFromCouch(upstream, config, { body: isHead ? null : JSON.stringify(filtered), stripHeaders: ["etag", "last-modified"], }); @@ -990,7 +984,7 @@ export const actors: Record = { const upstream = await fetchFromCouch(c, config, { stripRequestHeaders: ["if-none-match", "if-modified-since"], }); - if (!upstream.ok || !upstream.body) return toClientResponse(upstream); + if (!upstream.ok || !upstream.body) return toClientResponseFromCouch(upstream, config); const filtered = filterChangesStream( upstream.body, @@ -1001,7 +995,7 @@ export const actors: Record = { maxBufferBytes: config.server.maxBodyBytes, }, ); - const response = toClientResponse(upstream, { + const response = toClientResponseFromCouch(upstream, config, { body: filtered, stripHeaders: ["etag", "last-modified"], }); @@ -1070,11 +1064,12 @@ export const actors: Record = { ); } - const upstream = await fetchFromCouch(c, c.get("config"), { + const config = c.get("config"); + const upstream = await fetchFromCouch(c, config, { body: JSON.stringify({ ...filtered.rest, docs: filtered.allowed }), headers: { "Content-Type": "application/json", Accept: "application/json" }, }); - if (!upstream.ok) return toClientResponse(upstream); + if (!upstream.ok) return toClientResponseFromCouch(upstream, config); let results = (await upstream.json()) as Array>; if (!Array.isArray(results)) results = []; const newEditsFalse = String(body.new_edits) === "false"; @@ -1091,7 +1086,7 @@ export const actors: Record = { }); } await refreshWrittenDocs(c, state, writes, newEditsFalse); - return toClientResponse(upstream, { + return toClientResponseFromCouch(upstream, config, { body: JSON.stringify(mergeBulkResults(filtered.slots, results)), }); }, @@ -1134,7 +1129,7 @@ export const actors: Record = { "Content-Type": c.req.header("content-type") || "application/json", }, }); - if (!upstream.ok) return toClientResponse(upstream); + if (!upstream.ok) return toClientResponseFromCouch(upstream, config); let body: { results?: Array<{ id: string; docs: unknown[] }> }; try { body = JSON.parse( @@ -1155,7 +1150,7 @@ export const actors: Record = { requested: body.results?.length ?? 0, results: filtered.results?.length ?? 0, }); - return toClientResponse(upstream, { + return toClientResponseFromCouch(upstream, config, { body: JSON.stringify(filtered), }); }, @@ -1194,11 +1189,12 @@ export const actors: Record = { requestedKeys: Object.keys(body).length, allowedKeys: Object.keys(filtered).length, }); - const upstream = await fetchFromCouch(c, c.get("config"), { + const config = c.get("config"); + const upstream = await fetchFromCouch(c, config, { body: JSON.stringify(filtered), headers: { "Content-Type": "application/json", Accept: "application/json" }, }); - return toClientResponse(upstream); + return toClientResponseFromCouch(upstream, config); }, /** @@ -1207,20 +1203,21 @@ export const actors: Record = { */ async dblist(c) { const principal = c.get("principal"); - if (principal.admin) return forwardToCouch(c, c.get("config")); + const config = c.get("config"); + if (principal.admin) return forwardToCouch(c, config); const isHead = c.req.method === "HEAD"; - const upstream = await fetchFromCouch(c, c.get("config"), { + const upstream = await fetchFromCouch(c, config, { ...(isHead ? { method: "GET" } : {}), stripRequestHeaders: ["if-none-match", "if-modified-since"], headers: { Accept: "application/json" }, }); - if (!upstream.ok) return toClientResponse(upstream); + if (!upstream.ok) return toClientResponseFromCouch(upstream, config); let dbs: string[]; try { dbs = JSON.parse( - await readResponseTextLimited(upstream, c.get("config").server.maxBodyBytes), + await readResponseTextLimited(upstream, config.server.maxBodyBytes), ) as string[]; } catch (err) { if (err instanceof BodyTooLargeError) { @@ -1229,7 +1226,7 @@ export const actors: Record = { throw err; } if (!Array.isArray(dbs)) { - return toClientResponse(upstream, { + return toClientResponseFromCouch(upstream, config, { body: isHead ? null : JSON.stringify(dbs), stripHeaders: ["etag", "last-modified"], }); @@ -1263,7 +1260,7 @@ export const actors: Record = { visibleDbs: visible.length, visible, }); - const response = toClientResponse(upstream, { + const response = toClientResponseFromCouch(upstream, config, { body: isHead ? null : JSON.stringify(visible), stripHeaders: ["etag", "last-modified"], }); @@ -1315,7 +1312,7 @@ export const actors: Record = { "Content-Type": c.req.header("content-type") || "application/json", }, }); - if (!upstream.ok) return toClientResponse(upstream); + if (!upstream.ok) return toClientResponseFromCouch(upstream, config); let body: FindResponse; try { body = JSON.parse( @@ -1340,7 +1337,7 @@ export const actors: Record = { if (injectedId) { filtered.docs = filtered.docs.map(({ _id: _injectedId, ...doc }) => doc); } - return toClientResponse(upstream, { body: JSON.stringify(filtered) }); + return toClientResponseFromCouch(upstream, config, { body: JSON.stringify(filtered) }); }, /** Mango index management — admin only. */ diff --git a/test/integration/acl.test.ts b/test/integration/acl.test.ts index 3b31a43..e77d036 100644 --- a/test/integration/acl.test.ts +++ b/test/integration/acl.test.ts @@ -308,6 +308,29 @@ describe("integration ACL", () => { expect(body.error).toBe("not_found"); }); + it("PUT Location is relative and never leaks the Couch origin", async () => { + const headers = authHeaders("basic", "alice", "alice-pass"); + const put = await putDoc(`loc-probe-${suffix}`, { creator: "alice", hello: true }, headers); + expect(put.status).toBe(201); + const location = put.headers.get("location"); + expect(location).toBeTruthy(); + expect(location!).toMatch(new RegExp(`^/${DB}/loc-probe-${suffix}$`)); + expect(location!.toLowerCase()).not.toContain("couchdb"); + expect(location!).not.toMatch(/^https?:\/\//i); + + // Stress the fetchFromCouch → toClientResponse path; double-wrap races are rare. + for (let i = 0; i < 20; i++) { + const id = `loc-loop-${suffix}-${i}`; + const res = await putDoc(id, { creator: "alice", n: i }, headers); + expect(res.status, await res.text()).toBe(201); + const loc = res.headers.get("location"); + expect(loc).toBeTruthy(); + expect(loc!).not.toMatch(/^https?:\/\//i); + expect(loc!.toLowerCase()).not.toContain("couchdb"); + expect(loc!).toBe(`/${DB}/${id}`); + } + }); + it("COPY requires source read and destination write", async () => { const src = `copy-src-${suffix}`; const dst = `copy-dst-${suffix}`; diff --git a/test/unit/forward-location.test.ts b/test/unit/forward-location.test.ts new file mode 100644 index 0000000..005a0c7 --- /dev/null +++ b/test/unit/forward-location.test.ts @@ -0,0 +1,99 @@ +/** + * Location rewrite must happen once in toClientResponse — not in fetchFromCouch. + * Double-wrapping the upstream body can throw undici's + * "Response body object should not be disturbed or locked". + */ +import { Hono } from "hono"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { loadConfig } from "../../src/config.js"; +import { + fetchFromCouch, + toClientResponse, + toClientResponseFromCouch, +} from "../../src/proxy/forward.js"; + +const COUCH_ORIGIN = "http://couchdb:5984"; + +function testConfig() { + return loadConfig({ + COUCH_URL: COUCH_ORIGIN, + COUCH_ADMIN_URL: "http://admin:password@couchdb:5984", + RATE_LIMIT_ENABLED: "false", + }); +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("toClientResponse Location rewrite", () => { + it("rewrites absolute same-origin Location to a relative path and keeps the body", async () => { + const upstream = new Response(JSON.stringify({ ok: true, id: "doc1", rev: "1-abc" }), { + status: 201, + statusText: "Created", + headers: { + "Content-Type": "application/json", + Location: `${COUCH_ORIGIN}/db/doc1`, + }, + }); + + const client = toClientResponse(upstream, { + rewriteLocation: { fromOrigin: COUCH_ORIGIN }, + }); + + expect(client.status).toBe(201); + expect(client.headers.get("location")).toBe("/db/doc1"); + expect(await client.json()).toEqual({ ok: true, id: "doc1", rev: "1-abc" }); + }); + + it("toClientResponseFromCouch applies Couch-origin rewrite by default", async () => { + const config = testConfig(); + const upstream = new Response("{}", { + status: 201, + headers: { Location: `${COUCH_ORIGIN}/acldemo/rec-1?rev=1-x` }, + }); + + const client = toClientResponseFromCouch(upstream, config); + expect(client.headers.get("location")).toBe("/acldemo/rec-1?rev=1-x"); + expect(await client.text()).toBe("{}"); + }); +}); + +describe("fetchFromCouch Location handling", () => { + it("returns the raw upstream Response without rewriting Location", async () => { + const config = testConfig(); + const bodyJson = JSON.stringify({ ok: true, id: "doc1" }); + vi.stubGlobal( + "fetch", + vi.fn(async () => { + return new Response(bodyJson, { + status: 201, + headers: { + "Content-Type": "application/json", + Location: `${COUCH_ORIGIN}/db/doc1`, + }, + }); + }), + ); + + const app = new Hono(); + let upstream!: Response; + app.put("/db/doc1", async (c) => { + upstream = await fetchFromCouch(c, config); + // Single client wrap — must not throw even when Location was absolute. + return toClientResponseFromCouch(upstream, config); + }); + + const res = await app.request("http://proxy.test/db/doc1", { + method: "PUT", + headers: { "Content-Type": "application/json" }, + body: bodyJson, + }); + + // fetchFromCouch must leave Location absolute (no intermediate Response wrap). + expect(upstream.headers.get("location")).toBe(`${COUCH_ORIGIN}/db/doc1`); + expect(res.status).toBe(201); + expect(res.headers.get("location")).toBe("/db/doc1"); + expect(await res.json()).toEqual({ ok: true, id: "doc1" }); + }); +});