Repository navigation
fix(sse): strip stale Content-Encoding/Content-Length on non-stream forward - #2264
Conversation
…tion Embed Gemini, Antigravity and Windsurf public OAuth/Firebase identifiers (extracted from upstream CLI binaries) through a XOR-masked byte sequence in open-sse/utils/publicCreds.ts instead of source literals, so pattern scanners (GitHub Secret Scanning, Semgrep) stop raising false positives on every release. decodePublicCred passes raw values through unchanged for users who already have plaintext in their .env (no migration needed). - New utils/publicCreds.ts with decode/encode + tests - Replace 6 hardcoded Google client_id/secret in oauth.ts + providerRegistry - Drop literals from .env.example (comment-only documentation) - Sanitize error messages inside buildErrorBody so every caller (incl. createErrorResult) is covered; cursor.ts now reuses the shared helper - Cover the new helpers with unit tests (publicCreds + error sanitization) Resolves the open code-scanning js/stack-trace-exposure findings and the secret-scanning Google API Key alert without breaking existing setups. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…terns Fixes the 4 fixable alerts opened in the recent scan and adds enforceable guardrails so future development follows the same pattern. Code fixes: - src/mitm/cert/install.ts: pass certPath/certName/action via exec()'s env option instead of string-interpolating them into the bash script (CodeQL js/shell-command-injection-from-environment cheaper-inference#225) - scripts/docs/{gen-provider-reference,add-frontmatter,fix-internal-links}: escape backslash before other regex/markdown metacharacters (CodeQL js/incomplete-sanitization cheaper-inference#227, cheaper-inference#228, cheaper-inference#229) Documentation (mandatory patterns): - docs/security/PUBLIC_CREDS.md — embedding public upstream OAuth/Firebase identifiers via resolvePublicCred(); never as string literals - docs/security/ERROR_SANITIZATION.md — routing every error response through sanitizeErrorMessage()/buildErrorBody(); never raw err.stack/err.message - CLAUDE.md: 4 new Hard Rules (cheaper-inference#11-cheaper-inference#14) + Security section + scenario notes - AGENTS.md, CONTRIBUTING.md: cross-reference the two new docs - SECURITY.md: extended Hard Security Rules with the new mandatory patterns - docs/README.md: index entries pointing to the two new docs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The PATH_REGEX introduced in 1a39c31 used a greedy character class ([\w\-./\\]+) that can backtrack quadratically on inputs like "///////..." — flagged by CodeQL js/polynomial-redos (cheaper-inference#232). Replace it with simple whitespace tokenization + a length-bounded prefix-check helper. Same observable behaviour (paths replaced with <path>), but linear time regardless of input shape. Adds a 4096-char input cap as defence in depth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…precedent CodeQL js/stack-trace-exposure does not recognize sanitizers reached through a custom helper indirection — flagging callsites like open-sse/utils/error.ts::errorResponse and open-sse/executors/cursor.ts::buildErrorResponse even though both route through sanitizeErrorMessage(). Record the dismissal precedent (alerts cheaper-inference#224 and cheaper-inference#231, May 2026): - Add a "Known CodeQL limitation" section in docs/security/ERROR_SANITIZATION.md - Extend CLAUDE.md Hard Rule cheaper-inference#14 with the precedent Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…orward Upstream providers (Gemini in particular) reply to non-streaming chat completions with a gzip-compressed body whose `Content-Length` describes the compressed byte count. `readNonStreamingResponseBody` reads the upstream response via `response.text()`, which transparently decompresses gzip — so by the time we forward, both the original `Content-Encoding` and `Content-Length` headers describe a payload that no longer exists. Clients that honor `Content-Length` over a fully-buffered body then read only the first N bytes of the decompressed JSON and surface "Unterminated string in JSON at position …" parse failures. Extract `stripStaleForwardingHeaders(Headers): void` and call it immediately after copying upstream headers on the non-stream path, so the response framework can compute a fresh `Content-Length` (or pick `Transfer-Encoding: chunked`) for the decompressed payload. Tests cover three cases against the helper: removal of both headers, case-insensitivity (`Content-Length` vs `content-length`), and a no-op on a response that never had them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the stripStaleForwardingHeaders utility to remove content-encoding and content-length headers from upstream responses before they are re-emitted. This ensures that downstream clients receive accurate metadata for decompressed or buffered payloads, preventing parsing errors. The changes include integration into the handleChatCore handler and the addition of a new unit test suite. Feedback suggests also stripping the Transfer-Encoding header as it is a hop-by-hop header that becomes invalid after buffering, along with corresponding updates to the unit tests.
Addresses review feedback on the previous commit. `Transfer-Encoding` is a hop-by-hop header per RFC 7230 §6.1, so leaving the upstream `Transfer-Encoding: chunked` on a response whose body we have already fully read and re-emit is undefined behavior at the proxy boundary. Delete it alongside `Content-Encoding` and `Content-Length` so the response framework picks the framing it actually emits. The docstring is rewritten to spell out the three independent reasons for the strip (length-mismatch truncation, decoded compression, hop-by- hop framing) and to cite RFC 7230. Tests extend the existing fixture to assert `transfer-encoding` is also removed and that the helper stays case-insensitive for the new entry. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Thanks @gleber! 🎉 Excellent bug fix — the RFC 7230 §6.1 compliance and the clear JSDoc documentation on why each header is stripped are top-notch. This resolves the gzipped Gemini response truncation issue cleanly without affecting streaming paths. Merging into release/v3.8.0. |
9550491
into
cheaper-inference:release/v3.8.0
Self-contained Nix packaging for OmniRoute, sitting directly on top of `upstream/release/v3.8.0` — no source-level patches needed at this base because the four fixes that used to live in `nix/patches/` are now upstream: * PR cheaper-inference#2264 — Content-Length strip on non-stream forward * PR cheaper-inference#2265 — `managementPolicy` accepts manage- scope API keys * PR cheaper-inference#2266 — configurable default rate limits * the dynamic CLI-helper import fix (rewritten to static imports upstream in the v3.8.0 cycle) What the flake still does: * `packages.<system>.omniroute` — `buildNpmPackage` over Next.js 16 standalone output. Wires: - `libsecret` in `buildInputs` so keytar's native module finds `libsecret-1.pc` during node-gyp configure. - `npmInstallFlags = ["--include=dev"]` because `NODE_ENV=production` otherwise drops `@tailwindcss/postcss` and the rest of devDeps that the build actually needs. - `OMNIROUTE_USE_TURBOPACK=1` to match the devenv shell. - `postPatch` stubs three offline-build sticking points: * `next/font/google` (no network in sandbox) — replaced with a no-op `Inter()` factory; the `<link rel="preconnect">` in layout still loads webfonts at runtime. * `next-themes` import in `TierFlowDiagram.tsx` (upstream bug on release/v3.8.0: imported but not in `package.json`) — stubbed to always return dark mode. * `bin/cli/runtime/sqliteRuntime.mjs` (last two steps of its fallback chain reference `sql.js` and a dynamic `pkgRoot` path, breaking `outputFileTracing`) — replaced with a minimal stub that loads better-sqlite3 directly. - `autoPatchelfIgnoreMissingDeps` for the cross-platform binaries (musl, OpenBSD libc++) sharp, koffi, and ngrok ship alongside the glibc ones — platform-gated at load time, not loaded on glibc Linux. * `nixosModules.omniroute` (also `nixosModules.default`): - Options for ports/host/data dir, three secret-file options (jwt / api-key / optional initial password) plus an optional `storageEncryptionKeyFile` for AES-256-GCM key persistence — the failure mode that exists to prevent is losing the auto-generated key in `$DATA_DIR/server.env` and rendering every stored provider credential permanently unreadable. - Secrets are loaded via systemd `LoadCredential` (service-user readable only) and exported into the env by a small wrapper ExecStart, because the app reads `JWT_SECRET` / `API_KEY_SECRET` / `INITIAL_PASSWORD` / `STORAGE_ENCRYPTION_KEY` directly and does not honor the `_FILE` convention. - Standard hardening: NoNewPrivileges, ProtectSystem=strict, ProtectHome, RestrictAddressFamilies, etc. * `checks.<linux>.module` — `pkgs.testers.nixosTest` that boots the module, waits for the unit, asserts the server answers on the configured port, the data directory is owned by `omniroute:omniroute`, the configured secrets reached the app (so the "JWT_SECRET auto-generated" log line is absent), and `$DATA_DIR/server.env` is not written when a STORAGE_ENCRYPTION_KEY is supplied externally. Skipped on Darwin (`testers.nixosTest` needs a real QEMU VM). Verified locally on release/v3.8.0: `nix build .#default` succeeds and `nix build .#checks.x86_64-linux.module` passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…orward (cheaper-inference#2264) fix(sse): strip stale Content-Encoding/Content-Length on non-stream forward (cheaper-inference#2264 — thanks @gleber)
…orward (cheaper-inference#2264) fix(sse): strip stale Content-Encoding/Content-Length on non-stream forward (cheaper-inference#2264 — thanks @gleber)
…orward (cheaper-inference#2264) fix(sse): strip stale Content-Encoding/Content-Length on non-stream forward (cheaper-inference#2264 — thanks @gleber)
Summary
Non-streaming chat completion responses can be served by upstream providers (notably Gemini) with a
Content-Encoding: gzipbody whoseContent-Lengthdescribes the compressed byte count.readNonStreamingResponseBodyreads upstream viaresponse.text(), which transparently decompresses gzip — so by the time OmniRoute forwards, both the originalContent-EncodingandContent-Lengthdescribe a payload that no longer exists.Clients that honor
Content-Lengthover a fully-buffered body then read only the first N bytes of the decompressed JSON and surfaceUnterminated string in JSON at position …parse failures.Change
stripStaleForwardingHeaders(Headers): voidinopen-sse/handlers/chatCore.ts.Content-Length(or fall through toTransfer-Encoding: chunked).Test plan
node --import tsx/esm --test tests/unit/chatcore-strip-stale-headers.test.ts— 3 passingContent-Type,X-Request-Id)Content-Lengthvscontent-length)🤖 Generated with Claude Code