fix(config): check the PNG signature before copying a custom icon - #213
Conversation
COPY_TO_ICONS decided whether a picked file was a PNG by reading its extension, so a JPEG renamed to .png was copied into the Icons folder and served through the icons: protocol without anything ever looking at its bytes. The background flow stopped doing that in #207, where isJpegBytes confirms the SOI marker before a picture is cached or served. isPngBytes joins isJpegBytes in src/domain/backgrounds.ts and answers the same question about the same kind of file. The handler reads the first eight bytes through a file handle rather than pulling the whole picture into memory, since a custom icon has no size ceiling of its own, and a mismatch comes back as unsupported-format, the reason #206 already gave a file that is not a PNG.
Zaldaryon
left a comment
There was a problem hiding this comment.
Checked out cd6a2cc in an isolated worktree: typecheck, lint:ci, format:check clean, and the two targeted test files pass 78 of 78. Reproduced the mutation claim directly: deleting the signature-check block leaves exactly the two new rows red, the other 63 green.
The check-then-reopen shape between the header read and fse.copyFile is the same TOCTOU footprint pathPolicy.ts's assertManagedPath and #207's COPY_CUSTOM_BACKGROUND already have everywhere in this codebase, so it isn't a new gap. The unchecked bytesRead is safe by construction rather than luck: no byte of the PNG signature is 0x00, so a Buffer.alloc-zeroed truncated read can never complete a false match, confirmed empirically at every truncation length from 0 to 7. A genuinely 0-byte file returns unsupported-format correctly rather than misrouting through copy-failed, confirmed by running it through the real handler.
Two small things, neither blocking. The comment at pathsHandlers.ts's copy step says the copy below streams rather than buffers; fs.copyFile is a single OS-level copy_file_range/CopyFileEx call, not a stream, and the PR body's own phrasing (never pulls the picture into memory) is the accurate version, worth swapping in. Separately, and out of this PR's own scope: COPY_CUSTOM_BACKGROUND from #207 checks isFile() and a size ceiling before reading, and COPY_TO_ICONS still has neither, so a very large file with a valid PNG signature is still copied whole into userData with no cap. Filing as a follow-up issue rather than blocking here, since #211 was specifically about the missing byte check and this PR closes exactly that.
Approving.
Two flows in this launcher take a picture the player picked and put it somewhere the app will serve it back. Since #207 the background one confirms the JPEG start-of-image marker before anything is cached. The custom icon one was still deciding by extension alone, which is what Zaldaryon caught while reviewing #206 and wrote up as issue #211.
So a file called
holiday.pngthat is really a JPEG, or a zip, or a text file, went into the Icons folder and came back out through theicons:protocol with nobody having looked at a single byte of it. Chromium sniffs the real bytes when it renders, so this was never an exploit, just two neighbouring pieces of code that disagreed about how carefully they check. The disagreement is the problem.isPngBytesnow sits next toisJpegBytesinsrc/domain/backgrounds.ts, same shape, same eight-line answer, no image-type detector and no new dependency.COPY_TO_ICONSopens the picked file, reads the eight bytes of the PNG signature through the handle, and closes it. Reading only the header rather than the whole file matters here: an icon has no size cap the way a background does, and the copy that follows isfs.copyFile, which never pulls the picture into memory.A refusal reuses
unsupported-format, the reason #206 introduced for a file that is not a.png. The sentence behind it already reads "That file isn't a PNG! Custom icons have to be .png images, so convert it or pick another file", which is honest for a renamed JPEG as much as for a.txt, so no new string was added and the renderer keeps one refusal to handle instead of two. The cause behind the reason goes to the log at debug like every other refusal in that handler.On the tests: the existing icon rows wrote the literal string
fake-png-bytesas their source file, which the byte check would now reject, so they carry real PNG bytes instead. Two rows are new. One picks a file named.pngholding JPEG bytes and pins both halves, the named refusal and an Icons folder that stays empty. The other picks a file too short to hold the whole signature. Deleting the check in the handler fails both of them, which was checked by hand before pushing.Gates all green locally: typecheck,
lint:ci(15 pre-existing React hook warnings, no errors),format:check, and the full suite under coverage at 1402 passing with the floors holding.