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

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 paths 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.