Repository navigation
Fix three security and correctness issues across native bridge and forms - #37
Merged
Merged
Conversation
Three defects where a value quietly became a different value.
resetNativeChannel restarted correlation ids at 1. The reset exists to be
called on teardown with calls in flight, and it rejects them -- but it cannot
cancel the native work behind them, so a method that answers late still sends
its reply home under the id it was given. With ids restarted, that stale reply
matched a fresh call and resolved it with another call's result: a wrong answer
rather than a missing one. Ids are monotonic across resets now, so the late
reply matches nothing and takes the existing "reply nobody is waiting on" path.
The background bridge read module and method names straight off the registry,
which walks the prototype chain. NativeModules.constructor is Object and
NativeModules.toString is a function, so isNativeModuleAvailable('constructor')
answered true for a module nobody linked, and callNative applied what it found
instead of reporting "not linked". Names are screened against Object.prototype
rather than tested with hasOwnProperty, because the engine is free to hand
modules over through a prototype or a proxy and an own-property test would call
those unlinked.
createURL encoded its host with encodeURI, which leaves /, ? and # alone --
exactly the three characters that terminate an authority. A host carrying one
produced a different URL than the caller described, and the query the same call
appended landed after a ? that was already there. Ports and IPv6 brackets are
still left alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The JSX runtime wrote any prop it did not otherwise recognise through
setAttribute, and a string-valued on* prop is not an attribute -- it is an
inline event handler the browser compiles and runs. The ordinary way one
arrives is a {...props} spread carrying data nobody audited, so this was
markup injection through a path grep bindHtml does not find. That grep being
complete is the package's entire XSS story, and AGENTS.md states it as an
invariant, so the guard goes at setAttribute: the one chokepoint both the
static and the reactive paths share.
The test is on the element rather than on the name, because the name alone
cannot tell an inline handler from a custom attribute: onclick is a slot every
element has and once is not a slot at all. A function-valued handler never
reaches here -- jsx binds those with addEventListener first -- so custom event
names keep working too.
recycle's inventory diff is keyed by itemKey, and two rows under one key made
it describe a list nobody has. The map keeps only the last index for a repeated
key, so every earlier row reports as having moved from that index on an update
that changed nothing, and the engine re-queries against an inventory that
disagrees with the data. Both list() implementations already warn when their
keys collide; this is the same mistake with a symptom that is much harder to
trace back to itemKey, because a scroll drifting looks like anything but a key.
The check is free -- the set it reads was already being built.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
isSubmitting was tracked and never consulted, so two taps ran onSubmit twice.
The documented disabled={form.isSubmitting} does cover a double-click, but only
on that one button: handleSubmit is also wired straight to a confirm key and
called by hand. On Lynx there is no form element and no platform disabled at
all -- the playground's own screens wire onTap={form.handleSubmit} bare -- so
nothing stood between a second tap and a second onSubmit.
Both packages get the same guard, since a form is state and this defect is in
the half that has no platform in it. The guard lifts when the first submit
settles, so a form stays usable after a failed save; the tests pin both halves
of that, and fail without it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
serveCall read the registry a second time, raw, for the value it applies the method against. It was safe -- resolveMethod throws on the very next line for a name off Object.prototype, so the raw value was never applied -- but that is a guarantee that lives in the order of two statements and nothing says so. One screened lookup, used by all three callers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both Android backstops caught Exception, and the failure they most need to survive is not one. A host that linked the module but shipped without androidx.security.crypto -- R8 stripped it, or it was never declared -- raises NoClassDefFoundError, which is an Error. It escaped the executor without invoking the callback, so the caller's promise never settled: not a failed read with a branch to take, but a screen waiting forever, which is the exact failure mode this package's whole result-shaped design exists to avoid. isSecureStorageAvailable was worse on two counts. It had no backstop at all, and it is the call an app makes to ask whether the store works -- so the state it must survive is the store being as broken as it gets. The JavaScript side cannot cover for it either: its catch turns a rejection into false, and a callback that never fires is not a rejection. The Kotlin is not compiled in this environment (no ANDROID_HOME, so check:android skips); CI compiles it. The TypeScript parity suite, which parses this file's @LynxMethod surface, passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes three separate but critical issues:
Native bridge call correlation:
resetNativeChannelwas restarting correlation IDs at 1, causing stale replies from methods that answered after a reset to land on fresh calls and resolve them with unrelated results. IDs are now monotonic across resets.Prototype pollution in native module resolution: The background bridge was resolving module and method names off
Object.prototype, soisNativeModuleAvailable('constructor')would returntruefor an unlinked module andcallNativecould applyObjectinstead of reporting "not linked".Form submission reentrancy:
handleSubmitwas not checkingisSubmitting()before runningonSubmit, allowing multiple concurrent submissions when the form was triggered via multiple paths (button clicks, key presses, direct calls).Additionally, this PR includes fixes for:
setAttributenow blocks writing string values toon…event handler attributes, which are script sinks equivalent toinnerHTML.createURLnow properly encodes/,?, and#in thehostoption, which were previously left unencoded and would silently produce different URLs.isSecureStorageAvailableand the general error backstop now catchThrowableinstead of justException, handlingNoClassDefFoundErrorwhen the crypto dependency is missing.itemKey, which causes the inventory diff to describe a list that doesn't exist.Changes
nextId = 1reset inresetNativeChannelso correlation IDs remain monotonic; addedlookupModulehelper to screen both module resolution paths againstObject.prototype; addedinheritedhelper to detect prototype namesisEventAttributecheck insetAttributeto block inline event handler attributes with string valuesisSubmitting()guard at the start ofhandleSubmitin both@amritk/miniand@amritk/mini-lynxencodeAuthorityhelper to encode only the delimiters (/,?,#) that would end an authority, preserving ports and IPv6 bracketsExceptiontoThrowablein bothisSecureStorageAvailableand theanswerhelperdifffunction with a console warningTesting
on*attributeshttps://claude.ai/code/session_01EkaxNKHeqRTN1FH4hoFZav
📦 Bundle-size delta vs main (
b4bd35d→ab21b2d)Gzipped bytes of each bundled entry (esbuild, minified, browser/es2022, optional peer deps external). Bundling is deterministic, so unlike a timed benchmark these numbers carry no noise and every Δ is exact. ⚪ within ±0.5% · 🟢 smaller · 🔴 larger. Each package's
core (.)must stay flat as subpath features land — that is the whole charter, andsrc/core-size-budget.test.tsholds the absolute ceiling.