Skip to content

fix(exit-node): fix double-wrapped response envelope and panic in error paths#1209

Open
CaptainMirage wants to merge 3 commits into
therealaleph:mainfrom
CaptainMirage:fix/exit-node-response-decoding
Open

fix(exit-node): fix double-wrapped response envelope and panic in error paths#1209
CaptainMirage wants to merge 3 commits into
therealaleph:mainfrom
CaptainMirage:fix/exit-node-response-decoding

Conversation

@CaptainMirage
Copy link
Copy Markdown

Fixed two bugs in the exit-node relay path.

Commit 1 fixes #1022 — Code.gs was double-wrapping the exit node's
{s,h,b} response envelope, causing browsers to receive raw JSON instead
of page content. Adds the raw-return mode (req.r === true) to _doSingle
and fixes parse_exit_node_response to handle HTTP framing prefixes and
stale content-encoding headers.

Commit 2 fixes a panic (SIGILL/core dump) in four error format strings
that sliced a &str at byte offset 200, which crashes when the response
contains multi-byte UTF-8 characters (quota error pages, binary responses).
Replaced with char-aware truncation.

Tested against claude.ai, chatgpt.com, openai.com, perplexity.ai and general browsing with both
WARP and direct exit-node modes.

…herealaleph#1022)

Fixes therealaleph#1022 -- exit-node-routed requests returned raw {s,h,b} JSON
to the browser instead of actual page content.

Root cause: Code.gs had no raw-return handler. The Rust client sets
r:true on the outer Apps Script request to signal verbatim passthrough,
but without a matching branch Code.gs was wrapping the exit node's
{s,h,b} response in a second {s,h,b} envelope. parse_exit_node_response()
peeled one layer and handed the inner {s,h,b} JSON string to the browser
as the body.

Code.gs (_doSingle):
- add req.r === true branch returning resp.getContentText() verbatim
  via ContentService before the normal {s,h,b} wrap
- _buildOpts: followRedirects is now unconditionally true; r controls
  raw-return mode only, not redirect following

domain_fronter.rs (parse_exit_node_response):
- scan for \r\n\r\n separator and skip any HTTP framing prefix before
  JSON parsing; some Apps Script edge nodes prepend HTTP headers to the
  response body
- add content-encoding to SKIP_RESPONSE_HEADERS; exit node fetch()
  auto-decompresses so forwarding the header causes Content Encoding
  Error in the browser
…paths

Four error format strings truncated a &str at byte offset 200 using
&text[..text.len().min(200)]. When the response body contains multi-byte
UTF-8 characters (quota error HTML, brotli-compressed or binary Apps
Script responses, cold-start warning pages), byte offset 200 can fall
inside a character boundary, causing a panic and SIGILL core dump.

Replace byte-offset truncation with char-aware truncation via
.chars().take(200).collect::<String>() at all four sites:
- parse_relay_json: "no json in" and "no json end in" messages
- finalize_tunnel_response: "no json in tunnel response" message
- finalize_batch_response: "no json in batch response" message

Reproducible under normal operating conditions when Apps Script
returns a non-JSON body under quota pressure or transient errors.
@github-actions github-actions Bot added the type: fix fix: PR — auto-applied by release-drafter label May 14, 2026
@therealaleph
Copy link
Copy Markdown
Owner

Reviewed via Anthropic Claude.

Two real fixes I want to land, mixed with one regression I don't.

Good (want to keep):

  1. Char-aware truncation in 4 error format strings (parse_exit_node_response / parse_relay_json / two others). Replaces &text[..text.len().min(200)] (which panics at byte-boundary cuts through multi-byte UTF-8) with &text.chars().take(200).collect::<String>(). Real SIGILL fix when responses contain Persian-localized quota errors or binary payloads. Worth landing on its own.

  2. parse_exit_node_response skip past \r\n\r\n HTTP-framing prefix + add content-encoding to the stale-headers strip list. Handles the double-wrap case at the parse level without needing Code.gs changes. Belt-and-suspenders fix — also handles cases where the exit-node response comes back through chained relays that add HTTP framing. Worth landing.

  3. r: true raw-return mode in _doSingle is forward-compatible (current clients don't send r: true, so it's dead-code-ish today). Fine to land; clients can opt-in later.

Regression (need to fix before merge):

  1. The try/catch around UrlFetchApp.fetch(...) + Utilities.base64Encode(...) was removed in _doSingle. That try/catch was added in 502 error (Script Completed but did not return anything) on downloading files. #1047 commit e2a4be24 specifically to prevent the placeholder <title>Web App</title> HTML from leaking to the client when:
    • URL is too long → throws
    • Payload too large → throws
    • Quota exhausted → throws
    • Execution timeout (6 min) → throws
    • Base64 of 50MB response blows V8 heap → throws

Without the try/catch, those throws propagate unhandled, Apps Script serves the default HTML error page, and the client surfaces bad response: no json in: <title>Web App>... instead of a structured { e: "fetch failed: ..." }. That regresses on every #973 / #1019 / #948 / #755-class issue.

Diagnostic logging (need to remove before merge):

  1. EXIT_DIAG app_body len=... warnings added in domain_fronter.rs::relay_exit_node. Useful for debugging during the PR work, but they'd flood every user's RUST_LOG=info console with full first-200-bytes hexdumps in production. Strip before merge.

Suggested action:

If you'd like, I can split this PR's changes — keep (1), (2), (3) and drop (4)'s try/catch removal + (5) the EXIT_DIAG warnings. Or you can do it on your branch:

  • Restore the try/catch in _doSingle around UrlFetchApp.fetch(opts) + Utilities.base64Encode(resp.getContent()) (just add it back around the new code, keep the r:true branch outside the catch).
  • Strip the 2 tracing::warn!("EXIT_DIAG ...") calls from relay_exit_node.

After that, this lands.

Re: #1022 root cause — agree it was double-wrapping. The parse-side fix in (2) is the right architectural shape because it doesn't require a Code.gs/client coordinated upgrade.


[reply via Anthropic Claude | reviewed by @therealaleph]

@CaptainMirage
Copy link
Copy Markdown
Author

CaptainMirage commented May 15, 2026

i kinda didnt realized i removed that i was behind the main branch lol sorry, restored that one and removed the debug stuff, tho i would say it wouldnt clutter the info thing since i did put it in debug trace not warn traces but still not that useful outside of this fix so removed it anyway

Copy link
Copy Markdown
Owner

@therealaleph therealaleph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fast follow-up here. The two underlying fixes make sense to me, especially the char-aware truncation in the error paths.

One blocker before I can merge this: the current diff still contains the temporary EXIT_DIAG block in src/domain_fronter.rs:

tracing::debug!(
    "EXIT_DIAG app_body len={} first_200={:?}",
    app_body.len(),
    String::from_utf8_lossy(&app_body[..app_body.len().min(200)])
);

That can put the first bytes of proxied response content into logs. In the exit-node path that may be real destination content, not just relay metadata, so we should not ship it even at debug. The PR body/commit message says this was removed, but the patch still has it.

Please drop that diagnostic block and leave the direct parse_exit_node_response(&app_body) call. After that I can rerun local verification and merge if the rest stays clean.


[reply via Anthropic Claude | reviewed by @therealaleph]

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

Labels

type: fix fix: PR — auto-applied by release-drafter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mhrv-rs fails decoding exit-node response

2 participants