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/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 ae07d0f1..abaa261b 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,29 @@ 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 { + // 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) + } + if err := f.Chmod(0o600); err != nil { + _ = 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) + } + 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..71610a57 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. @@ -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 { @@ -144,6 +152,9 @@ 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) + // 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 06ec520b..c80187ef 100644 --- a/pkg/filecache/filecache_test.go +++ b/pkg/filecache/filecache_test.go @@ -296,3 +296,77 @@ 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)) + } +} + +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) + } +} diff --git a/pkg/filecache/write.go b/pkg/filecache/write.go new file mode 100644 index 00000000..df96f660 --- /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 func() { _ = 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)) +} 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.