Repository navigation
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Adds ts keys list and ts keys verify, shared service-URL validation, local-fixture tests, and key-rotation guidance. No concrete correctness defects found across the nine changed files; the documented feature-spec requirement remains unmet.
Cross-cutting / body-level findings
-
🔧 Missing required feature spec — The new CLI commands at
crates/trusted-server-cli/src/commands/keys/mod.rs:19–25meetCONTRIBUTING.md's definition of a feature. It requires a linked design spec with at least two recorded review rounds. The PR instead cites issue #1431 as its spec; neither that issue nor the changed files supplies the required design/review log.Proposed resolution: add and link a reviewed spec covering:
# Signing-Key Inspection CLI ## Command and output contracts ## Adapter impact ## Configuration, consent, and privacy ## Failure handling ## Compatibility and rollout ## Testing ## Review log
Apply manually — this requires a new document, actual review rounds, and a PR-description update, not a contiguous suggestion.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (axum native): PASS
- CodeQL: PASS
- Analyze (actions): PASS
- CI Node pin guard: PASS
- cargo test: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (rust): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- worker-deadlines (spin): PASS
- cargo test (cross-adapter parity): PASS
- ts dev lint domains (changed lines): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- format-docs: PASS (required)
- HTML EOF runtime (Fastly): PASS
- vitest: PASS
- Analyze (javascript-typescript): PASS
- docs-proposal scripts: PASS
- worker-deadlines (cloudflare): PASS
- format-typescript: PASS (required)
- prepare integration artifacts: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
Local git diff --check: PASS. Reviewer worktree clean at 8df9a40010c4279b6e067306994029dfe7db5847. Builds and tests were not rerun locally because remote CI passed; no suggestions required scratch verification.
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds ts keys list and ts keys verify on a shared url_guard::service_endpoint. Three issues block merge:
- Signatures that start with
-are rejected by clap. This is about one in 64 signatures, since they are base64url. - The 404 hint misdirects, while the status a broken signing store actually returns (500) gets no hint.
- Three behaviours the code promises have no test.
Both commands were exercised against the real Axum adapter seeded with Ed25519 keys. Every suggestion below passes cargo fmt, cargo clippy-cli and the full trusted-server-cli suite, both alone and applied together.
6 of the 8 inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The remaining comments describe the fix in prose because the change touches multiple files or lines outside the diff and can't be auto-applied.
Blocking
🔧 wrench
--signatureand--payloadreject values that start with-— see inline atcrates/trusted-server-cli/src/commands/keys/mod.rs:55- The 404 hint points the wrong way; a 500 gets no hint — see inline at
crates/trusted-server-cli/src/commands/keys/list.rs:66 - Three behaviours the code promises have no test — see inline at
crates/trusted-server-cli/tests/keys_cli.rs:181
Non-blocking
🤔 thinking / ⛏ nitpick
- 🤔 The guide doesn't say what
ts keys verifyproves — see inline atdocs/guide/key-rotation.md:328 - 🤔 "internal verification error" reads like a bad signature — see inline at
crates/trusted-server-cli/src/commands/keys/verify.rs:42 - 🤔 Service-controlled text reaches the terminal unescaped — see inline at
crates/trusted-server-cli/src/commands/keys/list.rs:42 - ⛏ The HTTPS error claims to protect credentials
ts keysnever sends — see inline atcrates/trusted-server-cli/src/url_guard.rs:56 - ⛏ The "byte for byte" test trims the newline it checks — see inline at
crates/trusted-server-cli/tests/keys_cli.rs:84
Cross-cutting / body-level findings
-
♻️ Converge with #1500 before the second of the two PRs merges.
- Command and dispatch: both PRs add
commands/keys/mod.rs, with incompatible shapes. Here it isKeysCommand { List, Verify }withrun(command, out) -> CliResult<RunOutcome>; in #1500 it isKeysCommand { Rotate, Deactivate }withrun(command, confirm, out) -> CliResult<()>. - HTTP plumbing exists twice:
sendhere andadmin_client::post_admin_jsonthere each build a runtime and a client withREQUEST_TIMEOUTandPolicy::none(), then read the body. - URL checks exist twice:
url_guard::service_endpointand #1500'sadmin_endpointare the same check, except for the dead userinfo test this PR correctly drops. - Proposed end state: whichever PR lands second should leave one
KeysCommand(list, verify, rotate, deactivate) and onerunthat returnsRunOutcomeand takes the confirmer. It should also leave one transport with optional Basic auth, used byts cache purgetoo. TheKeyshelp line ("Inspect a deployed service's request-signing keys") should cover rotation as well.
- Command and dispatch: both PRs add
-
🏕 Other guides still teach curl for these two routes:
docs/guide/error-reference.md:475(curl … | jq '.jwks.keys[].kid')docs/guide/request-signing.md:118-180and:336docs/guide/api-reference.md:685and:742- In this guide: the "Testing" list (
key-rotation.md:620-622) and the "Signature Verification Fails After Rotation" and "Key Not in JWKS" troubleshooting entries (:649-672)
Each one can switch to
ts keys listorts keys verifywith a one-line edit. -
📝 Release notes and the CLI index don't mention
ts keys.CHANGELOG.md### Addedhas no entry for the two commands. The hand-maintained command index indocs/guide/cli.mdhas nots keys,ts keys listorts keys verifyrows; it already lacksts cacheandts origintoo. -
📝 On the open questions:
--payload-file/stdin:/verify-signaturecaps the whole JSON body at 4096 bytes, and a 5000-byte--payloadgot413 Payload Too Largefrom the Axum adapter. The payload is also a JSON string verified as its UTF-8 bytes. A file input therefore can't carry large or binary payloads. It would still help with quoting and with$(cat file)dropping trailing newlines, so it fits a follow-up rather than this PR.--jsonforverify: printing the service's answer verbatim, with the same exit codes, would mirrorlist --json. Also a follow-up.
-
📝 The earlier spec finding still applies:
CONTRIBUTING.md("What counts as a feature") lists a new CLI command as a feature that needs a linked, agent-reviewed spec.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests (Fastly EC lifecycle): PASS
- cargo test (axum native): PASS
- CodeQL: PASS
- Analyze (actions): PASS
- CI Node pin guard: PASS
- cargo test: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (rust): PASS
- CLAUDE.md symlink guard: PASS
- cargo fmt: PASS (required)
- worker-deadlines (spin): PASS
- cargo test (cross-adapter parity): PASS
- ts dev lint domains (changed lines): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- format-docs: PASS (required)
- HTML EOF runtime (Fastly): PASS
- vitest: PASS
- Analyze (javascript-typescript): PASS
- docs-proposal scripts: PASS
- worker-deadlines (cloudflare): PASS
- format-typescript: PASS (required)
- prepare integration artifacts: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- Analyze (python): PASS
- Analyze (javascript-typescript): PASS
ts keys verify now accepts --signature and --payload values that start with a hyphen, which about one base64url signature in 64 does. Kid, type, curve, message and error text from the service go through escape_terminal_text before they reach the terminal. ts keys list hints that a 404 means --service is not a Trusted Server and that a 500 means unreadable signing keys. ts keys verify names the possible causes behind the service's sanitized internal verification error. The HTTPS refusal now says it guards the request and the answer, not only credentials. Add tests for redirect refusal, --json validation, a non-success verify status and the exact --json output. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Replace curl calls for the discovery document and /verify-signature in the error reference, request-signing guide, API reference and key-rotation troubleshooting with ts keys list and ts keys verify. Document the verify exit codes and that a deactivated key kept in storage still verifies. Add ts cache and ts keys rows to the CLI index and a changelog entry. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
|
@aram356 thanks for the review. All 8 inline threads are fixed in d995804 (code and tests) and d5aa7ab (docs), and I've resolved them. Replies to the review-body points:
I updated the PR description so |
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Adds signing-key inspection commands, shared service URL validation, and operator guidance. Non-success HTTP response bodies still reach the terminal unescaped; the inline suggestion closes that error-path bypass for both commands.
The inline comment carries one one-click GitHub
suggestion. Use Commit suggestion or Add suggestion to batch to apply it on the PR branch.
Blocking
🔧 wrench
- Escape HTTP error bodies before logging — see inline at
crates/trusted-server-cli/src/commands/keys/mod.rs:117.
CI Status
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- vitest: PASS
- CLAUDE.md symlink guard: PASS
- HTML EOF runtime (Fastly): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- ts dev lint domains (changed lines): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- docs-proposal scripts: PASS
- cargo fmt: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- format-typescript: PASS (required)
- CI Node pin guard: PASS
- Analyze (javascript-typescript): PASS
- CodeQL: PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- format-docs: PASS (required)
- Analyze (javascript-typescript): PASS
- Analyze (rust): PASS
- worker-deadlines (spin): PASS
- Analyze (actions): PASS
- prepare integration artifacts: PASS
- Analyze (python): PASS
- cargo test: PASS (required)
- worker-deadlines (cloudflare): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
Suggestion verification
The exact replacement was checked in isolation: formatting; all eight clippy aliases; Fastly, Axum, and Cloudflare compile checks; the native CLI suite; Fastly (including reusable-sandbox), Axum, Cloudflare, and Spin test aliases; and cross-adapter parity all passed. A CLI fixture confirmed that HTTP 500 terminal controls are escaped by both commands. The byte-exact post-verification drift check passed, and the scratch worktree was restored cleanly to the reviewed head.
The initial Fastly test attempt selected an outdated Viceroy 0.17 from PATH and could not link a required import; rerunning with the already-installed, repository-pinned Viceroy 0.21 passed.
aram356
left a comment
There was a problem hiding this comment.
Summary
All eight round-1 findings are fixed as suggested. These mutants now fail the trusted-server-cli suite:
- Round-1 mutants: redirect refusal,
--jsonvalidation, verify's status check, and the trailing newline. - New mutants: the 404 and 500 hints, both
allow_hyphen_valuesattributes, and the internal-error arm.
The escaping fix is pinned for most fields; two of the nitpicks below cover the three wrappers that tests don't yet reach. The branch still merges cleanly onto current main. One new blocker: the API and error references tell operators to base64-encode the payload, which makes a valid signature report as not verified.
All 4 inline comments below carry a one-click GitHub
suggestion— use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. The first also describes two lines outside the diff that need a manual edit.
Blocking
🔧 wrench
- The docs teach a base64 payload, but the service verifies raw bytes — see inline at
docs/guide/api-reference.md:749
Non-blocking
⛏ nitpick
[--json]breaks this line when it is copied — see inline atdocs/guide/api-reference.md:685- The list escaping test leaves
ktyandcrvunpinned — see inline atcrates/trusted-server-cli/src/commands/keys/list.rs:135 - The verify escaping test never reaches the
verified:line — see inline atcrates/trusted-server-cli/src/commands/keys/verify.rs:183
Cross-cutting / body-level findings
-
❓ Has a maintainer waived the spec requirement for this PR?
CONTRIBUTING.md("What counts as a feature") lists a new CLI command as a feature that needs a linked, agent-reviewed spec.- The description's checklist still leaves that box unticked.
- Please either link a spec or note here that a maintainer waived it for this change.
-
📝 On the open thread about error bodies (
commands/keys/mod.rs:117): we agree they should be escaped, with one refinement.- The suggested patch escapes before
run_listandrun_verifycall.trim(), so a trailing newline survives as a literal escape. - With a 404 page ending in a newline, the error ends
</html>\u{000A}. - Trimming first, with
escape_terminal_text(text.trim()).into_owned(), prints</html>. - The later
.trim()calls then have nothing left to remove.
- The suggested patch escapes before
-
📝 Merging with #1500. The plan in the description holds. A test merge of the two current heads:
- conflicts in
CHANGELOG.md,commands/cache/purge.rs,commands/keys/mod.rs(add/add),commands/mod.rs,run.rsanddocs/guide/cli.md; - merges
url_guard.rscleanly, because both PRs now add the sameservice_endpoint; - leaves two
cli.mdts keysrows with different summaries ("Inspect a deployed service's request-signing keys" and "Request-signing key rotation through a deployed service's admin routes"). The merged row and theKeyshelp line need one wording.
Please keep this PR's hyphen, escaping, redirect and hint tests through the rebase.
- conflicts in
-
🌱 A home for
escape_terminal_text.keys/list.rsandkeys/verify.rsnow import it fromcrate::ad_templates::output, a feature module, and the open error-body suggestion does too.crate::outputalready owns the CLI's terminal output, so it would be the better home, in a follow-up.
CI Status
- browser integration tests: PASS
- trace runtime and browser acceptance: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- vitest: PASS
- CLAUDE.md symlink guard: PASS
- HTML EOF runtime (Fastly): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- ts dev lint domains (changed lines): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- docs-proposal scripts: PASS
- cargo fmt: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- format-typescript: PASS (required)
- CI Node pin guard: PASS
- Analyze (javascript-typescript): PASS
- CodeQL: PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- format-docs: PASS (required)
- Analyze (javascript-typescript): PASS
- Analyze (rust): PASS
- worker-deadlines (spin): PASS
- Analyze (actions): PASS
- prepare integration artifacts: PASS
- Analyze (python): PASS
- cargo test: PASS (required)
- worker-deadlines (cloudflare): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Reviewed 5d63ba0aedff4cb789ecb4c15e1e758d7a2c16c8 against base 598a8517072c52690a6b1833e8797d9003f3dba5. No additional distinct defects found beyond existing feedback. The following blockers remain; links point to the existing discussions rather than duplicating inline comments.
Blocking
- 🔧 HTTP errors bypass terminal escaping — existing thread at
keys/mod.rs:117. Both commands interpolate non-success response bodies into logged errors. A loopback HTTP 500 fixture confirmed that ESC[2K and carriage return bytes reach stderr unchanged in both commands, allowing forged terminal output. Escape trimmed error bodies while preserving successful JSON, and cover both commands with a regression fixture. - 🔧 Payload guidance causes valid signatures to fail — existing thread at
api-reference.md:753. The examples teach base64 encoding, whilehandle_verify_signatureverifiespayload.as_bytes()directly. Encoding the original signed message changes its bytes. Use the original message, such asHello World, in the CLI/curl examples and correct the related request-body sample and error-reference guidance. Signature encoding is independently base64url or standard base64. This pass confirmed the contract by source inspection; it did not repeat the earlier Axum signing experiment. - 🔧 Required feature spec is missing —
CONTRIBUTING.mdexplicitly includes new CLI commands in features requiring a written spec reviewed over at least two rounds. The issue reference does not meet that requirement, and no maintainer waiver was established in the published discussion. Provide and link the reviewed spec or record a maintainer waiver. This remains a process/readiness obligation rather than a runtime defect.
Non-blocking
- ⛏ Replace literal
[--json]in the executable example — existing thread. The copied argument was reproduced as an unexpected-argument error with exit 2. Show separate invocations with and without--json. - ⛏ Exercise
ktyandcrvescaping — existing thread. The fixture currently puts control characters only inkid; add hostile values in the other two columns too. The production wrappers are present and correct. - ⛏ Exercise successful verification output escaping — existing thread. The escaping fixture currently exercises only
verified: false; add a success case with controls inkid. The production success wrapper is present and correct.
Verification and limits
All 14 changed paths and relevant unchanged signing, publication, URL-validation, output and process-exit paths were inspected. The initial eight inline findings are addressed. The planned integration with #1500 remains a future rebase obligation; this review covers the present revision.
With Rust 1.98.1 and Node 24.20.0:
- Nine native CLI fixture tests passed. The first attempt was blocked by sandbox loopback restrictions; the compiled tests passed with loopback permission.
- Ten key-command unit tests and the hyphen-value parser regression passed.
cargo fmt --all -- --checkandgit diff --checkpassed.- The HTTP error-body and literal
[--json]failures above were reproduced against the reviewed CLI.
The full CLI and adapter suites, clippy aliases, and deployment tests were not rerun locally. No new suggestion was applied or claimed scratch-verified; the prior mutation experiments for the two optional test improvements were not repeated. Hosted CI still has pending checks, with no failures in the current snapshot.
CI status
- browser integration tests: PENDING
- trace runtime and browser acceptance: PENDING
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- release scripts: PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- worker-deadlines (spin): PASS
- worker-deadlines (cloudflare): PASS
- cargo test: PASS (required)
- format-typescript: PASS (required)
- HTML EOF runtime (Fastly): PASS
- cargo test (axum native): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- CI Node pin guard: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (rust): PASS
- format-docs: PASS (required)
- Analyze (javascript-typescript): PASS
- docs-proposal scripts: PASS
- cargo test (cross-adapter parity): PASS
- Analyze (python): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- CLAUDE.md symlink guard: PASS
- ts dev lint domains (changed lines): PASS
- vitest: PASS
- prepare integration artifacts: PASS
Escape non-success response bodies in ts keys after trimming, pin the remaining escaped fields in tests, and document that /verify-signature verifies the raw payload, not a base64 encoding. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Summary
ts keys list [--json]so operators can confirm which signing keys a service publishes in/.well-known/trusted-server.jsonwithout the admin password or hand-written curl.ts keys verifyto have the service check a signature against the key stored under a kid. It exits0when the signature verifies,1when it does not and2when the check could not be made. The service checks stored keys whether or not they are still active, so usets keys listto see what partners can fetch.url_guard::service_endpointfromts cache purgeso every--serviceflag shares the same base-URL validation (HTTPS-only for non-loopback, no credentials/path/query/fragment).Changes
crates/trusted-server-cli/src/url_guard.rsservice_endpointhelpercrates/trusted-server-cli/src/commands/cache/purge.rsservice_endpointinstead of local validationcrates/trusted-server-cli/src/commands/keys/mod.rskeyscommand group, discovery fetch helperscrates/trusted-server-cli/src/commands/keys/list.rsts keys list [--json]crates/trusted-server-cli/src/commands/keys/verify.rsts keys verifycrates/trusted-server-cli/src/commands/mod.rs,src/run.rskeyscrates/trusted-server-cli/tests/keys_cli.rsdocs/guide/key-rotation.mdts keys list/verifyfor confirming publication and troubleshooting; document verify exit codesdocs/guide/error-reference.md,request-signing.md,api-reference.md/verify-signaturecurl calls withts keysdocs/guide/cli.md,CHANGELOG.mdts cacheandts keysindex rows and a changelog entryCloses
Closes #1431
Merge order with #1500
#1500 (
ts keys rotate/deactivate) also addscommands/keys/mod.rsand extracts shared URL validation. #1502 merges after #1500 and will be rebased onto it then. The rebase merges the two into oneKeysCommand(list, verify, rotate, deactivate), onerunthat returnsRunOutcome, and one transport and URL check, withts cache purgeusing them too.Follow-ups
--payload-file/ stdin forverify. The 4096-byte/verify-signaturebody cap rules out large or binary payloads, but a file input still helps with quoting.--jsonoutput forverify, mirroringlist --json.Spec
Spec: Scoped by issue #1431 (small CLI addition).
Test plan
cargo test-fastly && cargo test-fastly-reuse && cargo test-axum && cargo test-cloudflare && cargo test-spincargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest run(1838 passed / 1 skipped, Node 24.20.0)cd crates/trusted-server-js/lib && npm run format && npm run lintcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servets dev lint domains, CLI tests (cargo test -p trusted-server-cli --target <host>),template_build_digest, parityChecklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)