# SyncKit Client SDK Audit Review

- Last audited: 2026-03-28 (seventh audit, Run 12 cross-project)
- Previous audit: 2026-03-18 (sixth audit, Run 9 cross-project)
- Crate: `synckit-client` v0.3.0
- Path: `Shared/synckit-client/`

## Overall Grade: A

Run 12: 297 tests (197 unit + 99 integration + 1 doctest). 0 clippy warnings. Grade stable at A. No code changes. New dep advisory: rustls-webpki (RUSTSEC-2026-0049).

## Scorecard

| Dimension | Grade |
|-----------|-------|
| Observability | A |
| Code Quality | A |
| Architecture | A |
| Testing | A+ |
| Security | A- |
| Performance | A |
| Documentation | A |
| Dependencies | A |
| Type Safety | A |
| Concurrency | A |
| Resilience | A |
| API Consistency | A |
| Codebase Size | A |

## Module Heatmap

| Module | File | LOC | Tests | Grade | Notes |
|--------|------|-----|-------|-------|-------|
| client | `src/client.rs` | ~750 | 85 | A | Comprehensive unit tests. Auth state, encrypt/decrypt roundtrip (incl. unicode, empty row_id), type serialization, error classification, token expiry, OAuth URL construction. Blob E2E encryption. Send+Sync compile-time assert. with_http_client constructor for custom timeouts. |
| crypto | `src/crypto.rs` | ~480 | 19 | A+ | Excellent coverage. Roundtrip, wrong-key, version, truncation, uniqueness, zeroize. Binary blob encrypt/decrypt. Random salt wrapping. |
| types | `src/types.rs` | ~240 | 13 | A | Serde roundtrip, Display/serde consistency, from_str_opt rejection, Copy/Hash traits, skip_serializing_if, extra field tolerance, ISO timestamp parsing. |
| keystore | `src/keystore.rs` | 94 | 18 | A | Service name construction, base64 roundtrips, length validation, error handling. Platform behavior documented. |
| error | `src/error.rs` | ~100 | 10 | A | Send+Sync assert, Display for all variants, Debug no-panic, source() chain for Json/Base64, source() None for leaf variants, edge cases (empty/long messages). |
| lib | `src/lib.rs` | 53 | 1 (doctest) | A | Clean re-exports. Doctest validates API surface compiles. |

## Cold Spots

1. ~~**client.rs** (639 LOC, 0 tests, grade C)~~ -- Resolved: 81 unit tests added, grade now A-.
2. ~~**keystore.rs** (0 tests, grade D)~~ -- Resolved: 18 tests added, grade now A-.
3. ~~**No HTTP timeout**~~ -- Fixed: 30s request timeout + 10s connect timeout on `Client::builder()`.
4. ~~**await_holding_lock in change_password**~~ -- Fixed: lock guard now dropped before `.await` point.
5. **No token refresh** -- JWT tokens expire but the SDK has no detection or refresh mechanism. Consumers handle 401s ad hoc. (Token expiry detection was added, but no automatic refresh.)
6. ~~**op field is raw String**~~ -- Resolved: replaced by ChangeOp enum with serde UPPERCASE.

## Strengths

- **Crypto module is excellent** -- 12 unit tests covering all key operations. XChaCha20-Poly1305 with 192-bit random nonces eliminates nonce collision risk. Argon2id with OWASP-minimum parameters.
- **Minimal API surface** -- 8 types re-exported from lib.rs. Wire-only types are `pub(crate)`. Internal helpers are private.
- **Clean key hierarchy** -- Three layers (password -> wrapping key -> master key -> per-entry encryption) are well-documented and correctly implemented.
- **No consumer-specific logic** -- Fully generic. No references to GO, BB, or AF. Table names and data shapes are opaque to the SDK.
- **ZeroizeOnDrop** -- In-memory keys are zeroed on drop via volatile writes.

## Weaknesses

- ~~**CRITICAL: Blob data NOT encrypted**~~ -- RESOLVED: encrypt_bytes/decrypt_bytes added to crypto.rs, blob_upload/blob_download now encrypt/decrypt transparently (40 bytes overhead per blob).
- ~~**Deterministic Argon2 salt**~~ -- RESOLVED: wrap_master_key now generates random 32-byte salt, unwrap_master_key reads salt from envelope and derives wrapping key internally.
- **No key rotation mechanism** -- No way to rotate the master key without re-encrypting all data.
- ~~**13 Mutex `.unwrap()` calls**~~ -- RESOLVED: All replaced with `.lock().map_err()` returning SyncKitError::Internal.
- ~~**Master key copies not zeroized**~~ -- RESOLVED: All call sites wrap in ZeroizeOnDrop.
- ~~**Several public types that should be `pub(crate)`**~~ -- RESOLVED: KeyEnvelope, ZeroizeOnDrop inner field, 5 request/response types restricted.

### Resolved from first audit
- ~~**client.rs untested**~~ -- 66 unit tests added (S4)
- ~~**No resilience**~~ -- HTTP timeouts (30s + 10s connect), retry with backoff (3 retries), token expiry detection all added (S4)
- ~~**change_password concurrency bug**~~ -- Lock guard dropped before await (S4)
- ~~**op field raw String**~~ -- ChangeOp enum with serde UPPERCASE (S4)

## Mandatory Surprise

**change_password race + deterministic salt -- Both resolved.**

~~The `change_password` method holds a `std::sync::Mutex` guard across an `.await` point (`get_server_key`), which Clippy flags as `await_holding_lock`.~~ Fixed: lock guard now dropped before `.await` point.

More critically, if `put_server_key` fails after the old envelope is fetched, the method may log success while the server retains the old envelope. Concurrent `change_password` calls from two devices race silently -- one wins, the other's key cache becomes stale with no notification or error. (Race condition remains a theoretical concern but is low-priority given single-device typical usage.)

~~The Argon2 salt is deterministic from `(app_id, user_id)` via `SHA256(app_id || user_id)`. This means the salt never rotates on password change.~~ Fixed: wrap_master_key now generates a random 32-byte salt per operation, stored in the KeyEnvelope. unwrap_master_key reads the salt from the envelope and derives the wrapping key internally.

## Metrics Over Time

| Date | LOC | Files | Tests | Tests/KLOC |
|------|-----|-------|-------|------------|
| 2026-03-11 | 1,416 | 6 | 13 | 9.2 |
| 2026-03-13 | ~1.4K | 6 | 109 | ~77 |
| 2026-03-13 (post-fix) | ~1.5K | 6 | 118 | ~79 |
| 2026-03-13 (adversarial) | ~2.5K | 6 | 234 | ~94 |
| 2026-03-13 (perf+resilience) | ~2.6K | 6 | 243 | ~94 |
| 2026-03-13 (testing push) | ~2.8K | 6 | 297 | ~106 |
| 2026-03-16 (Run 6) | 4,327 | 6 | 297 | ~69 |
| 2026-03-18 (Run 9) | 4,327 | 6 | 298 | ~69 |
| 2026-03-28 (Run 12) | 4,327 | 6 | 297 | ~69 |

---

See [audit_history.md](./audit_history.md) for full chronological audit log.