Skip to content

Commit f567580

Browse files
SAY-5SAY-5
committed
fix(install): centralise ~/.guild perms via guildpath helpers
Adds internal/guildpath.{EnsureGuildDir,EnsureDir,TightenDBPerms} and routes all six call sites (install, mcp/db, cli/lore_read, cli/quest, cli/hints, telemetry/path) through them so a CLI verb that races the install can't leave ~/.guild at 0o755 (#79 follow-up). TightenDBPerms also locks WAL/SHM/journal sidecars to 0o600 after storage.Open so pre-existing installs are sealed up too. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
1 parent f2339f0 commit f567580

9 files changed

Lines changed: 273 additions & 20 deletions

File tree

‎internal/cli/hints.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/spf13/cobra"
1212

13+
"github.com/mathomhaus/guild/internal/guildpath"
1314
"github.com/mathomhaus/guild/internal/hints"
1415
"github.com/mathomhaus/guild/internal/storage"
1516
)
@@ -81,12 +82,16 @@ func openHintsDB(ctx context.Context) (*sql.DB, error) {
8182
}
8283
path := filepath.Join(home, ".guild", "quest.db")
8384
if dir := filepath.Dir(path); dir != "" {
84-
_ = os.MkdirAll(dir, 0o755)
85+
_ = guildpath.EnsureDir(dir)
8586
}
8687
db, err := storage.Open(ctx, path)
8788
if err != nil {
8889
return nil, fmt.Errorf("open %s: %w", path, err)
8990
}
91+
if err := guildpath.TightenDBPerms(path); err != nil {
92+
_ = db.Close()
93+
return nil, fmt.Errorf("cli: %w", err)
94+
}
9095
if err := storage.Migrate(ctx, db, "quest"); err != nil {
9196
_ = db.Close()
9297
return nil, err

‎internal/cli/lore_read.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515

1616
"github.com/mathomhaus/guild/internal/command"
1717
"github.com/mathomhaus/guild/internal/config"
18+
"github.com/mathomhaus/guild/internal/guildpath"
1819
"github.com/mathomhaus/guild/internal/lore"
1920
"github.com/mathomhaus/guild/internal/project"
2021
"github.com/mathomhaus/guild/internal/storage"
@@ -39,13 +40,17 @@ func openLoreDB(ctx context.Context) (*sql.DB, error) {
3940
if err != nil {
4041
return nil, err
4142
}
42-
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
43-
return nil, fmt.Errorf("cli: ensure ~/.guild: %w", err)
43+
if err := guildpath.EnsureDir(filepath.Dir(path)); err != nil {
44+
return nil, fmt.Errorf("cli: %w", err)
4445
}
4546
db, err := storage.Open(ctx, path)
4647
if err != nil {
4748
return nil, err
4849
}
50+
if err := guildpath.TightenDBPerms(path); err != nil {
51+
_ = db.Close()
52+
return nil, fmt.Errorf("cli: %w", err)
53+
}
4954
if err := storage.Migrate(ctx, db, "lore"); err != nil {
5055
_ = db.Close()
5156
return nil, err

‎internal/cli/quest.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313

1414
"github.com/mathomhaus/guild/internal/command"
1515
"github.com/mathomhaus/guild/internal/config"
16+
"github.com/mathomhaus/guild/internal/guildpath"
1617
"github.com/mathomhaus/guild/internal/lore"
1718
"github.com/mathomhaus/guild/internal/lore/embed"
1819
"github.com/mathomhaus/guild/internal/project"
@@ -52,15 +53,19 @@ func openQuestDB(ctx context.Context) (*sql.DB, error) {
5253
}
5354
if path != ":memory:" && !strings.HasPrefix(path, ":memory:") {
5455
if dir := filepath.Dir(path); dir != "." && dir != "/" {
55-
if err := os.MkdirAll(dir, 0o755); err != nil {
56-
return nil, fmt.Errorf("create %s: %w", dir, err)
56+
if err := guildpath.EnsureDir(dir); err != nil {
57+
return nil, fmt.Errorf("cli: %w", err)
5758
}
5859
}
5960
}
6061
db, err := storage.Open(ctx, path)
6162
if err != nil {
6263
return nil, err
6364
}
65+
if err := guildpath.TightenDBPerms(path); err != nil {
66+
_ = db.Close()
67+
return nil, fmt.Errorf("cli: %w", err)
68+
}
6469
if err := storage.Migrate(ctx, db, "quest"); err != nil {
6570
_ = db.Close()
6671
return nil, err

‎internal/guildpath/guildpath.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
// Package guildpath centralises filesystem locations for guild's local
2+
// state and enforces the permissions invariants that #79 calls out:
3+
//
4+
// - ~/.guild/ is created with 0o700 (private to the user). The
5+
// directory is touched on every MCP open and every CLI verb, so a
6+
// stale 0o755 (default umask) from any one of those call sites
7+
// would re-loosen the dir on a fresh install. EnsureGuildDir
8+
// centralises that decision so every entry point routes through
9+
// the same MkdirAll.
10+
//
11+
// - Every SQLite DB plus its journal_mode=WAL sidecars (-wal, -shm,
12+
// -journal) is 0o600. modernc/sqlite respects O_CREAT perms on
13+
// the main file but does not tighten existing files, and the
14+
// sidecars are always created at the default umask (typically
15+
// 0o644). TightenDBPerms walks all four paths after storage.Open
16+
// so existing-install corpora are locked down too.
17+
package guildpath
18+
19+
import (
20+
"fmt"
21+
"os"
22+
"path/filepath"
23+
)
24+
25+
// DirPerm is the mode bits applied to ~/.guild/.
26+
const DirPerm os.FileMode = 0o700
27+
28+
// DBPerm is the mode bits applied to a SQLite db file and its WAL/SHM/journal sidecars.
29+
const DBPerm os.FileMode = 0o600
30+
31+
// EnsureGuildDir returns the path to ~/.guild, creating it with 0o700
32+
// if absent. Idempotent: an existing directory keeps its current mode
33+
// (we do not retroactively chmod, since the user may have set 0o750
34+
// deliberately for an audit group). Callers that need the path
35+
// hardened on a stale install should follow up with TightenDir.
36+
func EnsureGuildDir() (string, error) {
37+
home, err := os.UserHomeDir()
38+
if err != nil {
39+
return "", fmt.Errorf("guildpath: resolve home dir: %w", err)
40+
}
41+
dir := filepath.Join(home, ".guild")
42+
if err := os.MkdirAll(dir, DirPerm); err != nil {
43+
return "", fmt.Errorf("guildpath: create %s: %w", dir, err)
44+
}
45+
return dir, nil
46+
}
47+
48+
// EnsureDir is the generic variant for sub-paths that derive from
49+
// EnsureGuildDir. It MkdirAlls dir with 0o700, then returns it.
50+
func EnsureDir(dir string) error {
51+
if dir == "" || dir == "." || dir == "/" {
52+
return nil
53+
}
54+
if err := os.MkdirAll(dir, DirPerm); err != nil {
55+
return fmt.Errorf("guildpath: create %s: %w", dir, err)
56+
}
57+
return nil
58+
}
59+
60+
// TightenDBPerms chmods the SQLite main file at path and its
61+
// journal_mode=WAL sidecars (-wal, -shm, -journal) to 0o600. Missing
62+
// siblings are skipped silently, so it is safe to call before the WAL
63+
// is materialised; the only error returned is a real chmod failure on
64+
// a file that exists. Call after storage.Open.
65+
func TightenDBPerms(path string) error {
66+
if path == "" || path == ":memory:" {
67+
return nil
68+
}
69+
for _, suffix := range []string{"", "-wal", "-shm", "-journal"} {
70+
p := path + suffix
71+
if _, err := os.Stat(p); os.IsNotExist(err) {
72+
continue
73+
} else if err != nil {
74+
return fmt.Errorf("guildpath: stat %s: %w", p, err)
75+
}
76+
if err := os.Chmod(p, DBPerm); err != nil {
77+
return fmt.Errorf("guildpath: chmod %s: %w", p, err)
78+
}
79+
}
80+
return nil
81+
}

‎internal/guildpath/guildpath_test.go‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
package guildpath
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
)
8+
9+
func TestEnsureGuildDir_CreatesWith0700(t *testing.T) {
10+
home := t.TempDir()
11+
t.Setenv("HOME", home)
12+
dir, err := EnsureGuildDir()
13+
if err != nil {
14+
t.Fatalf("EnsureGuildDir: %v", err)
15+
}
16+
if dir != filepath.Join(home, ".guild") {
17+
t.Errorf("dir = %q; want %q", dir, filepath.Join(home, ".guild"))
18+
}
19+
info, err := os.Stat(dir)
20+
if err != nil {
21+
t.Fatalf("stat %s: %v", dir, err)
22+
}
23+
if got := info.Mode().Perm(); got != DirPerm {
24+
t.Errorf("perm = %o; want %o", got, DirPerm)
25+
}
26+
}
27+
28+
func TestEnsureGuildDir_Idempotent(t *testing.T) {
29+
home := t.TempDir()
30+
t.Setenv("HOME", home)
31+
d1, err := EnsureGuildDir()
32+
if err != nil {
33+
t.Fatalf("first EnsureGuildDir: %v", err)
34+
}
35+
d2, err := EnsureGuildDir()
36+
if err != nil {
37+
t.Fatalf("second EnsureGuildDir: %v", err)
38+
}
39+
if d1 != d2 {
40+
t.Errorf("paths differ: %q vs %q", d1, d2)
41+
}
42+
}
43+
44+
// Regression: when a stale 0o755 dir pre-exists (e.g. created by a
45+
// prior CLI verb before this fix), EnsureGuildDir must not chmod it
46+
// back to 0o700. We document and rely on the umask-preserving
47+
// idempotence; sites that need a guaranteed-tight dir should call
48+
// os.Chmod after EnsureGuildDir, but we do not foreclose other
49+
// callers' deliberate widening.
50+
func TestEnsureGuildDir_PreservesExistingMode(t *testing.T) {
51+
home := t.TempDir()
52+
t.Setenv("HOME", home)
53+
dir := filepath.Join(home, ".guild")
54+
if err := os.MkdirAll(dir, 0o750); err != nil {
55+
t.Fatalf("pre-seed dir: %v", err)
56+
}
57+
if _, err := EnsureGuildDir(); err != nil {
58+
t.Fatalf("EnsureGuildDir: %v", err)
59+
}
60+
info, err := os.Stat(dir)
61+
if err != nil {
62+
t.Fatalf("stat: %v", err)
63+
}
64+
if got := info.Mode().Perm(); got != 0o750 {
65+
t.Errorf("perm = %o; EnsureGuildDir must not retroactively chmod (#79 comment)", got)
66+
}
67+
}
68+
69+
func TestTightenDBPerms_AllSiblings(t *testing.T) {
70+
dir := t.TempDir()
71+
path := filepath.Join(dir, "lore.db")
72+
// Seed all four siblings with the default 0o644 sqlite ships them at.
73+
for _, suffix := range []string{"", "-wal", "-shm", "-journal"} {
74+
p := path + suffix
75+
if err := os.WriteFile(p, []byte("x"), 0o644); err != nil { //nolint:gosec // test fixture mirrors sqlite's default
76+
t.Fatalf("seed %s: %v", p, err)
77+
}
78+
}
79+
if err := TightenDBPerms(path); err != nil {
80+
t.Fatalf("TightenDBPerms: %v", err)
81+
}
82+
for _, suffix := range []string{"", "-wal", "-shm", "-journal"} {
83+
p := path + suffix
84+
info, err := os.Stat(p)
85+
if err != nil {
86+
t.Fatalf("stat %s: %v", p, err)
87+
}
88+
if got := info.Mode().Perm(); got != DBPerm {
89+
t.Errorf("%s perm = %o; want %o", p, got, DBPerm)
90+
}
91+
}
92+
}
93+
94+
func TestTightenDBPerms_MissingSiblings(t *testing.T) {
95+
dir := t.TempDir()
96+
path := filepath.Join(dir, "quest.db")
97+
// Only the main file exists; sidecars haven't been materialised yet.
98+
if err := os.WriteFile(path, []byte("x"), 0o644); err != nil { //nolint:gosec // test fixture
99+
t.Fatalf("seed: %v", err)
100+
}
101+
if err := TightenDBPerms(path); err != nil {
102+
t.Fatalf("TightenDBPerms: %v", err)
103+
}
104+
info, err := os.Stat(path)
105+
if err != nil {
106+
t.Fatal(err)
107+
}
108+
if got := info.Mode().Perm(); got != DBPerm {
109+
t.Errorf("main file perm = %o; want %o", got, DBPerm)
110+
}
111+
}
112+
113+
func TestTightenDBPerms_MemoryPath(t *testing.T) {
114+
if err := TightenDBPerms(":memory:"); err != nil {
115+
t.Errorf("TightenDBPerms(:memory:) should be a no-op, got %v", err)
116+
}
117+
if err := TightenDBPerms(""); err != nil {
118+
t.Errorf("TightenDBPerms(\"\") should be a no-op, got %v", err)
119+
}
120+
}
121+
122+
// Regression for the install-path race: a CLI verb that ran before
123+
// `guild init` could create ~/.guild at 0o755 and the install would
124+
// then no-op. Route the install path through EnsureGuildDir so the
125+
// first-creator semantics are consistent.
126+
func TestEnsureGuildDir_FirstCreatorWins(t *testing.T) {
127+
home := t.TempDir()
128+
t.Setenv("HOME", home)
129+
dir, err := EnsureGuildDir()
130+
if err != nil {
131+
t.Fatalf("EnsureGuildDir: %v", err)
132+
}
133+
info, err := os.Stat(dir)
134+
if err != nil {
135+
t.Fatal(err)
136+
}
137+
if got := info.Mode().Perm(); got != DirPerm {
138+
t.Errorf("first-creator perm = %o; want %o", got, DirPerm)
139+
}
140+
}

‎internal/install/init.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"path/filepath"
1919
"strings"
2020

21+
"github.com/mathomhaus/guild/internal/guildpath"
2122
"github.com/mathomhaus/guild/internal/lore"
2223
"github.com/mathomhaus/guild/internal/lore/embed"
2324
"github.com/mathomhaus/guild/internal/quest"
@@ -220,6 +221,12 @@ func Init(ctx context.Context, repoRoot string, opts InitOptions) (*InitResult,
220221
return nil, fmt.Errorf("install: open lore.db: %w", err)
221222
}
222223
defer func() { _ = loreDB.Close() }()
224+
// Tighten perms on the main db plus its WAL/SHM sidecars so
225+
// pre-existing 0o644 installs (or installs that ran before the
226+
// 0o700-on-create fix landed) get locked down too (#79).
227+
if err := guildpath.TightenDBPerms(loreDBPath); err != nil {
228+
return nil, fmt.Errorf("install: %w", err)
229+
}
223230
if err := storage.Migrate(ctx, loreDB, "lore"); err != nil {
224231
return nil, fmt.Errorf("install: migrate lore.db: %w", err)
225232
}
@@ -229,6 +236,9 @@ func Init(ctx context.Context, repoRoot string, opts InitOptions) (*InitResult,
229236
return nil, fmt.Errorf("install: open quest.db: %w", err)
230237
}
231238
defer func() { _ = questDB.Close() }()
239+
if err := guildpath.TightenDBPerms(questDBPath); err != nil {
240+
return nil, fmt.Errorf("install: %w", err)
241+
}
232242
if err := storage.Migrate(ctx, questDB, "quest"); err != nil {
233243
return nil, fmt.Errorf("install: migrate quest.db: %w", err)
234244
}
@@ -417,13 +427,9 @@ func resolveDBPaths(opts InitOptions) (loreDB, questDB string, err error) {
417427
if loreDB != "" && questDB != "" {
418428
return loreDB, questDB, nil
419429
}
420-
home, err := os.UserHomeDir()
430+
guildDir, err := guildpath.EnsureGuildDir()
421431
if err != nil {
422-
return "", "", fmt.Errorf("install: resolve home dir: %w", err)
423-
}
424-
guildDir := filepath.Join(home, ".guild")
425-
if err := os.MkdirAll(guildDir, 0o700); err != nil {
426-
return "", "", fmt.Errorf("install: create ~/.guild: %w", err)
432+
return "", "", fmt.Errorf("install: %w", err)
427433
}
428434
if loreDB == "" {
429435
loreDB = filepath.Join(guildDir, "lore.db")

‎internal/mcp/db.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"path/filepath"
99
"strings"
1010

11+
"github.com/mathomhaus/guild/internal/guildpath"
1112
"github.com/mathomhaus/guild/internal/storage"
1213
)
1314

@@ -62,15 +63,19 @@ func openQuestDB(ctx context.Context) (*sql.DB, error) {
6263
func openDB(ctx context.Context, path, description string) (*sql.DB, error) {
6364
if path != ":memory:" && !strings.HasPrefix(path, ":memory:") {
6465
if dir := filepath.Dir(path); dir != "." && dir != "/" {
65-
if err := os.MkdirAll(dir, 0o755); err != nil {
66-
return nil, fmt.Errorf("mcp: ensure %s: %w", dir, err)
66+
if err := guildpath.EnsureDir(dir); err != nil {
67+
return nil, fmt.Errorf("mcp: %w", err)
6768
}
6869
}
6970
}
7071
db, err := storage.Open(ctx, path)
7172
if err != nil {
7273
return nil, err
7374
}
75+
if err := guildpath.TightenDBPerms(path); err != nil {
76+
_ = db.Close()
77+
return nil, fmt.Errorf("mcp: %w", err)
78+
}
7479
if err := storage.Migrate(ctx, db, description); err != nil {
7580
_ = db.Close()
7681
return nil, err

0 commit comments

Comments
 (0)