Address audit review feedback

ChristianPavilonis · ChristianPavilonis · commit dfa8a2985b75 · 2026-06-29T12:56:17.000-05:00
diff --git a/crates/trusted-server-cli/src/audit.rs b/crates/trusted-server-cli/src/audit.rs
@@ -421,16 +421,20 @@ mod tests {
             requested_url: "https://publisher.example/page".to_string(),
             final_url: "https://publisher.example/page".to_string(),
             page_title: Some("Example Publisher".to_string()),
-            html: r#"<html><head><title>Example Publisher</title><script src="https://securepubads.g.doubleclick.net/tag/js/gpt.js"></script></head></html>"#.to_string(),
-            script_tags: vec![CollectedScriptTag {
-                src: Some("https://www.googletagmanager.com/gtm.js?id=GTM-ABC123".to_string()),
-                inline_text: None,
-            }],
+            html: r#"<html><head><title>Example Publisher</title></head></html>"#.to_string(),
+            script_tags: vec![
+                CollectedScriptTag {
+                    src: Some("https://www.googletagmanager.com/gtm.js?id=GTM-ABC123".to_string()),
+                    inline_text: None,
+                },
+                CollectedScriptTag {
+                    src: Some("https://securepubads.g.doubleclick.net/tag/js/gpt.js".to_string()),
+                    inline_text: None,
+                },
+            ],
             network_requests: vec![CollectedRequest {
                 url: "https://cdn.publisher.example/app.js".to_string(),
-                method: "GET".to_string(),
                 resource_type: Some("script".to_string()),
-                status: None,
             }],
             warnings: Vec::new(),
         }
diff --git a/crates/trusted-server-cli/src/audit/analyzer.rs b/crates/trusted-server-cli/src/audit/analyzer.rs
@@ -11,6 +11,24 @@ use crate::error::{report_error, CliResult};
 
 static GTM_REGEX: LazyLock<Regex> =
     LazyLock::new(|| Regex::new(r"\bGTM-[A-Z0-9]+\b").expect("should compile GTM regex"));
+static GPT_INLINE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"(?i)\b(?:googletag|gpt\.js|googletagservices|securepubads)\b")
+        .expect("should compile GPT inline regex")
+});
+static DIDOMI_INLINE_REGEX: LazyLock<Regex> =
+    LazyLock::new(|| Regex::new(r"(?i)\bdidomi\b").expect("should compile Didomi inline regex"));
+static DATADOME_INLINE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"(?i)\bdatadome\b").expect("should compile DataDome inline regex")
+});
+static PERMUTIVE_INLINE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"(?i)\bpermutive\b").expect("should compile Permutive inline regex")
+});
+static LOCKR_INLINE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"(?i)(?:\blockr\b|\bloc\.kr\b)").expect("should compile Lockr inline regex")
+});
+static PREBID_INLINE_REGEX: LazyLock<Regex> = LazyLock::new(|| {
+    Regex::new(r"(?i)(?:\bprebid\b|\bpbjs\b)").expect("should compile Prebid inline regex")
+});
 
 pub(crate) fn analyze_collected_page(collected: &CollectedPage) -> CliResult<AuditArtifact> {
     let final_url = collected
@@ -22,7 +40,6 @@ pub(crate) fn analyze_collected_page(collected: &CollectedPage) -> CliResult<Aud
 
     let document = Html::parse_document(&collected.html);
     let title_selector = Selector::parse("title").expect("should parse title selector");
-    let script_selector = Selector::parse("script").expect("should parse script selector");
     let derived_title = document
         .select(&title_selector)
         .next()
@@ -46,30 +63,15 @@ pub(crate) fn analyze_collected_page(collected: &CollectedPage) -> CliResult<Aud
         ));
     }
 
