From bf1f8e51adb54721c3baaca6d6486be1e1200b88 Mon Sep 17 00:00:00 2001 From: Snider Date: Fri, 20 Mar 2026 17:35:09 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20Codex=20review=20round=202=20=E2=80=94?= =?UTF-8?q?=20path=20traversal,=20shutdown=20order,=20races?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit High: - embed.Extract: safePath validates all rendered paths stay under targetDir - embed.path: reject .. traversal on arbitrary fs.FS - ServiceShutdown: drain background tasks BEFORE stopping services Medium: - cli.Run: command lookup holds registry RLock (race fix) - NewWithFactories: propagate factory/registration failures Co-Authored-By: Virgil --- pkg/core/cli.go | 15 ++++++++++++++- pkg/core/embed.go | 34 ++++++++++++++++++++++++++++++---- pkg/core/runtime.go | 37 +++++++++++++++++++++++-------------- 3 files changed, 67 insertions(+), 19 deletions(-) diff --git a/pkg/core/cli.go b/pkg/core/cli.go index 12e4297..f658e69 100644 --- a/pkg/core/cli.go +++ b/pkg/core/cli.go @@ -50,7 +50,18 @@ func (cl *Cli) Run(args ...string) Result { clean := FilterArgs(args) - if cl.core == nil || cl.core.commands == nil || len(cl.core.commands.commands) == 0 { + if cl.core == nil || cl.core.commands == nil { + if cl.banner != nil { + cl.Print(cl.banner(cl)) + } + return Result{} + } + + cl.core.commands.mu.RLock() + cmdCount := len(cl.core.commands.commands) + cl.core.commands.mu.RUnlock() + + if cmdCount == 0 { if cl.banner != nil { cl.Print(cl.banner(cl)) } @@ -61,6 +72,7 @@ func (cl *Cli) Run(args ...string) Result { var cmd *Command var remaining []string + cl.core.commands.mu.RLock() for i := len(clean); i > 0; i-- { path := JoinPath(clean[:i]...) if c, ok := cl.core.commands.commands[path]; ok { @@ -69,6 +81,7 @@ func (cl *Cli) Run(args ...string) Result { break } } + cl.core.commands.mu.RUnlock() if cmd == nil { if cl.banner != nil { diff --git a/pkg/core/embed.go b/pkg/core/embed.go index 809226e..86cb0ba 100644 --- a/pkg/core/embed.go +++ b/pkg/core/embed.go @@ -367,7 +367,12 @@ func MountEmbed(efs embed.FS, basedir string) Result { } func (s *Embed) path(name string) string { - return filepath.ToSlash(filepath.Join(s.basedir, name)) + joined := filepath.ToSlash(filepath.Join(s.basedir, name)) + // Reject traversal outside the base directory + if HasPrefix(joined, "..") || Contains(joined, "/../") || HasSuffix(joined, "/..") { + return s.basedir + } + return joined } // Open opens the named file for reading. @@ -526,9 +531,24 @@ func Extract(fsys fs.FS, targetDir string, data any, opts ...ExtractOptions) Res return Result{err, false} } + // safePath ensures a rendered path stays under targetDir. + safePath := func(rendered string) (string, error) { + abs, err := filepath.Abs(rendered) + if err != nil { + return "", err + } + if !HasPrefix(abs, targetDir+string(filepath.Separator)) && abs != targetDir { + return "", E("embed.Extract", Concat("path escapes target: ", abs), nil) + } + return abs, nil + } + // Create directories (names may contain templates) for _, dir := range dirs { - target := renderPath(filepath.Join(targetDir, dir), data) + target, err := safePath(renderPath(filepath.Join(targetDir, dir), data)) + if err != nil { + return Result{err, false} + } if err := os.MkdirAll(target, 0755); err != nil { return Result{err, false} } @@ -552,7 +572,10 @@ func Extract(fsys fs.FS, targetDir string, data any, opts ...ExtractOptions) Res if renamed := opt.RenameFiles[name]; renamed != "" { name = renamed } - targetFile = filepath.Join(dir, name) + targetFile, err = safePath(filepath.Join(dir, name)) + if err != nil { + return Result{err, false} + } f, err := os.Create(targetFile) if err != nil { @@ -572,7 +595,10 @@ func Extract(fsys fs.FS, targetDir string, data any, opts ...ExtractOptions) Res if renamed := opt.RenameFiles[name]; renamed != "" { targetPath = filepath.Join(filepath.Dir(path), renamed) } - target := renderPath(filepath.Join(targetDir, targetPath), data) + target, err := safePath(renderPath(filepath.Join(targetDir, targetPath), data)) + if err != nil { + return Result{err, false} + } if err := copyFile(fsys, path, target); err != nil { return Result{err, false} } diff --git a/pkg/core/runtime.go b/pkg/core/runtime.go index 9e47ead..9ce010e 100644 --- a/pkg/core/runtime.go +++ b/pkg/core/runtime.go @@ -49,10 +49,24 @@ func (c *Core) ServiceStartup(ctx context.Context, options any) Result { return Result{OK: true} } -// ServiceShutdown runs OnStop for all registered services that have one. +// ServiceShutdown drains background tasks, then stops all registered services. func (c *Core) ServiceShutdown(ctx context.Context) Result { c.shutdown.Store(true) c.ACTION(ActionServiceShutdown{}) + + // Drain background tasks before stopping services + done := make(chan struct{}) + go func() { + c.wg.Wait() + close(done) + }() + select { + case <-done: + case <-ctx.Done(): + return Result{ctx.Err(), false} + } + + // Stop services var firstErr error stoppables := c.Stoppables() if stoppables.OK { @@ -68,16 +82,6 @@ func (c *Core) ServiceShutdown(ctx context.Context) Result { } } } - done := make(chan struct{}) - go func() { - c.wg.Wait() - close(done) - }() - select { - case <-done: - case <-ctx.Done(): - return Result{ctx.Err(), false} - } if firstErr != nil { return Result{firstErr, false} } @@ -108,10 +112,15 @@ func NewWithFactories(app any, factories map[string]ServiceFactory) Result { } r := factory() if !r.OK { - continue + return Result{E("core.NewWithFactories", Concat("factory \"", name, "\" failed"), nil), false} } - if svc, ok := r.Value.(Service); ok { - c.Service(name, svc) + svc, ok := r.Value.(Service) + if !ok { + return Result{E("core.NewWithFactories", Concat("factory \"", name, "\" returned non-Service type"), nil), false} + } + sr := c.Service(name, svc) + if !sr.OK { + return sr } } return Result{&Runtime{app: app, Core: c}, true}