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

Question for bajdev: 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.

To test the code anyway, I ran a throwaway 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: Pad, PadHasNotes, Sequences, PoolFiles, tune, mute group, copy, move, swap, clear, sequence edits, and Encode.
• Result: 12,726 cases, 0 panics. It takes about 10 minutes.

Options:

1. Keep FuzzLoad as it is (it meets the plan).
2. Also commit the sweep as a test, cut down to run in under a minute, e.g. the first pad and sequence only.
3. Replace FuzzLoad with a fuzz target that changes the decoded tree rather than the gzip bytes.

I'd suggest 2. It is a few lines in a follow-up PR or in this one if you prefer.

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