Skip to content

Commit 50e1c08

Browse files
Dumbrisclaude
andcommitted
fix(connect): reject traversal backup paths in undo (defense-in-depth)
The undo backup-path guard was prefix-only: a path like "<config>.bak.x/../../secret.json" kept the "<config>.bak." prefix yet escaped the config directory, letting undo read (and, in a crafted case, restore) an arbitrary file — defeating the documented "must not become an arbitrary-file-restore primitive" invariant. Anchor validation on the cleaned path's parent directory and basename instead, both traversal-proof. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7f516db commit 50e1c08

2 files changed

Lines changed: 45 additions & 4 deletions

File tree

internal/connect/undo.go

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"bytes"
55
"fmt"
66
"os"
7+
"path/filepath"
78
"strings"
89

910
"github.com/BurntSushi/toml"
@@ -50,10 +51,18 @@ func (s *Service) Undo(clientID, serverName, backupPath string) (*ConnectResult,
5051
return nil, fmt.Errorf("cannot determine config path for %s", clientID)
5152
}
5253

53-
// The backup must be a backup OF THIS client's config: undo must not become
54-
// an arbitrary-file-restore primitive for API callers.
55-
if backupPath != "" && !strings.HasPrefix(backupPath, cfgPath+".bak.") {
56-
return nil, fmt.Errorf("invalid backup path %q: not a backup of %s", backupPath, cfgPath)
54+
// The backup must be a real backup OF THIS client's config, living beside it:
55+
// undo must not become an arbitrary-file-restore primitive for API callers.
56+
// A prefix-only check is not enough — a path like "<cfg>.bak.x/../../secret"
57+
// keeps the prefix yet escapes the directory, so anchor on the cleaned path's
58+
// parent directory and basename (both traversal-proof).
59+
if backupPath != "" {
60+
cleaned := filepath.Clean(backupPath)
61+
if filepath.Dir(cleaned) != filepath.Dir(cfgPath) ||
62+
!strings.HasPrefix(filepath.Base(cleaned), filepath.Base(cfgPath)+".bak.") {
63+
return nil, fmt.Errorf("invalid backup path %q: not a backup of %s", backupPath, cfgPath)
64+
}
65+
backupPath = cleaned
5766
}
5867

5968
res, err := s.undo(client, cfgPath, serverName, backupPath)

internal/connect/undo_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,38 @@ func TestUndo_RejectsForeignBackupPath(t *testing.T) {
247247
}
248248
}
249249

250+
// A path that keeps the "<config>.bak." prefix but traverses out of the config
251+
// directory (e.g. "<config>.bak.x/../../secret.json") must be rejected before
252+
// any read — a prefix-only check would let undo read/restore an arbitrary file.
253+
func TestUndo_RejectsBackupPathTraversal(t *testing.T) {
254+
home := t.TempDir()
255+
svc := NewServiceWithHome("127.0.0.1:8080", "", home)
256+
res, err := svc.Connect("claude-code", "mcpproxy", false)
257+
if err != nil {
258+
t.Fatalf("connect: %v", err)
259+
}
260+
261+
// Craft a traversal path that still starts with "<config>.bak." so a
262+
// prefix-only guard would admit it, then climbs back out to a foreign file.
263+
secret := filepath.Join(home, "secret.json")
264+
if err := os.WriteFile(secret, []byte(`{"secret":true}`), 0o644); err != nil {
265+
t.Fatal(err)
266+
}
267+
rel, err := filepath.Rel(filepath.Dir(res.ConfigPath), secret)
268+
if err != nil {
269+
t.Fatal(err)
270+
}
271+
traversal := res.ConfigPath + ".bak.x/../" + rel
272+
273+
_, err = svc.Undo("claude-code", "mcpproxy", traversal)
274+
if err == nil {
275+
t.Fatal("undo must reject a traversal backup path that escapes the config directory")
276+
}
277+
if !strings.Contains(err.Error(), "backup") {
278+
t.Fatalf("error should mention the backup path problem: %v", err)
279+
}
280+
}
281+
250282
func TestUndo_MissingConfigFileRefuses(t *testing.T) {
251283
home := t.TempDir()
252284
svc := NewServiceWithHome("127.0.0.1:8080", "", home)

0 commit comments

Comments
 (0)