fix: Cross-device password sync (v0.18.9)

[pboueke]

No description provided.

[github-actions]

Carranca AI Review

Let me read the relevant source files to complete my security and correctness review:

Code Review: PR #9 "fix: Cross-device password sync (v0.18.9)"

Summary

This PR fixes a critical data corruption bug where changing a password on one device would cause another device to overwrite the remote with stale data during sync. The solution introduces per-device lastKnownRemoteSalt tracking to disambiguate local vs remote password changes. Additionally, it fixes a bug where attachment.encrypted flags weren't updated during password changes, causing attachment loading failures.

---

Findings

Critical

• None identified

High

• Missing cleanup on journal deletion: When local.deleteJournal is called, the corresponding canto:lastRemoteSalt:{journalId} entry in AsyncStorage is not cleaned up. This could lead to stale salt data being reused incorrectly if a journal with the same ID is recreated.

  • Location: src/lib/sync/manager.ts (no cleanup exists, should be added)
  • Recommendation: Add a method in SyncManager to clean up salt entries when a journal is deleted

• Error message reveals too much info: The error messages in engine.ts:141-151 include the journal title, which could leak sensitive information in error logs or crash reports.

  • Location: src/lib/sync/engine.ts:141,149
  • Recommendation: Consider sanitizing or redacting the journal title in error messages

Medium

• Silent failure in salt persistence: In manager.ts:127-135, if this.store.listRemoteJournals() throws, the salt is not stored but sync continues. The next sync will treat it as "no history" which could incorrectly push changes. While noted as non-fatal, this could still cause issues.

  • Location: src/lib/sync/manager.ts:127-135
  • Recommendation: Consider logging this failure clearly or retrying the operation

• Test uses non-null assertion without safety check: In cross-device-password-change.test.ts:521, code accesses drive.dump().find(...)!.content with ! assertion. If the file isn't found, this will throw a generic error rather than a descriptive one.

  • Location: src/lib/__tests__/cross-device-password-change.test.ts:521
  • Recommendation: Add a null check with a descriptive error message

Low

• Inconsistent error handling in salt recording: The try-catch block for recording salt (line 127-135) swallows all errors without even logging them in production.

  • Location: src/lib/sync/manager.ts:133-135
  • Recommendation: Add console.error logging even if non-fatal

• Potential lock leakage: If syncJournal throws before the finally block due to an uncaught exception in an await before the lock is added, the lock might not be cleaned up. However, looking at the code flow, this seems unlikely.

  • Location: src/lib/sync/manager.ts:77
  • Recommendation: Consider adding the lock to a finally block earlier in the method

Info

• Good test coverage: 14 new tests covering the cross-device password change scenario, manager-level salt tracking, and attachment encryption flag handling.

• Proper atomic-ish sync ordering: The comment and code at engine.ts:244-249 correctly identifies that uploading metadata/registry after page uploads provides better recovery on interruption.

• Clean separation of concerns: The lastKnownRemoteSalt is kept in per-device AsyncStorage rather than transmitted, avoiding any cross-device state synchronization complexity.

---

Verdict

APPROVE

This PR addresses a critical data corruption bug with a well-thought-out solution that adds minimal complexity while significantly improving cross-device password sync reliability. The test coverage is comprehensive, and the implementation follows secure coding practices. The identified issues are relatively minor and don't block the PR.

carranca bound glm-4.6

---

Automated review by carranca sandbox

[github-actions]

Carranca AI Review

I'll review PR #9 "fix: Cross-device password sync (v0.18.9)" focusing on the key areas specified.

Code Review: PR #9 - Cross-device password sync fix (v0.18.9)

Summary

This PR fixes a critical cross-device sync corruption bug where changing a password on one device and syncing on another device with stale data would corrupt the remote by overwriting it with stale data. The solution introduces lastKnownRemoteSalt tracking per device to distinguish "I changed my password locally" (push) from "another device changed the password" (abort), and fixes attachment encryption flags during password changes.

---

Findings

Security

