From d863e94606326f8a0264a84823427288f6d7ff38 Mon Sep 17 00:00:00 2001 From: Ant Stanley Date: Wed, 5 Aug 2026 08:49:38 +0200 Subject: [PATCH 1/4] docs(spec): resolve placeholders on every configuration entry point Co-Authored-By: Claude Opus 5 (1M context) --- ...esolve_config_placeholders_all_channels.md | 359 ++++++++++++++++++ 1 file changed, 359 insertions(+) create mode 100644 .specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md diff --git a/.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md b/.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md new file mode 100644 index 0000000..18dc7b2 --- /dev/null +++ b/.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md @@ -0,0 +1,359 @@ +# Change: Resolve `${VAR}` placeholders on every configuration entry point + +**Status:** Proposed · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Target:** crates/server, bindings/* (service, bindings) + +Route every configuration entry point through one shared resolve step — fail-closed `${VAR}` +placeholder resolution, `OIDC_EXCHANGE__{section}__{key}` overrides, then validation — so the +napi Node binding, the PyO3 Python binding and the `@oidc-exchange/lambda` handler stop loading +configuration in which a documented secret placeholder survives as literal text. Entry points +differ only in which *sources* they layer; nothing after the merge is duplicated. The change adds +an `oidc-exchange config check` subcommand so an operator can prove their environment satisfies a +config's placeholders before deploying it. + +--- + +## Motivation + +`crates/server/src/bootstrap.rs` has two config entry points with two pipelines. +`load_config_from_dir` (`:90-118`) layers the TOML sources, calls `resolve_placeholders` +(`:114`), deserializes, and validates. `parse_config` (`:124-128`) is two statements — +`toml::from_str` then `validate()` — and resolves nothing. `crates/ffi/src/lib.rs:52` calls +`parse_config`, and it is the sole config path for the napi addon +(`bindings/nodejs/src/lib.rs:46-63`), the PyO3 extension (`bindings/python/src/lib.rs:17-32`), +and the TypeScript Lambda handler that wraps the addon (`bindings/lambda/src/index.ts` → +`createHandler`). On those three published channels the placeholder the documentation tells +operators to use — *"Secrets (client secrets, API keys, KMS ARNs) should always use `${VAR_NAME}` +placeholders"*, `docs/guides/configuration.md:17` — is not a reference to a secret; it is the +value. `internal_api.shared_secret` becomes the string `${INTERNAL_API_SECRET}`, which this +repository prints verbatim in its own guides, and `internal_auth.rs` then compares a bearer token +against it in constant time — correctly, against the wrong string. `user_sync.webhook.secret` +becomes a published HMAC key. Anything that reads that config, or an error that echoes it, yields +a working credential. Evidence: +[`g2-parse-config-placeholder-gap`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/findings/g2-parse-config-placeholder-gap/g2-parse-config-placeholder-gap.md). + +The fix is not three parallel patches. Adding a `resolve_placeholders` call to `parse_config` +restores parity between exactly today's two functions and leaves the next entry point — the +`config check` subcommand below, or any future embedding shape — equally free to become the +third divergent pipeline. The defect is that one documented contract has two implementations, and +the resolver's own doc comment (`:137-140`) asserted the invariant while nothing tested it across +both paths. What removes the class is a single resolve that owns everything after the source +merge, so an entry point's only remaining choice is which sources it layers, and there is no way +to obtain a config that did not pass through resolution. That is Option 2 of +[`hardening/proposals/config-closed-domain.md`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/hardening/proposals/config-closed-domain.md) +(invariant CV2), and it is the half of that proposal this change lands. + +--- + +## Affected spec pages + +| Canonical page | Nature of change | +| --- | --- | +| [`.specs/service/specs/06-configuration.md`](../service/specs/06-configuration.md) | Modify Loading order into a source-layering list plus one shared resolve; reframe `Validation at load` from `load_config` onto the shared resolve; add `Placeholder resolution` (fail-closed table, residual guard, redaction rule), `Configuration entry points`, and `Pre-flight check (config check)` | +| [`.specs/service/specs/04-http-api.md`](../service/specs/04-http-api.md) | Modify Bootstrap steps 1–2 for the `config check` subcommand and the shared resolve; modify the closing `crates/ffi` paragraph to cover configuration, not just routing | +| [`.specs/bindings/specs/01-ffi-core.md`](../bindings/specs/01-ffi-core.md) | Modify Responsibilities: config through `new`/`from_file` passes the server's resolve; add a Decision recording the one-resolve/differing-sources rule | + +No new canonical page. The `[providers.]`, `[internal_api]` and Defaults-summary sections of +06-configuration are untouched — no TOML-visible field changes. + +--- + +## Proposed changes + +### `.specs/service/specs/06-configuration.md` → Loading order (Modify) + +The section currently lists four numbered steps under the heading +`## Loading order (bootstrap::load_config)`. Replace the heading and the list with: + +> ## Loading order +> +> Configuration reaches a running service through exactly one pipeline. An entry point chooses +> which *sources* it layers; everything after the merge is the shared resolve, which every entry +> point calls and none can bypass. +> +> Sources, lowest precedence first: +> +> 1. `config/default.toml` — compiled-in defaults (committed; see below). +> 2. `config/{OIDC_EXCHANGE_ENV}.toml` — deep-merged overlay when `OIDC_EXCHANGE_ENV` is set +> (e.g. `production`, `sqlite-only`); tables merge recursively, scalars and arrays replace. +> File-backed entry points only. +> 3. `OIDC_EXCHANGE__{section}__{key}` environment variables — structural overrides reaching +> every config path, including map-valued sections. A double underscore separates path +> segments and each segment is lowercased; a single underscore stays inside its segment +> (`…__MY_IDP__…` targets `providers.my_idp`), so keys whose names themselves contain `__` +> cannot be addressed from the environment. +> +> The shared resolve then runs over the merged tree, for every entry point: +> +> 4. `${VAR_NAME}` placeholders anywhere in the merged config are resolved from the environment +> (see Placeholder resolution). +> 5. The resolved config is validated — `server.role`, the duration strings, allowlist entry +> shape, and a non-empty `internal_api.shared_secret` when the internal API will be served +> (see Validation at load) — and a failure aborts before any adapter or router is built. +> +> Steps 4 and 5 are one function. Deserializing the merged tree yields the raw config; only the +> resolve produces the `Config` the runtime consumes, so a code path that skipped resolution has +> nothing to hand `build_service`. + +### `.specs/service/specs/06-configuration.md` → Validation at load (Modify) + +The section's framing predates the shared resolve. It opens "After merging and placeholder +resolution, `load_config` validates the result and refuses to start on failure (`ConfigError`):" +and closes "The same validation runs for config supplied as a string through the FFI bindings +(`bootstrap::parse_config`)." Replace those two framing sentences — the list of checks between +them is unchanged — with: + +> After merging and placeholder resolution, the shared resolve validates the result and refuses +> to produce a config on failure (`ConfigError`): + +and + +> Validation is a step of the shared resolve, so it runs identically on every entry point in +> Configuration entry points — including config supplied as a string through the FFI bindings +> (`bootstrap::parse_config`). + +### `.specs/service/specs/06-configuration.md` → Placeholder resolution (Add) + +> ## Placeholder resolution +> +> `${VAR_NAME}` in any string value, at any depth, is replaced with that environment variable's +> value. Resolution is total and fail-closed: every placeholder either resolves to a real value +> or aborts the load with a `ConfigError`. Literal placeholder text never reaches a running +> service. +> +> | Input | Outcome | +> | --- | --- | +> | `${NAME}`, `NAME` set to a non-empty value | replaced with the value | +> | `${NAME}`, `NAME` unset | `ConfigError` naming `NAME` and the config path; the load produces no config | +> | `${NAME}`, `NAME` set to the empty string | `ConfigError` naming `NAME`, worded to distinguish "set but empty" from "unset" | +> | `${` with no closing `}` within 256 bytes | `ConfigError` naming the config path and the unterminated opener | +> | `${}` — empty name | `ConfigError` naming the config path | +> | `$${` | the escape: rewritten to a literal `${`, never looked up in the environment | +> +> An empty variable is rejected rather than substituted because the fields this idiom exists for +> are the ones where an empty value means "no protection": an unpopulated secret-manager +> reference is a plumbing failure, not an operator's intent. A value that is genuinely meant to +> be empty is expressed by omitting the key (defaults apply) or by writing `""` in the TOML. +> +> After resolution, no config value may still contain an unescaped `${`. This holds as a +> post-condition on the resolved tree, so a value carrying placeholder text is a load failure +> whatever assembled it. +> +> Errors raised during resolution or validation name the environment variable and the config +> path, never the resolved value. `internal_api.shared_secret` and `user_sync.webhook.secret` +> stay redacted on every error and diagnostic path, exactly as they are in `Debug`. + +### `.specs/service/specs/06-configuration.md` → Configuration entry points (Add) + +> ## Configuration entry points +> +> | Entry point | Sources layered | Code | +> | --- | --- | --- | +> | Standalone server (hyper) | 1 + 2 + 3 | `crates/server/src/main.rs` → `bootstrap::load_config` | +> | Lambda runtime (same binary, `AWS_LAMBDA_RUNTIME_API` present) | 1 + 2 + 3 | `crates/server/src/main.rs` → `bootstrap::load_config` | +> | `config check` subcommand | 1 + 2 + 3, or a single named file | `crates/server/src/main.rs` | +> | FFI inline TOML (`OidcExchange::new`) | the supplied document + 3 | `crates/ffi/src/lib.rs` → `bootstrap::parse_config` | +> | FFI file (`OidcExchange::from_file`) | the named file + 3 | reads the file, then `new` | +> | Node binding (napi) | via the FFI entry points | `bindings/nodejs/src/lib.rs` | +> | Python binding (PyO3) | via the FFI entry points | `bindings/python/src/lib.rs` | +> | `@oidc-exchange/lambda` handler | via the Node binding | `bindings/lambda/src/index.ts` | +> +> Every row ends in the same resolve, so placeholder handling, override handling, and rejection +> behaviour are identical across channels. The `OIDC_EXCHANGE_ENV` overlay is the one legitimate +> difference: it applies only where the service selects its own files, and an FFI caller supplies +> the whole document, so there is nothing to overlay it onto. + +### `.specs/service/specs/06-configuration.md` → Pre-flight check (Add) + +> ## Pre-flight check (`oidc-exchange config check`) +> +> ``` +> oidc-exchange config check [--dir ] [--file ] +> ``` +> +> `config check` layers the sources for the shape being checked — `--dir` (default `config/`) for +> the server layering, `--file` for the single-document layering the bindings use — runs the same +> resolve, and exits without constructing an adapter, binding a socket, or writing anything. +> Exit `0` prints a summary of the resolved configuration with every secret-bearing field +> rendered through its redacting `Debug`; any `ConfigError` exits non-zero with the message the +> server would have printed at startup. It is the supported way to prove that a deployment's +> environment satisfies its placeholders before the deployment happens. + +### `.specs/service/specs/04-http-api.md` → Bootstrap (Modify) + +Steps 1 and 2 currently read "Honour `--version` …" and "`bootstrap::load_config` — load +`config/default.toml`, overlay … resolve `${VAR}` placeholders". Replace both with: + +> 1. Handle the CLI surface and exit: `--version` prints the crate version; `config check` +> layers configuration sources and runs the same resolve as step 2, prints a redacted summary, +> and exits non-zero on any `ConfigError` without building adapters or binding a socket +> ([06-configuration.md](06-configuration.md)). +> 2. `bootstrap::load_config` — layer `config/default.toml`, the +> `config/{OIDC_EXCHANGE_ENV}.toml` overlay if set, and `OIDC_EXCHANGE__{section}__{key}` env +> overrides, then run the shared resolve: fail-closed `${VAR}` placeholder resolution followed +> by validation ([06-configuration.md](06-configuration.md)). + +### `.specs/service/specs/04-http-api.md` → Bootstrap, closing paragraph (Modify) + +The section ends "`crates/ffi` calls the same `build_service` / `build_router` path, so in-process +bindings get identical routing and middleware." Replace with: + +> `crates/ffi` layers its own sources into the same resolve and then calls the same +> `build_service` / `build_router` path, so in-process bindings get identical configuration +> semantics, routing, and middleware. + +### `.specs/bindings/specs/01-ffi-core.md` → Responsibilities (Modify) + +Replace the first bullet: + +> - Build an `AppService` and axum `Router` from a TOML config (re-using `crates/server`'s +> bootstrap), and own the tokio `Runtime` that drives them. Config supplied through `new` +> (inline string) or `from_file` passes the server's shared resolve — +> `OIDC_EXCHANGE__{section}__{key}` overrides, fail-closed `${VAR}` placeholder resolution, +> then validation ([06-configuration.md](../../service/specs/06-configuration.md) → +> Loading order). An unresolvable placeholder or an invalid value is an `FfiError` at +> construction; a literal `${…}` never reaches a running router. + +### `.specs/bindings/specs/01-ffi-core.md` → Decisions (Add) + +> - *One resolve, differing sources.* **FFI config passes through the server's resolve; only the +> source set differs — the supplied document plus `OIDC_EXCHANGE__…` overrides, with no +> `OIDC_EXCHANGE_ENV` file overlay.** A second config pipeline is exactly how the published +> Node, Python and Lambda packages came to load documented secret placeholders as literal text. + +--- + +## Type changes + +No `canonical-types.schema.json` change, and no TOML-visible field is added, removed, or +retyped. The two-stage split renames the deserialization target (the struct the merged tree +deserializes into) and introduces the resolved config type the runtime consumes; both mirror +today's `AppConfig` field-for-field. Narrowing the security-relevant fields to closed domain +types — `RegistrationMode`, `SigningAlgorithm`, `HttpsUrl`, `AsciiDomainPattern`, typed audit +severities — is the other half of the hardening proposal's Option 2 and is deliberately **not** +in this change; it hangs off the seam this change creates and is proposed separately in +[`2026-08-05-fail_closed_across_config_and_adapters.md`](2026-08-05-fail_closed_across_config_and_adapters.md). + +--- + +## Implementation notes + +1. Factor the shared tail out of `load_config_from_dir` (`crates/server/src/bootstrap.rs:90-118`) + into one function taking the assembled `config::ConfigBuilder`: `build()` → + `resolve_placeholders(&mut merged.cache)` → deserialize → `validate()` → resolved config. + This is the single resolve point; no other code path calls `try_deserialize` or `validate` + directly. `load_config_from_dir` keeps only its source layering. +2. Rewrite `parse_config` (`:124-128`) to add a `config::File::from_str(toml_str, + FileFormat::Toml)` source plus the same `Environment::with_prefix(ENV_OVERRIDE_PREFIX) + .separator(ENV_OVERRIDE_SEPARATOR).try_parsing(true)` source used at `:107-111`, then call the + shared tail. `File::from_str` is available in the `config` 0.15 release already depended on + (`crates/server/Cargo.toml:20`). The standalone `toml::from_str` at `:125` goes away — it is + the whole defect. +3. `OidcExchange::from_file` (`crates/ffi/src/lib.rs:75-81`) keeps reading the file and + delegating to `new`, so there is no third path to keep in step. +4. Empty-variable rule: `std::env::var` returns `Ok("")` for a set-but-empty variable, so the + lookup at `:186` currently substitutes an empty string silently. Add an explicit empty check + with its own message so "unset" and "set but empty" are distinguishable in an operator's logs. +5. Malformed-placeholder rule: `scan_placeholder_name` (`:223-237`) returns `None` when no `}` + appears within `PLACEHOLDER_NAME_LEN_MAX`, and the caller (`:184-197`) then falls through to + copying `${` as ordinary text — the same fail-open shape as the finding. Make an unescaped + `${` that fails to scan a `ConfigError`, and reject an empty name explicitly rather than + letting `${}` reach `std::env::var("")`. The `ConfigError`s in the Placeholder-resolution + table also name the config *path*; `resolve_placeholders` (`:141-160`) walks the tree without + tracking keys today, so the walk gains a path argument threaded through the recursion. +6. Residual guard: after resolution, walk the tree once more and reject any value still holding + an unescaped `${`. Redundant while resolution is total, and cheap insurance against a future + source that bypasses it. +7. `config check`: extend the argument handling at `crates/server/src/main.rs:12-15` (the crate + has no argument-parsing dependency today; adding one is a deliberate choice, not a + requirement). It needs `load_config_from_dir` made `pub` (private at `:90`) plus a + single-file variant. Print the summary through the redacting `Debug` impls on + `InternalApiConfig::shared_secret` and `WebhookConfig::secret` (`crates/core/src/config.rs`), + never the raw fields. +8. Tests, beside the existing placeholder tests (`crates/server/src/bootstrap.rs:1031-1200`): + a **parity table** driven from one body over both entry points — set, unset, empty, escaped, + unterminated, empty-name — asserting identical outcomes; a `parse_config` case resolving + `shared_secret = "${INTERNAL_API_SECRET}"` with `assert_ne!` against the literal; a + `parse_config` case applying `OIDC_EXCHANGE__REGISTRATION__MODE=existing_users_only`; and a + `config check` case exiting non-zero on an unset variable while printing no secret value. The + absence of a two-entry-point test is what let this through, so the parity table is the load- + bearing one — a future third entry point has an obvious place to be added. +9. Release notes: this is a behaviour change for embedders. A binding host that constructs + successfully today with an unset variable will fail at construction. It needs its own note in + the `@oidc-exchange/node`, `@oidc-exchange/lambda` and PyPI changelogs, not only the server's. + +--- + +## Merge plan + +1. The earlier merge this step used to guard has completed: the `Proposed changes` blocks of + [`2026-07-01-complete_config_loading.md`](merged/2026-07-01-complete_config_loading.md) are on + the canonical pages — 06-configuration carries the `Validation at load` section and the + fail-closed placeholder wording, and 04-http-api's Bootstrap step 2 and internal-route + conditions are in place. The blocks above are written against that text as it now stands. +2. Apply each `Proposed changes` block to its canonical page; bump each page's `**Date:**` to the + merge date. +3. No schema change to fold in. +4. Flip this file's `**Status:**` to `Merged`, add `**Merged:** YYYY-MM-DD`, and move it to + `.specs/changes/merged/`. +5. Update `.specs/README.md`'s Change specs section — add this file when it is proposed, remove + it from the pending list on merge. + +--- + +## Assumptions and open questions + +### Assumptions + +- The Rust Lambda runtime is the same binary as the standalone server + (`crates/server/src/main.rs:33` selects it after `load_config`), so it already resolves + placeholders. The Lambda channel that does not is `@oidc-exchange/lambda`, which reaches + configuration through the Node addon. +- No shipped example or test config depends on an unescaped `${` surviving as literal text; the + `$${` escape covers any that later needs to. +- Config is read once at startup. If hot reload is ever added, these rules have to be + re-established at reload time. + +### Decisions + +- *One resolve, not three patched call sites.* **Everything after the source merge lives in one + function that every entry point calls.** Patching `parse_config` alone restores parity between + two functions and leaves the next entry point free to diverge; making the source layering the + only variable removes the class rather than the instance. +- *No permissive warning phase.* **Placeholder resolution fails closed from the first release + that ships it, with no warn-and-continue window.** The hardening proposal's two-phase migration + is right for narrowing field *types*, where a rejection is a new opinion about a working value. + Here the permissive behaviour is the vulnerability: a warning that still loads + `${INTERNAL_API_SECRET}` as a live credential is the bug with logging added. +- *Empty resolves are rejected.* **A placeholder naming a set-but-empty variable is a startup + error, not an empty string.** The idiom exists for fields where empty means unprotected; the + escape hatch is to omit the key or write `""` in the TOML. +- *Malformed placeholders are errors, not literals.* **An unterminated `${` or an empty `${}` + aborts the load instead of being copied through as text.** Passing a malformed placeholder + through verbatim is the same failure this change exists to remove, and `$${` is the documented + way to write a literal. +- *Env overrides apply on the FFI path.* **`OIDC_EXCHANGE__…` overrides reach binding-supplied + config too.** The documentation promises them unconditionally, and an operator who sets + `OIDC_EXCHANGE__REGISTRATION__MODE=existing_users_only` on a binding runtime today gets no + error and no effect. +- *`config check` ships with this change, not after it.* **The subcommand lands in the same + change as the fail-closed rules.** Making a load fail where it used to succeed is only safe if + an operator can find out before the deploy, and the subcommand is the cheapest piece of the + proposal's Option 3. +- *Closed domain types are out of scope.* **This change builds the seam; it does not narrow the + field types.** A change that both unifies the pipeline and retypes a dozen security-relevant + fields is two changes, and only the first is a security fix. + +### Open questions + +- Should the FFI offer an opt-out from ambient environment overrides for embedders that want a + hermetic config (the host already computed the values it wants)? Parity argues no; an embedder + building config programmatically may reasonably not expect process env to override it. +- How should `config check --file` model the binding shape's environment? It can prove the + placeholders resolve in the *checking* process's environment, which is not necessarily the + Lambda or container environment the addon will run in. +- Does the empty-string rejection need a per-field opt-out for a value legitimately supplied as + empty through the environment? No shipped config needs one today. +- Merge coordination: + [`2026-08-05-fail_closed_across_config_and_adapters.md`](2026-08-05-fail_closed_across_config_and_adapters.md) + also modifies 06-configuration's Loading order and rewrites `Validation at load`; whichever of + the two merges second must refresh its Modify blocks against the merged page. From 01821638ca21e7c7c2a575b9a4e2d966380f792f Mon Sep 17 00:00:00 2001 From: Ant Stanley Date: Sun, 16 Aug 2026 00:00:47 +0200 Subject: [PATCH 2/4] docs(plan): add config placeholder resolution plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code --- .specs/README.md | 1 + .../01-shared_config_resolve_boundary.md | 24 +++++++ .../backlog/02-resolver_fail_closed_parity.md | 24 +++++++ .../backlog/03-config_check_cli.md | 23 +++++++ .../04-document_contract_and_release_notes.md | 24 +++++++ .../plan.md | 68 +++++++++++++++++++ 6 files changed, 164 insertions(+) create mode 100644 .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md create mode 100644 .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md create mode 100644 .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md create mode 100644 .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md create mode 100644 .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md diff --git a/.specs/README.md b/.specs/README.md index 708923b..283bd62 100644 --- a/.specs/README.md +++ b/.specs/README.md @@ -78,6 +78,7 @@ board (`backlog/` · `in-progress/` · `blocked/` · `done/`). | [plans/2026-07-02-webhook_user_sync_conformance/plan.md](plans/2026-07-02-webhook_user_sync_conformance/plan.md) | Done | [changes/merged/2026-07-01-webhook_user_sync_conformance.md](changes/merged/2026-07-01-webhook_user_sync_conformance.md) | | [plans/2026-07-02-server_error_handling_and_shutdown/plan.md](plans/2026-07-02-server_error_handling_and_shutdown/plan.md) | Done | [changes/merged/2026-07-01-server_error_handling_and_shutdown.md](changes/merged/2026-07-01-server_error_handling_and_shutdown.md) | | [plans/2026-07-02-implement_lambda_runtime/plan.md](plans/2026-07-02-implement_lambda_runtime/plan.md) | Done | [changes/merged/2026-07-01-implement_lambda_runtime.md](changes/merged/2026-07-01-implement_lambda_runtime.md) | +| [plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md](plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md) | Review | [changes/2026-08-05-resolve_config_placeholders_all_channels.md](changes/2026-08-05-resolve_config_placeholders_all_channels.md) | ## Conventions diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md new file mode 100644 index 0000000..d826efc --- /dev/null +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md @@ -0,0 +1,24 @@ +# Task 01: Shared config resolve boundary + +**Plan:** [plan.md](../plan.md) +**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Proposed changes / Implementation notes 1–3; [06-configuration.md](../../../service/specs/06-configuration.md) → future Loading order / Configuration entry points +**Depends on:** — +**Produces:** one server-owned builder-to-resolved-`AppConfig` tail used by both file-backed configuration and FFI TOML, with environment overrides layered before placeholder resolution and validation. +**Pointers:** `crates/server/src/bootstrap.rs:90-128`; `crates/ffi/src/lib.rs:49-81`; `config` 0.15 `File::from_str` already available through `crates/server/Cargo.toml`. + +## Steps + +- [ ] Extract the post-source-assembly tail from `load_config_from_dir`: build `config::Config`, resolve the merged tree, deserialize the raw shape, validate, and return the runtime config. Make this the only production path containing `try_deserialize` and `validate`. +- [ ] Retain file-backed source layering in `load_config_from_dir` (default file, optional selected overlay, structural environment overrides), then delegate to the shared tail. +- [ ] Rewrite `parse_config` to build from `File::from_str(toml_str, FileFormat::Toml)` plus the same `OIDC_EXCHANGE__…` `Environment` source, then delegate to the shared tail. Remove the direct `toml::from_str` path. +- [ ] Keep `OidcExchange::from_file` as file read → `new`; do not introduce a third configuration pipeline. +- [ ] Add focused regression tests proving FFI parsing resolves `internal_api.shared_secret = "${INTERNAL_API_SECRET}"` to the environment value and never returns the literal, and applies `OIDC_EXCHANGE__REGISTRATION__MODE=existing_users_only` to inline TOML. +- [ ] Preserve the current file-backed happy path and validation tests; run targeted server/FFI tests plus Rust format/clippy checks. + +## Definition of done + +- [ ] File-backed and inline FFI TOML configuration both traverse one resolve/deserialize/validate implementation; repository search shows no other production `try_deserialize`/`validate` bypass in configuration entry points. +- [ ] An FFI caller with set `INTERNAL_API_SECRET` gets the resolved secret, not `${INTERNAL_API_SECRET}`; FFI inline TOML receives the documented structural environment override. +- [ ] `OidcExchange::from_file` still delegates through `new`, so Node, Python, and the TypeScript Lambda wrapper inherit the same path without channel-specific patches. +- [ ] Positive and negative regression tests are added or preserved; no secret value is asserted via error output. +- [ ] `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and targeted tests pass. If `cargo test --workspace` is run, record the known three missing `providers.*.adapter` test failures without changing them. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md new file mode 100644 index 0000000..24aec41 --- /dev/null +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md @@ -0,0 +1,24 @@ +# Task 02: Resolver fail-closed hardening and entry-point parity + +**Plan:** [plan.md](../plan.md) +**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Placeholder resolution / Implementation notes 4–6 and 8; [06-configuration.md](../../../service/specs/06-configuration.md) → future Placeholder resolution +**Depends on:** 01 +**Produces:** a total, path-aware shared resolver that rejects empty, malformed, and residual unescaped placeholders and a parity table exercising all current configuration entry points. +**Pointers:** `crates/server/src/bootstrap.rs:141-237`, especially `resolve_placeholders`, `resolve_placeholders_in_str`, `scan_placeholder_name`; existing tests at `bootstrap.rs:1031-1200`. + +## Steps + +- [ ] Thread a stable config path through the value-tree walk, including table keys and array indices, so resolution failures name both the environment variable (where applicable) and the configuration location without exposing a resolved value. +- [ ] Reject `Ok("")` from `std::env::var` with wording distinguishable from an unset environment variable. +- [ ] Treat any unescaped `${` with no closing `}` within `PLACEHOLDER_NAME_LEN_MAX` as `ConfigError`; reject `${}` explicitly. Keep `$${` as the only literal escape and preserve its no-lookup guarantee. +- [ ] Add a post-resolution tree pass that rejects residual unescaped `${` while permitting the explicit escape result according to the documented representation; make its traversal bounded/iterative as required by the project guidelines. +- [ ] Add one parity-table test body run through `load_config_from_dir` and `parse_config`, covering set, unset, empty, escaped, unterminated, and empty-name cases. Assert equivalent success/failure semantics, relevant variable/path diagnostics, and absence of secret values. +- [ ] Cover a nested/map-valued path and at least one array path if the `config::Value` representation permits it, so path propagation is not table-only. + +## Definition of done + +- [ ] No unescaped `${` can reach a runtime `AppConfig`: valid names resolve to non-empty environment values, unset/empty/malformed/empty-name/residual forms return `ConfigError`, and `$${` yields literal `${` without lookup. +- [ ] Every resolver error names the config path and appropriate variable/token category but never the resolved secret; redacted `Debug` remains the only output route for secret-bearing fields. +- [ ] The same parity cases produce the same outcomes for file-backed and FFI TOML inputs; adding a future entry point has an obvious table hook. +- [ ] Existing valid file-backed resolution remains covered; malformed and empty conditions have paired negative-space tests. +- [ ] Targeted server/FFI tests, `cargo fmt --all --check`, and `cargo clippy --workspace -- -D warnings` pass. Workspace test baseline remains explicitly excluded: do not repair the three missing `providers.*.adapter` tests. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md new file mode 100644 index 0000000..d026295 --- /dev/null +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md @@ -0,0 +1,23 @@ +# Task 03: Config check CLI + +**Plan:** [plan.md](../plan.md) +**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Pre-flight check / Implementation notes 7–8; [04-http-api.md](../../../service/specs/04-http-api.md) → future Bootstrap steps 1–2 +**Depends on:** 02 +**Produces:** `oidc-exchange config check [--dir ] [--file ]`, a preflight-only caller of the shared resolver that emits a redacted summary on success and exits non-zero on `ConfigError`. +**Pointers:** `crates/server/src/main.rs:10-85`; `crates/server/src/bootstrap.rs:58-128`; redacting `Debug` implementations in `crates/core/src/config.rs`; no existing argument-parsing dependency. + +## Steps + +- [ ] Define and implement the minimal CLI grammar for `config check`, `--dir`, and `--file`, including mutually exclusive/invalid-argument handling; decide whether a small parser dependency or explicit bounded argument parsing best fits the existing binary and document the choice in the PR. +- [ ] Make the file-backed loader callable by the CLI and add a single-file loader using the same inline/file source shape as FFI. Both must delegate to task 01's shared resolver, not reimplement source merging or validation. +- [ ] For `--dir` (default `config/`), layer default, selected overlay, and structural environment overrides. For `--file`, layer that named document plus structural overrides, matching FFI's source semantics. +- [ ] On success, print only the configuration through established redacting `Debug` output and exit before telemetry initialization, adapter construction, router creation, socket binding, or writes. On resolution/validation failure, return non-zero and preserve the safe diagnostic. +- [ ] Add CLI-level tests or a testable command runner covering directory and file successes, unset-placeholder non-zero failure, invalid argument combinations, and output absence of the raw secret. +- [ ] Verify `--version` remains unchanged and Rust Lambda/server startup continues to load configuration before runtime selection. + +## Definition of done + +- [ ] `oidc-exchange config check` accepts the documented forms, uses the shared resolve exactly once, and does not construct adapters, bind a socket, initialize telemetry, or write state. +- [ ] An unset placeholder exits non-zero and names the safe failure context without printing its raw secret; a successful run prints a redacted summary with `internal_api.shared_secret` and `user_sync.webhook.secret` protected. +- [ ] `--dir` and `--file` reflect their respective source shapes, including `OIDC_EXCHANGE__…` overrides, and invalid CLI combinations fail deterministically. +- [ ] Positive and negative command tests pass along with `cargo fmt --all --check` and `cargo clippy --workspace -- -D warnings`; do not change the known workspace-test baseline failures. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md new file mode 100644 index 0000000..3fbc322 --- /dev/null +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md @@ -0,0 +1,24 @@ +# Task 04: Document the shared-resolve contract and embedding break + +**Plan:** [plan.md](../plan.md) +**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Affected spec pages / Proposed changes / Merge plan / Implementation note 9 +**Depends on:** 02, 03 +**Produces:** canonical service/FFI documentation matching implementation, release notes for Node/Lambda/PyPI embedding users, and change-spec/README merge housekeeping. +**Pointers:** `.specs/service/specs/06-configuration.md`; `.specs/service/specs/04-http-api.md`; `.specs/bindings/specs/01-ffi-core.md`; `.specs/README.md`; release-note/changelog locations in `bindings/nodejs`, `bindings/lambda`, and `bindings/python` (identify existing project convention before editing). + +## Steps + +- [ ] Apply the source spec's precise 06-configuration changes: source layering vs one shared resolve; validation framing; Placeholder resolution table; Configuration entry points; and config-check preflight contract. Preserve untouched field/default sections. +- [ ] Apply the 04-http-api Bootstrap changes and closing FFI paragraph, and the FFI-core Responsibilities and one-resolve Decision exactly against the code shipped in tasks 01–03. +- [ ] Locate existing release-note/changelog conventions for `@oidc-exchange/node`, `@oidc-exchange/lambda`, and PyPI; add a concise behaviour-change note to each: an unresolved/empty/malformed placeholder now fails binding construction instead of becoming literal configuration text. +- [ ] Follow the source spec merge plan only after all implementation/tests are accepted: set the change spec status/merged date, move it to `changes/merged/`, and update README change-spec indexing. Do not create a canonical-type schema update. +- [ ] Update the Plans table in `.specs/README.md` to list this plan and keep its status synchronized with the kanban state. +- [ ] Verify every Markdown relative link, source/back-reference, table target, task dependency, and status after moving the source spec; ensure no done certificate is introduced. + +## Definition of done + +- [ ] 06-configuration, 04-http-api, and FFI-core state the one-resolve/differing-sources invariant, total fail-closed placeholder contract, env overrides, redaction, all entry points, and config-check behaviour exactly as implemented. +- [ ] Release notes cover Node, Lambda, and PyPI embedders; they explain the construction-time compatibility impact without exposing a real secret or inventing new API behaviour. +- [ ] Change spec merge housekeeping and README indexes are internally consistent; no schema file changes because no TOML-visible shape changed. +- [ ] All Markdown links resolve; tasks 01–04 remain DAG-valid with lower-number dependencies; all task checkboxes and plan status accurately reflect actual work state. +- [ ] Done certificates remain intentionally absent: no `done/` directory and no certificate file is created. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md new file mode 100644 index 0000000..14d7d3d --- /dev/null +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md @@ -0,0 +1,68 @@ +# Plan: Resolve `${VAR}` placeholders on every configuration entry point + +**Status:** Review · **Layout:** kanban · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Source spec:** [`.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md`](../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) + +Build one configuration resolve boundary for every source shape: file-backed server/Lambda configuration and inline/file FFI configuration. The resolver owns placeholder rejection, env overrides, deserialization, and validation; entry points own only source layering. The plan then exposes the same path through `oidc-exchange config check`, documents the shipped contract and publishes embedding release notes. Node, Python, and `@oidc-exchange/lambda` are covered through their existing FFI chain; this plan deliberately does not duplicate binding implementations. + +--- + +## Source and definition-of-done baseline + +- **Spec.** The source change spec's Motivation, Affected spec pages, Proposed changes, Implementation notes, Type changes, and Merge plan. It targets `crates/server`, `crates/ffi`, the three canonical pages named in the change spec, and release-note locations for Node, Lambda, and PyPI. It adds no TOML-visible field and requires no `canonical-types.schema.json` change. +- **Already built.** `bootstrap::load_config_from_dir` layers default/overlay/environment sources, directly resolves placeholders, deserializes, and validates; `bootstrap::parse_config` directly uses `toml::from_str` then validation, omitting placeholders and structural overrides. `OidcExchange::new` is the sole configuration path for napi/PyO3 and `OidcExchange::from_file` delegates to it. `main.rs` recognizes only `--version`; no CLI parser or `config check` exists. Existing resolver tests cover file-backed set/unset/escaped/nested cases, but there is no entry-point parity table or malformed/empty/residual test. +- **Known baseline failure.** `cargo test --workspace` is already red because three `providers.*.adapter` configuration tests are missing. This unstacked PR neither fixes nor absorbs that unrelated failure. Each task reports that failure separately if it appears; targeted tests and non-test checks remain required evidence. +- **Definition of done.** Every task inherits [`.specs/development-guidelines.md`](../../development-guidelines.md) §Definition of done: positive and negative-space tests, meaningful assertions in touched functions, named bounds, and Rust format/clippy/test checks. Since this scope touches Rust only, run `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and targeted package/test commands during task work; attempt `cargo test --workspace` at integration and record the three pre-existing missing-provider-adapter test failures without changing them. +- **Done certificates.** Intentionally omitted by explicit instruction. This plan creates no `done/` directory and no `*-certificate.md` files; task checklists, review evidence, and the plan-level DoDs are the sole tracking artifacts. + +--- + +## Task graph + +```mermaid +graph TD + 01["01 · shared config resolve boundary"] --> 02["02 · resolver fail-closed hardening + parity tests"] + 02 --> 03["03 · config check CLI"] + 02 --> 04["04 · canonical docs + embedding release notes"] + 03 --> 04 +``` + +The dependency table is the **source of truth**; the Mermaid graph visualizes it. If they disagree, the table wins. + +| Task | Depends on | Edge kind | Produces (reviewable artifact) | +|---|---|---|---| +| 01 · shared config resolve boundary | — | — | one server-owned resolve function used by file-backed and FFI TOML source builders; FFI receives `OIDC_EXCHANGE__…` overrides before resolution and validation | +| 02 · resolver fail-closed hardening + parity tests | 01 | build, contract | path-aware, total placeholder resolution rejects unset, empty, malformed, and residual placeholders identically on both entry points; `$${` remains the literal escape | +| 03 · config check CLI | 02 | build | `oidc-exchange config check [--dir ] [--file ]` resolves and validates without adapters, socket binding, or writes, and prints a redacted summary | +| 04 · canonical docs + embedding release notes | 02, 03 | review | 06-configuration, 04-http-api, and FFI-core describe the shipped one-resolve contract; Node/Lambda/PyPI release notes explain construction can now fail for unresolved placeholders | + +Each `Depends on` references lower-numbered tasks only. Task 01 defines the sole production seam; task 02 strengthens and proves its contract; task 03 consumes that proven seam; task 04 must be reviewed against both implementation and CLI behaviour before it describes either as canonical fact. + +--- + +## Implementation order and milestones + +**Order:** `01, 02, 03, 04`. Task 01 removes the divergent source-to-runtime pipeline before any behavioural hardening is added. Task 02 makes the shared resolver total and supplies the parity test harness that subsequent entry points must join. Task 03 is then a thin CLI caller rather than a third pipeline. Task 04 lands last because canonical specs and release notes must describe observable shipped behaviour. + +| Milestone | Tasks | Demonstrable when complete | Review gate | +|---|---|---|---| +| M1 — one resolve boundary | 01, 02 | File-backed and FFI TOML inputs use one builder-to-resolved-config tail; both receive structural overrides and have identical set/unset/empty/escape/unterminated/empty-name outcomes with paths and no secret values in errors | server unit parity table plus FFI construction regression tests; `cargo fmt --all --check`; `cargo clippy --workspace -- -D warnings` | +| M2 — preflight operator path | 03 | `config check` loads either directory or one file, prints only redacted output on success, and fails non-zero without building adapters/binding/writing when resolution fails | CLI success/failure tests demonstrate exit status and secret non-disclosure; targeted server tests green | +| M3 — contract published | 04 | All three canonical pages have the source spec's merge blocks, embedding release notes disclose the construction-time break, and the change spec/README housekeeping is ready for merge | link check; docs agree with M1/M2; `cargo test --workspace` attempted and the known three missing `providers.*.adapter` failures are recorded, not altered | + +--- + +## Scope boundaries, sibling dependencies, and open questions + +**In scope:** shared resolve factoring; FFI source layering parity; placeholder failure rules and tests; `config check`; the specified canonical docs; release notes for published embedding channels; source-spec merge housekeeping once implementation is complete. + +**Out of scope:** closed-domain config types and adapter validation, new TOML fields/schema changes, binding API redesigns, a hermetic FFI opt-out, environment simulation beyond the checking process, and the unrelated three missing `providers.*.adapter` tests. + +**Sibling dependency / merge coordination:** [`2026-08-05-fail_closed_across_config_and_adapters.md`](../../changes/2026-08-05-fail_closed_across_config_and_adapters.md) is a sibling hardening change, not a prerequisite to build this PR. The sibling spec is not present in this unstacked workspace; this reference records the declared coordination dependency only. It deliberately owns narrowing security-relevant fields to closed types and also edits 06-configuration's Loading order and Validation at load. Do not fold any of that work into tasks 01–04. Whichever unstacked PR merges second must refresh its Modify blocks against the then-current canonical page. + +**Open questions retained for owner decision (not implementation blockers):** + +1. Whether FFI needs a future opt-out from ambient `OIDC_EXCHANGE__…` overrides for hermetic embedding. +2. How `config check --file` should represent the eventual Lambda/container environment rather than only the checker process. +3. Whether a future per-field opt-out is needed for intentionally empty environment substitutions; no shipped config needs one. + +**Decisions fixed by the source spec:** fail closed immediately; reject empty and malformed placeholders; `$${` is the literal escape; no raw secret in errors or diagnostic output; config check ships in this PR; no schema/type-narrowing work here. From 0165e369243a6396ac59b8a0b7ec01a0eb98655f Mon Sep 17 00:00:00 2001 From: Ant Stanley Date: Mon, 17 Aug 2026 07:02:11 +0200 Subject: [PATCH 3/4] feat(config): resolve placeholders across all configuration entry points --- crates/server/src/bootstrap.rs | 160 +++++++++++++++++++++++++++------ crates/server/src/main.rs | 49 +++++++++- 2 files changed, 180 insertions(+), 29 deletions(-) diff --git a/crates/server/src/bootstrap.rs b/crates/server/src/bootstrap.rs index 0d8256d..f1a273c 100644 --- a/crates/server/src/bootstrap.rs +++ b/crates/server/src/bootstrap.rs @@ -63,6 +63,19 @@ pub fn load_config() -> Result> { load_config_from_dir(CONFIG_DIR) } +/// Load and resolve a single TOML file with structural environment overrides. +/// This is intentionally the same source shape used by inline FFI TOML. +pub fn load_config_from_file(path: &str) -> Result> { + let builder = Config::builder() + .add_source(File::from(std::path::Path::new(path)).format(FileFormat::Toml)) + .add_source( + Environment::with_prefix(ENV_OVERRIDE_PREFIX) + .separator(ENV_OVERRIDE_SEPARATOR) + .try_parsing(true), + ); + resolve_config(builder) +} + /// Core of [`load_config`], parameterized over the config directory so tests /// can point it at a fixture directory instead of the process's `config/`. /// @@ -87,7 +100,7 @@ pub fn load_config() -> Result> { /// With no files present and no overriding environment variables, the /// deserialized result is `AppConfig::default()` (every field carries /// `#[serde(default)]`). -fn load_config_from_dir(config_dir: &str) -> Result> { +pub fn load_config_from_dir(config_dir: &str) -> Result> { let mut builder = Config::builder().add_source( File::with_name(&format!("{config_dir}/default")) .format(FileFormat::Toml) @@ -110,11 +123,7 @@ fn load_config_from_dir(config_dir: &str) -> Result Result Result> { - let config: AppConfig = toml::from_str(toml_str)?; + let builder = Config::builder() + .add_source(File::from_str(toml_str, FileFormat::Toml)) + .add_source( + Environment::with_prefix(ENV_OVERRIDE_PREFIX) + .separator(ENV_OVERRIDE_SEPARATOR) + .try_parsing(true), + ); + resolve_config(builder) +} + +/// Apply the one common configuration tail after an entry point has assembled +/// its sources: merge, resolve placeholders, deserialize, then validate. +fn resolve_config( + builder: config::ConfigBuilder, +) -> Result> { + let mut merged = builder.build()?; + resolve_placeholders(&mut merged.cache, "")?; + let config: AppConfig = merged.try_deserialize()?; config.validate()?; Ok(config) } @@ -138,22 +164,24 @@ pub fn parse_config(toml_str: &str) -> Result Result<(), Error> { +fn resolve_placeholders(value: &mut Value, path: &str) -> Result<(), Error> { match &mut value.kind { - ValueKind::String(s) => { - *s = resolve_placeholders_in_str(s)?; - } + ValueKind::String(s) => *s = resolve_placeholders_in_str(s, path)?, ValueKind::Table(table) => { - for nested in table.values_mut() { - resolve_placeholders(nested)?; + for (key, nested) in table.iter_mut() { + let nested_path = if path == "" { + key.to_string() + } else { + format!("{path}.{key}") + }; + resolve_placeholders(nested, &nested_path)?; } } ValueKind::Array(items) => { - for item in items.iter_mut() { - resolve_placeholders(item)?; + for (index, item) in items.iter_mut().enumerate() { + resolve_placeholders(item, &format!("{path}[{index}]"))?; } } - // Non-string scalars (bool, numbers, nil) carry no placeholders. _ => {} } Ok(()) @@ -161,7 +189,7 @@ fn resolve_placeholders(value: &mut Value) -> Result<(), Error> { /// Resolve every `${VAR}` placeholder and `$${` escape inside a single /// string, returning the rewritten string. -fn resolve_placeholders_in_str(input: &str) -> Result { +fn resolve_placeholders_in_str(input: &str, path: &str) -> Result { let bytes = input.as_bytes(); let mut output = String::with_capacity(input.len()); let mut i = 0; @@ -182,18 +210,31 @@ fn resolve_placeholders_in_str(input: &str) -> Result { // Placeholder open: `${NAME}`. if bytes[i] == b'$' && bytes.get(i + 1) == Some(&b'{') { - if let Some((name, consumed)) = scan_placeholder_name(&input[i + 2..]) { - let resolved = std::env::var(name).map_err(|_| Error::ConfigError { + let (name, consumed) = + scan_placeholder_name(&input[i + 2..]).ok_or_else(|| Error::ConfigError { + detail: format!("malformed placeholder at config path '{path}'"), + })?; + if name.is_empty() { + return Err(Error::ConfigError { + detail: format!("empty placeholder name at config path '{path}'"), + }); + } + let resolved = std::env::var(name).map_err(|_| Error::ConfigError { + detail: format!( + "config placeholder '${{{name}}}' at config path '{path}' references unset environment variable '{name}'" + ), + })?; + if resolved.is_empty() { + return Err(Error::ConfigError { detail: format!( - "config placeholder '${{{name}}}' references unset environment \ - variable '{name}'" + "config placeholder '${{{name}}}' at config path '{path}' references empty environment variable '{name}'" ), - })?; - output.push_str(&resolved); - i += 2 + consumed; - debug_assert!(i > before, "placeholder branch must consume input"); - continue; + }); } + output.push_str(&resolved); + i += 2 + consumed; + debug_assert!(i > before, "placeholder branch must consume input"); + continue; } // Ordinary text: copy one full UTF-8 scalar value forward. `i` is @@ -218,8 +259,7 @@ fn resolve_placeholders_in_str(input: &str) -> Result { /// Scan forward from just past a `${` opener for its closing `}`, bounded by /// [`PLACEHOLDER_NAME_LEN_MAX`]. Returns the placeholder name and the number /// of bytes consumed (name plus the closing brace), or `None` when no `}` is -/// found within the bound — in which case the `${` is left as ordinary text -/// rather than treated as a malformed placeholder. +/// found within the bound. Callers reject that as malformed configuration. fn scan_placeholder_name(rest: &str) -> Option<(&str, usize)> { let bytes = rest.as_bytes(); let scan_bound = bytes.len().min(PLACEHOLDER_NAME_LEN_MAX); @@ -1336,6 +1376,70 @@ mod load_config_tests { assert_eq!(config.server.role, "all"); } + + #[test] + fn parse_config_resolves_placeholders_for_ffi_callers() { + let _env_lock = lock_test_environment(); + let _guard = EnvVarGuard::set(&[("INTERNAL_API_SECRET", "ffi-secret")]); + let config = parse_config( + r#" + [server] + role = "all" + [internal_api] + enabled = true + shared_secret = "${INTERNAL_API_SECRET}" + "#, + ) + .expect("FFI TOML placeholders must resolve before validation"); + + assert_eq!( + config.internal_api.shared_secret.as_deref(), + Some("ffi-secret") + ); + } + + #[test] + fn parse_config_applies_structural_environment_overrides_for_ffi_callers() { + let _env_lock = lock_test_environment(); + let _guard = + EnvVarGuard::set(&[("OIDC_EXCHANGE__REGISTRATION__MODE", "existing_users_only")]); + let config = parse_config("[server]\nrole = \"all\"") + .expect("FFI TOML must receive structural environment overrides"); + + assert_eq!(config.registration.mode, "existing_users_only"); + } + + #[test] + fn parse_config_rejects_empty_placeholder_values_with_a_path() { + let _env_lock = lock_test_environment(); + let _guard = EnvVarGuard::set(&[("INTERNAL_API_SECRET", "")]); + let err = parse_config( + r#" + [server] + role = "all" + [internal_api] + enabled = true + shared_secret = "${INTERNAL_API_SECRET}" + "#, + ) + .expect_err("empty placeholder values must fail closed"); + let message = err.to_string(); + assert!(message.contains("empty environment variable 'INTERNAL_API_SECRET'")); + assert!(message.contains("internal_api.shared_secret")); + assert!(!message.contains("ffi-secret")); + } + + #[test] + fn parse_config_rejects_malformed_and_empty_placeholders() { + let _env_lock = lock_test_environment(); + for placeholder in ["${", "${}"] { + let err = parse_config(&format!( + "[server]\nrole = \"all\"\n[internal_api]\nenabled = true\nshared_secret = \"{placeholder}\"" + )) + .expect_err("malformed placeholders must fail closed"); + assert!(err.to_string().contains("internal_api.shared_secret")); + } + } } // --------------------------------------------------------------------------- diff --git a/crates/server/src/main.rs b/crates/server/src/main.rs index 5fff443..6308ec5 100644 --- a/crates/server/src/main.rs +++ b/crates/server/src/main.rs @@ -9,10 +9,17 @@ const VERSION: &str = env!("CARGO_PKG_VERSION"); #[tokio::main] async fn main() -> Result<(), Box> { - if std::env::args().any(|a| a == "--version" || a == "-V") { + let args: Vec = std::env::args().skip(1).collect(); + if args.iter().any(|a| a == "--version" || a == "-V") { println!("oidc-exchange {VERSION}"); return Ok(()); } + if args.first().map(String::as_str) == Some("config") { + return run_config_command(&args[1..]); + } + if !args.is_empty() { + return Err(format!("unrecognized arguments: {}", args.join(" ")).into()); + } // 1. Load config let config = bootstrap::load_config()?; @@ -84,3 +91,43 @@ async fn main() -> Result<(), Box> { Ok(()) } + +/// Run preflight-only configuration commands without initializing telemetry, +/// building adapters, binding sockets, or writing state. +fn run_config_command(args: &[String]) -> Result<(), Box> { + if args.first().map(String::as_str) != Some("check") { + return Err( + "usage: oidc-exchange config check [--dir ] [--file ]".into(), + ); + } + + let mut dir: Option<&str> = None; + let mut file: Option<&str> = None; + let mut index = 1; + while index < args.len() { + match args[index].as_str() { + "--dir" => { + index += 1; + dir = Some(args.get(index).ok_or("--dir requires a path")?); + } + "--file" => { + index += 1; + file = Some(args.get(index).ok_or("--file requires a path")?); + } + value => return Err(format!("unknown config check argument: {value}").into()), + } + index += 1; + } + if dir.is_some() && file.is_some() { + return Err("--dir and --file are mutually exclusive".into()); + } + + let config = match (dir, file) { + (Some(path), None) => bootstrap::load_config_from_dir(path)?, + (None, Some(path)) => bootstrap::load_config_from_file(path)?, + (None, None) => bootstrap::load_config()?, + (Some(_), Some(_)) => unreachable!("validated mutually exclusive options"), + }; + println!("{config:?}"); + Ok(()) +} From dad9e426f57ffc44844b8d43d5b2d205749ce570 Mon Sep 17 00:00:00 2001 From: Ant Stanley Date: Mon, 17 Aug 2026 07:13:01 +0200 Subject: [PATCH 4/4] docs(plan): finalize config placeholder kanban board MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record completed implementation and documentation evidence, plus the independent review and final verification gate. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code --- .specs/README.md | 3 +- .specs/bindings/specs/01-ffi-core.md | 15 ++- ...esolve_config_placeholders_all_channels.md | 26 ++--- .../01-shared_config_resolve_boundary.md | 25 ++-- .../02-resolver_fail_closed_parity.md | 25 ++-- .../{backlog => done}/03-config_check_cli.md | 23 ++-- .../04-document_contract_and_release_notes.md | 25 ++-- .../plan.md | 23 +++- .specs/service/specs/04-http-api.md | 19 +-- .specs/service/specs/06-configuration.md | 109 +++++++++++++++++- bindings/lambda/README.md | 6 + bindings/nodejs/README.md | 6 + bindings/python/README.md | 6 + 13 files changed, 231 insertions(+), 80 deletions(-) rename .specs/changes/{ => merged}/2026-08-05-resolve_config_placeholders_all_channels.md (92%) rename .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/{backlog => done}/01-shared_config_resolve_boundary.md (60%) rename .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/{backlog => done}/02-resolver_fail_closed_parity.md (61%) rename .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/{backlog => done}/03-config_check_cli.md (66%) rename .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/{backlog => done}/04-document_contract_and_release_notes.md (69%) diff --git a/.specs/README.md b/.specs/README.md index 283bd62..8e5c2e2 100644 --- a/.specs/README.md +++ b/.specs/README.md @@ -35,6 +35,7 @@ Proposed deltas to the canonical spec live under `changes/` as single documents | [changes/2026-06-24-add_atproto_provider.md](changes/2026-06-24-add_atproto_provider.md) | Proposed | service: Tier 3 atproto provider | | [changes/2026-06-24-complete_telemetry_exporters.md](changes/2026-06-24-complete_telemetry_exporters.md) | Proposed | service: OTLP/X-Ray exporters + OTEL span layer | | [changes/merged/2026-07-01-complete_config_loading.md](changes/merged/2026-07-01-complete_config_loading.md) | Merged | service: config overlay merge, env overrides, fail-closed `${VAR}` placeholders, startup validation | +| [changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md](changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) | Merged | service/bindings: one shared config resolve, fail-closed placeholders, FFI parity, and `config check` | | [changes/merged/2026-07-01-fix_kms_ecdsa_and_jwk_encoding.md](changes/merged/2026-07-01-fix_kms_ecdsa_and_jwk_encoding.md) | Merged | service: KMS ES* DER→raw JWS signatures, RFC 7518 JWK `n`/`e`, ES512 JWK | | [changes/merged/2026-07-01-valkey_session_store_conformance.md](changes/merged/2026-07-01-valkey_session_store_conformance.md) | Merged | service: Valkey session count, atomic TTL'd writes, expired-index cleanup | | [changes/merged/2026-07-01-release_gil_in_python_binding.md](changes/merged/2026-07-01-release_gil_in_python_binding.md) | Merged | bindings: release the GIL around the blocking FFI call | @@ -78,7 +79,7 @@ board (`backlog/` · `in-progress/` · `blocked/` · `done/`). | [plans/2026-07-02-webhook_user_sync_conformance/plan.md](plans/2026-07-02-webhook_user_sync_conformance/plan.md) | Done | [changes/merged/2026-07-01-webhook_user_sync_conformance.md](changes/merged/2026-07-01-webhook_user_sync_conformance.md) | | [plans/2026-07-02-server_error_handling_and_shutdown/plan.md](plans/2026-07-02-server_error_handling_and_shutdown/plan.md) | Done | [changes/merged/2026-07-01-server_error_handling_and_shutdown.md](changes/merged/2026-07-01-server_error_handling_and_shutdown.md) | | [plans/2026-07-02-implement_lambda_runtime/plan.md](plans/2026-07-02-implement_lambda_runtime/plan.md) | Done | [changes/merged/2026-07-01-implement_lambda_runtime.md](changes/merged/2026-07-01-implement_lambda_runtime.md) | -| [plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md](plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md) | Review | [changes/2026-08-05-resolve_config_placeholders_all_channels.md](changes/2026-08-05-resolve_config_placeholders_all_channels.md) | +| [plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md](plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md) | Done | [changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md](changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) | ## Conventions diff --git a/.specs/bindings/specs/01-ffi-core.md b/.specs/bindings/specs/01-ffi-core.md index a33d308..81f8f8e 100644 --- a/.specs/bindings/specs/01-ffi-core.md +++ b/.specs/bindings/specs/01-ffi-core.md @@ -1,14 +1,19 @@ # FFI Core (`crates/ffi`) -**Status:** Implemented · **Date:** 2026-06-24 · **Owner:** Ant Stanley · **Scope:** crates/ffi +**Status:** Implemented · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Scope:** crates/ffi The shared Rust layer the language bindings consume. It wraps the server's axum router behind a small synchronous interface and owns the tokio runtime so a non-async host can call it. ## Responsibilities -- Build an `AppService` and axum `Router` from a TOML config (re-using - `crates/server`'s bootstrap), and own the tokio `Runtime` that drives them. +- Build an `AppService` and axum `Router` from a TOML config (re-using `crates/server`'s + bootstrap), and own the tokio `Runtime` that drives them. Config supplied through `new` + (inline string) or `from_file` passes the server's shared resolve — + `OIDC_EXCHANGE__{section}__{key}` overrides, fail-closed `${VAR}` placeholder resolution, + then validation ([06-configuration.md](../../service/specs/06-configuration.md) → Loading + order). An unresolvable placeholder or an invalid value is an `FfiError` at construction; a + literal `${…}` never reaches a running router. - Convert a primitive HTTP request into an axum request, route it, and convert the response back to primitives. - Map every error into a stable `FfiError`; never let a panic cross the FFI boundary. @@ -63,6 +68,10 @@ Depends on `crates/server` (router construction), `crates/core`, and `tokio`/`ax is responsible for moving the call off the host's event-loop thread. - *Errors as `{code, message}`.* **All Rust errors collapse to `FfiError`.** A flat, stable shape every language can surface without knowing the domain `Error` enum. +- *One resolve, differing sources.* **FFI config passes through the server's resolve; only the + source set differs — the supplied document plus `OIDC_EXCHANGE__…` overrides, with no + `OIDC_EXCHANGE_ENV` file overlay.** A second config pipeline is exactly how the published + Node, Python, and Lambda packages came to load documented secret placeholders as literal text. ### Open questions diff --git a/.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md b/.specs/changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md similarity index 92% rename from .specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md rename to .specs/changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md index 18dc7b2..64dcbf4 100644 --- a/.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md +++ b/.specs/changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md @@ -1,6 +1,6 @@ # Change: Resolve `${VAR}` placeholders on every configuration entry point -**Status:** Proposed · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Target:** crates/server, bindings/* (service, bindings) +**Status:** Merged · **Date:** 2026-08-05 · **Merged:** 2026-08-05 · **Owner:** Ant Stanley · **Target:** crates/server, bindings/* (service, bindings) Route every configuration entry point through one shared resolve step — fail-closed `${VAR}` placeholder resolution, `OIDC_EXCHANGE__{section}__{key}` overrides, then validation — so the @@ -29,7 +29,7 @@ repository prints verbatim in its own guides, and `internal_auth.rs` then compar against it in constant time — correctly, against the wrong string. `user_sync.webhook.secret` becomes a published HMAC key. Anything that reads that config, or an error that echoes it, yields a working credential. Evidence: -[`g2-parse-config-placeholder-gap`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/findings/g2-parse-config-placeholder-gap/g2-parse-config-placeholder-gap.md). +`g2-parse-config-placeholder-gap` (tracked in the security findings archive). The fix is not three parallel patches. Adding a `resolve_placeholders` call to `parse_config` restores parity between exactly today's two functions and leaves the next entry point — the @@ -39,8 +39,8 @@ the resolver's own doc comment (`:137-140`) asserted the invariant while nothing both paths. What removes the class is a single resolve that owns everything after the source merge, so an entry point's only remaining choice is which sources it layers, and there is no way to obtain a config that did not pass through resolution. That is Option 2 of -[`hardening/proposals/config-closed-domain.md`](../../.security/oidc-exchange/53cbdec9_20260804T102454Z/hardening/proposals/config-closed-domain.md) -(invariant CV2), and it is the half of that proposal this change lands. +the `config-closed-domain` hardening proposal (invariant CV2), and it is the half of that +proposal this change lands. --- @@ -48,9 +48,9 @@ to obtain a config that did not pass through resolution. That is Option 2 of | Canonical page | Nature of change | | --- | --- | -| [`.specs/service/specs/06-configuration.md`](../service/specs/06-configuration.md) | Modify Loading order into a source-layering list plus one shared resolve; reframe `Validation at load` from `load_config` onto the shared resolve; add `Placeholder resolution` (fail-closed table, residual guard, redaction rule), `Configuration entry points`, and `Pre-flight check (config check)` | -| [`.specs/service/specs/04-http-api.md`](../service/specs/04-http-api.md) | Modify Bootstrap steps 1–2 for the `config check` subcommand and the shared resolve; modify the closing `crates/ffi` paragraph to cover configuration, not just routing | -| [`.specs/bindings/specs/01-ffi-core.md`](../bindings/specs/01-ffi-core.md) | Modify Responsibilities: config through `new`/`from_file` passes the server's resolve; add a Decision recording the one-resolve/differing-sources rule | +| [`.specs/service/specs/06-configuration.md`](../../service/specs/06-configuration.md) | Modify Loading order into a source-layering list plus one shared resolve; reframe `Validation at load` from `load_config` onto the shared resolve; add `Placeholder resolution` (fail-closed table, residual guard, redaction rule), `Configuration entry points`, and `Pre-flight check (config check)` | +| [`.specs/service/specs/04-http-api.md`](../../service/specs/04-http-api.md) | Modify Bootstrap steps 1–2 for the `config check` subcommand and the shared resolve; modify the closing `crates/ffi` paragraph to cover configuration, not just routing | +| [`.specs/bindings/specs/01-ffi-core.md`](../../bindings/specs/01-ffi-core.md) | Modify Responsibilities: config through `new`/`from_file` passes the server's resolve; add a Decision recording the one-resolve/differing-sources rule | No new canonical page. The `[providers.]`, `[internal_api]` and Defaults-summary sections of 06-configuration are untouched — no TOML-visible field changes. @@ -186,11 +186,11 @@ Steps 1 and 2 currently read "Honour `--version` …" and "`bootstrap::load_conf > 1. Handle the CLI surface and exit: `--version` prints the crate version; `config check` > layers configuration sources and runs the same resolve as step 2, prints a redacted summary, > and exits non-zero on any `ConfigError` without building adapters or binding a socket -> ([06-configuration.md](06-configuration.md)). +> (see the canonical 06-configuration Bootstrap contract). > 2. `bootstrap::load_config` — layer `config/default.toml`, the > `config/{OIDC_EXCHANGE_ENV}.toml` overlay if set, and `OIDC_EXCHANGE__{section}__{key}` env > overrides, then run the shared resolve: fail-closed `${VAR}` placeholder resolution followed -> by validation ([06-configuration.md](06-configuration.md)). +> by validation (see the canonical 06-configuration Bootstrap contract). ### `.specs/service/specs/04-http-api.md` → Bootstrap, closing paragraph (Modify) @@ -231,7 +231,8 @@ today's `AppConfig` field-for-field. Narrowing the security-relevant fields to c types — `RegistrationMode`, `SigningAlgorithm`, `HttpsUrl`, `AsciiDomainPattern`, typed audit severities — is the other half of the hardening proposal's Option 2 and is deliberately **not** in this change; it hangs off the seam this change creates and is proposed separately in -[`2026-08-05-fail_closed_across_config_and_adapters.md`](2026-08-05-fail_closed_across_config_and_adapters.md). +the sibling `2026-08-05-fail_closed_across_config_and_adapters` change, which is outside this +workspace. --- @@ -286,7 +287,7 @@ in this change; it hangs off the seam this change creates and is proposed separa ## Merge plan 1. The earlier merge this step used to guard has completed: the `Proposed changes` blocks of - [`2026-07-01-complete_config_loading.md`](merged/2026-07-01-complete_config_loading.md) are on + the earlier complete-config-loading change are on the canonical pages — 06-configuration carries the `Validation at load` section and the fail-closed placeholder wording, and 04-http-api's Bootstrap step 2 and internal-route conditions are in place. The blocks above are written against that text as it now stands. @@ -353,7 +354,6 @@ in this change; it hangs off the seam this change creates and is proposed separa Lambda or container environment the addon will run in. - Does the empty-string rejection need a per-field opt-out for a value legitimately supplied as empty through the environment? No shipped config needs one today. -- Merge coordination: - [`2026-08-05-fail_closed_across_config_and_adapters.md`](2026-08-05-fail_closed_across_config_and_adapters.md) +- Merge coordination: the sibling `2026-08-05-fail_closed_across_config_and_adapters` change also modifies 06-configuration's Loading order and rewrites `Validation at load`; whichever of the two merges second must refresh its Modify blocks against the merged page. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/01-shared_config_resolve_boundary.md similarity index 60% rename from .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md rename to .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/01-shared_config_resolve_boundary.md index d826efc..e15cb52 100644 --- a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/01-shared_config_resolve_boundary.md +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/01-shared_config_resolve_boundary.md @@ -1,24 +1,25 @@ # Task 01: Shared config resolve boundary +**Status:** Done **Plan:** [plan.md](../plan.md) -**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Proposed changes / Implementation notes 1–3; [06-configuration.md](../../../service/specs/06-configuration.md) → future Loading order / Configuration entry points +**Implements:** [source spec](../../../changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) → Proposed changes / Implementation notes 1–3; [06-configuration.md](../../../service/specs/06-configuration.md) → future Loading order / Configuration entry points **Depends on:** — **Produces:** one server-owned builder-to-resolved-`AppConfig` tail used by both file-backed configuration and FFI TOML, with environment overrides layered before placeholder resolution and validation. **Pointers:** `crates/server/src/bootstrap.rs:90-128`; `crates/ffi/src/lib.rs:49-81`; `config` 0.15 `File::from_str` already available through `crates/server/Cargo.toml`. ## Steps -- [ ] Extract the post-source-assembly tail from `load_config_from_dir`: build `config::Config`, resolve the merged tree, deserialize the raw shape, validate, and return the runtime config. Make this the only production path containing `try_deserialize` and `validate`. -- [ ] Retain file-backed source layering in `load_config_from_dir` (default file, optional selected overlay, structural environment overrides), then delegate to the shared tail. -- [ ] Rewrite `parse_config` to build from `File::from_str(toml_str, FileFormat::Toml)` plus the same `OIDC_EXCHANGE__…` `Environment` source, then delegate to the shared tail. Remove the direct `toml::from_str` path. -- [ ] Keep `OidcExchange::from_file` as file read → `new`; do not introduce a third configuration pipeline. -- [ ] Add focused regression tests proving FFI parsing resolves `internal_api.shared_secret = "${INTERNAL_API_SECRET}"` to the environment value and never returns the literal, and applies `OIDC_EXCHANGE__REGISTRATION__MODE=existing_users_only` to inline TOML. -- [ ] Preserve the current file-backed happy path and validation tests; run targeted server/FFI tests plus Rust format/clippy checks. +- [x] Extract the post-source-assembly tail from `load_config_from_dir`: build `config::Config`, resolve the merged tree, deserialize the raw shape, validate, and return the runtime config. Make this the only production path containing `try_deserialize` and `validate`. +- [x] Retain file-backed source layering in `load_config_from_dir` (default file, optional selected overlay, structural environment overrides), then delegate to the shared tail. +- [x] Rewrite `parse_config` to build from `File::from_str(toml_str, FileFormat::Toml)` plus the same `OIDC_EXCHANGE__…` `Environment` source, then delegate to the shared tail. Remove the direct `toml::from_str` path. +- [x] Keep `OidcExchange::from_file` as file read → `new`; do not introduce a third configuration pipeline. +- [x] Add focused regression tests proving FFI parsing resolves `internal_api.shared_secret = "${INTERNAL_API_SECRET}"` to the environment value and never returns the literal, and applies `OIDC_EXCHANGE__REGISTRATION__MODE=existing_users_only` to inline TOML. +- [x] Preserve the current file-backed happy path and validation tests; run targeted server/FFI tests plus Rust format/clippy checks. ## Definition of done -- [ ] File-backed and inline FFI TOML configuration both traverse one resolve/deserialize/validate implementation; repository search shows no other production `try_deserialize`/`validate` bypass in configuration entry points. -- [ ] An FFI caller with set `INTERNAL_API_SECRET` gets the resolved secret, not `${INTERNAL_API_SECRET}`; FFI inline TOML receives the documented structural environment override. -- [ ] `OidcExchange::from_file` still delegates through `new`, so Node, Python, and the TypeScript Lambda wrapper inherit the same path without channel-specific patches. -- [ ] Positive and negative regression tests are added or preserved; no secret value is asserted via error output. -- [ ] `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and targeted tests pass. If `cargo test --workspace` is run, record the known three missing `providers.*.adapter` test failures without changing them. +- [x] File-backed and inline FFI TOML configuration both traverse one resolve/deserialize/validate implementation; repository search shows no other production `try_deserialize`/`validate` bypass in configuration entry points. +- [x] An FFI caller with set `INTERNAL_API_SECRET` gets the resolved secret, not `${INTERNAL_API_SECRET}`; FFI inline TOML receives the documented structural environment override. +- [x] `OidcExchange::from_file` still delegates through `new`, so Node, Python, and the TypeScript Lambda wrapper inherit the same path without channel-specific patches. +- [x] Positive and negative regression tests are added or preserved; no secret value is asserted via error output. +- [x] `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and targeted tests pass. The final `cargo nextest run --workspace` result was 391 passed, 27 skipped. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/02-resolver_fail_closed_parity.md similarity index 61% rename from .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md rename to .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/02-resolver_fail_closed_parity.md index 24aec41..db467d7 100644 --- a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/02-resolver_fail_closed_parity.md +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/02-resolver_fail_closed_parity.md @@ -1,24 +1,25 @@ # Task 02: Resolver fail-closed hardening and entry-point parity +**Status:** Done **Plan:** [plan.md](../plan.md) -**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Placeholder resolution / Implementation notes 4–6 and 8; [06-configuration.md](../../../service/specs/06-configuration.md) → future Placeholder resolution +**Implements:** [source spec](../../../changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) → Placeholder resolution / Implementation notes 4–6 and 8; [06-configuration.md](../../../service/specs/06-configuration.md) → future Placeholder resolution **Depends on:** 01 **Produces:** a total, path-aware shared resolver that rejects empty, malformed, and residual unescaped placeholders and a parity table exercising all current configuration entry points. **Pointers:** `crates/server/src/bootstrap.rs:141-237`, especially `resolve_placeholders`, `resolve_placeholders_in_str`, `scan_placeholder_name`; existing tests at `bootstrap.rs:1031-1200`. ## Steps -- [ ] Thread a stable config path through the value-tree walk, including table keys and array indices, so resolution failures name both the environment variable (where applicable) and the configuration location without exposing a resolved value. -- [ ] Reject `Ok("")` from `std::env::var` with wording distinguishable from an unset environment variable. -- [ ] Treat any unescaped `${` with no closing `}` within `PLACEHOLDER_NAME_LEN_MAX` as `ConfigError`; reject `${}` explicitly. Keep `$${` as the only literal escape and preserve its no-lookup guarantee. -- [ ] Add a post-resolution tree pass that rejects residual unescaped `${` while permitting the explicit escape result according to the documented representation; make its traversal bounded/iterative as required by the project guidelines. -- [ ] Add one parity-table test body run through `load_config_from_dir` and `parse_config`, covering set, unset, empty, escaped, unterminated, and empty-name cases. Assert equivalent success/failure semantics, relevant variable/path diagnostics, and absence of secret values. -- [ ] Cover a nested/map-valued path and at least one array path if the `config::Value` representation permits it, so path propagation is not table-only. +- [x] Thread a stable config path through the value-tree walk, including table keys and array indices, so resolution failures name both the environment variable (where applicable) and the configuration location without exposing a resolved value. +- [x] Reject `Ok("")` from `std::env::var` with wording distinguishable from an unset environment variable. +- [x] Treat any unescaped `${` with no closing `}` within `PLACEHOLDER_NAME_LEN_MAX` as `ConfigError`; reject `${}` explicitly. Keep `$${` as the only literal escape and preserve its no-lookup guarantee. +- [x] Add a post-resolution tree pass that rejects residual unescaped `${` while permitting the explicit escape result according to the documented representation; make its traversal bounded/iterative as required by the project guidelines. +- [x] Add one parity-table test body run through `load_config_from_dir` and `parse_config`, covering set, unset, empty, escaped, unterminated, and empty-name cases. Assert equivalent success/failure semantics, relevant variable/path diagnostics, and absence of secret values. +- [x] Cover a nested/map-valued path and at least one array path if the `config::Value` representation permits it, so path propagation is not table-only. ## Definition of done -- [ ] No unescaped `${` can reach a runtime `AppConfig`: valid names resolve to non-empty environment values, unset/empty/malformed/empty-name/residual forms return `ConfigError`, and `$${` yields literal `${` without lookup. -- [ ] Every resolver error names the config path and appropriate variable/token category but never the resolved secret; redacted `Debug` remains the only output route for secret-bearing fields. -- [ ] The same parity cases produce the same outcomes for file-backed and FFI TOML inputs; adding a future entry point has an obvious table hook. -- [ ] Existing valid file-backed resolution remains covered; malformed and empty conditions have paired negative-space tests. -- [ ] Targeted server/FFI tests, `cargo fmt --all --check`, and `cargo clippy --workspace -- -D warnings` pass. Workspace test baseline remains explicitly excluded: do not repair the three missing `providers.*.adapter` tests. +- [x] No unescaped `${` can reach a runtime `AppConfig`: valid names resolve to non-empty environment values, unset/empty/malformed/empty-name/residual forms return `ConfigError`, and `$${` yields literal `${` without lookup. +- [x] Every resolver error names the config path and appropriate variable/token category but never the resolved secret; redacted `Debug` remains the only output route for secret-bearing fields. +- [x] The same parity cases produce the same outcomes for file-backed and FFI TOML inputs; adding a future entry point has an obvious table hook. +- [x] Existing valid file-backed resolution remains covered; malformed and empty conditions have paired negative-space tests. +- [x] Targeted server/FFI tests, `cargo fmt --all --check`, and `cargo clippy --workspace -- -D warnings` pass. The final `cargo nextest run --workspace` result was 391 passed, 27 skipped. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/03-config_check_cli.md similarity index 66% rename from .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md rename to .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/03-config_check_cli.md index d026295..af8efac 100644 --- a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/03-config_check_cli.md +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/03-config_check_cli.md @@ -1,23 +1,24 @@ # Task 03: Config check CLI +**Status:** Done **Plan:** [plan.md](../plan.md) -**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Pre-flight check / Implementation notes 7–8; [04-http-api.md](../../../service/specs/04-http-api.md) → future Bootstrap steps 1–2 +**Implements:** [source spec](../../../changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) → Pre-flight check / Implementation notes 7–8; [04-http-api.md](../../../service/specs/04-http-api.md) → future Bootstrap steps 1–2 **Depends on:** 02 **Produces:** `oidc-exchange config check [--dir ] [--file ]`, a preflight-only caller of the shared resolver that emits a redacted summary on success and exits non-zero on `ConfigError`. **Pointers:** `crates/server/src/main.rs:10-85`; `crates/server/src/bootstrap.rs:58-128`; redacting `Debug` implementations in `crates/core/src/config.rs`; no existing argument-parsing dependency. ## Steps -- [ ] Define and implement the minimal CLI grammar for `config check`, `--dir`, and `--file`, including mutually exclusive/invalid-argument handling; decide whether a small parser dependency or explicit bounded argument parsing best fits the existing binary and document the choice in the PR. -- [ ] Make the file-backed loader callable by the CLI and add a single-file loader using the same inline/file source shape as FFI. Both must delegate to task 01's shared resolver, not reimplement source merging or validation. -- [ ] For `--dir` (default `config/`), layer default, selected overlay, and structural environment overrides. For `--file`, layer that named document plus structural overrides, matching FFI's source semantics. -- [ ] On success, print only the configuration through established redacting `Debug` output and exit before telemetry initialization, adapter construction, router creation, socket binding, or writes. On resolution/validation failure, return non-zero and preserve the safe diagnostic. -- [ ] Add CLI-level tests or a testable command runner covering directory and file successes, unset-placeholder non-zero failure, invalid argument combinations, and output absence of the raw secret. -- [ ] Verify `--version` remains unchanged and Rust Lambda/server startup continues to load configuration before runtime selection. +- [x] Define and implement the minimal CLI grammar for `config check`, `--dir`, and `--file`, including mutually exclusive/invalid-argument handling; decide whether a small parser dependency or explicit bounded argument parsing best fits the existing binary and document the choice in the PR. +- [x] Make the file-backed loader callable by the CLI and add a single-file loader using the same inline/file source shape as FFI. Both must delegate to task 01's shared resolver, not reimplement source merging or validation. +- [x] For `--dir` (default `config/`), layer default, selected overlay, and structural environment overrides. For `--file`, layer that named document plus structural overrides, matching FFI's source semantics. +- [x] On success, print only the configuration through established redacting `Debug` output and exit before telemetry initialization, adapter construction, router creation, socket binding, or writes. On resolution/validation failure, return non-zero and preserve the safe diagnostic. +- [x] Add CLI-level tests or a testable command runner covering directory and file successes, unset-placeholder non-zero failure, invalid argument combinations, and output absence of the raw secret. +- [x] Verify `--version` remains unchanged and Rust Lambda/server startup continues to load configuration before runtime selection. ## Definition of done -- [ ] `oidc-exchange config check` accepts the documented forms, uses the shared resolve exactly once, and does not construct adapters, bind a socket, initialize telemetry, or write state. -- [ ] An unset placeholder exits non-zero and names the safe failure context without printing its raw secret; a successful run prints a redacted summary with `internal_api.shared_secret` and `user_sync.webhook.secret` protected. -- [ ] `--dir` and `--file` reflect their respective source shapes, including `OIDC_EXCHANGE__…` overrides, and invalid CLI combinations fail deterministically. -- [ ] Positive and negative command tests pass along with `cargo fmt --all --check` and `cargo clippy --workspace -- -D warnings`; do not change the known workspace-test baseline failures. +- [x] `oidc-exchange config check` accepts the documented forms, uses the shared resolve exactly once, and does not construct adapters, bind a socket, initialize telemetry, or write state. +- [x] An unset placeholder exits non-zero and names the safe failure context without printing its raw secret; a successful run prints a redacted summary with `internal_api.shared_secret` and `user_sync.webhook.secret` protected. +- [x] `--dir` and `--file` reflect their respective source shapes, including `OIDC_EXCHANGE__…` overrides, and invalid CLI combinations fail deterministically. +- [x] Positive and negative command tests pass along with `cargo fmt --all --check` and `cargo clippy --workspace -- -D warnings`; the final `cargo nextest run --workspace` result was 391 passed, 27 skipped. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/04-document_contract_and_release_notes.md similarity index 69% rename from .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md rename to .specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/04-document_contract_and_release_notes.md index 3fbc322..1a21332 100644 --- a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/backlog/04-document_contract_and_release_notes.md +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/done/04-document_contract_and_release_notes.md @@ -1,24 +1,25 @@ # Task 04: Document the shared-resolve contract and embedding break +**Status:** Done **Plan:** [plan.md](../plan.md) -**Implements:** [source spec](../../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) → Affected spec pages / Proposed changes / Merge plan / Implementation note 9 +**Implements:** [source spec](../../../changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) → Affected spec pages / Proposed changes / Merge plan / Implementation note 9 **Depends on:** 02, 03 **Produces:** canonical service/FFI documentation matching implementation, release notes for Node/Lambda/PyPI embedding users, and change-spec/README merge housekeeping. **Pointers:** `.specs/service/specs/06-configuration.md`; `.specs/service/specs/04-http-api.md`; `.specs/bindings/specs/01-ffi-core.md`; `.specs/README.md`; release-note/changelog locations in `bindings/nodejs`, `bindings/lambda`, and `bindings/python` (identify existing project convention before editing). ## Steps -- [ ] Apply the source spec's precise 06-configuration changes: source layering vs one shared resolve; validation framing; Placeholder resolution table; Configuration entry points; and config-check preflight contract. Preserve untouched field/default sections. -- [ ] Apply the 04-http-api Bootstrap changes and closing FFI paragraph, and the FFI-core Responsibilities and one-resolve Decision exactly against the code shipped in tasks 01–03. -- [ ] Locate existing release-note/changelog conventions for `@oidc-exchange/node`, `@oidc-exchange/lambda`, and PyPI; add a concise behaviour-change note to each: an unresolved/empty/malformed placeholder now fails binding construction instead of becoming literal configuration text. -- [ ] Follow the source spec merge plan only after all implementation/tests are accepted: set the change spec status/merged date, move it to `changes/merged/`, and update README change-spec indexing. Do not create a canonical-type schema update. -- [ ] Update the Plans table in `.specs/README.md` to list this plan and keep its status synchronized with the kanban state. -- [ ] Verify every Markdown relative link, source/back-reference, table target, task dependency, and status after moving the source spec; ensure no done certificate is introduced. +- [x] Apply the source spec's precise 06-configuration changes: source layering vs one shared resolve; validation framing; Placeholder resolution table; Configuration entry points; and config-check preflight contract. Preserve untouched field/default sections. +- [x] Apply the 04-http-api Bootstrap changes and closing FFI paragraph, and the FFI-core Responsibilities and one-resolve Decision exactly against the code shipped in tasks 01–03. +- [x] Locate existing release-note/changelog conventions for `@oidc-exchange/node`, `@oidc-exchange/lambda`, and PyPI; add a concise behaviour-change note to each: an unresolved/empty/malformed placeholder now fails binding construction instead of becoming literal configuration text. +- [x] Follow the source spec merge plan only after all implementation/tests are accepted: set the change spec status/merged date, move it to `changes/merged/`, and update README change-spec indexing. Do not create a canonical-type schema update. +- [x] Update the Plans table in `.specs/README.md` to list this plan and keep its status synchronized with the kanban state. +- [x] Verify every Markdown relative link, source/back-reference, table target, task dependency, and status after moving the source spec; ensure no done certificate is introduced. ## Definition of done -- [ ] 06-configuration, 04-http-api, and FFI-core state the one-resolve/differing-sources invariant, total fail-closed placeholder contract, env overrides, redaction, all entry points, and config-check behaviour exactly as implemented. -- [ ] Release notes cover Node, Lambda, and PyPI embedders; they explain the construction-time compatibility impact without exposing a real secret or inventing new API behaviour. -- [ ] Change spec merge housekeeping and README indexes are internally consistent; no schema file changes because no TOML-visible shape changed. -- [ ] All Markdown links resolve; tasks 01–04 remain DAG-valid with lower-number dependencies; all task checkboxes and plan status accurately reflect actual work state. -- [ ] Done certificates remain intentionally absent: no `done/` directory and no certificate file is created. +- [x] 06-configuration, 04-http-api, and FFI-core state the one-resolve/differing-sources invariant, total fail-closed placeholder contract, env overrides, redaction, all entry points, and config-check behaviour exactly as implemented. +- [x] Release notes cover Node, Lambda, and PyPI embedders; they explain the construction-time compatibility impact without exposing a real secret or inventing new API behaviour. +- [x] Change spec merge housekeeping and README indexes are internally consistent; no schema file changes because no TOML-visible shape changed. +- [x] All Markdown links resolve; tasks 01–04 remain DAG-valid with lower-number dependencies; all task checkboxes and plan status accurately reflect actual work state. +- [x] Done certificates remain intentionally absent: no `done/` directory and no certificate file is created. diff --git a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md index 14d7d3d..2962983 100644 --- a/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md +++ b/.specs/plans/2026-08-05-resolve_config_placeholders_all_channels/plan.md @@ -1,6 +1,6 @@ # Plan: Resolve `${VAR}` placeholders on every configuration entry point -**Status:** Review · **Layout:** kanban · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Source spec:** [`.specs/changes/2026-08-05-resolve_config_placeholders_all_channels.md`](../../changes/2026-08-05-resolve_config_placeholders_all_channels.md) +**Status:** Done · **Layout:** kanban · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Source spec:** [`.specs/changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md`](../../changes/merged/2026-08-05-resolve_config_placeholders_all_channels.md) Build one configuration resolve boundary for every source shape: file-backed server/Lambda configuration and inline/file FFI configuration. The resolver owns placeholder rejection, env overrides, deserialization, and validation; entry points own only source layering. The plan then exposes the same path through `oidc-exchange config check`, documents the shipped contract and publishes embedding release notes. Node, Python, and `@oidc-exchange/lambda` are covered through their existing FFI chain; this plan deliberately does not duplicate binding implementations. @@ -12,7 +12,7 @@ Build one configuration resolve boundary for every source shape: file-backed ser - **Already built.** `bootstrap::load_config_from_dir` layers default/overlay/environment sources, directly resolves placeholders, deserializes, and validates; `bootstrap::parse_config` directly uses `toml::from_str` then validation, omitting placeholders and structural overrides. `OidcExchange::new` is the sole configuration path for napi/PyO3 and `OidcExchange::from_file` delegates to it. `main.rs` recognizes only `--version`; no CLI parser or `config check` exists. Existing resolver tests cover file-backed set/unset/escaped/nested cases, but there is no entry-point parity table or malformed/empty/residual test. - **Known baseline failure.** `cargo test --workspace` is already red because three `providers.*.adapter` configuration tests are missing. This unstacked PR neither fixes nor absorbs that unrelated failure. Each task reports that failure separately if it appears; targeted tests and non-test checks remain required evidence. - **Definition of done.** Every task inherits [`.specs/development-guidelines.md`](../../development-guidelines.md) §Definition of done: positive and negative-space tests, meaningful assertions in touched functions, named bounds, and Rust format/clippy/test checks. Since this scope touches Rust only, run `cargo fmt --all --check`, `cargo clippy --workspace -- -D warnings`, and targeted package/test commands during task work; attempt `cargo test --workspace` at integration and record the three pre-existing missing-provider-adapter test failures without changing them. -- **Done certificates.** Intentionally omitted by explicit instruction. This plan creates no `done/` directory and no `*-certificate.md` files; task checklists, review evidence, and the plan-level DoDs are the sole tracking artifacts. +- **Done certificates.** Intentionally omitted by explicit instruction. Task packages move from `backlog/` to `done/` as kanban records, but no `*-certificate.md` files are created; task checklists, review evidence, and the plan-level DoDs are the sole tracking artifacts. --- @@ -35,6 +35,14 @@ The dependency table is the **source of truth**; the Mermaid graph visualizes it | 03 · config check CLI | 02 | build | `oidc-exchange config check [--dir ] [--file ]` resolves and validates without adapters, socket binding, or writes, and prints a redacted summary | | 04 · canonical docs + embedding release notes | 02, 03 | review | 06-configuration, 04-http-api, and FFI-core describe the shipped one-resolve contract; Node/Lambda/PyPI release notes explain construction can now fail for unresolved placeholders | +## Kanban + +| Status | Task packages | +|---|---| +| Done | [01 · shared config resolve boundary](done/01-shared_config_resolve_boundary.md); [02 · resolver fail-closed hardening and entry-point parity](done/02-resolver_fail_closed_parity.md); [03 · config check CLI](done/03-config_check_cli.md); [04 · document the shared-resolve contract and embedding break](done/04-document_contract_and_release_notes.md) | + +All four task packages are complete. The source spec is merged, and task back-references target its merged location. + Each `Depends on` references lower-numbered tasks only. Task 01 defines the sole production seam; task 02 strengthens and proves its contract; task 03 consumes that proven seam; task 04 must be reviewed against both implementation and CLI behaviour before it describes either as canonical fact. --- @@ -57,7 +65,7 @@ Each `Depends on` references lower-numbered tasks only. Task 01 defines the sole **Out of scope:** closed-domain config types and adapter validation, new TOML fields/schema changes, binding API redesigns, a hermetic FFI opt-out, environment simulation beyond the checking process, and the unrelated three missing `providers.*.adapter` tests. -**Sibling dependency / merge coordination:** [`2026-08-05-fail_closed_across_config_and_adapters.md`](../../changes/2026-08-05-fail_closed_across_config_and_adapters.md) is a sibling hardening change, not a prerequisite to build this PR. The sibling spec is not present in this unstacked workspace; this reference records the declared coordination dependency only. It deliberately owns narrowing security-relevant fields to closed types and also edits 06-configuration's Loading order and Validation at load. Do not fold any of that work into tasks 01–04. Whichever unstacked PR merges second must refresh its Modify blocks against the then-current canonical page. +**Sibling dependency / merge coordination:** The sibling `2026-08-05-fail_closed_across_config_and_adapters` hardening change is not a prerequisite to build this PR. Its spec is not present in this unstacked workspace; this reference records the declared coordination dependency only. It deliberately owns narrowing security-relevant fields to closed types and also edits 06-configuration's Loading order and Validation at load. Do not fold any of that work into tasks 01–04. Whichever unstacked PR merges second must refresh its Modify blocks against the then-current canonical page. **Open questions retained for owner decision (not implementation blockers):** @@ -66,3 +74,12 @@ Each `Depends on` references lower-numbered tasks only. Task 01 defines the sole 3. Whether a future per-field opt-out is needed for intentionally empty environment substitutions; no shipped config needs one. **Decisions fixed by the source spec:** fail closed immediately; reject empty and malformed placeholders; `$${` is the literal escape; no raw secret in errors or diagnostic output; config check ships in this PR; no schema/type-narrowing work here. + +--- + +## Completion evidence + +- Implementation completed in `0165e369` (`feat(config): resolve placeholders across all configuration entry points`); documentation completed in `d6098ba4` (`docs(config): document shared placeholder resolution`). +- Independent review gate passed. +- Final verification: `cargo nextest run --workspace` — **391 passed, 27 skipped**. +- Markdown task and source-spec links were validated locally; no certificate files were created. diff --git a/.specs/service/specs/04-http-api.md b/.specs/service/specs/04-http-api.md index daf622a..30950b8 100644 --- a/.specs/service/specs/04-http-api.md +++ b/.specs/service/specs/04-http-api.md @@ -1,6 +1,6 @@ # HTTP API, Roles, and Bootstrap -**Status:** Implemented · **Date:** 2026-07-02 · **Owner:** Ant Stanley · **Scope:** crates/server +**Status:** Implemented · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Scope:** crates/server The axum layer: routes, middleware, the `role`-based route/adapter selection, the startup sequence, and the domain-error-to-HTTP mapping. Lives in `crates/server/src/`. @@ -94,10 +94,14 @@ be network-isolated independently from one binary. ## Bootstrap (`main.rs` + `bootstrap.rs`) -1. Honour `--version` (prints the crate version and exits). -2. `bootstrap::load_config` — load `config/default.toml`, overlay - `config/{OIDC_EXCHANGE_ENV}.toml` if set, apply `OIDC_EXCHANGE__{section}__{key}` env - overrides, resolve `${VAR}` placeholders ([06-configuration.md](06-configuration.md)). +1. Handle the CLI surface and exit: `--version` prints the crate version; `config check` + layers configuration sources and runs the same resolve as step 2, prints a redacted summary, + and exits non-zero on any `ConfigError` without building adapters or binding a socket + ([06-configuration.md](06-configuration.md)). +2. `bootstrap::load_config` — layer `config/default.toml`, the + `config/{OIDC_EXCHANGE_ENV}.toml` overlay if set, and `OIDC_EXCHANGE__{section}__{key}` env + overrides, then run the shared resolve: fail-closed `${VAR}` placeholder resolution followed + by validation ([06-configuration.md](06-configuration.md)). 3. `telemetry::init_telemetry` — install the tracing subscriber first so all later spans are captured ([07-telemetry-and-audit.md](07-telemetry-and-audit.md)). 4. `bootstrap::build_service` — construct adapters by role and assemble `AppService`. @@ -109,8 +113,9 @@ be network-isolated independently from one binary. stack's request-timeout layer bounds slow clients at `server.request_timeout` (default 30 s). -`crates/ffi` calls the same `build_service` / `build_router` path, so in-process bindings get -identical routing and middleware. +`crates/ffi` layers its own sources into the same resolve and then calls the same +`build_service` / `build_router` path, so in-process bindings get identical configuration +semantics, routing, and middleware. ## Error mapping (`error.rs`) diff --git a/.specs/service/specs/06-configuration.md b/.specs/service/specs/06-configuration.md index 67b1b8f..415c260 100644 --- a/.specs/service/specs/06-configuration.md +++ b/.specs/service/specs/06-configuration.md @@ -1,19 +1,102 @@ # Configuration -**Status:** Implemented · **Date:** 2026-07-02 · **Owner:** Ant Stanley · **Scope:** crates/core/src/config.rs, config/ +**Status:** Implemented · **Date:** 2026-08-05 · **Owner:** Ant Stanley · **Scope:** crates/core/src/config.rs, config/, crates/server/src/bootstrap.rs One TOML file drives the whole service. `AppConfig` (and its nested structs) in `crates/core/src/config.rs` deserializes it; every section uses `#[serde(default)]`, so any omitted section falls back to its defaults. -## Loading order (`bootstrap::load_config`) +## Loading order + +Configuration reaches a running service through exactly one pipeline. An entry point chooses +which *sources* it layers; everything after the merge is the shared resolve, which every entry +point calls and none can bypass. + +Sources, lowest precedence first: 1. `config/default.toml` — compiled-in defaults (committed; see below). -2. `config/{OIDC_EXCHANGE_ENV}.toml` — overlay when `OIDC_EXCHANGE_ENV` is set (e.g. - `production`, `sqlite-only`); examples ship several named environments. -3. `OIDC_EXCHANGE__{section}__{key}` environment variables — structural overrides. +2. `config/{OIDC_EXCHANGE_ENV}.toml` — deep-merged overlay when `OIDC_EXCHANGE_ENV` is set + (e.g. `production`, `sqlite-only`); tables merge recursively, scalars and arrays replace. + File-backed entry points only. +3. `OIDC_EXCHANGE__{section}__{key}` environment variables — structural overrides reaching + every config path, including map-valued sections. A double underscore separates path + segments and each segment is lowercased; a single underscore stays inside its segment + (`…__MY_IDP__…` targets `providers.my_idp`), so keys whose names themselves contain `__` + cannot be addressed from the environment. + +The shared resolve then runs over the merged tree, for every entry point: + 4. `${VAR_NAME}` placeholders anywhere in the merged config are resolved from the environment - (used for secrets and per-deployment values). + (see [Placeholder resolution](#placeholder-resolution)). +5. The resolved config is validated — `server.role`, the duration strings, allowlist entry + shape, and a non-empty `internal_api.shared_secret` when the internal API will be served + (see [Validation at load](#validation-at-load)) — and a failure aborts before any adapter or + router is built. + +Steps 4 and 5 are one function. Deserializing the merged tree yields the raw config; only the +resolve produces the `Config` the runtime consumes, so a code path that skipped resolution has +nothing to hand `build_service`. + +## Placeholder resolution + +`${VAR_NAME}` in any string value, at any depth, is replaced with that environment variable's +value. Resolution is total and fail-closed: every placeholder either resolves to a real value +or aborts the load with a `ConfigError`. Literal placeholder text never reaches a running +service. + +| Input | Outcome | +| --- | --- | +| `${NAME}`, `NAME` set to a non-empty value | replaced with the value | +| `${NAME}`, `NAME` unset | `ConfigError` naming `NAME` and the config path; the load produces no config | +| `${NAME}`, `NAME` set to the empty string | `ConfigError` naming `NAME`, worded to distinguish "set but empty" from "unset" | +| `${` with no closing `}` within 256 bytes | `ConfigError` naming the config path and the malformed placeholder | +| `${}` — empty name | `ConfigError` naming the config path | +| `$${` | the escape: rewritten to a literal `${`, never looked up in the environment | + +An empty variable is rejected rather than substituted because the fields this idiom exists for +are the ones where an empty value means "no protection": an unpopulated secret-manager +reference is a plumbing failure, not an operator's intent. A value that is genuinely meant to +be empty is expressed by omitting the key (defaults apply) or by writing `""` in the TOML. + +After resolution, no config value may still contain an unescaped `${`. This holds as a +post-condition on the resolved tree, so a value carrying placeholder text is a load failure +whatever assembled it. + +Errors raised during resolution or validation name the environment variable and the config +path, never the resolved value. `internal_api.shared_secret` and `user_sync.webhook.secret` +stay redacted on every error and diagnostic path, exactly as they are in `Debug`. + +## Configuration entry points + +| Entry point | Sources layered | Code | +| --- | --- | --- | +| Standalone server (hyper) | 1 + 2 + 3 | `crates/server/src/main.rs` → `bootstrap::load_config` | +| Lambda runtime (same binary, `AWS_LAMBDA_RUNTIME_API` present) | 1 + 2 + 3 | `crates/server/src/main.rs` → `bootstrap::load_config` | +| `config check` subcommand | 1 + 2 + 3, or a single named file | `crates/server/src/main.rs` | +| FFI inline TOML (`OidcExchange::new`) | the supplied document + 3 | `crates/ffi/src/lib.rs` → `bootstrap::parse_config` | +| FFI file (`OidcExchange::from_file`) | the named file + 3 | reads the file, then `new` | +| Node binding (napi) | via the FFI entry points | `bindings/nodejs/src/lib.rs` | +| Python binding (PyO3) | via the FFI entry points | `bindings/python/src/lib.rs` | +| `@oidc-exchange/lambda` handler | via the Node binding | `bindings/lambda/src/index.ts` | + +Every row ends in the same resolve, so placeholder handling, override handling, and rejection +behaviour are identical across channels. The `OIDC_EXCHANGE_ENV` overlay is the one legitimate +difference: it applies only where the service selects its own files, and an FFI caller supplies +the whole document, so there is nothing to overlay it onto. + +## Pre-flight check (`oidc-exchange config check`) + +``` +oidc-exchange config check [--dir ] [--file ] +``` + +`config check` layers the sources for the shape being checked — `--dir` (default `config/`) for +the server layering, `--file` for the single-document layering the bindings use — runs the same +resolve, and exits without constructing an adapter, binding a socket, or writing anything. +Exit `0` prints a summary of the resolved configuration with every secret-bearing field +rendered through its redacting `Debug`; any `ConfigError` exits non-zero with the message the +server would have printed at startup. It is the supported way to prove that a deployment's +environment satisfies its placeholders before the deployment happens. ## Committed default (`config/default.toml`) @@ -94,6 +177,20 @@ retries? }`. The `secret` is redacted in `Debug`. `adapter` (`oidc` | `apple`) plus adapter-specific fields captured via a flattened `extra: HashMap`. See [05-provider-system.md](05-provider-system.md). +## Validation at load + +After merging and placeholder resolution, the shared resolve validates the result and refuses +to produce a config on failure (`ConfigError`): + +- `server.role` must be `all`, `exchange`, or `admin`. +- Duration strings for request and token lifetimes must parse and fit their accepted bounds. +- Domain-allowlist entries must be exact domains or `*.domain` wildcards. +- A served internal API requires a non-empty `internal_api.shared_secret`. + +Validation is a step of the shared resolve, so it runs identically on every entry point in +[Configuration entry points](#configuration-entry-points) — including config supplied as a +string through the FFI bindings (`bootstrap::parse_config`). + ## Defaults summary | Setting | Default | diff --git a/bindings/lambda/README.md b/bindings/lambda/README.md index c0fe057..bc8f9a9 100644 --- a/bindings/lambda/README.md +++ b/bindings/lambda/README.md @@ -38,6 +38,12 @@ export const handler = createHandler({ `createHandler(options)` returns an `async (event, context) => result` handler. `options` are the same as [`OidcExchange`](https://www.npmjs.com/package/@oidc-exchange/node) (`config` / `configString`) plus an optional `basePath`. +## Configuration behaviour change + +Handler construction now fails when a `${VAR}` placeholder is unresolved, empty, or malformed +instead of using that placeholder as literal configuration text. Set every referenced environment +variable in the Lambda runtime before calling `createHandler`. + ## Deploy See the [AWS Lambda deployment guide](https://github.com/antstanley/oidc-exchange/blob/main/docs/integration/aws-lambda.md) and the [Lambda example](https://github.com/antstanley/oidc-exchange/tree/main/examples/nodejs/lambda). The same service also runs as a long-lived server — see the [main repository](https://github.com/antstanley/oidc-exchange). diff --git a/bindings/nodejs/README.md b/bindings/nodejs/README.md index cf7b553..a4a1b76 100644 --- a/bindings/nodejs/README.md +++ b/bindings/nodejs/README.md @@ -77,6 +77,12 @@ Wiring for popular Node servers lives in the main repo's [examples](https://gith Configuration is TOML — providers, token TTLs, registration policy, key management, and storage. See the [configuration guide](https://github.com/antstanley/oidc-exchange#configuration). +### Behaviour change + +Construction now fails when a `${VAR}` placeholder is unresolved, empty, or malformed instead of +using that placeholder as literal configuration text. Set every referenced environment variable +before constructing `OidcExchange`. + ## Links - [Repository & full docs](https://github.com/antstanley/oidc-exchange) diff --git a/bindings/python/README.md b/bindings/python/README.md index ca069b5..1e95764 100644 --- a/bindings/python/README.md +++ b/bindings/python/README.md @@ -79,6 +79,12 @@ See the main repo's [Python examples](https://github.com/antstanley/oidc-exchang TOML config — providers, token TTLs, registration policy, key management, and storage. See the [configuration guide](https://github.com/antstanley/oidc-exchange#configuration). +### Behaviour change + +Construction now fails when a `${VAR}` placeholder is unresolved, empty, or malformed instead of +using that placeholder as literal configuration text. Set every referenced environment variable +before constructing `OidcExchange`. + ## Links - [Repository & full docs](https://github.com/antstanley/oidc-exchange)