From f7ca150a7fadcbb6ef955793283cdfdd3883be9a Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 6 Sep 2026 19:05:28 -0500 Subject: [PATCH] Detach callback values before publishing configuration --- callback_ownership_test.go | 52 +++++++++++++++++++++++++++++++++++++ docs/config.md | 5 +++- docs/roadmap/remediation.md | 5 ++++ load.go | 16 ++++++++++++ projection.go | 20 +++++++++----- 5 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 callback_ownership_test.go diff --git a/callback_ownership_test.go b/callback_ownership_test.go new file mode 100644 index 0000000..f1af9af --- /dev/null +++ b/callback_ownership_test.go @@ -0,0 +1,52 @@ +package configkit + +import "testing" + +func TestCallbackAssignmentsAreDetachedAcrossLoads(t *testing.T) { + type object struct{ Value string } + type cfg struct { + Map map[string][]*object + Pointer *object + Secret Secret + } + for _, validatorAssignment := range []bool{false, true} { + name := "resolver" + if validatorAssignment { + name = "validator" + } + t.Run(name, func(t *testing.T) { + external := &object{Value: "original"} + shared := map[string][]*object{"key": {external}} + definition := Definition[cfg]{Resolve: func(c *cfg, r *Resolution) error { + Normalize(r, Keys("map"), &c.Map, shared, "populate") + c.Pointer = external + r.Record(Keys("pointer"), ChangeNormalization, "populate") + c.Secret = SecretEnv("CONFIGKIT_CALLBACK_SECRET") + r.Record(Keys("secret"), ChangeNormalization, "reference") + return nil + }} + if validatorAssignment { + definition.Validate = func(c *cfg, v *Validation) { c.Map = shared; c.Pointer = external } + } + calls := 0 + lookup := func(string) (string, bool) { calls++; return "credential", true } + first, err := loadWithSecretLookup(NoSource(), lookup, Options[cfg]{Definition: definition}) + if err != nil { + t.Fatal(err) + } + second, err := loadWithSecretLookup(NoSource(), lookup, Options[cfg]{Definition: definition}) + if err != nil { + t.Fatal(err) + } + first.Config.Map["key"][0].Value = "changed" + first.Config.Pointer.Value = "changed pointer" + external.Value = "external change" + if second.Config.Map["key"][0].Value != "original" || second.Config.Pointer.Value != "original" { + t.Fatal("published configuration shares callback state") + } + if second.Config.Secret.Value() != "credential" || !second.Config.Secret.IsResolved() || calls != 2 { + t.Fatal("publication changed secret state or repeated environment lookup") + } + }) + } +} diff --git a/docs/config.md b/docs/config.md index c34f707..8858fe9 100644 --- a/docs/config.md +++ b/docs/config.md @@ -34,7 +34,10 @@ root `T` must satisfy the application schema contract below. With no options it starts from the Go zero value. A `Definition[T].Defaults` factory, when supplied, runs exactly once after successful source parsing, schema construction, optional schema-version checking, and selected-profile loading. Its result is detached before -input is applied. +input is applied. Resolver-provided mutable values are detached before secret lookup +and validation, and a final detached copy prevents equal-valued aliases introduced +by validators from escaping into the returned configuration. The publication copy +preserves already validated secret state without rereading the environment. The current lifecycle is: diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index fab42ae..58f9c57 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -76,6 +76,11 @@ regular-file, permitted symlink, containment, and permission tests passing. ## 3. P1 — Detach mutable values introduced by resolution before publication +Status: complete. Added post-resolution and publication ownership copies; the latter +preserves already validated secret state. Regressions cover nested maps/slices, +pointers, direct/helper assignments, validator aliases, and one lookup per load. +Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass. + 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`. diff --git a/load.go b/load.go index 9476dd0..d454159 100644 --- a/load.go +++ b/load.go @@ -252,6 +252,14 @@ func loadWithSecretLookup[T any](source Source, lookup secretLookup, supplied .. accumulator.add(Diagnostic{Code: CodeExplainLimitExceeded, Severity: SeverityError, Message: "retained configuration history exceeds the node limit"}) return nil, accumulator.loadError() } + // Resolver assignments may borrow application-owned mutable values. Detach + // before secret resolution mutates reference fields or validation sees them. + detached, ownershipIssue := cloneConfiguration(schema, reflect.ValueOf(configuration), defaultProjectionLimits()) + if ownershipIssue != nil { + accumulator.add(diagnosticFromDecodeIssue(decodeIssueFromProjection(ownershipIssue))) + return nil, accumulator.loadError() + } + configuration = detached.Interface().(T) secretResolution := resolveSecretReferences(reflect.ValueOf(&configuration).Elem(), schema, inputDocument, lookup) accumulator.add(secretResolution.diagnostics...) if hasErrors(secretResolution.diagnostics) { @@ -264,6 +272,14 @@ func loadWithSecretLookup[T any](source Source, lookup secretLookup, supplied .. return nil, accumulator.loadError() } } + // A read-only validator can replace a value with an equal borrowed object. + // Projected mutation checks cannot detect that alias, so detach at publication. + published, ownershipIssue := cloneConfigurationState(schema, reflect.ValueOf(configuration), defaultProjectionLimits(), true) + if ownershipIssue != nil { + accumulator.add(diagnosticFromDecodeIssue(decodeIssueFromProjection(ownershipIssue))) + return nil, accumulator.loadError() + } + configuration = published.Interface().(T) secrets := classifySecretPaths(schema, options.Definition.Secret, metadata, final, applications, decoded.baseline, decoded.final) return &Result[T]{ Config: configuration, diff --git a/projection.go b/projection.go index 9b9e41d..e6af802 100644 --- a/projection.go +++ b/projection.go @@ -34,10 +34,11 @@ type valueVisit struct { } type valueOperation struct { - schema *schemaDescriptor - limits projectionLimits - active map[valueVisit]bool - nodes int + preserveSecrets bool + schema *schemaDescriptor + limits projectionLimits + active map[valueVisit]bool + nodes int } func projectConfiguration(schema *schemaDescriptor, value reflect.Value, limits projectionLimits) (*document.Node, *projectionIssue) { @@ -49,10 +50,15 @@ func projectConfiguration(schema *schemaDescriptor, value reflect.Value, limits } func cloneConfiguration(schema *schemaDescriptor, value reflect.Value, limits projectionLimits) (reflect.Value, *projectionIssue) { + return cloneConfigurationState(schema, value, limits, false) +} + +// Publication copies preserve only already-validated private reference state. +func cloneConfigurationState(schema *schemaDescriptor, value reflect.Value, limits projectionLimits, preserveSecrets bool) (reflect.Value, *projectionIssue) { if schema == nil || schema.root == nil || !value.IsValid() || value.Type() != schema.root.typeOf { return reflect.Value{}, &projectionIssue{code: CodeSchemaUnsupportedType, message: "configuration value does not match its schema"} } - operation := valueOperation{schema: schema, limits: limits, active: make(map[valueVisit]bool)} + operation := valueOperation{schema: schema, limits: limits, active: make(map[valueVisit]bool), preserveSecrets: preserveSecrets} return operation.clone(value, schema.root, Path{}, 0) } @@ -354,7 +360,9 @@ func (o *valueOperation) clone(value reflect.Value, descriptor *typeDescriptor, case descriptorSecret: result := reflect.New(value.Type()).Elem() reference := value.Interface().(Secret) - if reference.specified { + if o.preserveSecrets { + result.Set(value) + } else if reference.specified { result.Set(reflect.ValueOf(SecretEnv(reference.EnvName()))) } return result, nil