Add the Trusted Server CLI#669
Add the Trusted Server CLI#669ChristianPavilonis wants to merge 2 commits intofeature/ts-config-in-config-storefrom
Conversation
ChristianPavilonis
changed the base branch from
main
to
feature/ts-config-in-config-store
ChristianPavilonis
changed the title
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
Summary
New trusted-server-cli crate adds ts config|audit|dev|auth fastly|provision fastly commands and switches ts audit to a Chromium-driven collector. The shape is solid — clean module split, trait-seamed credential store, comprehensive MockFastlyApi for the provision flow — but several issues block merge: clippy fails on the host target with -D warnings, the new HTTP clients have no timeouts, ts audit accepts arbitrary URL schemes (local-file disclosure via headless Chromium), and the repo's CI workflows don't actually run for this PR.
Blocking
🔧 wrench
- Clippy fails on host target:
clippy::needless_pass_by_valueinbuild_audit_outputs(crates/trusted-server-cli/src/audit.rs:70). Inline comment. - No request/connect timeouts on Fastly API client (
crates/trusted-server-cli/src/fastly/api.rs:107-113). Inline comment. - No request/connect timeouts on audit HTTP collector (
crates/trusted-server-cli/src/audit/http_collector.rs:11-15). Inline comment. ts auditaccepts arbitrary URL schemes —Url::parseletsfile:///data:through, andvalidate_navigation_response(crates/trusted-server-cli/src/audit/browser_collector.rs:213-216) returnsOk(())for any non-http(s) scheme.ts audit file:///etc/passwdcan navigate headless Chromium to local files and serialize the rendered HTML into the on-disk artifact. Restrict scheme at parse time. Inline comment oncrates/trusted-server-cli/src/lib.rs:218.- No Rust CI runs for this PR:
.github/workflows/format.yml:6-7and.github/workflows/test.yml:9-10trigger only onpull_request: branches: [main]. PR #669 targetsfeature/ts-config-in-config-store, so neithercargo test,cargo clippy, nor the new host-CLI lanes have run on this commit. The push runs are recorded as failed with "log not found" — they never executed. Either retarget ontomain, expand thebranches:filter to include the stack base branch, or land the stack first. As direct evidence: the clippy break above was not caught because no clippy job ever ran here.
❓ question
--reuse-management-api-keyprivilege escalation (crates/trusted-server-cli/src/lib.rs:336-375). The flag plants a credential with full Fastly management privileges into the runtime secret store; if the worker ever leaks it, an attacker can mutate Fastly resources, not just sign requests. Is this a smoke-test convenience or a supported production path? Should--helpexplicitly call out the risk, and should it require confirmation even with--yes? Inline comment.
Non-blocking
🤔 thinking
- Dead
http_collectormodule (crates/trusted-server-cli/src/audit/http_collector.rs:9-79) — entire file is#[allow(dead_code)]with no consumer. Either wire it as a fallback when no browser is found, or delete it. Inline comment. analyze_htmlonly used by tests (crates/trusted-server-cli/src/audit.rs:60andcrates/trusted-server-cli/src/audit/analyzer.rs:117) — gated#[cfg_attr(not(test), allow(dead_code))]. Move both under#[cfg(test)]or rewrite the tests aroundanalyze_collected_page. Inline comment.- No
log::macros anywhere in the CLI — CLAUDE.md mandateslogmacros for instrumentation; the CLI only writes to stdout/stderr. Browser audit steps and Fastly API requests would benefit fromlog::debug!/log::trace!behindRUST_LOG. Today there is no way to introspect whatts provision fastly applyis doing beyond final stdout/stderr.
♻️ refactor
- Race condition in env-var tests (
crates/trusted-server-cli/src/fastly/auth.rs:166-196) — parallel mutation ofFASTLY_API_KEYacross threads. Useserial_test::serialor a per-suiteMutex. Inline comment. - Repeated regex compilation (
crates/trusted-server-cli/src/audit/analyzer.rs:201, 224) —GTM-[A-Z0-9]+$is rebuilt on every inline-script and recompiled in two functions. Hoist to aLazyLock<Regex>. Inline comment. - Trim API key before persisting (
crates/trusted-server-cli/src/fastly/auth.rs:122-133) — pasted tokens commonly arrive with trailing whitespace/newlines. Inline comment. format!allocations inclassify_party(crates/trusted-server-cli/src/audit/analyzer.rs:158-170) — two allocations per call, run for every asset URL; an allocation-free version usingstrip_suffixis straightforward. Inline comment.
🌱 seedling
- Pin
chromiumoxidevia workspace deps (crates/trusted-server-cli/Cargo.toml:27) — only direct version pin in this crate; everything else isworkspace = true. Inline comment. ts auditlacks--json— every other validate/status/plan/apply command supports--json;auditdoesn't. Consider adding for tooling parity.
⛏ nitpick
config validate --jsonreturns unstructured error blob (crates/trusted-server-cli/src/config.rs:97-99) — multi-lineReportdebug dump shoved into a single JSON string. Inline comment.navigator.webdriverspoofing (crates/trusted-server-cli/src/audit/browser_collector.rs:62-71) — bot-evasion technique that's likely unnecessary for auditing sites the operator owns. Either drop or document the specific failure mode it mitigates. Inline comment.- Reused
cache-shared-keyacross wasm and host lanes (.github/workflows/format.yml:30,55,.github/workflows/test.yml:29,65) — distinct keys per target are safer under concurrent runs. Inline comment. - Public items without doc comments —
lib.rs:25,156,171,audit.rs:42,52,config.rs:13,19,dev.rs:13,error.rs:4,output.rs:8,fastly/auth.rs:57-75,fastly/api.rs:12-99. CLAUDE.md says every public item must have a doc comment. Most are effectivelypub(crate)since the binary lives in the same crate — easiest fix is to downgrade visibility.
CI status
- fmt: PASS (verified locally — workflow did not run on this commit)
- clippy (workspace, wasm): not run
- clippy (CLI, host): FAIL — see
audit.rs:70finding - rust tests (workspace): not run
- rust tests (CLI, host): PASS locally (18/18) — workflow did not run on this commit
- js tests: not run for this commit
- analyze (javascript-typescript): PASS
| } | ||
|
|
||
| fn build_audit_outputs( | ||
| collected: collector::CollectedPage, |
There was a problem hiding this comment.
🔧 wrench — Clippy fails on host with -D warnings: clippy::needless_pass_by_value. The body only borrows collected (analyze_collected_page(&collected), collected.final_url()), so this should take a reference.
Fix:
fn build_audit_outputs(
collected: &collector::CollectedPage,
) -> Result<AuditOutputs, Report<CliError>> {And update the call site at audit.rs:67 to build_audit_outputs(&collected).
Reproduced locally:
cargo clippy -p trusted-server-cli --target $(rustc -vV | sed -n 's/^host: //p') --all-targets -- -D warnings
error: this argument is passed by value, but not consumed in the function body
--> crates/trusted-server-cli/src/audit.rs:70:16
| .build() | ||
| .change_context(CliError::FastlyApi)?; | ||
| Ok(Self { client, api_key }) | ||
| } |
There was a problem hiding this comment.
🔧 wrench — No request/connect timeouts on the Fastly API client. reqwest::blocking::Client has no default timeout, so a stalled Fastly endpoint will wedge ts provision fastly plan/apply indefinitely with no recovery path other than SIGINT.
Fix:
let client = Client::builder()
.user_agent("trusted-server-cli/0.1")
.connect_timeout(std::time::Duration::from_secs(10))
.timeout(std::time::Duration::from_secs(60))
.build()
.change_context(CliError::FastlyApi)?;| .user_agent("trusted-server-cli/0.1") | ||
| .redirect(reqwest::redirect::Policy::limited(10)) | ||
| .build() | ||
| .change_context(CliError::Audit)?; |
There was a problem hiding this comment.
🔧 wrench — Same as the Fastly client: no .timeout(...) / .connect_timeout(...). A slow target site will hang the audit. Add explicit timeouts.
| let url = url::Url::parse(&args.url).map_err(|error| { | ||
| Report::new(CliError::Arguments) | ||
| .attach(format!("invalid audit URL `{}`: {error}", args.url)) | ||
| })?; |
There was a problem hiding this comment.
🔧 wrench — Url::parse accepts arbitrary schemes (file://, data:, chrome://, …). Combined with validate_navigation_response (browser_collector.rs:213-216) returning Ok(()) for any non-http(s) scheme, this lets ts audit file:///etc/passwd navigate Chromium to a local file and then serialize the rendered HTML into the audit artifact written to disk. Headless Chromium permits file:// unless explicitly disabled.
Fix — restrict scheme at parse time:
let url = url::Url::parse(&args.url).map_err(|error| {
Report::new(CliError::Arguments)
.attach(format!("invalid audit URL `{}`: {error}", args.url))
})?;
if !matches!(url.scheme(), "http" | "https") {
return Err(Report::new(CliError::Arguments).attach(format!(
"`ts audit` only supports http/https URLs, got `{}`",
url.scheme()
)));
}| } | ||
| if reuse_management_api_key { | ||
| return Ok(Some(management_api_key.to_string())); | ||
| } |
There was a problem hiding this comment.
❓ question — Reusing the management API key as the runtime API key means edge code stores a credential with full Fastly management privileges. If the runtime token leaks (logs, error paths, future bug in the worker), an attacker can mutate Fastly resources, not just sign requests.
Questions:
- Is this intended only as a smoke-test convenience, or as a production-supported flow?
- If smoke-test only, can
--helpfor--reuse-management-api-keyflag the privilege-escalation risk explicitly? - Should this require a confirmation prompt unless
--yesis also set, so that operators can't accidentally promote their management token to the edge?
| if asset_host == page_host | ||
| || asset_host.ends_with(&format!(".{page_host}")) | ||
| || page_host.ends_with(&format!(".{asset_host}")) | ||
| { |
There was a problem hiding this comment.
♻️ refactor — Two format!(".{page_host}") allocations per call, and this runs once per asset URL. Allocation-free version:
fn host_matches(page_host: &str, asset_host: &str) -> bool {
asset_host == page_host
|| asset_host
.strip_suffix(page_host)
.is_some_and(|prefix| prefix.ends_with('.'))
|| page_host
.strip_suffix(asset_host)
.is_some_and(|prefix| prefix.ends_with('.'))
}| futures = { workspace = true } | ||
| tokio = { workspace = true, features = ["rt-multi-thread", "time"] } | ||
| which = { workspace = true } | ||
| chromiumoxide = "0.9.1" |
There was a problem hiding this comment.
🌱 seedling — Every other dependency in this crate uses workspace = true; only chromiumoxide = "0.9.1" is pinned at the crate level. Move to [workspace.dependencies] in the root Cargo.toml for consistency and centralized version bumps.
| path: resolved_path, | ||
| config_hash: None, | ||
| errors: vec![format!("{error:?}")], | ||
| } |
There was a problem hiding this comment.
⛏ nitpick — vec![format!("{error:?}")] puts a multi-line error_stack::Report debug dump into a single JSON string. Anything parsing the --json output gets an unstructured blob. Consider one entry per attached context layer or a structured {stage, message} shape so downstream tooling can pattern-match.
| get: () => false, | ||
| }); | ||
| "#, | ||
| ) |
There was a problem hiding this comment.
⛏ nitpick — Overriding navigator.webdriver is bot-evasion. For an audit tool intended to run against sites the operator owns, this is unnecessary, and most sites don't gate solely on this flag. Either drop the override or add a // Why: comment explaining the specific failure mode it mitigates so future maintainers don't strip it as a security smell.
| toolchain: ${{ steps.rust-version-cli.outputs.rust-version }} | ||
| target: wasm32-wasip1 | ||
| components: "clippy, rustfmt" | ||
| cache-shared-key: cargo-${{ runner.os }} |
There was a problem hiding this comment.
⛏ nitpick — Same cache-shared-key: cargo-${{ runner.os }} is reused across the wasm-target workspace lane and the host-target CLI lane (here, line 55, and test.yml lines 29 and 65). Concurrent jobs may contend on cache writes. Distinct keys per target (cargo-${{ runner.os }}-wasm vs -host) are safer.
2 participants
Summary
tshost CLI as a dedicated Rust crate for Trusted Server config, dev, auth, audit, and Fastly provisioning workflows.ts auditfrom a static HTML fetcher to a Chromium-backed browser audit while preserving the existing audit artifact and draft-config outputs.Changes
Cargo.tomltrusted-server-cliworkspace crate and host-side CLI/browser dependencies.Cargo.lockcrates/trusted-server-cli/Cargo.tomltsbinary crate and its dependencies.crates/trusted-server-cli/src/lib.rscrates/trusted-server-cli/src/main.rstsbinary entrypoint.crates/trusted-server-cli/src/error.rscrates/trusted-server-cli/src/output.rscrates/trusted-server-cli/src/config.rstrusted-server-coreruntime config loading.crates/trusted-server-cli/src/dev.rsts devand Rust-native local Fastly manifest rendering/launch behavior.crates/trusted-server-cli/src/fastly/auth.rsFASTLY_API_KEY.crates/trusted-server-cli/src/fastly/api.rscrates/trusted-server-cli/src/fastly/provision.rscrates/trusted-server-cli/src/fastly/mod.rscrates/trusted-server-cli/src/audit.rscrates/trusted-server-cli/src/audit/collector.rscrates/trusted-server-cli/src/audit/http_collector.rscrates/trusted-server-cli/src/audit/browser_collector.rscrates/trusted-server-cli/src/audit/analyzer.rsAuditArtifactschema..github/workflows/test.yml.github/workflows/format.yml.github/pull_request_template.mdCLAUDE.mdREADME.mdtsCLI usage examples and documented browser prerequisites forts audit.docs/guide/getting-started.mdtsconfig/dev/audit workflow examples and browser audit prerequisites.docs/guide/configuration.mdts config init/ts config validateguidance.docs/guide/fastly.mdts auth fastly ...andts provision fastly ...workflow documentation.Closes
Closes #
Test plan
cargo test --workspace --exclude trusted-server-clicargo test --package trusted-server-cli --target "$(rustc -vV | sed -n 's/^host: //p')"cargo clippy --workspace --exclude trusted-server-cli --all-targets --all-features -- -D warningscargo clippy --package trusted-server-cli --target "$(rustc -vV | sed -n 's/^host: //p')" --all-targets -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo run -p trusted-server-cli --target "$(rustc -vV | sed -n 's/^host: //p')" -- config validate --json --config trusted-server.example.tomlts audit https://example.com ...verification of the missing-Chrome/Chromium error pathChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)