test: cover the cachemodimg protocol handler - #176
Conversation
Pixnop
left a comment
There was a problem hiding this comment.
The extraction is the right move and the harness is genuinely behavioral: DI ports with a real IconMemoryCache, real temp dirs and symlinks, only the network edge mocked, and the wiring in index.ts is unchanged in the ways that matter (scheme privileges untouched, handler in the same position, isSafeProtocolFile still shared with the icons handler). Most of the mutation battery is caught, including the subtle one that matters most: scoping the safety check to the cache-miss path only, so a cache hit would skip it, still fails the ordering test. That property, the one #169's review flagged, is properly pinned.
Two gaps before merge, both additive.
First, the png extension gate is not actually tested, shown by mutation and then by demonstration: with the suffix check deleted, all nine tests still pass, because the one negative case that looks like it covers it (/aa.jpg in the invalid-paths test) never writes a file to disk, so its 404 comes from the ENOENT catch rather than from the gate. Writing a real non-png file into the icon root and requesting it under the mutated handler returned 200 with the file served through fetchFile. One added case closes it: create a real .txt or .jpg on disk, assert 404 and that fetchFile was never called. As written, the suite would stay green if the extension check vanished, and serving arbitrary cache-directory files over the protocol is exactly what it exists to prevent.
Second, the new Uint8Array(bytes) copy guards the detach hazard (a Response can transfer the buffer and corrupt the shared cached Buffer) but carries neither a test nor a comment, so it reads as removable noise to the next simplifier. A one-line comment is the minimum; a test that mutates the served bytes and asserts a later cache hit is unaffected would pin it properly.
Everything else holds: gates green with the PR body's exact numbers, and protocolFiles.ts enters the coverage ratchet automatically through the existing src/main include.
Pixnop
left a comment
There was a problem hiding this comment.
Approving. Re-ran the two mutations from the last round against 6ef2aea: neutralizing the png extension gate now fails exactly the new rejects-a-real-non-PNG-file test (which writes the file to disk and asserts fetchFile was never called, the shape that was missing), and the Uint8Array copy carries the comment that stops it reading as removable noise. The copy itself remains untested, which was the stated minimum, so on the record rather than blocking. Everything from the first round still holds.
Summary
Extract the cachemodimg protocol handler from the Electron bootstrap into a testable main-process module. The handler keeps the existing path containment, symlink checks, memory cache, fetch behavior, and response headers.
Type
Checklist
Testing
The focused protocol suite and the existing icon cache suite pass with 17 tests. The full suite passes with 108 test files, 1,229 tests, and 2 skipped tests. Coverage is 92.54% lines, 90.83% statements, 89.04% functions, and 87.78% branches.
The new tests cover cache misses and hits, eviction, safety checks before cached responses, traversal and malformed paths, missing files, directories, symlinks, response headers, non-success fetches, and thrown fetch errors.
Related issues
Fixes #171