Add the Trusted Server CLI#669
Add the Trusted Server CLI#669ChristianPavilonis wants to merge 5 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
Reviewer quick links
Start here if you want to test the new CLI directly:
docs/guide/cli.mddocs/guide/fastly-provisioning.mddocs/guide/fastly.mddocs/guide/getting-started.mdThe most useful review path is:
trusted-server.tomlchanges map to Fastly resources.Manual reviewer test plan
Use a disposable or test Fastly Compute service. Run the commands one by one so you can inspect each CLI step and its output.
1. Set up local CLI access
Install the host-target CLI from the checkout:
Confirm the command surface is available:
If you are not on Apple Silicon macOS, use the explicit host-target form from
docs/guide/cli.mdinstead ofcargo install_cli.2. Authenticate to Fastly
For local review, store a Fastly API token in the host credential store:
Inspect credential status in both human and JSON forms:
Expected result:
effective_sourceissecure-storage, unless you setFASTLY_API_KEY, in which case it should beenvironment.For CI-style review, skip
loginand use:3. Audit the test site
Create a temp directory so generated audit files do not overwrite your checkout files:
Run the browser-backed audit against the test site:
Inspect the generated files:
Expected result: the audit writes
js-assets.tomland a drafttrusted-server.toml. The terminal summary should include the audited URL, page title, JS asset count, third-party asset count, detected integrations, and written paths.4. Validate the generated config
Run validation in human-readable form:
ts config validate --config "$WORKDIR/trusted-server.toml"Run validation in JSON form:
ts config validate --config "$WORKDIR/trusted-server.toml" --jsonExpected result: JSON output has
valid: true, a resolvedpath, a non-emptyconfig_hash, and an emptyerrorsarray.5. Preview Fastly provisioning
Set your test service ID:
Run a JSON provisioning plan:
Inspect these fields in the JSON output:
service_idconfig_pathservice_version.latest_versionservice_version.target_versionservice_version.clone_requiredactionswarningsExpected result:
planreports what would be created, updated, or bound without mutating the Fastly service.6. Apply provisioning to the test service
Apply with JSON output and skip the confirmation prompt:
Inspect these fields in the JSON output:
completed_actionswarningsfailed_actionactivated_versionservice_version.target_versionExpected result:
completed_actionslists the changes made. If resource bindings changed,activated_versionshould betrue.If request signing is enabled and
api-keys/api_keyis missing, provide exactly one runtime API token source before applying:For a disposable test service only, you can instead add:
7. Re-run plan to check idempotency
Run the plan again:
Expected result: after a successful apply,
actionsshould usually be empty unless a runtime token was intentionally not applied or the service changed externally.8. Useful docs while reviewing output
ts provision fastly plants provision fastly applySummary
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.lock.cargo/config.tomlcrates/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/cli.mddocs/guide/fastly-provisioning.mddocs/guide/getting-started.mdtsconfig/dev/audit workflow examples and browser audit prerequisites.docs/guide/configuration.mdts config init/ts config validateguidance and linked provisioning behavior.docs/guide/fastly.mdts auth fastly ...,ts provision fastly ..., and provisioning-map links.Closes
Closes #
Automated test plan
cargo test --workspace --exclude trusted-server-clicargo test --package trusted-server-cli --target "$(rustc -vV | sed -n 's/^host: //p')"cargo test_clicargo 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 docs && npm run formatcd docs && npm run buildcd crates/js/lib && npx vitest runcd crates/js/lib && 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!