From 6becc6efbe26ad48496192c2dcca783c239aed9b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 23:25:22 +0000 Subject: [PATCH] fix(browser): close five holes found auditing the file-input path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-audit of the picker and camera work. Four correctness bugs and one robustness gap, none of them reachable by the happy path, all of them reachable. A malformed accept entry became the picker's filter. `accept="image/"` passed the "contains a slash, so it is a MIME type" test and went straight into Intent.setType, where it matches no provider — an empty picker with nothing to choose and no way out. A slashed token is now only a MIME type when both halves are actually present; otherwise it is unnameable and widens to everything, the same as an unknown extension. Test first, watched it fail. The main-process chooser host never reported when the system destroyed it without finish() — a low-memory kill while the picker is on top. The page's file input would then wait forever on a result nobody was left to send (dead for the life of the page), and the coordinator would hold the reply callback, and the controller behind it, for good. Reporting from onDestroy covers it. A recreated host now releases the input immediately too, instead of silently swallowing a pick it can no longer route. A second file input asking before the first pick returned overwrote the in-flight request. The page's own callback was already released, but the superseded request still owned camera scratch files and the URI grants handed to every camera app — nothing would ever come back for them, so they sat until the daily sweep. Superseding now runs the cancel path on the old request, and the same cleanup runs when a host is torn down mid-pick. Capture filenames were built from a clock and a per-object sequence. The main and `:napplet` processes each hold their own copy of that object, so the sequences run independently and two picks started in the same millisecond could name the same file, one capture silently overwriting the other. createTempFile removes the question. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FxfdHeR9Ry4qALXHT5Sf1Q --- .../napplet/WebFileChooserActivity.kt | 23 +++++++++++++++--- .../commons/browser/FileChooserAccept.kt | 24 +++++++++++++++---- .../commons/browser/FileChooserAcceptTest.kt | 10 ++++++++ .../napplethost/NappletBrowserActivity.kt | 1 + .../napplethost/NappletCaptureFiles.kt | 12 +++++----- .../napplethost/NappletHostActivity.kt | 1 + .../napplethost/WebFileChooserLauncher.kt | 17 +++++++++++++ 7 files changed, 75 insertions(+), 13 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/WebFileChooserActivity.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/WebFileChooserActivity.kt index 58172c35fb..c49ddda0c2 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/WebFileChooserActivity.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/napplet/WebFileChooserActivity.kt @@ -58,9 +58,14 @@ class WebFileChooserActivity : ComponentActivity() { return } - // A recreated instance (rotation) already has its pick in flight; launching again would stack a - // second picker on top of the first. - if (savedInstanceState != null) return + // A recreated instance has lost the in-flight request that its result would be matched against + // (configChanges keeps this rare — a system kill, not a rotation), and relaunching would stack a + // second picker on the first. Release the page's input now rather than let it wait on a result + // that can no longer be routed anywhere. + if (savedInstanceState != null) { + finish() + return + } chooser.launch( acceptTypes = ask.acceptTypes, @@ -76,6 +81,18 @@ class WebFileChooserActivity : ComponentActivity() { super.finish() } + /** + * The system can destroy this host without ever calling [finish] — a low-memory kill while the + * picker is on top. Without this the page's file input would wait on a result nobody is left to + * send, dead for the life of the page, and the coordinator would hold the reply callback (and the + * controller behind it) forever. + */ + override fun onDestroy() { + chooser.teardown() + report(null) + super.onDestroy() + } + private fun report(uris: Array?) { if (reported) return reported = true diff --git a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAccept.kt b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAccept.kt index a96fe90a9c..627058f992 100644 --- a/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAccept.kt +++ b/commons/src/commonMain/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAccept.kt @@ -74,10 +74,9 @@ object FileChooserAccept { val mimes = mutableListOf() for (token in tokens) { - val mime = if (token.contains('/')) token else extensionToMime(token.removePrefix(".")) // One name we can't translate means we cannot express this page's filter faithfully. Show // everything rather than a filter that silently excludes part of what it asked for. - if (mime.isNullOrEmpty()) return Resolved(ANY, emptyList()) + val mime = mimeOf(token, extensionToMime) ?: return Resolved(ANY, emptyList()) if (mime !in mimes) mimes.add(mime) } @@ -115,8 +114,7 @@ object FileChooserAccept { val media = mutableSetOf() for (token in tokens) { if (token == ANY) return setOf(CaptureMedia.IMAGE, CaptureMedia.VIDEO) - val mime = if (token.contains('/')) token else extensionToMime(token.removePrefix(".")) - when (mime?.substringBefore('/')) { + when (mimeOf(token, extensionToMime)?.substringBefore('/')) { "image" -> media.add(CaptureMedia.IMAGE) "video" -> media.add(CaptureMedia.VIDEO) } @@ -124,6 +122,24 @@ object FileChooserAccept { return media } + /** + * One `accept` entry as a MIME type, or null when it cannot be turned into one. + * + * A token with a slash is taken as a MIME type, but only when both halves are actually there: a + * page that writes `accept="image/"` would otherwise put that straight into `Intent.setType`, where + * it matches no provider and leaves the user staring at an empty picker with no way out. Everything + * else is an extension, with or without its leading dot. + */ + private fun mimeOf( + token: String, + extensionToMime: (String) -> String?, + ): String? { + if (!token.contains('/')) return extensionToMime(token.removePrefix(".")).takeIf { !it.isNullOrEmpty() } + val type = token.substringBefore('/') + val subtype = token.substringAfter('/') + return token.takeIf { type.isNotEmpty() && subtype.isNotEmpty() && !subtype.contains('/') } + } + /** * The narrowest single type covering [mimes]: the type itself when there is only one, the shared * family's wildcard when they all belong to one family, and [ANY] when they span families (or the diff --git a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAcceptTest.kt b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAcceptTest.kt index 613a256524..0e7daea130 100644 --- a/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAcceptTest.kt +++ b/commons/src/commonTest/kotlin/com/vitorpamplona/amethyst/commons/browser/FileChooserAcceptTest.kt @@ -126,6 +126,16 @@ class FileChooserAcceptTest { assertEquals(listOf("image/jpeg"), resolved.mimeTypes) } + @Test + fun malformedMimeTokenDoesNotBecomeTheFilter() { + // A half-written MIME would go straight into Intent.setType and match no provider at all, so the + // user gets an empty picker with no way out. Treat it as unnameable and show everything. + assertEquals(FileChooserAccept.ANY, resolve("image/").primaryType) + assertEquals(FileChooserAccept.ANY, resolve("/png").primaryType) + assertEquals(FileChooserAccept.ANY, resolve("/").primaryType) + assertEquals(emptyList(), resolve("image/").mimeTypes) + } + private fun capture(vararg accept: String) = FileChooserAccept.captureMedia(accept.toList(), map::get) @Test diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt index 9cc171e9e4..0446275389 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletBrowserActivity.kt @@ -302,6 +302,7 @@ class NappletBrowserActivity : ComponentActivity() { runCatching { unbindService(brokerConnection) } // A picker still up when the browser is torn down would otherwise leave its callback unanswered. pendingFileChooser.cancel() + fileChooserLauncher.teardown() if (this::webView.isInitialized) { // Detach from the view tree BEFORE destroy(). Destroying a WebView while it is still attached to // the window corrupts the SHARED multiprocess renderer/network state, which then breaks the OTHER diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletCaptureFiles.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletCaptureFiles.kt index 5f55d0eaad..f8d7183c2f 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletCaptureFiles.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletCaptureFiles.kt @@ -62,10 +62,12 @@ object NappletCaptureFiles { ): Pair? = runCatching { val dir = File(context.cacheDir, DIR).apply { mkdirs() } - // Not createTempFile's random name: a stable, sortable prefix makes the sweep below able to - // recognise our own files and nothing else. - val file = File(dir, "capture-${System.currentTimeMillis()}-${counter++}.$extension") - file.createNewFile() + // createTempFile, not a name built from a clock and a per-object sequence: the main and + // `:napplet` processes each hold their OWN copy of this object, so those sequences run + // independently and two picks started in the same millisecond could name the same file — + // one capture would then silently overwrite the other. The extension is what FileProvider + // types the URI from, so it has to survive into the suffix. + val file = File.createTempFile("capture-", ".$extension", dir) file to FileProvider.getUriForFile(context, authority(context), file) }.onFailure { Log.w(TAG, "Could not create a capture file", it) } .getOrNull() @@ -132,6 +134,4 @@ object NappletCaptureFiles { } private const val GRANT_FLAGS = Intent.FLAG_GRANT_WRITE_URI_PERMISSION or Intent.FLAG_GRANT_READ_URI_PERMISSION - - private var counter = 0 } diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt index 7abf81805b..c933bee4a3 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/NappletHostActivity.kt @@ -461,6 +461,7 @@ class NappletHostActivity : ComponentActivity() { keyActions.clear() // A picker still up when the applet is torn down would otherwise leave its callback unanswered. pendingFileChooser.cancel() + fileChooserLauncher.teardown() if (this::webView.isInitialized) { // Detach before destroy(): destroying an attached WebView corrupts the shared multiprocess // renderer/network state and breaks the other (embedded) WebViews in this `:napplet` process diff --git a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/WebFileChooserLauncher.kt b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/WebFileChooserLauncher.kt index 9cd4ecac21..4e1d1de74a 100644 --- a/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/WebFileChooserLauncher.kt +++ b/nappletHost/src/main/kotlin/com/vitorpamplona/amethyst/napplethost/WebFileChooserLauncher.kt @@ -98,6 +98,10 @@ class WebFileChooserLauncher( private fun open(cameraAllowed: Boolean) { val pending = ask ?: return onResult(null) ask = null + // A second file input can ask before the first pick returns. The page's own callback is already + // released by PendingFileChooser, but the superseded request still owns scratch files and camera + // grants that nothing else would ever come back for. + abandonInFlight() val built = NappletFileChooser.buildRequest( @@ -121,6 +125,19 @@ class WebFileChooserLauncher( } } + /** Releases the scratch files and camera grants of a request whose result will never be read. */ + private fun abandonInFlight() { + val stale = request ?: return + request = null + NappletFileChooser.parseResult(activity, stale, Activity.RESULT_CANCELED, null) + } + + /** + * Called when the host is going away with a pick still open, so the request's scratch files and + * camera grants are not left behind for the stale sweep to find a day later. + */ + fun teardown() = abandonInFlight() + private fun hasCameraPermission() = ContextCompat.checkSelfPermission(activity, Manifest.permission.CAMERA) == PackageManager.PERMISSION_GRANTED private companion object {