One bounded read, one symlink walk, one file name rule in the host

[Pixnop]

Summary

Three guards in the host layer exist in two copies each: the bounded-read loop, the symlink ancestor walk and the safe-file-name rule. These are security-relevant checks, so the point is not the line count, it is that each one can be tightened on one side and left stale on the other. One of them has already drifted in a way that costs another file twelve lines of workaround.

Nothing here removes a check. Every guard stays exactly where it is, including the defence in depth inside the workers; only the second copy goes.

Total estimated saving: about 65 lines, and three guards that can no longer be fixed on one side only.

1. network.ts writes the bounded-read loop twice, and the Node copy drops the status code

src/ipc/network.ts:51 (requestBoundedBuffer) and :169 (requestBoundedTextViaNode)

Each carries its own settle-once guard, wall-clock timeout, Content-Length pre-check, status check and streamed byte-cap loop. Two transports is deliberate and documented (#76: the auth service refuses net.request's sec-fetch-* metadata). Two copies of the reading loop is not, and they have already drifted: only the Electron copy throws BoundedResponseError carrying statusCode and headers, added later in PR #442 so netHandlers.releaseNotesFailureReason could read error.statusCode. The Node twin never got it.

Replace with: one collectBounded(response, maxBytes, cancel, finish) taking the response stream plus a cancel callback (request.abort for Electron, request.destroy for Node), called by both, and both throwing BoundedResponseError on a non-2xx. Electron's and Node's IncomingMessage are structurally compatible enough for one helper; the Array.isArray fold on content-length is a harmless no-op on the Node side.

Savings: about 35 lines in network.ts, plus about 12 in src/ipc/handlers/loginFailureReason.ts, which today carries STATUS_MESSAGE:79 and parses the status digits back out of the message text in httpReason:190-198 behind a nine-line comment explaining why that is safe. A typed error turns that into a plain number lookup, the way releaseNotesFailureReason already reads it.

Risk and test: the refusal message text is load-bearing in two places, netHandlers.fetchModDbListingArchive matches /redirect/ on the message to recognise ModDB's counted 302, and loginFailureReason maps three literal network messages by exact string. Keep the text byte-identical. Pinned by tests/ipc/network.test.ts (six cases against a real 127.0.0.1 server: Content-Length cap, streamed cap, timeout, 404, no browser metadata, header shape), tests/ipc/loginFailureReason.test.ts and tests/ipc/moddbListingArchive.test.ts. The network tests match the Node path by message substring only, so the typed error is compatible; loginFailureReason.test.ts builds plain Errors by hand and updates alongside the change.

Worth: high.

2. assertNoSymlinkComponents is implemented twice, once for the host and once for the workers

src/ipc/workers/extraction.ts:40 (18 lines) and src/ipc/pathPolicy.ts:156 (20 lines)

The same ancestor walk for symlinks. The worker copy has eight call sites across extraction.ts (88, 105, 172, 185, 492, 527) and innoExtraction.ts (115, 165); the host copy has one. Algorithmically identical (dirname versus resolve(current, "..") are equivalent for absolute paths); the return versus break at the filesystem root is unreachable because the root always exists. Both trace back to the fork seed commit, so this is inherited, not a chosen two-copy design.

Replace with: one implementation in src/ipc/validation.ts, which is already Electron-free and already reached by both sides (pathPolicy.ts:8 imports it directly, extraction.ts reaches it through archiveValidation.ts). Give it an optional message so pathPolicy keeps its "for managed paths" wording. extraction.ts already carries a comment saying the archive size ceilings were moved into validation.ts for exactly this reason (#362): two places to change one number is how the pair drifts.

Savings: about 18 lines net, and one copy of a symlink-traversal guard instead of two.

Risk and test: this is a security walk, so the refusal has to stay a refusal on both sides and the error classes differ (TypeError versus Error). No caller anywhere does instanceof TypeError on it, and all four pinned tests match by substring: tests/ipc/pathPolicy.test.ts:270 and :274, tests/ipc/pathsHandlers.test.ts:583, tests/ipc/innoExtraction.test.ts:117, all /Symbolic links are not allowed/. No renderer file imports validation.ts, so moving an fs read into it crosses no boundary.

Worth: high.

3. download.ts ships its own assertSafeFileName next to the one it already imports from

src/ipc/workers/download.ts:37 and src/ipc/validation.ts:208

The local copy rejects non-strings, empty, over 255, ., .. and /[\\/\0]/. The shared one rejects exactly the same set (assertString already covers empty, length and NUL). download.ts:22 already imports assertAllowedDownloadUrl from ../validation, so the shared one is one word away on an import line that exists. The local copy was added in PR #28 with no stated reason to duplicate.

Replace with: import assertSafeFileName from ../validation on the existing import line and delete the local copy. The guard stays inside the worker as defence in depth behind the handler's own check in pathsHandlers.ts:383.

Savings: verified by applying it: minus 39 plus 3 lines across the two files, once the now-redundant describe("assertSafeFileName") block in tests/ipc/download.test.ts goes (it re-tests rules tests/ipc-validation.test.ts already pins for the shared function).

Risk and test: the messages differ, local throws "Invalid download file name", shared throws "Invalid file name". tests/ipc/download.test.ts:162 and :343 match /Invalid download file name/, so pass the name through (assertSafeFileName(value, "download file name"), which the shared function supports). Behaviour is otherwise identical, typecheck and the suite stay green.

Worth: medium.

Suggested order

1. Item 3 (assertSafeFileName), smallest and already proven end to end, and it warms up the "worker imports from validation.ts" path.
2. Item 2 (symlink walk), same direction, one more consumer of validation.ts.
3. Item 1 (bounded read) last: it is the largest, it touches two files plus their tests, and it is the only one that changes an error class the login classifier reads.

Out of scope

None of the guards themselves. The IPC validation at the trust boundary, the path policy, the mutation-tested guards and the defence in depth inside the workers are deliberate and stay. So does the hexagonal split (pure src/domain, src/ipc and src/main as host, the renderer through window.api). tests/security-boundaries.test.ts, tests/log-provenance.test.ts, tests/text-contrast.test.ts and tests/i18n/i18n-parity.test.ts pin log provenance, no HTML sinks, contrast floors and locale parity: nothing above relaxes them. The two-transport split in network.ts (#76) stays two transports.

Some of these may appear in the SonarCloud duplicate-block list triaged in #107; that issue stays the umbrella for Sonar's own list.