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.
|
|
||
| /// Create from the current process environment and manifest defaults. | ||
| pub fn from_env(defaults: impl IntoIterator<Item = (String, String)>) -> Self { | ||
| Self::new(std::env::vars(), defaults) |
There was a problem hiding this comment.
High: Secret leakage risk — from_env snapshots the entire process environment
std::env::vars() captures every env var (DATABASE_URL, AWS_SECRET_ACCESS_KEY, API_TOKEN, etc.) and makes them all queryable via ctx.config_store().get(user_input). The demo app's /config/{name} handler (see handlers.rs:119) passes user-controlled path params directly to .get(), creating a direct information disclosure vector.
The doc comment acknowledges this but dismisses it as "unlikely" because config keys use dotted names. That's insufficient — an attacker doesn't need a collision, they just need to guess DATABASE_URL.
This is especially concerning because the Axum adapter docs mention container deployment, not only local dev.
Suggested fix: default to allowlist-only — only read env vars for keys declared in the manifest's [stores.config.defaults] section:
pub fn from_env(defaults: impl IntoIterator<Item = (String, String)>) -> Self {
let defaults: HashMap<String, String> = defaults.into_iter().collect();
let env = defaults.keys()
.filter_map(|key| std::env::var(key).ok().map(|val| (key.clone(), val)))
.collect();
Self { env, defaults }
}Also add a test asserting that sensitive env vars are not readable unless explicitly declared.
| let mut server = AxumDevServer::new(router); | ||
| if let Some(cfg) = m.stores.config.as_ref() { | ||
| let defaults = cfg.config_store_defaults().clone(); | ||
| let store = AxumConfigStore::from_env(defaults); |
There was a problem hiding this comment.
This is the call site that wires from_env into the server. After fixing from_env to allowlist-only, this should continue to work unchanged — config_store_defaults() already returns exactly the right key set.
Worth noting: if someone constructs AxumConfigStore::from_env(BTreeMap::new()) directly (empty defaults), the allowlist fix would make the env layer empty too, which is the safe default.
| #[action] | ||
| pub(crate) async fn config_get(RequestContext(ctx): RequestContext) -> Result<Response, EdgeError> { | ||
| let params: ConfigParams = ctx.path()?; | ||
| match ctx.config_store().and_then(|s| s.get(¶ms.name)) { |
There was a problem hiding this comment.
This handler passes params.name (user-controlled URL path segment) directly to config_store().get(). Combined with the Axum adapter's from_env snapshotting all env vars, this is a working exploit path for env var enumeration.
Even after fixing the Axum adapter, this pattern of passing raw user input to .get() should be called out in the config-store docs as something to be cautious about — or the demo should show allowlist validation.
| /// | ||
| /// Priority: adapter override → global name → `DEFAULT_CONFIG_STORE_NAME`. | ||
| pub fn config_store_name(&self, adapter: &str) -> &str { | ||
| let adapter_lower = adapter.to_ascii_lowercase(); |
There was a problem hiding this comment.
Medium: Case normalization inconsistency between manifest and metadata paths
This method lowercases the input adapter string and does a direct BTreeMap::get() lookup, which means the map keys must already be lowercase for the lookup to succeed.
Meanwhile, ConfigStoreMetadata::name_for_adapter() in app.rs:59 uses .eq_ignore_ascii_case() — a case-insensitive comparison against static strings.
So a manifest with [stores.config.adapters.Fastly] (capital F):
- Works via the macro metadata path (case-insensitive
eq_ignore_ascii_case) - Fails silently via the runtime manifest fallback path (lowercase lookup misses the
"Fastly"key in the BTreeMap)
Suggested fix: Either:
- Normalize adapter keys to lowercase during deserialization (add a custom deserialize or post-load normalization step), or
- Make this method case-insensitive by iterating keys like the metadata path does, or
- Add a validation rule that rejects non-lowercase adapter keys at manifest load time
Option 3 is simplest and catches the problem at the earliest point.
| } | ||
|
|
||
| pub fn name_for_adapter(&self, adapter: &str) -> &'static str { | ||
| self.adapters |
There was a problem hiding this comment.
This uses eq_ignore_ascii_case — correct in isolation, but inconsistent with ManifestConfigStoreConfig::config_store_name() in manifest.rs:353 which lowercases the input and does direct map lookup. See comment there for details.
Since adapter names are compile-time constants here ("axum", "fastly", "cloudflare"), this path always works. The risk is in the manifest fallback path.
| run_app_with_logging::<A>(logging.into(), req) | ||
| let m = manifest_loader.manifest(); | ||
| let logging = m.logging_or_default(edgezero_core::app::FASTLY_ADAPTER); | ||
| let config_name = A::config_store() |
There was a problem hiding this comment.
Medium: dispatch() bypasses config-store injection
The public dispatch() function (re-exported from request.rs) does not inject a config-store handle. Only dispatch_with_config() and the run_app / run_app_with_config helpers do.
Callers using the lower-level dispatch(&app, req) API directly (as shown in the existing Fastly adapter docs prior to this PR) will silently get ctx.config_store() == None even when the manifest declares a config store. This creates a non-obvious runtime difference between run_app and manual wiring.
Also note: run_app here double-resolves the config name — first from A::config_store() (macro metadata), then falling back to m.stores.config (manifest). Since the macro generates metadata from the manifest at compile time, these always agree for app! users. The fallback only helps hand-implemented Hooks, but could silently contradict macro metadata if both exist.
Suggested fix: Either:
- Unify
dispatchanddispatch_with_configby makingdispatchacceptOption<ConfigStoreHandle>(Axum adapter already does this cleanly inEdgeZeroAxumService::call), or - Deprecate
dispatchand makedispatch_with_configthe only public entry point, or - At minimum, document the behavior gap prominently in the
dispatchdoc comment
| init_logger().expect("init cloudflare logger"); | ||
| let app = A::build_app(); | ||
| dispatch(&app, req, env, ctx).await | ||
| dispatch_app( |
There was a problem hiding this comment.
Same concern as Fastly: dispatch() (still public) does not inject config-store, while run_app does via dispatch_app. Users calling dispatch directly get silently different behavior.
Also: run_app_with_manifest is #[deprecated] but brand new in this PR (line 96). Shipping a new function as deprecated on the same PR that introduces it is unusual — if no caller exists yet, consider just not shipping it. If it's needed for migration, defer the deprecation to a follow-up.
| @@ -2,22 +2,25 @@ | |||
|
|
|||
| use bytes::Bytes; | |||
| use edgezero_adapter_cloudflare::{ | |||
There was a problem hiding this comment.
Medium-Low: New WASM adapter code paths are weakly exercised in CI
These contract tests are gated behind #[cfg(all(feature = "cloudflare", target_arch = "wasm32"))] and wasm_bindgen_test, so they only run when compiled for wasm32-unknown-unknown with wasm-bindgen-test-runner. The Fastly adapter tests similarly require wasm32-wasip1.
Current CI runs host-target tests + feature compilation checks, but the actual WASM contract tests (including the new dispatch_with_config_missing_binding_skips_injection test) are effectively skipped on host.
Core config-store logic is well-tested, but the critical adapter injection paths — where the config store handle actually gets wired into requests — are not protected in CI.
Suggested improvement: Add explicit WASM test jobs to CI:
wasm32-unknown-unknownfor Cloudflare withwasm-bindgen-test-runnerwasm32-wasip1for Fastly with Wasmtime runner
This can be a follow-up PR, but without it the new adapter code paths are only verified by manual smoke tests.
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.
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)