Merge branch 'harden-load' of bajdev/mpckit into main

internal/app/audio.go

@@ -37,10 +37,13 @@ func (s *Service) serveAudio(w http.ResponseWriter, r *http.Request) {
 		http.NotFound(w, r)
 		return
 	}
-	s.mu.Lock()
-	v, err := s.p.Pad(slot)
-	src := s.p.Sources[v.File]
-	s.mu.Unlock()
+	// Unlock by defer, so a panic cannot leave the lock held.
+	v, src, err := func() (project.Pad, string, error) {
+		s.mu.Lock()
+		defer s.mu.Unlock()
+		v, err := s.p.Pad(slot)
+		return v, s.p.Sources[v.File], err
+	}()
 	if err != nil || v.Empty() || src == "" {
 		http.NotFound(w, r)
 		return

internal/project/load_test.go

@@ -0,0 +1,326 @@
+package project
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+
+	"git.monoexp.com/bajdev/mpckit/internal/xpj"
+)
+
+// loadChanged reads the device's DemoFile, applies change to its data, writes
+// it to a temporary folder and loads it.
+func loadChanged(t *testing.T, change func(d *xpj.Node)) error {
+	t.Helper()
+	f, err := xpj.ReadFile(filepath.Join(testdata, "device", "DemoFile.xpj"))
+	must(t, err)
+	d, err := f.Data()
+	must(t, err)
+	change(d)
+	path := filepath.Join(t.TempDir(), "Changed.xpj")
+	must(t, f.WriteFile(path))
+	_, _, err = Load(path)
+	return err
+}
+
+func set(t *testing.T, d *xpj.Node, path string, v *xpj.Node) {
+	t.Helper()
+	must(t, d.Set(path, v))
+}
+
+func remove(t *testing.T, d *xpj.Node, path, key string) {
+	t.Helper()
+	n, err := d.Get(path)
+	must(t, err)
+	if !n.DeleteKey(key) {
+		t.Fatalf("%s has no key %q", path, key)
+	}
+}
+
+func TestLoadRejectsMalformed(t *testing.T) {
+	for _, tc := range []struct {
+		name   string
+		change func(t *testing.T, d *xpj.Node)
+		want   string // in the error
+	}{
+		{"parent folder", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[0].path", xpj.NewString("../x.wav"))
+		}, `samples[0].path: file name "../x.wav" contains a folder separator`},
+		{"subfolder", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[1].path", xpj.NewString("a/b.wav"))
+		}, `samples[1].path: file name "a/b.wav" contains a folder separator`},
+		{"backslash", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[1].path", xpj.NewString(`..\b.wav`))
+		}, `samples[1].path: file name "..\\b.wav" contains a folder separator`},
+		{"dot dot", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[2].path", xpj.NewString(".."))
+		}, `samples[2].path: ".." is not a file name`},
+		{"empty", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[0].path", xpj.NewString(""))
+		}, `samples[0].path: file name is empty`},
+		{"control character", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[0].path", xpj.NewString("a\x00.wav"))
+		}, `samples[0].path: file name "a\x00.wav" contains a control character`},
+		{"track pool", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "tracks[0].samples[0].path", xpj.NewString("../x.wav"))
+		}, `tracks[0].samples[0].path: file name "../x.wav"`},
+		{"missing path", func(t *testing.T, d *xpj.Node) {
+			remove(t, d, "samples[3]", "path")
+		}, "samples[3].path is missing or not text"},
+		{"path not text", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "samples[3].path", xpj.NewInt(3))
+		}, "samples[3].path is missing or not text"},
+		{"layer outside", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "tracks[0].program.drum.instruments[17].layersv[2].sampleFile", xpj.NewString("../x.wav"))
+		}, `pad B2 layer 3 sampleFile: file name "../x.wav" contains a folder separator`},
+		{"layer not text", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "tracks[0].program.drum.instruments[0].layersv[0].sampleFile", xpj.NewNull())
+		}, "pad A1 layer 1 sampleFile is not text"},
+		{"missing bpm", func(t *testing.T, d *xpj.Node) {
+			remove(t, d, "sequences[0].value", "bpm")
+		}, "sequences[0]: bpm is missing or not a number"},
+		{"missing key", func(t *testing.T, d *xpj.Node) {
+			remove(t, d, "sequences[1]", "key")
+		}, "sequences[1]: key is missing or not a whole number"},
+		{"missing value", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "sequences[1].value", xpj.NewArray())
+		}, "sequences[1]: value is missing or not an object"},
+		{"missing length", func(t *testing.T, d *xpj.Node) {
+			remove(t, d, "sequences[0].value", "lengthBars")
+		}, "sequences[0]: lengthBars is missing or not a whole number"},
+		{"name not text", func(t *testing.T, d *xpj.Node) {
+			set(t, d, "sequences[0].value.name", xpj.NewInt(1))
+		}, "sequences[0]: name is missing or not text"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := loadChanged(t, func(d *xpj.Node) { tc.change(t, d) })
+			if err == nil || !strings.Contains(err.Error(), tc.want) {
+				t.Fatalf("got error %v, want one containing %q", err, tc.want)
+			}
+		})
+	}
+}
+
+func TestLoadAcceptsMissingLayerFile(t *testing.T) {
+	err := loadChanged(t, func(d *xpj.Node) {
+		remove(t, d, "tracks[0].program.drum.instruments[0].layersv[0]", "sampleFile")
+	})
+	must(t, err)
+}
+
+func TestLoadMissingVersion(t *testing.T) {
+	f, err := xpj.ReadFile(filepath.Join(testdata, "device", "DemoFile.xpj"))
+	must(t, err)
+	d, err := f.Data()
+	must(t, err)
+	d.DeleteKey("version")
+	path := filepath.Join(t.TempDir(), "NoVersion.xpj")
+	must(t, f.WriteFile(path))
+	_, warnings, err := Load(path)
+	must(t, err)
+	if len(warnings) != 1 || !strings.Contains(warnings[0], "version is missing") {
+		t.Fatalf("warnings = %q", warnings)
+	}
+}
+
+func TestPlainFileName(t *testing.T) {
+	for _, ok := range []string{"kick.wav", "80s-Kik-Dreamer2 SA.wav", "a..b.wav", ".hidden.wav", "Ünïcode.wav"} {
+		if err := PlainFileName(ok); err != nil {
+			t.Errorf("PlainFileName(%q) = %v", ok, err)
+		}
+	}
+	for _, bad := range []string{"", ".", "..", "../x.wav", "a/b.wav", `a\b.wav`, "/x.wav", "x\n.wav", "x\x7f.wav"} {
+		if err := PlainFileName(bad); err == nil {
+			t.Errorf("PlainFileName(%q) = nil", bad)
+		}
+	}
+}
+
+// FuzzLoad loads arbitrary files; none may make the read-only calls panic.
+func FuzzLoad(f *testing.F) {
+	seeds, err := filepath.Glob(filepath.Join(testdata, "device", "*.xpj"))
+	if err != nil || len(seeds) == 0 {
+		f.Fatalf("no seed files: %v", err)
+	}
+	for _, s := range seeds {
+		b, err := os.ReadFile(s)
+		if err != nil {
+			f.Fatal(err)
+		}
+		f.Add(b)
+	}
+	f.Fuzz(func(t *testing.T, b []byte) {
+		path := filepath.Join(t.TempDir(), "Fuzz.xpj")
+		if err := os.WriteFile(path, b, 0o644); err != nil {
+			t.Fatal(err)
+		}
+		p, _, err := Load(path)
+		if err != nil {
+			return
+		}
+		for slot := range Pads {
+			_, _ = p.Pad(slot)
+		}
+		p.Sequences()
+		p.PoolFiles()
+	})
+}
+
+// TestBrokenFieldsDoNotPanic changes, one at a time, each field that project
+// reads in DemoFile: replaced by null, text, a negative or large number, an
+// empty list or object, or deleted. Whatever wrap makes of the result, reads
+// and edits must not panic. Only the first item of each list is changed, so
+// the test stays fast; a full sweep (all fields to depth 5, 12,726 cases)
+// found nothing either and takes about 10 minutes.
+func TestBrokenFieldsDoNotPanic(t *testing.T) {
+	if testing.Short() {
+		t.Skip("slow")
+	}
+	base, err := xpj.ReadFile(filepath.Join(testdata, "device", "DemoFile.xpj"))
+	must(t, err)
+	d, err := base.Data()
+	must(t, err)
+
+	events := "sequences[0].value.trackClipMaps[0][0].value.eventList.events"
+	roots := []struct {
+		path  string
+		depth int
+	}{
+		{"version", 0}, {"currentSequence", 0}, {"tracks[0].name", 0},
+		{"samples", 3}, {"tracks[0].samples", 2},
+		{"tracks[0].program.drum.instruments", 1},
+		{"tracks[0].program.drum.instruments[0]", 1},
+		{"tracks[0].program.drum.instruments[0].layersv", 2},
+		{"tracks[0].program.drum.instruments[0].layersv[0].sliceInfo", 1},
+		{"sequences", 3},
+		{"sequences[0].value.trackClipMaps", 4},
+		{events, 3},
+	}
+	// The first event is automation; add the first note event too.
+	evs, err := d.Get(events)
+	must(t, err)
+	for i, ev := range evs.Items {
+		if num(ev.Lookup("type")) == eventNote {
+			roots = append(roots, struct {
+				path  string
+				depth int
+			}{fmt.Sprintf("%s[%d]", events, i), 1}, struct {
+				path  string
+				depth int
+			}{fmt.Sprintf("%s[%d].note.note", events, i), 0})
+			break
+		}
+	}
+	var paths []string
+	for _, r := range roots {
+		n, err := d.Get(r.path)
+		must(t, err)
+		fieldPaths(n, r.path, r.depth, &paths)
+	}
+
+	changes := []func(parent *xpj.Node, key string, i int){
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewNull()) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewString("../x.wav")) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewInt(-1)) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewInt(1000)) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewArray()) },
+		func(p *xpj.Node, k string, i int) { replace(p, k, i, xpj.NewObject()) },
+		func(p *xpj.Node, k string, i int) {
+			if k != "" {
+				p.DeleteKey(k)
+			} else {
+				_ = p.Remove(i)
+			}
+		},
+	}
+	names := []string{"null", "text", "-1", "1000", "list", "object", "deleted"}
+	for _, path := range paths {
+		parentPath, key, i := splitPath(path)
+		for c, change := range changes {
+			f := base.Clone()
+			fd, _ := f.Data()
+			parent, err := fd.Get(parentPath)
+			if err != nil {
+				continue // a key holding '.' or '[' cannot be addressed by path
+			}
+			change(parent, key, i)
+			func() {
+				defer func() {
+					if r := recover(); r != nil {
+						t.Errorf("%s %s: panic: %v", path, names[c], r)
+					}
+				}()
+				p, err := wrap(f, "Broken", "")
+				if err != nil {
+					return
+				}
+				exercise(p)
+			}()
+		}
+	}
+	t.Logf("%d fields, %d cases", len(paths), len(paths)*len(changes))
+}
+
+// fieldPaths lists path and the paths below n to depth levels, taking only
+// the first item of each list.
+func fieldPaths(n *xpj.Node, path string, depth int, out *[]string) {
+	*out = append(*out, path)
+	if depth == 0 {
+		return
+	}
+	if len(n.Items) > 0 {
+		fieldPaths(n.Items[0], path+"[0]", depth-1, out)
+	}
+	for _, m := range n.Members {
+		fieldPaths(m.Value, path+"."+m.Key, depth-1, out)
+	}
+}
+
+// splitPath splits a path into its parent and last step: a key, or an index
+// when key is "".
+func splitPath(path string) (parent, key string, i int) {
+	if strings.HasSuffix(path, "]") {
+		open := strings.LastIndexByte(path, '[')
+		i, _ = strconv.Atoi(path[open+1 : len(path)-1])
+		return path[:open], "", i
+	}
+	dot := strings.LastIndexByte(path, '.')
+	if dot < 0 {
+		return "", path, 0
+	}
+	return path[:dot], path[dot+1:], 0
+}
+
+func replace(parent *xpj.Node, key string, i int, v *xpj.Node) {
+	if key != "" {
+		_ = parent.SetKey(key, v)
+	} else if i < len(parent.Items) {
+		parent.Items[i] = v
+	}
+}
+
+// exercise runs every read and most edits on p.
+func exercise(p *Project) {
+	for slot := range Pads {
+		_, _ = p.Pad(slot)
+		p.PadHasNotes(slot)
+	}
+	p.PoolFiles()
+	for slot := range 2 {
+		_ = p.SetTune(slot, 1, 1)
+		_ = p.SetMuteGroup(slot, 1)
+		_ = p.CopyPad(slot, slot+20)
+		_ = p.MovePad(slot+20, slot+40)
+		_ = p.SwapPads(slot, slot+1)
+		_ = p.ClearPad(slot + 1)
+	}
+	for _, s := range p.Sequences() {
+		_ = p.SetSequence(s.Slot, "Changed", 100, 4)
+	}
+	_ = p.AddSequence(50)
+	_ = p.RemoveSequence(50)
+	_ = p.RemoveSequence(0)
+}

