mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-13 10:15:48 +00:00
fix(agent): close symlink escape + path-alias dedup + TTS mime type
Review of the medium-batch landed this session surfaced three issues:
P0 — symlink-ancestor escape in extractMediaFromContent. The lexical
filepath.Rel check passed "<ws>/shared/secret" when "shared" was a
symlink pointing outside the workspace. EvalSymlinks the resolved path
(and the workspace root, since macOS uses /var → /private/var) before
the Rel containment check. Leaf symlinks still rejected by Lstat.
P1 — duplicate attachments from path aliases. parseMediaResult stores
raw tool output ("./tts/x.mp3"); extractMediaFromContent stores Abs+
Clean ("<ws>/tts/x.mp3"). deduplicateMedia's exact-string map let both
through. Normalize the dedup key via filepath.Abs + Clean so the
sources collapse.
P1 — TTS Result.Media used the non-standard "audio/mp3" via
"audio/"+Extension. SynthResult.MimeType is already populated by every
provider with the RFC-3003 "audio/mpeg" (or "audio/ogg" for opus).
Prefer it; keep the extension concat as empty-string fallback.
Tests: add symlink-leaf-rejected and ancestor-symlink-escape cases to
lock the P0 behavior.
This commit is contained in:
1 parent
2a61365353
commit
3b445d6856
3 files changed
+78
-14
No files matched your search
@@ -65,9 +65,17 @@ func extractMediaFromContent(content, workspace string) []MediaResult {
|
||||
if len(matches) == 0 {
|
||||
return nil
|
||||
}
|
||||
// Resolve workspace to its real path (follows symlinks). Required because
|
||||
// macOS uses symlinks for /tmp → /private/tmp; if we only Clean the
|
||||
// workspace but EvalSymlinks the extracted paths, the Rel check below
|
||||
// would spuriously fail even for legitimate files.
|
||||
wsRoot := ""
|
||||
if abs, err := filepath.Abs(workspace); err == nil {
|
||||
wsRoot = filepath.Clean(abs)
|
||||
if resolved, err := filepath.EvalSymlinks(abs); err == nil {
|
||||
wsRoot = filepath.Clean(resolved)
|
||||
} else {
|
||||
wsRoot = filepath.Clean(abs)
|
||||
}
|
||||
}
|
||||
if wsRoot == "" {
|
||||
return nil
|
||||
@@ -90,18 +98,24 @@ func extractMediaFromContent(content, workspace string) []MediaResult {
|
||||
path = filepath.Join(wsRoot, path)
|
||||
}
|
||||
cleaned := filepath.Clean(path)
|
||||
// Workspace containment check — blocks "../" escapes and absolute paths
|
||||
// outside the agent's allowed area.
|
||||
rel, err := filepath.Rel(wsRoot, cleaned)
|
||||
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
|
||||
continue
|
||||
}
|
||||
// Existence + regular-file check prevents hallucinated paths and
|
||||
// directory/symlink-device shenanigans from reaching the outbound queue.
|
||||
// Existence + regular-file check. Lstat (not Stat) so a symlink at
|
||||
// the leaf is rejected outright.
|
||||
info, err := os.Lstat(cleaned)
|
||||
if err != nil || !info.Mode().IsRegular() {
|
||||
continue
|
||||
}
|
||||
// Resolve ancestor symlinks THEN check containment. A purely lexical
|
||||
// Rel check would pass "<ws>/<symlink-dir>/secret" when symlink-dir
|
||||
// points outside the workspace; EvalSymlinks closes that escape.
|
||||
resolved, err := filepath.EvalSymlinks(cleaned)
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
rel, err := filepath.Rel(wsRoot, resolved)
|
||||
if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
|
||||
continue
|
||||
}
|
||||
cleaned = resolved
|
||||
if _, dup := seen[cleaned]; dup {
|
||||
continue
|
||||
}
|
||||
@@ -114,7 +128,12 @@ func extractMediaFromContent(content, workspace string) []MediaResult {
|
||||
return results
|
||||
}
|
||||
|
||||
// deduplicateMedia removes duplicate media results by path, keeping the first occurrence.
|
||||
// deduplicateMedia removes duplicate media results, normalizing each path
|
||||
// so legacy entries from parseMediaResult (raw tool output — may contain
|
||||
// "./" segments or relative forms) collapse cleanly with entries from
|
||||
// extractMediaFromContent (already Clean'd + Abs'd). Prevents double-send
|
||||
// when a tool returns MEDIA:./x.mp3 and the LLM echoes it back in a form
|
||||
// that resolves to the same file.
|
||||
func deduplicateMedia(media []MediaResult) []MediaResult {
|
||||
if len(media) <= 1 {
|
||||
return media
|
||||
@@ -122,10 +141,16 @@ func deduplicateMedia(media []MediaResult) []MediaResult {
|
||||
seen := make(map[string]bool, len(media))
|
||||
result := make([]MediaResult, 0, len(media))
|
||||
for _, m := range media {
|
||||
if seen[m.Path] {
|
||||
key := m.Path
|
||||
if abs, err := filepath.Abs(key); err == nil {
|
||||
key = filepath.Clean(abs)
|
||||
} else {
|
||||
key = filepath.Clean(key)
|
||||
}
|
||||
if seen[key] {
|
||||
continue
|
||||
}
|
||||
seen[m.Path] = true
|
||||
seen[key] = true
|
||||
result = append(result, m)
|
||||
}
|
||||
return result
|
||||
|
||||
@@ -20,7 +20,13 @@ func writeTempFile(t *testing.T, workspace, relPath string) string {
|
||||
}
|
||||
|
||||
func TestExtractMediaFromContent(t *testing.T) {
|
||||
ws := t.TempDir()
|
||||
wsRaw := t.TempDir()
|
||||
// Resolve workspace symlinks up front (macOS has /var → /private/var) so
|
||||
// expected paths match what the extractor returns after EvalSymlinks.
|
||||
ws, err := filepath.EvalSymlinks(wsRaw)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
reportPath := writeTempFile(t, ws, "deliver/report.pdf")
|
||||
audioA := writeTempFile(t, ws, "a.mp3")
|
||||
audioB := writeTempFile(t, ws, "b.mp3")
|
||||
@@ -33,6 +39,23 @@ func TestExtractMediaFromContent(t *testing.T) {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// Symlink inside workspace pointing to outside: must be rejected by
|
||||
// EvalSymlinks-then-Rel containment. Covers the P0 ancestor-symlink
|
||||
// escape the lexical-only Rel check would have allowed.
|
||||
symlinkFile := filepath.Join(ws, "shortcut-to-leak.pdf")
|
||||
if err := os.Symlink(outsidePath, symlinkFile); err != nil {
|
||||
t.Skipf("symlink not supported: %v", err)
|
||||
}
|
||||
// Ancestor symlink case: dir symlink inside ws pointing outside.
|
||||
symDirParent := t.TempDir()
|
||||
if err := os.WriteFile(filepath.Join(symDirParent, "victim.pdf"), nil, 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
ancestorSym := filepath.Join(ws, "shared")
|
||||
if err := os.Symlink(symDirParent, ancestorSym); err != nil {
|
||||
t.Skipf("symlink not supported: %v", err)
|
||||
}
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
content string
|
||||
@@ -84,6 +107,16 @@ func TestExtractMediaFromContent(t *testing.T) {
|
||||
name: "absolute path with no workspace dropped",
|
||||
content: "MEDIA:" + reportPath,
|
||||
},
|
||||
{
|
||||
name: "symlink leaf rejected by Lstat",
|
||||
content: "MEDIA:shortcut-to-leak.pdf",
|
||||
workspace: ws,
|
||||
},
|
||||
{
|
||||
name: "ancestor symlink escape blocked (P0)",
|
||||
content: "MEDIA:shared/victim.pdf",
|
||||
workspace: ws,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
|
||||
@@ -242,11 +242,17 @@ func (t *TtsTool) Execute(ctx context.Context, args map[string]any) *Result {
|
||||
// Set Result.Media explicitly (matching create_audio) so the agent loop's
|
||||
// media collector uses the authoritative path even when the ForLLM
|
||||
// MEDIA: prefix is reshaped by a provider bridge (e.g. claude_cli MCP).
|
||||
// Prefer the provider-supplied MimeType ("audio/mpeg", "audio/ogg") over
|
||||
// "audio/"+Extension — the latter yields the non-standard "audio/mp3".
|
||||
mimeType := result.MimeType
|
||||
if mimeType == "" {
|
||||
mimeType = "audio/" + result.Extension
|
||||
}
|
||||
r := &Result{
|
||||
ForLLM: forLLM,
|
||||
Media: []bus.MediaFile{{
|
||||
Path: audioPath,
|
||||
MimeType: "audio/" + result.Extension,
|
||||
MimeType: mimeType,
|
||||
}},
|
||||
}
|
||||
r.Deliverable = fmt.Sprintf("[Generated audio: %s]\nText: %s", filepath.Base(audioPath), text)
|
||||
|
||||
Reference in new issue
Block a user