clickhouse-proxy: body re-injection breaks charset-suffixed JSON and urlencoded requests; multipart forwarding works only accidentally
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 9.9k
- Forks
- 471
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 117
Description
Context
While investigating ClickHouse/support-escalation#8482 (filter sidebar values vanishing behind query proxies — fixed client-side in #2932 by inlining facet keys), several latent bugs surfaced in the /clickhouse-proxy request-body re-injection (packages/api/src/routers/api/clickhouseProxy.ts, proxyReq handler). #2932 initially hardened them but was scoped down to keep that file untouched; this issue tracks the deferred fixes.
Latent bugs
The handler re-injects req.body after the express parsers have (possibly) consumed the request stream:
let body = _req.body;
if (_req.headers['content-type'] === 'application/json') {
try { body = JSON.stringify(body); } catch (e) { console.error(e); }
}
try {
proxyReq.write(body);
} catch {
console.error(`clickhouseProxy error writing body, body is type ${typeof body}`);
}
- Strict
content-type === 'application/json'comparison misses charset-suffixed headers (application/json; charset=utf-8).express.json()consumed the stream, the object is never re-serialized,proxyReq.write(object)throws, nothing is piped (stream already consumed) — the upstream waits forContent-Lengthbytes that never arrive (hang/timeout or empty-query 400). application/x-www-form-urlencodedbodies are parsed and then dropped — same failure mode as (1): parsed to an object, never re-serialized, stream consumed.- Unparsed bodies (e.g.
multipart/form-data) hitproxyReq.write({})— body-parser initializesreq.body = {}even for content types it skips, so the write throws on every such request. The error is swallowed with aconsole.errorand forwarding only works accidentally because httpxy subsequently pipes the still-unconsumed raw stream. The passthrough behavior is pinned byclickhouseProxy.int.test.ts; the noisy caught-throw should be replaced with an explicit skip. Content-Lengthis not synced when the re-injected payload's byte length differs from the original request header (e.g.JSON.stringifynormalization), risking truncated/over-long upstream reads.- Write failures are silent: the request proceeds body-less instead of failing loudly, surfacing to users as an opaque 400 from ClickHouse.
Suggested fix
In the proxyReq handler: write string/Buffer bodies as-is; startsWith('application/json') → JSON.stringify; urlencoded objects → URLSearchParams re-serialization; anything unparsed → skip the write and let the raw stream pipe; sync Content-Length on re-injected payloads (when no transfer-encoding); destroy the proxied request with a real error on write failure. A working implementation (with passing integration tests for all five behaviors) exists in #2932's history: hyperdxio/hyperdx@335c8f96c.
Coverage
packages/api/src/routers/api/__tests__/clickhouseProxy.int.test.ts pins the currently-working behaviors (text/plain verbatim, multipart passthrough); the JSON-charset and urlencoded cases from 335c8f96c can be restored alongside the fix.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in packages/api/src/routers/api/clickhouseProxy.ts at the proxyReq handler, then read packages/api/src/routers/api/tests/clickhouseProxy.int.test.ts and commit 335c8f96c. Restore coverage for JSON with a charset, urlencoded bodies, content-length handling, write failures, and multipart passthrough; the integration tests should pass for all five behaviors.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- clickhouse, typescript
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100