feat(cli): implement ts dev audit headers cache-header audit (#834)

[vasujain00]

Summary

Implements the origin cache-header audit designed in the #834 spec (PR #930). ts dev audit headers fetches origin responses — explicit URLs or discovered from the origin root HTML — classifies each by content type, evaluates cache directives, and reports a per-type pass/warn/fail verdict.

Changes

Area	Change
commands/dev/audit/mod.rs	New DevAuditCommand group + exit-code dispatch
commands/dev/audit/headers/rules.rs	ContentTypeGroup taxonomy, unit Verdict enum, per-group cacheability rules
commands/dev/audit/headers/fetch.rs	AuditHeadersArgs, OriginClient trait + reqwest blocking impl, HTML-parse discovery
commands/dev/audit/headers/analyze.rs	Worst-of rollup per group, summary, exit-code mapping
commands/dev/audit/headers/output.rs	Writer-injected human table + JSON
commands/dev/mod.rs	Adds cross-platform Audit variant to existing DevCommand
Cargo.toml	reqwest (blocking) scoped to cfg(not(target_arch = "wasm32"))

Design fidelity

• no-store alone passes HTML/RTB (RFC 9111 §5.2.2.5)
• s-maxage honored as CDN TTL; Vary (User-Agent/Cookie) checked on all cacheable groups
• Surrogate-Key only checked on cacheable groups; MIME params stripped before classification
• Discovery HTML-parses the origin root — never probes edge-only /_ts/ routes
• Audit is available on all host platforms (unlike the macOS-only proxy)

Closes

Refs #834

Test plan

• cargo test --package trusted-server-cli --target <host> — 105 pass (25 new)
• cargo clippy --package trusted-server-cli --all-targets -- -D warnings — clean
• cargo fmt --all -- --check — clean
• ts dev audit headers --help renders

Checklist

• Follows AGENTS.md conventions (CliResult string errors, writer injection)
• No unwrap() in production code
• Uses log macros (not println!) outside the output sink
• New code has tests
• No secrets committed

[aram356]

Summary

Well-structured module: the pure rules.rs / analyze.rs split, the injected OriginClient and Write sink, and 25 colocated tests make the logic genuinely testable without network I/O. The blockers below are about the new dependency's effect on the rest of the CLI, and about cases where the audit reports a clean bill of health on postures it exists to catch.

Every finding below was verified by running code against this head, not by inspection alone.

2 of the inline comments carry a one-click GitHub suggestion — use Commit suggestion (or Add suggestion to batch) to apply them. The remaining comments describe the fix in prose because the change spans multiple files or lines outside the diff.

Blocking

🔧 wrench

• Adding reqwest breaks ts dev proxy at runtime and fails CI — see inline at crates/trusted-server-cli/Cargo.toml:28
• Vary: * silently passes on JS, CSS, and images — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:525
• Exit code 2 collides with the CLI's own error code — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/analyze.rs:61
• Discovery fetches arbitrary cross-origin URLs — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:210
• Redirects mask the audited URL's real cache headers — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/fetch.rs:78

Non-blocking

🤔 thinking / ♻️ refactor / ⛏ nitpick / 📝 note

• s-maxage is not honored as a CDN TTL, contrary to the PR description — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:393
• Surrogate-Control: no-store on cacheable groups is never flagged — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:397
• max-age = 3600 with spaces loses its value — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:206
• Redundant double-Option match — see inline at crates/trusted-server-cli/src/commands/dev/audit/headers/rules.rs:356
• ts dev audit sits alongside an unrelated top-level ts audit — see inline at crates/trusted-server-cli/src/commands/dev/audit/mod.rs:16

Cross-cutting / body-level findings

• 🔧 CI gate cargo test (ts CLI, native) is failing — 196 passed, 4 failed. All four are dev::proxy TLS tests panicking in rustls with "Could not automatically determine the process-level CryptoProvider". This is not flaky and not pre-existing: I built both sides and ran them locally. Base 066ea3c6 passes 175/175; this head fails 4. Root cause and fix are in the Cargo.toml:28 comment. The check is not in branch protection's required list, but AGENTS.md treats the adapter test aliases as PR gates.

• 📝 No documentation for the new command. docs/ has no reference to ts dev audit headers. Given the exit-code contract is meant for CI gating, the codes and their meanings are worth writing down wherever the other ts commands are documented.

CI Status

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

[aram356]

🔧 wrench — This dependency breaks ts dev proxy at runtime and fails the cargo test (ts CLI, native) gate.

The workspace reqwest is declared with features = ["json", "rustls-tls"], and rustls-tls turns on reqwest's __rustls-ring, which enables rustls's ring backend. The macOS proxy stack already pulls in aws-lc-rs. With both providers compiled in, rustls cannot infer a process default and every ClientConfig::builder() / ServerConfig::builder() call panics.

Verified against this head rather than inferred:

# cargo tree -p trusted-server-cli -e features, aarch64-apple-darwin
base 066ea3c6 -> rustls feature "aws-lc-rs"          (only)
this head     -> rustls feature "aws-lc-rs" AND "ring"

# and `ring` traces to exactly one place:
rustls feature "ring"
  |-- hyper-rustls feature "ring"  <- reqwest feature "__rustls-ring"
  |-- tokio-rustls feature "ring"  <- reqwest feature "__rustls-ring"
  `-- reqwest feature "__rustls-ring"

This is not test-only. Running the built binary:

# base 066ea3c6
$ ts dev proxy --from a.example.com --to b.example.com --listen 127.0.0.1:18098
ts dev proxy listening on 127.0.0.1:18098

# this head
$ ts dev proxy --from a.example.com --to b.example.com --listen 127.0.0.1:18097
thread 'main' panicked at rustls-0.23.41/src/crypto/mod.rs:249:14:
Could not automatically determine the process-level CryptoProvider from Rustls crate features.

So the PR as it stands ships a ts dev proxy that panics on startup. Local cargo test -p trusted-server-cli --lib matches CI exactly: base 175/175 pass, this head 196 passed / 4 failed.

Proposed fix (apply manually — spans proxy/mod.rs plus the two test modules, so it cannot be one suggestion):

Pin the provider explicitly. I verified this resolves the runtime panic — the proxy starts and logs listening again:

// crates/trusted-server-cli/src/commands/dev/proxy/mod.rs

/// Pins the process-level rustls [`CryptoProvider`] to `aws-lc-rs`.
///
/// Crates in the dependency graph enable both the `ring` and `aws-lc-rs`
/// rustls backends, so rustls cannot infer a process default and the
/// `ClientConfig` / `ServerConfig` builders panic. Installing explicitly is
/// idempotent: a second call returns `Err`, which is ignored.
fn install_crypto_provider() {
    let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();
}

pub fn run(args: &ProxyArgs) -> core::result::Result<(), error_stack::Report<ProxyError>> {
    install_crypto_provider();
    // ... existing body

The four failing tests construct TLS configs directly without going through run(), so they each need the same call in their setup (or a shared #[cfg(test)] helper) to go green.

Worth noting for whoever applies it: a crate-local reqwest = { default-features = false, ... } override does not work here — I tried it, and Cargo's workspace feature unification still resolves ring through the edgezero-* crates that depend on the workspace reqwest. The explicit install_default() is the fix that actually holds.

[aram356]

🔧 wrench — Vary: * is reported as a Pass on JS bundles, CSS/fonts, and images.

The wildcard branch is gated on flag_wildcard, which is true only at the HTML call site (line 289); JS/static (line 344) and images (line 417) pass false. vary_has_high_cardinality matches only the exact tokens user-agent and cookie, so it does not catch * either, and control flow falls through to the Pass at line 545.

Vary: * makes a response unusable from any cache. A hashed bundle served with public, max-age=31536000, immutable and Vary: * is never cached by anyone — precisely the misconfiguration this audit exists to surface — yet it currently rolls up clean. The emitted Pass message is also untrue: it reads no *, User-Agent, or Cookie while the observed value is *.

Verified by running evaluate() on this head:

JavaScript  + Vary: *  -> Vary: Pass, rollup Pass
Image       + Vary: *  -> Vary: Pass, rollup Pass
StaticAsset + Vary: *  -> Vary: Pass, rollup Pass

The existing vary_wildcard_warns_only_for_html test asserts only the HTML half, so it does not pin this behaviour — the name just describes the bug.

The suggestion below checks the wildcard on every group. After applying it I re-ran the same probe:

JavaScript  + Vary: *               -> rollup Warn
Image       + Vary: *               -> rollup Warn
StaticAsset + Vary: *               -> rollup Warn
JavaScript  + Vary: Accept-Encoding -> rollup Pass   (no false positive)

cargo fmt --check clean, clippy -D warnings clean, and all 60 audit:: tests still pass with it applied. It leaves _flag_wildcard in place so the two call sites stay untouched; consider dropping the parameter entirely in a follow-up.

Suggested change

	/// `Vary` checks: `*` disables all caching (HTML only), and `User-Agent` /
	/// `Cookie` destroy the CDN hit ratio on any group.
	fn evaluate_vary(headers: &ResponseHeaders, flag_wildcard: bool) -> Option<HeaderVerdict> {
	let value = headers.vary.as_deref()?;
	if flag_wildcard && value.split(',').any(|token| token.trim() == "*") {
	/// `Vary` checks: `*` makes a response uncacheable by every cache, and
	/// `User-Agent` / `Cookie` destroy the CDN hit ratio on any group.
	fn evaluate_vary(headers: &ResponseHeaders, _flag_wildcard: bool) -> Option<HeaderVerdict> {
	let value = headers.vary.as_deref()?;
	if value.split(',').any(|token| token.trim() == "*") {

(compile-verified and behaviour-checked with the probe above — please re-run ./scripts/test-cli.sh after applying.)

[aram356]

🔧 wrench — Exit code 2 already means "the CLI failed", so a warn-only audit is indistinguishable from an audit that never ran.

main.rs:8 exits 2 for any error returned out of run_from_env(). This function also returns 2 for "warnings only", and audit/mod.rs:36 passes it straight to process::exit.

Verified against the built binary on this head:

warn-only audit (JS max-age=600)   -> exit 2
unreachable origin                 -> exit 2   [ts] failed to fetch ...
invalid --origin value             -> exit 2   [ts] invalid origin `not-a-url`

A CI job that treats 2 as "warnings, proceed" will silently pass when the audit could not reach the origin at all — which is the failure mode most worth catching, since it is indistinguishable from success in the logs.

Proposed fix (apply manually — the exit-code contract is documented in four places: this function, its test at line 280, the run doc comment at headers/mod.rs:23, the module doc at headers/mod.rs:5, and the comment at audit/mod.rs:32):

Move the warn code off 2 — 3 is free:

    /// The process exit code the CLI should return: 1 if any group failed,
    /// 3 if any warned (and none failed), 0 otherwise.
    ///
    /// 2 is deliberately skipped: `main` already exits 2 for any CLI error, so
    /// reusing it for warnings would make a warn-only audit indistinguishable
    /// from an audit that never ran.
    pub(crate) fn exit_code(&self) -> i32 {
        if self.summary.fail > 0 {
            1
        } else if self.summary.warn > 0 {
            3
        } else {
            0
        }
    }

I applied this plus the matching test update locally: cargo fmt --check clean and all 60 audit:: tests pass.

[aram356]

🔧 wrench — Discovery follows asset URLs to any host, including link-local addresses.

base.join(raw) resolves absolute URLs to whatever host they name, and every resolved URL is then fetched. Nothing constrains the result to the origin being audited. The module doc says discovery "never probes edge-only /_ts/ routes", which holds, but the far broader case of a third-party or internal host is unconstrained.

Verified against the built binary: I served a page linking one external asset and one metadata-service URL, then ran the audit against it:

urls_sampled: [ "http://127.0.0.1:PORT/",
                "http://example.com/evil.js",          <- fetched, cross-origin
                "http://127.0.0.1:PORT/favicon.ico" ]

stderr: skipping http://169.254.169.254/latest/meta-data/: failed to fetch ...

The 169.254.169.254 request was attempted and only failed because nothing was listening. Auditing a page whose markup you do not fully control therefore turns the operator's machine (or a CI runner, where that address usually does answer) into the requester. The --json output also attributes those third-party responses to the audited origin's report.

Two things compound it in the same fetch path:

• No redirect policy is set on the client, so reqwest follows up to 10 redirects by default — an in-origin URL can still land off-origin.
• response.text() at fetch.rs:86 reads the body with no size cap, on every fetch, though only the root's body is ever used for discovery.

Proposed fix (apply manually — spans discover_asset_urls, the client builder, and fetch):

Restrict discovered URLs to the origin being audited, and bound the transport:

// in discover_asset_urls, inside push_resolved:
if let Ok(resolved) = base.join(raw)
    && resolved.origin() == base.origin()
    && !urls.contains(&resolved)
{
    urls.push(resolved);
}

// in ReqwestOriginClient::new:
let client = reqwest::blocking::Client::builder()
    .timeout(REQUEST_TIMEOUT)
    .redirect(reqwest::redirect::Policy::none())
    .build()

Explicit --url arguments are a deliberate operator action and can reasonably stay unrestricted; it is the automatic crawl that should not wander. If cross-origin assets are intentionally in scope (auditing a CDN's posture is a plausible goal), an opt-in flag such as --include-cross-origin would make that a choice rather than the default.

[aram356]

🔧 wrench — Redirects are followed silently, so the audited URL's own cache headers are never seen.

No redirect policy is set on the client, so reqwest applies its default of following up to 10 redirects. extract_headers then reads the headers of the final response, while the report still lists the original URL.

This matters because a redirect carries its own cache posture, and a long-lived cached redirect is a real and commonly-missed misconfiguration — exactly the class of problem this tool is meant to find.

Verified against the built binary. The root returns 301 with a deliberately bad posture, pointing at a target with a good one:

GET /       -> 301, Cache-Control: public, max-age=999999   (the problem)
GET /final  -> 200, Cache-Control: no-store                 (fine)

$ ts dev audit headers --origin http://127.0.0.1:PORT/
Origin: http://127.0.0.1:PORT/

(check) HTML
    (check) Cache-Control      --

Summary (per content type): 1 pass, 0 warn, 0 fail (1 types audited)
exit 0

The public, max-age=999999 on the redirect is invisible, and the tool reports the origin as clean while attributing /final's headers to /.

Proposed fix (apply manually — touches the client builder and the response handling together):

Stop following redirects and audit each hop's own headers:

let client = reqwest::blocking::Client::builder()
    .timeout(REQUEST_TIMEOUT)
    .redirect(reqwest::redirect::Policy::none())
    .build()

With that set, a 3xx is returned as-is and its headers get classified on their own merits. If you would rather keep following, record the final URL on FetchedResponse and render it next to the requested one, so the report cannot silently attribute one URL's posture to another — but auditing the redirect itself is the more useful behaviour here.

[aram356]

🤔 thinking — The PR description says "s-maxage honored as CDN TTL", but Cache-Control: s-maxage is never read.

s_maxage is parsed (line 191) and used at exactly one place: line 393, inside if let Some(surrogate) = headers.surrogate_control.as_deref(). So the value is only ever consulted when a Surrogate-Control header is present, and it is read from that header rather than from Cache-Control.

Two consequences:

1. An origin using the standard, non-Fastly-specific Cache-Control: s-maxage=... to set its shared-cache TTL gets no credit for it. Verified: an image with Cache-Control: public, s-maxage=604800, max-age=60 and no Surrogate-Control yields only Cache-Control: Warn ("Images re-fetched too frequently") and Surrogate-Key: Pass — no Surrogate-Control verdict at all, and the CDN/browser comparison never runs. A correctly configured week-long edge TTL is reported as under-cached.

2. .or(surrogate_directives.s_maxage) is a fallback within Surrogate-Control, but s-maxage is a Cache-Control directive; Fastly's Surrogate-Control uses max-age. So that fallback is effectively dead, while the level it was presumably meant to implement is missing.

The doc comment at lines 186-187 describes the intended three-level resolution ("Surrogate-Control first, then s-maxage, then max-age"), which reads like the right model — the s-maxage level just needs to come from Cache-Control:

let cdn_ttl = surrogate_directives
    .max_age
    .or_else(|| cache_control.as_ref().and_then(|d| d.s_maxage))
    .or(browser_ttl);

Either implement that, or adjust the PR description so the claim matches the code.

[aram356]

🤔 thinking — A Surrogate-Control that disables edge caching is never flagged on cacheable groups.

This comparison requires (Some(cdn), Some(browser)). When Surrogate-Control carries no numeric TTL — no-store, private, no-cache — cdn_ttl is None, the if let is skipped, and no verdict is emitted at all.

Verified on this head: an image with Cache-Control: public, max-age=86400 and Surrogate-Control: no-store produces Cache-Control: Pass, Surrogate-Key: Pass, rollup Pass. An origin that has switched off edge caching for its creatives gets a green audit from a tool whose purpose is to notice that.

evaluate_surrogate_no_store already encodes the inverse check, but it is wired only into HTML (line 290) and RTB (line 451). The cacheable groups want the mirror image of it — a Surrogate-Control that forbids caching should be a Warn (or Fail) for JS, images, and static assets.

Note Surrogate-Control: max-age=0 is caught correctly today, because it has a numeric value — so the gap is specifically the token-only directives.

[aram356]

⛏ nitpick — max-age = 3600 (spaces around =) silently loses its value.

split_once('=') on "max-age = 31536000" gives name "max-age " and value " 31536000". The name is trimmed at lines 207 and 212, so the token is recorded correctly, but the value is only trim_matches('"')-ed — never trim()-ed — so " 31536000".parse::<u64>() fails and max_age becomes None.

Whitespace around = is legal in HTTP header parameters and tolerated by most parsers, so this produces a false positive. Verified on this head: JS with Cache-Control: public, max-age = 31536000, immutable yields Cache-Control: Warn instead of Pass.

The suggestion below trims both sides of the quote-stripping, so a quoted value with padding (max-age = "3600") also parses. After applying it I confirmed both forms now Pass:

public, max-age = 31536000, immutable   -> Cache-Control: Pass
public, max-age="31536000", immutable   -> Cache-Control: Pass

cargo fmt --check clean, clippy -D warnings clean, 60 audit:: tests pass with it applied.

Suggested change

	let seconds = seconds.trim_matches('"').parse::<u64>().ok();
	let seconds = seconds.trim().trim_matches('"').trim().parse::<u64>().ok();

[aram356]

♻️ refactor — This double-Option match is redundant and reads as though more cases are possible than are.

cache_control on the line above is derived from headers.cache_control via .map(), so the two Options are structurally identical by construction: (Some, None) and (None, Some) cannot occur, and the _ arm only ever fires on (None, None).

Behaviour is correct — this is purely about reader load. A single match on the parsed value, with value taken from the same source, says the same thing:

match headers.cache_control.as_deref() {
    Some(value) => {
        let directives = CacheDirectives::parse(value);
        // ... existing Some arm, using `value`
    }
    None => verdicts.push(HeaderVerdict::flagged(/* ... */)),
}

That also matches the shape evaluate_html and evaluate_rtb already use, so all three read alike. The only wrinkle is that the parsed directives are needed again at line 394 for the CDN/browser comparison — binding them before the match keeps that working.

[aram356]

📝 note — Two unrelated commands now both read as "audit".

ts audit already exists at the top level (run.rs:27) and does something quite different: it drives a headless browser over a public page and writes draft Trusted Server artifacts. This adds ts dev audit headers, which fetches origin responses and grades cache posture.

Nothing is wrong here, and the dev grouping does separate them. But "audit" is now doing double duty for two unrelated operations, and the two live one word apart — ts audit vs ts dev audit. Worth a moment's thought about whether a name like ts dev check-headers or ts dev cache-headers would save operators from reaching for the wrong one, particularly since this command is intended to be wired into CI where the invocation is written once and rarely re-read.

Not blocking, and entirely reasonable to keep as-is if the dev prefix feels like enough separation.