Fix three security and correctness issues across native bridge and forms

.changeset/form-submit-reentrancy.md

@@ -0,0 +1,14 @@
+---
+'@amritk/mini': patch
+'@amritk/mini-lynx': patch
+---
+
+`handleSubmit` now ignores a second submit while the first is still in flight.
+
+`isSubmitting` was tracked but never consulted, so two taps ran `onSubmit`
+twice. The documented `disabled={form.isSubmitting}` covers a double-click on
+that one button, but `handleSubmit` is also wired straight to a confirm key and
+called by hand — and on Lynx there is no form element and no platform
+`disabled` at all, so a `bindtap` had nothing between a second tap and a second
+`onSubmit`. That is where an order gets placed. The guard lifts as soon as the
+first submit settles, so a form stays usable after a failed save.

.changeset/native-bridge-hardening.md

@@ -0,0 +1,25 @@
+---
+'@amritk/mini-lynx-native': patch
+'@amritk/lynx-deep-linking': patch
+---
+
+Fix three ways a call or a URL could quietly mean something other than what it
+said.
+
+`resetNativeChannel` no longer restarts correlation ids at 1. A reset rejects
+the calls in flight but cannot cancel the native work behind them, so a method
+that answered late still sent its reply home under the id it was given — and
+that reply landed on whichever fresh call had been filed under the same number,
+resolving it with an unrelated call's result. Ids are monotonic across resets
+now, so a stale reply matches nothing and is dropped.
+
+The background bridge no longer resolves module or method names off
+`Object.prototype`. `NativeModules.constructor` is `Object` and
+`NativeModules.toString` is a function, so `isNativeModuleAvailable('constructor')`
+answered `true` for a module nobody linked and `callNative` applied whatever it
+found instead of reporting "not linked".
+
+`createURL` now encodes `/`, `?` and `#` in the `host` option. Those three
+characters end an authority, so a host carrying one produced a different URL
+than the caller described — and the `query` the same call asked for landed after
+a `?` that was already there. Ports and IPv6 brackets are still left alone.

.changeset/secure-storage-error-backstop.md

@@ -0,0 +1,19 @@
+---
+'@amritk/lynx-secure-storage': patch
+---
+
+The Android half no longer leaves a promise unsettled when the store is broken
+in the worst way.
+
+Both backstops caught `Exception`, and the failure that matters most here is
+not one. A host app that linked the module but shipped without
+`androidx.security.crypto` — R8 stripped it, or the dependency was never
+declared — raises a `NoClassDefFoundError`, which escaped the executor without
+ever invoking the callback. The caller's promise then never settled at all:
+not a failed read it could branch on, but a screen waiting forever. Worst on
+`isSecureStorageAvailable`, since that is precisely the call an app makes to
+find out whether the store works, and the JavaScript `catch` around it cannot
+help — a callback that never fires is not a rejection.
+
+Both now catch `Throwable`, so a broken store reports as `keystoreFailure` and
+an unavailable one as `false`.

.changeset/xss-and-recycle-keys.md

@@ -0,0 +1,24 @@
+---
+'@amritk/mini': patch
+'@amritk/mini-lynx': patch
+---
+
+Close an inline-handler injection in the JSX runtime, and say something when a
+recycling list's keys collide.
+
+`@amritk/mini` no longer writes an `on…` prop through as an attribute when its
+value is not a function. `setAttribute('onclick', someString)` installs an
+inline handler — script the browser compiles and runs — and the ordinary way a
+string got there was a `{...props}` spread carrying data nobody audited. That
+was markup injection on a path `grep bindHtml` does not find, which is the grep
+this package's whole XSS story rests on. The name is tested against the element,
+so a custom attribute that merely starts with those two letters (`once`,
+`online`) is untouched, and a function-valued handler still binds through
+`addEventListener` exactly as before.
+
+`@amritk/mini-lynx`'s `recycle` now warns when two rows share an `itemKey`. The
+inventory diff is keyed, so a repeat made it describe a list nobody has: the map
+keeps only the last index for a duplicated key, and every earlier row then
+reports as having moved from it — on an update that changed nothing. Both
+`list()` implementations already warn on a key collision; this is the same
+mistake with a symptom much harder to trace back.

packages/lynx-deep-linking/src/create-url.ts

