Skip to content

Commit 5777684

Browse files
committed
fix(search): classify each discovered path in search_files and multi_grep
H-4: search_files and multi_grep previously only classified the root path, then walked and read every matching file. Add checkSearchPath helpers that classify each descended directory and each file the same way the root path is checked; sensitive paths are skipped and reported in a new Skipped field rather than returned silently. H-5: add regression tests for case-insensitive ~/.odek trust-anchor matching (macOS APFS) and shell rc files. - file_tool.go: wire checkSearchPath into searchContent and searchFiles; return Skipped in searchFilesResult. - perf_tools.go: add multiGrepTool.checkSearchPath; guard multi_grep walker; return Skipped in grepPatternResult. - file_tool_test.go / perf_tools_test.go / classifier_test.go: regression tests for skipped sensitive paths and case-insensitive anchors.
1 parent cd1845d commit 5777684

6 files changed

Lines changed: 327 additions & 9 deletions

File tree

cmd/odek/file_tool.go

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -525,6 +525,7 @@ type searchMatch struct {
525525

526526
type searchFilesResult struct {
527527
Matches []searchMatch `json:"matches"`
528+
Skipped []string `json:"skipped,omitempty"`
528529
Error string `json:"error,omitempty"`
529530
}
530531

@@ -567,13 +568,28 @@ func (t *searchFilesTool) Call(argsJSON string) (string, error) {
567568
}
568569
}
569570

571+
// checkSearchPath classifies a discovered path the same way the root path was
572+
// classified. If the path is more restrictive (e.g. a file under ~/.odek
573+
// discovered while searching $HOME), it returns skip=true so the walker does
574+
// not silently read sensitive files.
575+
func (t *searchFilesTool) checkSearchPath(path string) (skip bool, reason string) {
576+
risk := danger.ClassifyPath(path)
577+
if err := t.dangerousConfig.CheckOperation(danger.ToolOperation{
578+
Name: "search_files", Resource: path, Risk: risk,
579+
}, nil); err != nil {
580+
return true, err.Error()
581+
}
582+
return false, ""
583+
}
584+
570585
func (t *searchFilesTool) searchContent(args searchFilesArgs) (string, error) {
571586
re, err := regexp.Compile(args.Pattern)
572587
if err != nil {
573588
return jsonError(fmt.Sprintf("invalid regex pattern %q: %v", args.Pattern, err))
574589
}
575590

576591
var matches []searchMatch
592+
var skipped []string
577593
limit := args.Limit
578594
resultBytes := 0
579595

@@ -590,6 +606,13 @@ func (t *searchFilesTool) searchContent(args searchFilesArgs) (string, error) {
590606
if skipDir(info.Name()) {
591607
return filepath.SkipDir
592608
}
609+
// Security: a broad search root may contain a sensitive subtree
610+
// (e.g. ~/.odek under $HOME). Classify the directory before
611+
// descending; if it would require approval/denial, skip it.
612+
if skip, reason := t.checkSearchPath(path); skip {
613+
skipped = append(skipped, path+": "+reason)
614+
return filepath.SkipDir
615+
}
593616
return nil
594617
}
595618
// Skip symlinks — prevents TOCTOU on the path and avoids listing
@@ -598,6 +621,13 @@ func (t *searchFilesTool) searchContent(args searchFilesArgs) (string, error) {
598621
return nil
599622
}
600623

624+
// Security: classify each file before reading. This prevents a broad
625+
// search from silently returning files that read_file would gate.
626+
if skip, reason := t.checkSearchPath(path); skip {
627+
skipped = append(skipped, path+": "+reason)
628+
return nil
629+
}
630+
601631
// Apply file_glob filter
602632
if args.FileGlob != "" {
603633
match, _ := filepath.Match(args.FileGlob, info.Name())
@@ -659,7 +689,7 @@ func (t *searchFilesTool) searchContent(args searchFilesArgs) (string, error) {
659689
return jsonError(fmt.Sprintf("search failed: %v", err))
660690
}
661691

662-
return jsonResult(searchFilesResult{Matches: matches})
692+
return jsonResult(searchFilesResult{Matches: matches, Skipped: skipped})
663693
}
664694

665695
func (t *searchFilesTool) searchFiles(args searchFilesArgs) (string, error) {
@@ -673,7 +703,15 @@ func (t *searchFilesTool) searchFiles(args searchFilesArgs) (string, error) {
673703
}
674704

675705
var matches []searchMatch
706+
var skipped []string
676707
for _, p := range paths {
708+
// Security: each discovered path is classified the same way the root
709+
// path is. Sensitive files (e.g. under ~/.odek, /etc) are skipped
710+
// rather than returned silently.
711+
if skip, reason := t.checkSearchPath(p); skip {
712+
skipped = append(skipped, p+": "+reason)
713+
continue
714+
}
677715
matches = append(matches, searchMatch{Path: wrapUntrusted(t.toolCtx(), "search_files:"+p, p)})
678716
}
679717

@@ -688,7 +726,7 @@ func (t *searchFilesTool) searchFiles(args searchFilesArgs) (string, error) {
688726
return fi.ModTime().After(fj.ModTime())
689727
})
690728

691-
return jsonResult(searchFilesResult{Matches: matches})
729+
return jsonResult(searchFilesResult{Matches: matches, Skipped: skipped})
692730
}
693731

