Skip to content

Rewrite Sourcepoint responses without Content-Length - #1183

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

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

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Sep 18, 2026 •

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.
  • Skip body rewriting for compressed, unsupported, or malformed response encodings without reading the body, and log collection failures with the Sourcepoint path and actual error.
  • 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. Axum, 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 (65 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

Review follow-up

Commit 5f527d07 adds the response-encoding guard, preserves separate invalid-UTF-8 fallback coverage, adds contextual collection-failure logging, and corrects the adapter documentation. The encoded-response regression failed before the guard and passed afterward. Full Fastly, Axum, Cloudflare, and Spin tests, all applicable adapter Clippy checks, Rust formatting, and docs formatting passed.

The overflow policy is unchanged: declared oversized bodies pass through; overflow during collection returns 502 for both buffered and streaming bodies. This does not prevent adapters from allocating their initial upstream buffer.

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

Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs
Comment thread docs/guide/integrations/sourcepoint.md Outdated
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs
@ChristianPavilonis ChristianPavilonis added this to the 202610 milestone Oct 1, 2026

@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 follow-up commit resolves the earlier review's points. An encoding guard now means compressed or malformed-encoding JS/HTML responses pass through without being collected. Collection failures log a Sourcepoint-specific reason, and the docs list Axum among the buffering adapters. The overflow policy (502 on collection overflow for both body types) was a deliberate, documented choice, and I'm fine with it. Only minor nits remain.

3 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch.

Non-blocking

⛏ nitpick

  • The new warn log writes the whole error report again: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:994
  • Asserts with no messages in handle_keeps_encoded_responses_streaming: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:1317
  • Asserts with no messages in handle_rewrites_explicit_identity_encoded_responses: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:1376

👍 praise

  • The encoding guard is strict and fully tested: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:938

CI Status

  • browser integration tests: PASS
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native): PASS
  • cargo test: PASS (required)
  • cargo fmt: PASS (required)
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • vitest: PASS
  • CLAUDE.md symlink guard: PASS
  • CodeQL: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS

Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs

@aram356 aram356 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

This does what #1088 asks, and the follow-ups from the earlier rounds are in. One blocking problem remains: since 5f527d0, the ineligible-response test no longer exercises any of the eligibility gates it lists. A mutation probe shows the method, status, rewrite_sdk and content-type checks have no test coverage at all.

dhruv8sh's open nits (assertion messages, {error} in the collection log) are not repeated here. The suggestions below were also verified with those three applied.

3 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining inline comment describes the fix in prose because it touches lines outside the diff.

Blocking

🔧 wrench

  • handle_keeps_ineligible_responses_streaming no longer exercises its gates: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:1594

Non-blocking

♻️ refactor

  • The new encoding bypass is the only rewrite skip with no reason-tagged log: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:952

🤔 thinking

  • The static cache policy is selected by excluding one exact path: see inline at crates/trusted-server-core/src/integrations/sourcepoint.rs:668

⛏ nitpick

  • Docs say rewritten JavaScript "retains" the configured TTL: see inline at docs/guide/integrations/sourcepoint.md:92

Cross-cutting / body-level findings

  • 🌱 No end-to-end coverage for the Sourcepoint proxy. The integration fixture (crates/trusted-server-integration-tests/fixtures/configs/trusted-server.integration.toml) has [integrations.sourcepoint] enabled = false. After this PR, every Sourcepoint response on Fastly uses the streaming send path (with_stream_response() for every method and content type, not only rewrite candidates), and no test runs that end to end. The risk is low because js_asset_proxy and asset routes already use the same adapter path, but the PR's unchecked fastly compute serve smoke is the only planned live check. A follow-up could enable Sourcepoint against a mock CDN in the Viceroy suite to cover rewrite and pass-through framing.

Verification

  • Each suggestion was applied alone in a scratch tree: fmt, the 65 Sourcepoint tests and cargo clippy-fastly pass, the docs suggestion passes prettier, and the test change also passes cargo test-fastly.
  • All three suggestions together with dhruv8sh's three pass the full gate: fmt, docs prettier, all six clippy aliases, test-fastly, test-axum, test-cloudflare, test-spin and parity.
  • The CI below last ran on 2026-10-01, before #1179 (reusable sandbox) merged. A local merge of this head with current main (d8937e1) is clean and passes fmt, clippy-fastly, test-fastly, test-fastly-reuse and parity.

CI Status

Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/sourcepoint.rs Outdated
Comment thread docs/guide/integrations/sourcepoint.md Outdated
aram356 and others added 2 commits October 8, 2026 08:41
Preserve upstream cache policy for rewritten JavaScript outside likely
static bundle paths. Log encoded-response bypasses at warning level only
when identity encoding was requested.

Repair eligibility coverage, add a path-aware cache regression, and
clarify assertion messages, collection diagnostics, and cache docs.
@ChristianPavilonis
ChristianPavilonis force-pushed the fix/sourcepoint-unknown-length-1088 branch from 2bf5e23 to c185093 Compare October 8, 2026 21:09

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