Skip to content

Rewrite Sourcepoint responses without Content-Length - #1183

Open
ChristianPavilonis wants to merge 4 commits into
mainfrom
fix/sourcepoint-unknown-length-1088
Open

ChristianPavilonis wants to merge 4 commits into
mainfrom
fix/sourcepoint-unknown-length-1088

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • Rewrite eligible Sourcepoint JavaScript and HTML even when upstream omits Content-Length, so embedded URLs and privacy-manager assets still use the first-party proxy.
  • Use Fastly's streaming response path and the existing 5 MiB collector. Bodies that exceed the limit during collection return 502; declared oversized bodies still pass through unchanged.
  • Request uncompressed /mms/v2/get_site_data responses and preserve their upstream and cookie-aware cache policy instead of applying the static JavaScript cache policy.

Changes

File Change
crates/trusted-server-core/src/integrations/sourcepoint.rs Remove the missing-length bypass, request streaming where supported, handle site-data encoding and caching, and add regression tests for rewriting, limits, pass-through, and headers.
docs/guide/integrations/sourcepoint.md Document the rewrite limit, 502 overflow policy, cache behavior, and adapter limitations.

Scope

Limited to the Sourcepoint integration and its documentation, using existing collection and streaming APIs. Most added code is regression coverage. No adapter implementation or browser JavaScript changes are included.

Fastly enforces the limit while reading the upstream stream. Cloudflare and Spin still buffer upstream bodies before the integration checks them; fixing that adapter-level limitation is deferred. This change does not address campaign or consent-state behavior that can suppress the banner.

Closes

Closes #1088

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run (893 passed)
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM release build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • cargo test-cloudflare && cargo test-spin
  • cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm
  • cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity (13 passed)
  • cargo test-fastly integrations::sourcepoint (63 passed under Viceroy)

The new missing-length JavaScript and HTML tests failed before the fix and passed afterward. Stream tests cover exactly 5 MiB, overflow, understated lengths, and stopping reads at the limit. Independent review found no introduced correctness issues.

Tests use stub upstream streams. A live Sourcepoint exchange, wire framing, and deployed cache behavior have not been smoke-tested.

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code
  • Uses log macros, not println!, as required by CLAUDE.md
  • New code has tests
  • No secrets or credentials committed

Allow bounded JavaScript and HTML rewriting when upstream responses omit
Content-Length. Request streaming on supported adapters so the 5 MiB
collector can stop before buffering an oversized response.

Return 502 on collection overflow, retain declared-oversize pass-through,
and request identity encoding for site data without overriding its dynamic
cache policy. Cover stream limits, rewriting, pass-through, and headers.

Closes #1088
ChristianPavilonis added a commit that referenced this pull request Sep 28, 2026
@ChristianPavilonis
ChristianPavilonis marked this pull request as ready for review September 28, 2026 21:10
ChristianPavilonis added a commit that referenced this pull request Sep 28, 2026

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Eligible Sourcepoint JavaScript and HTML responses now use the existing bounded collector when Content-Length is absent, with explicit 502 overflow behavior and streaming pass-through on Fastly. Site-data responses request identity encoding and preserve upstream or cookie-aware cache policy; I found no actionable introduced issues.

Reviewed commit 4552fcfa621231789bb0830518091995ada52ace, including both changed files and the downstream adapter response paths. Regression coverage includes unknown and understated lengths, exact-limit bodies, overflow and early termination, declared oversize and ineligible pass-through, buffered adapters, cache/cookie policy, and invalid UTF-8.

Validation relies on the passing GitHub CI checks below. I did not rerun tests locally or smoke-test a live upstream exchange.

CI Status

@dhruv8sh dhruv8sh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

The rewrite path now collects JavaScript and HTML bodies with no Content-Length, stopping at 5 MiB. Bodies that declare more than 5 MiB still pass through, and on Fastly the upstream body keeps streaming until it is read. The get_site_data encoding and cache handling matches what #1088 observed. The change looks correct and the tests are thorough. The comments below are non-blocking design thoughts and nits.

1 of the inline comments below carries a one-click GitHub suggestion. Use Commit suggestion to apply it as a commit on the PR branch. The remaining comments describe the fix in prose because the lines involved are outside the diff and can't be auto-applied.

Non-blocking

🤔 thinking

  • Compressed JS/HTML responses with no Content-Length are now held in memory for nothing: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:968
  • On adapters that buffer, the 502 doesn't save any memory: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:970

⛏ nitpick

  • Going over the limit no longer leaves a Sourcepoint-specific log line: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:968
  • Docs leave Axum out of the adapters that buffer: see inline at docs/guide/integrations/sourcepoint.md:74

👍 praise

  • The tests prove the limit holds: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:1259

CI Status

return Ok(response);
}

// Content-Length is optional and advisory. Stop at the actual

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 thinking: Compressed JS/HTML responses with no Content-Length are now held in memory for nothing.

The check that decides whether to rewrite a body (lines 936–939) never looks at Content-Encoding. Uncompressed content (Accept-Encoding: identity) is only requested for likely JS/HTML paths. On any other path the client's gzip, br is forwarded, so upstream can return a compressed body with a JS or HTML content type. Before this PR, a compressed body without Content-Length streamed straight through. Now it is read into memory (up to 5 MiB, with a 502 if it goes over), fails the UTF-8 check, and passes through unchanged anyway. If a brotli body happened to be valid UTF-8, it would be rewritten (garbling it) and Content-Encoding would still be stripped. That risk already existed for bodies with Content-Length; this PR extends it to bodies without one.

