From b54daae418a6ecc24f8ce01130b66b3dc9f93aab Mon Sep 17 00:00:00 2001 From: Snider Date: Fri, 17 Apr 2026 20:31:06 +0100 Subject: [PATCH] fix(reference): harden core reference edge cases Co-Authored-By: Virgil --- .core/TODO.md | 1 + .core/reference/cli.go | 13 +++++++- .core/reference/error.go | 5 +++ .core/reference/fs.go | 33 ++++++++++++++++--- .core/reference/runtime.go | 4 +++ .../workspace/default/.core/reference/cli.go | 13 +++++++- .../default/.core/reference/error.go | 5 +++ .../workspace/default/.core/reference/fs.go | 33 ++++++++++++++++--- .../default/.core/reference/runtime.go | 3 ++ .../test-extract/.core/reference/cli.go | 13 +++++++- .../test-extract/.core/reference/error.go | 5 +++ .../test-extract/.core/reference/fs.go | 33 ++++++++++++++++--- .../test-extract/.core/reference/runtime.go | 3 ++ 13 files changed, 149 insertions(+), 15 deletions(-) create mode 100644 .core/TODO.md diff --git a/.core/TODO.md b/.core/TODO.md new file mode 100644 index 0000000..9b5d585 --- /dev/null +++ b/.core/TODO.md @@ -0,0 +1 @@ +- @test pkg/monitor/harvest_test.go:49 — `go test ./...` currently fails in `TestHarvest_*` because the git commit setup command exits with code 1 when adding the binary fixture. diff --git a/.core/reference/cli.go b/.core/reference/cli.go index 5e4b9f7..1f375d4 100644 --- a/.core/reference/cli.go +++ b/.core/reference/cli.go @@ -103,7 +103,18 @@ func (cl *Cli) Run(args ...string) Result { opts.Set(key, true) } } else if !IsFlag(arg) { - opts.Set("_arg", arg) + if !opts.Has("_arg") { + opts.Set("_arg", arg) + } + argsResult := opts.Get("_args") + args := []string{} + if argsResult.OK { + if existing, ok := argsResult.Value.([]string); ok { + args = append(args, existing...) + } + } + args = append(args, arg) + opts.Set("_args", args) } } diff --git a/.core/reference/error.go b/.core/reference/error.go index d562494..c56ea7c 100644 --- a/.core/reference/error.go +++ b/.core/reference/error.go @@ -375,6 +375,11 @@ func (h *ErrorPanic) appendReport(report CrashReport) { var reports []CrashReport if data, err := os.ReadFile(h.filePath); err == nil { if err := json.Unmarshal(data, &reports); err != nil { + Default().Error(Concat("crash report file corrupted path=", h.filePath, " err=", err.Error(), " raw=", string(data))) + backupPath := Concat(h.filePath, ".corrupt") + if backupErr := os.WriteFile(backupPath, data, 0600); backupErr != nil { + Default().Error(Concat("crash report backup failed path=", h.filePath, " err=", backupErr.Error())) + } reports = nil } } diff --git a/.core/reference/fs.go b/.core/reference/fs.go index d37b8f8..7f75fa9 100644 --- a/.core/reference/fs.go +++ b/.core/reference/fs.go @@ -177,10 +177,20 @@ func (m *Fs) WriteMode(p, content string, mode os.FileMode) Result { // dir := fs.TempDir("agent-workspace") // defer fs.DeleteAll(dir) func (m *Fs) TempDir(prefix string) string { - dir, err := os.MkdirTemp("", prefix) + root := m.root + if root == "" || root == "/" { + root = os.TempDir() + } else if err := os.MkdirAll(root, 0755); err != nil { + return "" + } + dir, err := os.MkdirTemp(root, prefix) if err != nil { return "" } + if vp := m.validatePath(dir); !vp.OK { + os.RemoveAll(dir) + return "" + } return dir } @@ -358,15 +368,30 @@ func WriteAll(writer any, content string) Result { return Result{E("core.WriteAll", "not a writer", nil), false} } _, err := wc.Write([]byte(content)) + var closeErr error if closer, ok := writer.(io.Closer); ok { - closer.Close() + closeErr = closer.Close() } if err != nil { return Result{err, false} } + if closeErr != nil { + return Result{closeErr, false} + } return Result{OK: true} } +func (m *Fs) isProtectedPath(full string) bool { + if full == "/" { + return true + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return false + } + return full == home +} + // CloseStream closes any value that implements io.Closer. // // core.CloseStream(r.Value) @@ -383,7 +408,7 @@ func (m *Fs) Delete(p string) Result { return vp } full := vp.Value.(string) - if full == "/" || full == os.Getenv("HOME") { + if m.isProtectedPath(full) { return Result{E("fs.Delete", Concat("refusing to delete protected path: ", full), nil), false} } if err := os.Remove(full); err != nil { @@ -399,7 +424,7 @@ func (m *Fs) DeleteAll(p string) Result { return vp } full := vp.Value.(string) - if full == "/" || full == os.Getenv("HOME") { + if m.isProtectedPath(full) { return Result{E("fs.DeleteAll", Concat("refusing to delete protected path: ", full), nil), false} } if err := os.RemoveAll(full); err != nil { diff --git a/.core/reference/runtime.go b/.core/reference/runtime.go index c9a8223..92e6e14 100644 --- a/.core/reference/runtime.go +++ b/.core/reference/runtime.go @@ -153,8 +153,12 @@ func (r *Runtime) ServiceName() string { return "Core" } // ServiceStartup starts all services via the embedded Core. func (r *Runtime) ServiceStartup(ctx context.Context, options any) Result { + if r == nil || r.Core == nil { + return Result{OK: true} + } return r.Core.ServiceStartup(ctx, options) } + // ServiceShutdown stops all services via the embedded Core. func (r *Runtime) ServiceShutdown(ctx context.Context) Result { if r.Core != nil { diff --git a/pkg/lib/workspace/default/.core/reference/cli.go b/pkg/lib/workspace/default/.core/reference/cli.go index 5e4b9f7..1f375d4 100644 --- a/pkg/lib/workspace/default/.core/reference/cli.go +++ b/pkg/lib/workspace/default/.core/reference/cli.go @@ -103,7 +103,18 @@ func (cl *Cli) Run(args ...string) Result { opts.Set(key, true) } } else if !IsFlag(arg) { - opts.Set("_arg", arg) + if !opts.Has("_arg") { + opts.Set("_arg", arg) + } + argsResult := opts.Get("_args") + args := []string{} + if argsResult.OK { + if existing, ok := argsResult.Value.([]string); ok { + args = append(args, existing...) + } + } + args = append(args, arg) + opts.Set("_args", args) } } diff --git a/pkg/lib/workspace/default/.core/reference/error.go b/pkg/lib/workspace/default/.core/reference/error.go index 182fd14..0913757 100644 --- a/pkg/lib/workspace/default/.core/reference/error.go +++ b/pkg/lib/workspace/default/.core/reference/error.go @@ -396,6 +396,11 @@ func (h *ErrorPanic) appendReport(report CrashReport) { var reports []CrashReport if data, err := os.ReadFile(h.filePath); err == nil { if err := json.Unmarshal(data, &reports); err != nil { + Default().Error(Concat("crash report file corrupted path=", h.filePath, " err=", err.Error(), " raw=", string(data))) + backupPath := Concat(h.filePath, ".corrupt") + if backupErr := os.WriteFile(backupPath, data, 0600); backupErr != nil { + Default().Error(Concat("crash report backup failed path=", h.filePath, " err=", backupErr.Error())) + } reports = nil } } diff --git a/pkg/lib/workspace/default/.core/reference/fs.go b/pkg/lib/workspace/default/.core/reference/fs.go index d37b8f8..7f75fa9 100644 --- a/pkg/lib/workspace/default/.core/reference/fs.go +++ b/pkg/lib/workspace/default/.core/reference/fs.go @@ -177,10 +177,20 @@ func (m *Fs) WriteMode(p, content string, mode os.FileMode) Result { // dir := fs.TempDir("agent-workspace") // defer fs.DeleteAll(dir) func (m *Fs) TempDir(prefix string) string { - dir, err := os.MkdirTemp("", prefix) + root := m.root + if root == "" || root == "/" { + root = os.TempDir() + } else if err := os.MkdirAll(root, 0755); err != nil { + return "" + } + dir, err := os.MkdirTemp(root, prefix) if err != nil { return "" } + if vp := m.validatePath(dir); !vp.OK { + os.RemoveAll(dir) + return "" + } return dir } @@ -358,15 +368,30 @@ func WriteAll(writer any, content string) Result { return Result{E("core.WriteAll", "not a writer", nil), false} } _, err := wc.Write([]byte(content)) + var closeErr error if closer, ok := writer.(io.Closer); ok { - closer.Close() + closeErr = closer.Close() } if err != nil { return Result{err, false} } + if closeErr != nil { + return Result{closeErr, false} + } return Result{OK: true} } +func (m *Fs) isProtectedPath(full string) bool { + if full == "/" { + return true + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return false + } + return full == home +} + // CloseStream closes any value that implements io.Closer. // // core.CloseStream(r.Value) @@ -383,7 +408,7 @@ func (m *Fs) Delete(p string) Result { return vp } full := vp.Value.(string) - if full == "/" || full == os.Getenv("HOME") { + if m.isProtectedPath(full) { return Result{E("fs.Delete", Concat("refusing to delete protected path: ", full), nil), false} } if err := os.Remove(full); err != nil { @@ -399,7 +424,7 @@ func (m *Fs) DeleteAll(p string) Result { return vp } full := vp.Value.(string) - if full == "/" || full == os.Getenv("HOME") { + if m.isProtectedPath(full) { return Result{E("fs.DeleteAll", Concat("refusing to delete protected path: ", full), nil), false} } if err := os.RemoveAll(full); err != nil { diff --git a/pkg/lib/workspace/default/.core/reference/runtime.go b/pkg/lib/workspace/default/.core/reference/runtime.go index 55f7ec9..33d3e62 100644 --- a/pkg/lib/workspace/default/.core/reference/runtime.go +++ b/pkg/lib/workspace/default/.core/reference/runtime.go @@ -167,6 +167,9 @@ func (r *Runtime) ServiceName() string { return "Core" } // // runtime.ServiceStartup(context.Background(), nil) func (r *Runtime) ServiceStartup(ctx context.Context, options any) Result { + if r == nil || r.Core == nil { + return Result{OK: true} + } return r.Core.ServiceStartup(ctx, options) } diff --git a/tests/cli/extract/.core/workspace/test-extract/.core/reference/cli.go b/tests/cli/extract/.core/workspace/test-extract/.core/reference/cli.go index 5e4b9f7..1f375d4 100644 --- a/tests/cli/extract/.core/workspace/test-extract/.core/reference/cli.go +++ b/tests/cli/extract/.core/workspace/test-extract/.core/reference/cli.go @@ -103,7 +103,18 @@ func (cl *Cli) Run(args ...string) Result { opts.Set(key, true) } } else if !IsFlag(arg) { - opts.Set("_arg", arg) + if !opts.Has("_arg") { + opts.Set("_arg", arg) + } + argsResult := opts.Get("_args") + args := []string{} + if argsResult.OK { + if existing, ok := argsResult.Value.([]string); ok { + args = append(args, existing...) + } + } + args = append(args, arg) + opts.Set("_args", args) } } diff --git a/tests/cli/extract/.core/workspace/test-extract/.core/reference/error.go b/tests/cli/extract/.core/workspace/test-extract/.core/reference/error.go index 182fd14..0913757 100644 --- a/tests/cli/extract/.core/workspace/test-extract/.core/reference/error.go +++ b/tests/cli/extract/.core/workspace/test-extract/.core/reference/error.go @@ -396,6 +396,11 @@ func (h *ErrorPanic) appendReport(report CrashReport) { var reports []CrashReport if data, err := os.ReadFile(h.filePath); err == nil { if err := json.Unmarshal(data, &reports); err != nil { + Default().Error(Concat("crash report file corrupted path=", h.filePath, " err=", err.Error(), " raw=", string(data))) + backupPath := Concat(h.filePath, ".corrupt") + if backupErr := os.WriteFile(backupPath, data, 0600); backupErr != nil { + Default().Error(Concat("crash report backup failed path=", h.filePath, " err=", backupErr.Error())) + } reports = nil } } diff --git a/tests/cli/extract/.core/workspace/test-extract/.core/reference/fs.go b/tests/cli/extract/.core/workspace/test-extract/.core/reference/fs.go index d37b8f8..7f75fa9 100644 --- a/tests/cli/extract/.core/workspace/test-extract/.core/reference/fs.go +++ b/tests/cli/extract/.core/workspace/test-extract/.core/reference/fs.go @@ -177,10 +177,20 @@ func (m *Fs) WriteMode(p, content string, mode os.FileMode) Result { // dir := fs.TempDir("agent-workspace") // defer fs.DeleteAll(dir) func (m *Fs) TempDir(prefix string) string { - dir, err := os.MkdirTemp("", prefix) + root := m.root + if root == "" || root == "/" { + root = os.TempDir() + } else if err := os.MkdirAll(root, 0755); err != nil { + return "" + } + dir, err := os.MkdirTemp(root, prefix) if err != nil { return "" } + if vp := m.validatePath(dir); !vp.OK { + os.RemoveAll(dir) + return "" + } return dir } @@ -358,15 +368,30 @@ func WriteAll(writer any, content string) Result { return Result{E("core.WriteAll", "not a writer", nil), false} } _, err := wc.Write([]byte(content)) + var closeErr error if closer, ok := writer.(io.Closer); ok { - closer.Close() + closeErr = closer.Close() } if err != nil { return Result{err, false} } + if closeErr != nil { + return Result{closeErr, false} + } return Result{OK: true} } +func (m *Fs) isProtectedPath(full string) bool { + if full == "/" { + return true + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return false + } + return full == home +} + // CloseStream closes any value that implements io.Closer. // // core.CloseStream(r.Value) @@ -383,7 +408,7 @@ func (m *Fs) Delete(p string) Result { return vp } full := vp.Value.(string) - if full == "/" || full == os.Getenv("HOME") { + if m.isProtectedPath(full) { return Result{E("fs.Delete", Concat("refusing to delete protected path: ", full), nil), false} } if err := os.Remove(full); err != nil { @@ -399,7 +424,7 @@ func (m *Fs) DeleteAll(p string) Result { return vp } full := vp.Value.(string) - if full == "/" || full == os.Getenv("HOME") { + if m.isProtectedPath(full) { return Result{E("fs.DeleteAll", Concat("refusing to delete protected path: ", full), nil), false} } if err := os.RemoveAll(full); err != nil { diff --git a/tests/cli/extract/.core/workspace/test-extract/.core/reference/runtime.go b/tests/cli/extract/.core/workspace/test-extract/.core/reference/runtime.go index 55f7ec9..33d3e62 100644 --- a/tests/cli/extract/.core/workspace/test-extract/.core/reference/runtime.go +++ b/tests/cli/extract/.core/workspace/test-extract/.core/reference/runtime.go @@ -167,6 +167,9 @@ func (r *Runtime) ServiceName() string { return "Core" } // // runtime.ServiceStartup(context.Background(), nil) func (r *Runtime) ServiceStartup(ctx context.Context, options any) Result { + if r == nil || r.Core == nil { + return Result{OK: true} + } return r.Core.ServiceStartup(ctx, options) }