API-style comments and ktlint formatting
Every comment is reduced to its first sentence plus @param, @return and @throws tags. Comment lines: 20040 to 8526. ktlint 1.8 applied with its default (official Kotlin) style across all modules; 262 files reformatted. The 264 remaining ktlint findings are naming and filename rules, which are code changes, and are left alone. Comments and formatting only. 1208 tests pass, counted from the XML. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
8a7f40fb13
commit
2fa430f21c
@@ -12,23 +12,36 @@ import kotlin.test.assertNull
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
class VaultTest {
|
||||
|
||||
/** Stands in for a directory on a phone, and lets a test see the raw bytes. */
|
||||
private class MapStorage : Vault.Storage {
|
||||
val records = LinkedHashMap<String, ByteArray>()
|
||||
|
||||
override fun read(name: String) = records[name]
|
||||
override fun write(name: String, bytes: ByteArray) { records[name] = bytes }
|
||||
override fun delete(name: String) { records.remove(name) }
|
||||
|
||||
override fun write(
|
||||
name: String,
|
||||
bytes: ByteArray,
|
||||
) {
|
||||
records[name] = bytes
|
||||
}
|
||||
|
||||
override fun delete(name: String) {
|
||||
records.remove(name)
|
||||
}
|
||||
|
||||
override fun names() = records.keys.toList()
|
||||
}
|
||||
|
||||
private fun key(): SecretKey =
|
||||
KeyGenerator.getInstance("AES").apply { init(256) }.generateKey()
|
||||
private fun key(): SecretKey = KeyGenerator.getInstance("AES").apply { init(256) }.generateKey()
|
||||
|
||||
private fun bundleFor(identity: DeviceIdentity): PeerBundle {
|
||||
val oneTime = identity.generateOneTimePreKeys(1).first()
|
||||
return PeerBundle.fromWire(identity.registrationWire().put("opk",
|
||||
JSONObject().put("id", oneTime.id).put("pub", Bytes.b64(oneTime.publicKey))))
|
||||
return PeerBundle.fromWire(
|
||||
identity.registrationWire().put(
|
||||
"opk",
|
||||
JSONObject().put("id", oneTime.id).put("pub", Bytes.b64(oneTime.publicKey)),
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -71,16 +84,6 @@ class VaultTest {
|
||||
|
||||
/**
|
||||
* The chain a re-establishment replaces must survive it.
|
||||
*
|
||||
* Both sides open the chat at once, so each runs X3DH before seeing the
|
||||
* other's prekey and each adopts the other's chain to read what arrived.
|
||||
* From then on the peer sends on the chain being replaced. A client that
|
||||
* stored only the newest session could not open any of it, silently,
|
||||
* because an envelope that will not decrypt is skipped, not reported.
|
||||
*
|
||||
* This is the storage half of the fix. `E2ESessionTest` covers the engine
|
||||
* primitives, and passes with or without it, because a test holding both
|
||||
* sessions in local variables never exercises what a client keeps.
|
||||
*/
|
||||
@Test
|
||||
fun `a retired session is kept and still opens its chain`() {
|
||||
@@ -111,27 +114,20 @@ class VaultTest {
|
||||
val reply = onTheRetiredChain.encrypt("bob, this is the old chain")
|
||||
|
||||
val fresh = Vault(storage, secret)
|
||||
assertNull(runCatching { fresh.loadSession(bob.deviceId)!!.decrypt(reply) }.getOrNull(),
|
||||
assertNull(
|
||||
runCatching { fresh.loadSession(bob.deviceId)!!.decrypt(reply) }.getOrNull(),
|
||||
"the adopted session must NOT open the retired chain, otherwise this " +
|
||||
"test would pass without the retired copy existing at all")
|
||||
assertEquals("bob, this is the old chain",
|
||||
fresh.loadRetiredSession(bob.deviceId)!!.decrypt(reply))
|
||||
"test would pass without the retired copy existing at all",
|
||||
)
|
||||
assertEquals(
|
||||
"bob, this is the old chain",
|
||||
fresh.loadRetiredSession(bob.deviceId)!!.decrypt(reply),
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* A failed decrypt damages the session object it was handed, so the
|
||||
* thing that gets retired must be re-read from the vault.
|
||||
*
|
||||
* This is the bug the first version of the retirement fix shipped with, and
|
||||
* the reason `a retired session is kept and still opens its chain` did not
|
||||
* catch it: that test retires a PRISTINE session, which is not what the
|
||||
* receive path has in its hand. The real path has just proved the peer is on
|
||||
* another chain, by handing this session an envelope it could not open,
|
||||
* and `E2ESession` is a class, so that attempt consumed skipped keys and
|
||||
* stepped the chain. Retiring THAT instance stores something that can no
|
||||
* longer open anything, which looks exactly like not retiring at all.
|
||||
*
|
||||
* Asserts both halves: the mutated instance fails, the re-read one works.
|
||||
* A failed decrypt damages the session object it was handed, so the thing
|
||||
* that gets retired must be re-read from the vault.
|
||||
*/
|
||||
@Test
|
||||
fun `the session retired after a failed decrypt must be re-read, not reused`() {
|
||||
@@ -140,40 +136,49 @@ class VaultTest {
|
||||
val alice = DeviceIdentity.create()
|
||||
val bob = DeviceIdentity.create()
|
||||
|
||||
// Bob's own chain, stored, with a message Alice will later answer on it.
|
||||
// Bob's own chain, stored, with a message Alice will later answer on
|
||||
// it.
|
||||
val bobsOwn = E2ESession.initiate(bob, bundleFor(alice))
|
||||
val bobsFirst = bobsOwn.encrypt("from bob")
|
||||
Vault(storage, secret).saveSession(bob.deviceId, bobsOwn)
|
||||
|
||||
val alicesReplyOnBobsChain = E2ESession.respond(alice, bobsFirst).let {
|
||||
it.decrypt(bobsFirst)
|
||||
it.encrypt("answering on the chain bob started")
|
||||
}
|
||||
val alicesReplyOnBobsChain =
|
||||
E2ESession.respond(alice, bobsFirst).let {
|
||||
it.decrypt(bobsFirst)
|
||||
it.encrypt("answering on the chain bob started")
|
||||
}
|
||||
|
||||
// The receive path: Bob's stored session is handed a foreign envelope
|
||||
// and fails. THIS is the instance the buggy version retired.
|
||||
// and fails.
|
||||
val foreign = E2ESession.initiate(alice, bundleFor(bob)).encrypt("different chain")
|
||||
val damaged = Vault(storage, secret).loadSession(bob.deviceId)!!
|
||||
assertNull(runCatching { damaged.decrypt(foreign) }.getOrNull(),
|
||||
"the foreign envelope must NOT open, or this test proves nothing")
|
||||
assertNull(
|
||||
runCatching { damaged.decrypt(foreign) }.getOrNull(),
|
||||
"the foreign envelope must NOT open, or this test proves nothing",
|
||||
)
|
||||
|
||||
// Retiring the damaged instance loses the chain.
|
||||
val wrong = MapStorage().also { storage.records.forEach(it.records::put) }
|
||||
Vault(wrong, secret).saveRetiredSession(bob.deviceId, damaged)
|
||||
assertNull(
|
||||
runCatching {
|
||||
Vault(wrong, secret).loadRetiredSession(bob.deviceId)!!
|
||||
Vault(wrong, secret)
|
||||
.loadRetiredSession(bob.deviceId)!!
|
||||
.decrypt(alicesReplyOnBobsChain)
|
||||
}.getOrNull(),
|
||||
"a session damaged by a failed decrypt should NOT still open its chain, " +
|
||||
"if this ever starts passing, the hazard is gone and this test can go")
|
||||
"if this ever starts passing, the hazard is gone and this test can go",
|
||||
)
|
||||
|
||||
// Re-reading the vault copy keeps it. This is the fix.
|
||||
// Re-reading the vault copy keeps it.
|
||||
val pristine = Vault(storage, secret).loadSession(bob.deviceId)!!
|
||||
Vault(storage, secret).saveRetiredSession(bob.deviceId, pristine)
|
||||
assertEquals("answering on the chain bob started",
|
||||
Vault(storage, secret).loadRetiredSession(bob.deviceId)!!
|
||||
.decrypt(alicesReplyOnBobsChain))
|
||||
assertEquals(
|
||||
"answering on the chain bob started",
|
||||
Vault(storage, secret)
|
||||
.loadRetiredSession(bob.deviceId)!!
|
||||
.decrypt(alicesReplyOnBobsChain),
|
||||
)
|
||||
}
|
||||
|
||||
/** Signing out must take the retired chains too, they hold chain keys. */
|
||||
@@ -192,8 +197,10 @@ class VaultTest {
|
||||
|
||||
assertNull(Vault(storage, secret).loadSession(bob.deviceId))
|
||||
assertNull(Vault(storage, secret).loadRetiredSession(bob.deviceId))
|
||||
assertTrue(storage.records.keys.none { it.contains("session") },
|
||||
"a record still names a session: ${storage.records.keys}")
|
||||
assertTrue(
|
||||
storage.records.keys.none { it.contains("session") },
|
||||
"a record still names a session: ${storage.records.keys}",
|
||||
)
|
||||
}
|
||||
|
||||
/** What is on disk must not be readable, or none of this was worth doing. */
|
||||
@@ -210,10 +217,7 @@ class VaultTest {
|
||||
}
|
||||
|
||||
/**
|
||||
* The failure this catches: a stored blob moved from one slot to
|
||||
* another. Without the name in the AAD, a peer's session pasted into a
|
||||
* different peer's slot decrypts perfectly and the app attributes their
|
||||
* messages to the wrong person.
|
||||
* The failure this catches: a stored blob moved from one slot to another.
|
||||
*/
|
||||
@Test
|
||||
fun `a record moved to another slot does not open`() {
|
||||
@@ -251,10 +255,7 @@ class VaultTest {
|
||||
}
|
||||
|
||||
/**
|
||||
* A corrupt identity must NOT read as "no identity". The caller treats
|
||||
* null as a new install and mints a fresh device, which every contact sees
|
||||
* as "security keys changed", the warning that is supposed to mean an
|
||||
* attack. Absent and unreadable are different answers.
|
||||
* A corrupt identity must NOT read as "no identity".
|
||||
*/
|
||||
@Test
|
||||
fun `an unreadable identity is not mistaken for a missing one`() {
|
||||
@@ -305,8 +306,7 @@ class VaultTest {
|
||||
|
||||
@Test
|
||||
fun `two writes of the same value do not produce the same bytes`() {
|
||||
// A fresh nonce per write. Repeating one under the same key is the
|
||||
// catastrophic AES-GCM failure, not a degraded one.
|
||||
// A fresh nonce per write.
|
||||
val storage = MapStorage()
|
||||
val secret = key()
|
||||
val identity = DeviceIdentity.create()
|
||||
@@ -316,23 +316,10 @@ class VaultTest {
|
||||
assertNotEquals(Bytes.b64(first), Bytes.b64(storage.records.getValue("identity")))
|
||||
}
|
||||
|
||||
// ---- Trust records and the platform's JSON ---------------------------
|
||||
// ---- Trust records and the platform's JSON ---------------------------.
|
||||
|
||||
/**
|
||||
* A contact with no known address must come back with NO address.
|
||||
*
|
||||
* `saveTrust` used to write `"fnid": null` explicitly, and Android's
|
||||
* bundled `org.json` reads a JSON null back as the four-character string
|
||||
* `"null"`, a non-null value that is not an address. `Trust.pin` compares a
|
||||
* newly offered fnid against the pinned one and treats a difference as a
|
||||
* different person, so the first time a real address arrived the contact was
|
||||
* marked CHANGED: the "security keys changed" warning, which is meant to
|
||||
* mean an attack, fired on an ordinary first sighting.
|
||||
*
|
||||
* The JVM's `org.json` returns `""` there, so this passed on a laptop and
|
||||
* failed on a phone. The two tests below cover both halves, what is
|
||||
* written now, and what has to be tolerated because it is already on
|
||||
* devices in the field.
|
||||
*/
|
||||
@Test
|
||||
fun `a trust record with no address writes no address at all`() {
|
||||
@@ -340,39 +327,35 @@ class VaultTest {
|
||||
val secret = key()
|
||||
Vault(storage, secret).saveTrust(7, Vault.Trust(listOf("aaa"), verified = false, fnid = null))
|
||||
|
||||
assertNull(Vault(storage, secret).loadTrust(7)?.fnid,
|
||||
"an absent address came back as a value")
|
||||
assertNull(
|
||||
Vault(storage, secret).loadTrust(7)?.fnid,
|
||||
"an absent address came back as a value",
|
||||
)
|
||||
}
|
||||
|
||||
/**
|
||||
* This is the test that reproduces the device, and the one above is
|
||||
* not. On the JVM a real JSON null reads back as `""` whether the writer
|
||||
* omits the key or not, so the round trip above passes with the bug intact.
|
||||
* Handing the reader the literal string is what Android actually does, so
|
||||
* writing it here is what makes a laptop run tell the truth.
|
||||
* This is the test that reproduces the device, and the one above is not.
|
||||
*/
|
||||
|
||||
@Test
|
||||
fun `a record already holding the literal string null reads as no address`() {
|
||||
// Records written the old way exist on phones already. The reader has
|
||||
// to repair them, or those contacts stay permanently mis-addressed,
|
||||
// `rememberIdentity` returns early once `fnid != null`, so nothing would
|
||||
// ever ask again.
|
||||
// Records written the old way exist on phones already.
|
||||
val storage = MapStorage()
|
||||
val secret = key()
|
||||
val vault = Vault(storage, secret)
|
||||
vault.saveTrust(7, Vault.Trust(listOf("aaa"), verified = false, fnid = "null"))
|
||||
|
||||
assertNull(Vault(storage, secret).loadTrust(7)?.fnid,
|
||||
"the literal string \"null\" was accepted as somebody's global address")
|
||||
assertNull(
|
||||
Vault(storage, secret).loadTrust(7)?.fnid,
|
||||
"the literal string \"null\" was accepted as somebody's global address",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a real address still round-trips`() {
|
||||
val storage = MapStorage()
|
||||
val secret = key()
|
||||
Vault(storage, secret).saveTrust(
|
||||
7, Vault.Trust(listOf("aaa"), verified = true, fnid = "fnid:abc123"))
|
||||
Vault(storage, secret).saveTrust(7, Vault.Trust(listOf("aaa"), verified = true, fnid = "fnid:abc123"))
|
||||
val back = Vault(storage, secret).loadTrust(7)
|
||||
assertEquals("fnid:abc123", back?.fnid)
|
||||
assertTrue(back!!.verified)
|
||||
|
||||
Reference in New Issue
Block a user