fix(security): atomic engine extraction swap and fail-fast macOS ditto in ChromiumAutoDownloader (#910)

[KhushiTrivediii]

Fixes #910

Changes

• Replaces pre-extraction deletion with temporary extraction directory and atomic promotion/swap (promotePendingInstall) in ChromiumAutoDownloader.installFromCandidates so an extraction failure preserves any existing working engine.
• Prevents silent fallback to Java ZipInputStream on macOS when native ditto extraction fails (which produces non-booting framework symlinks) by throwing an explicit exception.
• Adds unit tests in ChromiumAutoDownloaderTest.

[choksi2212]

Cross-referencing against #910 (our filing) since three PRs now sit on it (#929, #942, #946) — one substantive correctness concern in this approach plus the structural notes:

1. Promotion failure is reported as success. The flow calls the startup-only helper promotePendingInstall from the live reinstall path. Every warn-and-return path inside that helper — target.renameTo(backup) failing, or the pending.renameTo(target)-fails-then-restore path — leaves the OLD engine at target with its executable.name present, so the subsequent exists() check passes and the install reports success while the newly downloaded engine is discarded. Two of the helper's three failure paths become false "installed" results, and the version file still shows the old version.

2. The rename-fails path is the expected case at this call site, not an edge. The helper's own KDoc: "Must be called at startup BEFORE isChromiumInstalled / engine creation, while nothing holds files in the engine directory." Calling it mid-session during a reinstall — while JxBrowser may hold engine files, the exact Windows scenario the helper's comment names (AV/lingering handles) — is where renameTo fails.

3. Structural, for the maintainer's triage: this PR and fix(security): re-validate stripped branch name in DesktopGitService.checkout (#919) #934 are near-duplicates opened 30 minutes apart; this one carries a 20-file bundle whose title mentions only the engine; CI is RED from a detekt TooGenericExceptionCaught at SnapshotManager.kt:56:18 (the new catch in this same PR), and the base is main (46 commits behind dev) where the rest of this wave targets dev — mergeStateStatus BLOCKED.

For comparison, #946 implements the swap inline (atomicEngineSwap + promoteAndVerify + restoreEngine) with tests for the extraction-failure, verification-failure-rollback, and happy-path-cleanup cases, and #942 uses a .incoming sibling with startup restore. Whichever shape lands, the false-success path above is worth a regression test before merge.

[kshivang]

@KhushiTrivediii — thank you for the security work here; the intent of every commit is the right one (atomic engine install, fail-closed persistence, credential redaction), and the ChromiumAutoDownloader commit in particular is a well-motivated response to #910. This review (sweep 2026-09-22T0929Z-codeq, agent qwen38-27b) has to park the PR as a draft, with specifics:

Why the PR as a whole can't land (structural):

• The branch carries 6 stacked commits / ~20 files, not just the engine fix named in the title. Four of the six are superseded by code already on dev (landed via Integrate dev: security hardening, reliable persistence, and Home Favorites #935 and the security wave):
  • SnapshotManager (SnapshotManager: path traversal in processId escapes snapshots directory, and save uses non-atomic writes #873): dev's version already validates processId via IpcAddressResolver.validateProcessIdentifier, pins POSIX 0600, and writes atomically with fail-closed semantics. This PR's version replaces that with a local regex + canonical-path check and drops the IPC-layer validation — a regression, not an addition.
  • SettingsServiceImpl (SettingsServiceImpl: concurrent setSetting calls race on uncoordinated writes, and saveToDisk uses non-atomic truncation #871): dev already has the mutations Mutex and atomic move; this PR's version removes the Mutex.
  • PluginManifestReader / LocalPluginRepository (atomicWriteText perms are umask-inherited (world-readable on stock desktops) and two stats/recent-project writes are still non-atomic #860/Unbounded plugin.json entry reads in the loader and local repository let a hostile jar OOM the host before signature verification #857): dev already has the bounded manifest reads (Mihir-Rabari's fix(plugin): bound the manifest entry reads before signature verification #858-series work, landed 09-17/09-18).
  • McpArgumentSanitizer (Security: a credential inside a URI authority reaches the approval dialog and the ledger unmasked #886) and EditorServiceImpl (Security: the out-of-process EditorService saveFile tears user files on crash and its path gate misses Windows system paths and symlink escapes #885) have dedicated open PRs — fix(security): redact URI credentials, curl user flags, AWS keys, and PEM keys in McpArgumentSanitizer (#886) #898 and fix(security): implement atomic save and canonical path validation in EditorServiceImpl (#885) #904 (your own branches) — so stacking them here duplicates the surface.
• As a result the branch is mergeable=DIRTY against dev and CI is red on code-quality (detekt) plus the ipc matrix on the stale base.

Specific issues in the engine commit (1ad17ca), which I verified against dev's current code:

1. False success on promotion failure. The non-staged path calls the startup-only helper promotePendingInstall(pending, target, backup). That helper's contract (its own KDoc) is "must be called at startup BEFORE engine creation, while nothing holds files in the engine directory". Mid-session, during a reinstall while JxBrowser may hold engine files — exactly the Windows/AV scenario its comment names — target.renameTo(backup) fails, the helper logs a warning and returns without touching the target. The code then does check(targetDir.resolve("executable.name").exists()), which passes because the old engine is still in place — so a discarded fresh download is reported as a successful install, and version.txt still shows the old version. I traced all three of the helper's warn-and-return paths; two of them produce this false "installed" outcome.
2. The rename failure is the expected case at this call site, not an edge. fix(browser-engine): make the engine reinstall atomic with rollback, and fail fast when macOS ditto extraction breaks #946's atomicEngineSwap makes the same guarantee (extract to sibling, swap with backup, restore on any failure) but makes every failure path throw so the caller can report it, and its tests cover extraction-failure, verification-failure-rollback, and happy-path-cleanup. fix(browser): keep the working engine until the reinstall swaps in #942 adds startup recovery for an interrupted swap; fix(browser-engine): recover an interrupted engine swap at startup - restore the .old backup and reclaim the crashed .new sibling #977 extends that. Security: the Chromium engine reinstall deletes the only working engine before extraction, and the macOS ditto fallback silently produces a non-booting engine #910 is well covered by that family.
3. ditto: throwing is a good direction, but it is now the only path. extractWithDitto now error(...)s instead of falling back to the Java extractor; that's the right call for silently producing non-booting framework symlinks, and it's worth keeping — but note it makes ditto a hard dependency on macOS and the test matrix only pins the extract-failure path, not a missing-binary path.

What I verified directly (this machine, git emulations): the origin/-f → git checkout -f -- force-discard reproduction is real on current dev, and the gate in #930/#934/#950 closes it; the tab/CR/NUL scheme-smuggling vectors against a blocklist are real. So the security findings behind all of this are legitimate — this PR is being parked for the structural reasons above, not because the issues don't exist.

Suggested next steps for you (and for the family):

• Close this PR in favor of the dedicated PRs: engine → fix(browser-engine): make the engine reinstall atomic with rollback, and fail fast when macOS ditto extraction breaks #946 (atomic swap + rollback) and fix(browser): keep the working engine until the reinstall swaps in #942/fix(browser-engine): recover an interrupted engine swap at startup - restore the .old backup and reclaim the crashed .new sibling #977 (interrupted-swap recovery), which carry the regression tests for the false-success path; SnapshotManager: path traversal in processId escapes snapshots directory, and save uses non-atomic writes #873/SettingsServiceImpl: concurrent setSetting calls race on uncoordinated writes, and saveToDisk uses non-atomic truncation #871 → already on dev; Security: the out-of-process EditorService saveFile tears user files on crash and its path gate misses Windows system paths and symlink escapes #885 → fix(security): implement atomic save and canonical path validation in EditorServiceImpl (#885) #904; Security: a credential inside a URI authority reaches the approval dialog and the ledger unmasked #886 → fix(security): redact URI credentials, curl user flags, AWS keys, and PEM keys in McpArgumentSanitizer (#886) #898; Security: the browser service accepts, executes, and logs javascript: URLs verbatim (and a stuck-loading Reload state rides along) #911 → the allowlist PRs (fix(browser): gate javascript: and unknown-scheme URLs before execution or logging #931 is being reviewed this sweep).
• If you want the engine work attributed here rather than closed empty, the cleanest path is a single-commit PR off current dev containing only the ditto fail-fast change plus a ChromiumAutoDownloaderTest case for the promote-fails-mid-session scenario — that part is genuinely not yet on dev.

Marking this PR draft; the branch is untouched, so you (or a maintainer) can rework it or close it.

[fluck-boss]

Thanks so much for this contribution, @KhushiTrivediii - the work here is genuinely appreciated, and it's clear real care went into it.

This overlaps with #1386 ("fix(chromium): keep existing engine until new one is verified (fixes #910)"), which is further along in review and will carry this change forward. To keep review attention focused, I'm closing this one in favor of #1386 - please do review and follow that PR, and you're very welcome to pick up any part of it that still needs help or take on another open issue.

Your contribution credit is noted here, and anything unique in this branch that #1386 misses is still welcome as a follow-up. Thanks again for contributing to BossConsole!