From a08a110eb90d31679814325077e134207562100a Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 25 Oct 2026 23:49:06 -0400 Subject: [PATCH] fix: an unreadable public value is refused, never answered from the secret namespace (r139) WorkspaceValue, ReadAuthority and Recheck read through workspaceValueLive. When the public read failed (a missing file carrier, a value delivered both inline and by file), workspaceValueLive discarded the error and returned whatever the secret namespace held under the same name. Review r139 probed this and got "secretvalue" back with a nil error. An authority read could pin, or a Recheck could pass, on a value from a delivery the host never made for that name. workspaceValueLive now goes through workspaceValueIfSetLive, which moves on to the secret namespace only when the public value is absent and returns any other failure. WorkspaceValue returns that failure as it is; ReadAuthority and Recheck wrap it under ErrAuthorityValueMissing / ErrAuthorityValueChanged with "cannot be read", so the refusal names the delivery's reason. The new test fails against the old code at the WorkspaceValue assertion. The listing doc comments now say what the code does: a live file carrier is listed by its path, so one naming an empty file is listed and then reads as absent. TestWorkspaceConfigurationKeysSeparatesAbsenceFromFailure covers that case and a value delivered both inline and by file. Co-Authored-By: Claude Opus 5.5 --- authority.go | 6 +++ codefly.go | 13 ++++--- file_carrier_test.go | 46 +++++++++++++++++++++++ for.go | 55 ++++++++++++++++++---------- workspace_configuration_keys_test.go | 32 ++++++++++++++++ 5 files changed, 127 insertions(+), 25 deletions(-) diff --git a/authority.go b/authority.go index bd01a4d..11cd9e9 100644 --- a/authority.go +++ b/authority.go @@ -95,6 +95,9 @@ func ReadAuthority(ctx context.Context, names ...AuthorityValueName) (*Authority continue } value, err := query.workspaceValueLive(name.Name, name.Key) + if err != nil && !isWorkspaceValueAbsent(err) { + return nil, fmt.Errorf("%w: %s cannot be read: %w", ErrAuthorityValueMissing, name, err) + } if err != nil || strings.TrimSpace(value) == "" { return nil, fmt.Errorf("%w: %s", ErrAuthorityValueMissing, name) } @@ -164,6 +167,9 @@ func (a *Authority) Recheck(ctx context.Context) error { query := For(ctx) for _, name := range a.Names() { value, err := query.workspaceValueLive(name.Name, name.Key) + if err != nil && !isWorkspaceValueAbsent(err) { + return fmt.Errorf("%w: %s cannot be read: %w", ErrAuthorityValueChanged, name, err) + } if err != nil || strings.TrimSpace(value) == "" { return fmt.Errorf("%w: %s is no longer configured", ErrAuthorityValueChanged, name) } diff --git a/codefly.go b/codefly.go index d435e9c..e479e7f 100644 --- a/codefly.go +++ b/codefly.go @@ -269,12 +269,13 @@ func codeflyEnvironmentVariables() []string { return append([]string(nil), environmentVariablesSnapshot...) } -// deliveredNames lists, from the carriers the SDK already holds, the name of -// every value that is delivered non-empty: the snapshot (process values, the -// values file carriers delivered at load, and injected values) and the live -// process environment, where an inline value is listed by its own name and a -// file carrier by the key it carries. It reads no file: a carrier's path is -// evidence of delivery, and its content is read only by the per-key lookup, so +// deliveredNames lists the name of every carrier the SDK already holds with a +// non-empty value: the snapshot (process values, the values file carriers +// delivered at load, and injected values) and the live process environment, +// where an inline value is listed by its own name and a file carrier by the +// key it carries. For a live file carrier the non-empty value is its PATH: no +// file is opened, so a carrier naming an empty file is listed, and the per-key +// lookup then answers absent. Its content is read only by that lookup, so // listing a secret namespace neither materialises nor caches a secret value. func deliveredNames() []string { names := map[string]struct{}{} diff --git a/file_carrier_test.go b/file_carrier_test.go index 91a7bfe..3051879 100644 --- a/file_carrier_test.go +++ b/file_carrier_test.go @@ -129,3 +129,49 @@ func TestAnUnreadableFileCarrierFailsClosed(t *testing.T) { require.ErrorIs(t, err, resources.ErrFileCarrier) require.Error(t, codefly.LoadEnvironmentVariables()) } + +// A public delivery that cannot be read is refused with its reason. It is +// never answered from the secret namespace under the same name: that is a +// different delivery, and answering with it would silently replace the value +// the host meant this process to read (review r139 probed exactly this and got +// "secretvalue" back with no error). The same holds where the value is read as +// authority: ReadAuthority refuses, and Recheck refuses a pinned value whose +// public delivery broke even when the secret namespace holds the pinned value. +func TestAnUnreadablePublicValueIsNeverAnsweredFromTheSecretNamespace(t *testing.T) { + ctx := context.Background() + query := codefly.For(ctx) + public := resources.WorkspaceConfigurationPrefix + "__PROBE_FB__BINDING" + t.Setenv(resources.FileCarrierKey(public), filepath.Join(t.TempDir(), "absent")) + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_FB__BINDING", "secretvalue") + + // The premise: the public read fails, and the secret namespace answers. + _, err := query.WorkspaceConfiguration("probe-fb", "BINDING") + require.ErrorIs(t, err, resources.ErrFileCarrier) + secret, err := query.WorkspaceSecret("probe-fb", "BINDING") + require.NoError(t, err) + require.Equal(t, "secretvalue", secret) + + value, err := query.WorkspaceValue("probe-fb", "BINDING") + require.ErrorIs(t, err, resources.ErrFileCarrier, "the refusal names the public delivery's failure") + require.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent) + require.Empty(t, value) + require.NotContains(t, err.Error(), "secretvalue") + + _, err = codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-fb", Key: "BINDING"}) + require.ErrorIs(t, err, codefly.ErrAuthorityValueMissing) + require.ErrorIs(t, err, resources.ErrFileCarrier) + + pinned := resources.WorkspaceConfigurationPrefix + "__PROBE_FB_PINNED__BINDING" + t.Setenv(pinned, "pinnedvalue") + authority, err := codefly.ReadAuthority(ctx, codefly.AuthorityValueName{Name: "probe-fb-pinned", Key: "BINDING"}) + require.NoError(t, err) + require.NoError(t, os.Unsetenv(pinned)) + t.Setenv(resources.FileCarrierKey(pinned), filepath.Join(t.TempDir(), "absent")) + t.Setenv(resources.WorkspaceSecretConfigurationPrefix+"__PROBE_FB_PINNED__BINDING", "pinnedvalue") + + err = authority.Recheck(ctx) + require.ErrorIs(t, err, codefly.ErrAuthorityValueChanged) + require.ErrorIs(t, err, resources.ErrFileCarrier) + _, err = query.WorkspaceValue("probe-fb-pinned", "BINDING") + require.ErrorIs(t, err, resources.ErrFileCarrier) +} diff --git a/for.go b/for.go index d6d66d8..aa1e04f 100644 --- a/for.go +++ b/for.go @@ -406,14 +406,17 @@ var ErrWorkspaceConfigurationAbsent = errors.New("no workspace configuration val // Codefly's environment variable encoding. // // Each key is spelled so that WorkspaceConfiguration(name, key) reads its -// value, and only keys a carrier delivers non-empty are listed. A group no -// carrier holds returns an empty list and an error wrapping +// value. A key is listed when a carrier names it: an inline value that is +// non-empty, or a file carrier whose path is non-empty. A group no carrier +// holds returns an empty list and an error wrapping // ErrWorkspaceConfigurationAbsent. // // Listing reads names, never values: no file a carrier names is opened, and -// no authority pin is read. A value is read through the per-key accessors, -// which apply their own rules, so a listed key whose delivery cannot be read -// fails there, naming its reason, rather than being dropped from the list. +// no authority pin is read. A file's content is judged only by the per-key +// accessors, which apply their own rules, so a listed key can still read as +// absent (its file is empty) or fail with a named reason (its file is missing, +// or it is delivered both inline and by file). It is never dropped from the +// list. func (q *Query) WorkspaceConfigurationKeys(name string) ([]string, error) { return workspaceConfigurationKeys(resources.WorkspaceConfigurationPrefix, name) } @@ -458,6 +461,11 @@ func workspaceConfigurationKeys(prefix string, name string) ([]string, error) { // then the secret namespace. It is intended for settings whose sensitivity is // deployment-defined while preserving a single SDK-only lookup boundary. // +// Only an absent public value moves the lookup on to the secret namespace. A +// public delivery that cannot be read (a missing file, a value delivered both +// inline and by file) is refused with that reason, and the secret namespace +// is not consulted. +// // A value this process read as authority-bearing (see ReadAuthority) answers // from what was read at boot, and a live value that has since drifted is // refused with ErrAuthorityValueChanged rather than returned. Without that, a @@ -467,6 +475,9 @@ func (q *Query) WorkspaceValue(name string, key string) (string, error) { addressed := AuthorityValueName{Name: name, Key: key}.canonical() pinned, isPinned := authorityPin(addressed) value, err := q.workspaceValueLive(name, key) + if err != nil && !isWorkspaceValueAbsent(err) { + return "", err + } if !isPinned { return value, err } @@ -479,16 +490,16 @@ func (q *Query) WorkspaceValue(name string, key string) (string, error) { return pinned, nil } -// WorkspaceValueIfSet resolves a workspace value and distinguishes the three -// answers WorkspaceValue flattens into two: the value is set, the value is -// genuinely not configured, or the lookup FAILED. +// WorkspaceValueIfSet resolves a workspace value and answers absence without +// an error: the value is set, the value is genuinely not configured, or the +// lookup FAILED. // -// WorkspaceValue reports "not configured" and "the delivery could not be read" -// with the same error, because its callers want a value or nothing. A caller -// deciding a posture question cannot live with that: a setting that must be -// asserted before something is permitted has to treat an unreadable delivery, -// and an authority-bearing value that has drifted, as a refusal rather than as -// "the operator did not assert it". Absence is an answer; a failed read is not. +// WorkspaceValue answers absence with an error, because its callers want a +// value or nothing. A caller deciding a posture question needs absence as an +// answer: a setting that must be asserted before something is permitted has to +// treat an unreadable delivery, and an authority-bearing value that has +// drifted, as a refusal rather than as "the operator did not assert it". +// Absence is an answer; a failed read is not. // // So an unreadable file carrier, a value delivered both inline and by file, and // ErrAuthorityValueChanged all come back as errors here, and only a value no @@ -540,14 +551,20 @@ func isWorkspaceValueAbsent(err error) bool { return errors.Is(err, ErrWorkspaceConfigurationAbsent) } +// workspaceValueLive is workspaceValueIfSetLive with absence as an error. A +// public delivery that cannot be read is returned as the failure it is, never +// stepped over to the secret namespace: a value under the same name there is a +// different delivery, and answering with it would let an unreadable public +// value be silently replaced. func (q *Query) workspaceValueLive(name string, key string) (string, error) { - if value, err := q.WorkspaceConfiguration(name, key); err == nil && value != "" { - return value, nil + value, found, err := q.workspaceValueIfSetLive(name, key) + if err != nil { + return "", err } - if value, err := q.WorkspaceSecret(name, key); err == nil && value != "" { - return value, nil + if !found { + return "", fmt.Errorf("%w for %s/%s", ErrWorkspaceConfigurationAbsent, name, key) } - return "", fmt.Errorf("%w for %s/%s", ErrWorkspaceConfigurationAbsent, name, key) + return value, nil } func (q *Query) workspaceConfigurationValue(prefix string, name string, key string) (string, error) { diff --git a/workspace_configuration_keys_test.go b/workspace_configuration_keys_test.go index 0c410d2..49027eb 100644 --- a/workspace_configuration_keys_test.go +++ b/workspace_configuration_keys_test.go @@ -176,4 +176,36 @@ func TestWorkspaceConfigurationKeysSeparatesAbsenceFromFailure(t *testing.T) { assert.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "an unreadable delivery is not absence") assert.ErrorIs(t, err, resources.ErrFileCarrier) }) + + t.Run("a file carrier naming an empty file", func(t *testing.T) { + key := "CODEFLY__WORKSPACE_CONFIGURATION__KEYS_EMPTY_FILE__BINDING" + path := filepath.Join(t.TempDir(), "empty") + require.NoError(t, os.WriteFile(path, nil, 0o600)) + t.Setenv(resources.FileCarrierKey(key), path) + + keys, err := codefly.For(ctx).WorkspaceConfigurationKeys("keys-empty-file") + + require.NoError(t, err) + assert.Equal(t, []string{"BINDING"}, keys, "listing opens no file, so the carrier's path is what it sees") + _, err = codefly.For(ctx).WorkspaceConfiguration("keys-empty-file", "BINDING") + assert.ErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "an empty file reads as absent") + }) + + t.Run("a value delivered both inline and by file", func(t *testing.T) { + key := "CODEFLY__WORKSPACE_CONFIGURATION__KEYS_TWICE__BINDING" + path := filepath.Join(t.TempDir(), "binding") + require.NoError(t, os.WriteFile(path, []byte("by file"), 0o600)) + t.Setenv(key, "inline") + t.Setenv(resources.FileCarrierKey(key), path) + + keys, err := codefly.For(ctx).WorkspaceConfigurationKeys("keys-twice") + + require.NoError(t, err) + assert.Equal(t, []string{"BINDING"}, keys) + _, err = codefly.For(ctx).WorkspaceConfiguration("keys-twice", "BINDING") + require.Error(t, err) + assert.NotErrorIs(t, err, codefly.ErrWorkspaceConfigurationAbsent, "two sources for one fact is not absence") + assert.ErrorIs(t, err, resources.ErrFileCarrier) + assert.ErrorContains(t, err, "delivered both inline and by file") + }) }