fix: Codex review round 3 — 5 remaining findings

- Command: allow overwriting auto-created parent placeholders
- NewWithFactories: wrap original factory error cause
- I18n.SetTranslator: reapply saved locale to new translator
- Options/Drive: copy slices on intake (prevent aliasing)
- Embed.path: returns Result, rejects traversal with error

Co-Authored-By: Virgil <virgil@lethean.io>
This commit is contained in:
Snider 2026-03-20 17:46:47 +00:00
parent bf1f8e51ad
commit ee9e715243
6 changed files with 37 additions and 13 deletions

View file

@ -144,7 +144,7 @@ func (c *Core) Command(path string, command ...Command) Result {
c.commands.mu.Lock()
defer c.commands.mu.Unlock()
if _, exists := c.commands.commands[path]; exists {
if existing, exists := c.commands.commands[path]; exists && existing.Action != nil {
return Result{E("core.Command", Concat("command \"", path, "\" already registered"), nil), false}
}

View file

@ -88,8 +88,10 @@ func New(opts ...Options) *Core {
}
if len(opts) > 0 {
c.options = &opts[0]
name := opts[0].String("name")
cp := make(Options, len(opts[0]))
copy(cp, opts[0])
c.options = &cp
name := cp.String("name")
if name != "" {
c.app.Name = name
}

View file

@ -62,10 +62,12 @@ func (d *Drive) New(opts Options) Result {
d.handles = make(map[string]*DriveHandle)
}
cp := make(Options, len(opts))
copy(cp, opts)
handle := &DriveHandle{
Name: name,
Transport: transport,
Options: opts,
Options: cp,
}
d.handles[name] = handle

View file

@ -366,13 +366,12 @@ func MountEmbed(efs embed.FS, basedir string) Result {
return Mount(efs, basedir)
}
func (s *Embed) path(name string) string {
func (s *Embed) path(name string) Result {
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 Result{E("embed.path", Concat("path traversal rejected: ", name), nil), false}
}
return joined
return Result{joined, true}
}
// Open opens the named file for reading.
@ -380,7 +379,11 @@ func (s *Embed) path(name string) string {
// r := emb.Open("test.txt")
// if r.OK { file := r.Value.(fs.File) }
func (s *Embed) Open(name string) Result {
f, err := s.fsys.Open(s.path(name))
r := s.path(name)
if !r.OK {
return r
}
f, err := s.fsys.Open(r.Value.(string))
if err != nil {
return Result{err, false}
}
@ -389,7 +392,11 @@ func (s *Embed) Open(name string) Result {
// ReadDir reads the named directory.
func (s *Embed) ReadDir(name string) Result {
return Result{}.Result(fs.ReadDir(s.fsys, s.path(name)))
r := s.path(name)
if !r.OK {
return r
}
return Result{}.Result(fs.ReadDir(s.fsys, r.Value.(string)))
}
// ReadFile reads the named file.
@ -397,7 +404,11 @@ func (s *Embed) ReadDir(name string) Result {
// r := emb.ReadFile("test.txt")
// if r.OK { data := r.Value.([]byte) }
func (s *Embed) ReadFile(name string) Result {
data, err := fs.ReadFile(s.fsys, s.path(name))
r := s.path(name)
if !r.OK {
return r
}
data, err := fs.ReadFile(s.fsys, r.Value.(string))
if err != nil {
return Result{err, false}
}
@ -421,7 +432,11 @@ func (s *Embed) ReadString(name string) Result {
// r := emb.Sub("testdata")
// if r.OK { sub := r.Value.(*Embed) }
func (s *Embed) Sub(subDir string) Result {
sub, err := fs.Sub(s.fsys, s.path(subDir))
r := s.path(subDir)
if !r.OK {
return r
}
sub, err := fs.Sub(s.fsys, r.Value.(string))
if err != nil {
return Result{err, false}
}

View file

@ -69,7 +69,11 @@ func (i *I18n) Locales() Result {
func (i *I18n) SetTranslator(t Translator) {
i.mu.Lock()
i.translator = t
locale := i.locale
i.mu.Unlock()
if t != nil && locale != "" {
_ = t.SetLanguage(locale)
}
}
// Translator returns the registered translation implementation, or nil.

View file

@ -112,7 +112,8 @@ func NewWithFactories(app any, factories map[string]ServiceFactory) Result {
}
r := factory()
if !r.OK {
return Result{E("core.NewWithFactories", Concat("factory \"", name, "\" failed"), nil), false}
cause, _ := r.Value.(error)
return Result{E("core.NewWithFactories", Concat("factory \"", name, "\" failed"), cause), false}
}
svc, ok := r.Value.(Service)
if !ok {