Skip to content

Commit 35ce8b9

Browse files
ampagentPierrunoYT
andcommitted
fix(sandbox): parse quoted patch paths
Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfb-b1e9-765e-ac9c-240512dfcd2f Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
1 parent 83f8578 commit 35ce8b9

2 files changed

Lines changed: 52 additions & 3 deletions

File tree

internal/sandbox/risk.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -325,15 +325,31 @@ func patchHeaderPaths(patch string) []string {
325325
oldRemaining, newRemaining = parsePatchHunkCounts(line)
326326
inHunk = oldRemaining > 0 || newRemaining > 0
327327
case strings.HasPrefix(line, "--- "), strings.HasPrefix(line, "+++ "):
328-
fields := strings.Fields(line)
329-
if len(fields) >= 2 {
330-
paths = append(paths, stripPatchPrefix(fields[1]))
328+
if path := patchFileHeaderPath(line); path != "" {
329+
paths = append(paths, stripPatchPrefix(path))
331330
}
332331
}
333332
}
334333
return paths
335334
}
336335

336+
// patchFileHeaderPath mirrors the apply_patch parser's handling of unified-diff
337+
// file headers: paths may be C-quoted and may contain spaces, while an optional
338+
// timestamp is separated by a tab.
339+
func patchFileHeaderPath(line string) string {
340+
rest := line[len("--- "):] // "--- " and "+++ " are both 4 bytes
341+
if tab := strings.IndexByte(rest, '\t'); tab >= 0 {
342+
rest = rest[:tab]
343+
}
344+
rest = strings.TrimSpace(rest)
345+
if len(rest) >= 2 && rest[0] == '"' && rest[len(rest)-1] == '"' {
346+
if unquoted, err := strconv.Unquote(rest); err == nil {
347+
return unquoted
348+
}
349+
}
350+
return rest
351+
}
352+
337353
func parsePatchHunkCounts(line string) (int, int) {
338354
_, rest, ok := strings.Cut(line, "@@")
339355
if !ok {

internal/tools/daemon_token_exclusion_test.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,39 @@ func TestEngineDeniesDaemonTokenFileTools(t *testing.T) {
171171
}
172172
}
173173

174+
func TestApplyPatchDeniesQuotedDaemonTokenPathWithSpaces(t *testing.T) {
175+
ws, err := filepath.EvalSymlinks(t.TempDir())
176+
if err != nil {
177+
t.Fatalf("EvalSymlinks: %v", err)
178+
}
179+
token := filepath.Join(ws, "bridge token")
180+
if err := os.WriteFile(token, []byte("bridge-secret\n"), 0o600); err != nil {
181+
t.Fatalf("write token: %v", err)
182+
}
183+
t.Setenv(remote.EnvToken, "")
184+
t.Setenv(remote.EnvTokenFile, token)
185+
engine := sandbox.NewEngine(sandbox.EngineOptions{WorkspaceRoot: ws, Policy: sandbox.DefaultPolicy()})
186+
187+
registry := NewRegistry()
188+
registry.Register(NewScopedApplyPatchTool(ws, nil))
189+
patch := "diff --git \"a/bridge token\" \"b/bridge token\"\n" +
190+
"--- \"a/bridge token\"\n" +
191+
"+++ \"b/bridge token\"\n" +
192+
"@@ -1 +1 @@\n" +
193+
"-bridge-secret\n" +
194+
"+attacker-controlled\n"
195+
result := registry.RunWithOptions(context.Background(), "apply_patch", map[string]any{
196+
"patch": patch,
197+
}, RunOptions{Sandbox: engine, PermissionGranted: true})
198+
if result.Status == StatusOK || !strings.Contains(result.Output, "remote bridge token") {
199+
t.Fatalf("apply_patch on quoted protected path: status=%s output=%q, want bridge-token denial", result.Status, result.Output)
200+
}
201+
contents, err := os.ReadFile(token)
202+
if err != nil || string(contents) != "bridge-secret\n" {
203+
t.Fatalf("token changed after denied patch: contents=%q err=%v", contents, err)
204+
}
205+
}
206+
174207
// TestDaemonTokenAliasesDeniedEndToEnd covers the in-process tools, which are
175208
// the layer that can close inode aliases: they see every requested path before
176209
// opening it, so a symlink or hard link to the token resolves back to the

0 commit comments

Comments
 (0)