ReviewOS

also looking at this

craft-native/craft

fix(android): an apostrophe in a reply hangs the page forever

#156
Merged glennmichael123 wants to merge fix/android-js-escaping into main
4 files +279 -73
9 days ago

Closes #154.

The bug

Fifty-eight emissions in CraftBridge.kt interpolated a Kotlin value straight into a single-quoted JavaScript string:

"window._craftDeleteEventReject && window._craftDeleteEventReject('${e.message}')"

An apostrophe in the value ends the string early, the script does not parse, and evaluateJavascript runs nothing — not the reject, not the resolve. These promises are hand-built rather than made by _createCallback, so nothing times out behind them. The page waits forever with no console error, because the failure happens inside evaluateJavascript where nothing is watching.

It is reachable from the page, because Java puts the offending input in the exception:

craft.calendar.delete("1'x")
  → NumberFormatException: For input string: "1'x"
  → ...Reject('For input string: "1'x"')
                                    ^ string ends here

dbExecute and dbQuery echo the caller's SQL the same way; addContact and createCalendarEvent echo field values.

Not primarily a security issue: the script runs in the page's own context and the page can already call eval. The cost is the hang.

What changed beyond the issue

The issue counted 21 reject paths. It is 58, because the resolve paths carry the same shape and the same caller-supplied text — _craftSharedKeychainResolve echoes both the key and the value, _craftVoiceResolve the phrase, _craftSaveResolve an absolute path. All 58 are converted.

jsQuote wraps JSONObject.quote, which this file already used correctly in exactly one place (registerPush). A Kotlin null still renders as the string "null", matching what plain interpolation produced — whether it should be a JS null is a separate question from escaping and is left alone.

The guard

A mechanical fix is exactly the kind that comes undone: the next reply path written by hand will look like the 58 before it, read naturally, compile, and work until someone types an apostrophe. android_reply_escaping_test.zig scans every Kotlin template for the two characters '$ — narrower than "escape your JavaScript", trivially decidable, and with no false positives in this tree.

The scanner is handed a known-bad input before it is trusted with the real files, and the guard was mutation-tested:

error: 'no Kotlin template interpolates a value into a single-quoted JavaScript string' failed:
       CraftBridge.kt.template:2420 interpolates a value into a single-quoted JavaScript string:

           webView.evaluateJavascript("window._craftNotifReject && window._craftNotifReject('${e.message}')", null)

One thing that did not work

Generating the template list by reading the directory in build.zig was tried and removed. zig build caches that script's evaluation against its own contents, so adding a template did not regenerate the list — the completeness check passed by reading stale data, which is the exact failure it existed to prevent. The list is hand-written now and the comment says so rather than implying coverage it does not have.

Testing

zig build test:android passes. The full zig build test has one pre-existing failure unrelated to this change: injected_js_test cannot compile against ~/Documents/Libraries/zig-js, whose sources predate this toolchain's std.ArrayListUnmanaged and getLast changes.

4 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.