internal/project/pads.go

@@ -63,14 +63,18 @@ func text(n *xpj.Node) string {
 	return n.Text
 }
 
+// num and float return a number field's value, or 0 if it is missing or
+// not a number of that kind.
 func num(n *xpj.Node) int64 {
-	if n == nil {
-		return 0
-	}
 	i, _ := n.Int()
 	return i
 }
 
+func float(n *xpj.Node) float64 {
+	f, _ := n.Float()
+	return f
+}
+
 // padFiles returns the pool files used by any layer of pad.
 func padFiles(pad *xpj.Node) []string {
 	var files []string

internal/project/project.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"strings"
+	"unicode"
 
 	"git.monoexp.com/bajdev/mpckit/internal/xpj"
 )
@@ -62,6 +63,8 @@ type Project struct {
 
 	file *xpj.File
 	data *xpj.Node
+	// The lists every operation works on, found once by wrap.
+	nodes struct{ pool, trackPool, pads, sequences *xpj.Node }
 
 	lastBPM float64 // BPM of the most recently edited sequence; 0 if none
 }
@@ -126,11 +129,12 @@ func Load(path string) (*Project, []string, error) {
 	if f.Header[1] != xpj.TestedHeader[1] {
 		warnings = append(warnings, fmt.Sprintf("saved by firmware %s; MPCkit is tested with %s", f.Header[1], xpj.TestedHeader[1]))
 	}
-	if v, err := p.data.Lookup("version").Int(); err != nil || v != TestedVersion {
+	if v, err := p.data.Lookup("version").Int(); err != nil {
+		warnings = append(warnings, fmt.Sprintf("project data version is missing; MPCkit is tested with %d", TestedVersion))
+	} else if v != TestedVersion {
 		warnings = append(warnings, fmt.Sprintf("project data version %d; MPCkit is tested with %d", v, TestedVersion))
 	}
-	for _, e := range p.pool().Items {
-		file := e.Lookup("path").Text
+	for _, file := range p.PoolFiles() {
 		p.Sources[file] = filepath.Join(dataDir, file)
 	}
 	return p, warnings, nil
@@ -142,21 +146,124 @@ func wrap(f *xpj.File, name, sourceDir string) (*Project, error) {
 		return nil, err
 	}
 	p := &Project{Name: name, SourceDir: sourceDir, Sources: map[string]string{}, file: f, data: d}
-	for _, path := range []string{"samples", "tracks[0].samples", "tracks[0].program.drum.instruments", "sequences"} {
-		n, err := d.Get(path)
+	for _, l := range []struct {
+		path string
+		node **xpj.Node
+	}{
+		{"samples", &p.nodes.pool},
+		{"tracks[0].samples", &p.nodes.trackPool},
+		{"tracks[0].program.drum.instruments", &p.nodes.pads},
+		{"sequences", &p.nodes.sequences},
+	} {
+		n, err := d.Get(l.path)
 		if err != nil {
 			return nil, fmt.Errorf("project: %w", err)
 		}
 		if n.Kind != xpj.Array {
-			return nil, fmt.Errorf("project: %s is not a list", path)
+			return nil, fmt.Errorf("project: %s is not a list", l.path)
 		}
+		*l.node = n
 	}
 	if n := len(p.pads().Items); n != Pads {
 		return nil, fmt.Errorf("project: %d pads, want %d", n, Pads)
 	}
+	if err := p.check(); err != nil {
+		return nil, fmt.Errorf("project: %w", err)
+	}
 	return p, nil
 }
 
+// check refuses a file whose sample names or sequences the operations
+// cannot use safely. Pool file names are joined to folders at load and
+// export, so a name such as "../x.wav" would reach outside the project.
+func (p *Project) check() error {
+	pools := []struct {
+		path string
+		node *xpj.Node
+	}{{"samples", p.pool()}, {"tracks[0].samples", p.trackPool()}}
+	for _, pool := range pools {
+		for i, e := range pool.node.Items {
+			at := fmt.Sprintf("%s[%d].path", pool.path, i)
+			n := e.Lookup("path")
+			if n == nil || n.Kind != xpj.String {
+				return fmt.Errorf("%s is missing or not text", at)
+			}
+			if err := PlainFileName(n.Text); err != nil {
+				return fmt.Errorf("%s: %w", at, err)
+			}
+		}
+	}
+	for slot, pad := range p.pads().Items {
+		layers := pad.Lookup("layersv")
+		if layers == nil {
+			continue
+		}
+		for i, l := range layers.Items {
+			// An empty name is an empty layer.
+			n := l.Lookup("sampleFile")
+			if n == nil {
+				continue
+			}
+			at := fmt.Sprintf("pad %s layer %d sampleFile", SlotName(slot), i+1)
+			if n.Kind != xpj.String {
+				return fmt.Errorf("%s is not text", at)
+			}
+			if n.Text == "" {
+				continue
+			}
+			if err := PlainFileName(n.Text); err != nil {
+				return fmt.Errorf("%s: %w", at, err)
+			}
+		}
+	}
+	for i, s := range p.sequences().Items {
+		if err := checkSequence(s); err != nil {
+			return fmt.Errorf("sequences[%d]: %w", i, err)
+		}
+	}
+	return nil
+}
+
+// checkSequence checks the fields Sequences and the sequence edits read.
+func checkSequence(s *xpj.Node) error {
+	if _, err := s.Lookup("key").Int(); err != nil {
+		return errors.New("key is missing or not a whole number")
+	}
+	v := s.Lookup("value")
+	if v == nil || v.Kind != xpj.Object {
+		return errors.New("value is missing or not an object")
+	}
+	if _, err := v.Lookup("bpm").Float(); err != nil {
+		return errors.New("bpm is missing or not a number")
+	}
+	if _, err := v.Lookup("lengthBars").Int(); err != nil {
+		return errors.New("lengthBars is missing or not a whole number")
+	}
+	if n := v.Lookup("name"); n == nil || n.Kind != xpj.String {
+		return errors.New("name is missing or not text")
+	}
+	return nil
+}
+
+// PlainFileName checks that name is a single file name with no folder part,
+// safe to join to a folder on any system: not empty, not "." or "..", and
+// free of slashes, backslashes and control characters.
+func PlainFileName(name string) error {
+	switch {
+	case name == "":
+		return errors.New("file name is empty")
+	case name == "." || name == "..":
+		return fmt.Errorf("%q is not a file name", name)
+	case strings.ContainsAny(name, `/\`):
+		return fmt.Errorf("file name %q contains a folder separator", name)
+	case strings.IndexFunc(name, unicode.IsControl) >= 0:
+		return fmt.Errorf("file name %q contains a control character", name)
+	case filepath.Base(name) != name:
+		return fmt.Errorf("%q is not a plain file name", name)
+	}
+	return nil
+}
+
 // File returns the project file for writing. Callers must not edit it.
 func (p *Project) File() *xpj.File { return p.file }
 
@@ -216,10 +323,7 @@ func checkSlot(slot int) error {
 	return nil
 }
 
-func (p *Project) pads() *xpj.Node {
-	n, _ := p.data.Get("tracks[0].program.drum.instruments")
-	return n
-}
+func (p *Project) pads() *xpj.Node { return p.nodes.pads }
 
 func (p *Project) pad(slot int) *xpj.Node { return p.pads().Items[slot] }
 
@@ -230,20 +334,11 @@ func layer(pad *xpj.Node) (*xpj.Node, error) {
 // pool is samples[]; trackPool is tracks[0].samples[]. The device keeps
 // the same entries in both, appending to the first and prepending to the
 // second.
-func (p *Project) pool() *xpj.Node {
-	n, _ := p.data.Get("samples")
-	return n
-}
+func (p *Project) pool() *xpj.Node { return p.nodes.pool }
 
-func (p *Project) trackPool() *xpj.Node {
-	n, _ := p.data.Get("tracks[0].samples")
-	return n
-}
+func (p *Project) trackPool() *xpj.Node { return p.nodes.trackPool }
 
-func (p *Project) sequences() *xpj.Node {
-	n, _ := p.data.Get("sequences")
-	return n
-}
+func (p *Project) sequences() *xpj.Node { return p.nodes.sequences }
 
 // edit applies several Sets and keeps the first error, so an operation can
 // list its writes plainly. Paths come from the template; a failure means the

internal/project/sequences.go

@@ -31,11 +31,10 @@ func (p *Project) Sequences() []Sequence {
 	var out []Sequence
 	for _, s := range p.sequences().Items {
 		v := s.Lookup("value")
-		bpm, _ := v.Lookup("bpm").Float()
 		out = append(out, Sequence{
 			Slot:     int(num(s.Lookup("key"))),
 			Name:     text(v.Lookup("name")),
-			BPM:      bpm,
+			BPM:      float(v.Lookup("bpm")),
 			Bars:     int(num(v.Lookup("lengthBars"))),
 			HasNotes: hasNotes(v),
 		})

internal/xpj/node.go

@@ -8,6 +8,7 @@
 package xpj
 
 import (
+	"errors"
 	"fmt"
 	"math"
 	"strconv"
@@ -126,8 +127,15 @@ func FormatFloat(f float64) string {
 	return out
 }
 
-// Int returns the value of a Number node as an integer.
+// errMissing is returned by the value getters for a nil node.
+var errMissing = errors.New("xpj: value is missing")
+
+// Int returns the value of a Number node as an integer. A nil node, as
+// Lookup returns for a missing key, is an error.
 func (n *Node) Int() (int64, error) {
+	if n == nil {
+		return 0, errMissing
+	}
 	if n.Kind != Number {
 		return 0, fmt.Errorf("xpj: %s is not a number", n.Kind)
 	}
@@ -141,8 +149,11 @@ func (n *Node) Int() (int64, error) {
 	return int64(f), nil
 }
 
-// Float returns the value of a Number node.
+// Float returns the value of a Number node. A nil node is an error.
 func (n *Node) Float() (float64, error) {
+	if n == nil {
+		return 0, errMissing
+	}
 	if n.Kind != Number {
 		return 0, fmt.Errorf("xpj: %s is not a number", n.Kind)
 	}

internal/xpj/path.go

@@ -63,6 +63,9 @@ func parsePath(path string) ([]step, error) {
 
 func (n *Node) walk(steps []step, path string) (*Node, error) {
 	cur := n
+	if cur == nil {
+		return nil, fmt.Errorf("xpj: path %q: node is missing", path)
+	}
 	for i, s := range steps {
 		if s.index < 0 {
 			next := cur.Lookup(s.key)

internal/xpj/xpj_test.go

@@ -365,3 +365,19 @@ func TestNumberAccessors(t *testing.T) {
 		t.Errorf("Float = %v, %v", f, err)
 	}
 }
+
+func TestNilNode(t *testing.T) {
+	var n *Node
+	if _, err := n.Int(); err == nil {
+		t.Error("Int on nil: no error")
+	}
+	if _, err := n.Float(); err == nil {
+		t.Error("Float on nil: no error")
+	}
+	if n.Lookup("a") != nil {
+		t.Error("Lookup on nil: not nil")
+	}
+	if _, err := n.Get("a[0]"); err == nil {
+		t.Error("Get on nil: no error")
+	}
+}

main.go

@@ -43,9 +43,17 @@ func init() {
 func main() {
 	d := &dialogs{}
 	svc := app.NewService(d)
-	a := application.New(application.Options{
+	var a *application.App
+	a = application.New(application.Options{
 		Name:        "MPCkit",
 		Description: "Desktop kit editor for the MPC Sample",
+		// Wails exits on a panic in an event handler unless this is set.
+		// Log it and tell the user instead, so a case the checks missed
+		// cannot close the app and lose their changes.
+		PanicHandler: func(p *application.PanicDetails) {
+			log.Printf("panic: %v\n%s", p.Error, p.StackTrace)
+			a.Event.Emit(eventError, "Something went wrong inside MPCkit, and the last action may not have finished.")
+		},
 		Services: []application.Service{
 			application.NewService(svc),
 		},

pull-requests/harden-load/reject-malformed-projects-at-load.md

@@ -0,0 +1,71 @@
+# Reject malformed projects at load
+
+Branch: `harden-load` → `main`
+
+PR 1 of the review fixes plan (`docs/review-fixes-plan.md`). Part of 0.5.2; the version does not change in this PR.
+
+## What changed
+
+**Sample names stay inside the project (S1, load side).** Loading a project now refuses it if any sample file name in the pool is not a plain file name. A plain name is not empty, not `.` or `..`, and has no `/`, `\` or control characters. The same rule applies to each pad layer's `sampleFile`, which must be text or missing. Before, a pool `path` of `../../../secret.txt` was joined to the data folder as it was. The error names the bad entry, e.g. `samples[0].path: file name "../x.wav" contains a folder separator`. The rule is `project.PlainFileName`, which PR 3 reuses at export.
+
+**Missing fields no longer crash (S3).**
+- Load refuses a pool entry without a text `path`. It also refuses a sequence without a whole-number `key`, an object `value`, a number `bpm`, a whole-number `lengthBars` or a text `name`. Each error names the entry, e.g. `sequences[0]: bpm is missing or not a number`. Before, the first two cases were nil-pointer panics.
+- `xpj.Node.Int` and `Float` return an error on a nil node, and `Get` on a nil node returns an error. `Lookup` was already nil-safe. In `project`, the remaining direct `.Text` and `.Float()` uses on lookups now go through `text()`, `num()` and a new `float()`.
+- A missing `data.version` now gives the warning "project data version is missing" instead of "version 0".
+- `main.go` sets Wails's `PanicHandler`. A panic is logged with its stack and the window shows "Something went wrong inside MPCkit, and the last action may not have finished." Without the handler, Wails exits the app on a panic in an event handler such as file drop.
+- The preview audio handler now releases the project lock with `defer`, so a panic there cannot leave the lock held and freeze every later call. This was not in the plan, but it follows from the same finding.
+
+**Main nodes found once (M2).** `wrap` finds `samples`, `tracks[0].samples`, `tracks[0].program.drum.instruments` and `sequences` once and stores them on `Project`. `pool()`, `trackPool()`, `pads()` and `sequences()` return the stored nodes instead of looking them up again with the error dropped.
+
+**Tests.**
+- `TestLoadRejectsMalformed` builds 16 broken copies of DemoFile in `t.TempDir()` and checks each error. The cases cover pool paths `../x.wav`, `a/b.wav`, `..\b.wav`, `..`, an empty name, a control character, a bad track pool entry, and a missing or non-text `path`. They also cover a bad layer `sampleFile`, and a sequence missing `bpm`, `key`, `value` or `lengthBars`, or with a non-text name.
+- Other new tests cover a layer with no `sampleFile` (still loads), a missing `data.version` (warning), `PlainFileName`, and `xpj` getters on a nil node.
+- `FuzzLoad` is the fuzz target the plan describes.
+- `TestBrokenFieldsDoNotPanic` breaks DemoFile's fields one at a time, then runs every read and most edits on the result. None may panic. It covers 162 fields and 1,134 cases, and takes about 30 seconds on the server; `go test -short` skips it. See "Fuzzing" below for why it was added.
+- Against the old code, the path cases load without error and the missing-`path` case panics.
+
+## Notes
+
+Choices the plan left open. Say if any should change:
+- **The track pool is checked too.** `tracks[0].samples` holds the same entries as `samples`, so its `path`s get the same check.
+- **Empty `sampleFile` is allowed.** The device writes `""` on every empty pad layer (1,018 of them in DemoFile), so an empty name means "no sample" and only non-empty names must be plain.
+- **Whole numbers only.** `key` and `lengthBars` must be whole numbers, since both are used as integers. `bpm` may be any number.
+- **Not checked, because the plan doesn't ask and neither can panic:** a sequence `key` outside 0–127, duplicate keys, and duplicate pool names.
+- **Two messages after a panic in a bound method.** Wails already turns such a panic into a failed call, and the frontend shows that error. With the handler set, the handler's message is also sent, and both go to the same banner, so the last one shows.
+- **The fuzzer's progress line is wrong for this target.** It shows `execs: 9 (0/sec)` for most of the run. The fuzz function itself counted 42,097 calls in 15 seconds, so the 2-minute run made about 300,000.
+
+**Fuzzing.** `FuzzLoad` barely reaches the project code. The seeds are gzip files, and gzip checks a CRC at the end of the stream, so almost every mutated input fails at decompression. In a 15-second sample:
+- 5,885 inputs ran.
+- 5,882 failed in gzip or flate.
+- Only the 3 unchanged seeds loaded.
+
+Fuzzing the decompressed JSON does not help either: each project is about 3 MB uncompressed, too big for Go's fuzzer to mutate usefully.
+
+So I first ran a full sweep, not committed:
+- It changed each of 1,818 fields that `project` reads in DemoFile in 7 ways: null, text, -1, 1000, an empty list, an empty object, or deleted.
+- After each change, it ran every read and edit.
+- Result: 12,726 cases, 0 panics, in about 10 minutes.
+
+bajdev chose to keep `FuzzLoad` and also commit a cut-down sweep that runs in under a minute: `TestBrokenFieldsDoNotPanic`.
+- It changes only the first item of each list. For pads it covers the first pad's own fields, its layers and `sliceInfo`, rather than every field below the pad.
+- It checks that nothing panics, not that the file is refused. Some changes load and are handled as an empty value.
+- With the load checks and nil guards turned off, it reports 67 panics; with them, none.
+
+## Testing
+
+**On the server:**
+- `go test ./...` passes, including the round-trip and golden tests, unchanged.
+- `go vet ./...` and `staticcheck ./...` are clean.
+- `govulncheck ./...` reports only GO-2026-6859 in the standard library's `net/http`. That needs Go 1.27.4 (PR 4).
+- `go test -fuzz=FuzzLoad -fuzztime=2m ./internal/project` found nothing.
+- `TestBrokenFieldsDoNotPanic` passes in about 30 seconds.
+- `npm install`, `npm run check` (0 errors, 0 warnings) and `npm run build` pass. The lockfile `npm install` wrote was not committed.
+- `wails3 build` builds.
+- `timeout 15 xvfb-run -a bin/mpckit` starts and runs. The log holds only the usual headless GTK warnings.
+
+**On the laptop (PowerShell, in the repo):**
+1. `git fetch; git checkout harden-load`
+2. `go test ./internal/project ./internal/xpj`: both `ok` (the project tests take about half a minute). On Windows, `filepath` also treats `\` and drive letters as path parts, so this checks the name rule there.
+3. `wails3 dev`: load `testdata\device\DemoFile.xpj`. It opens as before: pads A1–A4, B1 and B2 filled; sequences Intro and Sequence 02.
+4. Load `testdata\accepted\MpckitT1.xpj`, then `MpckitT3.xpj`. Both open with no error or warning.
+5. Drop a WAV on a pad, change its tune and mute group, then add and remove a sequence. All work as before.