From be8c77ca8ac11a9c3112825558ed7529975d68c5 Mon Sep 17 00:00:00 2001 From: Prathmesh Borle <65400885+cx-prathmesh-borle@users.noreply.github.com> Date: Fri, 11 Sep 2026 14:14:36 +0530 Subject: [PATCH] Enhance credential store tests and functionality - Updated `TestStoreRequiredRoundTripKeepsValueOutOfYAML` to verify that the credential value is not written to the YAML file. - Added new tests: - `TestStoreAndClearAutoTouchConfigFileMtime` to ensure the config file's modification time is updated after storing or clearing credentials. - `TestStoreFailureDoesNotTouchConfigFile` to confirm that a failed store operation does not create a config file. - `TestTouchConfigFileToleratesUnusablePaths` to handle various invalid paths gracefully. - Modified `Store` and `Clear` methods in the resolver to touch the config file's mtime upon successful operations, ensuring consumers are notified of changes. --- internal/credentialstore/resolver.go | 39 +++++++++- .../credentialstore/resolver_write_test.go | 78 +++++++++++++++++-- 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/internal/credentialstore/resolver.go b/internal/credentialstore/resolver.go index 4f02db59..1d0186e6 100644 --- a/internal/credentialstore/resolver.go +++ b/internal/credentialstore/resolver.go @@ -4,10 +4,12 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "os/user" "path/filepath" "sync" + "time" "github.com/checkmarx/ast-cli/internal/configfile" "github.com/checkmarx/ast-cli/internal/logger" @@ -18,6 +20,8 @@ import ( const ( checkmarxDirName = ".checkmarx" checkmarxFileName = "checkmarxcli.yaml" + // ownerOnlyFilePerm matches how configfile writes the config file: owner-only on Unix. + ownerOnlyFilePerm = 0o600 ) // Resolver resolves credential values across explicit, env, keyring and config-file layers. @@ -93,7 +97,11 @@ func (r *Resolver) Store(ctx context.Context, credentialName, value string) erro if r.policy == PolicyDisabled { return configfile.SetKey(r.filePath, viperKeyFor(credentialName), value) } - return r.store.Set(ctx, credentialName, value) + err := r.store.Set(ctx, credentialName, value) + if err == nil { + touchConfigFile(r.filePath) + } + return err } // Clear removes a credential following the policy, mirroring Store. A missing @@ -113,7 +121,34 @@ func (r *Resolver) Clear(ctx context.Context, credentialName string) error { } return configfile.RemoveKey(r.filePath, viperKeyFor(credentialName)) } - return r.store.Delete(ctx, credentialName) + err := r.store.Delete(ctx, credentialName) + if err == nil { + touchConfigFile(r.filePath) + } + return err +} + +// touchConfigFile bumps the config file's mtime so consumers watching it (cx-agentic-ai's hooks +// key their caches off it) still see keyring-backed credential changes. Best-effort, never fatal. +func touchConfigFile(path string) { + if path == "" { + return + } + now := time.Now() + err := os.Chtimes(path, now, now) + if err == nil { + return + } + if !errors.Is(err, fs.ErrNotExist) { + logger.PrintfIfVerbose("credentialstore: could not touch config file mtime: %v", err) + return + } + file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, ownerOnlyFilePerm) + if err != nil { + logger.PrintfIfVerbose("credentialstore: could not create config file to touch mtime: %v", err) + return + } + _ = file.Close() } // StoresInConfigFile reports whether this policy persists credentials in the diff --git a/internal/credentialstore/resolver_write_test.go b/internal/credentialstore/resolver_write_test.go index 8fda75ee..fe4aaa7b 100644 --- a/internal/credentialstore/resolver_write_test.go +++ b/internal/credentialstore/resolver_write_test.go @@ -2,9 +2,11 @@ package credentialstore import ( "context" + "errors" "os" "path/filepath" "testing" + "time" "github.com/checkmarx/ast-cli/internal/configfile" "github.com/stretchr/testify/assert" @@ -47,9 +49,9 @@ func TestClearDisabledRemovesYAMLAndReportsMissing(t *testing.T) { assert.Empty(t, value) } -// PolicyRequired round-trips through the keyring store only; the YAML layer -// is neither read nor written. -func TestStoreRequiredRoundTripIgnoresYAML(t *testing.T) { +// PolicyRequired round-trips through the keyring store only; Store touches the YAML file +// but never writes the credential value into it. +func TestStoreRequiredRoundTripKeepsValueOutOfYAML(t *testing.T) { keyring.MockInit() t.Cleanup(keyring.MockInit) store := NewCredentialStore(CanonicalConfigPath(t.TempDir())) @@ -58,9 +60,9 @@ func TestStoreRequiredRoundTripIgnoresYAML(t *testing.T) { assert.NoError(t, resolver.Store(ctx, CredentialAPIKey, "required-value")) - if _, err := os.Stat(resolver.filePath); !os.IsNotExist(err) { - t.Fatalf("required mode must not create the YAML file") - } + data, err := os.ReadFile(resolver.filePath) + assert.NoError(t, err) + assert.NotContains(t, string(data), "required-value") value, err := resolver.Resolve(ctx, CredentialAPIKey) assert.NoError(t, err) assert.Equal(t, "required-value", value) @@ -142,6 +144,70 @@ func TestClearDisabledUnreadableConfigPropagatesError(t *testing.T) { assert.NotErrorIs(t, err, ErrNotFound) } +// A keyring-backed Store/Clear must still bump the config file's mtime, since the credential +// value itself never lands in the file for mtime-watching consumers to see. +func TestStoreAndClearAutoTouchConfigFileMtime(t *testing.T) { + keyring.MockInit() + t.Cleanup(keyring.MockInit) + yamlPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + resolver := NewResolver(yamlPath, PolicyAuto, nil) + ctx := context.Background() + + assert.NoError(t, resolver.Store(ctx, CredentialAPIKey, "v")) + assert.FileExists(t, yamlPath) + + // Backdate rather than sleep: filesystems with coarse mtime granularity would + // otherwise report both writes at the same instant. + past := time.Now().Add(-time.Hour) + assert.NoError(t, os.Chtimes(yamlPath, past, past)) + assert.NoError(t, resolver.Clear(ctx, CredentialAPIKey)) + + info, err := os.Stat(yamlPath) + assert.NoError(t, err) + assert.True(t, info.ModTime().After(past)) +} + +// A failed keyring write must leave no trace: no config file, hence no mtime bump +// telling consumers a credential changed. +func TestStoreFailureDoesNotTouchConfigFile(t *testing.T) { + store := newFakeStore() + store.setErr = errors.New("keyring write failed") + yamlPath := filepath.Join(t.TempDir(), "checkmarxcli.yaml") + resolver := NewResolver(yamlPath, PolicyAuto, store) + + assert.Error(t, resolver.Store(context.Background(), CredentialAPIKey, "v")) + assert.NoFileExists(t, yamlPath) +} + +// Unusable paths must be swallowed: a best-effort touch never fails a credential write, +// and it never leaves a stray file behind. +func TestTouchConfigFileToleratesUnusablePaths(t *testing.T) { + missingParent := filepath.Join(t.TempDir(), "no-such-dir", "checkmarxcli.yaml") + nonDirParent := filepath.Join(existingFile(t), "checkmarxcli.yaml") + + for name, path := range map[string]string{ + "empty path": "", + "missing parent": missingParent, + "parent is a file": nonDirParent, + "path is a dir": t.TempDir(), + } { + t.Run(name, func(t *testing.T) { + touchConfigFile(path) + }) + } + + assert.NoFileExists(t, missingParent) + assert.NoFileExists(t, nonDirParent) +} + +// existingFile returns a regular file's path, for cases needing a non-directory parent. +func existingFile(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "not-a-dir") + assert.NoError(t, os.WriteFile(path, []byte("x"), 0o600)) + return path +} + // Auto mode surfaces a config-file read failure instead of masking it as // not-found once the keyring layer also misses. func TestResolveAutoConfigFileReadErrorSurfaces(t *testing.T) {