test: require an unconditional denial in the main-process security handlers

[Pixnop]

Zaldaryon's first finding on the beta.8 promotion (#374), quoted in full:

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.

He is right, and I reproduced it before touching anything: with the pin as it stands on dev, wrapping the real callback(false) in if (false) leaves the whole suite green. The pin I wrote in #367 checks what the callback is called with, never where the call sits, so a denial in a branch the runtime never takes satisfied it.

Second review round

Zaldaryon found two more holes in the first version of this branch, and both were real. I reproduced each one before changing anything.

The first: the check-handler assertion accepted any unconditional false expression, and the helper that collected those expressions took plain expression statements as well as returns. So setPermissionCheckHandler(() => { false }) passed while handing Electron undefined, and setPermissionCheckHandler(() => { false; return true }) passed while granting every permission check.

The second: the reachability helper stopped only at an exit written directly in the block. if (x) return; callback(false) walked straight past it, and so did a conditional throw, a try { return } catch {} and a loop containing a return. The denial after any of those runs on some paths and not others, and the pin still accepted it. It reproduced on the permission request callback, the navigation guards and the window-open denial.

The rule as it now stands

Two conditions. The first is about what the denial is, the second about where it sits.

A denial is the value the handler answers with. Electron reads what these handlers return, so evaluating a value and throwing it away is not answering with it. That means one of two shapes: the expression body of a one-expression arrow, which is what src/main/index.ts ships for both permission handlers, or a return that the runtime reaches. A bare false on its own line is not a denial, and neither is a bare ({ action: "deny" }). The permission request handler is the exception in spelling, not in principle: there the answer travels through the callback, so the denial is the callback(false) call, whether written as the expression body, as a statement, or returned. A bare false, or a mention of callback that never calls it, answers nothing and is rejected with is not called.

Nothing before the denial may be able to leave the handler. This is rule (a) of the two Zaldaryon offered: a statement that contains a return, a throw, a break or a continue anywhere inside it, outside a nested function, ends the reachable run, and anything written after it is treated as unreachable. Bodies of nested functions are skipped, because a return in there leaves that function rather than this handler.

I took (a) rather than validating every path because I read the four shipped handlers first and none of them needs (b). Three of them are one expression. The fourth, the window-open handler, is the only one that runs anything at all before its denial: a try/catch around assertAllowedBrowserUrl and shell.openExternal, and neither block contains a return, a throw, a break or a continue. So (a) costs the real code nothing today, while (b) means a genuine control-flow walk in a contract test, with the denial having to dominate every exit, and every future reader has to trust that walk when the pin goes green.

(a) is conservative in one visible way, and I think that is the right trade in a security pin. A throw written inside a try whose catch swallows it does not really leave the handler, but this rule rejects a denial written after it. openAfterTryExit in the fixture table records that on purpose. The cost of the false positive is reflowing a handler; the cost of the false negative is a permission denial that never runs.

Two helpers carry it. canLeaveTheBody says whether a statement can hand control out of the handler on any path. reachableStatements returns the direct statements of a block up to and including the first one that can. directExpressions returns the expressions the handler evaluates directly, each tagged with whether it is the handler's answer (an expression body or a return) or a discarded expression statement. A call nested in an if, a ternary, a try, a loop or another function is in none of these, which is the whole point: findNodes walks the entire subtree, so anything built on it alone accepts a defense the runtime can step over.

Per handler:

• setPermissionRequestHandler: every call to the callback anywhere still has to pass false (that rule stays, it catches log(false); callback(true)), and one of those calls has to be the reachable direct denial.
• setPermissionCheckHandler: at most one return in the whole handler, and false has to be the expression body or the value of a reachable direct return.
• setWindowOpenHandler: at most one return anywhere, and the action: "deny" object has to be the expression body or a reachable direct return. The expression-bodied form is now accepted, which it was not before; it is the same denial written shorter.
• will-navigate, will-redirect, will-frame-navigate: the guard is looked up in the reachable prefix, so anything that can leave the handler before it pushes the guard out of that prefix.

Failure messages still name the handler, so a red test says setPermissionCheckHandler: ... rather than leaving the reader to find which of the four broke.

Fixtures

Accepted:

