envsec: fix P0 secrets-hygiene bugs (name validation, file perms, --format)

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

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

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

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

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)
 

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

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,17 +68,25 @@ 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.
 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))
 }

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

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

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.