• Medium (src/lib/sync/manager.ts:1675): Salts stored in AsyncStorage are not encrypted on web platform. While AsyncStorage uses Keychain/EncryptedSharedPreferences on iOS/Android (secure), web uses localStorage in plaintext. Salts are not secrets, but this information disclosure on web could aid in cryptographic analysis. Consider adding platform-specific encryption for web or documenting this limitation.

• Low (src/lib/sync/engine.ts:1631-1632): Error messages expose journal titles: Sync aborted: the password for "${localJournal.title}" was changed on another device. This is minor information disclosure since journal titles are already in the remote registry, but could be used for targeted attacks. Consider a generic error message if this is a security concern.

• Info (src/lib/sync/engine.ts:1644-1647): Comment notes that encrypted-flag mismatch without salt-mismatch "falls through to push behaviour". While this edge case shouldn't occur normally, it means the system would accept and upload data even if remote.encrypted differs from local.secure without salt change. This could theoretically lead to inconsistent states but is defensive since the original engine had the same behavior.

Encryption Correctness

• Low (src/lib/tests/helpers/key-aware-crypto.ts:1196): The keyTag function uses only the first 4 bytes of the 32-byte key for the tag. While this is sufficient for test mocking (16⁴ ≈ 4.3 billion possible values), real AES-GCM uses a 16-byte auth tag. The comment correctly notes this is test-only mock behavior.

• Info (src/lib/storage/local.ts:1531-1544, local.web.ts:1566-1579): The reencryptJournal fix correctly updates attachment.encrypted flags to match !!newKey. This fixes the bug where attachments couldn't be decrypted after password changes. The two-layer encryption (device + optional password) is properly modeled.

TypeScript & Code Quality

• Info (src/lib/sync/manager.ts:1706): The null coalescing ?? undefined is technically redundant since AsyncStorage.getItem() already returns string | null, and null is acceptable as previousRemoteSalt?: string. However, this makes the intent explicit and is harmless.

Performance

• Low (src/lib/sync/manager.ts:1739-1743): After successful sync, the manager makes a second call to listRemoteJournals() to fetch the updated salt. This doubles remote registry API calls per sync. While this ensures accuracy (salt may have changed if a key rotation occurred during sync), it could impact performance with many journals. Consider passing the salt from the sync engine result or caching the registry.

Test Coverage

• Info: Excellent test coverage with 14 new tests covering all critical paths. The tests use key-aware encryption mocks to properly simulate AES-GCM auth-tag failures. However, I notice no test for the performance concern above (double listRemoteJournals call).

State Management

• Medium (src/components/home/NewJournalModal.tsx:261): Cloud import calls await manager.recordRemoteSalt(remote.id, remote.salt) AFTER saving the journal locally. If the save succeeds but recordRemoteSalt fails, the next sync would fail with a false-positive abort. Consider wrapping both operations in a transaction or handling the error more gracefully.

• Info (src/lib/sync/manager.ts:1744-1748): The catch block swallows errors from salt recording with a console.warn. This is correct (non-fatal), but could mask persistent AsyncStorage issues. Consider adding a counter or monitoring to detect repeated failures.

React Native / Expo Patterns

• Info (src/components/home/NewJournalModal.tsx:268-327): The platform-specific modal styling (transparent on web, fullscreen on native) correctly uses Platform.OS check and useSafeAreaInsets. No issues found.

Cross-Device Test Simulation

• Low (src/lib/tests/cross-device-password-change.test.ts:440-452, 502-508): The test uses a global asyncStore mock with snapshot/restore to simulate per-device AsyncStorage isolation. While this works, it's fragile - any test that doesn't properly restore state could pollute others. Consider using a per-test storage factory for better isolation.

---

Verdict

COMMENT

This PR addresses a critical data corruption bug with a well-thought-out solution and excellent test coverage. The implementation is solid, but I have identified a few medium-priority concerns:

1. Web platform stores salts in plaintext (not critical but worth documenting)
2. Double listRemoteJournals() call impacts performance
3. Missing error handling for salt recording failure after cloud import

None of these issues are blockers, but addressing them would improve the robustness of the solution. The core logic is sound, the security model is maintained, and the test coverage is exceptional.

---

carranca bound zai-coding-plan/glm-4.6

---

Automated review by carranca sandbox