-    for element in document.select(&script_selector) {
-        if let Some(src) = element.value().attr("src") {
+    for tag in &collected.script_tags {
+        if let Some(src) = &tag.src {
             if let Ok(asset_url) = final_url.join(src) {
                 let integration = detect_integration_from_url(&asset_url);
                 record_integration(&mut integrations, &integration, asset_url.as_str());
                 insert_asset(&mut assets_by_url, &final_url, &asset_url, integration);
             } else {
                 warnings.push(format!("could not resolve script URL `{src}`"));
             }
-        } else {
-            let inline_text = element.text().collect::<Vec<_>>().join(" ");
-            for (integration_id, evidence) in detect_integrations_from_inline_script(&inline_text) {
-                integrations.entry(integration_id).or_insert(evidence);
-            }
-        }
-    }
-
-    for tag in &collected.script_tags {
-        if let Some(src) = &tag.src {
-            if let Ok(asset_url) = Url::parse(src) {
-                let integration = detect_integration_from_url(&asset_url);
-                record_integration(&mut integrations, &integration, asset_url.as_str());
-                insert_asset(&mut assets_by_url, &final_url, &asset_url, integration);
-            }
         }
 
         if let Some(inline_text) = &tag.inline_text {
@@ -167,6 +169,7 @@ pub(crate) fn classify_party(page_url: &Url, asset_url: &Url) -> AssetParty {
 }
 
 fn host_matches(page_host: &str, asset_host: &str) -> bool {
+    // This is an advisory heuristic, not public-suffix-aware eTLD+1 classification.
     asset_host == page_host
         || asset_host
             .strip_suffix(page_host)
@@ -213,9 +216,15 @@ pub(crate) fn detect_integrations_from_inline_script(script: &str) -> Vec<(Strin
         ));
     }
 
-    let lowered = script.to_ascii_lowercase();
-    for integration in ["gpt", "didomi", "datadome", "permutive", "lockr", "prebid"] {
-        if lowered.contains(integration) {
+    for (integration, regex) in [
+        ("gpt", &*GPT_INLINE_REGEX),
+        ("didomi", &*DIDOMI_INLINE_REGEX),
+        ("datadome", &*DATADOME_INLINE_REGEX),
+        ("permutive", &*PERMUTIVE_INLINE_REGEX),
+        ("lockr", &*LOCKR_INLINE_REGEX),
+        ("prebid", &*PREBID_INLINE_REGEX),
+    ] {
+        if regex.is_match(script) {
             matches.push((
                 integration.to_string(),
                 format!("inline script matched `{integration}`"),
@@ -259,16 +268,20 @@ mod tests {
             requested_url: "https://publisher.example/page".to_string(),
             final_url: "https://publisher.example/page".to_string(),
             page_title: Some("Browser Title".to_string()),
-            html: r#"<html><head><title>HTML Title</title><script src="https://www.googletagmanager.com/gtm.js?id=GTM-ABCD123"></script></head></html>"#.to_string(),
-            script_tags: vec![CollectedScriptTag {
-                src: Some("https://securepubads.g.doubleclick.net/tag/js/gpt.js".to_string()),
-                inline_text: None,
-            }],
+            html: r#"<html><head><title>HTML Title</title></head></html>"#.to_string(),
+            script_tags: vec![
+                CollectedScriptTag {
+                    src: Some("https://www.googletagmanager.com/gtm.js?id=GTM-ABCD123".to_string()),
+                    inline_text: None,
+                },
+                CollectedScriptTag {
+                    src: Some("https://securepubads.g.doubleclick.net/tag/js/gpt.js".to_string()),
+                    inline_text: None,
+                },
+            ],
             network_requests: vec![CollectedRequest {
                 url: "https://cdn.example.com/dynamic.js".to_string(),
-                method: "GET".to_string(),
                 resource_type: Some("Script".to_string()),
-                status: Some(200),
             }],
             warnings: vec!["partial settle".to_string()],
         };
@@ -345,9 +358,7 @@ mod tests {
             }],
             network_requests: vec![CollectedRequest {
                 url: "https://cdn.example.com/prebid.js".to_string(),
-                method: "GET".to_string(),
                 resource_type: Some("script".to_string()),
-                status: Some(200),
             }],
             warnings: Vec::new(),
         };
@@ -371,8 +382,17 @@ mod tests {
             requested_url: "https://publisher.example/page".to_string(),
             final_url: "https://publisher.example/path/page".to_string(),
             page_title: None,
-            html: r#"<html><head><script src="/static/app.js"></script><script src="http://[invalid"></script></head></html>"#.to_string(),
-            script_tags: Vec::new(),
+            html: "<html><head></head></html>".to_string(),
+            script_tags: vec![
+                CollectedScriptTag {
+                    src: Some("/static/app.js".to_string()),
+                    inline_text: None,
+                },
+                CollectedScriptTag {
+                    src: Some("http://[invalid".to_string()),
+                    inline_text: None,
+                },
+            ],
             network_requests: Vec::new(),
             warnings: Vec::new(),
         };
@@ -463,6 +483,22 @@ mod tests {
             .any(|(integration, _)| integration == "didomi"));
     }
 
+    #[test]
+    fn detect_integrations_from_inline_script_avoids_short_substring_matches() {
+        let matches = detect_integrations_from_inline_script("const svgptimize = blockrResult;");
+
+        assert!(
+            !matches.iter().any(|(integration, _)| integration == "gpt"),
+            "should not match incidental GPT substrings"
+        );
+        assert!(
+            !matches
+                .iter()
+                .any(|(integration, _)| integration == "lockr"),
+            "should not match lockr inside a larger token"
+        );
+    }
+
     #[test]
     fn detect_integration_from_url_recognizes_known_patterns() {
         let cases = [
diff --git a/crates/trusted-server-cli/src/audit/browser_collector.rs b/crates/trusted-server-cli/src/audit/browser_collector.rs
@@ -7,7 +7,7 @@ use futures::StreamExt as _;
 use serde::Deserialize;
 use tempfile::TempDir;
 use tokio::runtime::Builder;
-use tokio::time::sleep;
+use tokio::time::{sleep, timeout};
 use url::Url;
 use which::which;
 
@@ -19,6 +19,9 @@ use crate::error::{report_error, CliResult};
 const SETTLE_QUIET_PERIOD: Duration = Duration::from_millis(750);
 const SETTLE_POLL_INTERVAL: Duration = Duration::from_millis(250);
 const SETTLE_MAX_WAIT: Duration = Duration::from_secs(6);
+const NAVIGATION_TIMEOUT: Duration = Duration::from_secs(30);
+const BROWSER_CLOSE_TIMEOUT: Duration = Duration::from_secs(5);
+const RESOURCE_TIMING_BUFFER_WARNING_THRESHOLD: usize = 250;
 
 #[derive(Default)]
 pub(crate) struct BrowserAuditCollector;
@@ -72,10 +75,14 @@ async fn collect_page_via_browser_async(target_url: &Url) -> CliResult<Collected
 
     let result = collect_page_from_browser(&mut browser, target_url).await;
 
-    let close_result = browser
-        .close()
+    let close_result = timeout(BROWSER_CLOSE_TIMEOUT, browser.close())
         .await
-        .map_err(|error| report_error(format!("failed to close browser after audit: {error}")));
+        .map_err(|_| report_error("timed out closing browser after audit"))
+        .and_then(|result| {
+            result.map_err(|error| {
+                report_error(format!("failed to close browser after audit: {error}"))
+            })
+        });
     if close_result.is_err() {
         handler_task.abort();
     }
@@ -95,18 +102,28 @@ async fn collect_page_from_browser(
         report_error(format!("failed to create browser page for audit: {error}"))
     })?;
 
-    page.goto(target_url.as_str())
+    timeout(NAVIGATION_TIMEOUT, page.goto(target_url.as_str()))
         .await
+        .map_err(|_| report_error(format!("timed out navigating to `{target_url}`")))?
         .map_err(|error| report_error(format!("failed to navigate to `{target_url}`: {error}")))?;
 
-    let navigation_response = page.wait_for_navigation_response().await.map_err(|error| {
-        report_error(format!(
-            "failed to read main document navigation response: {error}"
-        ))
-    })?;
-    validate_navigation_response(navigation_response)?;
+    let navigation_response = timeout(NAVIGATION_TIMEOUT, page.wait_for_navigation_response())
+        .await
+        .map_err(|_| {
+            report_error(format!(
+                "timed out waiting for main document navigation response from `{target_url}`"
+            ))
+        })?
+        .map_err(|error| {
+            report_error(format!(
+                "failed to read main document navigation response: {error}"
+            ))
+        })?;
 
     let mut warnings = Vec::new();
+    if let Some(warning) = validate_navigation_response(navigation_response)? {
+        warnings.push(warning);
+    }
     if !wait_for_page_settle(&page).await? {
         warnings.push(
             "browser audit timed out while waiting for the page to settle; results may be partial"
@@ -164,6 +181,13 @@ async fn collect_page_from_browser(
             ))
         })?;
 
+    if network_requests.len() >= RESOURCE_TIMING_BUFFER_WARNING_THRESHOLD {
+        warnings.push(
+            "browser resource timing buffer reached its default size; some network assets may be missing"
+                .to_string(),
+        );
+    }
+
     Ok(CollectedPage {
         requested_url: target_url.to_string(),
         final_url,
@@ -180,9 +204,7 @@ async fn collect_page_from_browser(
             .into_iter()
             .map(|entry| CollectedRequest {
                 url: entry.url,
-                method: "GET".to_string(),
                 resource_type: entry.initiator_type,
-                status: None,
             })
             .collect(),
         warnings,
@@ -230,7 +252,7 @@ async fn wait_for_page_settle(page: &chromiumoxide::Page) -> CliResult<bool> {
     Ok(false)
 }
 
-fn validate_navigation_response(navigation_response: ArcHttpRequest) -> CliResult<()> {
+fn validate_navigation_response(navigation_response: ArcHttpRequest) -> CliResult<Option<String>> {
     let request = navigation_response
         .ok_or_else(|| report_error("browser audit did not capture the main document response"))?;
 
@@ -245,11 +267,11 @@ fn validate_navigation_response(navigation_response: ArcHttpRequest) -> CliResul
     })?;
 
     if is_successful_navigation_status(response.status) {
-        return Ok(());
+        return Ok(None);
     }
 
-    Err(report_error(format!(
-        "audit request returned HTTP {} {} for `{}`",
+    Ok(Some(format!(
+        "audit request returned HTTP {} {} for `{}`; results may be partial",
         response.status, response.status_text, response.url
     )))
 }
diff --git a/crates/trusted-server-cli/src/audit/collector.rs b/crates/trusted-server-cli/src/audit/collector.rs
@@ -27,9 +27,7 @@ pub(crate) struct CollectedScriptTag {
 #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
 pub(crate) struct CollectedRequest {
     pub(crate) url: String,
-    pub(crate) method: String,
     pub(crate) resource_type: Option<String>,
-    pub(crate) status: Option<u16>,
 }
 
 impl CollectedPage {
diff --git a/crates/trusted-server-cli/src/error.rs b/crates/trusted-server-cli/src/error.rs
@@ -5,5 +5,7 @@ pub(crate) fn cli_error<T>(message: impl Into<String>) -> CliResult<T> {
 }
 
 pub(crate) fn report_error(message: impl Into<String>) -> String {
-    message.into()
+    let message = message.into();
+    log::error!("{message}");
+    message
 }
diff --git a/crates/trusted-server-integration-tests/Cargo.lock b/crates/trusted-server-integration-tests/Cargo.lock
diff --git a/crates/trusted-server-integration-tests/Cargo.toml b/crates/trusted-server-integration-tests/Cargo.toml

Original file line number	Diff line number	Diff line change
`@@ -27,9 +27,7 @@ pub(crate) struct CollectedScriptTag {`
`27`	`27`	`#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]`
`28`	`28`	`pub(crate) struct CollectedRequest {`
`29`	`29`	`pub(crate) url: String,`
`30`		`- pub(crate) method: String,`
`31`	`30`	`pub(crate) resource_type: Option<String>,`
`32`		`- pub(crate) status: Option<u16>,`
`33`	`31`	`}`
`34`	`32`
`35`	`33`	`impl CollectedPage {`
Original file line number	Diff line number	Diff line change
`@@ -5,5 +5,7 @@ pub(crate) fn cli_error<T>(message: impl Into<String>) -> CliResult<T> {`
`5`	`5`	`}`
`6`	`6`
`7`	`7`	`pub(crate) fn report_error(message: impl Into<String>) -> String {`
`8`		`- message.into()`
	`8`	`+ let message = message.into();`
	`9`	`+ log::error!("{message}");`
	`10`	`+ message`
`9`	`11`	`}`