Conversation
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Thanks for the cross-adapter config store work — the overall direction looks good. I’m requesting changes for one high-severity issue plus follow-ups.
Findings
-
High — Axum config store can expose full process environment (secret leakage risk)
crates/edgezero-adapter-axum/src/config_store.rs:12crates/edgezero-adapter-axum/src/config_store.rs:37examples/app-demo/crates/app-demo-core/src/handlers.rs:119AxumConfigStore::from_envsnapshots all env vars, so any handler pattern that accepts user-controlled keys can accidentally expose unrelated secrets.- Requested fix: replace blanket
std::env::vars()capture with an explicit allowlist (manifest-declared keys only), and avoid arbitrary key-lookup patterns in examples intended for production-like usage.
-
Medium — Adapter override key casing is inconsistent across resolution paths
crates/edgezero-core/src/manifest.rs:352crates/edgezero-macros/src/app.rs:120crates/edgezero-core/src/app.rs:59- Mixed-case adapter keys can work in one path and fail in another.
- Requested fix: normalize keys at parse/finalize time (or enforce lowercase with validation) and add a mixed-case adapter-key test.
-
Low — Missing positive-path injection coverage in adapter tests
crates/edgezero-adapter-fastly/tests/contract.rs:17crates/edgezero-adapter-cloudflare/tests/contract.rs:188- Please add success-path assertions that config store injection/retrieval works when bindings are present.
Once the high-severity item is addressed, this should be in good shape.
There was a problem hiding this comment.
Review: Config Store Feature
Overall this is a well-structured feature that follows the existing adapter pattern cleanly. The core trait, contract test macro, per-adapter implementations, and manifest/macro integration are all thoughtfully designed. Test coverage is solid across all three adapters and the docs are thorough.
That said, I found issues across four areas that should be addressed before merge — one high-severity security concern, two medium design issues, and one CI coverage gap.
Summary
| Severity | Finding |
|---|---|
| High | Axum config-store exposes entire process environment (secret leakage risk) |
| Medium | Case handling for adapter overrides is inconsistent between manifest and metadata paths |
| Medium | dispatch() bypasses config-store injection, diverging from run_app behavior |
| Medium-Low | New WASM adapter code paths are weakly exercised in CI |
See inline comments for details and suggested improvements.
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Follow-up review complete. No new issues were found in the current changeset, and previously noted concerns appear addressed.
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Solid config store abstraction with good test coverage across all three adapters. The contract test macro and error mapping are well-designed. Main concerns are around the public dispatch API silently dropping KV handles, and a dual config resolution path that needs clarification.
Findings
Blocking
- 🔧
dispatch_with_config/dispatch_with_config_handlesilently drop KV: Both Fastly and Cloudflare versions passNonefor KV. Users migrating fromdispatchordispatch_with_kvto a config-aware path will lose KV access with no warning. (cloudflarerequest.rs:106-130, fastlyrequest.rs:78-103) - ❓ Dual config name resolution in
run_app: Both adapters checkA::config_store()(compile-time) then fall back to runtime manifest parsing. Since both derive from the sameedgezero.toml, when would these diverge? Needs documentation or removal of dead path. (cloudflarelib.rs:86-94, fastlylib.rs:97-107)
Non-blocking
- 🤔
CloudflareConfigStore::new()silent fallback: A binding typo gives an empty store with only alog::warn. Consider renaming tonew_or_empty()or removing in favor oftry_newonly. - ♻️ Duplicate bounded dedup caches: Fastly
RecentStringSetand CloudflareConfigCacheare structurally identical. Candidate for a shared core utility. - ⛏
_ctxprefix on used variable:cloudflare/tests/contract.rs:55— underscore implies unused but variable is read. - 🏕
wasm-bindgen-testin[dependencies]: Pre-existing but file was touched — test-only crate shouldn't be in production deps. - 🌱 Consider a
ConfigStoreextractor: Handlers callctx.config_store()manually; aConfigStore(store)extractor (likeKv) would complete the pattern.
📌 Out of Scope
wasm-bindgen-testdep placement should be fixed in a separate cleanup PR- Bounded cache dedup is a candidate for future core utility extraction
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS
| .stores | ||
| .config | ||
| .as_ref() | ||
| .map(|cfg| cfg.config_store_name(edgezero_core::app::CLOUDFLARE_ADAPTER)) |
There was a problem hiding this comment.
❓ Dual config name resolution — when do paths diverge? This first checks A::config_store() (compile-time metadata from the #[app] macro), then falls back to parsing manifest.stores.config at runtime. Since both derive from the same edgezero.toml, these should always agree.
When would A::config_store() return None but the manifest has [stores.config]? Presumably when someone implements Hooks manually without the macro. If so, this should be documented. If not, the fallback is dead code.
Same pattern in edgezero-adapter-fastly/src/lib.rs:97-107.
There was a problem hiding this comment.
The fallback is not dead code: it fires when a caller implements Hooks manually without the #[app] macro, in which case A::config_store() returns None while [stores.config] may still be present in the manifest. Added a comment explaining this in both adapters.
dispatch_with_config and dispatch_with_config_handle in both the Cloudflare and Fastly adapters were passing None for the KV handle, silently dropping KV access for callers on those paths. Both now resolve the default KV binding/store (non-required) alongside the config store. Additional cleanup from review: - Document why run_app has two config-name resolution paths (macro-generated vs. manual Hooks impls) - Rename CloudflareConfigStore::new() to new_or_empty() to make the silent fallback-to-empty behavior explicit - Fix _ctx prefix on an actively-read variable in cloudflare contract tests - Move wasm-bindgen-test to [dev-dependencies]
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Well-structured, well-tested portable ConfigStore abstraction with implementations for Fastly, Cloudflare, and Axum. The contract test macro, security boundaries (env-var allowlisting, demo handler key allowlist), and deprecation strategy are all strong. CI passes cleanly. Two blocking issues around a silent behavioral change to ServiceUnavailable formatting and a logic bug in warning deduplication.
😃 Praise
- Contract test macro (
config_store_contract_tests!) with configurable#[$test_attr]is excellent — enableswasm_bindgen_testfor Cloudflare while others use#[test] AxumConfigStore::from_envonly reads declared keys — strong security boundary preventing the config store from becoming an env-var oracle- Demo handler allowlist (
ALLOWED_CONFIG_KEYS) — great documentation-by-example of the recommended security practice - Deprecation strategy —
dispatch()deprecated with clear migration path viadispatch_rawand new dispatch variants - CI additions — explicit
cloudflare-wasm-testsandfastly-wasm-testsjobs with properwasm-bindgen-cliversion resolution
Findings
Blocking
- 🔧
ServiceUnavailableformat string change:#[error("service unavailable: {message}")]→#[error("{message}")]removes the prefix for ALL callers, not just config store — silent behavioral change (crates/edgezero-core/src/error.rs:22) - 🔧
RecentStringSet::insertlogic bug: returnstruewithout tracking the key whenlimit == 0, defeating dedup (crates/edgezero-adapter-fastly/src/request.rs:227)
Non-blocking
- 🤔
contract_empty_key_returns_nonemay not match real Fastly behavior — realtry_get("")returnsKeyInvaliderror, notOk(None)(crates/edgezero-core/src/config_store.rs:168) - 🤔 Dispatch function proliferation — 7 dispatch-related functions per adapter; consider an options struct pattern as store types grow
- 🤔 Cloudflare
ConfigCachecachesNonefor invalid JSON permanently — correct but should document why caching failure is safe (isolate immutability) - ♻️ Duplicated bounded LRU pattern —
RecentStringSet(Fastly) andConfigCache(Cloudflare) implement the same HashMap+VecDeque eviction; extract if it appears a third time - ⛏
warned_store_cache()wrapper — function wrapping a static is unnecessary indirection; could inline inwarn_missing_store_once
📌 Out of Scope
- ❓ Spin adapter missing — PR title says "across all adapters" but Spin is excluded from
SUPPORTED_CONFIG_STORE_ADAPTERS. Consider adjusting title or creating a follow-up issue - 🌱 Spin config store support —
spin_sdk::variables::get()maps well to this abstraction; worth a follow-up issue - 📝 Cloudflare JSON-in-TOML approach — creative workaround for platform binding-name limitations; DX could be smoothed by CLI tooling generating JSON from TOML defaults
CI Status
- fmt: ✅ PASS
- clippy: ✅ PASS
- tests: ✅ PASS
- Revert #[error("{message}")] back to #[error("service unavailable: {message}")]
to restore the Display/to_string() prefix for all ServiceUnavailable callers
- Fix RecentStringSet::insert: limit==0 path returned true without tracking
the key, defeating dedup; always insert into keys/order, evict only when
limit > 0
- Document why caching None is safe in CloudflareConfigStore lookup_cached
(Cloudflare bindings are immutable within an isolate lifetime)
- Inline warned_store_cache() static into warn_missing_store_once, matching
the pattern used by warn_missing_kv_store_once
aram356
left a comment
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
This PR introduces a well-designed portable ConfigStore abstraction with implementations across Axum, Fastly, and Cloudflare adapters. The overall architecture is clean, follows established project patterns, and has solid test coverage. However, it needs a rebase onto current main to pick up the Spin adapter (#166), and there are a few gaps in CI placement and test coverage.
Praise
- 😃 Synchronous
ConfigStoretrait is the right call for all current backends — avoids unnecessary async in WASM - 😃 Contract test macro (
config_store_contract_tests!) with configurable test attribute is a great reusable pattern - 😃 Axum
from_lookupdependency injection is textbook testability design - 😃 Consistent
resolve → inject → dispatchflow across all three adapters makes the codebase easy to navigate - 😃 Cloudflare module-level docs excellently explain the JSON-in-string-binding approach with concrete examples
- 😃 Two-path config resolution comments in Fastly adapter prevent future confusion
- 😃 Feature gating (
serde_jsonbehindcloudflarefeature) is correct - 😃 Demo app handler with allowlist check, graceful degradation, and proper error propagation
- 😃 Smoke test script is well-structured with cleanup trap, readiness polling, and pass/fail summary
Findings
Blocking
- 🔧 Spin adapter missing throughout:
SUPPORTED_CONFIG_STORE_ADAPTERSinmanifest.rs:57omits"spin", andSPIN_ADAPTERconstant is missing fromapp.rs. Branch needs rebase onto main (#166). CI gate 4 (cargo check --features "fastly cloudflare spin") fails. (see inline comments) - 🔧 Cloudflare wasm check misplaced in CI:
.github/workflows/test.yml:170— the stepcargo check -p edgezero-adapter-cloudflare --features cloudflare --target wasm32-unknown-unknownis in thefastly-wasm-testsjob instead ofcloudflare-wasm-tests. If the Fastly job fails early, this check never runs. This also forces the Fastly job to installwasm32-unknown-unknownunnecessarily. Fix: Move this step to thecloudflare-wasm-testsjob. - 🔧 Missing test for store-level key miss:
handlers.rs:458—config_get_returns_404_when_key_missingtests the allowlist, notstore.get()returningNone. The store-miss code path (lines 162-165) has zero test coverage. (see inline comment)
Non-blocking
- ❓ Axum skips
A::config_store()hooks path (dev_server.rs:219): Fastly and Cloudflare use two-path resolution (compile-time hooks → manifest fallback), Axum only reads the manifest. Cross-adapter behavioral inconsistency. (see inline comment) - 🤔
contract_empty_key_returns_nonemay not hold on all backends (config_store.rs:168): Fastly Config Store SDK may error on empty keys rather than returningNone. (see inline comment) - 🤔 No convenience re-exports for config_store types (
lib.rs:6):key_value_storegetspub usere-exports butconfig_storedoesn't. (see inline comment) - 🤔
new_or_emptysilently swallows missing bindings (cloudflare/config_store.rs:37): Returns empty store with no warning when binding is missing. (see inline comment) - ♻️ Unbounded
BTreeSetin Fastly KV warn-once (fastly/request.rs:248): Config store warn-once uses boundedRecentStringSet(64 entries) but the pre-existing KV variant uses an unboundedBTreeSet. Unify on the bounded pattern. - 🏕 Missing test for
ConfigStoreError::Internalconversion (error.rs:120):UnavailableandInvalidKeycovered,Internalnot. (see inline comment) - ⛏ Test name misleading (
handlers.rs:458):config_get_returns_404_when_key_missingtests allowlist, not store miss. Consider renaming toconfig_get_returns_404_when_key_not_in_allowlist.
Out of Scope
- 📌
RecentStringSetduplicated across Fastly and Cloudflare adapters — future refactor to share in core - 📌
serve_with_listenergrew to 5 positional params — consider struct param if a third store type is added - 📌 KV adapter keys lack the same validation as config store keys (pre-existing)
- 📌 Spin adapter config store implementation (separate PR after rebase)
CI Status
- fmt: PASS
- clippy: PASS
- tests: PASS (478 tests)
- feature check (
spin): FAIL — branch needs rebase onto main
crates/edgezero-core/src/manifest.rs
Outdated
| } | ||
|
|
||
| pub const DEFAULT_CONFIG_STORE_NAME: &str = "EDGEZERO_CONFIG"; | ||
| const SUPPORTED_CONFIG_STORE_ADAPTERS: &[&str] = &["axum", "cloudflare", "fastly"]; |
There was a problem hiding this comment.
🔧 Missing "spin" in SUPPORTED_CONFIG_STORE_ADAPTERS
The Spin adapter was merged to main in #166. Once this branch is rebased, users configuring [stores.config.adapters.spin] will get a validation error because "spin" is not in this list.
Fix:
const SUPPORTED_CONFIG_STORE_ADAPTERS: &[&str] = &["axum", "cloudflare", "fastly", "spin"];| /// Canonical adapter name for the Cloudflare adapter. | ||
| pub const CLOUDFLARE_ADAPTER: &str = "cloudflare"; | ||
| /// Canonical adapter name for the Fastly adapter. | ||
| pub const FASTLY_ADAPTER: &str = "fastly"; |
There was a problem hiding this comment.
🔧 Missing SPIN_ADAPTER constant
The Spin adapter exists on main (#166) but there is no SPIN_ADAPTER constant here. When the branch is rebased, the #[app] macro will have no constant to reference for Spin config store metadata generation.
Fix:
pub const SPIN_ADAPTER: &str = "spin";| } | ||
|
|
||
| #[test] | ||
| fn config_get_returns_404_when_key_missing() { |
There was a problem hiding this comment.
🔧 Missing test for store-level "key not found" path
This test uses "missing.key" which is NOT in ALLOWED_CONFIG_KEYS, so the 404 comes from the allowlist check (line 146), not from store.get() returning Ok(None) (lines 162-165). The store-miss code path has zero test coverage.
Fix: Add a test where the key IS in ALLOWED_CONFIG_KEYS but IS NOT in the store:
#[test]
fn config_get_returns_404_when_key_not_in_store() {
let ctx = context_with_config_key("greeting", &[("other_key", "value")]);
let response = block_on(config_get(ctx)).expect("handler ok");
assert_eq!(response.status(), StatusCode::NOT_FOUND);
}Also consider renaming this test to config_get_returns_404_when_key_not_in_allowlist to reflect what it actually tests.
| router: RouterService, | ||
| listener: tokio::net::TcpListener, | ||
| enable_ctrl_c: bool, | ||
| config_store_handle: Option<ConfigStoreHandle>, |
There was a problem hiding this comment.
❓ Axum run_app skips A::config_store() hooks path
Fastly and Cloudflare both implement two-path config resolution: check A::config_store() (compile-time metadata from #[app] macro) first, then fall back to the manifest. Axum only reads from the manifest.
This means if a user implements Hooks::config_store() manually but has no [stores.config] in the manifest, it works on Fastly/Cloudflare but is silently ignored on Axum.
Is this intentional because Axum needs defaults from the manifest? If so, a comment explaining the divergence would prevent future confusion.
| } | ||
|
|
||
| #[$test_attr] | ||
| fn contract_empty_key_returns_none() { |
There was a problem hiding this comment.
🤔 contract_empty_key_returns_none may not hold on all backends
This contract test asserts that store.get("") returns Ok(None). However, Fastly's Config Store SDK may return an error for empty keys rather than None. If that's the case, the adapter would need to swallow a real error to satisfy this contract.
Consider either removing this from the contract or relaxing the assertion to allow Ok(None) OR Err(InvalidKey).
| pub mod app; | ||
| pub mod body; | ||
| pub mod compression; | ||
| pub mod config_store; |
There was a problem hiding this comment.
🤔 No convenience re-exports for config_store types
The key_value_store module gets re-exports at line 22:
pub use key_value_store::{KvError, KvHandle, KvPage, KvStore};But config_store doesn't, so users must write edgezero_core::config_store::ConfigStore instead of edgezero_core::ConfigStore. Consider adding:
pub use config_store::{ConfigStore, ConfigStoreError, ConfigStoreHandle};| /// Returns an empty store (every key returns `None`) if the binding is absent or | ||
| /// its value is not valid JSON. Use [`Self::try_new`] when you need to distinguish | ||
| /// a missing/invalid binding from a valid but empty config. | ||
| pub fn new_or_empty(env: &Env, binding_name: &str) -> Self { |
There was a problem hiding this comment.
🤔 new_or_empty silently swallows missing bindings
This returns an empty store with no warning when the binding is missing, while try_new logs a warning. If a user misspells a binding name in wrangler.toml, this silently degrades.
Consider adding a doc comment on this method noting that missing bindings are silent, and suggesting try_new for debugging.
| ConfigStoreError::Internal { source } => EdgeError::internal(source), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🏕 Missing test for ConfigStoreError::Internal → EdgeError conversion
Tests cover Unavailable and InvalidKey mappings but not Internal. Consider adding:
#[test]
fn config_store_error_internal_maps_to_internal_server_error() {
let err = EdgeError::from(ConfigStoreError::internal(anyhow::anyhow!("boom")));
assert_eq!(err.status(), StatusCode::INTERNAL_SERVER_ERROR);
}| } | ||
| } | ||
|
|
||
| pub trait ConfigStore: Send + Sync { |
There was a problem hiding this comment.
😃 Synchronous ConfigStore trait is the right call
Making get() synchronous rather than async is correct for all current backends (Fastly Config Store, Cloudflare env bindings, and env vars are all sync). This avoids unnecessary async machinery and keeps WASM compatibility simple. The contract test macro with configurable #[$test_attr] is also a great pattern.
| Self::from_lookup(defaults, |key| std::env::var(key).ok()) | ||
| } | ||
|
|
||
| fn from_lookup<F>(defaults: impl IntoIterator<Item = (String, String)>, mut lookup: F) -> Self |
There was a problem hiding this comment.
😃 Clean from_lookup test seam
Textbook dependency injection: production code calls from_env which delegates to from_lookup with std::env::var, and tests inject a closure. The security property of only reading env vars for declared keys is a nice touch.
3 participants
Summary
ConfigStoreabstraction inedgezero-corethat lets handlers read key/value configuration at runtime without coupling to a specific platform#[app]macro and each adapter's request pipeline so handlers receive aConfigStoreHandleviaRequestContextwith no boilerplateChanges
edgezero-core/src/config_store.rsConfigStoretrait,ConfigStoreHandlewrapper, and shared contract test macroedgezero-core/src/manifest.rsConfigStoreTOML binding and adapter name resolutionedgezero-core/src/context.rsconfig_store()accessor and injection helpers toRequestContextedgezero-core/src/app.rsApp::buildhooks extended to accept config store configurationedgezero-core/src/lib.rsmanifestmoduleedgezero-adapter-axum/src/config_store.rsAxumConfigStorewith defaults supportedgezero-adapter-axum/src/service.rsConfigStoreHandleinto each request before routingedgezero-adapter-axum/src/dev_server.rsDevServerConfigedgezero-adapter-axum/src/lib.rsedgezero-adapter-fastly/src/config_store.rsFastlyConfigStorebacked by Fastly edge dictionaryedgezero-adapter-fastly/src/lib.rsedgezero-adapter-fastly/src/request.rsedgezero-adapter-fastly/tests/contract.rsedgezero-adapter-cloudflare/src/config_store.rsCloudflareConfigStorebacked byworker::Envbindingsedgezero-adapter-cloudflare/src/lib.rsedgezero-adapter-cloudflare/src/request.rsedgezero-adapter-cloudflare/tests/contract.rsedgezero-macros/src/app.rs#[app]macro generates config store setup from manifestexamples/app-demo/docs/guide/scripts/smoke_test_config.sh.gitignoreCloses
Closes #51
Test plan
cargo test --workspace --all-targetscargo clippy --workspace --all-targets --all-features -- -D warningscargo check --workspace --all-targets --features "fastly cloudflare"wasm32-wasip1(Fastly) /wasm32-unknown-unknown(Cloudflare)edgezero-cli devChecklist
{id}syntax (not:id)edgezero_core(nothttpcrate)