diff --git a/internal/cmd/config.go b/internal/cmd/config.go index 4f467b4..c0e260d 100644 --- a/internal/cmd/config.go +++ b/internal/cmd/config.go @@ -56,6 +56,22 @@ var configGetCmd = &cobra.Command{ fmt.Fprintf(os.Stderr, "Configuration key '%s' not found\n", key) os.Exit(1) } + + // Redacted for the same reason `config list` is: what this defends + // against is incidental disclosure — pasted terminal output, a + // screen-share, a script whose stdout lands in a CI log — and `get` is + // the spelling most likely to be captured by one. It was never a way to + // reach a secret cu uses, since authentication reads the keyring; a + // value here is an unused plaintext leftover. The pointer goes to + // stderr so it reaches a person without joining piped output. + if config.IsCredentialKey(key) { + fmt.Println(config.RedactedValue) + fmt.Fprintf(os.Stderr, + "%q is not printed. cu authenticates via the system keyring; this value is an unused plaintext leftover.\nTo read or remove it, edit %s directly.\n", + key, config.GlobalConfigPath()) + return + } + fmt.Println(value) }, } diff --git a/internal/cmd/config_test.go b/internal/cmd/config_test.go index 237c06a..c877652 100644 --- a/internal/cmd/config_test.go +++ b/internal/cmd/config_test.go @@ -1,11 +1,15 @@ package cmd import ( + "bytes" + "io" + "os" "strings" "testing" "github.com/spf13/viper" "github.com/stretchr/testify/assert" + "github.com/timimsms/cu/internal/config" ) // Simple tests that don't involve os.Exit @@ -135,3 +139,63 @@ func TestConfigValueHandling(t *testing.T) { }) } } + +// captureStdout runs fn with os.Stdout redirected and returns what it wrote. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + orig := os.Stdout + os.Stdout = w + defer func() { os.Stdout = orig }() + + fn() + _ = w.Close() + + var buf bytes.Buffer + if _, err := io.Copy(&buf, r); err != nil { + t.Fatalf("read captured stdout: %v", err) + } + return buf.String() +} + +func TestConfigGetRedactsCredentials(t *testing.T) { + // The value is never printed even though `get` names the key explicitly: + // what redaction defends against is incidental disclosure, and `get` is the + // spelling most likely to be captured into a log or a pasted transcript. + t.Run("credential key is redacted", func(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + viper.Set("api_token", "sk-must-not-be-printed") + + out := captureStdout(t, func() { configGetCmd.Run(configGetCmd, []string{"api_token"}) }) + + assert.NotContains(t, out, "sk-must-not-be-printed", "the token must not reach stdout") + assert.Contains(t, out, config.RedactedValue) + }) + + t.Run("ordinary key still prints its value", func(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + viper.Set("default_list", "abc123") + + out := captureStdout(t, func() { configGetCmd.Run(configGetCmd, []string{"default_list"}) }) + + assert.Contains(t, out, "abc123") + }) +} + +func TestConfigListRedactsCredentials(t *testing.T) { + viper.Reset() + t.Cleanup(viper.Reset) + viper.Set("api_token", "sk-must-not-be-printed") + viper.Set("default_list", "abc123") + + out := captureStdout(t, func() { configListCmd.Run(configListCmd, nil) }) + + assert.NotContains(t, out, "sk-must-not-be-printed") + assert.Contains(t, out, "api_token="+config.RedactedValue) + assert.Contains(t, out, "default_list=abc123", "ordinary keys are unaffected") +} diff --git a/internal/config/config.go b/internal/config/config.go index 38e6459..e424ec2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -67,6 +67,31 @@ func IsCredentialKey(key string) bool { // plaintext leftover, not the keyring entry cu actually authenticates with. const RedactedValue = "" +// stripCredentials returns settings without any credential key, warning about +// each one it drops. Both config files get the same treatment: a credential in +// either is a plaintext secret that cu will never authenticate with, so it is +// refused on the way in (a project .cu.yml being read) and on the way out (a +// project .cu.yml being written). +func stripCredentials(settings map[string]interface{}, path string) map[string]interface{} { + out := make(map[string]interface{}, len(settings)) + for k, v := range settings { + if IsCredentialKey(k) { + fmt.Fprintf(os.Stderr, + "cu: ignoring %q in %s — cu authenticates via the system keyring, not a config file\n", + k, path) + continue + } + out[k] = v + } + return out +} + +// GlobalConfigPath returns the global config file cu reads and writes, so +// commands can point the user at it by name rather than guessing. +func GlobalConfigPath() string { + return globalPath() +} + // globalPath returns the global config file to write. An explicit --config // always wins; otherwise a discovered file is used only while it still lives // under the configured directory, since DefaultConfigDir is a variable that @@ -127,15 +152,7 @@ func Init(cfgFile string) error { // Read project config if err := projectViper.ReadInConfig(); err == nil { - settings := projectViper.AllSettings() - for _, k := range credentialKeys { - if _, present := settings[k]; present { - delete(settings, k) - fmt.Fprintf(os.Stderr, - "cu: ignoring %q in %s — credentials come from the keyring, environment, or your global config\n", - k, projectConfigPath) - } - } + settings := stripCredentials(projectViper.AllSettings(), projectConfigPath) // MergeConfigMap merges into viper's *config* layer, so project // values override the global file while still losing to // environment variables and command-line flags. Using viper.Set @@ -317,6 +334,12 @@ func SaveProjectConfig(settings map[string]interface{}) error { } } + // A credential must not reach .cu.yml either. Init already refuses to read + // one back from that file, so writing it would leave a plaintext secret on + // disk that nothing ever uses — exactly the state the refusal exists to + // prevent, just reached from the other direction. + settings = stripCredentials(settings, projectConfigPath) + // Update with new settings for k, v := range settings { projectViper.Set(k, v) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 34bcb42..497892d 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -654,3 +654,43 @@ func TestCredentialKeysAreNeverStaged(t *testing.T) { assert.Contains(t, string(written), "legacy-token") }) } + +func TestSaveProjectConfigRefusesCredentials(t *testing.T) { + // Init already refuses to read a credential back out of .cu.yml, so writing + // one there would strand a plaintext secret on disk that cu never uses. + // The guard closes that direction. + _, projDir := newLayeredFixture(t, "default_space: global-space\n", "default_list: from-project\n") + require.NoError(t, Init("")) + + require.NoError(t, SaveProjectConfig(map[string]interface{}{ + "api_token": "sk-must-not-be-written", + "default_list": "written", + })) + + written, err := os.ReadFile(filepath.Join(projDir, ProjectConfigFileName)) + require.NoError(t, err) + assert.NotContains(t, string(written), "sk-must-not-be-written", + "a credential must never be written to .cu.yml") + assert.Contains(t, string(written), "written", "ordinary keys are still saved") +} + +func TestSaveProjectConfigDoesNotMutateCallerMap(t *testing.T) { + // stripCredentials copies rather than deleting in place, so a caller that + // reuses its settings map does not silently lose keys. + newLayeredFixture(t, "", "default_list: from-project\n") + require.NoError(t, Init("")) + + settings := map[string]interface{}{"api_token": "sk-x", "default_list": "y"} + require.NoError(t, SaveProjectConfig(settings)) + + assert.Len(t, settings, 2, "the caller's map must be left alone") + assert.Equal(t, "sk-x", settings["api_token"]) +} + +func TestGlobalConfigPathNamesTheFileSaveWrites(t *testing.T) { + cfgDir, _ := newLayeredFixture(t, "default_space: global-space\n", "default_list: from-project\n") + require.NoError(t, Init("")) + + assert.Equal(t, filepath.Join(cfgDir, ConfigFileName+"."+ConfigType), GlobalConfigPath(), + "the path shown to users must be the one Save actually writes") +}