fix: an unreadable public value is refused, never answered from the secret namespace (r139)

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)
 		}

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{}{}

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)
+}

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) {

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")
+	})
 }