Proposed fix (apply manually. It can't be a one-click suggestion because lines 936–939 are outside the diff):

let response_is_identity_encoded = response
    .headers()
    .get(header::CONTENT_ENCODING)
    .and_then(|v| v.to_str().ok())
    .is_none_or(|v| v.trim().eq_ignore_ascii_case("identity"));
if method == Method::GET
    && response.status() == StatusCode::OK
    && self.config.rewrite_sdk
    && response_is_identity_encoded
    && (response_is_javascript || response_is_html)

handle_preserves_invalid_utf8_bytes_and_headers would need updating too: its body would stay a stream, and take_body_bytes returns empty for streams, so that test would collect with into_bytes_bounded instead.


// Content-Length is optional and advisory. Stop at the actual
// byte limit even when the header is absent or understates the
// size. Overflow discards the partial body and returns a 502.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 thinking: On adapters that buffer, the 502 doesn't save any memory.

On Axum, Cloudflare and Spin (none of them support streaming responses), the whole body is already in memory as EdgeBody::Once before the integration sees it. Returning 502 when it's over 5 MiB saves no memory and drops a response that could pass through unchanged, which is what used to happen when Content-Length was missing. #1088 weighed 502 against passing through for the streaming case, where passing through means keeping the prefix and re-chaining the stream. For an already-buffered body, passing through is free. The trade-off is that Fastly and the other adapters would behave differently. If you prefer they behave the same, the current behaviour is reasonable.

Possible shape (apply manually. Lines 971–977 are unchanged context outside the diff):

let (resp_parts, resp_body) = response.into_parts();
if let EdgeBody::Once(bytes) = &resp_body
    && bytes.len() > MAX_REWRITE_BODY_SIZE as usize
{
    log::warn!(
        "Sourcepoint: buffered body for {path} exceeds {MAX_REWRITE_BODY_SIZE} bytes, \
         skipping rewrite (reason: buffered_length_too_large)"
    );
    let mut response = http::Response::from_parts(resp_parts, resp_body);
    self.apply_cache_headers(&mut response, forwarded_cookies);
    return Ok(response);
}

return Ok(response);
}

// Content-Length is optional and advisory. Stop at the actual

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⛏ nitpick: Going over the limit no longer leaves a Sourcepoint-specific log line.

The removed branch logged a greppable reason: missing_content_length, and the oversize branch still logs reason: known_length_too_large. The new overflow path just returns the error with ?, and the generic error handler turns it into a 502 without the Sourcepoint path or a reason tag. Something like this would keep the logs consistent:

.await
.inspect_err(|_| {
    log::warn!(
        "Sourcepoint: response body for {path} exceeded {MAX_REWRITE_BODY_SIZE} bytes \
         during collection, returning 502 (reason: collected_length_too_large)"
    );
})?;

Apply manually. Line 977 (.await?;) is outside the diff.

- If collection exceeds the limit, Trusted Server stops reading and returns `502 Bad Gateway`. This also applies when `Content-Length` understates the size. The partial body is discarded, not returned to the browser.
- Other content types, disabled rewriting, and ineligible methods or statuses do not collect the body for rewriting.

On Fastly, upstream responses remain streaming until the bounded collector reads them. It retains at most 5 MiB of input plus the current transport chunk, with additional bounded allocations for rewriting. Non-rewritten responses remain streaming. Adapters without streaming support still buffer upstream responses before this check; this limit is not an adapter-level memory guarantee on Cloudflare or Spin.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⛏ nitpick: Docs leave Axum out of the adapters that buffer. Only the Fastly adapter overrides supports_streaming_responses(), and Axum also uses the default false.

Suggested change
On Fastly, upstream responses remain streaming until the bounded collector reads them. It retains at most 5 MiB of input plus the current transport chunk, with additional bounded allocations for rewriting. Non-rewritten responses remain streaming. Adapters without streaming support still buffer upstream responses before this check; this limit is not an adapter-level memory guarantee on Cloudflare or Spin.
On Fastly, upstream responses remain streaming until the bounded collector reads them. It retains at most 5 MiB of input plus the current transport chunk, with additional bounded allocations for rewriting. Non-rewritten responses remain streaming. Adapters without streaming support still buffer upstream responses before this check; this limit is not an adapter-level memory guarantee on Axum, Cloudflare, or Spin.

}

#[test]
fn handle_accepts_exact_rewrite_limit_and_stops_reading_on_overflow() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍 praise: The read-counting StreamingHttpClient lets these tests show that reading stops at the end of the body or at the first chunk past the limit. That holds for both content types, with Content-Length missing or understated, at exactly the limit and over it. The pass-through tests show that bodies declaring more than 5 MiB, and responses that aren't eligible for rewriting, are never read. The get_site_data tests cover the cache policy for every cookie and upstream-header combination.

This branch has not been deployed

No deployments
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.

Rewrite bounded Sourcepoint responses when upstream omits Content-Length

4 participants