State<T> + nested/array #[secret] + Fastly dispatch fidelity + app! app-state injection#306
State<T> + nested/array #[secret] + Fastly dispatch fidelity + app! app-state injection#306aram356 wants to merge 80 commits into
Conversation
…nfig/app-demo test coverage, workspace commands
…dy-mode rename, Internal->500, scaffold CI checks
… roadmap update, macro crate test
Match the edgezero-cli precedent and the workspace-dependency convention. Adds quote to [workspace.dependencies].
…veat - emit `include_bytes!` const in app! output so edits to edgezero.toml trigger a Cargo rebuild without requiring a .rs change - strengthen middleware_sees_introspection_data to assert manifest_json presence and non-empty route list, mirroring dispatch test - add content-type assertion to routes_lists_registered_routes - add security caveat to routing.md Introspection Routes section noting endpoints are unauthenticated and environment.variables values are emitted
… no edgezero.toml change)
… atomic #[action(manifest|routes)] TDD tasks
…lity inject); fix plan single-filter cmd + probe response
…ne; cargo test in Task 10a
…rrying handler struct
Design spec for two upstream edgezero-core/edgezero-macros primitives: - State<T> extractor + RouterBuilder::with_state for app-owned shared state - nested/array #[secret] support via path-qualified SecretField metadata Filed under docs/superpowers/specs/. Includes a maintainer-review appendix (§8) verifying every current-mechanics claim against origin/main @ 42843b1 and folding in the corrections found: http-facade use in the router plumbing, the inaccurate lib.rs re-export step, the omitted validate_excluding_secrets consumer (needs nested-ValidationErrors navigation, not a rename), the per-struct guard-enforcement rewording, and B-3 being forced to the secret_fields() fn lowering.
…ts, crate-root app-demo state, real route TOML)
…)] opt-in; check off completed plan boxes
…sions; model cached app_state (OnceLock) Finding 1: app!(state = <expr>) runs the state expression each time build_router() is called (per request on Fastly), so the demo's app_state() now caches via OnceLock and the guide warns to build heavy state once + hand out Arc clones. Finding 2: handlers.md gains the app!(state = ...) macro path; fastly.md documents run_app_with_request_extensions (JA4/H2/client-IP hook) and owns_logging opt-out.
…tate - handlers.md: app! allows only one state=<expr> (macro rejects duplicates); use an aggregate app-state struct for multiple values. Fix stray '>' typo. - fastly.md: get_tls_ja4() returns Option<&str>; Ja4 wraps String -> .to_owned(). - p0d plan: model the cached OnceLock<Arc<_>> app_state (was fresh Arc per call).
… model - cli-reference.md / cli-walkthrough.md: bundled `edgezero config push` errors (no raw push); typed push writes ONE BlobEnvelope with every field verbatim, including #[secret] key NAMES — nothing is flattened or stripped. Correct the per-adapter mechanics to one (key, envelope_json) entry and fix the stale Fastly command (config-store-entry update --upsert --stdin). - blob-app-config-migration.md: C::SECRET_FIELDS -> C::secret_fields(). - fastly.md: client IP is carried via FastlyRequestContext (only JA4/H2 need the raw hook); fix import path to context::FastlyRequestContext.
- axum.md: the local file is { "<store_id>": "<BlobEnvelope JSON>" }, not a
flat per-leaf map; secret key names are preserved (never stripped/resolved).
Show the AppConfig<C> extractor as the reader; drop the flat store.get example.
- cli-walkthrough.md: Spin secret note + env-overlay example — push preserves
secret key NAMES and writes the nested blob, it does not strip secrets.
- axum config_store.rs: module + method doc comments describe the envelope
entry (outer map still string->string; typed push writes one envelope string).
- spin.md / manifest-store-migration.md: use <your-cli> config push (bundled edgezero config push errors by design; typed push runs from the app CLI). - axum.md + axum config_store.rs: the outer blob key is the *selected* config key — defaults to the logical store id, overridable with --key. - axum config_store.rs: add generated_at to the BlobEnvelope example (required field), matching the fuller shape in the public Axum docs.
… selector - cli-reference.md: add the shipped `config diff` command (typed-only; --format unified/json/structured, --exit-code, --key/--store/--local/--no-env/etc.), and document the config push flags that were missing: --key, --no-diff, --yes/-y. - manifest-store-migration.md: EDGEZERO__STORES__CONFIG__<ID>__KEY is a concrete runtime blob selector (staging/canary), not free-form tuning — dedicated table row + note; disambiguate the free-form row's metavariable to <SUFFIX>. - configuration.md: mention the __KEY selector next to __NAME so operators find it.
dry-run needs a remote read-back to show the diff; Spin Cloud read-back is Unsupported, so the push errors there. Point readers at --local or --yes, matching the runtime error message. (cli-reference.md + blob migration guide.)
…-secrets-spec-review # Conflicts: # crates/edgezero-cli/src/bin/check_no_nested_app_config.rs # crates/edgezero-core/src/handler.rs # crates/edgezero-core/src/router.rs # crates/edgezero-macros/src/app.rs
CI resolves prettier ^3.4.2 to 3.8.3, which normalizes list spacing more strictly than the 3.4.2 I formatted with locally — remove the stray blank line in the Spin push bullet so format-docs passes.
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Four well-separated upstream primitives (State<T>, nested/array #[secret], Fastly dispatch fidelity, app! app-state) with no blocking findings — implementation is solid and the test coverage is exceptional. All findings below are non-blocking suggestions.
😃 Praise
- Test discipline is exceptional: trybuild UI tests with exact stderr, a true end-to-end nested-secret test through
AppConfig<C>::from_requestwith live in-memory stores, an interleaved-poll no-bleed test forState, and Set-Cookie multi-value regression tests in both conversion directions. - The app-demo
app_state()doc comment warning about per-requestbuild_router()on Fastly (theOnceLockpattern) is exactly the operational note downstream users need.
Findings
Non-blocking (body-level)
- ♻️ Three parallel path-walkers: path navigation is implemented three times — the JSON walk (
crates/edgezero-core/src/extractor.rs,resolve_secret_field), the TOML walk (crates/edgezero-cli/src/config.rs,collect_secret_leaves), and the prune walk (crates/edgezero-core/src/app_config.rs,prune_secret_leaf). They are already slightly divergent on optional/array handling (see inline comment on the optional-flag conflation). Full unification across three value types would be over-engineering, but a shared conformance test table (the same path/shape fixtures run against all three walkers) would pin them together cheaply. - 🌱 No CLI test for an optional (
Option<String>) secret leaf:collect_secret_leaves' optional arm has no direct test — the runtime walk covers both null and absent leaves, the CLI covers neither. Oneconfig validatetest with an absent optional leaf closes the gap. - 🌱 Per-request
Vecallocation insecret_fields(): this was aconst; it now allocates per call, andsecret_walkcalls it per request. Negligible next to secret-store I/O today; if it ever shows up in profiles, astd::sync::OnceLock<Vec<SecretField>>in the emitted impl fixes it without an API change. - 📝
TypedSecretEntry.field_name&str→Stringis a public API break (crates/edgezero-adapter/src/registry.rs): fine pre-1.0, and#[non_exhaustive]+ the genericnew()keep call sites compiling. Noting for changelog awareness.
CI Status
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -D warnings): PASS - tests (
--workspace --all-targets): PASS
Rename to remove P0-C/P0-D/P0-CD phase tags from filenames: - specs/2026-07-03-edgezero-p0cd-fastly-dispatch-and-appstate-design.md -> ...-fastly-dispatch-and-appstate-design.md - plans/2026-07-04-edgezero-p0c-fastly-dispatch-fidelity.md -> ...-fastly-dispatch-fidelity.md - plans/2026-07-04-edgezero-p0d-app-macro-state.md -> ...-app-macro-state.md Update the plan-to-plan and plan-to-spec path references accordingly.
…-cycles Address PR review findings (all non-blocking): - secret_walk / collect_secret_leaves: `field.optional` reflects only the LEAF (`Option<String>`); a missing/null intermediate object/array is now a ConfigOutOfDate error in BOTH the runtime and CLI walkers, not a silent skip that degraded to a vaguer serde error. Re-aligns the two walkers. - secret_walk: skip `StoreRef` fields at the loop top (they hold a store id, not a key) — avoids a wasted full-path descent before discarding. - derive: reject DIRECT self-cyclic `#[app_config(nested)]` at compile time (new trybuild UI fixture). Mutual A<->B cycles are undetectable by a proc-macro (one type at a time) — documented. - check_no_nested_app_config footer + configuration.md: opt-in supports `T` / `Vec<T>` only (not Option/Box wrappers); document required-intermediate and cyclic-nesting semantics. - tests: optional-leaf-absent + missing-required-intermediate for both walkers.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Requesting changes for one secret-redaction issue in runtime AppConfig validation. I also left one medium Fastly proxy fidelity comment. CI is green, and the earlier nested-secret review feedback appears addressed, but the secret-value exposure path should be fixed before merge.
… port (P2)
- P1 (security): after secret_walk resolves #[secret] fields to real values,
cfg.validate() runs over the resolved struct. validator error params echo the
rejected value, and that string flowed into EdgeError::ConfigOutOfDate ->
HTTP response/logs, leaking the secret. Redact to the field path only
('app config failed validation for field <path>'). Regression test asserts a
failing resolved secret appears in neither the error message nor the response.
- P2 (fastly proxy): the synthesized Host header was built from uri.host()
only, dropping an explicit target port. Use uri.authority() (host:port) so
http://example.com:8080 sends 'Host: example.com:8080'; backend/TLS SNI still
key off uri.host().
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Approving per request. CI is green and the previously requested redaction and Host-port fixes are present. I left inline suggestions for the remaining follow-up issues I found.
| let host_header = match uri.authority() { | ||
| Some(authority) => Some(authority.as_str()), | ||
| None => uri.host(), | ||
| }; | ||
| if let Some(value) = host_header { | ||
| fastly_request.set_header("Host", value); | ||
| } |
There was a problem hiding this comment.
Fastly proxy Host header can include URI userinfo
authority() includes userinfo, so a target like http://user:pass@example.com:8080/path would send Host: user:pass@example.com:8080. That is not valid Host semantics and can leak credentials to the origin. Build the value from host() plus the optional port instead.
| let host_header = match uri.authority() { | |
| Some(authority) => Some(authority.as_str()), | |
| None => uri.host(), | |
| }; | |
| if let Some(value) = host_header { | |
| fastly_request.set_header("Host", value); | |
| } | |
| if let Some(host) = uri.host() { | |
| let value = match uri.port() { | |
| Some(port) => format!("{host}:{}", port.as_str()), | |
| None => host.to_owned(), | |
| }; | |
| fastly_request.set_header("Host", value.as_str()); | |
| } |
There was a problem hiding this comment.
Fixed in 04f9e77. build_fastly_request now composes Host from uri.host() plus uri.port() (when present), never uri.authority() — so a http://user:pass@example.com:8080/... target sends Host: example.com:8080 with no userinfo leaked. Backend + TLS SNI still key off uri.host().
| // (Mutual cycles A<->B are invisible to a proc-macro — it only ever sees one | ||
| // type at a time — so those remain a documented limitation.) | ||
| for descriptor in &nested_descriptors { | ||
| if type_is_bare_ident(descriptor.child_ty, &input.ident) { |
There was a problem hiding this comment.
Qualified direct self-cycles still bypass the guard
This catches Vec<Config>, but Vec<crate::Config> still compiles and then Config::secret_fields() recursively calls itself until stack overflow. That can abort CLI validation/push/diff or trap a worker on first runtime config extraction. Please either expand the obvious self-cycle detection (Self, qualified crate::Config/module paths where resolvable) or add a runtime depth/seen-set guard around generated secret-field collection.
There was a problem hiding this comment.
Fixed in 04f9e77 with the runtime guard option. Every derived nested secret_fields() body now enters a SecretFieldsRecursionGuard (thread-local depth cap = 64) that panics with a clear message before the stack overflows. This covers exactly the cases the compile-time check cannot resolve — mutual A↔B and path-qualified Vec<crate::Config> self-references. I did not broaden the compile-time check to terminal-ident matching because that false-positives on legitimately-distinct same-named types (some_mod::Config nested in a Config); the bare single-segment self-cycle stays a compile error for fast feedback. should_panic test in tests/app_config_cycle_guard.rs.
| quote! {} | ||
| } else { | ||
| quote! { | ||
| const _: fn() = || { |
There was a problem hiding this comment.
Generic nested AppConfig children fail outside generic scope
For a generic config wrapper such as struct Wrapper<T> { #[app_config(nested)] child: T }, even with T: AppConfigRoot + AppConfigMeta, this module-scope const cannot see T and the derive fails with cannot find type T in this scope. Move the assertion into the generated impl/secret_fields() scope, or add the necessary generated where-clause bounds instead of emitting the check at module scope.
There was a problem hiding this comment.
Fixed in 04f9e77. The AppConfigRoot bound check moved from the module-scope const _ into the secret_fields() method body, where the impl generics are in scope — so struct Wrapper<T: AppConfigRoot + AppConfigMeta> { #[app_config(nested)] child: T } compiles instead of failing with "cannot find type T". The no-op __assert::<T>() call is compiled out but still enforces the bound. Added a trybuild .pass fixture (app_config_generic_nested_child.rs) that instantiates and calls Wrapper::<Child>::secret_fields().
| The default `edgezero` binary exposes the same commands but has no typed app-config | ||
| struct in scope, so it runs the **raw** `config validate` path and **cannot** run | ||
| `config push` at all (the bundled `config push` errors, pointing you at the downstream | ||
| CLI — the blob model needs the typed `AppConfig<C>`). Downstream CLIs upgrade to the | ||
| typed paths so `validator` rules, `#[secret]` / `#[secret(store_ref)]` checks, and | ||
| Spin's flat-namespace collision check all run. |
There was a problem hiding this comment.
Clarify which validators run in typed CLI paths
The implementation uses validate_excluding_secrets for typed validate/push/diff, so validators on secret leaves are deferred until runtime after the actual secret value is resolved. The docs should not imply CLI preflight runs every validator rule on #[secret] fields.
| The default `edgezero` binary exposes the same commands but has no typed app-config | |
| struct in scope, so it runs the **raw** `config validate` path and **cannot** run | |
| `config push` at all (the bundled `config push` errors, pointing you at the downstream | |
| CLI — the blob model needs the typed `AppConfig<C>`). Downstream CLIs upgrade to the | |
| typed paths so `validator` rules, `#[secret]` / `#[secret(store_ref)]` checks, and | |
| Spin's flat-namespace collision check all run. | |
| The default `edgezero` binary exposes the same commands but has no typed app-config | |
| struct in scope, so it runs the **raw** `config validate` path and **cannot** run | |
| `config push` at all (the bundled `config push` errors, pointing you at the downstream | |
| CLI — the blob model needs the typed `AppConfig<C>`). Downstream CLIs upgrade to the | |
| typed paths so non-secret `validator` rules, `#[secret]` / `#[secret(store_ref)]` | |
| checks, and Spin's flat-namespace collision check all run. Validators on secret | |
| leaves run at runtime after `AppConfig<C>` resolves the actual secret values. |
There was a problem hiding this comment.
Fixed in 04f9e77 — applied your suggestion. The walkthrough now says the typed CLI runs non-secret validator rules, the #[secret]/#[secret(store_ref)] checks, and the Spin collision check, and that validators on secret leaves run at runtime after AppConfig<C> resolves the actual values (the typed paths use validate_excluding_secrets).
…hildren
- Fastly proxy (userinfo leak): build Host from uri.host()+uri.port(), not
uri.authority() (which includes user:pass@) — no credential leak, port kept.
- Cyclic nesting the derive can't see (mutual A<->B, or path-qualified
Vec<crate::A>): a SecretFieldsRecursionGuard entered in every nested
secret_fields() body panics with a clear message instead of overflowing the
stack. Bare direct self-cycles remain a compile error. (should_panic test.)
- Generic nested children: moved the AppConfigRoot bound check from module
scope into secret_fields() so 'struct Wrapper<T>{ #[app_config(nested)] child: T }'
compiles instead of failing with 'cannot find type T'. (trybuild pass fixture.)
- Docs: cli-walkthrough clarifies typed CLI runs non-secret validators only
(secret-leaf validators run at runtime); configuration.md cycle caveat updated.
Four independent upstream primitives for the trusted-server -> EdgeZero migration. Builds on #300 (introspection routes), now merged; this PR targets
main(merged up to date).Closes #304.
Specs:
docs/superpowers/specs/2026-07-02-edgezero-state-and-nested-secrets-design.md(State<T>+ nested secrets),docs/superpowers/specs/2026-07-03-edgezero-fastly-dispatch-and-appstate-design.md(Fastly dispatch + app-state). Task-by-task plans underdocs/superpowers/plans/(all boxes checked).State<T>extractorState<T>extractor (FromRequestby type from request extensions;Deref/DerefMut/into_inner;500when unregistered).RouterBuilder::with_state<T>stores app state in a singleExtensionsbag; dispatch doesrequest.extensions_mut().extend(state.clone())after the introspection injects (last-write-wins byTypeId). Zero macro change --#[action]composesState<T>via the genericFromRequestpath.docs/guide/handlers.md"Sharing app state".Nested / array
#[secret]SecretField { kind, path: Vec<SecretPathSegment>, optional };AppConfigMeta::secret_fields() -> Vec<SecretField>(wasconst SECRET_FIELDS);dotted_path().#[app_config(nested)]opt-in recurses into sub-struct /Vec<_>fields; acceptsOption<String>; rejectsOption<String>on#[secret(store_ref)]; empty/bare#[app_config]hard-errors;AppConfigRootbound assertion; serderename/rename_allguards extended along the path.secret_walkis a path navigator (nested objects, arrays per-element, optional absent/nullskip,KeyInNamedStoresibling resolved in the innermost parent, dotted[n]errors).validate_excluding_secretspruning.config validate/push/diff;TypedSecretEntrycarries a dotted label.#[app_config(nested)].docs/guide/configuration.md"Nested and array secrets".Fastly
run_appdispatch fidelityset_header->append_headerinfrom_core_responseand the proxy response/request conversion (originSet-Cookieno longer collapses).Hooks::owns_logging()-- platform-neutral opt-out gated in all four adapters'run_app;app!(owns_logging = <bool>)macro argument.run_app_with_request_extensions-- a generic pre-dispatch hook: an app closure reads raw-fastly::Requestsignals (e.g. JA4 / H2 / client IP -- the app supplies the closure and its own type) into a scratchExtensionsbefore conversion, merged into the core request; visible to middleware and theState/extractor layer.app!app-state injectionapp!(state = <expr>)makes the macro-generatedbuild_router()call.with_state(<expr>), reusing theState<T>dispatch injection. Macro-only -- no adapter orHookschange.app-demogains astate = crate::app_state()+State<Arc<DemoState>>handler with an end-to-end test.Tests / CI gates
cargo fmt,cargo clippy --workspace --all-targets --all-features -D warnings,cargo test --workspace --all-targets, feature check (fastly cloudflare spin), spinwasm32-wasip2, the nested-AppConfig audit, app-demo (own workspace) tests + clippy, and the Fastly adapter's wasm32-wasip1 + Viceroy tests (headers + request-extension hook). Green locally and on CI after mergingmainup to date.Notes
cargo test.#[expect]are documenteddead_codeon#[derive(AppConfig)]test fixtures.