Skip to content

fix: stop following upstream redirects on the client's behalf (#661) - #664

Open
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-09_redirect-manual
Open

fix: stop following upstream redirects on the client's behalf (#661)#664
ranxianglei wants to merge 1 commit into
masterfrom
2026-09-09_redirect-manual

Conversation

@ranxianglei

Copy link
Copy Markdown
Owner

Problem (issue #661, second symptom)

With bili proxy + CA in front of an upstream that redirects (CDN/WAF), requests failed with HTTP 405.

Root cause

fetchWithTimeout (src/fetch-util.ts) never set the redirect option, so undici's global fetch used its default redirect: "follow". Per the fetch spec, following a 301/302/303 on a POST downgrades the request to GET and drops the body. So when the upstream answered the POST with a redirect, bili re-issued it as a GET at the redirect target — an API endpoint that only accepts POST — which returned 405 Method Not Allowed.

A forward proxy must not follow redirects on the client's behalf: the 3xx (status + Location) must be passed through and the client follows it with its own policy.

Changes

  • src/fetch-util.tsfetchWithTimeout now defaults redirect to "manual" (the proxied request path: forward, compat retry, fake-completion retry, compress-loop / preflight / loop re-sends all inherit this).
  • src/server.ts — the internal POST /__bili/upstream/test probe explicitly opts into redirect: "follow" (behavior unchanged).
  • src/registry.ts — the internal models.dev registry fetch explicitly opts into redirect: "follow" (behavior unchanged).
  • tests/redirect-passthrough.test.ts (new) — regression tests:
    • POST: upstream answers 302 → client sees 302 + Location (not 405), the redirect target is never contacted, and the forwarded body still carries ACP render tags → compression stays fully active.
    • GET: same pass-through behavior.
    • Verified the tests fail with 405 when the default is reverted to "follow" (reproduces the reported symptom exactly).

ACP compression impact

None. The fix is transport-only: the kernel round-trip (prepareAnthropicprocessTurncoreToAnthropic) still runs before the forward; only the redirect policy of the outgoing fetch changed. The new tests assert ACP tags are present in the forwarded body.

Hypotheses checked (per issue request)

  1. Redirect changes the method → 405 — ✅ confirmed, this is the root cause (fixed above).
  2. Header rewrites (Accept-Encoding / Content-Encoding / Content-Length) — ❌ not the cause: content-length is stripped and recomputed from the actual body; content-encoding is decoded and stripped before forwarding (consistent). Side finding (minor, not fixed here): the Follow-up from #617: robust session identity + no-spurious-400 for header-less clients (e.g. ZCode) #619 unsupported-encoding path forwards an still-encoded body while content-encoding is stripped as hop-by-hop, which would confuse the upstream (400-class, not 405).
  3. ACP compression changes method/path/headers — ❌ not the case: method is preserved (req.method ?? "GET"); ACP only rewrites the body.

Pre-flight

  • npm run typecheck
  • npm test ✅ 1248/1248 (includes 2 new tests)
  • npm run build
  • E2E codex suite not run here (needs a reachable Responses upstream); unit + integration coverage of the redirect path is provided by the new tests.

Related: PR #662 (per-route passthrough for body-fingerprinting upstreams) is an independent, complementary fix for the first symptom of #661.

fetchWithTimeout inherited undici's default redirect:"follow", which
downgrades a POST to a GET and drops the body when the upstream answers
301/302/303. A redirecting upstream (CDN/WAF in front of the API) therefore
turned a valid POST into a 405 at the redirect target.

A forward proxy must not follow redirects for the client: pass the 3xx
through (status + Location) and let the client follow it with its own
policy. ACP compression is untouched — the kernel round-trip still runs
before the forward (covered by the new regression tests).

- fetch-util.ts: fetchWithTimeout defaults redirect to "manual"
- server.ts: upstream/test probe opts into follow (internal, unchanged)
- registry.ts: models.dev fetch opts into follow (internal, unchanged)
- tests/redirect-passthrough.test.ts: 302 pass-through for POST and GET,
  target never contacted, ACP tags still present in the forwarded body
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

📦 Built Package Artifact

Branch: 2026-09-09_redirect-manual (74a20d5)

Option A — Install from npm PR tag (recommended)

npm install -g billion-context@pr-664

Each push to this PR publishes a new version under the pr-664 npm tag.

Option B — Download artifact

  1. Download the artifact from the Actions run
  2. Extract the tarball and install:
tar xzf billion-context-pr664.tgz
npm install -g package

This comment is automatically updated on each push.

@ranxianglei

Copy link
Copy Markdown
Owner Author

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review: LGTM — recommend merge. CI is green (test ubuntu 22/24, windows 22/24, build-artifact, version-guard all pass). I independently re-ran the pre-flight on the PR branch and reproduced the bug both ways.

What I verified

  1. Root cause is real — reproduced both directions. On pr-664 I temporarily reverted the default to redirect: "follow": both new tests fail with exactly the reported symptom — client must see the upstream 302 passed through; got 405: {"error":"method not allowed"}. Restored "manual": both pass. The regression tests genuinely pin the bug.
  2. Pre-flight re-run on the branch: npm run typecheck ✅, npm test ✅ 1248/1248 (matches your claim), npm run build ✅.
  3. Call-site audit — every fetchWithTimeout consumer accounted for:
    • src/registry.ts:109 (models.dev) and src/server.ts:873 (/__bili/upstream/test probe) — explicitly opt into "follow", behavior unchanged ✅
    • src/server.ts:3318 (main forward) — inherits "manual"; a 3xx lands in the !upstream.ok branch (src/server.ts:3449) and is passed through verbatim; location is not in UPSTREAM_HOP_HEADERS (src/server.ts:208-227), so the client gets status + Location ✅
    • src/server.ts:3390 (compat role-rewrite retry), src/server.ts:3973 (fake-completion retry) — 3xx handled gracefully ("retry rejected (HTTP 302); presenting original" / original 400 continues) ✅
    • fetchWithRetry consumers (src/preflight.ts:330, src/loop/core.ts:213, src/compress-loop-responses.ts:218) — a 3xx now surfaces as UpstreamHttpError(3xx) instead of a silently-followed POST→GET: preflight reports a clean failure message; the compress loops emit a protocol error event. Strictly better than before, and a raw 302 couldn't pass through mid-stream anyway (headers already committed) ✅
  4. No coverage gap: I grepped for direct fetch( calls bypassing fetchWithTimeout — the rest are local/management-plane only (__bili/plugin/*, web UI, npm auto-update, launcher probes). fetchWithTimeout is the complete choke point for proxied client traffic.
  5. Both compression modes: the change is in the shared transport layer, mode-agnostic; plugin-mode re-sends and proxy-mode loop re-sends both inherit "manual" and handle 3xx cleanly. ACP impact: none — the new tests assert ACP render tags are present in the forwarded body.

Layer judgment

The 405 is a symptom; the root cause is the proxy following redirects on the client's behalf (undici's default redirect: "follow" downgrades POST→GET and drops the body on 301/302/303). The fix targets the root cause at the single transport choke point with correct forward-proxy semantics (pass 3xx through, client follows with its own policy) — not a symptom patch. The two explicit "follow" opt-ins keep internal bili fetches behavior-identical.

Minor observations (non-blocking)

  • The main forward path now logs a legitimate 3xx as warn via the existing non-2xx logging path (src/server.ts:3564, the fix: log non-2xx upstream responses (status + request-id + body snippet) #174 "always log a non-2xx" policy). Slightly noisier for redirecting upstreams, but consistent with existing behavior — fine as-is.
  • I independently verified your Follow-up from #617: robust session identity + no-spurious-400 for header-less clients (e.g. ZCode) #619 side finding: on the unsupported-encoding path (src/server.ts:1001-1007) the still-encoded body is forwarded while buildForwardTarget (src/server.ts:2803-2807) strips content-encoding via UPSTREAM_HOP_HEADERS (src/server.ts:217) — the upstream gets an encoded body with no encoding declaration (400-class), and the "content-encoding header stays intact" comment at src/server.ts:1003-1004 is stale. Correctly deferred; it's documented in the PR description, which satisfies the report-only bar for a minor issue. Happy to file a tracking issue for it if you want one.

Merge is human-only per repo rules — the PR is ready when you are.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant