From f2339f07fd9c2154ea49cdfdae7f1c7dabf554ed Mon Sep 17 00:00:00 2001 From: SAY-5 Date: Sun, 10 May 2026 15:29:05 -0700 Subject: [PATCH 1/2] fix(install): create ~/.guild with 0700 perms Fixes #79. Signed-off-by: SAY-5 --- internal/install/init.go | 2 +- internal/install/init_test.go | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/install/init.go b/internal/install/init.go index 88a3d73..ce35d0c 100644 --- a/internal/install/init.go +++ b/internal/install/init.go @@ -422,7 +422,7 @@ func resolveDBPaths(opts InitOptions) (loreDB, questDB string, err error) { return "", "", fmt.Errorf("install: resolve home dir: %w", err) } guildDir := filepath.Join(home, ".guild") - if err := os.MkdirAll(guildDir, 0o755); err != nil { + if err := os.MkdirAll(guildDir, 0o700); err != nil { return "", "", fmt.Errorf("install: create ~/.guild: %w", err) } if loreDB == "" { diff --git a/internal/install/init_test.go b/internal/install/init_test.go index 06cb996..f8fdd01 100644 --- a/internal/install/init_test.go +++ b/internal/install/init_test.go @@ -397,6 +397,25 @@ func TestTemplate_ContainsSectionMarker(t *testing.T) { } } +// --------------------------------------------------------------------------- +// ~/.guild/ directory created with 0o700 (regression for #79) +// --------------------------------------------------------------------------- + +func TestResolveDBPaths_GuildDirPerm0700(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + if _, _, err := resolveDBPaths(InitOptions{}); err != nil { + t.Fatalf("resolveDBPaths: %v", err) + } + info, err := os.Stat(filepath.Join(home, ".guild")) + if err != nil { + t.Fatalf("stat ~/.guild: %v", err) + } + if got := info.Mode().Perm(); got != 0o700 { + t.Errorf("~/.guild perm = %o; want 0o700", got) + } +} + // --------------------------------------------------------------------------- // No duplicate sections on multiple runs // --------------------------------------------------------------------------- From 77951db0a1360fb8eefab5118fe407531ee5cddd Mon Sep 17 00:00:00 2001 From: SAY-5 Date: Mon, 11 May 2026 10:57:13 -0700 Subject: [PATCH 2/2] fix(install): centralise ~/.guild perms through guildpath helpers Signed-off-by: SAY-5 --- internal/cli/hints.go | 7 +- internal/cli/lore_read.go | 9 +- internal/cli/quest.go | 9 +- internal/guildpath/guildpath.go | 81 ++++++++++++++++ internal/guildpath/guildpath_test.go | 140 +++++++++++++++++++++++++++ internal/install/init.go | 18 ++-- internal/mcp/db.go | 9 +- internal/telemetry/path.go | 11 ++- internal/telemetry/usage_test.go | 9 +- 9 files changed, 273 insertions(+), 20 deletions(-) create mode 100644 internal/guildpath/guildpath.go create mode 100644 internal/guildpath/guildpath_test.go diff --git a/internal/cli/hints.go b/internal/cli/hints.go index 5ee1493..ebc63d4 100644 --- a/internal/cli/hints.go +++ b/internal/cli/hints.go @@ -10,6 +10,7 @@ import ( "github.com/spf13/cobra" + "github.com/mathomhaus/guild/internal/guildpath" "github.com/mathomhaus/guild/internal/hints" "github.com/mathomhaus/guild/internal/storage" ) @@ -81,12 +82,16 @@ func openHintsDB(ctx context.Context) (*sql.DB, error) { } path := filepath.Join(home, ".guild", "quest.db") if dir := filepath.Dir(path); dir != "" { - _ = os.MkdirAll(dir, 0o755) + _ = guildpath.EnsureDir(dir) } db, err := storage.Open(ctx, path) if err != nil { return nil, fmt.Errorf("open %s: %w", path, err) } + if err := guildpath.TightenDBPerms(path); err != nil { + _ = db.Close() + return nil, fmt.Errorf("cli: %w", err) + } if err := storage.Migrate(ctx, db, "quest"); err != nil { _ = db.Close() return nil, err diff --git a/internal/cli/lore_read.go b/internal/cli/lore_read.go index f0a9fe1..5c9c659 100644 --- a/internal/cli/lore_read.go +++ b/internal/cli/lore_read.go @@ -15,6 +15,7 @@ import ( "github.com/mathomhaus/guild/internal/command" "github.com/mathomhaus/guild/internal/config" + "github.com/mathomhaus/guild/internal/guildpath" "github.com/mathomhaus/guild/internal/lore" "github.com/mathomhaus/guild/internal/project" "github.com/mathomhaus/guild/internal/storage" @@ -39,13 +40,17 @@ func openLoreDB(ctx context.Context) (*sql.DB, error) { if err != nil { return nil, err } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return nil, fmt.Errorf("cli: ensure ~/.guild: %w", err) + if err := guildpath.EnsureDir(filepath.Dir(path)); err != nil { + return nil, fmt.Errorf("cli: %w", err) } db, err := storage.Open(ctx, path) if err != nil { return nil, err } + if err := guildpath.TightenDBPerms(path); err != nil { + _ = db.Close() + return nil, fmt.Errorf("cli: %w", err) + } if err := storage.Migrate(ctx, db, "lore"); err != nil { _ = db.Close() return nil, err diff --git a/internal/cli/quest.go b/internal/cli/quest.go index 1c18c2f..64f0037 100644 --- a/internal/cli/quest.go +++ b/internal/cli/quest.go @@ -13,6 +13,7 @@ import ( "github.com/mathomhaus/guild/internal/command" "github.com/mathomhaus/guild/internal/config" + "github.com/mathomhaus/guild/internal/guildpath" "github.com/mathomhaus/guild/internal/lore" "github.com/mathomhaus/guild/internal/lore/embed" "github.com/mathomhaus/guild/internal/project" @@ -52,8 +53,8 @@ func openQuestDB(ctx context.Context) (*sql.DB, error) { } if path != ":memory:" && !strings.HasPrefix(path, ":memory:") { if dir := filepath.Dir(path); dir != "." && dir != "/" { - if err := os.MkdirAll(dir, 0o755); err != nil { - return nil, fmt.Errorf("create %s: %w", dir, err) + if err := guildpath.EnsureDir(dir); err != nil { + return nil, fmt.Errorf("cli: %w", err) } } } @@ -61,6 +62,10 @@ func openQuestDB(ctx context.Context) (*sql.DB, error) { if err != nil { return nil, err } + if err := guildpath.TightenDBPerms(path); err != nil { + _ = db.Close() + return nil, fmt.Errorf("cli: %w", err) + } if err := storage.Migrate(ctx, db, "quest"); err != nil { _ = db.Close() return nil, err diff --git a/internal/guildpath/guildpath.go b/internal/guildpath/guildpath.go new file mode 100644 index 0000000..4805e7c --- /dev/null +++ b/internal/guildpath/guildpath.go @@ -0,0 +1,81 @@ +// Package guildpath centralises filesystem locations for guild's local +// state and enforces the permissions invariants that #79 calls out: +// +// - ~/.guild/ is created with 0o700 (private to the user). The +// directory is touched on every MCP open and every CLI verb, so a +// stale 0o755 (default umask) from any one of those call sites +// would re-loosen the dir on a fresh install. EnsureGuildDir +// centralises that decision so every entry point routes through +// the same MkdirAll. +// +// - Every SQLite DB plus its journal_mode=WAL sidecars (-wal, -shm, +// -journal) is 0o600. modernc/sqlite respects O_CREAT perms on +// the main file but does not tighten existing files, and the +// sidecars are always created at the default umask (typically +// 0o644). TightenDBPerms walks all four paths after storage.Open +// so existing-install corpora are locked down too. +package guildpath + +import ( + "fmt" + "os" + "path/filepath" +) + +// DirPerm is the mode bits applied to ~/.guild/. +const DirPerm os.FileMode = 0o700 + +// DBPerm is the mode bits applied to a SQLite db file and its WAL/SHM/journal sidecars. +const DBPerm os.FileMode = 0o600 + +// EnsureGuildDir returns the path to ~/.guild, creating it with 0o700 +// if absent. Idempotent: an existing directory keeps its current mode +// (we do not retroactively chmod, since the user may have set 0o750 +// deliberately for an audit group). Callers that need the path +// hardened on a stale install should follow up with TightenDir. +func EnsureGuildDir() (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("guildpath: resolve home dir: %w", err) + } + dir := filepath.Join(home, ".guild") + if err := os.MkdirAll(dir, DirPerm); err != nil { + return "", fmt.Errorf("guildpath: create %s: %w", dir, err) + } + return dir, nil +} + +// EnsureDir is the generic variant for sub-paths that derive from +// EnsureGuildDir. It MkdirAlls dir with 0o700, then returns it. +func EnsureDir(dir string) error { + if dir == "" || dir == "." || dir == "/" { + return nil + } + if err := os.MkdirAll(dir, DirPerm); err != nil { + return fmt.Errorf("guildpath: create %s: %w", dir, err) + } + return nil +} + +// TightenDBPerms chmods the SQLite main file at path and its +// journal_mode=WAL sidecars (-wal, -shm, -journal) to 0o600. Missing +// siblings are skipped silently, so it is safe to call before the WAL +// is materialised; the only error returned is a real chmod failure on +// a file that exists. Call after storage.Open. +func TightenDBPerms(path string) error { + if path == "" || path == ":memory:" { + return nil + } + for _, suffix := range []string{"", "-wal", "-shm", "-journal"} { + p := path + suffix + if _, err := os.Stat(p); os.IsNotExist(err) { + continue + } else if err != nil { + return fmt.Errorf("guildpath: stat %s: %w", p, err) + } + if err := os.Chmod(p, DBPerm); err != nil { + return fmt.Errorf("guildpath: chmod %s: %w", p, err) + } + } + return nil +} diff --git a/internal/guildpath/guildpath_test.go b/internal/guildpath/guildpath_test.go new file mode 100644 index 0000000..cde51ed --- /dev/null +++ b/internal/guildpath/guildpath_test.go @@ -0,0 +1,140 @@ +package guildpath + +import ( + "os" + "path/filepath" + "testing" +) + +func TestEnsureGuildDir_CreatesWith0700(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + dir, err := EnsureGuildDir() + if err != nil { + t.Fatalf("EnsureGuildDir: %v", err) + } + if dir != filepath.Join(home, ".guild") { + t.Errorf("dir = %q; want %q", dir, filepath.Join(home, ".guild")) + } + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("stat %s: %v", dir, err) + } + if got := info.Mode().Perm(); got != DirPerm { + t.Errorf("perm = %o; want %o", got, DirPerm) + } +} + +func TestEnsureGuildDir_Idempotent(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + d1, err := EnsureGuildDir() + if err != nil { + t.Fatalf("first EnsureGuildDir: %v", err) + } + d2, err := EnsureGuildDir() + if err != nil { + t.Fatalf("second EnsureGuildDir: %v", err) + } + if d1 != d2 { + t.Errorf("paths differ: %q vs %q", d1, d2) + } +} + +// Regression: when a stale 0o755 dir pre-exists (e.g. created by a +// prior CLI verb before this fix), EnsureGuildDir must not chmod it +// back to 0o700. We document and rely on the umask-preserving +// idempotence; sites that need a guaranteed-tight dir should call +// os.Chmod after EnsureGuildDir, but we do not foreclose other +// callers' deliberate widening. +func TestEnsureGuildDir_PreservesExistingMode(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + dir := filepath.Join(home, ".guild") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("pre-seed dir: %v", err) + } + if _, err := EnsureGuildDir(); err != nil { + t.Fatalf("EnsureGuildDir: %v", err) + } + info, err := os.Stat(dir) + if err != nil { + t.Fatalf("stat: %v", err) + } + if got := info.Mode().Perm(); got != 0o750 { + t.Errorf("perm = %o; EnsureGuildDir must not retroactively chmod (#79 comment)", got) + } +} + +func TestTightenDBPerms_AllSiblings(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "lore.db") + // Seed all four siblings with the default 0o644 sqlite ships them at. + for _, suffix := range []string{"", "-wal", "-shm", "-journal"} { + p := path + suffix + if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { //nolint:gosec // test fixture mirrors sqlite's default + t.Fatalf("seed %s: %v", p, err) + } + } + if err := TightenDBPerms(path); err != nil { + t.Fatalf("TightenDBPerms: %v", err) + } + for _, suffix := range []string{"", "-wal", "-shm", "-journal"} { + p := path + suffix + info, err := os.Stat(p) + if err != nil { + t.Fatalf("stat %s: %v", p, err) + } + if got := info.Mode().Perm(); got != DBPerm { + t.Errorf("%s perm = %o; want %o", p, got, DBPerm) + } + } +} + +func TestTightenDBPerms_MissingSiblings(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "quest.db") + // Only the main file exists; sidecars haven't been materialised yet. + if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { //nolint:gosec // test fixture + t.Fatalf("seed: %v", err) + } + if err := TightenDBPerms(path); err != nil { + t.Fatalf("TightenDBPerms: %v", err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != DBPerm { + t.Errorf("main file perm = %o; want %o", got, DBPerm) + } +} + +func TestTightenDBPerms_MemoryPath(t *testing.T) { + if err := TightenDBPerms(":memory:"); err != nil { + t.Errorf("TightenDBPerms(:memory:) should be a no-op, got %v", err) + } + if err := TightenDBPerms(""); err != nil { + t.Errorf("TightenDBPerms(\"\") should be a no-op, got %v", err) + } +} + +// Regression for the install-path race: a CLI verb that ran before +// `guild init` could create ~/.guild at 0o755 and the install would +// then no-op. Route the install path through EnsureGuildDir so the +// first-creator semantics are consistent. +func TestEnsureGuildDir_FirstCreatorWins(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + dir, err := EnsureGuildDir() + if err != nil { + t.Fatalf("EnsureGuildDir: %v", err) + } + info, err := os.Stat(dir) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != DirPerm { + t.Errorf("first-creator perm = %o; want %o", got, DirPerm) + } +} diff --git a/internal/install/init.go b/internal/install/init.go index ce35d0c..88a9560 100644 --- a/internal/install/init.go +++ b/internal/install/init.go @@ -18,6 +18,7 @@ import ( "path/filepath" "strings" + "github.com/mathomhaus/guild/internal/guildpath" "github.com/mathomhaus/guild/internal/lore" "github.com/mathomhaus/guild/internal/lore/embed" "github.com/mathomhaus/guild/internal/quest" @@ -220,6 +221,12 @@ func Init(ctx context.Context, repoRoot string, opts InitOptions) (*InitResult, return nil, fmt.Errorf("install: open lore.db: %w", err) } defer func() { _ = loreDB.Close() }() + // Tighten perms on the main db plus its WAL/SHM sidecars so + // pre-existing 0o644 installs (or installs that ran before the + // 0o700-on-create fix landed) get locked down too (#79). + if err := guildpath.TightenDBPerms(loreDBPath); err != nil { + return nil, fmt.Errorf("install: %w", err) + } if err := storage.Migrate(ctx, loreDB, "lore"); err != nil { return nil, fmt.Errorf("install: migrate lore.db: %w", err) } @@ -229,6 +236,9 @@ func Init(ctx context.Context, repoRoot string, opts InitOptions) (*InitResult, return nil, fmt.Errorf("install: open quest.db: %w", err) } defer func() { _ = questDB.Close() }() + if err := guildpath.TightenDBPerms(questDBPath); err != nil { + return nil, fmt.Errorf("install: %w", err) + } if err := storage.Migrate(ctx, questDB, "quest"); err != nil { return nil, fmt.Errorf("install: migrate quest.db: %w", err) } @@ -417,13 +427,9 @@ func resolveDBPaths(opts InitOptions) (loreDB, questDB string, err error) { if loreDB != "" && questDB != "" { return loreDB, questDB, nil } - home, err := os.UserHomeDir() + guildDir, err := guildpath.EnsureGuildDir() if err != nil { - return "", "", fmt.Errorf("install: resolve home dir: %w", err) - } - guildDir := filepath.Join(home, ".guild") - if err := os.MkdirAll(guildDir, 0o700); err != nil { - return "", "", fmt.Errorf("install: create ~/.guild: %w", err) + return "", "", fmt.Errorf("install: %w", err) } if loreDB == "" { loreDB = filepath.Join(guildDir, "lore.db") diff --git a/internal/mcp/db.go b/internal/mcp/db.go index f4170f5..dd2c855 100644 --- a/internal/mcp/db.go +++ b/internal/mcp/db.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" + "github.com/mathomhaus/guild/internal/guildpath" "github.com/mathomhaus/guild/internal/storage" ) @@ -62,8 +63,8 @@ func openQuestDB(ctx context.Context) (*sql.DB, error) { func openDB(ctx context.Context, path, description string) (*sql.DB, error) { if path != ":memory:" && !strings.HasPrefix(path, ":memory:") { if dir := filepath.Dir(path); dir != "." && dir != "/" { - if err := os.MkdirAll(dir, 0o755); err != nil { - return nil, fmt.Errorf("mcp: ensure %s: %w", dir, err) + if err := guildpath.EnsureDir(dir); err != nil { + return nil, fmt.Errorf("mcp: %w", err) } } } @@ -71,6 +72,10 @@ func openDB(ctx context.Context, path, description string) (*sql.DB, error) { if err != nil { return nil, err } + if err := guildpath.TightenDBPerms(path); err != nil { + _ = db.Close() + return nil, fmt.Errorf("mcp: %w", err) + } if err := storage.Migrate(ctx, db, description); err != nil { _ = db.Close() return nil, err diff --git a/internal/telemetry/path.go b/internal/telemetry/path.go index 173f99d..d035e81 100644 --- a/internal/telemetry/path.go +++ b/internal/telemetry/path.go @@ -22,6 +22,8 @@ import ( "fmt" "os" "path/filepath" + + "github.com/mathomhaus/guild/internal/guildpath" ) // guildDir returns the path to ~/.guild/, using os.UserHomeDir() so that @@ -52,10 +54,13 @@ func MissesLogPath() (string, error) { return filepath.Join(dir, "misses.log"), nil } -// ensureDir creates dir (and parents) with 0755 if it does not already exist. +// ensureDir creates dir (and parents) with the canonical ~/.guild +// mode (0o700) if it does not already exist. Routes through +// guildpath so every entry point uses the same first-creator +// semantics (#79). func ensureDir(dir string) error { - if err := os.MkdirAll(dir, 0o755); err != nil { - return fmt.Errorf("telemetry: create dir %s: %w", dir, err) + if err := guildpath.EnsureDir(dir); err != nil { + return fmt.Errorf("telemetry: %w", err) } return nil } diff --git a/internal/telemetry/usage_test.go b/internal/telemetry/usage_test.go index 85c8e00..e898360 100644 --- a/internal/telemetry/usage_test.go +++ b/internal/telemetry/usage_test.go @@ -208,8 +208,9 @@ func TestRecord_OptOut_ConfigField(t *testing.T) { // ---- directory auto-create -------------------------------------------------- -// TestRecord_DirAutoCreate verifies that ~/.guild/ is created (0755) if it -// does not exist before the first Record call. +// TestRecord_DirAutoCreate verifies that ~/.guild/ is created (0o700) if it +// does not exist before the first Record call. Tightened from 0o755 to +// 0o700 in #79 so the corpus is never world-readable on shared hosts. func TestRecord_DirAutoCreate(t *testing.T) { home := tempHome(t) // ~/.guild does NOT exist yet (fresh temp dir). @@ -230,8 +231,8 @@ func TestRecord_DirAutoCreate(t *testing.T) { t.Errorf("~/.guild is not a directory") } // Verify permissions (mode bits excluding sticky/setuid). - if got := info.Mode().Perm(); got != 0o755 { - t.Errorf("~/.guild mode: got %04o, want 0755", got) + if got := info.Mode().Perm(); got != 0o700 { + t.Errorf("~/.guild mode: got %04o, want 0700", got) } }