Conversation
Six one-line changes in the main process survived the whole suite: sandbox and nodeIntegration on the window webPreferences, a remote host in the renderer script-src, the will-navigate guard, the window open handler and the permission request handler. The webPreferences flags and the CSP are now read as values (a flag map parsed out of the BrowserWindow options, a directive map parsed out of the meta tag) rather than matched as substrings. The handlers stay source-text pins, anchored so a renamed local or a prettier pass still passes and a weakened value does not.
…nses test(security): pin the main-process renderer defenses
The detail endpoint returns a null among Vanilla Variants' category tags. The Manage Mods filter derivations folded case on every entry during the first render, so null.toLowerCase() threw, and with no error boundary the whole page went blank for anyone with that mod installed (#370). Every reader in installedFilters.ts now goes through one helper that keeps string entries only and reads a malformed list as empty, so a shape the server does not honour can no longer take the page down.
Guarding the outer releases field let { releases: [null] } through, and
release.tags then threw on the entry: the same render crash this helper
exists to rule out. Each entry is now checked as well.
fix(mods): stop Manage Mods going blank on a null tag from the mod database
Bump to 1.7.0-beta.8
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Request changes
The new head includes PR #373, and the required checks pass. I found two blocking issues in the security contract and malformed ModDB handling that must be fixed before this promotion reaches main.
1. The permission contract accepts a denial in a dead branch
In tests/security-boundaries.test.ts:406-420, findNodes(handler.body, ...) accepts any matching callback anywhere in the handler body. The current negative fixture at lines 511-517 rejects if (false) callback(true) because of the true argument, not because the callback is unreachable. I confirmed that the helper accepts this weakened fixture:
if (false) callback(false)Please add that exact regression case and make the assertion require a direct, unconditional denial in the executable handler body. Otherwise a future change can leave permission requests conditionally or never denied while this contract test remains green.
2. Malformed release entries can still crash Manage Mods before filtering
The new installedFilters.ts guards only its own reads. useGetCompleteInstalledMods.ts:71-77 still iterates the raw dmod.releases array and reads release.modversion and release.tags first. parseModDetailResponse checks that releases is an array, but it does not validate its members. A response containing { releases: [null] } therefore throws at release.modversion; a release whose tags is null reaches evaluateModCompatibility and throws there.
Please sanitize release entries at the ModDB parser boundary or guard this hook and every other raw release consumer. Add a Manage Mods or hook regression that feeds the raw malformed JSON and verifies that loading completes without an exception. The current installedFilters unit tests do not cover this earlier path.
The five required checks passed on the new head, but these findings prevent approval of the release promotion.
The beta.7 blank Manage Mods page came from a detail payload the parser waved through: Vanilla Variants carried a null among its tags, and the first render died reading it. #371 taught the installed filters to read around that shape, but useGetCompleteInstalledMods walks the raw releases array before any filter runs, so a `{ releases: [null] }` response still threw on release.modversion and a release with `tags: null` still threw inside evaluateModCompatibility. readModDetail now cleans what it hands out. A release that is not an object is dropped, its tags become a list of strings only, and the mod's own tags get the same treatment. Every reader downstream already assumes exactly that shape, so fixing it once here is smaller than guarding four call sites and it covers the ones nobody has written yet. A release whose modversion is not a string is kept with an empty one rather than dropped, because newestReleaseFileId reads releases[0] to build the download URL and removing an entry would quietly change which file "install newest" picks. Every consumer already treats a falsy modversion as unusable. Part of #370.
|
Both findings are right, and the second one is the more important: #371 only guarded the filters' own reads while #375 fixes that at the boundary rather than per consumer. The permission pin is being tightened in a separate PR on |
|
The permission pin is up as #376. Reproduced your fixture first: on the current dev, |
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Changes requested
This re-review is anchored at the unchanged promotion head 1d5280a3db7efc6af7bb2adb32d7214530c1d446. The two blocking findings from the previous review remain, and the security contract review has two additional bypass cases.
High: malformed ModDB releases still crash Manage Mods
moddb.ts:126-137 validates only that releases is an array and returns its raw members. useGetCompleteInstalledMods.ts:71-77 then dereferences each member before installedFilters.ts runs. A response with releases: [null] throws at release.modversion; a release with tags: null throws inside evaluateModCompatibility.
The changed filter tests construct typed in-memory objects and never exercise the parser or this hook, so they do not cover the crashing path. Sanitize release objects and consumed fields at the parser boundary, or guard every raw consumer, then add a parser-to-hook or Manage Mods regression using malformed JSON.
High: the security contract accepts unreachable or conditional denials
assertPermissionRequestHandlerDenies recursively accepts callback(false) anywhere, including if (false) callback(false). assertWindowOpenHandlerDenies similarly accepts a sole denial nested inside a conditional. assertNavigationHandler requires one matching guard somewhere in the body, so an early return can make that guard unreachable while the contract still passes.
These are false-positive security tests: a future renderer defense regression can pass the suite without denying a permission request, window open, or navigation. Require direct, unconditional executable-body denials and add negative fixtures for if (false) callback(false), conditional window denial, and an early return before each navigation guard.
PRs #375 and #376 propose the missing corrections, but neither change is part of this promotion.
Verification
- Local
npm cipassed with no reported vulnerabilities. npm run typecheckpassed.npm run lint:cipassed with 15 existing warnings and no errors.npm run format:checkpassed.- Focused
installedFilters.test.tsandsecurity-boundaries.test.ts: 54 passed. npm run build:unpackpassed.- The five required GitHub contexts passed on the exact head. SonarCloud passed and the manual macOS build was skipped by policy.
git diff --checkpassed for the promotion range.
Because these findings remain, I cannot approve or merge the beta.8 promotion.
Sanitize ModDB release entries at the parser boundary
The permission pin from #367 searched the whole handler subtree with findNodes, so a denial in a branch the runtime never takes satisfied it. Zaldaryon is right on #374: `if (false) callback(false)` passed. The rule is now about placement, not only about the argument. A denial has to be a direct statement of the executable handler body: the expression body of a one-expression arrow, or a top-level statement of a block body that nothing before it can skip. Anything nested in an if, a ternary, a try, a loop or another function no longer counts. setPermissionCheckHandler and setWindowOpenHandler get the same rule, and the navigation guards now have to sit in the reachable prefix of their body, which closes an early return in front of the guard.
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Changes requested
PR #375 is now included in this promotion and resolves the malformed ModDB release finding. The current head still contains the older security contract from before #376, so the promotion is not safe to merge into main.
The following helpers still accept denials that the runtime can skip:
tests/security-boundaries.test.ts:406-420recursively acceptscallback(false), includingif (false) callback(false).tests/security-boundaries.test.ts:435-444recursively accepts anaction: "deny"return inside a conditional.tests/security-boundaries.test.ts:321-366accepts a navigation guard after an early return because it searches the handler's full direct statement list without modeling reachability.
PR #376 addresses the original forms, but its current head still has additional false-positive cases in the permission-check and reachability helpers. Land a corrected #376, or apply the equivalent fixes to this promotion, then rerun the promotion checks and request a fresh review.
Verification
- Current promotion head:
5fa04624a76e4433bf16fe86e1c73d51d657fe4d. - Current
devpoints to the same head. - The five required contexts pass, as do the Ubuntu and Windows matrices and SonarCloud. macOS is skipped by repository policy.
- The focused parser, filter, and security suites pass 87 tests, but the security contract still accepts the bypasses above.
- No merge is eligible while these findings remain.
…swers with
The check-handler assertion accepted any unconditional false expression, so
`() => { false }` and `() => { false; return true }` both passed while returning
undefined and true. The reachability helper also stopped only at exits written
directly in the block, so a conditional return or throw before a denial left it
unreachable and still accepted.
A denial now has to be the value the handler answers with: an expression body,
or a reachable direct return. And a statement that can leave the handler on any
path, wherever the return, throw, break or continue sits inside it, ends the
reachable run.
…itional test: require an unconditional denial in the main-process security handlers
|
Both blockers are in: #375 (release entries sanitised at the parser boundary) landed at 13:44 and #376 (denials must be direct, reachable statements of the executable handler body; bare |
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Both findings on the promotion are closed now that the head has advanced to 7a90aa3.
Finding 1, the AST security contract. #376 merged into dev as 7a90aa3, so this promotion head carries canLeaveTheBody and reachableStatements. I reran the shipped assert helpers against the weakened shapes from the review: if (false) callback(false), if (granted) return; callback(false), and a window-open denial nested in an if are all rejected now, and the four real handlers in src/main/index.ts still pass. npx vitest run tests/security-boundaries.test.ts is 32 passing on this head.
Finding 2, the raw releases walk. #375 is on dev, so src/domain/mods/moddb.ts sanitises every release at the parser boundary: readRelease coerces tags through cleanStrings and modversion to a string and drops entries that are not objects, so useGetCompleteInstalledMods can no longer meet a malformed entry before the filters run.
The promotion range against main is the version bump to 1.7.0-beta.8, the #375 ModDB shape fixes, the #367 and #376 security-boundary pins, and the #371 Manage Mods regression test. #368, #369 and #372 are not in it, which matches the PR description holding them for beta.9. I scanned the full added diff: no AI attribution, no private paths, no credentials, no files outside src/, tests/ and the two package files.
Full local gate on 7a90aa3 is green: typecheck, lint:ci (0 errors, 15 pre-existing hook warnings), format:check, test:coverage at 93.39% statements and 90.18% branches against the 87/85 floors, build:unpack on Electron 44.1.1. All five required GitHub contexts pass on the head, SonarCloud passes, macOS is skipped by policy.
Approving. Merge as a merge commit per the description. Tagging main is the maintainers' release step and is not part of this review.
2 participants
Hotfix promotion. Merge #373 first so
maincarries 1.7.0-beta.8, then merge this one as a merge commit (not a squash), then tagmain.What players get
Opening Manage Mods went blank for anyone with a mod whose mod database listing carries a null among its category tags, Vanilla Variants being the popular case, reported within eleven hours of beta.7 on the listing and the Discord (#370). The filter derivations added in beta.7 folded case on every entry during the first render and threw on the null. Every reader now keeps string entries only and treats a malformed list, or a null entry inside a valid list, as nothing, so a shape the server does not honour can no longer take the page down (#371).
Nothing else user-facing. The other change on
devsince beta.7 is #367, which pins the main process's renderer defenses in the test suite (sandbox, node integration, CSP script sources, navigation, window-open and permission handlers) so a one-line regression on any of them turns CI red. Tests only.Held back on purpose for beta.9, with a proper test round: the error boundary that turns a page crash into a message and a log line (#372), the readable compatibility verdict on the mod release list (#369), and the login failure log classification (#368).
Before tagging
Confirm
package.jsononmainreads 1.7.0-beta.8 after this merges, then tag frommain.