Repository navigation
Extract GTM container IDs from audit evidence - #1220
ChristianPavilonis wants to merge 4 commits into
Conversation
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Approved. The audit extractor now returns only the matched GTM container ID from integration evidence, fixing generated configuration that previously contained the full script URL. Reviewed both changed files, evidence producers, config generation, runtime validation, and regression coverage; no findings.
The regression tests cover bare IDs, URLs with and without trailing query parameters, and generated TOML enabling GTM with the extracted ID.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- cargo test (ts CLI, native): PASS
- Analyze (rust): PASS
- CodeQL: PASS
- cargo fmt: PASS (required)
- Analyze (actions): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- format-docs: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (axum native): PASS
- Analyze (javascript-typescript): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- CLAUDE.md symlink guard: PASS
The CLI test and lint jobs were verified at reviewed head 9fd27e460c88052a2de039302a042ea6e819f6e1. CI was not rerun locally; whitespace checks pass and the review worktree is clean.
dhruv8sh
left a comment
dhruv8sh
left a comment
There was a problem hiding this comment.
Summary
Fixes #1219 by returning only the GTM_REGEX match from integration evidence instead of the whole evidence string, so URL evidence no longer produces an invalid container_id in the generated draft config. The fix is minimal and well covered by a table test plus an end-to-end TOML test; no blocking issues.
Non-blocking
🌱 seedling / ♻️ refactor
- CLI regex still accepts IDs that runtime validation rejects — see inline at
crates/trusted-server-cli/src/commands/audit/generate/analyzer.rs:243 - Merge the two lookup loops (optional) — see inline at
crates/trusted-server-cli/src/commands/audit/generate/analyzer.rs:241-246
👍 praise
- Fix matches the existing asset fallback — see inline at
crates/trusted-server-cli/src/commands/audit/generate/analyzer.rs:242-246
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- cargo test (ts CLI, native): PASS
- Analyze (rust): PASS
- CodeQL: PASS
- cargo fmt: PASS (required)
- Analyze (actions): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- format-docs: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (axum native): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- CLAUDE.md symlink guard: PASS
| if integration.id == "google_tag_manager" | ||
| && let Some(matched) = GTM_REGEX.find(&integration.evidence) | ||
| { | ||
| return Some(matched.as_str().to_string()); | ||
| } |
There was a problem hiding this comment.
👍 praise — Nice minimal fix: switching from is_match + returning the whole evidence to find(..).as_str() mirrors the asset fallback right below, so both lookup paths now yield the same normalized ID. The end-to-end test in mod.rs that parses the generated TOML (rather than string-matching) is a good guard against the original symptom.
| if integration.id == "google_tag_manager" && GTM_REGEX.is_match(&integration.evidence) { | ||
| return Some(integration.evidence.clone()); | ||
| if integration.id == "google_tag_manager" | ||
| && let Some(matched) = GTM_REGEX.find(&integration.evidence) |
There was a problem hiding this comment.
🌱 seedling — Related edge this PR doesn't need to fix: the CLI's GTM_REGEX (\bGTM-[A-Z0-9]+\b, line 15) is looser than the runtime validator GTM_CONTAINER_ID_PATTERN (^GTM-[A-Z0-9]{4,20}$ in trusted-server-core/src/integrations/google_tag_manager.rs). An ID with fewer than 4 or more than 20 characters after GTM- would still be extracted and written to the draft, then rejected at startup — the same class of failure #1219 describes. A follow-up could bound the quantifier so extraction and validation agree:
static GTM_REGEX: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"\bGTM-[A-Z0-9]{4,20}\b").expect("should compile GTM regex"));(Note this regex is also used by detect_integrations_from_inline_script, so tightening it changes detection too — alternatively validate only in extract_gtm_container_id and route non-conforming IDs to manual review.)
| for integration in &artifact.detected_integrations { | ||
| if integration.id == "google_tag_manager" && GTM_REGEX.is_match(&integration.evidence) { | ||
| return Some(integration.evidence.clone()); | ||
| if integration.id == "google_tag_manager" | ||
| && let Some(matched) = GTM_REGEX.find(&integration.evidence) | ||
| { | ||
| return Some(matched.as_str().to_string()); | ||
| } |
There was a problem hiding this comment.
♻️ refactor — Optional: now that both paths do the same GTM_REGEX.find → to_string, the two loops could collapse into one chain:
pub(crate) fn extract_gtm_container_id(artifact: &AuditArtifact) -> Option<String> {
let evidence = artifact
.detected_integrations
.iter()
.filter(|integration| integration.id == "google_tag_manager")
.map(|integration| integration.evidence.as_str());
let asset_urls = artifact
.assets
.iter()
.filter(|asset| asset.integration.as_deref() == Some("google_tag_manager"))
.map(|asset| asset.url.as_str());
evidence
.chain(asset_urls)
.find_map(|text| GTM_REGEX.find(text))
.map(|matched| matched.as_str().to_string())
}Apply manually — can't be auto-applied as a suggestion because the replacement spans lines 241-257 and the asset loop (249-257) is outside the diff hunk.
| if integration.id == "google_tag_manager" && GTM_REGEX.is_match(&integration.evidence) { | ||
| return Some(integration.evidence.clone()); | ||
| if integration.id == "google_tag_manager" | ||
| && let Some(matched) = GTM_REGEX.find(&integration.evidence) |
There was a problem hiding this comment.
♻️ refactor: Bound GTM_REGEX to the runtime container ID length
This fixes the URL case, but the extracted token still isn't guaranteed to pass the validation #1219 is about. GTM_REGEX (line 15) is \bGTM-[A-Z0-9]+\b, while GoogleTagManagerConfig validates ^GTM-[A-Z0-9]{4,20}$. Any GTM-shaped token outside 4..=20 characters is still written as enabled = true with a container_id that deploy validation rejects. Running drafts through validate::check_candidate:
| evidence | written container_id |
deploy validation |
|---|---|---|
…/gtm.js?id=GTM-ABC123&l=dataLayer |
GTM-ABC123 |
pass |
…/gtm.js?id=GTM-ABC |
GTM-ABC |
fail |
…/gtm.js?id=GTM-ABCDEFGHIJKLMNOPQRSTU |
the 21-char ID | fail |
inline script containing GTM-ID |
GTM-ID |
fail |
With the quantifier bounded, all three fall through to the existing manual-review comment and the draft validates:
static GTM_REGEX: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"\bGTM-[A-Z0-9]{4,20}\b").expect("should compile GTM regex"));Regression test (fails at this head, passes with the change above):
#[test]
fn extract_gtm_container_id_skips_ids_outside_runtime_length() {
for evidence in [
"https://tags.example.com/gtm.js?id=GTM-ABC",
"https://tags.example.com/gtm.js?id=GTM-ABCDEFGHIJKLMNOPQRSTU",
] {
let artifact = AuditArtifact {
audited_url: "https://example.com".to_string(),
page_title: None,
js_asset_count: 0,
third_party_asset_count: 0,
detected_integrations: vec![DetectedIntegration {
id: "google_tag_manager".to_string(),
evidence: evidence.to_string(),
}],
assets: Vec::new(),
warnings: Vec::new(),
};
assert_eq!(
extract_gtm_container_id(&artifact),
None,
"should leave evidence {evidence} for manual review"
);
}
}One side effect to be aware of: detect_integrations_from_inline_script shares this regex, so an inline-only out-of-range token stops being reported as detected GTM rather than landing in the manual-review list. The runtime would reject that token anyway, so that seems acceptable; if you'd rather keep detection loose, apply the bound only inside extract_gtm_container_id.
Apply manually: line 15 is outside the diff hunks, so this can't be a one-click suggestion. With this change plus the two test suggestions on this PR, the full CLI lib suite (725 passed), cargo fmt --check, and cargo clippy-cli are green.
| let url = Url::parse("https://example.com").expect("should parse URL"); | ||
| let artifact = AuditArtifact { | ||
| audited_url: url.to_string(), | ||
| page_title: None, | ||
| js_asset_count: 0, | ||
| third_party_asset_count: 0, | ||
| detected_integrations: vec![DetectedIntegration { | ||
| id: "google_tag_manager".to_string(), | ||
| evidence: "https://tags.example.com/gtm.js?id=GTM-ABC123&l=dataLayer".to_string(), | ||
| }], | ||
| assets: Vec::new(), | ||
| warnings: Vec::new(), | ||
| }; | ||
|
|
||
| let draft = build_draft_config(&url, &artifact, &gpt_slots::DiscoveredSlots::default()) | ||
| .expect("should build draft config"); | ||
| let config = toml::from_str::<toml::Value>(&draft).expect("should parse draft config"); | ||
| let gtm = &config["integrations"]["google_tag_manager"]; | ||
|
|
||
| assert_eq!( | ||
| gtm["enabled"].as_bool(), | ||
| Some(true), | ||
| "should enable GTM when URL evidence contains a container ID" | ||
| ); | ||
| assert_eq!( | ||
| gtm["container_id"].as_str(), | ||
| Some("GTM-ABC123"), | ||
| "should write only the container ID rather than the evidence URL" | ||
| ); |
There was a problem hiding this comment.
♻️ refactor: Assert the #1219 symptom, that the draft passes deploy validation
The issue's repro ends with "Validate the generated configuration", but this test stops at the field value. It can't check validation as written either: with https://example.com the draft's publisher.domain becomes the placeholder example.com, so deploy validation fails for an unrelated reason. Switching to a non-placeholder host lets the test assert the outcome the issue is about, using the same check_candidate(&config, &config) pattern as validate::tests::valid_candidate_passes_without_warnings.
| let url = Url::parse("https://example.com").expect("should parse URL"); | |
| let artifact = AuditArtifact { | |
| audited_url: url.to_string(), | |
| page_title: None, | |
| js_asset_count: 0, | |
| third_party_asset_count: 0, | |
| detected_integrations: vec![DetectedIntegration { | |
| id: "google_tag_manager".to_string(), | |
| evidence: "https://tags.example.com/gtm.js?id=GTM-ABC123&l=dataLayer".to_string(), | |
| }], | |
| assets: Vec::new(), | |
| warnings: Vec::new(), | |
| }; | |
| let draft = build_draft_config(&url, &artifact, &gpt_slots::DiscoveredSlots::default()) | |
| .expect("should build draft config"); | |
| let config = toml::from_str::<toml::Value>(&draft).expect("should parse draft config"); | |
| let gtm = &config["integrations"]["google_tag_manager"]; | |
| assert_eq!( | |
| gtm["enabled"].as_bool(), | |
| Some(true), | |
| "should enable GTM when URL evidence contains a container ID" | |
| ); | |
| assert_eq!( | |
| gtm["container_id"].as_str(), | |
| Some("GTM-ABC123"), | |
| "should write only the container ID rather than the evidence URL" | |
| ); | |
| let url = Url::parse("https://publisher.example.com").expect("should parse URL"); | |
| let artifact = AuditArtifact { | |
| audited_url: url.to_string(), | |
| page_title: None, | |
| js_asset_count: 0, | |
| third_party_asset_count: 0, | |
| detected_integrations: vec![DetectedIntegration { | |
| id: "google_tag_manager".to_string(), | |
| evidence: "https://tags.example.com/gtm.js?id=GTM-ABC123&l=dataLayer".to_string(), | |
| }], | |
| assets: Vec::new(), | |
| warnings: Vec::new(), | |
| }; | |
| let draft = build_draft_config(&url, &artifact, &gpt_slots::DiscoveredSlots::default()) | |
| .expect("should build draft config"); | |
| let config = toml::from_str::<toml::Value>(&draft).expect("should parse draft config"); | |
| let gtm = &config["integrations"]["google_tag_manager"]; | |
| assert_eq!( | |
| gtm["enabled"].as_bool(), | |
| Some(true), | |
| "should enable GTM when URL evidence contains a container ID" | |
| ); | |
| assert_eq!( | |
| gtm["container_id"].as_str(), | |
| Some("GTM-ABC123"), | |
| "should write only the container ID rather than the evidence URL" | |
| ); | |
| let warnings = | |
| validate::check_candidate(&draft, &draft).expect("should validate draft config"); | |
| assert!( | |
| warnings.is_empty(), | |
| "should pass deploy validation, got {warnings:?}" | |
| ); |
| "should extract only the container ID from evidence {evidence}" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
♻️ refactor: Pin evidence precedence and the asset fallback
#1219 names "URL evidence taking precedence over the asset fallback" as the missing coverage, but both new tests use assets: Vec::new(), so neither precedence nor fall-through is exercised. Two plausible regressions pass every existing GTM test today: collapsing the first loop to return GTM_REGEX.find(..).map(..) (which drops the asset fallback when the evidence is a gtag/js?id=G-… URL), and checking assets before evidence. The test below fails on both and passes at this head.
| } | |
| } | |
| #[test] | |
| fn extract_gtm_container_id_prefers_evidence_then_falls_back_to_assets() { | |
| for (evidence, expected) in [ | |
| ( | |
| "https://tags.example.com/gtm.js?id=GTM-AAA111", | |
| "GTM-AAA111", | |
| ), | |
| ("https://tags.example.com/gtag/js?id=G-ABC123", "GTM-BBB222"), | |
| ] { | |
| let artifact = AuditArtifact { | |
| audited_url: "https://example.com".to_string(), | |
| page_title: None, | |
| js_asset_count: 1, | |
| third_party_asset_count: 1, | |
| detected_integrations: vec![DetectedIntegration { | |
| id: "google_tag_manager".to_string(), | |
| evidence: evidence.to_string(), | |
| }], | |
| assets: vec![AuditedAsset { | |
| kind: "script".to_string(), | |
| url: "https://tags.example.com/gtm.js?id=GTM-BBB222".to_string(), | |
| host: "tags.example.com".to_string(), | |
| party: AssetParty::ThirdParty, | |
| integration: Some("google_tag_manager".to_string()), | |
| }], | |
| warnings: Vec::new(), | |
| }; | |
| assert_eq!( | |
| extract_gtm_container_id(&artifact).as_deref(), | |
| Some(expected), | |
| "should resolve {expected} for evidence {evidence}" | |
| ); | |
| } | |
| } |
This branch has not been deployed
4 participants
Summary
Changes
crates/trusted-server-cli/src/commands/audit/generate/analyzer.rscrates/trusted-server-cli/src/commands/audit/generate/mod.rsCloses
Closes #1219
Test plan
Both new regression tests failed before the fix by returning the full URL instead of
GTM-ABC123, then passed after the fix.cargo test --package trusted-server-cli --target x86_64-unknown-linux-gnu gtm_container_id./scripts/test-cli.sh, including opt-in browser fixturescargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity, 14 passedcargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm && cargo clippy-cli && cargo clippy-codegencargo fmt --all -- --checkcd crates/trusted-server-js/lib && node build-all.mjscd crates/trusted-server-js/lib && npx vitest run, 1,155 passedcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatNo deployment or manual Fastly smoke test performed. This changes only CLI extraction logic and its tests.
Checklist
unwrap()or logging changes