From 276f9f189ab885a48ad77b0c77cefd6cec55ce5c Mon Sep 17 00:00:00 2001 From: michael Date: Tue, 26 May 2026 12:37:36 -0700 Subject: [PATCH] test: tighten CredentialStoreSerializationTest against silent reorders Code review of 40a079a flagged two gaps: - Round-trip test set all 5 booleans to true, so a reorder among the boolean writeTo lines would not break the test. Now uses a distinguishable mix (T/T/F/T/F) and asserts each predicate explicitly. - FIELD_VERSION rejection branch in readFrom had zero coverage. Adds a test that forges an out-of-band version byte and asserts ISOException(SW_DATA_INVALID). Reuses the existing hasAccessDocument() predicate (which already returns accessDocumentFinalized) rather than adding a new test-only helper. Manually verified the round-trip test catches a silent boolean reorder by swapping committed/credentialPubKeySet writes in writeTo and observing isCommitted() assertion fail (reverted before commit). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../CredentialStoreSerializationTest.java | 65 ++++++++++++++----- 1 file changed, 47 insertions(+), 18 deletions(-) diff --git a/applet/src/test/java/com/dangerousthings/aliro/CredentialStoreSerializationTest.java b/applet/src/test/java/com/dangerousthings/aliro/CredentialStoreSerializationTest.java index b3b505d..cf14835 100644 --- a/applet/src/test/java/com/dangerousthings/aliro/CredentialStoreSerializationTest.java +++ b/applet/src/test/java/com/dangerousthings/aliro/CredentialStoreSerializationTest.java @@ -3,9 +3,15 @@ package com.dangerousthings.aliro; import java.util.ArrayList; import java.util.List; +import javacard.framework.ISO7816; +import javacard.framework.ISOException; + import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** @@ -23,9 +29,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; class CredentialStoreSerializationTest { private static final byte[] KNOWN_32_BYTES = makeRange(32, 0xA0); - private static final byte[] KNOWN_64_BYTES = makeRange(64, 0x10); private static final byte[] KNOWN_64_BYTES_B = makeRange(64, 0x80); - private static final byte[] KNOWN_DOC_BYTES = makeDoc(128); private static byte[] makeRange(int len, int start) { byte[] out = new byte[len]; @@ -35,26 +39,20 @@ class CredentialStoreSerializationTest { return out; } - private static byte[] makeDoc(int len) { - byte[] out = new byte[len]; - for (int i = 0; i < len; i++) { - // A distinguishable, non-trivial pattern that isn't a simple - // run — flushes out off-by-one read/write bugs. - out[i] = (byte) ((i * 31 + 7) & 0xFF); - } - return out; - } - @Test void credentialStoreRoundTripsViaSerializationContract() { + // Deliberately distinguishable mix (T/T/F/T/F) so that a silent + // reorder among the boolean writeTo lines would change which field + // ends up true/false after readFrom — and fail the per-predicate + // assertions below. If all five booleans were true the reorder + // would be undetectable. CredentialStore src = CredentialStore.bootstrap(); src.resetForTesting(); src.setCredentialPrivKey(KNOWN_32_BYTES, (short) 0); - src.setCredentialPubKey(KNOWN_64_BYTES, (short) 0); + // Intentionally do NOT setCredentialPubKey: leave credentialPubKeySet=false. src.setReaderPubKey(KNOWN_64_BYTES_B, (short) 0); - src.writeAccessDocumentChunk(KNOWN_DOC_BYTES, (short) 0, (short) 0, - (short) KNOWN_DOC_BYTES.length); - src.finalizeAccessDocument((short) KNOWN_DOC_BYTES.length); + // Intentionally do NOT finalizeAccessDocument: leave accessDocumentFinalized=false + // and accessDocumentLen=0. With len=0, copyAccessDocument() returns an empty array. src.commit(); RecordingSink buf = new RecordingSink(); @@ -62,11 +60,42 @@ class CredentialStoreSerializationTest { CredentialStore restored = CredentialStore.readFrom(buf.toSource()); + // Byte-array fields that were set must round-trip exactly. assertArrayEquals(KNOWN_32_BYTES, restored.copyCredentialPrivKey()); - assertArrayEquals(KNOWN_64_BYTES, restored.copyCredentialPubKey()); assertArrayEquals(KNOWN_64_BYTES_B, restored.copyReaderPubKey()); - assertArrayEquals(KNOWN_DOC_BYTES, restored.copyAccessDocument()); + + // The unset credential pubkey buffer should round-trip as all-zero — + // resetForTesting cleared it and we never wrote to it. + assertArrayEquals(new byte[CredentialStore.CRED_PUBK_LEN], + restored.copyCredentialPubKey()); + + // accessDocumentLen=0, so copyAccessDocument() returns an empty array. + assertArrayEquals(new byte[0], restored.copyAccessDocument()); + + // Per-predicate assertions: any swap in the boolean section of + // writeTo flips at least one of these. assertTrue(restored.isCommitted()); + assertTrue(restored.hasCredentialPrivKey()); + assertFalse(restored.hasCredentialPubKey()); + assertTrue(restored.hasReaderPubKey()); + assertFalse(restored.hasAccessDocument()); + } + + /** + * Forges a payload with a bogus FIELD_VERSION byte and asserts that + * {@link CredentialStore#readFrom} rejects it with + * {@code ISOException(SW_DATA_INVALID)}. The version check is the very + * first read in readFrom, so no further padding is required — the + * replay source is never read past the version byte. + */ + @Test + void readFromRejectsWrongFieldVersion() { + RecordingSink buf = new RecordingSink(); + buf.write((byte) 0x7F); // != FIELD_VERSION (== 1) + + ISOException ex = assertThrows(ISOException.class, + () -> CredentialStore.readFrom(buf.toSource())); + assertEquals(ISO7816.SW_DATA_INVALID, ex.getReason()); } /**