Skip to content

Commit 4139ba8

Browse files
authored
[chore] document settings.toml trust model + path-containment guard (#246)
* [chore] add fileutil.ContainedJoin path-containment helper Exported helper that joins a name onto a base directory and rejects any name that nets outside the base via "..". Uses filepath.Clean(base) + prefix check to avoid an uncoverable filepath.Rel error branch, keeping coverage at 100% for the new file. * [chore] guard copy_files and lockfile paths with ContainedJoin CopyEntries and HashLockfile now call fileutil.ContainedJoin before building paths, rejecting any config-supplied name that escapes the worktree via "..". Defense-in-depth: the same config already grants shell execution via post_create/install, so this is belt-and-suspenders rather than a primary security boundary. * [chore] document settings.toml trust model in README Add a "Trust model" section explaining that post_create and deps.modules[].install run verbatim via sh -c. Cloning an untrusted repo and running rimba add executes that repo's shell scripts — this was previously undocumented. Also notes the defense-in-depth path constraint on copy_files and lockfile, and why worktree_dir is exempt. * [chore] address review: ErrPathEscapes sentinel + validate dstPath - Export ErrPathEscapes sentinel so callers can use errors.Is instead of substring matching on the error string - Apply ContainedJoin to dstPath in CopyEntries as well; was previously safe via srcPath short-circuit, now explicitly guarded on both paths - Update nolint comment and path_test.go to use errors.Is * [chore] assert errors.Is(ErrPathEscapes) in traversal tests hash_test.go and copy_test.go were checking the error string but not the wrapped sentinel. Mirror the pattern from path_test.go: assert errors.Is(err, fileutil.ErrPathEscapes) so any regression in the wrapping chain is caught at test time, not only via string matching.
1 parent a5432a2 commit 4139ba8

7 files changed

Lines changed: 144 additions & 5 deletions

File tree

README.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
- [Quick Start](#quick-start)
2323
- [Commands](#commands)
2424
- [Configuration](#configuration)
25+
- [Trust model](#trust-model)
2526
- [Troubleshooting](#troubleshooting)
2627
- [Contributing](CONTRIBUTING.md)
2728
- [License](#license)
@@ -141,6 +142,17 @@ agent = 'claude'
141142
142143
Running `rimba init --agents` or `rimba init -g` additionally creates or patches MCP server config files in client tools (`.mcp.json`, `.cursor/mcp.json`, `~/.claude/settings.json`, and others). See [docs/configuration.md#mcp-server-registration](docs/configuration.md#mcp-server-registration) for the full list of patched files and entry format.
143144

145+
## Trust model
146+
147+
**`.rimba/settings.toml` is trusted-author input.** The following fields execute shell commands verbatim via `sh -c`:
148+
149+
- `post_create` — runs after a worktree is created (e.g. `rimba add`)
150+
- `deps.modules[].install` — runs when installing dependencies into a worktree
151+
152+
**Cloning an untrusted repository and running `rimba add` executes that repo's `post_create` and `install` shell scripts.** Always review `.rimba/settings.toml` before running rimba in a repository you do not control.
153+
154+
As a defense-in-depth measure, the `copy_files` entries and `deps.modules[].lockfile` paths are validated to stay within the worktree directory — paths that escape via `..` are rejected with an error. Note that `worktree_dir` is intentionally not subject to this constraint, because its default value (`../<repo>-worktrees`) is itself a `../`-relative path.
155+
144156
## Global flags
145157

146158
| Flag | Description |

internal/deps/hash.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@ import (
44
"crypto/sha256"
55
"fmt"
66
"os"
7-
"path/filepath"
7+
8+
"github.com/lugassawan/rimba/internal/fileutil"
89
)
910

1011
// ModuleWithHash pairs a Module with its lockfile hash.
@@ -16,7 +17,11 @@ type ModuleWithHash struct {
1617
// HashLockfile computes the SHA-256 hex digest of a lockfile.
1718
// Returns an empty string if the file doesn't exist.
1819
func HashLockfile(worktreePath, lockfile string) (string, error) {
19-
data, err := os.ReadFile(filepath.Join(worktreePath, lockfile))
20+
path, err := fileutil.ContainedJoin(worktreePath, lockfile)
21+
if err != nil {
22+
return "", err
23+
}
24+
data, err := os.ReadFile(path)
2025
if err != nil {
2126
if os.IsNotExist(err) {
2227
return "", nil

internal/deps/hash_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
package deps
22

33
import (
4+
"errors"
5+
"strings"
46
"testing"
7+
8+
"github.com/lugassawan/rimba/internal/fileutil"
59
)
610

711
const testLockfile = "lock.yaml"
@@ -57,6 +61,21 @@ func TestHashLockfileDifferentContent(t *testing.T) {
5761
}
5862
}
5963

64+
func TestHashLockfileRejectsTraversal(t *testing.T) {
65+
dir := t.TempDir()
66+
67+
_, err := HashLockfile(dir, "../outside")
68+
if err == nil {
69+
t.Fatal("HashLockfile with traversal path: expected error, got nil")
70+
}
71+
if !errors.Is(err, fileutil.ErrPathEscapes) {
72+
t.Fatalf("error %v does not wrap fileutil.ErrPathEscapes", err)
73+
}
74+
if !strings.Contains(err.Error(), "contains ..") {
75+
t.Fatalf("error %q does not contain %q", err.Error(), "contains ..")
76+
}
77+
}
78+
6079
func TestHashModules(t *testing.T) {
6180
dir := t.TempDir()
6281
writeFile(t, dir, LockfilePnpm, "lockfile content")

internal/fileutil/copy.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,14 @@ const copyErrFmt = "copy %s: %w"
1515
func CopyEntries(src, dst string, entries []string) (copied []string, skippedSymlinks []string, err error) {
1616
copied = make([]string, 0, len(entries))
1717
for _, name := range entries {
18-
srcPath := filepath.Join(src, name)
19-
dstPath := filepath.Join(dst, name)
18+
srcPath, err := ContainedJoin(src, name)
19+
if err != nil {
20+
return copied, skippedSymlinks, fmt.Errorf(copyErrFmt, name, err)
21+
}
22+
dstPath, err := ContainedJoin(dst, name)
23+
if err != nil {
24+
return copied, skippedSymlinks, fmt.Errorf(copyErrFmt, name, err)
25+
}
2026

2127
ok, syms, copyErr := copyEntry(srcPath, dstPath, name)
2228
if copyErr != nil {
@@ -125,7 +131,7 @@ func copyFile(src, dst string) (retErr error) {
125131
return err
126132
}
127133

128-
out, err := os.OpenFile(filepath.Clean(dst), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, info.Mode()) //nolint:gosec // dst is derived from config copy_files, not user input
134+
out, err := os.OpenFile(filepath.Clean(dst), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, info.Mode()) //nolint:gosec // dst validated by ContainedJoin before copyEntry is called
129135
if err != nil {
130136
return err
131137
}

internal/fileutil/copy_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package fileutil_test
22

33
import (
4+
"errors"
45
"os"
56
"path/filepath"
67
"reflect"
@@ -610,6 +611,25 @@ func TestCopyEntriesDirCopyFileError(t *testing.T) {
610611
}
611612
}
612613

614+
func TestCopyEntriesRejectsTraversal(t *testing.T) {
615+
src := t.TempDir()
616+
dst := t.TempDir()
617+
618+
copied, _, err := fileutil.CopyEntries(src, dst, []string{"../escape"})
619+
if err == nil {
620+
t.Fatalf("CopyEntries with traversal path: expected error, got nil (copied=%v)", copied)
621+
}
622+
if !errors.Is(err, fileutil.ErrPathEscapes) {
623+
t.Fatalf("error %v does not wrap fileutil.ErrPathEscapes", err)
624+
}
625+
if !strings.Contains(err.Error(), "contains ..") {
626+
t.Fatalf("error %q does not contain %q", err.Error(), "contains ..")
627+
}
628+
if len(copied) != 0 {
629+
t.Errorf("copied = %v, want empty on traversal error", copied)
630+
}
631+
}
632+
613633
func TestCopyEntriesRecursiveCopyDirError(t *testing.T) {
614634
src := t.TempDir()
615635
dst := t.TempDir()

internal/fileutil/path.go

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package fileutil
2+
3+
import (
4+
"errors"
5+
"fmt"
6+
"path/filepath"
7+
"strings"
8+
)
9+
10+
// ErrPathEscapes is returned by ContainedJoin when a name escapes the base directory.
11+
var ErrPathEscapes = errors.New("path escapes base directory")
12+
13+
// ContainedJoin joins name onto base and verifies the result stays within base.
14+
// A name that nets back inside base (e.g. "sub/../file") is allowed; one that
15+
// escapes via ".." is rejected. base must be a cleaned directory path.
16+
func ContainedJoin(base, name string) (string, error) {
17+
base = filepath.Clean(base)
18+
joined := filepath.Join(base, name)
19+
if joined != base && !strings.HasPrefix(joined, base+string(filepath.Separator)) {
20+
return "", fmt.Errorf("path %q contains ..: %w", name, ErrPathEscapes)
21+
}
22+
return joined, nil
23+
}

internal/fileutil/path_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
package fileutil_test
2+
3+
import (
4+
"errors"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
9+
"github.com/lugassawan/rimba/internal/fileutil"
10+
)
11+
12+
func TestContainedJoin(t *testing.T) {
13+
base := t.TempDir()
14+
15+
tests := []struct {
16+
name string
17+
input string
18+
wantErr bool
19+
}{
20+
{name: "simple file", input: "file.txt"},
21+
{name: "nested file", input: "sub/file.txt"},
22+
{name: "net-inside traversal", input: "sub/../file.txt"},
23+
{name: "escape one level", input: "../x", wantErr: true},
24+
{name: "escape two levels", input: "../../x", wantErr: true},
25+
{name: "escape after descent", input: "a/../../x", wantErr: true},
26+
}
27+
28+
for _, tc := range tests {
29+
t.Run(tc.name, func(t *testing.T) {
30+
got, err := fileutil.ContainedJoin(base, tc.input)
31+
if tc.wantErr {
32+
if err == nil {
33+
t.Fatalf("ContainedJoin(%q, %q) = %q, nil; want error", base, tc.input, got)
34+
}
35+
if !errors.Is(err, fileutil.ErrPathEscapes) {
36+
t.Fatalf("error %v is not ErrPathEscapes", err)
37+
}
38+
if !strings.Contains(err.Error(), "contains ..") {
39+
t.Fatalf("error %q does not contain %q", err.Error(), "contains ..")
40+
}
41+
return
42+
}
43+
if err != nil {
44+
t.Fatalf("ContainedJoin(%q, %q) unexpected error: %v", base, tc.input, err)
45+
}
46+
if !strings.HasPrefix(got, base) {
47+
t.Fatalf("result %q is not inside base %q", got, base)
48+
}
49+
if got != filepath.Join(base, tc.input) {
50+
t.Fatalf("ContainedJoin(%q, %q) = %q; want %q", base, tc.input, got, filepath.Join(base, tc.input))
51+
}
52+
})
53+
}
54+
}

0 commit comments

Comments
 (0)