@@ -45,11 +45,26 @@ export const createURL = (scheme: string, path = '', options: CreateURLOptions =
 
   // A host and a path both want the `/` between them; a path with no host is
   // the `myapp://profile/42` shape, where the first segment IS the authority.
-  const base = `${scheme}://${encodeURI(host)}${host && segments ? '/' : ''}${segments}`
+  const base = `${scheme}://${encodeAuthority(host)}${host && segments ? '/' : ''}${segments}`
 
   const search = Object.entries(query)
     .map(([key, value]) => `${encodeURIComponent(key)}=${encodeURIComponent(value)}`)
     .join('&')
 
   return `${base}${search ? `?${search}` : ''}${fragment ? `#${encodeURIComponent(fragment)}` : ''}`
 }
+
+/**
+ * Encodes an authority without destroying the punctuation that belongs in one.
+ *
+ * `encodeURIComponent` is too strong here — it would eat the `:` in
+ * `localhost:3000` and the brackets around an IPv6 address — and `encodeURI` is
+ * too weak, because it leaves `/`, `?` and `#` alone and those are exactly the
+ * three characters that END an authority. A host carrying one of them would not
+ * be a badly encoded host, it would be a different URL: `{ host: 'a?b' }` turns
+ * the rest of the link into that host's query string, and the `query` this
+ * function was asked to append lands after a `?` that is already there.
+ *
+ * So encode the delimiters and leave the rest to `encodeURI`.
+ */
+const encodeAuthority = (host: string): string => encodeURI(host).replace(/[/?#]/g, encodeURIComponent)

packages/lynx-deep-linking/src/url.test.ts

@@ -119,6 +119,18 @@ describe('createURL', () => {
     expect(createURL('myapp', 'profile/42', { host: 'app' })).toBe('myapp://app/profile/42')
   })
 
+  it('keeps a port in the host but encodes the delimiters that would end it', () => {
+    // `:` belongs in an authority; `/`, `?` and `#` terminate one, so a host
+    // carrying them would silently produce a different URL than the caller
+    // described — and the query below would land after a `?` already there.
+    expect(createURL('https', 'a', { host: 'localhost:3000' })).toBe('https://localhost:3000/a')
+
+    const url = createURL('myapp', 'path', { host: 'a?b', query: { k: 'v' } })
+    expect(url).toBe('myapp://a%3Fb/path?k=v')
+    expect(parseURL(url).host).toBe('a%3Fb')
+    expect(parseURL(url).query).toEqual({ k: 'v' })
+  })
+
   it('builds a bare scheme URL', () => {
     expect(createURL('myapp')).toBe('myapp://')
   })

packages/lynx-secure-storage/android/src/main/java/dev/amritk/minilynx/securestorage/MiniLynxSecureStorageModule.kt

@@ -172,7 +172,18 @@ class MiniLynxSecureStorageModule(context: LynxContext) : LynxContextModule(cont
    */
   @LynxMethod
   fun isSecureStorageAvailable(callback: Callback) {
-    STORAGE.execute { callback.invoke(SecureStore.isUsable(appContext)) }
+    STORAGE.execute {
+      // Guarded like every other method here, and it matters most on this one:
+      // it is what an app calls to find out whether the store works at all, so
+      // the state it has to survive is the store being as broken as it gets.
+      val usable =
+        try {
+          SecureStore.isUsable(appContext)
+        } catch (error: Throwable) {
+          false
+        }
+      callback.invoke(usable)
+    }
   }
 
   /**
@@ -184,13 +195,22 @@ class MiniLynxSecureStorageModule(context: LynxContext) : LynxContextModule(cont
    * because an exception escaping into the executor kills the thread this
    * package needs for every subsequent call, and leaves the caller's promise
    * unsettled with nothing to read.
+   *
+   * `Throwable` rather than `Exception`, which is the wider net on purpose. The
+   * failure this has to survive is a host that linked the module but shipped
+   * without `androidx.security.crypto` — R8 stripped it, or the dependency was
+   * never declared — and that arrives as a `NoClassDefFoundError`, which is an
+   * `Error` and not an `Exception`. Catching only the latter would let the one
+   * case this package most needs to report cleanly become the one case where
+   * the caller's promise never settles at all: not a failed read, but a screen
+   * that waits forever.
    */
   private fun answer(callback: Callback, operation: () -> JavaOnlyMap) {
     STORAGE.execute {
       val result =
         try {
           operation()
-        } catch (error: Exception) {
+        } catch (error: Throwable) {
           StorageResults.failure(
             StorageResults.KEYSTORE_FAILURE,
             "the encrypted store could not be used (${error.javaClass.simpleName})",

packages/mini-lynx-native/src/background/install-bridge.ts

@@ -101,9 +101,9 @@ export const installNativeBridge = (options: InstallBridgeOptions = {}): (() =>
    * is a version problem. They are fixed in different places.
    */
   const resolveMethod = (module: string, method: string): ((...args: unknown[]) => unknown) => {
-    const target = modules?.[module]
+    const target = lookupModule(module)
     if (!target) throw new Error(`the native module "${module}" is not linked into this app`)
-    const fn = target[method]
+    const fn = inherited(method) ? undefined : target[method]
     if (typeof fn !== 'function') {
       throw new Error(`the native module "${module}" has no method "${method}"`)
     }
@@ -112,11 +112,16 @@ export const installNativeBridge = (options: InstallBridgeOptions = {}): (() =>
 
   /** Answers an `exists` probe. Never throws: not-linked is the answer, not a failure. */
   const moduleExists = (module: string, method: string): boolean => {
-    const target = modules?.[module]
+    const target = lookupModule(module)
     if (!target) return false
-    return method === '' ? true : typeof target[method] === 'function'
+    if (method === '') return true
+    return !inherited(method) && typeof target[method] === 'function'
   }
 
+  /** The registry read both paths go through, so neither can forget the screen above. */
+  const lookupModule = (module: string): Readonly<Record<string, unknown>> | undefined =>
+    inherited(module) ? undefined : modules?.[module]
+
   const serveCall = (call: CallPayload): void => {
     const { id, module, method, args, form } = call
 
@@ -126,7 +131,12 @@ export const installNativeBridge = (options: InstallBridgeOptions = {}): (() =>
     }
 
     try {
-      const target = modules?.[module]
+      // Through the same screened lookup `resolveMethod` uses, rather than a
+      // second raw read of the registry. The raw one was safe only because
+      // `resolveMethod` throws on the line below before this value is ever
+      // applied — which is a guarantee that lives in the ORDER of two
+      // statements, and the next person to reorder them would not know that.
+      const target = lookupModule(module)
       const fn = resolveMethod(module, method)
 
       if (form === 'return') {
@@ -227,6 +237,23 @@ export const installNativeBridge = (options: InstallBridgeOptions = {}): (() =>
  * stringified rather than dropped: native bridges throw strings and plain
  * objects often enough that discarding them would lose the only clue.
  */
+/**
+ * Whether a name is one every object already answers to.
+ *
+ * A plain property read walks the prototype chain, so `NativeModules.constructor`
+ * is `Object` and `NativeModules.toString` is a function — which means a probe
+ * for a module nobody linked answers `true`, and a call for one gets `Object`
+ * applied to its arguments instead of an honest "not linked". The same is true
+ * of a method name on a module bag.
+ *
+ * The screen is on the NAME rather than an own-property test on the bag, and
+ * deliberately: the engine is free to hand modules or their methods over
+ * through a prototype or a proxy, and a `hasOwnProperty` check would call those
+ * unlinked. No native module or method is called `toString`, so screening the
+ * handful of names `Object.prototype` occupies costs nothing real.
+ */
+const inherited = (name: string): boolean => name in Object.prototype
+
 const describe = (error: unknown): string => {
   if (error instanceof Error) return error.message
   return typeof error === 'string' ? error : `${String(error)}`

packages/mini-lynx-native/src/channel.test.ts

@@ -191,4 +191,47 @@ describe('channel', () => {
 
     await expect(pending).rejects.toThrow(/reset before this call was answered/)
   })
+
+  it('never settles a fresh call with a reply left over from before a reset', async () => {
+    const contexts = createFakeContexts()
+    setPeerContext(contexts.mainThread)
+
+    // A method that keeps its callback rather than answering, so a reply can be
+    // made to arrive after the reset that rejected the call it belongs to.
+    const held: ((value: unknown) => void)[] = []
+    uninstall = installNativeBridge({
+      peer: contexts.background,
+      modules: { StorageModule: { hold: (done: (value: unknown) => void) => held.push(done) } },
+    })
+
+    const abandoned = callNativeAsync('StorageModule', 'hold')
+    await expect(
+      (async () => {
+        resetNativeChannel()
+        await abandoned
+      })(),
+    ).rejects.toThrow(/reset before this call was answered/)
+
+    setPeerContext(contexts.mainThread)
+    const fresh = callNativeAsync('StorageModule', 'hold')
+
+    // The abandoned call finally answers. It must not be mistaken for this one:
+    // ids restarting at 1 used to hand this result straight to `fresh`.
+    held[0]?.('stale')
+
+    const raced = await Promise.race([fresh, Promise.resolve('still pending')])
+    expect(raced).toBe('still pending')
+  })
+
+  it('does not mistake a name off Object.prototype for a linked module', async () => {
+    setup()
+
+    // `NativeModules.constructor` is `Object` and `NativeModules.toString` is a
+    // function, so a plain property read would call both of these linked.
+    await expect(isNativeModuleAvailable('constructor')).resolves.toBe(false)
+    await expect(isNativeModuleAvailable('toString')).resolves.toBe(false)
+    await expect(isNativeModuleAvailable('StorageModule', 'hasOwnProperty')).resolves.toBe(false)
+    await expect(callNative('constructor', 'call')).rejects.toThrow(/is not linked into this app/)
+    await expect(callNative('StorageModule', 'valueOf')).rejects.toThrow(/has no method "valueOf"/)
+  })
 })

packages/mini-lynx-native/src/channel.ts

@@ -67,6 +67,19 @@ export type NativeEventListener = (...args: readonly unknown[]) => void
 
 let attached = false
 let ready = false
+
+/**
+ * The correlation id for the next call, and it never goes back.
+ *
+ * `resetNativeChannel` deliberately does NOT reset this. A reset rejects the
+ * calls in flight, but it cannot cancel the native work already under way, and
+ * a method that answers late still sends its reply home under the id it was
+ * given. If ids restarted, that stale reply would land on whichever fresh call
+ * happened to be filed under the same number and resolve it with another
+ * call's result — a wrong answer rather than a missing one, which is far worse.
+ * Monotonic ids make the stale reply match nothing, so it is warned about and
+ * dropped, which is what it is.
+ */
 let nextId = 1
 
 const pending = new Map<number, Pending>()
@@ -244,7 +257,6 @@ export const resetNativeChannel = (): void => {
   }
   attached = false
   ready = false
-  nextId = 1
 
   const inflight = [...pending.values()]
   pending.clear()

packages/mini-lynx/src/forms/create-form.test.ts

@@ -126,6 +126,35 @@ describe('create-form', () => {
     expect(form.field('email').error()).toBeUndefined()
   })
 
+  it('ignores a second submit while the first is still in flight', async () => {
+    // There is no form element and no platform `disabled` here, so
+    // `handleSubmit` hangs off a `bindtap` with nothing between a second tap
+    // and a second `onSubmit` — which is where an order gets placed.
+    let calls = 0
+    let resolveSubmit: (() => void) | undefined
+    const form = createForm({
+      initialValues: { name: 'Ada' },
+      onSubmit: () => {
+        calls++
+        return new Promise<void>((resolve) => (resolveSubmit = resolve))
+      },
+    })
+
+    const first = form.handleSubmit()
+    await form.handleSubmit()
+    expect(calls).toBe(1)
+
+    resolveSubmit?.()
+    await first
+    expect(form.isSubmitting()).toBe(false)
+
+    // The guard lifts once the first one settles, so the form is still usable.
+    const second = form.handleSubmit()
+    expect(calls).toBe(2)
+    resolveSubmit?.()
+    await second
+  })
+
   it('restores everything on reset', async () => {
     const form = createForm({
       initialValues: { email: '' },

packages/mini-lynx/src/forms/create-form.ts

@@ -259,6 +259,13 @@ export const createForm = <V extends FieldValues>(config: FormConfig<V>): Form<V
     })
 
   const handleSubmit = async (): Promise<void> => {
+    // A submit already in flight owns the form until it settles. This matters
+    // more here than it does on the web: there is no form element and no
+    // platform `disabled`, so `handleSubmit` hangs off a `bindtap` and a
+    // `bindconfirm` with nothing between a second tap and a second `onSubmit`.
+    // That is where an order gets placed, so running it twice is the expensive
+    // kind of bug.
+    if (isSubmitting()) return
     batch(() => {
       submitted(true)
       submitError(undefined)

packages/mini-lynx/src/recycle/recycle.test.tsx

@@ -357,4 +357,24 @@ describe('recycle', () => {
     expect(engine.findAll('list-item')).toHaveLength(before)
     setErrorHandler(null)
   })
+
+  it('warns when two rows share an itemKey', () => {
+    // The inventory diff is keyed, so a repeated key makes it describe a list
+    // nobody has: rows report as moving on an update that changed nothing.
+    // Both `list()` implementations already warn on a key collision; this is
+    // the same mistake with a symptom that is far harder to trace back.
+    const warnings: string[] = []
+    const warn = console.warn
+    console.warn = (message: string) => warnings.push(message)
+    try {
+      const rows = signal<Row[]>([
+        { id: 'same', label: 'A' },
+        { id: 'same', label: 'B' },
+      ])
+      setup(rows)
+      expect(warnings.some((message) => message.includes('duplicate itemKey'))).toBe(true)
+    } finally {
+      console.warn = warn
+    }
+  })
 })