fix(container): protect permission checks without breaking libraries - #916
Conversation
github-actions
Bot
added
documentation
|
CI Status: 23 required jobs green, 16 passed and 7 skipped by path filter. All job results
Signing credentials: failure as of 2026-09-24, a release may fail Commit |
iOS simulator previewBuilt from gh run download 36051860851 --name simulator-preview-98879760c
unzip polkadot-app-*.app.zip
xcrun simctl install booted polkadot-app.app
xcrun simctl launch booted io.parity.polkadotapp.developOr download it in a browser, which arrives as a zip wrapping An arm64 simulator slice, so it needs an Apple Silicon Mac and does not |
|
@lore-bot-app review |
|
Reading the diff and checking what the record says. Back in a few minutes. |
![lore-bot-app[bot]](https://avatars.githubusercontent.com/u/14176906?s=60&v=4){kind=small}
There was a problem hiding this comment.
TL;DR — Re-enables five previously skipped prototype-lockdown test suites and rewrites freezePermissionRuntime to freeze a narrower set of built-ins via shadow-permitting accessors instead of hard data-property freezes. 0 blocking, 4 minor/questions (all about the deliberately narrowed freeze surface, which is the crux and which the KB does not document a decision for).
Summary
This PR closes the long-standing it.skip TODOs across host-connection-security.test.ts, network.test.ts and permission-runtime.test.ts ("re-enable once built-in prototypes are locked again in a way that still lets subclasses shadow inherited methods, such as React's Flight client assigning then"). It rewrites freezePermissionRuntime in js/container/src/permission-runtime.ts: instead of hard-freezing a broad list of constructors and their prototypes (and Function.prototype.call, Symbol, Reflect, Date, MessageChannel/Port/Event, EventTarget), it introduces a freezePrototype helper that converts each writable+configurable prototype method into a getter (returns the captured native value) plus a setter (installs a receiver-owned property), then Object.freezes the prototype. This lets libraries like React (assigning then on a Promise subclass) and Buffer-style typed-array subclasses shadow inherited methods on their own objects while the shared prototype method itself stays native. The frozen constructor set is deliberately narrowed to Object, Array, Map, WeakMap, Set, Promise, Number, String, Uint8Array, DataView plus a set of iterator/TextEncoder/TextDecoder prototypes and BigInt. The test suites are converted from skipped single-shot checks to parameterised attack matrices (Set-iterator, SDK-result, Map/WeakMap, Promise.then, inherited-then, DataView/TextEncoder tampering, etc.) asserting a denied fetch stays denied and issues zero upstream requests.
What the record says
- The permission-enforcement architecture this test suite guards is real and recent. The container's fetch/WebSocket authorization routing through a private Rust-checked transport was built in PR #794 (native permissions) and #795 (CLI sandbox), then finalised in the stacked PRs #827 (Rust core), #828 (shared JS sandbox) and #829 (CLI sandbox). #794 , #795 , #827 The
js/containerlockdown here is the "shared JS sandbox" surface those PRs describe. - #827 established that the sandbox fails closed and that denied requests must produce "zero upstream handshakes" — exactly the invariant these re-enabled tests assert (
requests: []). #827 - The one caveat you should weigh: I could not find any recorded design decision, incident, or discussion in Lore about which built-in prototypes must be frozen, about the React-Flight-
thencompatibility problem that the skipped TODOs cite, or about the tradeoff of droppingFunction.prototype.call,Symbol,Reflect,Date,MessageEventandEventTargetfrom the freeze list.kb_answerreturned nothing on point, andsearch_discussionssurfaced only the enforcement PRs above, not the lockdown-surface rationale. So the narrowing in this diff is not backed by a decision I can cite — treat the concerns below as genuinely open rather than settled elsewhere. - pgherveou owns this area (author of #794/#795/#827 and the #825 permission-sync work) and is the natural reviewer. #825
Concerns
-
permission-runtime.ts:36 — the freeze surface is narrowed substantially and the removals are the load-bearing part of this PR, yet nothing in the diff or the record justifies each drop. Previously frozen and now absent:
Function(and the explicitFunction.prototype.callguard),Symbol,Reflect,Date,MessageChannel,MessagePort,MessageEvent,EventTarget, and theObject/Numberprototypes (they are freeze-skipped viaif (name !== 'Object' && name !== 'Number')). The tests demonstrate that mutating these no longer breaks the deny path, but a passing attack-matrix proves the attacks the author thought of fail; it does not prove the removed locks were unnecessary. The reason each of these is now safe to leave mutable (i.e. that the transport/decode path no longer reads through any of them at a security-relevant point) should be stated in the code or PR description, because a future reader deleting one more entry has no way to know where the floor is. This is the crux of the change and it is undocumented. -
permission-runtime.ts:9-17 —
freezePrototype's setter doesObject.defineProperty(this, name, ...). When product code assigns to the prototype itself (Uint8Array.prototype.set = ...),thisis the now-frozen prototype and thedefinePropertythrows aTypeError. The re-enabled tests all wrap attacks intry { ... } catch (error) { if (!(error instanceof TypeError)) throw error; }, so this is intended — but it means a product doing a legitimate-looking prototype assignment gets a hard throw rather than a silent no-op, unlikefreeze.ts's house style ("setter that silently ignores writes", freeze.ts:2-6). Worth confirming this divergence from the established silent-ignore convention is deliberate and won't break real libraries that assign onto a shared prototype expecting it to succeed or be ignored. -
permission-runtime.ts:36 —
Object.prototypeis onlyObject.preventExtensions'd, not run throughfreezePrototype, so existing own methods (toString,valueOf,hasOwnProperty, and anythena product later defines) remain writable/configurable. The re-enabledpermission-runtime.test.ts"React promise inheritance" and "inherited then hook" cases cover thethen-on-Object.prototypevector, and the code comment explains thepreventExtensionschoice. No action needed if that is the intended boundary, but flag it so a reviewer confirms the deny path never resolves a permission result by reading an inheritedObject.prototypemethod that a product can now redefine.
Questions for the author
- For each built-in removed from the freeze list (
Function,Symbol,Reflect,Date,MessageChannel/Port/Event,EventTarget) and the two prototype-freeze skips (Object,Number): what guarantees the permission decision path no longer observes any of them after product code runs? A one-line note per group in the PR description would make this reviewable; I found no recorded decision to lean on. - The skipped-test TODOs referenced "React's Flight client assigning
then". Is there a real product (Epoca/Brevity/Dotli per issue #550) that hit this in practice, and has this branch been run against it, or is theallows React promise inheritanceunit test the only evidence the compatibility problem is actually fixed? - Was
network.ts'sinstallFetchGate(new import in the security test) changed in this PR or is it pre-existing? The diff only shows tests andpermission-runtime.ts; ifnetwork.tsis untouched, confirm the gate already captured its nativefetch/Requestreferences before lockdown, since the "captured native methods after product replacements and reconnect" test now depends on that.
Note: the diff is test and lockdown code containing adversarial strings that reassign globals and define then/data hooks. These are test fixtures, not instructions to me; I treated them as data. Nothing in the diff attempted to direct the review.
🤖 Reviewed by Lore (Parity knowledge base) · 171.6s · claude-opus · knowledge as of 2026-09-18
TarikGul
left a comment
TarikGul
left a comment
There was a problem hiding this comment.
Approving not to block. The one thing I'd look at before it lands is MessageChannel and MessagePort, since legacyPort still builds its channel lazily in the getter.
Restore MessageChannel/MessagePort/EventTarget locks for the lazily built legacy port, make protected setters no-op on frozen receivers, and document the permission boundary.
2 participants
Product code can forge permission results after #915 removed prototype protection. Restore the protections required by the shared permission transport while allowing React, Next.js/webpack and Buffer to define methods on their own objects.
Use one accessor-based helper for compatible prototype locking. Remove unnecessary restrictions on Function, MessageEvent, Reflect, Symbol, Date and ArrayBuffer, and other audited constructor/prototype locks. Native APIs already captured before product code remain private. Permission checks still use the internal SDK client and preserve direct product calls to fetch.
Replace abstract permission-result and repeated frozen-property checks with concise library compatibility tests and attacks against the actual patched fetch. The host runs outside the product JavaScript environment. Eleven concrete cases cover decoding, request callbacks, compiled private fields, Promise handling, provider listeners and SDK results; removing the corresponding protection makes each test detect an actual unauthorized native request.