Fixture	Result
the shipped shapes in src/main/index.ts	accepted
parameters renamed and bodies reflowed to blocks (respond(false), return false, renamed details)	accepted
four-parameter request handler (kept from #367)	accepted
expression body in each of the three handlers, () => false, => callback(false), => ({ action: "deny" })	accepted

Rejected:

Fixture	Rejected with
if (false) callback(false)	direct statement
try { callback(false) } catch {}	direct statement
granted ? callback(false) : callback(true)	called with false
granted ? callback(false) : callback(false)	direct statement
const deny = () => callback(false), never called	direct statement
empty request handler body	is not called
request handler { false }	is not called
request handler { callback }, referenced and never called	is not called
request handler if (granted) return; callback(false)	direct statement
request handler if (granted) throw new Error("no"); callback(false)	direct statement
request handler try { return } catch {} callback(false)	direct statement
request handler for (const p of pending) { return } callback(false)	direct statement
check handler if (false) return false	direct statement
check handler try { return false } catch {}	direct statement
check handler granted ? false : false	direct statement
check handler const deny = () => false	direct statement
check handler if (granted) return true; return false	two returns
empty check handler body	direct statement
check handler { false }	must return false
check handler { false; return true }	must return false
check handler if (granted) throw new Error("no"); return false	direct statement
check handler try { throw new Error("no") } catch {} return false	direct statement
check handler for (const p of pending) { return true } return false	two returns
window open if (details.url) { return { action: "deny" } }	direct statement
window open try { return { action: "deny" } } catch {}	direct statement
window open throw ...; return { action: "deny" }	direct statement
empty window open body	exactly one object return
window open { ({ action: "deny" }) } as an expression statement	exactly one object return
window open if (details.url) throw new Error("no"); return { action: "deny" }	direct statement
window open try { throw new Error("no") } catch {} return { action: "deny" }	direct statement
window open for (const feature of details.features) { throw ... } return { action: "deny" }	direct statement
window open if (details.url) return; return { action: "deny" }	exactly one object return, caught by the return count before reachability
will-navigate guard after an early return	one direct guard
will-navigate guard inside a try	one direct guard
will-navigate guard after if (trusted) return	must prevent a rejected URL
will-navigate guard after if (trusted) throw new Error("no")	must prevent a rejected URL
will-navigate guard after try { return } catch {}	one direct guard
will-navigate guard after for (const frame of frames) { return }	one direct guard

The comment-only, dead-branch-with-true and second-false-call fixtures from #367 are unchanged and still rejected. The two navigation cases that report must prevent a rejected URL rather than one direct guard are the ones where the early exit is itself an if: it becomes the only reachable if of the body, and its condition is not a URL rejection, so the guard lookup fails there instead. Rejected either way, and the message still names the event.

Mutants

Each applied alone against the committed tree, the pin run, the mutant reverted, git status checked clean before the next one. All against the real handlers in src/main/index.ts.

First round, kept here for the record:

Mutant	On dev	Here	Killed by
M1 wrap callback(false) in if (false)	survives	killed	setPermissionRequestHandler: ... direct statement of the handler body
M2 early return, denial moved into a try	survives	killed	same
M3 ternary on a flag, both branches denying	survives	killed	same
M4 denial deleted	killed	killed	setPermissionRequestHandler: ... is not called
M5 window open denial behind if (details.url)	survives	killed	denies every window the renderer asks Electron to open
M6 check handler denial in a dead branch	killed	killed	refuses every renderer permission request
M7 will-navigate guard behind an early return	survives	killed	blocks any main-frame navigation the renderer policy rejects

Second round, the five for the findings above. Every one of them leaves the suite green on the first version of this branch:

Mutant	Result	Message
M8 check handler body replaced with { false }	killed	setPermissionCheckHandler: permission check handler must return false, as the expression body of the handler or as a reachable direct statement of the handler body ...
M9 check handler body replaced with { false; return true }	killed	same
M10 if (details.requestingUrl) return inserted before callback(false)	killed	setPermissionRequestHandler: permission request callback callback must be called with false as a reachable direct statement of the handler body ...
M11 if (details.url) throw new Error("blocked") inserted before the window-open denial	killed	setWindowOpenHandler: window open handler must return action deny, as the expression body of the handler or as a reachable direct statement of the handler body ...
M12 if (event) return inserted before the will-navigate guard	killed	will-navigate must prevent a rejected URL, under blocks any main-frame navigation the renderer policy rejects

Controls, which must stay green:

Control	Result
C1 parameters renamed across all four handlers and the bodies reflowed to blocks	survives, as intended
C2 C1 plus the guard function renamed to isPermittedMainFrameUrl	killed, two navigation tests

C2 is worth repeating from the first round. #367 listed a control that renamed the will-navigate parameters and the guard function and called it green, and that half of the line was wrong: isRejectedUrlCondition matches isAllowedMainFrameUrl by name, so renaming the guard has always broken the navigation pin. Parameter renames and reformatting are tolerated, the guard function name is pinned. I left it pinned rather than loosening a security assertion to match an old note.

Testing

On head df7a7b8:

• npm run typecheck: passed (node, web, tests).
• npm run lint:ci: 0 errors, the same 15 pre-existing React Hooks warnings.
• npm run format:check: passed.
• npm run test:coverage: 161 files, 2043 passed, 2 skipped; 93.4% statements, 90.21% branches, 92.7% functions, 94.97% lines locally, floors 87 / 85 / 85 / 89. The CI run on this head is the reference figure if the two differ by a hundredth.
• npx vitest run tests/security-boundaries.test.ts: 32 passed, up from 29.

src/main/index.ts is untouched. The only file in the diff is tests/security-boundaries.test.ts.

Type

• Bug fix
• Feature
• Performance
• Refactor or cleanup
• Tests only
• Docs or build

Checklist

• Targets dev, not main.
• npm run typecheck passes.
• npm run lint:ci passes.
• npm run format:check passes.
• npm run test:coverage passes, coverage at or above the floor in vitest.config.ts.
• npm run build:unpack passes.

Related issues

• Follows test(security): pin the main-process renderer defenses #367, which added the pin this tightens
• Unblocks Promote 1.7.0-beta.8 to main #374, finding 1 of 2 (the ModDB release sanitising is the other half and is not in this branch)
• Part of Pin the main-process renderer defenses in security-boundaries #349

[Zaldaryon]

Changes requested

The new reachability checks close the original branch, try, ternary, and early-return examples, but the permission contract still accepts handlers that do not deny.

At tests/security-boundaries.test.ts:477, the assertion accepts any unconditional false expression. Because unconditionalExpressions also collects direct expression statements, both handlers below pass the contract even though the first returns undefined and the second returns true:

session.defaultSession.setPermissionCheckHandler(() => { false })
session.defaultSession.setPermissionCheckHandler(() => { false; return true })

Restrict the check to an expression-bodied false or a reachable direct return false, and add both handlers as negative fixtures.

There is a second reachability gap. reachableStatements stops only at exits that appear directly in the handler block. A conditional return or throw before the later denial can still make the denial unreachable while the helper accepts it. Sol reproduced this for the permission request callback, navigation guard, and window-open denial. Reject earlier control flow that can leave the handler, or validate every path, and add regression fixtures for those three handler families.

Verification

The exact head is current with dev. Required checks, test matrices, and SonarCloud pass. The focused security suite passes 29 tests, but it does not cover these bypasses. No merge is eligible until the contract rejects them.

[Pixnop]

@Zaldaryon both findings were correct, and I reproduced each of them before touching the file.

The bare false expression statement was the worse of the two: () => { false } handed Electron undefined and () => { false; return true } granted every check, and the pin was green for both. A denial now has to be the value the handler answers with, which means an expression body or a reachable direct return, and the same discipline is applied to the window-open object and to the callback(false) call.

On the reachability gap I took your option (a). I read the four shipped handlers first: three are a single expression, and the only one that runs anything before its denial is the window-open handler, whose try/catch contains no return, throw, break or continue. So the conservative rule costs the real code nothing, while a dominator walk would put a control-flow analysis inside a contract test that every future reader has to trust. The one visible cost is that a throw inside a locally caught try before a denial is rejected even though control does reach it; openAfterTryExit records that on purpose, and reflowing a handler is cheaper than a denial that never runs.

Five mutants on the real src/main/index.ts, each alone against the committed tree and reverted after: the check body as { false }, as { false; return true }, a conditional return before the permission callback, a conditional throw before the window-open denial, and a conditional return before the will-navigate guard. All five turn the pin red naming the handler, and all five were green on the previous head. The control that renames parameters and reflows the bodies still survives. Tables and the full fixture list are in the PR body.

[Zaldaryon]

Both of the holes from the last round are closed, and I checked them against the shipped handlers rather than only the fixtures.

assertPermissionCheckHandlerDenies now requires the false to be the value the handler answers with, so () => { false } (hands Electron undefined) and () => { false; return true } (grants) are both rejected, pinned in "rejects a denial written as an expression statement that discards it". canLeaveTheBody plus reachableStatements stop the reachable prefix at the first statement that can return, throw, break or continue, so a denial written after a conditional exit no longer counts, pinned across all four handler families in "rejects a denial reached only when an earlier statement does not leave the handler". canLeaveTheBody skips nested function bodies through ts.isFunctionLike, and it is only ever handed a statement from the handler body's own list, so the handler's own arrow is never the visited node.

The real handlers in src/main/index.ts still pass: setPermissionCheckHandler(() => false) and setPermissionRequestHandler((_webContents, _permission, callback) => callback(false)) are accepted as expression bodies, and the window-open handler's leading try { ... } catch { logMessage(...) } contains no return, throw, break or continue, so reachableStatements does not truncate before its return { action: "deny" }.

npx vitest run tests/security-boundaries.test.ts is 32 passing here, up from 29. Full local gate on df7a7b8 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, and build:unpack on Electron 44.1.1.

One follow-up for a later PR, not a blocker here: { action: "deny", ...override } is accepted, but a spread written after action would override it at runtime. That shape does not happen by accident the way a conditional return does, so it should not hold up the merge this unblocks. A one-line guard rejecting a trailing SpreadAssignment would close it.

Approving.