diff --git a/PARITY.md b/PARITY.md index b6edd1a..17f455f 100644 --- a/PARITY.md +++ b/PARITY.md @@ -339,7 +339,7 @@ the messenger rather than staying separate apps. | Files engine: names, key wrapping, chunked content | ✅ pinned to the **browser's** published vectors, not to the Swift engine | | Files engine: KDF and identity record | ✅ an identity written by the browser unlocks here | | Local vault: add, organise, delete | ✅ index, folders, both storage modes, bulk delete with its report | -| Files screen: set up, unlock, browse, add, folders, delete, promote | ✅ ⚠️ **not yet run on a device** — see below | +| Files screen: set up, unlock, browse, add, folders, delete, promote | ✅ **verified on a device** — set up, encrypt, lock, cold-start reload, link via SAF | | The security lattice (labels, clearance) | ✅ encoding and clearance in Kotlin, pinned to independently-computed vectors. ⚠️ **Not Bell–LaPadula** — the ★-property is deliberately unenforced on both clients; see android#15 | | Share a file into a chat or group | ✅ both directions — send from the vault, save a received one back at the label it arrived with | | Wallet engine: BIP-39, Base58, SLIP-0010 | ✅ pinned to the published vectors | @@ -348,13 +348,40 @@ the messenger rather than staying separate apps. | Wallet: fresh receive address per receive | ✅ counter in the vault; the screen says why the address changes | | Wallet: balance and transactions from a node | ✅ both, with **unread drawn as an em dash and never as zero** | -⚠️ **The Files screen compiles and every layer under it is tested, but it has -not been exercised on a device or emulator.** The machine was carrying an iOS -simulator at 99% CPU when it landed, and booting an emulator on top of that -produces a flaky run rather than evidence. Two things specifically need a real -device before this can be called done: the Storage Access Framework picker and -its persistable read grant, and whether a reopened app can still read a linked -file after a reboot. Neither can be faked on the JVM. +🔴 **The device run found a bug no JVM test could have.** 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 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 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. +- `bookmark` produced a reference pointing at `"null"`, permanently missing. + +Absence is now decided by `JSONObject.isNull`, never by comparing a decoded +string to a sentinel, and the literal `"null"` is rejected too. Two regression +tests reproduce the device's behaviour on the JVM by writing the literal string +the platform hands over — feeding a real JSON `null` would pass with the broken +code and prove nothing. + +⚠️ **Worth a sweep.** `optString` on a nullable field is wrong everywhere in +this app, not only here, and it fails only on the platform it ships to. + +**What the device run covered** (emulator, API 35): set-up with the passphrase, +a wrong passphrase refused, the SAF `OpenDocument` picker with the display name +pre-filled, encrypting into the vault at a chosen classification, lock and +unlock, and a cold-start reload of the vault. On disk: the blob begins `ENCL\x01`, +both wrapped keys carry version `0x11` and the name `0x01`, the filename is not +in the clear, and the plaintext is absent from the entire app container. Linking +a file works and reports "Nothing was copied". + +⚠️ **Still owed: whether a persistable read grant survives a reboot.** The +emulator reboot ran at a machine load of 38 and the launcher itself ANR'd, which +is not a condition that produces evidence about anything. 🔴 **Android asks for a Files passphrase, exactly as iOS does, and this was a decision rather than a port.** The Android wallet sits behind the Keystore-sealed diff --git a/client/src/main/kotlin/io/flamenet/client/LocalVault.kt b/client/src/main/kotlin/io/flamenet/client/LocalVault.kt index c9094e4..41044b8 100644 --- a/client/src/main/kotlin/io/flamenet/client/LocalVault.kt +++ b/client/src/main/kotlin/io/flamenet/client/LocalVault.kt @@ -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"), ) } diff --git a/client/src/test/kotlin/io/flamenet/client/LocalVaultTest.kt b/client/src/test/kotlin/io/flamenet/client/LocalVaultTest.kt index 7f5a9f2..c7d54e3 100644 --- a/client/src/test/kotlin/io/flamenet/client/LocalVaultTest.kt +++ b/client/src/test/kotlin/io/flamenet/client/LocalVaultTest.kt @@ -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 { reopened.contents(back) } + } + @Test fun `an empty directory loads as an empty vault rather than failing`() { val fresh = File(root, "elsewhere").also { it.mkdirs() }