694732
// ── Patch Tool ─────────────────────────────────────────────────────────
@@ -1091,14 +1129,16 @@ func confineToCWD(path string) (string, error) {
10911129
// must not be writable through the generic file tools. Keep in sync with
10921130
// the SystemWrite escalation in danger.ClassifyPath.
10931131
func isProtectedOdekPath(rel string) bool {
1094-
rel = filepath.Clean(rel)
1132+
// Case-folding defends against case-insensitive filesystems (macOS APFS):
1133+
// ~/.odek/CONFIG.JSON and ~/.odek/config.json are the same file.
1134+
rel = strings.ToLower(filepath.Clean(rel))
10951135

10961136
// Exact-file trust anchors. Rewriting any of these can disable safety
10971137
// policy, exfiltrate secrets, or persist attacker control.
10981138
protectedExact := []string{
10991139
"config.json",
11001140
"secrets.env",
1101-
"IDENTITY.md",
1141+
"identity.md",
11021142
"schedules.json",
11031143
"schedule-state.json",
11041144
"schedules.lock",

cmd/odek/file_tool_test.go

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"path/filepath"
88
"strings"
9+
"sync"
910
"testing"
1011

1112
"github.com/BackendStack21/odek/internal/danger"
@@ -1220,6 +1221,179 @@ func TestWriteFile_SecurityDenied(t *testing.T) {
12201221
}
12211222
}
12221223

1224+
// ── Search path classification tests (H-4 / H-5) ────────────────────────
1225+
1226+
// countingApprover is a test double that approves the first N prompt requests
1227+
// and denies the rest. It lets us approve a search root while denying the
1228+
// sensitive files discovered underneath it.
1229+
type countingApprover struct {
1230+
mu sync.Mutex
1231+
calls int
1232+
allowFirstN int
1233+
}
1234+
1235+
func (a *countingApprover) PromptCommand(cls danger.RiskClass, cmd, description string) error {
1236+
return a.prompt()
1237+
}
1238+
1239+
func (a *countingApprover) PromptOperation(op danger.ToolOperation) error {
1240+
return a.prompt()
1241+
}
1242+
1243+
func (a *countingApprover) prompt() error {
1244+
a.mu.Lock()
1245+
defer a.mu.Unlock()
1246+
a.calls++
1247+
if a.calls <= a.allowFirstN {
1248+
return nil
1249+
}
1250+
return fmt.Errorf("denied by test approver")
1251+
}
1252+
1253+
// makeTestHomeDir creates a HOME directory outside /tmp and /var so that
1254+
// ClassifyPath treats paths under it as real home-directory paths rather than
1255+
// temporary/system paths. The directory is created under the current working
1256+
// directory and removed when the test ends.
1257+
func makeTestHomeDir(t *testing.T) string {
1258+
t.Helper()
1259+
cwd, err := os.Getwd()
1260+
if err != nil {
1261+
t.Fatalf("cannot get cwd: %v", err)
1262+
}
1263+
home, err := os.MkdirTemp(cwd, "odek-test-home-*")
1264+
if err != nil {
1265+
t.Fatalf("cannot create test home: %v", err)
1266+
}
1267+
t.Cleanup(func() { os.RemoveAll(home) })
1268+
return home
1269+
}
1270+
1271+
func TestSearchFiles_TargetFiles_SkipsSensitiveOdekPaths(t *testing.T) {
1272+
home := makeTestHomeDir(t)
1273+
t.Setenv("HOME", home)
1274+
1275+
odekDir := filepath.Join(home, ".odek")
1276+
if err := os.MkdirAll(odekDir, 0755); err != nil {
1277+
t.Fatal(err)
1278+
}
1279+
os.WriteFile(filepath.Join(odekDir, "config.json"), []byte("api-key-secret\n"), 0644)
1280+
os.WriteFile(filepath.Join(odekDir, "notes.md"), []byte("hello world\n"), 0644)
1281+
1282+
cfg := danger.DangerousConfig{
1283+
Classes: map[danger.RiskClass]danger.Action{danger.SystemWrite: danger.Prompt},
1284+
Approver: &countingApprover{allowFirstN: 1}, // approve the search root only
1285+
}
1286+
tool := &searchFilesTool{dangerousConfig: cfg}
1287+
1288+
args := fmt.Sprintf(`{"pattern":"*","target":"files","path":%q}`, odekDir)
1289+
result := callJSON(t, tool, args)
1290+
1291+
var r struct {
1292+
Matches []struct {
1293+
Path string `json:"path"`
1294+
} `json:"matches"`
1295+
Skipped []string `json:"skipped"`
1296+
Error string `json:"error,omitempty"`
1297+
}
1298+
mustUnmarshal(t, result, &r)
1299+
if r.Error != "" {
1300+
t.Fatalf("unexpected error: %s", r.Error)
1301+
}
1302+
if len(r.Matches) != 1 {
1303+
t.Fatalf("expected 1 allowed match, got %d", len(r.Matches))
1304+
}
1305+
if !strings.Contains(unwrapUntrusted(r.Matches[0].Path), "notes.md") {
1306+
t.Errorf("expected notes.md match, got %q", r.Matches[0].Path)
1307+
}
1308+
if len(r.Skipped) != 1 {
1309+
t.Fatalf("expected 1 skipped sensitive path, got %d: %v", len(r.Skipped), r.Skipped)
1310+
}
1311+
if !strings.Contains(r.Skipped[0], "config.json") {
1312+
t.Errorf("expected config.json in skipped list, got %q", r.Skipped[0])
1313+
}
1314+
}
1315+
1316+
func TestSearchFiles_TargetFiles_SkipsCaseInsensitiveAnchor(t *testing.T) {
1317+
home := makeTestHomeDir(t)
1318+
t.Setenv("HOME", home)
1319+
1320+
odekDir := filepath.Join(home, ".odek")
1321+
if err := os.MkdirAll(odekDir, 0755); err != nil {
1322+
t.Fatal(err)
1323+
}
1324+
os.WriteFile(filepath.Join(odekDir, "CONFIG.JSON"), []byte("api-key-secret\n"), 0644)
1325+
os.WriteFile(filepath.Join(odekDir, "notes.md"), []byte("hello world\n"), 0644)
1326+
1327+
cfg := danger.DangerousConfig{
1328+
Classes: map[danger.RiskClass]danger.Action{danger.SystemWrite: danger.Prompt},
1329+
Approver: &countingApprover{allowFirstN: 1},
1330+
}
1331+
tool := &searchFilesTool{dangerousConfig: cfg}
1332+
1333+
args := fmt.Sprintf(`{"pattern":"*","target":"files","path":%q}`, odekDir)
1334+
result := callJSON(t, tool, args)
1335+
1336+
var r struct {
1337+
Matches []struct {
1338+
Path string `json:"path"`
1339+
} `json:"matches"`
1340+
Skipped []string `json:"skipped"`
1341+
Error string `json:"error,omitempty"`
1342+
}
1343+
mustUnmarshal(t, result, &r)
1344+
if r.Error != "" {
1345+
t.Fatalf("unexpected error: %s", r.Error)
1346+
}
1347+
if len(r.Matches) != 1 {
1348+
t.Fatalf("expected 1 allowed match, got %d", len(r.Matches))
1349+
}
1350+
if len(r.Skipped) != 1 || !strings.Contains(strings.ToLower(r.Skipped[0]), "config.json") {
1351+
t.Fatalf("expected CONFIG.JSON skipped, got %v", r.Skipped)
1352+
}
1353+
}
1354+
1355+
func TestSearchFiles_CheckSearchPath_DeniesSystemWrite(t *testing.T) {
1356+
home := makeTestHomeDir(t)
1357+
t.Setenv("HOME", home)
1358+
1359+
cfg := danger.DangerousConfig{
1360+
Classes: map[danger.RiskClass]danger.Action{danger.SystemWrite: danger.Deny},
1361+
}
1362+
tool := &searchFilesTool{dangerousConfig: cfg}
1363+
1364+
skip, reason := tool.checkSearchPath(filepath.Join(home, ".odek", "config.json"))
1365+
if !skip {
1366+
t.Fatalf("expected checkSearchPath to skip ~/.odek/config.json")
1367+
}
1368+
if !strings.Contains(reason, "system_write") {
1369+
t.Errorf("expected system_write denial, got %q", reason)
1370+
}
1371+
1372+
skip, _ = tool.checkSearchPath(filepath.Join(home, ".odek", "notes.md"))
1373+
if skip {
1374+
t.Errorf("expected non-anchor ~/.odek path to be allowed")
1375+
}
1376+
}
1377+
1378+
// TestIsProtectedOdekPath_CaseInsensitive verifies the file-tool write guard
1379+
// rejects case variants of trust anchors on case-insensitive filesystems.
1380+
func TestIsProtectedOdekPath_CaseInsensitive(t *testing.T) {
1381+
cases := []string{
1382+
"CONFIG.JSON",
1383+
"Secrets.Env",
1384+
"IDENTITY.MD",
1385+
"SKILLS/evil/SKILL.md",
1386+
"SESSIONS/abc.json",
1387+
"AUDIT/turn.json",
1388+
"PLANS/evil.md",
1389+
}
1390+
for _, c := range cases {
1391+
if !isProtectedOdekPath(c) {
1392+
t.Errorf("isProtectedOdekPath(%q) = false, want true", c)
1393+
}
1394+
}
1395+
}
1396+
12231397
// ── Helpers ────────────────────────────────────────────────────────────
12241398

12251399
func callJSON(t *testing.T, tool interface {

cmd/odek/perf_tools.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1198,6 +1198,7 @@ type grepPatternResult struct {
11981198
Pattern string `json:"pattern"`
11991199
Matches []grepMatch `json:"matches"`
12001200
Count int `json:"count"`
1201+
Skipped []string `json:"skipped,omitempty"`
12011202
Error string `json:"error,omitempty"`
12021203
}
12031204

@@ -1269,6 +1270,20 @@ func (t *multiGrepTool) Call(argsJSON string) (string, error) {
12691270
return jsonResult(multiGrepResult{Results: results})
12701271
}
12711272

1273+
// checkSearchPath classifies a discovered path the same way the root path was
1274+
// checked in Call. If the path is more restrictive (e.g. a file under ~/.odek
1275+
// discovered while searching $HOME), it returns skip=true so the walker does
1276+
// not silently read sensitive files.
1277+
func (t *multiGrepTool) checkSearchPath(path string) (skip bool, reason string) {
1278+
risk := danger.ClassifyPath(path)
1279+
if err := t.dangerousConfig.CheckOperation(danger.ToolOperation{
1280+
Name: "multi_grep", Resource: path, Risk: risk,
1281+
}, nil); err != nil {
1282+
return true, err.Error()
1283+
}
1284+
return false, ""
1285+
}
1286+
12721287
func (t *multiGrepTool) searchPattern(pattern, root, fileGlob string, limit int) (result grepPatternResult) {
12731288
defer func() {
12741289
if r := recover(); r != nil {
@@ -1283,6 +1298,7 @@ func (t *multiGrepTool) searchPattern(pattern, root, fileGlob string, limit int)
12831298
var matches []grepMatch
12841299
resultBytes := 0
12851300

1301+
var skipped []string
12861302
filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
12871303
if err != nil || info == nil {
12881304
return nil
@@ -1294,12 +1310,22 @@ func (t *multiGrepTool) searchPattern(pattern, root, fileGlob string, limit int)
12941310
if skipDir(info.Name()) {
12951311
return filepath.SkipDir
12961312
}
1313+
// Security: classify each directory before descending.
1314+
if skip, reason := t.checkSearchPath(path); skip {
1315+
skipped = append(skipped, path+": "+reason)
1316+
return filepath.SkipDir
1317+
}
12971318
return nil
12981319
}
12991320
// Skip symlinks — prevents TOCTOU and listing unreadable files.
13001321
if info.Mode()&os.ModeSymlink != 0 {
13011322
return nil
13021323
}
1324+
// Security: classify each file before reading.
1325+
if skip, reason := t.checkSearchPath(path); skip {
1326+
skipped = append(skipped, path+": "+reason)
1327+
return nil
1328+
}
13031329
if fileGlob != "" {
13041330
match, _ := filepath.Match(fileGlob, info.Name())
13051331
if !match {
@@ -1352,6 +1378,7 @@ func (t *multiGrepTool) searchPattern(pattern, root, fileGlob string, limit int)
13521378
Pattern: pattern,
13531379
Matches: matches,
13541380
Count: len(matches),
1381+
Skipped: skipped,
13551382
}
13561383
}
13571384

0 commit comments

Comments
 (0)