mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 19:28:25 +00:00
fix(browser): close five holes found auditing the file-input path
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FxfdHeR9Ry4qALXHT5Sf1Q
This commit is contained in:
+20
-3
@@ -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<String>?) {
|
||||
if (reported) return
|
||||
reported = true
|
||||
|
||||
+20
-4
@@ -74,10 +74,9 @@ object FileChooserAccept {
|
||||
|
||||
val mimes = mutableListOf<String>()
|
||||
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<CaptureMedia>()
|
||||
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
|
||||
|
||||
+10
@@ -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
|
||||
|
||||
+1
@@ -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
|
||||
|
||||
+6
-6
@@ -62,10 +62,12 @@ object NappletCaptureFiles {
|
||||
): Pair<File, Uri>? =
|
||||
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
|
||||
}
|
||||
|
||||
+1
@@ -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
|
||||
|
||||
+17
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user