Add ts cli#799
Conversation
7fdbff6 to
2ea46f1
Compare
2ea46f1 to
edb7f0c
Compare
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Introduces the host-target ts operator CLI and, more significantly, re-architects runtime settings loading: config moves from a build-time-embedded TOML (include_bytes!(target/trusted-server-out.toml), baked by build.rs) to a runtime read of an EdgeZero BlobEnvelope from a platform config store, with per-chunk + envelope SHA-256 integrity verification and a Fastly chunk-pointer reassembly path. build.rs becomes a no-op, the config crate drops to a dev-dependency, and EdgeZero git deps are pinned to a fixed rev.
Clean, well-tested work — strong unit coverage (round-trip, tamper-rejection, chunk reassembly, placeholder rejection) and good error messages. No blocking issues; all findings below are non-blocking.
3 of the inline comments carry a one-click GitHub
suggestion(verified in a scratch worktree:cargo fmt --check,cargo clippy -p trusted-server-core --all-targets --all-features -D warnings, andcargo test -p trusted-server-core— 1426 passed). The canary comment is prose because it has no single clean fix.
Non-blocking
♻️ refactor
request_body_bytesis an incomplete abstraction (proxy) — see inline atcrates/trusted-server-core/src/proxy.rs:44request_body_bytesis an incomplete abstraction (request signing) — see inline atcrates/trusted-server-core/src/request_signing/endpoints.rs:27
🤔 thinking
- Unbounded pre-allocation from operator-controlled
envelope_len— see inline atcrates/trusted-server-core/src/settings_data.rs:113 - EdgeZero entry-point canary downgraded to a no-op — see inline at
crates/trusted-server-integration-tests/tests/common/ec.rs:283
Cross-cutting / body-level findings
- 🤔 Startup
INSECURE: proxy.certificate_check is disabledwarning dropped — the oldload_settingsemitted a startuplog::warn!whenproxy.certificate_checkwas off; the new blob path (settings_from_config_blob/get_settings_from_config_store) does not. A per-backend warning still exists at request time (adapter-fastly/src/backend.rs:219), so coverage isn't zero, but the loud startup signal is gone. Cheap to re-add inget_settings_from_config_store. - 📝
#[serde(deny_unknown_fields)]added broadly (settings.rs,consent_config.rs,auction_config_types.rs) — good hardening (catches typo'd keys; thefrom_tomltest flipped from "extra fields ignored" to "rejected"), but it's a behavior change: any deployed config carrying extra/forward-compat keys now fails to load. Validated atts config pushtime, which is the right place — flagging the migration implication for existing operator configs. - 🌱 Minor:
adapter-fastly/src/main.rsopens the sameTRUSTED_SERVER_CONFIG_STOREtwice per request (open_trusted_server_config_storefor the flag +edgezero_config_store_handlefor dispatch) — harmless, mildly wasteful. Separately,.github/workflows/test.yml's "Verify Fastly WASM release build" step still setsTRUSTED_SERVER__PUBLISHER__ORIGIN_URL, now vestigial sincebuild.rsno longer bakes env-derived config.
👍 praise
- EdgeZero git deps pinned from
branch = "main"to a fixedrev— reproducible builds. - Integrity model in
resolve_fastly_chunk_pointer+settings_from_config_blob: per-chunk len+sha, envelope len+sha, andBlobEnvelope::verify()— solid defense-in-depth, withtampered_blob_hash_is_rejectedandstrings_that_look_like_json_scalars_round_trip_as_stringsguarding the TOML→JSON→TOML secret round-trip. build.rsreduced from 72 lines of#[path]-included module hacks to a 3-line no-op.
CI Status — all 14 checks PASS
- cargo test: PASS (required)
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- Analyze (rust): PASS
- Analyze (javascript-typescript): PASS
- Analyze (actions): PASS
- CodeQL: PASS
- vitest: PASS
- integration tests: PASS
- integration tests (EdgeZero entry point): PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Review of #799. The substantive change is sound: runtime config moves from build-time-baked TOML to a config-store blob envelope with solid integrity checks (per-chunk + envelope len/SHA-256 + verify() + post-load placeholder rejection), and the host-only ts CLI is cleanly isolated from the wasm build. CI is green. Findings are mostly non-blocking — no 🔧 blockers — with one open ❓ question on secret handling worth resolving before merge.
Blocking
❓ question
- Plaintext secrets in the app-config blob —
crates/trusted-server-core/src/config.rs:92(inline).
Non-blocking
🤔 thinking
- Settings reloaded up to 3×/request —
crates/trusted-server-adapter-fastly/src/main.rs:355,383(inline). - Dead build-time env exports —
scripts/integration-tests.sh:54-58(inline). - EdgeZero entry-point canary downgraded to a warning —
crates/trusted-server-integration-tests/tests/common/ec.rs:296(inline). - Auction test name overstates coverage —
crates/trusted-server-core/src/auction/endpoints.rs:808(inline). - Broad
#[serde(deny_unknown_fields)](settings.rs+ ~20 config structs): unknown/typo'd keys now hard-fail load, which at runtime drops the whole app to the startup-error router. Good safety, but potentially breaking for existing operator configs that carried extra keys — worth a release note.
♻️ refactor
- No-op
Resultbody wrappers with unused_endpoint—crates/trusted-server-core/src/proxy.rs:41,crates/trusted-server-core/src/request_signing/endpoints.rs:27(inline). - Track the EdgeZero
feature/extensible-clibranch —Cargo.toml:39-44,crates/trusted-server-integration-tests/Cargo.toml:13(inline suggestions). Verified compiling against branch HEADbf177b13.
🌱 seedling / 📝 note
prebid::validate_config_for_startuplacks a direct unit test —crates/trusted-server-core/src/integrations/prebid.rs:201(inline).- Integration
cargo testnow depends on a generated config artifact —crates/trusted-server-integration-tests/tests/environments/fastly.rs:82(inline). - Unused workspace dependency
edgezero-adapter(Cargo.toml:39) — no first-party crate references it (only transitive viaedgezero-cli). Drop it or document intent. - Reproducibility — a
branch =ref (per the suggestions above) is not reproducible across machines/CI; pin back to a specific rev before merge.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests (vitest): PASS
- integration tests (incl. EdgeZero entry point): PASS
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Strong, well-tested PR. Unifies runtime config loading onto the EdgeZero app_config blob, adds the host-target ts operator CLI, pins all edgezero git deps to a fixed rev, and hardens request-body handling. Read every changed file; no bugs found. Findings below are non-blocking — one open question on secret-at-rest storage and several consistency/hardening suggestions.
Blocking
❓ question
- Secrets stored in the config-store blob, not the secret store —
TrustedServerAppConfig::SECRET_FIELDS = &[]keepsec.passphrase,publisher.proxy_secret,handler.password, andec.partners[].api_tokenas plaintext JSON inside theapp_configconfig store. Intentional for this phase? (See inline comment onconfig.rs:92.)
Non-blocking
♻️ refactor
- Auction stream-body handling —
auction/endpoints.rs:81silently treats a streamed body as empty; add an explicitis_stream()guard for parity withproxy.rs/request_signing. - Vacuous
Resultonbody_as_reader—proxy.rs:40andpublisher.rs:35returnResultbut never error; add the stream guard or keep infallible.
🤔 thinking
x-ts-entry-point: edgezeroon all responses — exposes internal routing/migration state to clients; used as a CI canary (main.rs:488). Consider gating/documenting.- Dependency-parity allowlist grew ~18 crates —
check-integration-dependency-versions.sh; each entry weakens the drift guard.
🌱 seedling
get_settings_from_servicesunused —settings_data.rs:42,pubwith no in-repo caller; confirm it's for future adapters or drop.- Unbounded blob reassembly —
settings_data.rs:114, chunks concatenated before the total-length check.
📝 note
#[serde(deny_unknown_fields)]is a behavior change — existingtrusted-server.tomlfiles with extra/typo'd/legacy keys now hard-fail validation and runtime load (the round-trip test flipped from "extra fields ignored" to "rejected"). Good hardening — call it out in migration/release notes (settings.rs, plus the consent/auction config structs).- CLI error type —
Result<(), String>deviates from theerror-stackconvention but is constrained byedgezero_cli's API (run.rs:54).
⛏ nitpick
- Store-id resolution —
default_config_store_namereads raw env whiledefault_config_keyusesEnvConfig(settings_data.rs:52).
👍 praise
- Streaming-body rejection on sign/verify/rotate/deactivate request paths closes a real correctness gap (streamed bodies were silently treated as empty — could bypass signature verification), with thorough tests.
- Pinning all edgezero deps from
branch = "main"to a fixed rev — reproducible builds. - Defense-in-depth on Fastly chunk reconstruction: per-chunk len + sha, envelope len + sha, then envelope integrity verify.
- Reusing the request-scope
AppStatesettings snapshot instead of re-reading the config store on every finalize / EC step — removes redundant reads and guarantees finalize uses the same settings that built the router. is_edgezero_enabledswitched totry_get(no panic on backend error) with graceful legacy fallback.- Removing build-time secret env injection from CI now that runtime loads from the config store.
CI Status
- fmt: PASS
- clippy (workspace + host-target CLI): PASS
- rust tests (workspace + host-target CLI): PASS
- js tests (vitest): PASS
- integration / edgezero / browser: PASS
- docs + ts format, CodeQL: PASS
aram356
left a comment
There was a problem hiding this comment.
Re-review — follow-up fd0322c2
The follow-up commit resolves the substantive findings from the prior review, with tests:
- Per-request settings reload →
edgezero_mainnow reuses the request-scopeAppState.settingssnapshot for entry-point + EC finalization (one load per request), with a reload fallback only for startup-error routers. - Streaming request bodies →
request_body_bytesrejectsBody::StreamwithBadRequest; sign / verify / rotate streaming-rejection tests added. - EdgeZero entry-point guard → new
x-ts-entry-point: edgezeroheader is hard-asserted by the EdgeZero CI job, replacing the dropped405canary. - Dead build-time env exports removed from
scripts/integration-tests.shandtest.yml; strict key validation documented; missing Viceroy config now errors with guidance. - Bonus hardening: chunk-pointer reconstruction no longer pre-allocates from the operator-controlled
envelope_len(String::new()), removing an untrusted-capacity vector. 👍
The open ❓ on SECRET_FIELDS = &[] is answered (intentional inline-secret behavior for this PR; secrets-store routing deferred), and the auction-test-name / prebid-direct-test items are acknowledged as scoped follow-ups.
Approving. A few non-blocking nits inline: a leftover no-op Result in body_as_reader, the always-on entry-point header's client visibility, EC-finalize branch duplication, and an unused workspace dependency.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests (vitest): PASS
- integration tests (incl. EdgeZero entry point): PASS
Replace the custom trusted-server CLI lifecycle and config payload plumbing with a thin EdgeZero delegation layer using typed config push/validate flows. Add TrustedServerAppConfig wrapper in core with deploy-time validation and move blob reconstruction into runtime helpers. Drop flattened config entry publishing and route app-config through EdgeZero blob envelope handling while keeping edgezero flag reads in trusted_server_config. Update CLI and architecture docs for the new model and adjust fastly adapter store selection.
fd0322c to
4c2f0ab
Compare
495df25 to
1fe62da
Compare
aram356
left a comment
There was a problem hiding this comment.
Re-review (3rd pass) — head 37718b7
Everything actionable from the two prior review rounds is resolved, and the new work is well-tested. CI is green across all adapter jobs (fastly, cloudflare, spin, axum, cross-adapter parity).
Prior nits addressed: unused edgezero-adapter dep removed; the no-op Result in body_as_reader fixed in both proxy.rs and publisher.rs; SECRET_FIELDS = &[] documented with a Phase-1 rationale; the x-ts-entry-point header removed.
New work reviewed:
- Chunk-pointer hardening (
settings_data.rs): validates the ≤8000-byte Fastly entry limit, per-chunk lengths, and achecked_addsum againstenvelope_lenbefore allocating, with a running overflow guard and a new mismatch test. Switched toEnvConfigconsistently. - Cloudflare runtime config loading: clean
cfg(wasm32)OnceLockinjection viaenv.var("TRUSTED_SERVER_CONFIG")→settings_from_config_blob, with properReporterror handling. - Route-dispatch test coverage, rebase API-drift fixes, and an EdgeZero dep rev bump (still pinned).
Approving. Three non-blocking notes inline: the removed EdgeZero entry-point CI guard (now relying on forced rollout_pct=100 rather than a positive assertion), the Cloudflare example-config fallback shipping placeholder secrets, and a brittle test-config string substitution.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests (incl. cross-adapter parity): PASS
- js tests (vitest): PASS
- integration tests (incl. EdgeZero entry point): PASS
|
|
||
| fn generated_config_store_blocks(envelope_json: &str, edgezero_enabled: bool) -> String { | ||
| let edgezero_enabled_value = if edgezero_enabled { "true" } else { "false" }; | ||
| let edgezero_rollout_pct = if edgezero_enabled { "100" } else { "0" }; |
There was a problem hiding this comment.
🤔 thinking — Forcing edgezero_rollout_pct = "100" here is a good way to make the EdgeZero CI job take the EdgeZero path. But note cfcd35885 also removed assert_edgezero_entry_point, the x-ts-entry-point header, and the EXPECT_EDGEZERO_ENTRY_POINT gate — so there's no longer a positive assertion that test_ec_lifecycle_fastly actually routed through edgezero_main. If main() silently falls back to legacy_main (e.g. the config store can't be opened), the EC scenarios still pass and the integration tests (EdgeZero entry point) job stays green. Forcing rollout makes that unlikely, but the explicit guard is gone. If keeping it matters, a debug-gated signal or a startup log-assertion would restore the guarantee without the always-on client-visible header. Non-blocking.
| return build_state_with_settings(settings); | ||
| } | ||
|
|
||
| let settings = Settings::from_toml(include_str!("../../../trusted-server.example.toml"))?; |
There was a problem hiding this comment.
📝 note — When TRUSTED_SERVER_CONFIG is unset, build_state falls back to Settings::from_toml(example.toml). Unlike the blob path (settings_from_config_blob), from_toml does not run reject_placeholder_secrets, so a misconfigured Worker boots with the template's placeholder secrets instead of failing fast. Dev-only adapter and pre-existing pattern, but worth a guard (or at least a log::warn!) so a missing config surfaces loudly. Non-blocking.
| template_path.display() | ||
| ))?; | ||
| let config_json = cloudflare_config_json(origin_port())?; | ||
| let generated = template.replace( |
There was a problem hiding this comment.
⛏ nitpick — template.replace("TRUSTED_SERVER_CONFIG = \"{}\"", …) silently no-ops if the template placeholder ever drifts, in which case the generated config omits TRUSTED_SERVER_CONFIG and the Worker quietly falls back to example config. Consider asserting the replacement changed the string (e.g. assert_ne!(generated, template)). Test-only.
Summary
tsoperator CLI crate and binary.Changes
crates/trusted-server-cli/crates/trusted-server-core/src/config_payload.rscrates/trusted-server-core/src/settings_data.rscrates/trusted-server-adapter-fastly/src/app.rs,src/main.rs.github/workflows/*,.cargo/config.tomledgezero.toml,trusted-server.example.toml,.gitignoredocs/guide/cli.md,docs/guide/getting-started.md,README.mdCloses
No issue provided; this PR is split out from the combined
feature/ts-cli-nextbranch.Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -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 test --package trusted-server-cli --target aarch64-apple-darwin— 12 passedChecklist
unwrap()in production code — useexpect("should ...")println!)