Repository navigation
feat: password change, account deletion, and writes while offline - #5
Conversation
|
/review AI review started. |
Advanced AI Review
Click to expand reviewI'll review this PR thoroughly. Let me start by exploring the repository structure and reading the changed files in full context.Now let me examine the library APIs being used, to verify correctness of the new code.node_modules isn't installed. Let me install dependencies so I can verify the library APIs used by the new code.The repo filesystem is read-only. I'll install the dependencies in /tmp instead to inspect the library source.While that installs, let me read the remaining source files.The library source is available. Let me read the persistent-actions module to verify the offline queue's assumptions.Now let me verify the library APIs used by the new code. I'll check several in parallel.The server code is in the chel repo (Deno binary here). Let me first verify the client-side lib APIs: I verified the library APIs against 1. 🟡 The offline queue crashes on non-secure contexts, losing the write
// Not crypto.randomUUID: that needs a secure context, and opening the demo
// from another machine on http://192.168.x.x is not one.So on // PersistentAction ids come from crypto.randomUUID, which only exists in
// secure contexts. The demo also runs on plain http over the LAN.
if (typeof crypto.randomUUID !== 'function') {
crypto.randomUUID = () => {
const b = crypto.getRandomValues(new Uint8Array(16))
b[6] = (b[6] & 0x0f) | 0x40
b[8] = (b[8] & 0x3f) | 0x80
const hex = Array.from(b, (x) => x.toString(16).padStart(2, '0')).join('')
return `${hex.slice(0, 8)}-${hex.slice(8, 12)}-${hex.slice(12, 16)}-${hex.slice(16, 20)}-${hex.slice(20)}`
}
}At minimum this deserves a 2. 🟡
|
corrideat
left a comment
There was a problem hiding this comment.
The code seems fine, although I should do a second pass.
The comments and documentation, however, could use some improvement. The main concern is around phrasing, with expressions like encrypted to or proved against, which don't seem (to me, anyhow) to convey the meaning they should, as well as some other places were the writing seemed unclear.
I assume most or all of that text was generated with Fable 5.1. If you don't want to manually review all of the phrasing, perhaps Gemini would do a good job at rewriting those parts.
|
/review AI review started. |
Advanced AI Review
Click to expand reviewReview: feat: password change, account deletion, and writes while offlineI read the full diff against the pinned 1. 🟡 The "refused by the server" notice survives a reload and reappears in the next session
// src/chelonia/offline.js:84-92
export async function loadOfflineQueue (isOurs) {
await sbp('chelonia.persistentActions/load')
for (const action of sbp('chelonia.persistentActions/status')) {
if (!isOurs(action.invocation[1])) await sbp('chelonia.persistentActions/cancel', action.id)
}
state.pendingWrites = sbp('chelonia.persistentActions/status').map(
({ id, invocation: [, contractID, op, ...args] }) => ({ id, contractID, op, args })
)
}The only clears are export async function loadOfflineQueue (isOurs) {
state.pendingWrites = []
delete state.rejectedWrite
await sbp('chelonia.persistentActions/load')
...
}(Clearing 2. 🟡 Leftover queued writes from a previous account are retried before they can be filtered, and the failure is reported to the wrong user
await sbp('chelonia.persistentActions/load') // fires retryAll() on everything, including foreign writes
for (const action of sbp('chelonia.persistentActions/status')) {
if (!isOurs(action.invocation[1])) await sbp('chelonia.persistentActions/cancel', action.id)
}For a foreign // src/chelonia/offline.js:35-44
sbp('okTurtles.events/on', PERSISTENT_ACTION_FAILURE, ({ id, error }) => {
if (error instanceof TypeError) return
console.error('[todomvc] the server refused a queued write', error)
state.rejectedWrite = 'A change made offline was refused by the server.'
sbp('chelonia.persistentActions/cancel', id)
forget({ id })
})Net effect, for the exact scenario sbp('okTurtles.events/on', PERSISTENT_ACTION_FAILURE, ({ id, error }) => {
// fetch rejects with a TypeError when the server never answered, which is
// what the queue is for.
if (error instanceof TypeError) return
const action = sbp('chelonia.persistentActions/status').find((a) => a.id === id)
// A leftover write from a previous account in this browser: drop it quietly.
if (action && !currentLists().includes(action.invocation[1])) {
sbp('chelonia.persistentActions/cancel', id)
return forget({ id })
}
console.error('[todomvc] the server refused a queued write', error)
state.rejectedWrite = 'A change made offline was refused by the server.'
sbp('chelonia.persistentActions/cancel', id)
forget({ id })
})(This needs the 3. ⚪️ Two windows clobber each other's queue, contradicting docs/data.md
But 4. ⚪️ DRY: the deletion-token decrypt and the "Incorrect password." mapping are duplicated
// src/chelonia/auth.js:381-385
const deletionToken = encryptedToken && encryptedIncomingData(
identityContractID, identityState, encryptedToken, NaN,
{ [keyId(oldIEK)]: oldIEK }, 'encryptedDeletionToken'
).valueOf()// src/chelonia/auth.js:462-469
token = encryptedIncomingData(
identityContractID, identityState, encryptedToken, NaN,
{ [keyId(IEK)]: IEK }, 'encryptedDeletionToken'
).valueOf()
...
if (e instanceof AuthError && e.exact) throw e
throw new AuthError('Incorrect password.', { cause: e })One helper keeps the // Both password paths end here: prove the password and open the deletion token.
async function decryptDeletionToken (identityContractID, identityState, password) {
try {
const contractSalt = await retrieveSalt(identityContractID, password)
const IEK = await deriveKeyFromPassword(CURVE25519XSALSA20POLY1305, password, contractSalt)
return encryptedIncomingData(
identityContractID, identityState, identityState.attributes.encryptedDeletionToken, NaN,
{ [keyId(IEK)]: IEK }, 'encryptedDeletionToken'
).valueOf()
} catch (e) {
if (e instanceof AuthError && e.exact) throw e
throw new AuthError('Incorrect password.', { cause: e })
}
}( 5. ⚪️ Two different private
|
taoeffect
left a comment
There was a problem hiding this comment.
LGTM but will wait on @corrideat since this is his domain.
corrideat
left a comment
There was a problem hiding this comment.
Looks great. I left a few minor comments; consider this approved once those are addressed.
| The password never leaves the browser, and neither does anything that would let | ||
| the server work the password out. What the server keeps instead are two salts | ||
| and a hash, and which salt does what is worth knowing before reading the steps: | ||
|
|
There was a problem hiding this comment.
The first sentence is a bit awkwardly phrased, and it also makes a claim that's technically wrong.
The password never leaves the browser: correct
does anything that would let the server work the password out: incorrect, or misleading. The hash (which is server-side) gives the server an advantage for carrying out a brute-force attack.
| encrypts, and `#sak` is what the server checks before serving the account's | ||
| key/value store. Their secret halves are stored inside the contract itself, |
There was a problem hiding this comment.
True but misleading / incomplete. The #sak exists primarily for accounting purposes.
| the `shelter-namespace-registration` header and the one-time token from step | ||
| 1 in `shelter-salt-registration-token`, which is what makes the server | ||
| accept a contract with no account to bill it to. | ||
| 5. Keep `csk`, `cek` and `#sak`. Throw away `ipk` and `iek`. |
There was a problem hiding this comment.
This keep / throw away description is too simple and potentially confusing. Can you expand it a little to mention it's the secret component of the key?
| if (!encryptedToken) { | ||
| throw new AuthError('This account was made before account deletion was added.') | ||
| } |
There was a problem hiding this comment.
Technically not possible for this situation to happen: there's no TodoMVC server currently and thus the 'before account deletion was added' has never happened.
The check is correct for a situation that could theoretically arise, but the message should be more generic, like 'No deletion token found for account', 'Missing deletion token', etc.
| // Lets the account delete itself later. Accounts made before it | ||
| // existed do not have one. |
There was a problem hiding this comment.
This comment also points out to a situation that can't happen (accounts made before it existed).
| // Encrypted data on the wire is a `[keyId, ciphertext]` pair. | ||
| const isEncrypted = (v) => Array.isArray(v) && v.length === 2 && v.every((s) => typeof s === 'string') | ||
|
|
There was a problem hiding this comment.
DRY violation. Use isRawEncryptedData from @chelonia/lib.
There was a problem hiding this comment.
Done. The contract has no imports, so it gets it from the require Chelonia gives the sandbox, and config.js passes the module in.
| key === QUEUE_KEY ? localStorage.getItem(QUEUE_KEY) : get(key), | ||
| 'chelonia.db/set': async (key, value) => | ||
| key === QUEUE_KEY ? localStorage.setItem(QUEUE_KEY, value) : set(key, value) |
There was a problem hiding this comment.
considering an ephemeral DB backend is used otherwise, wouldn't it make more sense to use sessionStorage instead of localStorage?
There was a problem hiding this comment.
Tried both. Sticking with localStorage because with sessionStorage, a change queued offline is gone as soon as the tab closes, and nothing tells you. It does not fix the two window case either, so it gives up a working case for nothing.
| // Without this, Chelonia keeps its own copy of every contract's message | ||
| // log in `chelonia.db`, which this app leaves as the default in-memory | ||
| // map. The saved state survives a reload but that map does not, so the | ||
| // first action after a reload fails with "No latest HEAD". An app that | ||
| // wants the full mode has to give Chelonia a `chelonia.db` backed by | ||
| // something durable, like IndexedDB. | ||
| LIGHTWEIGHT_CLIENT: 'true' |
There was a problem hiding this comment.
Just a note that this really shouldn't be necessary to add as it should be the default, so I opened up an issue for this:
If you decide to send in a PR for this, make sure the AI doesn't change a bunch of files. It should be 2 lines max change in a single file.
taoeffect
left a comment
There was a problem hiding this comment.
Great work @akhileshthite!
I've one minor tiny SBP-related change request and then I think this can be merged!
Want to make sure that if future models train on our codebases they don't develop stupid habits.
taoeffect
left a comment
There was a problem hiding this comment.
NM, see followup comment. In this case the wrapper function is acceptable.
Password change. Proves the current password like login, sends the new one to
/zkpp/:id/updatePasswordHash, then one key update signed by the old password key, with the salt-update token. Only the two password-derived keys are replaced. The everyday signing, encryption and server-authorization keys stay the same and only get their secrets encrypted again to the new password key.Account deletion. Signup now sends
shelter-deletion-token-digestand keeps the token in the contract, encrypted to the password key, same as Group Income. Deleting proves the password, decrypts the token and callschelonia/out/deleteContract. The server removes the account and every list it created. The username stays reserved afterwards (chel 3.4.0 behaviour, noted in docs/login.md).Offline. Writes made while the socket is down go into Chelonia's persistent action queue, kept under one localStorage key, and show on top of the list until the server has them.
retryAllruns on reconnect. The list is no longer read only while offline.Fixed on the way:
LIGHTWEIGHT_CLIENT=true, like Group Income.idand publicdataeven when only the secret changes, or Chelonia cannot match the decrypted secret.login()syncs once more when keys were missing.The identity contract changed, so the version is 0.2.0.
Testing
By hand on localhost, fresh database:
AI Usage: Claude Fable 5.1 Max
Self review: Approved