Skip to content

Tighten parsing, error handling, and Keychain hygiene#3

Merged
felizvida merged 2 commits into
felizvida:mainfrom
lagostimgomes:fix/code-audit-findings-3
May 25, 2026
Merged

Tighten parsing, error handling, and Keychain hygiene#3
felizvida merged 2 commits into
felizvida:mainfrom
lagostimgomes:fix/code-audit-findings-3

Conversation

@lagostimgomes

Copy link
Copy Markdown
Contributor

Summary

Third audit pass focused on three classes of issue: defensive JSON parsing for ledger/render files, error propagation through cleanup paths, and a Keychain hygiene fix on iOS.

Safety / correctness

  • help_search_service.dart — the OpenAI body read had no timeout; only the header exchange was time-bounded. Mirrors the fix already in notebook_search_service.dart (PR Fix 12 issues from second code audit #2) — the same caller pattern is duplicated here. Adds a 120 s body-read timeout.
  • backup_models.BackupRunManifest.fromJson — hard cast json['createdAt'] as String raised TypeError on a corrupt or older manifest. Now raises a clear FormatException ("Backup run manifest is missing required date createdAt") and re-uses the parsed value for the completedAt fallback so the field is only parsed once.
  • backup_models.RenderNotebook.fromJson — same hard-cast pattern on createdAt; replaced with DateTime.tryParse + epoch fallback so an old render file no longer prevents the rest of the index from loading.
  • backup_models.RenderNode.fromJson / RenderPart.fromJsonjson['id'] as int is the only remaining hard cast for required fields; switched to as int? ?? 0 for parity with the other safe casts in the same constructors.
  • labarchives_client.downloadNotebookBackup — in the catch block, partial.delete() runs unwrapped. If the unlink fails (e.g. transient FS error on Windows after a connection drop), the unlink exception replaces and hides the original download error, so the user sees a misleading message and rethrow is never reached. Wrap the cleanup in try/catch.
  • backup_service._classifyBackupError — the final lower.contains('auth') branch was meant to catch authorization-related errors but also matches "author", "authority", "co-author", etc. Replaced with the narrower 'authoriz', 'auth_code', and 'auth code' tokens.
  • backup_service._withBackupLock — if lock.unlock() throws (rare but possible on remote filesystems), lock.close() is skipped and the file handle leaks. Wrap unlock in try/catch so close always runs.
  • ios/Runner/AppDelegate.swift — explicitly pin kSecAttrSynchronizable = false on every Keychain query. The default on iOS is already non-synchronizable, but pinning it makes the read/write/delete queries unambiguous: BenchVault credentials are never eligible for iCloud Keychain sync, and a synchronizable item under the same service/account from older code (or another app) would not silently shadow the local one.

Test plan

  • flutter analyze is clean
  • Run an AI help search — confirm answer arrives and that a deliberately slow upstream surfaces a TimeoutException instead of hanging
  • Corrupt backups/.../run.json by removing createdAt, reload manifests — confirm a FormatException with a clear message, not a TypeError
  • Trigger a notebook download against a target that returns headers then drops the connection — confirm the surfaced error is the network error, not a "cannot delete file" error
  • Inject an error message containing the word "author" — confirm it is not classified as BackupFailureCategory.authorization
  • On iPad, save a credential and verify in Keychain Access (paired Mac, if applicable) that the item is not marked synchronizable

🤖 Generated with Claude Code

lagostimgomes and others added 2 commits May 25, 2026 18:00
- help_search_service: add 120s timeout to OpenAI response body read
  (same bug the notebook_search_service fix already addressed)
- backup_models.BackupRunManifest.fromJson: raise a clear FormatException
  when createdAt is missing or non-string instead of throwing TypeError;
  reuse the parsed value for completedAt fallback
- backup_models.RenderNotebook.fromJson: tolerate missing/invalid createdAt
  using DateTime.tryParse + epoch fallback
- backup_models.RenderNode/RenderPart.fromJson: null-safe id casts so
  a corrupt render_notebook.json no longer breaks the whole load
- labarchives_client.downloadNotebookBackup: wrap partial.delete() cleanup
  in try/catch so a failed unlink can't replace the original download
  error with an unrelated cleanup error
- backup_service._classifyBackupError: replace lower.contains('auth')
  (which also matched 'author', 'authority') with the narrower 'authoriz',
  'auth_code', and 'auth code' tokens
- backup_service._withBackupLock: wrap lock.unlock() so a failure can't
  skip lock.close() and leak the file handle
- ios AppDelegate: pin kSecAttrSynchronizable=false so credentials never
  sync to iCloud Keychain

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@felizvida felizvida merged commit bfa3ba1 into felizvida:main May 25, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants