Repository navigation
Preserve Next.js and Google Tag Manager rewrites in streamed pages - #1222
ChristianPavilonis wants to merge 15 commits into
Conversation
Apply matching script stages in sequence and emit each fragment once. Keep GTM buffering response-local and bounded, and rewrite captured Flight payloads only after reading their original record boundaries. Preserve unsafe originals and recompute decoded record lengths. Cover Pages and App Router data, bootstrap controls, escaped queries, source-order fallback, and complete identity/gzip adapter responses. Refs #1206, #1207, #1208
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Composes script stages instead of letting one integration's Replace overwrite another's, and threads GTM rewriting into captured Flight payloads with recomputed T lengths. The fallback paths are conservative and the boundary-sweep test coverage is excellent; no blocking issues found. A few design questions and minor cleanups below.
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 comments describe the fix in prose because the change touches lines outside the diff or needs an author decision.
Non-blocking
🤔 thinking / ⛏ nitpick
- Flight GTM rewriting is broader than the HTML policy (bare origins, rendered content) — see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:194 ContentType::Htmlremoves the last escaping layer for request-host-derived bytes — see inline atcrates/trusted-server-core/src/html_processor.rs:716- Script rewriter selectors are now effectively
script-only — see inline atcrates/trusted-server-core/src/html_processor.rs:682 - Per-character work on every inline script — see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:476 - Redundant
enabledfilter — see inline atcrates/trusted-server-core/src/integrations/nextjs/mod.rs:114 - Assertions without messages — see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:3606andcrates/trusted-server-core/src/html_processor.rs:838
Cross-cutting / body-level findings
-
🏕 Registration order is now load-bearing but undocumented — GTM's
protects_current_flight_fragmentreads a snapshot the Next.js placeholder stage writes for the same fragment, sonextjsmust stay ahead ofgoogle_tag_managerinbuilders()(crates/trusted-server-core/src/integrations/mod.rs). LikewiseNextJsNextDataRewritermust precedeNextJsRscPlaceholderRewriter, which consumes its one-shotnext_data_fragment. A reorder would make GTM read the previous fragment's flag, and only the mixed integration tests would catch it. Suggest a comment alongside the existing "This must remain first" note (apply manually —mod.rsis outside the diff):// `nextjs` must precede `google_tag_manager`: GTM's script stage reads the // Flight-protection snapshot the Next.js stage records for the same fragment.
-
📝 PR description misses the new file — the Changes table doesn't list the new
nextjs/script_lexer.rs(318 lines, the lexical filter that decides executable vs inert__next_fspellings), and Scope says "11-file diff" while the diff is 12 files. Worth adding so reviewers know it exists and that it is intentionally "not a JavaScript parser". -
👍 Atomic capture and fallback discipline — batched capture in
rsc_placeholders.rsdiscovers every range and validates the queue budget before committing anything, fallback always restores untouched originals (enabled_gtm_transform_never_mutates_fallback_originalslocks that in), and the every-split-boundary sweeps plus identity/gzip end-to-end checks through both Axum and Fastly with decoded T-length assertions are exactly the right regression net for this class of streaming bug.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- integration tests: PASS
- cargo test (cross-adapter parity): PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- format-docs: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- format-typescript: PASS (required)
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- cargo test (axum native): PASS
- cargo fmt: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- CLAUDE.md symlink guard: PASS
- Analyze (actions): PASS
Keep regex literals after statement blocks inert without hiding Flight calls after object, function, or class expressions. Bound brace and pending-class context alongside existing lexical nesting limits. Add fragment-boundary and identity/gzip regressions for origin/GTM rewrites and decoded Flight T lengths. Both reported brace/slash regressions were reproduced before the fix.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Request changes for three reproduced P2 correctness issues:
- A superclass function body confuses the new lexer, allowing GTM rewriting to produce an invalid Flight T-record byte length.
- A trailing
__next_fproperty reference disables rewriting for the current and subsequent Flight scripts. - An ordinary GTM URL before a Flight call is rewritten differently depending on input chunk boundaries.
The inline comments describe the reproductions and proposed remedies.
Validation: 2,876 existing native core tests passed, as did Axum tests, cargo clippy-axum, cargo fmt --all -- --check, and the focused Fastly complete-response regression under Viceroy. Additional scratch regression tests reproduced the issues above. GitHub CI was green at submission.
Reviewed commit: d738d93f70e3a1e79bfa9256cc730a317d14381a.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the streamed Next.js/GTM composition and the host-validation follow-up at e81e5d98d7518836ac2aad3e80728785b535c9c4. Request changes: the three previously reproduced P2 correctness findings remain unresolved; the affected runtime code is unchanged from the earlier review, and the author confirms the follow-up commits do not address them.
Blocking
🔧 wrench
- [P2] Superclass function bodies confuse pending class state, permitting an invalid Flight T-record length — existing inline finding at
nextjs/script_lexer.rs:279. - [P2] A trailing
__next_fproperty reference disables rewriting for the current and subsequent Flight scripts — existing inline finding atnextjs/rsc_placeholders.rs:113. - [P2] Ordinary GTM source before a captured Flight call is rewritten differently depending on input chunk boundaries — existing inline finding at
google_tag_manager.rs:1134.
The existing inline threads contain the reproductions and proposed remedies. No duplicate inline comments or new findings are included in this re-review.
Validation
Compared the affected runtime code with the previously reviewed commit d738d93f70e3a1e79bfa9256cc730a317d14381a: the lexer and payload-discovery files are byte-identical, and the GTM change is an assertion message only. Reviewed the complete changed files and removed diff code with independent reviewers. Verified the remote CI results at the reviewed head, including all eight target-matched clippy steps; no full local CI rerun was performed.
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- Analyze (rust): PASS
- format-typescript: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (actions): PASS
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at b3538b47d. 4d73741 resolves the three P2s from the previous round. I checked each fix against its regression test: the superclass lexer state, trailing __next_f references, and the chunk-invariant ordinary GTM before captured Flight. One correctness issue remains in the same family as the first P2. A lexer false negative still lets GTM corrupt raw Flight T lengths, because protection depends only on the masked-code view. Requesting changes for a fail-closed guard instead of more per-spelling lexer fixes.
Blocking
🔧 wrench
- Lexer false negatives fail open into invalid Flight T lengths. Five valid-JS prefixes reproduce it. A raw-qualifier protection fallback was prototyped locally and fixes all five. See inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:435.
Non-blocking
🤔 thinking
- Underscore host labels now return 400. See inline at
crates/trusted-server-core/src/http_util.rs:242.
Cross-cutting / body-level findings
- 📌 GTM without Next.js still corrupts App Router Flight lengths. With
nextjsdisabled and GTM enabled,<script>self.__next_f.push([1,"1:T64,{\"tag\":\"http://www.googletagmanager.com/gtm.js?id=…\"…}"])</script>is rewritten to declare0x64(100) for a 101-byte body.protects_current_flight_fragmentreturnsfalsewhen no Next.js state exists. By code reading, this path is unchanged frommain, so it is out of scope here. It is worth a follow-up issue, since GTM-only deployments on App Router sites hit it with no Next.js involvement. - 👍 Prior-round fixes are well locked in.
trailing_next_f_references_preserve_current_and_later_flight,ordinary_gtm_before_captured_flight_is_chunk_invariantand the expanded superclass sweep cover every source split plus identity/gzip. TheSuppress→ unprotected change inrewrite_claimed_fragmentfixes chunk variance without touching GTM's raw fallback restoration.
Local validation (PR head b3538b47d)
cargo test -p trusted-server-core --lib --target aarch64-apple-darwin: 2,946 passedcargo test-fastly(Viceroy): pass (core 2,940 passed / 6 ignored; adapter 196 passed)cargo test-axum: passcargo clippy-fastly: passcargo fmt --all -- --check: pass
CI Status
All 19 checks pass, including cargo fmt, cargo test, axum, cloudflare, spin, the parity, CLI and integration suites, vitest, format-docs, format-typescript and CodeQL.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Approving. Script stages now compose instead of overwriting each other, ContentType::Html stops entity-escaping JS, GTM state is per-document, and the raw push-head observer from af69781 closes the fail-open lexer path from round 3. I found no fail-open path in Next.js-emitted spellings. The earlier round-1 and round-3 points are resolved or explicitly deferred.
Verified locally at 514a617: core lib tests (2949 passed), cargo test-spin, cargo test-axum, Fastly adapter under Viceroy (196 passed), and clippy-fastly / clippy-axum / clippy-spin-native / clippy-spin-wasm all clean.
CI note: 7 jobs (integration, browser integration, spin, format-typescript, CodeQL rust/python/js-ts) were cancelled with "job was not acquired by Runner", which is an infra failure, not code. Please re-run them before merge.
Non-blocking follow-ups
- 🤔 Inline-script cost with Next.js enabled (
rsc_placeholders.rs~L434/441/540). This adds numbers to the deferred perf thread. Release build, native arm64, 8 KB chunks, Next.js + GTM: a 2.1 MB non-Flight inline script goes from ~2 ms (base 8048301) to ~36 ms (~18×), and a 1.7 MB Flight push goes from ~8 ms to ~20 ms. WASM will be slower. A cheap pre-filter would avoid most of it: only mask and observe when the fragment plus a short carried tail contains__next_f, or when a head is in progress. Batched trimming instead of a per-chardrainwould also help. Worth a tracked issue if it stays deferred. - 🤔 Host validation applies to every proxied request (
publisher.rs~L4443,http_util.rs~L185), including assets that never reach a script rewriter. An emptyX-Forwarded-Host, aForwardedvalue that is not visible ASCII, or a valuelesshost=now returns 400 instead of falling back to a lower-priority header. Low impact on Fastly and Spin, which strip these headers, but notable for Axum or Cloudflare behind a proxy. Either document it as intended or tolerate an unparseableForwardedvalue that has nohost=. - ⛏ Redundant
RequestInfo::from_requestinhandle_page_bids(publisher.rs~L6985). It duplicates the extraction ~30 lines earlier, and the first binding is still live. - ⛏ About 40 new
assert!/assert_eq!calls lack a"should …"message (AGENTS.md). Examples:adapter-axum/tests/routes.rs:1079,adapter-fastly/src/app.rs:3191,test_support.rs, andnextjs/mod.rs. - 📝 For the deferred Flight GTM policy discussion:
rewrite_gtm_rsc_spanrewrites quotedanalytics.google.comURLs and bare origins without any GTM marker. The ordinary script path deliberately doesn't treat that domain as a trigger.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 514a617a0. The composition model, per-document GTM state, raw ContentType::Html emission and host validation hold up. Two new correctness bugs remain in the Flight path: a lexer miss can still leave invalid T lengths when the row continues in the next script (Next.js alone, a regression from main), and GTM's whole-span Flight routing can produce http:/integrations/… for a URL split after its scheme. The branch also conflicts with main, and the required format-typescript check never ran.
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 comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
- A Flight push the lexer misses still corrupts T lengths when its row continues in the next script: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:475 - Whole-span routing half-rewrites a URL split after its scheme across Flight payload scripts: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:204 - Conflicts with main: see cross-cutting below
- Required CI never ran: see cross-cutting below
Non-blocking
🤔 thinking / ♻️ refactor / ⛏ nitpick
- Non-null form state and binary pushes disable Flight rewriting for the rest of the page: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:40 - Skip the script handlers when no script rewriter is registered: see inline at
crates/trusted-server-core/src/html_processor.rs:665 - Domain-lint markers for PR-added test hosts: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:3636andcrates/trusted-server-core/src/integrations/nextjs/mod.rs:1216
Cross-cutting / body-level findings
-
🔧 Conflicts with main.
google_tag_manager.rsconflicts with #1179, which moved GTM's buffer to a per-documentScriptTextAccumulator(registry.rs, re-exported fromintegrations/mod.rs). A resolution that keeps this PR'sGtmScriptStatechecks out locally:- Take the PR side of both hunks (the struct and the top of
rewrite). Main's comment aboveget_or_insert_with, its// allow-domain:comment and its newan_interrupted_document_leaves_no_residue_for_the_next_documenttest merge cleanly. - Restore
Mutexinuse std::sync::{Arc, LazyLock, Mutex};(the auto-merge drops it) and drop the now-unuseduse crate::integrations::ScriptTextAccumulator;(clippy-D warningsfails on it). - Delete
ScriptTextAccumulatorfromregistry.rsand its re-export fromintegrations/mod.rs. After the merge it is public API with no users, and the deletion passes fmt and clippy.
On that merged tree,
cargo fmt --check,clippy-fastly,clippy-axum,test-axum,test-fastlyandtest-fastly-reusepass, as do 2,956 native core tests,template_build_digest(no fixture update needed: the build digest rotates automatically) and both of main's new regression tests against this PR's logic. - Take the PR side of both hunks (the struct and the top of
-
🔧 Required CI never ran. Seven jobs were cancelled with "The job was not acquired by Runner of type hosted even after multiple attempts" and never re-run:
format-typescript(required),integration tests,browser integration tests,cargo check/build/test (spin native + wasm32-wasip1),Analyze (rust),Analyze (javascript-typescript)andAnalyze (python). The ruleset requires the branch to be up to date with main, so the merge above will start a fresh run; please make sure these go green. -
📌 The ordinary GTM script path has the same scheme-split problem, and it predates this PR.
rewrite_gtm_urls(L515-537, called at L1182) triesGTM_WHOLE_URL_PATTERN, which is meant for attribute values, on held script text that can begin at any chunk boundary. A split just after the scheme givesj.src='http:/integrations/google_tag_manager/gtm.js?id='+i+dl, and a split at the opening quote leaves later URLs in the script unrewritten. Main behaves the same, so this is follow-up material: a quoted-only helper for the script path fixes it (the URL is then left unrewritten instead of corrupted). Relatedly, the retained boundary in the PR description ("a hostname split inside its initial six-byte marker prefix") understates the window: every split from just after the opening quote throughgooglmisses, about 17 positions for"https://www.googletagmanager.com. -
🌱 GTM and Next.js now depend on each other. GTM calls
nextjs::protects_current_flight_fragment(google_tag_manager.rs:1125),nextjs/mod.rsreadsGoogleTagManagerConfigand takesrewrite_gtm_rsc_span, and correctness depends onbuilders()order. A third integration that needs Flight protection or a Flight transform would add more cross-imports. A neutral home would fit better as a follow-up: a registry-owned "current fragment holds raw Flight" flag onIntegrationScriptContext, and a span-transform hook such asIntegrationRegistration::with_flight_span_transformthat GTM registers. -
🤔 The lexer keeps producing this bug class. The superclass case, the five round-3 prefixes and the continuation case above all come from
script_lexer.rsmisclassifying publisher code that shares a<script>with a Flight push. Next.js never emits that: each push is its own script. An alternative that removes most of the 513-line lexer is to capture only scripts whose whole content is a sequence of Next.js-shaped pushes, and to treat any other script with a raw qualified data head as unsupported (bypass_rsc), as main did for unsupported pushes. The trade-off is losing rewriting on pages where publisher code and a push share a script. Worth weighing before the next lexer fix. -
🏕 Docs and changelog.
docs/guide/integrations/google_tag_manager.md("1. Script Rewriting") doesn't mention that GTM/GA URLs inside App Router Flight payloads are now rewritten with recomputed T lengths, or that this depends onnextjsbeing enabled. No guide mentions the new 400 for a malformed effective host; main'sdocs/guide/ts-dev-proxy.md:291says core "prefersX-Forwarded-Host, thenHost", omittingForwardedand the 400.docs/guide/creative-processing.md:752still saysremove_node()deletes the script element, which contradicts the newScriptRewriteAction::RemoveNodedoc inregistry.rs.CHANGELOG.mdhas no[Unreleased]entry. -
🏕 Leftover function-local imports.
crates/trusted-server-adapter-axum/tests/routes.rs:883-885still hasuse std::sync::Arc;anduse trusted_server_core::test_support::nextjs_auction;insidenextjs_auction_output_holds_until_the_structural_body_close, now that the PR hoists both to module level. AGENTS.md asks for no local imports. -
♻️ Transform plumbing.
Option<fn(&str) -> String>now runs through eight signatures (rewrite_span, both rewrite functions, the factory and the processor), and the fix for the whole-span bug pushesrewrite_spanto seven arguments, the AGENTS.md limit. A smallSpanRewrite { rewriter, origin_host, request_host, request_scheme, transform }withfn apply(&self, span, record_text)would carry it. The single-payload loop (rsc.rs~L465-513) and the combined loop (~L715-763) differ only in the length function and could share one helper, andrewrite_rsc_scripts_combined_with_limitnow only forwardsNone.
Lower-priority notes (each verified, all minor):
- 🤔
html_processor.rs:667-679: the flat match-flag vector is reset at every<script>start tag and never restored at its end tag. Inside<svg>a<script>can nest, so the outer script's remaining text runs with the inner script's flags (<svg><script>a<script id="target">b</script>c</script></svg>feeds"c"to ascript#targetstage). Negligible in practice; worth a doc line. - 📝
http_util.rs:214-255accepts any single DNS label, includingscript. With that host, bare-host Flight rewriting (host_rewrite.rs) turns an origin-supplied raw</origin.example.com>into</script>. Not a regression (main had no validation, and Next.js never emits a raw<in Flight), but it qualifies the reasoning that validation makes raw emission safe. Not treating<or</before the host as a boundary would close it. - 🤔
http_util.rs:202-203: an emptyHost:now returns 400, while an absentHoststill yields an empty host. RFC 9112 sends an empty Host when the target has no authority, so the two cases mean the same thing. It only affects Axum or proxy setups. - 📝
rsc.rs:223(unchanged by this PR): the unescaped-length model miscounts\xe9,\0, octal escapes,\q, line continuations and\u{…}. Next.js'sJSON.stringifyplushtmlEscapeJsonStringnever emits them, and the GTM transform can't introduce them, so nothing is needed here. - 📝
google_tag_manager.rs:1147-1159: the limit counts held bytes plus the current fragment, so a single final fragment larger thanmax_buffered_script_bytesnow passes through unrewritten, where main rewrote it. Unreachable at the defaults (16 MiB limit, 8 KB chunks); a one-line comment would make the intent clear. - ⛏
google_tag_manager.rs:451-455:GtmScriptStateand its fields have no doc comments, and the release blocks at L1129-1134 and L1154-1159 are identical. Animpl GtmScriptState { fn release(&mut self, content: &str) -> ScriptRewriteAction }would remove the duplication. - ⛏
script_lexer.rs:MAX_TEMPLATE_DEPTH(L6) also caps braces, parentheses and pending classes, soMAX_LEXICAL_DEPTHwould read better;self.word.len() < 10(L130) is a magic number;ScriptLexer(L56) and itsexpression_start/statement_startinvariants are undocumented. Inrsc_placeholders.rs, slicingcodeat offsets taken fromcontentrelies onmaskemitting one#per masked byte; adebug_assert_eq!(code.len(), content.len())would document it. - ⛏
rsc.rs:626(mostly pre-existing): the classifier compares payload bytes with the limit, but the combined rewriter adds 7 marker bytes per boundary, so a group within7 * (n - 1)bytes of the limit is classified rewritable and then skipped with twowarn!lines. Removing the origin-host early exit for GTM-enabled groups makes this more reachable.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: CANCELLED
- browser integration tests: CANCELLED
- CodeQL: SKIPPED
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- prepare integration artifacts: PASS
- format-docs: PASS (required)
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- cargo test: PASS (required)
- cargo fmt: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- vitest: PASS
- cargo check/build/test (spin native + wasm32-wasip1): CANCELLED
- format-typescript: CANCELLED (required)
- Analyze (rust): CANCELLED
- Analyze (javascript-typescript): CANCELLED
- Analyze (python): CANCELLED
Brings #1222 at 514a617. Integration decisions: - RequestInfo::from_request keeps #1251's source order (authenticated forwarder, immutable ingress, runtime origin, Host) and applies #1222's host syntax check to whichever host is selected. It returns BadRequest for a malformed or non-text selected host. Raw Forwarded and X-Forwarded-Host are never selected in the RC, so #1222's tests for those headers now assert they are ignored instead of rejected. - RC-only callers (DataDome protection, Fastly compat, Spin, forwarder, orchestrator and publisher tests) handle the fallible signature. DataDome only reads RequestInfo behind an authenticated origin and keeps its request_host fallback. - The first-party sign path prefers the authenticated forwarder scheme (#1251) and propagates #1222's host error. - The GTM rewriter takes #1222's per-document GtmScriptState, which supersedes main's ScriptTextAccumulator use from #1179; state still lives in IntegrationDocumentState, and #1179's two-document residue tests remain.
… state, and apply approved cleanup.
|
@ChristianPavilonis I don't see resolution for the previous review. Can you please address with comment and resolve it? Thanks. |
Resolve merge conflicts preserving streamed rewrites and base regressions; exclude build-only git metadata from legacy settings parsing.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed at 6b317b558. Every round-1 blocker is resolved, and each fix was checked against the original repro: the lexer-miss continuation now fails closed with consistent T lengths, a Flight URL split after its scheme is preserved (no bad T header across about 100k sweep cases), non-null form state no longer disables rewriting, the handler guard and lint markers are applied, the branch is up to date with main, and all 22 checks pass.
Requesting changes for what is still open: how far an inert data-head hint should reach, the settings change that came in through a merge commit, the round-1 leftovers (ScriptTextAccumulator, the local imports, docs, changelog and PR description) and a test gap on the new record_text sites.
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 comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Requested before merge
- 🤔 Inert
self.__next_f.push([1,text in any script disables all later Flight rewriting: see inline atcrates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:35 - ♻️ Settings change from merge commit d0cfd88 duplicates open PR #1326: see inline at
crates/trusted-server-core/src/settings.rs:3057 - ♻️
ScriptTextAccumulatoris now dead public API: see inline atcrates/trusted-server-core/src/integrations/registry.rs:575 - ⛏ Function-local imports still present: see inline at
crates/trusted-server-adapter-axum/tests/routes.rs:10 - 🏕 Docs, changelog and PR description: see inline at
crates/trusted-server-core/src/integrations/registry.rs:69 - 🏕 Pin every
record_text = falsesite: see inline atcrates/trusted-server-core/src/integrations/nextjs/mod.rs:1322
Other findings
- 🤔 The document-wide Flight fallback is silent: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:586 - ⛏ Restore main's comment that the merge dropped: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:1135 - ⛏ Say why a combined record body is still safe to route: see inline at
crates/trusted-server-core/src/integrations/google_tag_manager.rs:194 - ♻️ Observer state sprawl: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_stream.rs:73 - ⛏ Oversized pass-through branch is unreachable through lol_html: see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc_placeholders.rs:258 - 📌 A removed
<script src>with an inline Flight push fails the whole response (pre-existing): see inline atcrates/trusted-server-core/src/html_processor.rs:684 - 📌 An unanchored T-header lookalike can leave a stale length (pre-existing): see inline at
crates/trusted-server-core/src/integrations/nextjs/rsc.rs:478
Lower-priority notes:
- 📝 A comment or BOM inside a data head (
push(/*c*/[1,orpush(\u{FEFF}[1,) still yields a stale T length, but only when the lexer also misses that push (for example afterfoo: { bar() } /'/.test(s);). Without such a prefix this head handles both,[1.0,and[0x1,correctly, and main is stale on the comment spelling even without a prefix, so this is not a regression. - ⛏ The
22ef7ca07subject is 103 characters and ends with a period (CONTRIBUTING.md asks for about 50 and no period). A squash merge makes this moot.
Round-1 verification
| Round-1 finding | Status at 6b317b558 |
|---|---|
| Lexer miss leaves a stale T length when the row continues in the next script | Fixed, fail-closed: all five prefixes give consistent lengths at chunk 16, 32 and 8192, Next.js only and mixed; the regression test fails when the bypass assignment is removed |
| Whole-span routing half-rewrites a URL split after its scheme | Fixed for the reported shape; see the test-gap comment for the other three span sites |
| Non-null form state disables later rewriting | Fixed; binary channel 3 keeps the document fallback by design, as documented |
| Script handlers registered with zero rewriters | Applied, byte-identical to the suggestion |
| Domain-lint markers | Applied; ts dev lint domains --changed-vs a9037770a reports 0 violations |
| Conflict with main | Resolved; up to date with main, main's #1179 regression tests pass |
| Required CI never ran | Resolved; all checks pass |
CI Status
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (rust): PASS
- Analyze (python): PASS
- cargo test (axum native): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (actions): PASS
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- prepare integration artifacts: PASS
- vitest: PASS
- Analyze (javascript-typescript): PASS
|
|
||
| // A missed data push may open a T row that a later captured script continues. | ||
| // Generic push hints still protect only their current script from ordinary GTM. | ||
| static QUALIFIED_FLIGHT_DATA_HEAD: LazyLock<Regex> = LazyLock::new(|| { |
There was a problem hiding this comment.
🤔 thinking: Inert self.__next_f.push([1, text in any script now disables all later Flight rewriting
This pattern runs on raw text, so a data-head spelling inside a string, comment, template literal or JSON-LD body of an unrelated script reaches the end-of-script bypass (L586) and turns off origin and GTM rewriting for every later Flight payload on the page. With a large input chunk it also restores earlier payloads still queued in that chunk. Each document below is an earlier Flight push, then the hint script, then a later push carrying origin and GTM URLs (*_origin=true means the origin URL was rewritten):
hint chunk head 6b317b558 main a9037770a (Next.js only)
plain string 8192 earlier_origin=false later_origin=false earlier_origin=true later_origin=true
plain string 16 earlier_origin=true later_origin=false earlier_origin=true later_origin=true
line or block comment 8192 earlier_origin=false later_origin=false earlier_origin=true later_origin=true
single quotes, template 8192 earlier_origin=false later_origin=false earlier_origin=true later_origin=true
JSON-LD articleBody 8192 earlier_origin=false later_origin=false earlier_origin=true later_origin=true
Generic push( hints, a captured push whose payload contains the text, form state and __NEXT_DATA__ do not trigger it. window.app.__next_f=1;self.__next_f.push([1,…]) in one script does: rewrite_fragment releases the whole fragment when the first __next_f has a foreign receiver (L403-421), and that release now escalates through this guard.
This costs coverage, never correctness, and our round-1 sketch proposed keying on exactly this text, so the trade-off is partly ours. The realistic trigger is a page whose content quotes the Flight push (developer blogs, docs). Two ways forward:
- (a) Accept it: pin it with a regression test (a JSON-LD hint followed by a later push, asserting the later push stays raw with consistent T lengths) and state it in the PR description.
- (b) Narrow it: escalate an uncaptured raw data head to the whole document only when the lexer ends the script outside clean code state, and keep per-script protection otherwise. In a probe, all five round-1 lexer-miss prefixes end non-clean and every hint case ends clean. The caveat is a contrived double trap that ends clean while masking a real push, which this head handles today.
Either way, picking the first __next_f with a qualified receiver in rewrite_fragment would remove the foreign-reference trigger. Two related edges: binary [3,…] has the same queued-restore chunk dependence (worth one sentence in the L605-608 doc), and form state nested deeper than the lexer's 32-level limit that also contains push text is bypassed by the opaque branch (L527) before rewrite_complete can recognize it.
| if ctx.is_last_in_text_node { | ||
| // Raw data can continue a row in the next script. Keep the entire | ||
| // remaining Flight stream raw, not merely this GTM snapshot. | ||
| state.bypass_rsc |= state.raw_data_seen || state.raw_data_unclaimed; |
There was a problem hiding this comment.
🤔 thinking: The document-wide Flight fallback is silent
When this line (or the opaque branch at L527, or the unsupported-push path) flips bypass_rsc, every later Flight payload on the page loses origin and GTM rewriting, yet rsc_placeholders.rs contains no log:: call. Every other Flight fallback logs log::warn!("Next.js RSC fallback: …") in rsc_stream.rs (L346, L384, L394, L401, L424, L561), and release_bypass only warns when a group is held. One log::warn! when bypass_rsc first goes from false to true would tell operators that a page lost Flight rewriting, and why.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🏕 camp site: Pin every record_text = false site, not just one
flight_url_split_after_scheme_preserves_the_whole_destination covers the single-payload before span (rsc.rs L458). Flipping any of the other three false sites to true (L474: a continuation that precedes a T row; L706: one grouped with the next payload by a trailing hex run; L724: one before a T body that spans the next payload) brings back http:/integrations/google_tag_manager/..., and all 3,013 existing native core tests still pass. The test below covers all three plus the complete-record case. Locally it passes at this head and fails under each of the three mutations:
left: "0:{\"href\":\"https://test.example.com/p\",\"tag\":\"http:/integrations/google_tag_manager/gtm.js?id=GTM-MIX1\"}\n1:T5,hello"
right: "0:{\"href\":\"https://test.example.com/p\",\"tag\":\"http://www.googletagmanager.com/gtm.js?id=GTM-MIX1\"}\n1:T5,hello"
| } | |
| } | |
| #[test] | |
| fn flight_url_split_after_scheme_is_preserved_at_every_span_site() { | |
| for scheme in ["http:", "https:"] { | |
| let body = json!({ | |
| "tag": format!("{scheme}//www.googletagmanager.com/gtm.js?id=GTM-MIX1"), | |
| "href": "https://origin.example.com/p", | |
| }); | |
| let payload = format!("0:{body}\n"); | |
| let split = payload | |
| .find("//www.") | |
| .expect("should locate the tag slashes"); | |
| let (head, continuation) = payload.split_at(split); | |
| // The continuation precedes a T row, is grouped by a trailing hex run, | |
| // or precedes a T row whose body spans the next payload. | |
| for parts in [ | |
| vec![head.to_owned(), format!("{continuation}1:T5,hello")], | |
| vec![ | |
| head.to_owned(), | |
| format!("{continuation}2:\"ab"), | |
| "cd\"\n".to_owned(), | |
| ], | |
| vec![ | |
| head.to_owned(), | |
| format!("{continuation}1:T5,he"), | |
| "llo".to_owned(), | |
| ], | |
| ] { | |
| let refs: Vec<&str> = parts.iter().map(String::as_str).collect(); | |
| let expected = parts | |
| .concat() | |
| .replace("origin.example.com", "test.example.com"); | |
| for chunk_size in [16, 8192] { | |
| let payloads = flight_payloads(&mixed_output(&flight_html(&refs), chunk_size)); | |
| assert_eq!( | |
| payloads.len(), | |
| parts.len(), | |
| "should retain payload boundaries" | |
| ); | |
| assert_eq!( | |
| payloads.concat(), | |
| expected, | |
| "should preserve a GTM URL split after its scheme at every span site" | |
| ); | |
| } | |
| } | |
| } | |
| let url = "http://www.googletagmanager.com/gtm.js?id=GTM-MIX1"; | |
| let record = format!("1:T{:x},{url}", url.len()); | |
| let split = record.len() - "MIX1".len(); | |
| let payloads = flight_payloads(&mixed_output( | |
| &flight_html(&[&record[..split], &record[split..]]), | |
| 8192, | |
| )); | |
| let routed = "/integrations/google_tag_manager/gtm.js?id=GTM-MIX1"; | |
| assert_eq!( | |
| payloads.concat(), | |
| format!("1:T{:x},{routed}", routed.len()), | |
| "should route a whole T-record URL whose query spans payload scripts" | |
| ); | |
| } |
| // Per document, never per registry: a registry-lifetime buffer would | ||
| // carry one document's partial script into the next. | ||
| let accumulator = ctx | ||
| let state = ctx |
There was a problem hiding this comment.
⛏ nitpick: Restore main's comment that the merge dropped
The conflict resolution in d0cfd88 dropped main's (#1179) comment explaining why this state is per document. Our round-1 note said that comment would merge cleanly, which was wrong: it sat inside the conflict hunk.
| let state = ctx | |
| // Per document, never per registry: a registry-lifetime buffer would | |
| // carry one document's partial script into the next. | |
| let state = ctx |
| /// `record_text` marks exactly one complete T-record body. Other spans can start | ||
| /// mid-URL at a payload-script boundary and must not be routed as whole URLs. |
There was a problem hiding this comment.
⛏ nitpick: Say why a combined record body is still safe to route
In the combined path a complete T-record body can contain RSC_MARKER, i.e. a payload boundary inside a URL. That is safe because a marker inside the routed prefix (scheme, slashes, host, path) prevents a match, and one in the suffix is copied verbatim; a sweep over URLs split at every position inside a spanning T body confirmed both. Worth stating in the doc:
| /// `record_text` marks exactly one complete T-record body. Other spans can start | |
| /// mid-URL at a payload-script boundary and must not be routed as whole URLs. | |
| /// `record_text` marks exactly one complete T-record body. Other spans can start | |
| /// mid-URL at a payload-script boundary and must not be routed as whole URLs. | |
| /// A record body may still span payloads: a boundary marker inside the routed | |
| /// prefix prevents routing, and one in the suffix is copied verbatim. |
| Keep, | ||
| /// Pass replacement text to subsequent matching stages. | ||
| Replace(String), | ||
| /// Suppress this fragment; downstream stages still run and may flush held text. |
There was a problem hiding this comment.
🏕 camp site: Docs, changelog and PR description
Round 1's docs items are still open, and the description has drifted since 22ef7ca:
docs/guide/integrations/google_tag_manager.md("1. Script Rewriting") doesn't mention that GTM/GA URLs inside App Router Flight payloads are now rewritten with recomputed T lengths, or that this depends onnextjsbeing enabled.- No guide mentions the new 400 for a malformed effective host;
docs/guide/ts-dev-proxy.md:291still says core "prefersX-Forwarded-Host, thenHost". docs/guide/creative-processing.md:752saysremove_node()deletes the script element, which contradicts theRemoveNodedoc here.CHANGELOG.mdhas no[Unreleased]entry.- PR description: "Unsupported or unsafe payloads retain their original data" reads as per payload, but an uncaptured data head, an inert data-head hint (see the comment on
rsc_placeholders.rs:35) and binary[3,…]now keep the rest of the document raw. The newly inert channel-2 form state andsettings.rsare missing, the test counts are stale (3,013 native core tests pass at this head), and main's Require specs for feature PRs #1385 template now asks for a## Specline ("Not a feature" for a fix).
| } | ||
| let script_patterns = patterns.clone(); | ||
| let script_document_state = document_state.clone(); | ||
| element_content_handlers.push(text!("script", move |text| { |
There was a problem hiding this comment.
📌 out of scope: A removed <script src> with an inline Flight push fails the whole response (pre-existing)
When an attribute rewriter removes a script element (the Prebid integration does this for publisher Prebid scripts), lol_html still runs this text handler on the removed element's content. If that body is a Flight push, Next.js captures it and queues a placeholder, lol_html drops the output, and the next placeholder reaches the stream processor out of order:
<script src="https://cdn.example.com/prebid.js">self.__next_f.push([1,"0:{\"url\":\"https://origin.example.com/a\"}\n"])</script>
<script>…normal Flight push…</script>
Next.js + GTM and Next.js only: ERR(Next.js RSC output contains an unknown generated placeholder),
for the whole document and all 174 two-way splits
Main behaves identically, browsers ignore the inline body of a src script, and Next.js never emits this shape, so it is not for this PR. Worth a follow-up issue: skipping the composed stages for a removed element would close it.
| let chunk_content = &content[chunk.header_end..chunk.content_end]; | ||
| let rewritten_content = | ||
| rewriter.rewrite_to_string(chunk_content, origin_host, request_host, request_scheme); | ||
| let rewritten_content = rewrite_span( |
There was a problem hiding this comment.
📌 out of scope: An unanchored T-header lookalike can leave a stale length (pre-existing, now also for GTM URLs)
TCHUNK_PATTERN (L11) matches anywhere, so a lookalike inside a JSON string swallows the real T header that follows it, and a rewrite inside the swallowed range leaves that header stale. Decoded input 0:["see 1:T41,"]\n2:T2e,{"u":"http://www.googletagmanager.com/gtm.js"}3:"xxxx…"\n comes out as 2:T2e,{"u":"/integrations/google_tag_manager/gtm.js"} (46 bytes declared, 47 actual). The same scan already does this to origin-host rewrites on main (39 declared, 37 actual for an origin URL), so this PR extends an existing failure rather than introducing it. Follow-up material: anchor T-header discovery to row starts.
| pub(super) raw_data_seen: bool, | ||
| /// Data observed in released source cannot be accounted for by a later buffer. | ||
| pub(super) raw_data_unclaimed: bool, | ||
| pub(super) raw_data_tail: String, |
There was a problem hiding this comment.
♻️ refactor: Observer state sprawl
NextJsDocumentState now carries three parallel observer groups (flight_qualifier_tail/flight_node_owned, raw_qualifier_tail/raw_flight_seen/raw_flight_unclaimed, raw_data_tail/raw_data_seen/raw_data_unclaimed), reset field by field in rsc_placeholders.rs (L476-486, L586-599, with partial clears at L190 and L303). A HeadObservation { tail, seen, unclaimed } with a reset() would make a missed field impossible. Smaller cleanups in the same area: the same slices are scanned once per regex (L116-119, L170-173, L180, L198-200), the if was_buffered { replace } else { Keep } return arm repeats five times, and the 2\s*,\s*null branch of INERT_FLIGHT_CONTROL_ARGUMENTS is now covered by the serde form-state path.
| if !state.flight_node_owned { | ||
| return ScriptRewriteAction::Keep; | ||
| } | ||
| if is_last && content.len() > limit && content.contains("__next_f") { |
There was a problem hiding this comment.
⛏ nitpick: This oversized pass-through branch is unreachable through lol_html
lol_html delivers every non-empty script as non-last fragments followed by an empty last fragment, so Idle && is_last && content.len() > limit only happens in unit tests; oversized scripts take BypassUntilLast instead. The branch's head_start also comes from the first qualified push rather than the captured call, so a raw data head between the two would not be scanned. A comment marking it as defensive, or slicing the data check at the captured call's start, would make that explicit.
This branch has not been deployed
4 participants
Summary
Changes
Core paths below are relative to
crates/trusted-server-core/src/.html_processor.rshttp_util.rspublisher.rsproxy.rsintegrations/mod.rsintegrations/registry.rsintegrations/prebid.rsintegrations/google_tag_manager.rsintegrations/nextjs/mod.rsintegrations/nextjs/rsc.rsintegrations/nextjs/rsc_placeholders.rsintegrations/nextjs/rsc_stream.rsintegrations/nextjs/script_rewriter.rsintegrations/nextjs/script_lexer.rstest_support.rscrates/trusted-server-adapter-axum/tests/routes.rscrates/trusted-server-adapter-fastly/src/app.rsScope
These three issues share the same script-processing defect, so this PR fixes them together. The diff is mostly regression tests and test fixtures covering fragment boundaries, compression, multiple payloads, buffering limits, bootstrap scripts, and escaped queries. Runtime changes stay in the shared Rust processing path; the adapter changes are tests only.
There are no new settings, dependencies, integration-order changes, or adapter error-handling changes. Missing generated placeholders still fail rather than silently hiding a lost rewrite.
Retained boundaries:
Deferred review discussion
Closes
Closes #1206
Closes #1207
Closes #1208
Test plan
Regression failures were recorded before their corresponding fixes, including lost placeholders, invalid JSON, duplicate/reordered fragments, bootstrap bypass, escaped-query misses, and raw-source escaping. The malformed-host adapter regression returns 200 on the original code and 400 after validation. The deferred Flight-policy concern is recorded above.
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npm exec --no -- vitest runusing the installed Vitest without dependency installation.cd crates/trusted-server-js/lib && npm run formatnpm run formatscript passed with a command-local PATH to the existing Prettier 3.8.1, matching the docs lockfile; the ordinary command cannot resolve Prettier because docs-local dependencies are absent.cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serve; direct Viceroy client testing was used instead.Final counts include 2,883 native core tests, 14 adapter parity tests, and 1,155 JavaScript tests. Existing target-specific panic/doctest skips remain; native panic counterparts and all CLI browser fixtures ran. Local transport testing used debug WASM and a synthetic HTTP origin, not production TLS.
Checklist
unwrap()in new production code; useexpect("should ...").logmacros rather thanprintln!, as required by AGENTS.md.