From 25cb3c47e7f904d607871a1e87264f31f6572c0e Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 15 Oct 2026 22:11:44 +0000 Subject: [PATCH] Record source version review findings --- docs/roadmap/remediation.md | 204 +++++++++++++++++++++++++++++++++++- 1 file changed, 203 insertions(+), 1 deletion(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index b38f7bb..19bcd8b 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,4 +1,4 @@ -# Destination review findings +# ConfigKit review findings Reviewed `d3d468a..945e111` against the approved [input-destinations roadmap](input-destinations.md), @@ -145,3 +145,205 @@ At `10edd95d4d9060bd46db51e0d6782c12dbda3120`, using - `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...`. The review changed only this findings document; it did not implement fixes. + +## Shared source-version review (2026-10-15) + +Reviewed `70c95bf..020d720` against the +[source-version roadmap](source-version-checks.md), completed +[implementation plan](implementation.md), and +[ADR 0013](../adr/0013-shared-source-version-checks.md). + +The root-owned mode and evidence records delegate to private per-load checking +state. Main's local header is retained, accepted non-main headers are removed +before placement/composition, and the destination-prefixed main rule reaches +payload protection and typed conversion. Warnings, raw accounting, source +contexts, and final loading reuse the existing pipeline. Four must-address +issues remain. Findings 1 and 2 above remain completed historical findings. + +### 3. Preserve distinct semantic shapes during evidence classification (P1) + +Status: open. + +Affected: `classifySecretPathsWithEvidence` and `addSecretEvidencePath` in +[secret.go](../../internal/engine/secret.go), lines 78–104 at `020d720`. + +Evidence candidates are stored in `map[string]Path`, keyed only by pointer text. +Adding a later candidate overwrites an earlier semantic Path with the same +pointer. A numeric mapping key and sequence index therefore compete for one +classification candidate. If the last shape is incompatible with a schema map, +`schemaSecretRoot` misses the secret field tag on the earlier mapping-shaped +header. Final source inspection can disclose its accepted text. + +Reproduction through `LoadLayered`: + +- Select required mode, `ParsePath("/meta/0/version")`, and a checker accepting + every declaration. +- The final Go type has ordinary `Meta map[string]Header` and + `Data.Meta map[string]SecretHeader`, with + `SecretHeader.Version string` tagged `yaml:"version" configkit:"secret"`. +- Main is `meta: {"0": {version: main}}` plus `data: {meta: null}`. +- Mount two separate underlay tiers at `Keys("data")`. The first document is + `meta: {"0": {version: KEY-HEADER-CANARY}}`; the second is + `meta: [{version: INDEX-HEADER-CANARY}]`. + +Both headers are accepted and consumed. Main's null replaces their remaining +payload, so final decoding succeeds. The mapping header's source record exposes +`Text: "KEY-HEADER-CANARY"` with `Redacted: false`, despite its schema tag. +Reversing the underlay order redacts that same header; the classification result +depends on which shape overwrites the candidate, not the established sensitivity +policy. Three repeated runs reproduced this difference. The canaries are +disposable fixture text, not real credentials. + +Required remediation: + +- Retain/evaluate all distinct original semantic candidate shapes before + combining their classification results under the existing sensitivity policy. + Preserve both evidence and payload candidates; replacing one shape with + another must not erase applicable schema or predicate classification. +- Reuse historical/dependency sensitivity closure and conservative failure + disclosure. Do not fix this by exposing less evidence, forbidding numeric + keys/sequence headers, or requiring fragment conformance to the final Go type. +- Add a public regression covering both acquisition orders, the tagged consumed + header, and unchanged final payload. Include a focused shape-collision test at + the classification boundary and reuse existing neighboring-public-value tests. +- Run source-version/sensitivity/history tests, the everyday check, and the full + race suite. Reconcile the plan's Stage 9–12 completion evidence. + +### 4. Preserve import opt-in before stripping source directives (P2) + +Status: open. + +Affected: `assembleBaseInputTrackedForRole` in +[load_input.go](../../internal/engine/load_input.go), lines 54–87, and +`processSourceVersion` in +[source_version.go](../../internal/engine/source_version.go), at `020d720`. + +Enabled source checking calls `splitImportDirectives` regardless of Imports. +For an input with imports disabled, it then checks `disabledDirectiveDiagnostics` +on the already stripped application payload. A valid root directive disappears +without either traversal or the required `import.disabled` error. + +For example, load bytes containing: + +```yaml +version: main +configkit: + imports: + - path: missing.yml +value: ok +``` + +Use a version rule at `Keys("version")`, an accepting checker, and leave Imports +false. Default mode correctly returns `import.disabled`. Both enabled modes +incorrectly return a successful configuration, silently omitting the requested +import. This changes import semantics even though the new mode selects only +version checking. + +Required remediation: + +- Enforce the existing directive opt-in against original source content before + stripping/consumption can hide it. Cover both ordinary main and direct underlay + acquisition through their shared boundary. Keep genuine enabled imports and + selected-overlay restrictions unchanged. +- Add a focused public regression for enabled modes with Imports false and + source-root imports. Keep default-mode behavior and safe source-local failure + attribution; do not add a second implicit import-enabling mechanism. +- Run source-version/directive/import tests and the everyday check. + +### 5. Retain the complete requested path for absent declaration evidence (P2) + +Status: open. + +Affected: `resolveSourceVersionPath` and `sourceVersionCheckForEvidence` in +[source_version.go](../../internal/engine/source_version.go), especially lines +74–90 at `020d720`; evidence classification uses the same partial-path fallback. + +Lookup retains only the prefix reached before absence or obstruction, and that +prefix is still a valid Path. Public evidence therefore selects it instead of +the complete configured header path. For a rule at `Keys("meta", "version")`, +an optional underlay containing `meta: false` produces `Present: false` but +`VersionCheck.Path == "/meta"`, not `/meta/version`. Missing an early member or +an intermediate sequence index likewise discards the remaining suffix. The +record identifies an existing ancestor or partial path rather than the header +whose absence was checked. + +Required remediation: + +- Preserve the full requested local path when lookup stops early. Resolve the + traversed prefix's key/index intent using actual source shape and retain + unresolved suffix intent without guessing nonexistent container kinds. +- Keep nearest physical Location separate from the declaration Path. Reuse the + complete path consistently in successful/callback/failure evidence and its + classification candidates; do not change omission policy or invent authored + coordinates below the obstruction. +- Add bounded regressions for a missing early member, scalar obstruction, and + unavailable intermediate index, including successful optional and failed + required loads. Reuse existing present-path and copy-ownership coverage. +- Run source-version/path/source-record tests and the everyday check. + +### 6. Apply structural-overlap rejection only to an actual declaration (P2) + +Status: open. This is an unintended strengthening of the agreed restriction. + +Affected: `processSourceVersion` in +[source_version.go](../../internal/engine/source_version.go), lines 197–219 at +`020d720`. + +The structural-overlap test runs before handling absence and examines the +partially resolved prefix. A valid scalar `extends` control obstructing a deeper +header lookup is treated as a present overlapping declaration, even though +lookup correctly classified the requested header as absent. The roadmap's +accepted overlap restriction concerns an actual header/control overlap; missing +headers remain governed by the selected mode. + +Reproduction: choose if-present mode and +`Keys("entries", "child", "extends", "version")`, with a checker accepting +main absence. Mount main `{}` at `Keys("main")`. Mount a separate underlay at +`Keys("under")` containing: + +```yaml +entries: + base: {value: good} + child: {extends: base} +``` + +The underlay has no declaration, so its checker should be skipped and its valid +inheritance should remain available. Its inheritance paths do not intersect +main's destination-prefixed protected path. Instead, the load stops with +`version.error` reporting a structural overlap. This rejects an otherwise valid +optional omission, not a present value serving two structural roles. +The same fixture succeeds in default mode. + +Required remediation: + +- Base declaration-overlap rejection on actual header presence. Continue + validating malformed source controls independently, and preserve remaining + payload/inheritance ownership checks after omission handling. +- Keep optional non-main absence permissible and required absence a missing + declaration failure. Do not widen the accepted restriction into a ban on all + configured paths beneath directive spellings. +- Add a focused load-level regression with separate mounted scopes as above, + plus missing-required behavior. Reuse present-overlap and malformed-control + tests to ensure genuine conflicts remain rejected. +- Run source-version/directive/inheritance tests and the everyday check. + +### Shared source-version review validation + +At `020d72084d5988e958c46381724e5dad78cada3d`, using +`go1.26.9 linux/amd64`, these existing checks passed: + +- `GOTOOLCHAIN=local ./scripts/check.sh`. +- `GOTOOLCHAIN=local GOPROXY=off go test -race ./...`. +- `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...`. + +Four additional external-package regression probes reproduced findings 3–6 in +an isolated temporary module outside this repository, with a local replace to +the reviewed checkout. The full probe command was +`GOTOOLCHAIN=local GOPROXY=off GOWORK=off go test -mod=mod -count=1 -v ./...`; +the classification and absent-control cases also reproduced with `-count=3`. +The module was located at `/tmp/configkit-source-version-review.L7YGaM` during +this review; the descriptions above preserve the cases independently of that +temporary directory. + +No production fixes, repository regression tests, dependency updates, or release +changes were made. Only this findings document was revised.