Fix: the vault index failed to load on a real phone

🔴 **Android's bundled `org.json` coerces a JSON `null` to the four-character string `"null"`. The Maven `org.json` the tests run against returns `""`.** Every nullable field in the vault index therefore passed on a laptop and broke on a device: - `parentID` threw `Invalid UUID string: null`, so a vault holding any top-level file failed to load AT ALL. Adding the first file worked, because it was still in memory; reopening the app lost everything, with an error nobody could act on. - `wrappedContentKey` was worse, because it did NOT throw. `"null"` is valid base64, so the node came back with a plausible three-byte content key and simply refused to decrypt, for ever, with nothing to explain it. - `bookmark` produced a reference pointing at `"null"`, permanently missing. Absence is now decided by `JSONObject.isNull` and never by comparing a decoded string to a sentinel. The literal `"null"` is rejected as well: it is not a valid value for any of these fields, and tolerating it costs nothing next to reintroducing this. ⚠️ **The two regression tests write the literal string `"null"` into the index on purpose.** That is exactly what the decoder is handed on a device, so it is what makes a JVM run reproduce the device's behaviour. Feeding a real JSON `null` — which is what the vault actually writes — passes with the broken code and proves nothing. Both were confirmed by restoring the old decoding and watching them go red. Found by running the app (android!52). No JVM test could have caught it, which is an argument for the device run rather than against the tests. ⚠️ `optString` on a nullable field is wrong everywhere in this app, not only here, and it fails only on the platform it ships to. Worth a sweep. Verified on the emulator afterwards: the vault survives a cold start, and on disk the blob begins `ENCL\x01`, both wrapped keys carry version 0x11, the name 0x01, the filename is not in the clear, and the plaintext is absent from the whole app container. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-04 16:08:19 -04:00
parent 6ec626cde2
commit 436ce240c7
3 changed files with 128 additions and 13 deletions
@@ -276,23 +276,54 @@ class LocalVault(
        .put("storage", node.storage.wire)
        .put("bookmark", node.bookmark ?: JSONObject.NULL)

+    /**
+     * 🔴 **`optString` is not safe on a nullable field, and the JVM will not
+     * tell you.** Android's bundled `org.json` coerces a JSON `null` to the
+     * four-character string `"null"`; the Maven `org.json` these tests run
+     * against returns `""`. Every nullable field below therefore passed on a
+     * laptop and broke on a phone:
+     *
+     * - `parentID` threw `Invalid UUID string: null` — so a vault holding any
+     *   top-level file failed to load AT ALL. Adding the first file worked,
+     *   because it was already in memory; reopening the app lost everything,
+     *   with an error nobody could act on.
+     * - `wrappedContentKey` was worse, because it did NOT throw: `"null"` is
+     *   valid base64, so the node came back with a plausible three-byte content
+     *   key and simply refused to decrypt.
+     * - `bookmark` produced a reference pointing at `"null"`, permanently
+     *   missing.
+     *
+     * ⚠️ So absence is decided by [JSONObject.isNull], never by comparing a
+     * decoded string to a sentinel. The literal `"null"` is rejected as well:
+     * it is not a valid value for any of these fields, and tolerating it costs
+     * nothing next to reintroducing this.
+     *
+     * Found by running the app on a device (android!52). No JVM test could have
+     * caught it, which is the argument for the device run, not against the
+     * tests.
+     */
+    private fun optional(o: JSONObject, key: String): String? {
+        if (o.isNull(key)) return null
+        val value = o.optString(key, "")
+        return value.takeIf { it.isNotEmpty() && it != "null" }
+    }
+
    private fun decode(o: JSONObject): Node {
        val bits = o.optJSONArray("compartments") ?: JSONArray()
        return Node(
            id = UUID.fromString(o.getString("id")),
-            parentId = o.optString("parentID", "").takeIf { it.isNotEmpty() }?.let(UUID::fromString),
+            parentId = optional(o, "parentID")?.let(UUID::fromString),
            isFolder = o.optBoolean("isFolder", false),
            classification = o.getInt("classification").toShort(),
            compartments = (0 until bits.length()).map { bits.getInt(it) },
            wrappedNodeKey = unb64(o.getString("wrappedNodeKey")),
            nameDigest = unb64(o.getString("nameDigest")),
            wrappedName = unb64(o.getString("wrappedName")),
-            wrappedContentKey = o.optString("wrappedContentKey", "")
-                .takeIf { it.isNotEmpty() }?.let(::unb64),
+            wrappedContentKey = optional(o, "wrappedContentKey")?.let(::unb64),
            byteCount = o.optInt("byteCount", 0),
            addedAtMillis = o.optLong("addedAt", 0L),
-            storage = Storage.parse(o.optString("storage", "").takeIf { it.isNotEmpty() }),
-            bookmark = o.optString("bookmark", "").takeIf { it.isNotEmpty() },
+            storage = Storage.parse(optional(o, "storage")),
+            bookmark = optional(o, "bookmark"),
        )
    }

@@ -160,6 +160,63 @@ class LocalVaultTest {
        assertEquals(LocalVault.Storage.VAULT, reopen().list(null).single().storage)
    }

+    /**
+     * 🔴 **The bug the JVM could not see.** Android's bundled `org.json`
+     * coerces a JSON `null` to the four-character string `"null"`; the Maven
+     * `org.json` this suite runs against returns `""`. Every nullable field in
+     * the index therefore passed here and broke on a phone — a vault with any
+     * top-level file threw `Invalid UUID string: null` on load, so adding the
+     * first file worked and reopening the app lost everything.
+     *
+     * ⚠️ **This test writes the literal string `"null"` on purpose.** That is
+     * exactly what the decoder is handed on a device, so putting it in the JSON
+     * here is what makes the JVM run reproduce the device's behaviour. Feeding
+     * a real JSON `null` — which is what the vault actually writes — would pass
+     * with the broken code and prove nothing.
+     */
+    @Test
+    fun `a nullable field decodes as absent however the platform hands it over`() {
+        val node = vault.add("x".toByteArray(), "top.txt", secret)
+        val reference = File(root, "index.json").readText()
+
+        for (shape in listOf(
+            reference.replace("\"parentID\":null", "\"parentID\":\"null\""),
+            reference.replace("\"parentID\":null", ""). // key missing entirely
+                replace(",,", ","),
+            reference,
+        )) {
+            File(root, "index.json").writeText(shape)
+            val reopened = reopen()
+            assertEquals(1, reopened.count, "the index failed to load: $shape")
+            assertNull(assertNotNull(reopened.node(node.id)).parentId)
+            assertEquals("top.txt", reopened.list(null).single().name)
+        }
+    }
+
+    /**
+     * 🔴 The one that would NOT have thrown. `"null"` is valid base64, so a
+     * content key decoded from it is a plausible three bytes — and the file
+     * simply refuses to decrypt, for ever, with nothing to explain why.
+     */
+    @Test
+    fun `a bookmark or content key of the literal string null is treated as absent`() {
+        val node = vault.add("x".toByteArray(), "f.txt", secret)
+        val index = File(root, "index.json")
+        index.writeText(
+            index.readText()
+                .replace("\"bookmark\":null", "\"bookmark\":\"null\"")
+                .replace(Regex("\"wrappedContentKey\":\"[^\"]*\""),
+                         "\"wrappedContentKey\":\"null\"")
+        )
+        val reopened = reopen()
+        val back = assertNotNull(reopened.node(node.id))
+        assertNull(back.bookmark)
+        assertNull(back.wrappedContentKey,
+            "a three-byte content key decoded from the string \"null\" was accepted")
+        // And the failure is one somebody can act on, not a decryption error.
+        assertFailsWith<LocalVault.VaultException> { reopened.contents(back) }
+    }
+
    @Test
    fun `an empty directory loads as an empty vault rather than failing`() {
        val fresh = File(root, "elsewhere").also { it.mkdirs() }