Skip to content

Quarantine unreadable settings instead of wiping them - #405

Draft
tsouth89 wants to merge 1 commit into
mainfrom
cursor/sbs-1074-settings-read-fail-closed-22d3
Draft

Quarantine unreadable settings instead of wiping them#405
tsouth89 wants to merge 1 commit into
mainfrom
cursor/sbs-1074-settings-read-fail-closed-22d3

Conversation

@tsouth89

@tsouth89 tsouth89 commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Summary

Settings::read_path mapped secure_file::read_string errors (DPAPI unprotect, unsupported ProtectedFile version, IO on an existing file) to defaults with no .bak. The next try_update then atomically replaced the live undecodable settings.json.

Parse failures already quarantine (SBS-954 / SBS-1029). This change treats those read failures the same way:

  • Unlocked load leaves the live file in place and asks load() to retry under the state lock.
  • Locked load / try_update moves the original to settings.json.bak before writing defaults onto the vacated path.

The account ledger persist path had the same fail-open wipe: record_and_persist used load_default() (undecodable → empty) then saved. Persist now fails closed, matching API keys and window_geometry.json.

Related issue

Closes SBS-1074.

Affected areas

  • Tray panel
  • Settings UI
  • Config file / settings persistence
  • CLI
  • Provider-specific behavior
  • Installer / release packaging
  • Startup / background behavior
  • Documentation
  • Other:

Validation

The four new settings tests failed on main before the fix (unlocked read did not set pending_quarantine; try_update wrote defaults over the live file with no .bak). They pass after the change.

Commands run on this Linux cloud agent (Windows-native DPAPI/NTFS not available here):

  • Other:
    • cargo fmt --all --check --manifest-path rust/Cargo.toml — pass
    • cargo test --manifest-path rust/Cargo.toml — pass (1192 lib + 32 bin)
    • cargo clippy --manifest-path rust/Cargo.toml --all-targets -- -D warnings — fails on pre-existing Linux-only dead code in secure_file.rs / updater.rs (Windows-gated). Not introduced by this PR. Hosted CI is Windows.
    • cargo test --manifest-path apps/desktop-tauri/src-tauri/Cargo.toml — cannot build here (gdk-3.0 missing). Hosted desktop CI is Windows.

Hosted CI on c0daa54d (rebased onto main / 1.5.36) is green: Frontend, Rust / shared, Rust / desktop, Rust aggregator, and CodeQL all passed. An earlier Rust / shared failure on cli::tty_runner::tests::test_run_sends_script_through_pty was an unrelated PTY flake and did not reproduce.

UI / tray proof

  • Not applicable

Notes for reviewers

  • Settings recovery matches parse failures: quarantine then write defaults to the vacated live path. Readers still fail open.
  • Sibling sweep of secure_file stores:
    • ApiKeys / ManualCookies / token accounts / directory accounts / geometry already fail closed for writers.
    • Account ledger persist now fails closed too. load_default remains fail-open for readers.
  • Derived history stores (usage-history.json, quota-run-history.json, capacity-event baselines) still fail-open then persist. They are rebuildable charts/events, not user-authored settings; left alone to keep this change scoped.
  • Do not merge. Draft PR left open for review.
Open in Web Open in Cursor 

Note

Quarantine unreadable settings.json and account ledger instead of overwriting them

  • Previously, unreadable settings files (DPAPI unprotect failure, unsupported ProtectedFile version, IO errors) were silently replaced with empty defaults on next save. Now read_path delegates both parse and read failures to handle_undecodable, which either quarantines the live file to .bak (when locked) or sets pending_quarantine so load() retries under the lock.
  • The account ledger's record_and_persist now goes through a new persist_if_changed helper that loads from the default path first. If the file is undecodable or unreadable, it warns and returns early without writing, leaving the corrupt content intact.
  • Adds tests covering unsupported ProtectedFile versions, DPAPI unprotect failures, invalid UTF-8, and undecodable ledgers.
  • Behavioral Change: Settings.read_path and AccountLedger.record_and_persist no longer overwrite unreadable files; callers that relied on silent replacement will instead find a .bak file and a fresh defaults file written to the vacated live path.

Macroscope summarized bd930fc.

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
ceiling c0daa54 Commit Preview URL

Branch Preview URL
Aug 23 2026, 10:31 PM

Settings::read_path treated DPAPI, unsupported ProtectedFile, and IO
errors as defaults with no .bak, so try_update overwrote the live file.
Match the SBS-954 parse quarantine and fail-closed ledger persist.

Co-authored-by: Tyler <tyler@southboundsoftware.com>
@cursor
cursor Bot force-pushed the cursor/sbs-1074-settings-read-fail-closed-22d3 branch from bd930fc to c0daa54 Compare August 23, 2026 22:30
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