From 8e9bf9e7415d89aeafaea119dfe60c898b6c3ce3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 04:47:34 +0000 Subject: [PATCH 1/4] envsec: fix P0 secrets-hygiene bugs (name validation, file perms, --format) Fixes bug-scan P0 #1, #2, #3 and #6 from specs/bug-scan.md: - #1: anchor the env-var name regex with `$` so names like `A/../../other/prod/KEY` or `PATH=/evil` are rejected instead of being written to another project's SSM path or into the exec environment. - #2: `envsec download` now writes the output file with 0600 and tightens an existing file to 0600, so secrets aren't world-readable. - #3: pkg/filecache writes cache files atomically with 0600 inside 0700 directories, protecting the cached Cognito AWS credentials. The atomic rename also replaces old 0644 cache files. - #6: `envsec download --format` now defaults to "" (which ValidateFormat accepts) instead of "env" (which it rejected), so plain `envsec download .env` works again and `.json` auto-detection runs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bqy92pq662Fgv5A4GoaUD2 --- envsec/pkg/envcli/download.go | 3 +- envsec/pkg/envsec/download.go | 24 ++++++++++++++-- envsec/pkg/envsec/download_test.go | 45 ++++++++++++++++++++++++++++++ envsec/pkg/envsec/set.go | 2 +- envsec/pkg/envsec/set_test.go | 28 +++++++++++++++++++ pkg/filecache/filecache.go | 6 ++-- pkg/filecache/filecache_test.go | 36 ++++++++++++++++++++++++ pkg/filecache/write.go | 36 ++++++++++++++++++++++++ 8 files changed, 172 insertions(+), 8 deletions(-) create mode 100644 envsec/pkg/envsec/download_test.go create mode 100644 envsec/pkg/envsec/set_test.go create mode 100644 pkg/filecache/write.go diff --git a/envsec/pkg/envcli/download.go b/envsec/pkg/envcli/download.go index 7017878f..ac43a62e 100644 --- a/envsec/pkg/envcli/download.go +++ b/envsec/pkg/envcli/download.go @@ -35,7 +35,8 @@ func DownloadCmd() *cobra.Command { flags.register(command) command.Flags().StringVarP( - &flags.format, "format", "f", "env", "file format: dotenv or json", + &flags.format, "format", "f", "", + "file format: dotenv or json (default: json if the file ends in .json, dotenv otherwise)", ) return command diff --git a/envsec/pkg/envsec/download.go b/envsec/pkg/envsec/download.go index ae07d0f1..aaa14137 100644 --- a/envsec/pkg/envsec/download.go +++ b/envsec/pkg/envsec/download.go @@ -57,9 +57,8 @@ func (e *Envsec) Download(ctx context.Context, path, format string) error { return errors.WithStack(err) } - err = os.WriteFile(path, contents, 0o644) - if err != nil { - return errors.WithStack(err) + if err = writeSecretFile(path, contents); err != nil { + return err } err = tux.WriteHeader(e.Stderr, "[DONE] Downloaded environment variables to %q for environment: %s\n", @@ -91,3 +90,22 @@ func encodeToDotEnv(m map[string]string) ([]byte, error) { } return []byte(envContents), nil } + +// writeSecretFile writes contents to path so that only the current user can +// read it. If the file already exists, its permissions are tightened to 0600 +// since it is about to hold secrets. +func writeSecretFile(path string, contents []byte) error { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + if err != nil { + return errors.WithStack(err) + } + if err := f.Chmod(0o600); err != nil { + _ = f.Close() + return errors.WithStack(err) + } + if _, err := f.Write(contents); err != nil { + _ = f.Close() + return errors.WithStack(err) + } + return errors.WithStack(f.Close()) +} diff --git a/envsec/pkg/envsec/download_test.go b/envsec/pkg/envsec/download_test.go new file mode 100644 index 00000000..02c20cc3 --- /dev/null +++ b/envsec/pkg/envsec/download_test.go @@ -0,0 +1,45 @@ +package envsec + +import ( + "os" + "path/filepath" + "testing" +) + +func TestWriteSecretFilePermissions(t *testing.T) { + path := filepath.Join(t.TempDir(), ".env") + + // An existing world-readable file should be tightened. + if err := os.WriteFile(path, []byte("OLD=1\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := writeSecretFile(path, []byte("NEW=1\n")); err != nil { + t.Fatalf("writeSecretFile() error = %v", err) + } + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("perm = %o, want 600", perm) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != "NEW=1\n" { + t.Errorf("contents = %q, want %q", got, "NEW=1\n") + } +} + +func TestValidateFormat(t *testing.T) { + for _, f := range []string{"", "json", "dotenv"} { + if err := ValidateFormat(f); err != nil { + t.Errorf("ValidateFormat(%q) = %v, want nil", f, err) + } + } + if err := ValidateFormat("yaml"); err == nil { + t.Error("ValidateFormat(\"yaml\") = nil, want error") + } +} diff --git a/envsec/pkg/envsec/set.go b/envsec/pkg/envsec/set.go index 61c38b60..d1def8b0 100644 --- a/envsec/pkg/envsec/set.go +++ b/envsec/pkg/envsec/set.go @@ -85,7 +85,7 @@ func parseSetArgs(args []string) (map[string]string, error) { return envMap, nil } -const nameRegexStr = "^[a-zA-Z_][a-zA-Z0-9_]*" +const nameRegexStr = "^[a-zA-Z_][a-zA-Z0-9_]*$" var nameRegex = regexp.MustCompile(nameRegexStr) diff --git a/envsec/pkg/envsec/set_test.go b/envsec/pkg/envsec/set_test.go new file mode 100644 index 00000000..9dc0dc0d --- /dev/null +++ b/envsec/pkg/envsec/set_test.go @@ -0,0 +1,28 @@ +package envsec + +import "testing" + +func TestEnsureValidNames(t *testing.T) { + valid := []string{"A", "_A", "abc_123", "FOO_BAR"} + for _, name := range valid { + if err := ensureValidNames([]string{name}); err != nil { + t.Errorf("ensureValidNames(%q) = %v, want nil", name, err) + } + } + + invalid := []string{ + "", + "1A", + "A/../../other-project/prod/KEY", + "PATH=/evil:/bin", + "A B", + "A-B", + "A\n", + "jetpack_foo", + } + for _, name := range invalid { + if err := ensureValidNames([]string{name}); err == nil { + t.Errorf("ensureValidNames(%q) = nil, want error", name) + } + } +} diff --git a/pkg/filecache/filecache.go b/pkg/filecache/filecache.go index b449ab04..52f41e33 100644 --- a/pkg/filecache/filecache.go +++ b/pkg/filecache/filecache.go @@ -57,7 +57,7 @@ func (c *Cache[T]) Set(key string, val T, dur time.Duration) error { return errors.WithStack(err) } - return errors.WithStack(os.WriteFile(c.filename(key), d, 0o644)) + return writeFile(c.filename(key), d) } // SetWithTime is like Set but it allows the caller to specify the expiration @@ -68,7 +68,7 @@ func (c *Cache[T]) SetWithTime(key string, val T, t time.Time) error { return errors.WithStack(err) } - return errors.WithStack(os.WriteFile(c.filename(key), d, 0o644)) + return writeFile(c.filename(key), d) } // Get retrieves a value from the cache with the given key. @@ -144,6 +144,6 @@ func IsCacheMiss(err error) bool { func (c *Cache[T]) filename(key string) string { dir := filepath.Join(c.cacheDir, c.domain) - _ = os.MkdirAll(dir, 0o755) + _ = os.MkdirAll(dir, 0o700) return filepath.Join(dir, cachehash.Slug(key)) } diff --git a/pkg/filecache/filecache_test.go b/pkg/filecache/filecache_test.go index 06ec520b..990ea990 100644 --- a/pkg/filecache/filecache_test.go +++ b/pkg/filecache/filecache_test.go @@ -296,3 +296,39 @@ func TestIsCacheMiss(t *testing.T) { t.Error("IsCacheMiss(other) = true") } } + +func TestSetWritesPrivateFiles(t *testing.T) { + c := newTestCache[string](t) + path := c.filename("secret") + + // Simulate a cache file left behind by an older version with loose perms. + if err := os.WriteFile(path, []byte(`{}`), 0o644); err != nil { + t.Fatal(err) + } + if err := c.Set("secret", "value", time.Hour); err != nil { + t.Fatalf("Set() error = %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("cache file perm = %o, want 600", perm) + } + + dirInfo, err := os.Stat(filepath.Dir(path)) + if err != nil { + t.Fatal(err) + } + if perm := dirInfo.Mode().Perm(); perm != 0o700 { + t.Errorf("cache dir perm = %o, want 700", perm) + } + + entries, err := os.ReadDir(filepath.Dir(path)) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 { + t.Errorf("cache dir has %d entries, want 1 (temp file leaked?)", len(entries)) + } +} diff --git a/pkg/filecache/write.go b/pkg/filecache/write.go new file mode 100644 index 00000000..f579c737 --- /dev/null +++ b/pkg/filecache/write.go @@ -0,0 +1,36 @@ +package filecache + +import ( + "os" + "path/filepath" + + "github.com/pkg/errors" +) + +// writeFile atomically writes data to path with 0600 permissions. Cached +// values may contain credentials, so they must not be readable by other +// users. Writing to a temp file and renaming also replaces any existing file +// (and its possibly looser permissions) and avoids leaving a truncated file +// behind if the process dies mid-write. +func writeFile(path string, data []byte) error { + tmp, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".tmp-*") + if err != nil { + return errors.WithStack(err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // no-op after a successful rename + + // CreateTemp already uses 0600, but be explicit. + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + return errors.WithStack(err) + } + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return errors.WithStack(err) + } + if err := tmp.Close(); err != nil { + return errors.WithStack(err) + } + return errors.WithStack(os.Rename(tmpName, path)) +} From 88bbfc8e9fe83bf8ab5916242df009c64c2c61f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 04:51:12 +0000 Subject: [PATCH 2/4] specs/bug-scan: mark P0 #1, #2, #3, #6 as fixed Adds the status as its own line under the matching "Suggested fix order" item so it doesn't conflict with the other P0 fix PRs. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bqy92pq662Fgv5A4GoaUD2 --- specs/bug-scan.md | 1 + 1 file changed, 1 insertion(+) diff --git a/specs/bug-scan.md b/specs/bug-scan.md index 284e300a..761c3880 100644 --- a/specs/bug-scan.md +++ b/specs/bug-scan.md @@ -92,6 +92,7 @@ This is a static review of the code. Every finding was re-read in the source, an ## Suggested fix order 1. **envsec secrets hygiene (#1–3, #6):** add `$` to the name regex, write secret and credential files `0600`, and fix the `--format` default. Each is a one-line fix with high payoff. + - ✅ **Fixed** #1, #2, #3, #6 in [jetify-com/opensource#614](https://github.com/jetify-com/opensource/pull/614). 2. **Auth panics (#4, #5):** use a comma-ok assertion for `id_token` and nil-check `IDClaims()` everywhere. Together these unblock token refresh for every Jetify CLI. 3. **aisdk OpenAI/Anthropic correctness (#7–11):** these break basic tool calling, no-arg tools, and multi-turn reasoning. 4. **`pkg/osdir` and `runx` (#12–18):** small, contained fixes, each of which should get a regression test. From 46b89b1b31c9a99ca52cbe0655b05cd566940bed Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 04:53:38 +0000 Subject: [PATCH 3/4] filecache: check os.Remove error in deferred cleanup (errcheck) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bqy92pq662Fgv5A4GoaUD2 --- pkg/filecache/write.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/filecache/write.go b/pkg/filecache/write.go index f579c737..df96f660 100644 --- a/pkg/filecache/write.go +++ b/pkg/filecache/write.go @@ -18,7 +18,7 @@ func writeFile(path string, data []byte) error { return errors.WithStack(err) } tmpName := tmp.Name() - defer os.Remove(tmpName) // no-op after a successful rename + defer func() { _ = os.Remove(tmpName) }() // no-op after a successful rename // CreateTemp already uses 0600, but be explicit. if err := tmp.Chmod(0o600); err != nil { From 71c95ad825d725c074048e7bc572f9952536969a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 04:54:13 +0000 Subject: [PATCH 4/4] envsec, filecache: address review findings on permissions and truncation - filecache: tighten an existing cache dir to 0700 and a legacy 0644 cache file to 0600 on read, so credentials cached by older versions aren't left readable by other users until they expire. - envsec download: chmod before truncating so a failed chmod doesn't wipe an existing file. - Add a test that the download command's actual --format default passes ValidateFormat, and a filecache upgrade test from 0755/0644. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bqy92pq662Fgv5A4GoaUD2 --- envsec/pkg/envcli/download_test.go | 19 +++++++++++++++ envsec/pkg/envsec/download.go | 9 ++++++- pkg/filecache/filecache.go | 13 +++++++++- pkg/filecache/filecache_test.go | 38 ++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 envsec/pkg/envcli/download_test.go diff --git a/envsec/pkg/envcli/download_test.go b/envsec/pkg/envcli/download_test.go new file mode 100644 index 00000000..e90ef013 --- /dev/null +++ b/envsec/pkg/envcli/download_test.go @@ -0,0 +1,19 @@ +package envcli + +import ( + "testing" + + "go.jetify.com/envsec/pkg/envsec" +) + +// The --format flag's default must be a value that ValidateFormat accepts, +// otherwise a plain `envsec download .env` always fails. +func TestDownloadFormatDefaultIsValid(t *testing.T) { + flag := DownloadCmd().Flags().Lookup("format") + if flag == nil { + t.Fatal("download command has no --format flag") + } + if err := envsec.ValidateFormat(flag.DefValue); err != nil { + t.Errorf("default --format %q is rejected: %v", flag.DefValue, err) + } +} diff --git a/envsec/pkg/envsec/download.go b/envsec/pkg/envsec/download.go index aaa14137..abaa261b 100644 --- a/envsec/pkg/envsec/download.go +++ b/envsec/pkg/envsec/download.go @@ -95,7 +95,10 @@ func encodeToDotEnv(m map[string]string) ([]byte, error) { // read it. If the file already exists, its permissions are tightened to 0600 // since it is about to hold secrets. func writeSecretFile(path string, contents []byte) error { - f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) + // Don't truncate on open: tighten permissions first so that if Chmod + // fails (e.g. the file belongs to another user) the existing contents + // are left untouched. + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE, 0o600) if err != nil { return errors.WithStack(err) } @@ -103,6 +106,10 @@ func writeSecretFile(path string, contents []byte) error { _ = f.Close() return errors.WithStack(err) } + if err := f.Truncate(0); err != nil { + _ = f.Close() + return errors.WithStack(err) + } if _, err := f.Write(contents); err != nil { _ = f.Close() return errors.WithStack(err) diff --git a/pkg/filecache/filecache.go b/pkg/filecache/filecache.go index 52f41e33..71610a57 100644 --- a/pkg/filecache/filecache.go +++ b/pkg/filecache/filecache.go @@ -76,9 +76,17 @@ func (c *Cache[T]) Get(key string) (T, error) { path := c.filename(key) resultData := data[T]{} - if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { + info, err := os.Stat(path) + if errors.Is(err, os.ErrNotExist) { return resultData.Val, NotFound } + if err == nil && info.Mode().Perm()&0o077 != 0 { + // Files written by older versions were 0644. Tighten them on read so + // that cached credentials don't stay readable by other users until + // they expire and get rewritten. Best effort: if this fails (e.g. the + // file belongs to another user) the read below behaves as before. + _ = os.Chmod(path, 0o600) + } content, err := os.ReadFile(path) if err != nil { @@ -145,5 +153,8 @@ func IsCacheMiss(err error) bool { func (c *Cache[T]) filename(key string) string { dir := filepath.Join(c.cacheDir, c.domain) _ = os.MkdirAll(dir, 0o700) + // MkdirAll doesn't change an existing directory, and older versions + // created it 0755. Tighten it so other users can't reach the files inside. + _ = os.Chmod(dir, 0o700) return filepath.Join(dir, cachehash.Slug(key)) } diff --git a/pkg/filecache/filecache_test.go b/pkg/filecache/filecache_test.go index 990ea990..c80187ef 100644 --- a/pkg/filecache/filecache_test.go +++ b/pkg/filecache/filecache_test.go @@ -332,3 +332,41 @@ func TestSetWritesPrivateFiles(t *testing.T) { t.Errorf("cache dir has %d entries, want 1 (temp file leaked?)", len(entries)) } } + +func TestGetTightensLegacyPermissions(t *testing.T) { + c := newTestCache[string](t) + if err := c.Set("creds", "secret", time.Hour); err != nil { + t.Fatal(err) + } + path := c.filename("creds") + dir := filepath.Dir(path) + + // Simulate a cache written by an older version: 0755 dir, 0644 file. + if err := os.Chmod(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(path, 0o644); err != nil { + t.Fatal(err) + } + + // A valid cache hit must not leave the old permissions in place. + val, err := c.Get("creds") + if err != nil || val != "secret" { + t.Fatalf("Get() = %q, %v; want %q, nil", val, err, "secret") + } + + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("cache file perm = %o, want 600", perm) + } + dirInfo, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + if perm := dirInfo.Mode().Perm(); perm != 0o700 { + t.Errorf("cache dir perm = %o, want 700", perm) + } +}