ReviewOS

also looking at this

craft-native/craft

feat(android): the secure-storage quartet — ratchet 95 -> 91

#147
Merged chris wants to merge feat/android-secure-storage into main
6 files +501 -3
10 days ago

Ratchet 95 → 91. Four actions, and a pattern that generalises.

Kotlin passes the store; Zig does not build one

Every other Android action here reaches its platform object through the Activity. These four don't, deliberately.

securePrefs is EncryptedSharedPreferences.create(activity, "craft_secure_prefs", masterKey, AES256_SIV, AES256_GCM) over a lazily-built MasterKey. Zig could construct that — two builder calls and two enum constants — and constructing it would be a mistake.

It's a second implementation of a security-sensitive construction, in a second language, that has to agree with the first exactly. A different key scheme, file name, or SIV mode and the two halves write data neither can read. Nothing fails at build time. The symptom is a user's saved token disappearing after an app update.

So the natives take the SharedPreferences as a parameter. The object is Kotlin's, the lifetime is Kotlin's, and Zig calls only the plain SharedPreferences interface — which is all the Kotlin does too, since EncryptedSharedPreferences is one and the encryption sits behind the interface.

This generalises: where the shim already holds a configured platform object, handing it across the seam beats rebuilding it.

secureGet has three answers where the others have two

The Kotlin returns String?, and null there means "no such key". The seam also needs "Zig did not serve this" — and a nullable String can't carry both, because the caller does different things about them.

There's no spare value to steal: "" is a legitimate stored value, and so is any sentinel a caller could have saved. So the reply is a {"found":…} envelope, null is reserved for declined, and the holder decodes it into a sealed SecureRead:

when (val read = CraftNative.secureGet(securePrefs, key)) {
    is CraftNative.SecureRead.Found -> return read.value   // may be ""
    CraftNative.SecureRead.NotFound -> return null
    CraftNative.SecureRead.Declined -> Unit                // fall through
}

A mutation that wasn't caught

Returning "" for an absent key passed every test in the file. I'd tested renderRead and not get — and the absent-versus-empty distinction is settled in get, against getString(key, null).

A fake SharedPreferences covers it now, including end to end, because the two halves are only correct together: a get that loses the distinction and a renderRead that keeps it still produce {"found":true} for a key nobody stored.

mutationbeforeafter
quotes dropped from the envelopecaughtcaught
absent key returned as ""not caughtcaught, by two tests

apply, not commit

Matching the Kotlin. apply returns void, so a disk failure is invisible to both implementations and true means the edit was accepted, not persisted. Switching to commit would make Zig report a truth the shim doesn't — a divergence even when it's an improvement.

The conformance check earned its keep

I registered the four natives before writing the Kotlin half, and got RegisteredNativeNotDeclared at build time — rather than RegisterNatives refusing the whole batch at load, on a device, with every action silently dropping to the shim.

zig build test:android → 92 tests, 11/11 steps · both .so build · generator 10/10.

6 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.