From e0f9541f5fe2757432144ad8b29931d1b75b69ee Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 6 Sep 2026 19:03:00 -0500 Subject: [PATCH] Preserve secret tags in dynamic configuration values --- docs/config.md | 4 +- docs/roadmap/remediation.md | 242 ++++++++++++++++++++++++++++++++++ internal/document/document.go | 13 +- load.go | 2 +- projection.go | 9 ++ render_test.go | 51 +++++++ secret.go | 12 +- 7 files changed, 325 insertions(+), 8 deletions(-) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/config.md b/docs/config.md index 40399af..c979d25 100644 --- a/docs/config.md +++ b/docs/config.md @@ -113,7 +113,9 @@ selected profile, then synthetic CLI records in supplied order. ### Secret classification and effective rendering Secret classification combines `configkit:"secret"` field tags with the optional -pure `Definition.Secret(Path) bool` predicate. A tagged or predicate-classified +pure `Definition.Secret(Path) bool` predicate. Tags on concrete structs inside +supported dynamic values are retained in typed projections, including superseded +defaults and pre-resolution values. A tagged or predicate-classified container makes all descendants sensitive. ConfigKit evaluates final and historical paths, so replacing or deleting a definition does not erase its classification. The predicate can add sensitivity but cannot declassify a tagged path. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..b220c99 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,242 @@ +# Implementation Review Remediation + +## Status and scope + +Status: in progress. Review baseline: `f42ecc70c330a399aa6dada87658694402a515ec`. +This review covers the initial implementation against the +[architecture policy](../policy/architecture.md), [configuration contract](../config.md), +and [implementation plan](implementation.md). The package boundaries and principal +lifecycle are in place, but the findings below must be addressed before claiming +that the initial target is fully satisfied. No production code was changed during +this review. All findings below were reproduced through the public API or local +validation tools, with the additional affected call sites identified by inspection. + +Priorities: P1 means high-impact confidentiality, isolation, or availability; +P2 means a correctness or validation gap that also requires remediation. +Each item remains open until its regression checks and the relevant baseline pass. +Keep current contracts and implementation evidence accurate when fixing these issues; +do not change the agreed semantics simply to accommodate the implementation. + +## 1. P1 — Honor secret tags inside supported dynamic concrete values + +Status: complete. Typed projections now retain concrete field sensitivity; final, +baseline and pre-resolution projections feed historical/dependency classification. +Dynamic nested, replaced, deleted and introduced-value regressions pass. Full tests +pass with the temporary canonical-TMPDIR workaround for issue 8; vet, build and +whitespace checks pass. + +Evidence: [secret.go](../../secret.go), `schemaSecretRoot` stops at +`descriptorDynamic`; [projection.go](../../projection.go), `dynamicDescriptor` +accepts concrete structs and describes their fields, but their sensitivity is not +carried into the classification pass. + +Reproduction: define `Sensitive` with a string field +`Token` tagged `yaml:"token" configkit:"secret"`. Return +`Config{Payload: Sensitive{Token: "review-canary"}}` from defaults, where `Payload` +is `any`, and load with `NoSource()`. Loading succeeds. `WriteEffective` prints +`payload.token` in plaintext, and `Explain(Keys("payload", "token"))` returns that +same text with `Redacted=false`. This does not involve a typed `Secret` reference; +it breaks the separate field-tag classification contract for accepted Go values. + +Required work: preserve sensitivity from concrete dynamic descriptors through +projection and historical classification, including values subsequently replaced +or deleted and dependency propagation. Respect existing supported value shapes; +do not silently ignore tags or require callers to duplicate them in a predicate. + +Acceptance: canary tests for dynamic structs, nested containers, defaults and +recorded resolution must prove redaction in effective output and final/historical +explanation. Include replacement/deletion and derived dependencies. Typed secret +references must remain prohibited inside dynamic values. + +## 2. P1 — Reject nonregular source files before a blocking open + +Evidence: [source.go](../../source.go), `readSource`, calls `os.Open` before +checking regular-file status. [imports.go](../../imports.go), `readImport`, and +[profile.go](../../profile.go), `readProfileFile`, likewise open before checking. + +Reproduction: create a POSIX FIFO with no writer and call +`Load[map[string]any](File(fifoPath))`. The child process remained blocked until +terminated after two seconds; no nonregular-file diagnostic was returned. Byte +limits do not help because the open itself blocks. + +Required work: inspect canonical targets before opening and reject nonregular +files while retaining containment and post-open checks. Apply the protection to +selected files, imports, and profiles. This fits the existing stable-filesystem +contract and does not require introducing a cancellation API or promising hostile +filesystem race protection. Explicit `Reader` inputs retain their stream behavior. + +Acceptance: subprocess-based FIFO regressions on supported POSIX platforms must +finish promptly with safe nonregular-file diagnostics for all three paths. Keep +regular-file, permitted symlink, containment, and permission tests passing. + +## 3. P1 — Detach mutable values introduced by resolution before publication + +Evidence: [resolution.go](../../resolution.go), `Default`/`Normalize`, directly +assign supplied values; `runResolution` projects them but does not clone the typed +object. [load.go](../../load.go) publishes that same object in `Result.Config`. + +Reproduction: a reusable resolver normalizes a map field to one captured map and +records the assignment through `Normalize`. Perform two sequential `NoSource` +loads with this definition. Changing the first result's map changes the second +result's map. Both loads succeed, while the second result's frozen inspection +still shows the earlier value. There is no concurrent callback use or retained +callback context in this reproduction. + +Required work: establish detached typed ownership after application resolution, +covering direct recorded assignments as well as helpers. Preserve resolved-secret +semantics and avoid extra environment reads; cloning only defaults is insufficient. +Ensure validators do not reintroduce equivalent-valued mutable aliases that bypass +the projection-based mutation check and escape into published results. + +Acceptance: repeated loads with shared resolver-provided maps, slices, pointers, +and nested values remain independent of each other and captured application state. +Keep final projection, secret lookup, and validation ordering consistent. + +## 4. P2 — Reconcile history replay with final-only default application + +Evidence: [explain.go](../../explain.go), `Explain` initializes replay from defaults +and merges each input into it. Actual decoding applies defaults to the final +composed/overlaid input, so intermediate replacements are not lasting default resets. + +Reproduction: defaults contain `m: {a: 1}`, base YAML contains `m: null`, and a +selected profile contains `m: {b: 2}`. The successful typed result correctly contains +both `a: 1` and `b: 2`. Explanation of `/m` ends its history at only `b: 2`. +Explanation of `/m/a` reports its removal by the base and never explains its return, +even though its effective value is `1` with a default origin. + +Required work: reconstruct the input-layer history without conflating it with +incremental typed-default application. Explicitly account for effective default +retention/reappearance and canonical decoding so history and final inspection agree. +Preserve visibility of superseded policy-valid inputs, even when they were not +valid for the final destination type. Do not change decoding to retain reset markers. + +Acceptance: cover null-to-map and scalar-to-map overlays above defaulted containers, +including CLI overlays and descendant queries. The historical account must explain +the effective value and origin without implying that a final-present path is absent. + +## 5. P2 — Replay newly created resolution paths + +Evidence: [explain.go](../../explain.go), `replaceHistoryPath`, returns immediately +when a mapping entry is absent, even when the replacement is a newly created value. + +Reproduction: load `m: {}` into a string-keyed integer map. In the resolver, set +`m["new"] = 7` and record `/m/new` with `ChangeDerivedDefault`. Loading succeeds. +`Explain(Keys("m", "new"))` reports effective YAML `7`, but the recorded resolution +change has `After.Present=false` and empty YAML. Parent explanation also misses +that insertion during replay. + +Required work: apply recorded additions as well as replacements and deletions, +including creation of necessary mapping ancestors when justified by the audited +change. Handle overlapping parent/child records without inventing assignment order +or duplicating effects; the contract records net domain changes. + +Acceptance: explain added map entries and nested additions at both leaf and parent +paths, alongside deletion/replacement cases. After-values and final snapshots must +agree while preserving reasons, dependencies, origins and redaction. + +## 6. P2 — Preserve semantic path kinds and reject nonexistent explanation paths + +Evidence: [metadata.go](../../metadata.go) indexes queries solely by `Path.String`, +losing explicit key/index distinctions. [explain.go](../../explain.go), +`applicationOperationPath`, considers an atomic ancestor relevant even when the +requested descendant never existed; an absent-to-absent event then makes `Explain` +report success. + +Reproduction: load `items: [1]`, `m: {"0": 2}`, and `x: 3` into `map[string]any`. +`Metadata.Lookup(Keys("items", "0"))` and `Metadata.Lookup(Keys("m").Index(0))` +both succeed despite the incorrect segment kinds. Their explanation behavior +also disagrees with metadata. `Explain(Keys("x", "missing"))` returns true and +`WriteExplanation` succeeds, although this path never existed. + +Required work: resolve query segment kinds against effective and historical +container shapes before looking up textual indexes. Distinguish a genuinely +removed path from an impossible descendant of an atomic value. Share traversal +semantics across metadata and explanation while allowing parsed numeric tokens +to resolve contextually. + +Acceptance: explicit key/index queries reject incompatible containers; parsed +`/items/0` and `/m/0` still resolve correctly. Never-existing scalar descendants +return false/nil, and their writer returns an error without output. Historically +existing descendants remain explainable after parent replacement. + +## 7. P2 — Reject unsupported inline pointer-to-map schemas safely + +Evidence: [schema.go](../../schema.go), `describeFields`, dereferences inline field +types before choosing the map branch, then stores a map descriptor for a field +whose actual reflected value is a pointer. [projection.go](../../projection.go) +subsequently applies map operations to that pointer. + +Reproduction: a configuration field declared +`Extra *map[string]int` with `yaml:",inline"`, initialized to a non-nil map in +defaults, panics in `Load` with `reflect: call of reflect.Value.Len on ptr to +non-array Value`. This is an internal reflection panic, not an application-hook panic. + +Required work: reject pointer-to-map inline declarations during schema construction +with a safe schema diagnostic. The agreed inline shapes are structs, +pointers-to-structs, and string-keyed maps; do not broaden them accidentally. +Audit nearby descriptor/value-kind assumptions for the same failure class. + +Acceptance: nil and non-nil inline map pointers fail consistently without invoking +default factories or panicking. Supported inline maps and inline struct pointers +continue to load, project, clone and classify correctly. + +## 8. P2 — Fix the baseline test's canonical-path assumption on macOS + +Evidence: [source_test.go](../../source_test.go), +`TestFileSourcesReopenAndClassifyFailures`, compares `SourceInfo.Path` directly +with the path under `t.TempDir()`. Successful sources intentionally report a +symlink-resolved canonical path. + +Reproduction: both `GOPROXY=off go test ./...` and its `-race` variant fail at this +assertion on the review host, where the temporary path traverses macOS's `/var` +symlink. Both suites pass with `TMPDIR=/private/tmp`. Source contents are correct; +the test expectation is wrong. + +Required work: derive the expected canonical path through the filesystem contract, +and separately verify the declared source label. Do not remove canonicalization +from production code or require developers to override `TMPDIR`. + +Acceptance: the ordinary documented test and race commands pass with the host's +default temporary directory and with an intentionally symlinked temporary directory. + +## 9. P2 — Make the release checker's Bash prerequisite executable on macOS + +Evidence: [scripts/release-check.sh](../../scripts/release-check.sh) uses +`mapfile -d ''`, but [the release procedure](../release.md) requires only Bash. +The macOS system Bash is 3.2 and has no `mapfile`; the null-delimiter option also +requires a sufficiently recent newer Bash. + +Reproduction: `/bin/bash -c "mapfile -d '' xs