Add Fermyon Spin adapter (wasm32-wasip1)#735
Conversation
Adds trusted-server-adapter-spin: Spin entrypoint via edgezero_adapter_spin::run_app, EdgeZero and Spin runtime manifests, platform runtime services (null geo, sync Spin variable secret adapter, conservative buffered HTTP client with gzip/br decompression policy), route/auth smoke tests, and EdgeZero manifest validation test. Bumps EdgeZero deps to ce6bcf74b529d9066d08ba87b2971af8379eb29e to access edgezero-adapter-spin. The new rev requires fastly = "0.12" (edgezero-adapter-fastly workspace dependency) and worker = "0.8" (Cloudflare adapter type compatibility). Pins viceroy 0.16.5 to resolve the Fastly SDK 0.12 bot-analysis hostcall issue. Extends parity tests to include Spin as a third in-process adapter. Adds target-matched cargo aliases and CI jobs for Spin native and wasm32-wasip1 targets. Updates CLAUDE.md lint and build guidance for the mixed-runtime workspace. Known MVP limits: Spin component variables do not map cleanly to all Trusted Server config keys; authenticated key rotation success is not claimed. Spin KV TTL is unavailable in the current EdgeZero Spin KV adapter.
KIDs starting with a digit would alias in the Spin variable encoder (both "1foo" and "n1foo" map to "v_n1foo"). Blocking them at validation eliminates the risk without changing the encoding scheme.
- Filter WASI HTTP P2 forbidden outbound headers (host, connection, keep-alive, transfer-encoding, upgrade, proxy-connection) before building the Spin outbound request; the host header caused every proxy request to fail with HeaderError::Forbidden - Fix push_spin_variable_escape to emit _xhh not _hh; corrects all variable names in spin.toml and encoded test expectations - Prepend n to digit-leading keys in spin_variable_name so Spin's letter-led segment requirement is met; aliasing now unreachable because validate_kid rejects digit-leading KIDs - Fix with_capacity to account for the optional n prefix - Remove redundant localhost entries from allowed_outbound_hosts (http://*:* already covers them) - Update spin.toml encoding comment: collision-free → collision-resistant
Resolve conflict in parity.rs: preserve axum_www_auth binding from base branch (required by assert_eq! below) and move spin WWW-Authenticate presence check to end of function, consistent with deactivate test pattern.
Spin's WASI HTTP bridge does not surface the incoming Host header via IncomingRequest::headers() — the authority is only accessible through Spin's spin-full-url synthetic header. EdgeZero's into_core_request forwards all headers including spin-full-url, but the Host header is absent. extract_request_host() returns "" which causes classify_response_route to fall back to BufferedUnmodified, skipping the HTML processor entirely: no TSJS injection, no URL rewriting, no GTM proxying. Inject Host from spin-full-url in dispatch before routing, via a simple host_from_spin_url helper that strips the scheme and path. The fix is Spin-specific and does not touch shared publisher code. Also addresses PR review findings: remove build_per_request_services passthrough wrapper, reduce MAX_DECOMPRESSED_SIZE to 8 MiB, annotate streaming body limitation, expand validate_kid digit test, upgrade parity WWW-Authenticate assertions from presence to value equality, and document the anyhow exception at the WASM FFI boundary.
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds a Fermyon Spin entry-point crate (wasm32-wasip1 component) with an EdgeZero manifest, platform runtime services, conservative response policy (hop-by-hop sanitization, gzip/br decompression with a 64 MiB cap), route/auth smoke tests, and a three-way parity test extension. Bumps the EdgeZero rev plus fastly 0.12, worker 0.8, and Viceroy 0.16.5 to track the new SDK surface.
Overall the adapter is well-tested with documented MVP limits, but the Spin variable encoder makes safety claims it doesn't enforce, and outbound HTTP has no timeout. Requesting changes on those two items; the rest are non-blocking suggestions.
Blocking
🔧 wrench
spin_variable_namecontract is broader than its safety claim: the comment + test name anchor onvalidate_kid, but the encoder is called by config and secret stores with arbitrary keys. Two reachable foot-guns — digit-leading aliasing and adjacent-underscore invalidity for uppercase/_/-/./:-leading keys. Fix by rejecting at the encoder boundary. (crates/trusted-server-adapter-spin/src/platform.rs:138-176)- No outbound HTTP timeout on Spin:
spin_sdk::http::sendis called with no timeout;NoopBackenddiscardsPlatformBackendSpec::first_byte_timeout. Either honor it via a race-with-delay future or document the limitation explicitly. (crates/trusted-server-adapter-spin/src/platform.rs:399-444)
Non-blocking
♻️ refactor
startup_error_routerskipsAuthMiddlewareand methods beyondGET/POST— the fallback router leaks startup error text on admin routes without a 401 challenge, and non-GET/POSTmethods bypass the error message entirely. (crates/trusted-server-adapter-spin/src/app.rs:128-154)- Duplicate
/first-party/signhandlers —fp_sign_get_handlerandfp_sign_post_handlerare byte-identical closures. (crates/trusted-server-adapter-spin/src/app.rs:264-288)
🤔 thinking
- Multi-provider fan-out rejected at request time only —
selectreturns an error when 2+ providers are submitted, but operators won't discover this until/auctiontraffic hits. A startup-time warning when adapter + provider count are incompatible would surface the limit sooner. (crates/trusted-server-adapter-spin/src/platform.rs:507-547) SpinSecretStoreAdapterUTF-8 and plaintext constraints — Spin variables are UTF-8 strings and unencrypted in the manifest by default. Today's JSON-encoded signing keys work; binary secrets would silently break, and production deployments must back the variables with a real secret-provider source. Worth a# Limitationsrustdoc section. (crates/trusted-server-adapter-spin/src/platform.rs:570-598)
⛏ nitpick
with_capacityunder-allocates — escapes are 4 bytes per char; current bound assumes 1:1. (platform.rs:150)- Test name overpromises —
spin_variable_name_digit_prefix_aliasing_is_unreachable_for_valid_kidsis true only for callers that pre-validate. Pair with an encoder-level test or rename. (platform.rs:980)
🌱 seedling
- No live Spin-runtime CI gate — the PR test plan's
spin up --from ...is intentionally unchecked. Today's CI builds and runs native-host route tests; a follow-up could exercise the WASM artifact under a real Spin runtime (the analog of Viceroy for Fastly).
📌 out of scope
clippy-cloudflareadded toformat.yml— closes an existing gap but is orthogonal to the Spin adapter. Calling it out so it isn't lost.
CI Status
- fmt: PASS
- clippy (Fastly / Axum / Cloudflare / Spin native + wasm32-wasip1): PASS
- cargo test (Fastly via Viceroy): PASS
- cargo test (Axum native): PASS
- cargo test (Cloudflare native): PASS
- cargo check/build (Spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- vitest: PASS
- format-docs / format-typescript: PASS
- integration + browser tests: PASS
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Submitting the review findings as a comment review. CI is passing. I did not duplicate the existing inline review comments for the Spin variable encoder, missing outbound timeout, or startup fallback because those are already covered by an existing changes-requested review on this PR.
The additional findings below are folded into the review body because their referenced lines are outside this stacked PR's current GitHub diff, so GitHub cannot attach them as inline comments on PR #735.
Additional findings
P1 — proxy-rebuild can mint valid click redirects without validating the original token
crates/trusted-server-core/src/proxy.rs:1107
handle_first_party_proxy_rebuild extracts tsurl, ignores any supplied tstoken, then signs a new /first-party/click URL. Because this route is public, an attacker can create valid click redirects to arbitrary URLs and, when a victim follows them, handle_first_party_click may append the victim's EC ID to the attacker-controlled redirect target.
Suggestion: Validate payload.tsclick with reconstruct_and_validate_signed_target before applying changes, reject missing/invalid tstoken, and enforce settings.proxy.allowed_domains in the click redirect path before returning Location.
P2 — Cloudflare integration build uses stale SYNTHETIC env var
.github/actions/setup-integration-test-env/action.yml:132
The Cloudflare build step sets TRUSTED_SERVER__SYNTHETIC__SECRET_KEY, but the current settings field is edge_cookie.secret_key. The override is ignored, so the Cloudflare integration artifact is built with the default config value instead of the intended integration-test secret.
Suggestion: Replace it with:
TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY: integration-test-secret-keyP3 — Public docs/README do not reflect Spin or target-matched commands
docs/guide/architecture.md:7, README.md:51
The public docs still describe only Fastly/Axum runtimes and recommend broad workspace clippy commands that the project now documents as problematic for mixed runtimes.
Suggestion: Update README and guide pages to include Spin commands (cargo test-spin, cargo check-spin, the Spin build command) and target-matched clippy aliases.
Existing findings already covered by prior review
- P1 — Spin variable encoder accepts unsafe key shapes (
crates/trusted-server-adapter-spin/src/platform.rs). - P1 — Spin outbound HTTP ignores configured timeouts (
crates/trusted-server-adapter-spin/src/platform.rs). - P2 — Spin startup fallback bypasses admin auth and only handles GET/POST (
crates/trusted-server-adapter-spin/src/app.rs).
Blocking: - Reject keys not starting with a lowercase ASCII letter at the spin_variable_name encoder boundary; removes the aliasing-prone digit-leading n-prefix branch and fixes with_capacity to worst-case (key.len() * 4 + 3). Updates affected tests to assert rejection. - Document the no-configurable-outbound-timeout limitation in SpinPlatformHttpClient rustdoc; PlatformBackendSpec::first_byte_timeout is ignored by NoopBackend and Spin's http::send has no per-request timeout API. Non-blocking: - startup_error_router now logs the error and returns a generic "Service Unavailable" body so deployment state is not leaked to anonymous callers; adds PUT/DELETE handlers for all catch-all routes. - Collapse duplicate fp_sign_get_handler/fp_sign_post_handler into one closure + clone, matching the get_fallback/post_fallback pattern. - Add # Limitations rustdoc section to SpinSecretStoreAdapter covering UTF-8-only variables and plaintext-at-rest in the default manifest. Security (P1 — proxy-rebuild): - handle_first_party_proxy_rebuild now validates the tstoken on the original tsclick URL via reconstruct_and_validate_signed_target before applying any parameter mutations. Without this an attacker could submit an unsigned tsclick and mint valid click redirects to arbitrary URLs. - Enforce settings.proxy.allowed_domains on tsurl before issuing the new signed redirect. CI fix (P2): - Replace stale TRUSTED_SERVER__SYNTHETIC__SECRET_KEY env var with TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY in the Cloudflare integration build step; the previous key was ignored, leaving the artifact built with the default config value.
- README Quick Start: add cargo test-spin - README Development: replace broad workspace clippy with target-matched aliases and note why; add cargo test-spin to test list - architecture.md: mention Cloudflare/Spin in the high-level overview; add trusted-server-adapter-spin section with build/test/lint commands and known MVP limits; expand Runtime Targets table to include all four adapters; add note on target-matched clippy requirement
Test was constructing a tsclick URL without a signature. Now that handle_first_party_proxy_rebuild validates the tstoken via reconstruct_and_validate_signed_target, the test must supply a properly signed click URL. Compute the token with compute_encrypted_sha256_token over tsurl + original params before building tsclick.
- Collapse nested if/if-let/if-let into a single let-chain condition (collapsible_if, Rust 2024 edition let chains) - Move host_from_spin_url test module to end of file so no items follow the test module (items_after_test_module)
Conflict resolutions: - .cargo/config.toml: keep PR18's clippy-cloudflare without --all-features (the cloudflare feature has a non-wasm32 compile_error! guard) alongside PR19's spin aliases - CLAUDE.md / README.md: keep PR19's per-adapter lint command list; drop the broad --all-features workspace clippy suggestion that the cloudflare guard would trip - cloudflare build.sh: keep PR19's worker-build ^0.8 (matches the worker 0.8 dependency bump) - integration-tests/Cargo.toml: keep PR19's edgezero rev ce6bcf74 (matches the bumped root workspace pin) with PR18's dual-bump warning comment, PR19's exact tokio pin, and PR18's urlencoding - core proxy.rs: keep PR18's #[test] + block_on style (core has no tokio, tests must run on wasm via Viceroy) with PR19's updated proxy_rebuild test body that signs the click URL for tstoken validation - parity.rs: keep PR19's three-way Spin coverage on top of PR18's routes_with_settings seams; spin_router() helper added; JSON key-set parity extended to Spin - Lock files regenerated from the merged manifests Semantic fixes for PR16-18 API changes in the Spin adapter: - handle_proxy now takes ProxyDispatchInput - handle_auction gained kv, partner registry, and EC context parameters - PlatformSelectResult gained failed_backend_name - supports_concurrent_fanout() reports false (send_async executes eagerly) so the orchestrator rejects multi-provider auctions before any request launches; select keeps its defense-in-depth rejection - resolve_publisher_response delegates to the shared buffer_publisher_response so the max_buffered_body_bytes cap applies on Spin too - TrustedServerApp::routes_with_settings() seam added; Spin middleware and route tests build explicit test settings instead of the baked placeholder secrets that get_settings() rejects Also adapt fastly main.rs TLS metadata calls to the fastly 0.12 API (get_tls_protocol / get_tls_cipher_openssl_name now return Result<Option<&str>>)
The merged [lints.clippy] block from PR18 now applies to this branch's pre-existing integration test files: - Collapse the nested if in is_ec_cookie_expired into a let-chain - Replace redundant closures with serde_json::Value::as_u64 - Replace the denied panic! in the EC scenario loop with an assert! carrying the same diagnostic message CI's freshly installed worker-build ^0.8 requires wasm-bindgen 0.2.123 or newer, but the lock files held worker 0.8.3 with wasm-bindgen 0.2.121 (js-sys exact-pins its companion wasm-bindgen, so only the worker 0.8.4 bump moves it). Update worker in both the workspace and integration-tests lock files.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the new Spin adapter, the core proxy/signing deltas, and the adapter parity/CI changes. CI is green, and the earlier review items for the Spin variable encoder, outbound timeout documentation, startup fallback leak, and unsigned proxy-rebuild have been addressed.
I found several issues that should block merging: the Spin adapter does not preserve trusted request scheme/sanitize spoofable forwarded headers for publisher/integration rewriting, its route table does not match the Fastly/Axum publisher fallback methods for non-GET/POST requests, and proxy-rebuild can strip the tsexp replay bound from a signed click URL and re-sign it indefinitely. I also left one non-blocking compatibility note about digit-leading legacy KIDs.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native+wasm build/check, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing reviews already covered and the current HEAD appears to have fixed the prior Spin variable encoder, timeout documentation, startup fallback, Cloudflare env var, docs, and initial proxy-rebuild validation findings.
Conflict resolutions: - CLAUDE.md / README.md: kept the spin-aware clippy aliases (clippy-spin-native, clippy-spin-wasm) and the build-alias note from the base. - integration.rs: both branches replaced the panic with an assert; kept the base's "should succeed" wording. - parity.rs: combined PR18's stronger assertions with PR19's three-adapter coverage — full parsed-body equality for the discovery JSON (incl. spin), /auction status equality plus non-404 across axum/cf/spin, and canonical /_ts/admin/keys/* paths for the admin auth parity tests. To make the merged admin-auth contract hold on Spin (which previously only registered legacy /admin/keys/*), registered the canonical /_ts/admin/keys/* routes on the Spin adapter and aligned its route tests to the production-shaped ^/_ts/admin handler regex, mirroring the PR18 fix for the Axum and Cloudflare adapters.
The Spin fallback dispatch reconstructed only the Host header from the trusted spin-full-url and left client-supplied Forwarded/X-Forwarded-* headers intact. Because build_runtime_services sets no TLS signal, detect_request_scheme had no trusted scheme source and fell back to http, or to a client-spoofed scheme/host, which publisher HTML rewriting, integration URL rewriting, and request-signing context then consumed. Strip the spoofable forwarded headers (mirroring the Fastly/Axum edge sanitization), then inject both the trusted Host and the trusted scheme parsed from spin-full-url. Extracted the logic into apply_trusted_host_and_scheme and added unit tests covering spoofed X-Forwarded-* headers and Host preservation.
The Spin route table only registered GET and POST for / and /{*rest} and only
each named route's primary method, so HEAD /, OPTIONS /page (CORS preflight),
HEAD /first-party/proxy, and similar requests returned a router-level 405
instead of reaching the publisher/integration fallback like Fastly and Axum.
Mirror the Fastly/Axum publisher_fallback_methods() pattern: register GET, POST,
HEAD, OPTIONS, PUT, PATCH, and DELETE for the catch-all and for every named
path's non-primary methods, all routed through the shared fallback. Dynamic
/static/tsjs= handling stays GET-only (now gated inside dispatch on the request
method). Added route tests for HEAD /, OPTIONS, and a non-primary method on a
named path; updated the PUT /verify-signature test to assert fall-through.
handle_first_party_proxy_rebuild validated the original tstoken but then let payload.del/add mutate any parameter, including the reserved tsexp replay bound that handle_first_party_proxy_sign attaches. A public rebuild request could take a still-valid expiring /first-party/click URL, delete tsexp, and receive a freshly signed click URL that reconstruct_and_validate_signed_target accepts indefinitely (expiration is optional). Treat tsexp, tstoken, and tsurl as reserved: reject del/add for them so the bounded expiration is preserved and re-signed. Added regression tests asserting deletion of tsexp is rejected and that a normal rebuild retains the tsexp bound.
The digit-leading KID rejection (added to block Spin variable-encoder aliasing) was applied through validate_kid to every caller, including handle_deactivate_key. Deployments that created operator-supplied digit-leading KIDs under the previous validation rules could no longer deactivate or delete them, since the request was rejected before storage was touched. Split validation: validate_kid_format enforces the structural constraints (length, charset) and validate_kid layers the digit-leading rejection on top for creation/rotation. Deactivation and deletion now use validate_kid_format so legacy digit-leading KIDs remain removable. Added tests covering both paths.
The Cloudflare build installed worker-build with a floating `^0.8`, which began resolving to worker-build 0.8.5. That release passes `--force-enable-abort-handler` to wasm-bindgen, but worker-build downloads the wasm-bindgen CLI matching the locked `wasm-bindgen` (0.2.123, pulled by worker 0.8.4), and that CLI does not recognise the flag — so the integration-test Cloudflare bundle build failed with "unexpected argument '--force-enable-abort-handler'". Derive the worker crate version from Cargo.lock and pin worker-build to it. worker-build is released in lockstep with worker, so this keeps the build tool and the wasm-bindgen CLI it drives compatible, and the pin advances automatically when the lockfile bumps worker. Verified locally: build.sh installs worker-build 0.8.4 and produces build/index.js without error.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter head, the prior review fixes, route/header behavior, request-signing/proxy changes, and CI/test coverage. CI is currently green and the earlier blocking review items appear to have been addressed, but I found one remaining blocking Spin correctness issue: the first-party proxy/click/sign path still receives path-only Spin URIs while the shared core handlers parse absolute URLs. That makes the creative first-party proxy flow fail on Spin. I also left two non-blocking compatibility findings for startup fallback method coverage and KID validation consistency.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing review feedback already covered the Spin variable encoder, outbound timeout docs, trusted scheme/header sanitization in publisher fallback, route fallback methods, proxy-rebuild signing controls, legacy digit-leading KID removal, Cloudflare worker-build pinning, docs, and startup error leak. I avoided duplicating resolved items except where the current code still leaves a distinct edge case.
Spin builds the core request URI from IncomingRequest::path_with_query(), so it is path-only (e.g. "/first-party/proxy?..."). The shared first-party proxy/click/sign handlers parse req.uri() with url::Url::parse, which rejected the relative path with "Invalid URL" — breaking the creative first-party proxy flow, click redirects, and GET sign query parsing on Spin. Rename apply_trusted_host_and_scheme to normalize_spin_request and have it rebuild an absolute URI from the trusted spin-full-url scheme+host, then apply it to every named first-party handler (proxy, click, sign GET+POST, proxy-rebuild) in addition to the publisher fallback. Also build the startup error router from the full publisher fallback method set so HEAD, OPTIONS, and PATCH on "/" and nested paths return the generic 503 instead of a router-level 405, matching the healthy router. Add regression tests: a signed proxy round-trip and GET sign query parse through the Spin router, plus a startup-fallback method-coverage unit test.
validate_kid now requires a lowercase ASCII leading character, matching the strictest platform key-name encoder (the Spin variable encoder, which rejects uppercase- and punctuation-leading KIDs and aliases digit-leading ones). A KID minted on any adapter must be storable on every backend, so create/rotate validates against the strictest common denominator and returns a client- correctable 400 instead of a runtime 5xx on Spin. Deactivation keeps the looser format check so legacy KIDs stay removable. Expose kid_is_creatable and add a Spin contract test asserting the encoder accepts every creatable KID, pinning core >= encoder strictness so the two lowercase-leading rules cannot silently drift.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter implementation, request normalization/routing parity, Spin platform services, core proxy/signing changes, and CI/tooling updates. I did not find a blocking correctness, security, data-loss, authorization, or severe compatibility issue, so this automated workflow is submitting COMMENT rather than REQUEST_CHANGES.
I left inline comments for one high-priority Spin response-size/resource-safety issue and a few non-blocking compatibility/tooling gaps.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing review threads on the Spin variable encoder, outbound timeout documentation, startup fallback, first-party request URI normalization, route fallback methods, proxy-rebuild signing controls, legacy KID removal, and worker-build pinning appear to have been addressed at the current head.
The 8 MiB ceiling was only applied after decompressing a gzip or br body. An identity response, or an unsupported encoding such as zstd, returned the fully buffered Vec<u8> with no size check, so a large origin/ad-server/proxy response could still be forwarded after exhausting the Spin WASM heap budget instead of failing with a typed error. apply_spin_response_policy now bounds the buffered body on every path via enforce_response_size before returning. For gzip/br this also rejects an overlarge compressed body before decompression, while decompress_body keeps bounding the expanded size. Add tests for an oversized identity body, an oversized unsupported-encoding body, and an identity body exactly at the limit.
normalize_spin_request now always sets Host from the trusted spin-full-url host instead of preserving a client-supplied value. Keeping an incoming Host while rebuilding req.uri() from the spin-full-url host left the shared RequestInfo path (publisher HTML rewriting, integration URL rewriting, signing context) reading one authority while handlers parsing req.uri() saw another. Overriding from the single trusted authority keeps both consistent and drops a client-spoofing vector. The unit test now asserts replacement. Also add GET /health returning 200 "ok" to both the healthy router and the startup-error fallback, matching the Fastly entry point and Axum adapter so deployments can reuse the same liveness probe on Spin. Spin's platform-provided /.well-known/spin/health is left untouched. Add route and startup-fallback health tests.
These workspace aliases excluded only Axum and Cloudflare, so after adding trusted-server-adapter-spin as a member they compiled the Spin adapter on wasm32-wasip1 without the spin feature. Match the same split already used by test-fastly and clippy-fastly and rely on the Spin-specific aliases/jobs for Spin coverage.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the new Fermyon Spin adapter, request normalization/routing, request-signing/admin endpoints, Spin platform services, parity tests, and CI/tooling updates.
I found one authorization issue that should block merge: the new Spin router exposes legacy /admin/keys/* admin aliases, but the production-shaped handler regex and Settings::ADMIN_ENDPOINTS only cover /_ts/admin/*, so unauthenticated requests can reach the admin handlers on those aliases. I also left one non-blocking privacy/compatibility note about forwarding Spin runtime synthetic headers to publisher origins.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing review threads on the Spin variable encoder, outbound timeout docs, startup fallback, first-party URI normalization, fallback methods, proxy-rebuild signing controls, legacy KID removal, response-size limits, Host normalization, health route, and worker-build/alias fixes appear addressed at the current head.
…aders The Spin router registered legacy `/admin/keys/*` aliases as admin routes, but the production basic-auth handler regex `^/_ts/admin` does not match them, so AuthMiddleware never challenged those paths and unauthenticated callers could reach the key rotate/deactivate handlers. Remove the aliases; unrouted, they fall through to the publisher fallback like any unknown path. The canonical `/_ts/admin/keys/*` routes stay auth-gated. Also strip Spin's synthetic `spin-full-url` and `spin-client-addr` headers in normalize_spin_request. They are runtime metadata whose parsed values are already available via SpinRequestContext, and leaving them on the request let the publisher fallback forward them to publisher origins. spin-full-url is removed as it is consumed; spin-client-addr is removed unconditionally. Add a parity regression test asserting the legacy aliases are not auth-gated admin routes and behave like an unknown publisher-fallback path, and extend the normalize test to assert both synthetic headers are stripped.
…/edgezero-pr19-spin-adapter # Conflicts: # crates/integration-tests/tests/parity.rs
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter request normalization, routing/auth behavior, Spin platform services, parity coverage, and the prior review fixes. CI is green, but I found two remaining security/correctness issues that should block merging: the adapter derives trusted host/scheme from a spoofable spin-full-url header path, and the auction route bypasses Spin normalization entirely so forwarded-header spoofing can still affect request-signing metadata. I also left one non-blocking compatibility finding for missing EC API route coverage on Spin.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing review threads on the Spin variable encoder, outbound timeout docs, startup fallback, first-party URI normalization, fallback methods, proxy-rebuild signing controls, legacy KID removal, response-size limits, Host normalization, health route, worker-build/alias fixes, and legacy admin aliases appear addressed at the current head. These comments avoid duplicating those resolved issues.
Two trusted-authority issues on the Spin adapter: - normalize_spin_request derived the trusted scheme/host from the first spin-full-url value (HeaderMap::remove returns the first). Spin appends its synthetic spin-full-url after the original client headers, so a client could prepend its own spin-full-url and win, choosing the host and scheme consumed by publisher HTML rewriting, integration URL rewriting, and request signing. Select the last (Spin-supplied) value instead and strip every copy. - POST /auction passed the request straight to handle_auction without normalization. Prebid request signing builds SigningParams from RequestInfo::from_request, which trusts Forwarded / X-Forwarded-* when present, so a client could spoof the host/scheme baked into signed OpenRTB metadata. Normalize the auction request like the other handlers. Add a unit test proving a client-duplicated spin-full-url cannot override the trusted authority, and a parity test proving spoofed forwarded headers do not change Spin /auction behavior. Also document that the EC API routes (batch-sync, identify) are intentionally unregistered on Spin, matching the Axum and Cloudflare adapters.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter head, request normalization/header trust boundaries, platform services, core proxy/signing changes, and CI/parity coverage.
CI is green, and the prior review threads for Spin full-url authority, admin aliases, response-size limits, proxy-rebuild signing controls, KID creation validation, route fallback methods, and tooling appear addressed. I found one remaining security issue that should block merge: the adapter still trusts a client-spoofable spin-client-addr value when building RuntimeServices::client_info, so client IP can be forged for EC generation and downstream auction/integration metadata. I also left one non-blocking compatibility note for TTL-backed consent persistence on Spin KV.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing review feedback already covered and appears to have fixed the earlier Spin variable encoder, outbound timeout docs, startup fallback, first-party URI normalization, fallback methods, proxy-rebuild signing controls, legacy KID removal, response-size limits, Host/full-url normalization, health route, worker-build pinning, and legacy admin alias issues.
aram356
left a comment
There was a problem hiding this comment.
Summary
The Spin adapter is well-built, and the security-critical normalize_spin_request hardening (3e6fbd90) verifies as correct — last-duplicate-wins authority selection, forwarded-header stripping, Host/scheme override, response-size + decompression bounding, and the parity/spoofing regression tests all hold up. All 12 CI checks are green. The blocking items below are about making the request-normalization invariant robust against future regressions and resolving one outbound-policy question — not a live exploit.
1 of the inline comments carries a one-click GitHub
suggestion. The rest describe the fix in prose because the change is architectural, a config-policy decision, or a doc note.
Blocking
🔧 wrench
- Make
normalize_spin_requesta chokepoint, not opt-in per handler — see inline atcrates/trusted-server-adapter-spin/src/app.rs:363
❓ question
- Does the proxy need plaintext
http://*:*outbound? — see inline atcrates/trusted-server-adapter-spin/spin.toml:35
Non-blocking
🤔 thinking / 📝 note / ⛏ nitpick
- Client-IP trust assumes direct termination (undocumented) — see inline at
crates/trusted-server-adapter-spin/src/platform.rs:714 - TTL-backed consent writes error on Spin KV (no silent degrade) — see inline at
crates/trusted-server-adapter-spin/src/platform.rs:213 - CI job name omits that it runs tests — see inline at
.github/workflows/test.yml:120(suggestion) scheme_host_from_spin_urldoesn't strip userinfo — see inline atcrates/trusted-server-adapter-spin/src/app.rs:144--no-default-featuresonclippy-spin-nativelooks redundant (unverified) — see inline at.cargo/config.toml:31
CI Status
- cargo fmt: PASS
- cargo test: PASS
- cargo check/build (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- vitest: PASS
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
- format-typescript: PASS
- format-docs: PASS
- No checks are marked required by branch protection on this stacked feature branch.
Address PR #735 review findings: - Fix client-IP spoofing (P1): edgezero populates `SpinRequestContext::client_addr` from the *first* `spin-client-addr` header, but Spin appends its synthetic value *after* client headers, so a client could prepend its own and forge the IP used by EC ID hashing, `/auction` `device.ip`, and integration `X-Forwarded-For`. `normalize_spin_request` now re-derives the trusted IP from the last occurrence and overwrites the context before stripping the header. - Make normalization a structural chokepoint: add `NormalizeMiddleware` (innermost, after auth) so every routed request is de-spoofed, instead of per-handler opt-in that discovery/verify/rotate/deactivate skipped. Remove the per-handler calls. - Document the `allowed_outbound_hosts` wildcard (arbitrary publisher/ad origins, some plaintext http) with a follow-up to scope it to configured origins. - Document the direct-termination trust assumption on `extract_client_ip`. - Clarify that Spin KV TTL writes error (not silently degrade), so TTL-backed consent persistence is unavailable and the core caller treats it as non-fatal. - Rename the CI job to reflect that it also runs Spin tests. - Strip optional userinfo in `scheme_host_from_spin_url` (cosmetic robustness). - Drop the redundant `--no-default-features` from `clippy-spin-native` (crate default is empty); verified clippy still passes. Adds regression tests for last-wins client-addr selection and userinfo stripping.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Automated review: I reviewed PR #735 against feature/edgezero-pr18-phase5-verification, focusing on the current Spin adapter implementation, request normalization/header trust boundaries, platform services, core proxy/signing deltas, parity coverage, and CI/tooling changes. CI is green, and I did not find a blocking correctness, security, data-loss, authorization, or severe compatibility issue at the current head.
I left one inline P2 finding for an integration-test build configuration issue: the Cloudflare artifact override was changed in this PR, but it still targets a non-existent EC settings key, so the intended integration-test passphrase is not actually embedded.
CI / Existing Reviews:
gh pr checks 735: all reported checks pass (fmt, Rust tests, cross-adapter parity, Spin native + wasm32-wasip1 check/build, Cloudflare checks, vitest, docs/TS formatting, integration/browser tests).- Existing review threads on Spin variable encoding, outbound timeout docs, startup fallback, request authority/client-IP normalization, first-party URI normalization, route fallback methods, proxy-rebuild signing controls, legacy KID removal, response-size limits, Host/full-url normalization, health route, worker-build pinning, and legacy admin aliases appear addressed at the current head. This review avoids duplicating those resolved items.
| TRUSTED_SERVER__PUBLISHER__ORIGIN_URL: http://127.0.0.1:${{ inputs.origin-port }} | ||
| TRUSTED_SERVER__PUBLISHER__PROXY_SECRET: integration-test-proxy-secret | ||
| TRUSTED_SERVER__SYNTHETIC__SECRET_KEY: integration-test-secret-key | ||
| TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY: integration-test-secret-key |
There was a problem hiding this comment.
Automated review: P2 / Medium — the Cloudflare integration artifact still ignores the intended EC passphrase override.
This PR changes the Cloudflare build from the removed SYNTHETIC key to TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY, but the current settings schema is [ec].passphrase, exposed as TRUSTED_SERVER__EC__PASSPHRASE (the Fastly build earlier in this action already uses that key). There is no edge_cookie.secret_key setting, so this override is ignored and the Cloudflare bundle is built with the committed local-dev passphrase instead of the integration-test passphrase. That makes EC behavior differ from the Fastly artifact and leaves the integration job unable to catch cross-runtime EC regressions.
Suggested fix: use the real settings key and a value that satisfies the 32-byte minimum, e.g. TRUSTED_SERVER__EC__PASSPHRASE: integration-test-ec-secret-padded-32. The Axum build step just above has the same stale EDGE_COOKIE override and should be corrected at the same time.
There was a problem hiding this comment.
Fixed in 76f2207. Both the Axum and Cloudflare integration builds now set TRUSTED_SERVER__EC__PASSPHRASE: integration-test-ec-secret-padded-32 (the real [ec].passphrase key the Fastly build already uses) instead of the dead EDGE_COOKIE__SECRET_KEY override, so the bundles embed the integration passphrase.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed PR #735 at the current head against feature/edgezero-pr18-phase5-verification, focusing on the Spin runtime path, shared proxy/signing behavior, and the CI/build path for the new adapter. CI is green, and many earlier findings appear fixed, but I found remaining issues that should be addressed before merge.
I left inline comments for the concrete findings. A few include GitHub suggestions that can be applied directly; the duplicate-header and streaming/body-cap items need a slightly broader runtime/adapter change.
CI / Existing Reviews
gh pr checks 735 reports all checks passing: fmt, Rust tests, Spin native + wasm32-wasip1 check/build/test, Axum, Cloudflare, cross-adapter parity, vitest, docs/TS formatting, integration, and browser tests. I avoided re-opening resolved prior review threads except where the current head still leaves a distinct issue.
| builder.body(body_bytes); | ||
| let spin_request = builder.build(); | ||
|
|
||
| let spin_response: spin_sdk::http::Response = |
There was a problem hiding this comment.
P1 / High — Spin outbound responses collapse duplicate headers.
spin_sdk::http::Response stores headers in a map in the locked Spin SDK, and headers() therefore exposes only one value per header name. That means upstream publisher/proxy responses with repeated headers — especially multiple Set-Cookie values — are collapsed before Trusted Server core can process or forward them. The final response path has the same risk through the high-level Spin response conversion reached from edgezero_adapter_spin::run_app in src/lib.rs.
This is a correctness issue for Trusted Server because publisher cookies, consent cookies, and EC/session cookies often depend on preserving repeated Set-Cookie headers.
Suggested fix: use the lower-level Spin/WASI response APIs for both outbound fetches and final response conversion so header entries are preserved as a list (e.g. IncomingResponse/Fields::entries() and OutgoingResponse/Fields::from_list), or patch/update edgezero-adapter-spin to preserve duplicate header values end-to-end.
There was a problem hiding this comment.
Acknowledged — deferring to an upstream follow-up rather than a partial in-tree fix. The outbound-fetch half is in our platform.rs, but the final response→client conversion runs in edgezero_adapter_spin::run_app (the pinned external stackpop/edgezero adapter), whose high-level OutgoingResponse is map-backed. Preserving duplicate Set-Cookie end-to-end requires the lower-level WASI Fields::entries()/from_list on both sides, including a patch + repin of edgezero-adapter-spin. This is the same situation as the already-tracked Cloudflare Set-Cookie collapse, so handling it as a shared upstream follow-up. Can file a tracking issue if you want it linked here.
| // scheme+host lets those handlers validate the signed target instead of failing | ||
| // with "Invalid URL". | ||
| pub(crate) fn normalize_spin_request(req: &mut Request) { | ||
| sanitize_forwarded_headers(req); |
There was a problem hiding this comment.
P1 / High — Client-supplied X-Forwarded-For still reaches integrations/proxy targets.
This normalization re-derives a trusted client IP from Spin's synthetic spin-client-addr, but it leaves any client-provided x-forwarded-for header on the request. Shared proxy code forwards that header when present, and Prebid copies it into its outbound request headers, so a Spin client can spoof the IP seen by downstream services while auction device.ip uses the trusted runtime address.
Please strip inbound x-forwarded-for here and either omit it or reinsert exactly one trusted value derived from trusted_client_addr. A minimal safe version is to drop it:
| sanitize_forwarded_headers(req); | |
| sanitize_forwarded_headers(req); | |
| req.headers_mut().remove("x-forwarded-for"); |
Also add a regression test with a spoofed X-Forwarded-For and a trusted spin-client-addr so this cannot regress.
There was a problem hiding this comment.
Fixed in 59c4977. normalize_spin_request now strips inbound x-forwarded-for — Spin exposes no trusted forwarded-for chain and the trusted client IP comes from spin-client-addr. Added regression test normalize_spin_request_strips_client_supplied_x_forwarded_for (spoofed XFF + trusted spin-client-addr).
| // Enforce allowed_domains on the redirect target before issuing a new signed URL. | ||
| if let Ok(parsed_tsurl) = url::Url::parse(&tsurl) { | ||
| if let Some(host) = parsed_tsurl.host_str() { | ||
| if !redirect_is_permitted(&settings.proxy.allowed_domains, host) { | ||
| return Err(Report::new(TrustedServerError::Proxy { | ||
| message: format!( | ||
| "redirect to `{host}` blocked: host not in proxy allowed_domains" | ||
| ), | ||
| })); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2 / Medium — proxy-rebuild applies the proxy SSRF allowlist to click redirects only during rebuild.
proxy.allowed_domains is documented as governing first-party proxy redirect-chain SSRF control, not advertiser click redirects. /first-party/click itself will still redirect any valid signed click URL, but this rebuild path rejects the same signed target if its host is outside proxy.allowed_domains. That creates a cross-adapter behavior regression for deployments that restrict server-side proxy fetch destinations while still allowing normal signed click redirects.
Unless we introduce and consistently enforce a separate click-target allowlist in both /first-party/click and rebuild, the rebuild path should validate the original tstoken and reserved parameters without applying the proxy-fetch allowlist:
| // Enforce allowed_domains on the redirect target before issuing a new signed URL. | |
| if let Ok(parsed_tsurl) = url::Url::parse(&tsurl) { | |
| if let Some(host) = parsed_tsurl.host_str() { | |
| if !redirect_is_permitted(&settings.proxy.allowed_domains, host) { | |
| return Err(Report::new(TrustedServerError::Proxy { | |
| message: format!( | |
| "redirect to `{host}` blocked: host not in proxy allowed_domains" | |
| ), | |
| })); | |
| } | |
| } | |
| // Rebuild validates the original signed click URL above. Do not apply | |
| // proxy.allowed_domains here: that setting protects server-side proxy fetch | |
| // redirect chains, while click redirect policy must stay consistent with | |
| // handle_first_party_click. |
There was a problem hiding this comment.
Fixed in b394581. Removed the proxy.allowed_domains check from handle_first_party_proxy_rebuild. That setting governs server-side proxy fetch redirect chains, not click 302s, and handle_first_party_click does not apply it — so enforcing it only during rebuild was the cross-adapter regression. The rebuild already validates the original signed click URL, and tsurl is a reserved (immutable) parameter, so the redirect host stays fixed by the validated original.
| run: cargo check-spin | ||
|
|
||
| - name: Build Spin adapter release WASM | ||
| run: cargo build --package trusted-server-adapter-spin --target wasm32-wasip1 --features spin --release |
There was a problem hiding this comment.
P2 / Medium — The Spin release build artifact is compiled with placeholder secrets.
The Spin release build currently runs plain Cargo, so the generated WASM embeds the default trusted-server.toml placeholders (for example publisher.proxy_secret = "change-me-proxy-secret"). Runtime startup rejects those placeholders and falls back to the generic 503 router, so CI proves compilation but not that the documented Spin artifact can boot with usable settings. The spin.toml build command has the same operational gap and should be documented/sourced similarly.
Please mirror the Fastly release-build overrides for this Spin artifact build, e.g.:
| run: cargo build --package trusted-server-adapter-spin --target wasm32-wasip1 --features spin --release | |
| env: | |
| TRUSTED_SERVER__PUBLISHER__ORIGIN_URL: http://127.0.0.1:8080 | |
| TRUSTED_SERVER__PUBLISHER__PROXY_SECRET: integration-test-proxy-secret | |
| TRUSTED_SERVER__EC__PASSPHRASE: integration-test-ec-secret-padded-32 | |
| TRUSTED_SERVER__PROXY__CERTIFICATE_CHECK: "false" | |
| run: cargo build --package trusted-server-adapter-spin --target wasm32-wasip1 --features spin --release |
There was a problem hiding this comment.
Fixed in 76f2207. The Spin release-build step now carries the same PUBLISHER__ORIGIN_URL / PUBLISHER__PROXY_SECRET / EC__PASSPHRASE / PROXY__CERTIFICATE_CHECK overrides as the Fastly release build, so the artifact embeds usable settings and boots instead of falling back to the 503 router.
| .headers() | ||
| .map(|(name, value)| (name.to_string(), value.as_bytes().to_vec())) | ||
| .collect(); | ||
| let body = spin_response.into_body(); |
There was a problem hiding this comment.
P2 / Medium — The 8 MiB response ceiling is enforced after the body is already materialized.
spin_sdk::http::send returns a fully buffered body here, and only after that does apply_spin_response_policy check the size. A large origin/ad-server response can therefore still allocate in the Wasm heap before the adapter returns a typed error, so the current limit does not fully protect the Spin component from OOM/resource exhaustion.
Suggested fix: switch the outbound response path to collect the Spin incoming response body incrementally with a cap (or configure a host-side body cap) before copying the full response into guest memory, then keep the decompression-size limit on the bounded buffer.
There was a problem hiding this comment.
Acknowledged — deferring with the duplicate-header item above, since both need the same lower-level Spin path. spin_sdk::http::send returns a fully buffered body, so the 8 MiB cap currently runs post-materialization. A complete fix means switching the outbound path to incremental IncomingResponse streaming with a bounded collector before copying into guest memory — a streaming rework of the Spin HTTP client. The existing cap still bounds the decompression buffer in the meantime.
…ification' into feature/edgezero-pr19-spin-adapter # Conflicts: # .github/workflows/format.yml # crates/trusted-server-adapter-cloudflare/build.sh # crates/trusted-server-adapter-fastly/src/platform.rs # crates/trusted-server-adapter-fastly/src/route_tests.rs
The Axum and Cloudflare integration-test builds set the removed TRUSTED_SERVER__EDGE_COOKIE__SECRET_KEY, which no settings key reads, so those bundles embedded the committed local-dev passphrase instead of the integration passphrase and could not catch cross-runtime EC regressions. Use the real TRUSTED_SERVER__EC__PASSPHRASE (32-byte value) the Fastly build already uses. Also give the Spin release-build step the same publisher/EC/proxy overrides so the artifact embeds usable settings and boots instead of falling back to the generic 503 router on placeholder-secret rejection.
normalize_spin_request re-derived a trusted client IP from the synthetic spin-client-addr but left any inbound x-forwarded-for in place. Shared proxy code forwards that header to origins and Prebid copies it into its outbound request, so a Spin client could spoof the IP seen by downstream services while auction device.ip used the trusted runtime address. Strip x-forwarded-for during normalization — Spin exposes no trusted forwarded-for chain. Adds a regression test with a spoofed X-Forwarded-For and a trusted spin-client-addr.
handle_first_party_proxy_rebuild rejected click targets whose host fell outside proxy.allowed_domains, but proxy.allowed_domains governs server-side proxy fetch redirect chains, not advertiser click 302s — and handle_first_party_click itself redirects any valid signed target without consulting it. Applying it only during rebuild was a cross-adapter regression for deployments that restrict proxy fetch destinations while allowing normal signed click redirects. The original signed click URL (including the immutable reserved tsurl parameter) is already validated in the rebuild, so the redirect host is fixed by the validated original.
Summary
trusted-server-adapter-spin— the Fermyon Spin entry point (wasm32-wasip1component) usingedgezero_adapter_spin::run_appwith Spin-backedRuntimeServices(config store, KV, secrets, buffered proxy client with gzip/br decompression)ce6bcf74(addsedgezero-adapter-spin), Fastly SDK to0.12, Cloudflareworkerto0.8, and Viceroy to0.16.5to resolve bot-analysis hostcall breakage introduced by the SDK bumpChanges
crates/trusted-server-adapter-spin/edgezero.toml,spin.toml, platform runtime services, route/auth smoke tests, EdgeZero manifest validation testCargo.toml/Cargo.locktrusted-server-adapter-spin; bump EdgeZero rev,fastly 0.12,log-fastly 0.12,worker 0.8crates/integration-tests/Cargo.toml/tests/parity.rs.cargo/config.tomltest-spin,check-spin,clippy-spin-native,clippy-spin-wasmaliases.github/workflows/test.ymlwasm32-wasip1CI jobs.github/workflows/format.yml.github/actions/setup-integration-test-env/action.yml0.16.5.tool-versions0.16.5CLAUDE.mdcrates/trusted-server-adapter-fastly/src/{main,platform,route_tests}.rsfastly 0.12API surfacecrates/trusted-server-adapter-cloudflare/Cargo.tomlworkerto0.8docs/superpowers/specs/2026-03-19-edgezero-migration-design.mdCloses
Closes #732
Known MVP limits
cargo build --workspaceintentionally fails (mixedwasm32-wasip1/wasm32-unknown-unknowntargets); target-matched aliases are the correct verification pathTest plan
cargo fmt --all -- --checkcargo check(native)cargo check-spin(wasm32-wasip1)cargo build --package trusted-server-adapter-spin --target wasm32-wasip1 --features spin --releasecargo test-spin(route/auth smoke tests, native host)cargo test-fastly(Viceroy 0.16.5)cargo test-axumcargo test-cloudflarecargo test --manifest-path crates/integration-tests/Cargo.toml --test parity(Fastly + Cloudflare + Spin)cargo clippy-fastly,clippy-axum,clippy-cloudflare,clippy-spin-native,clippy-spin-wasmcargo clippy --manifest-path crates/integration-tests/Cargo.toml -- -D warningscargo check -p trusted-server-adapter-fastly --target wasm32-wasip1cargo check -p trusted-server-adapter-cloudflare --target wasm32-unknown-unknown --features cloudflaregit diff --checkspin up --from crates/trusted-server-adapter-spin(requires local Spin CLI install)Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)