macOS updater can no longer orphan saved logins — keychain item untouched, fallback re-sign strictly verified (salvage #90961) - #95283
Merged
Merged
Conversation
…mpt on every launch The self-updater rebuilds the desktop app locally via electron-builder after every update (4aa9f73). On macOS, the rebuilt app gets ad-hoc signed, producing a different cdhash than the original CI-signed build. macOS ties the 'Hermes Safe Storage' keychain item's ACL to the code signature, so the new signature doesn't match → macOS re-prompts for keychain access on every launch. After re-signing, delete the existing keychain item so Electron recreates it with the correct ACL for the newly-signed app on next launch. The trade-off: previously encrypted tokens become unreadable (the user re-enters the gateway token once), but the keychain prompt stops appearing on every launch. The delete-generic-password command doesn't require reading the secret (no ACL check), so it runs without prompting.
The previous commit deleted the 'Hermes Safe Storage' keychain item after every successful re-sign, including the stable certificate-anchored identity path. On that path the designated requirement is stable across rebuilds, so after the first launch under the new identity the keychain ACL already matches; deleting the item on every update permanently orphaned gateway-token and native-OAuth credentials that were working fine (both are safeStorage-backed: electron/main.ts connection config and native-oauth-tokens.json). Addresses review feedback on #90961: - Rename _desktop_macos_update_keychain_acl -> _desktop_macos_reset_keychain_safe_storage (it deletes, it does not update an ACL). - Only invoke it on the legacy ad-hoc fallback path, where every rebuild produces a new cdhash so the ACL can never match and the alternative is a recurring prompt. The trade-off (re-enter credentials once per update) is documented; the durable fix is a stable signing identity. - Add regression tests: stable path must NOT reset, ad-hoc fallback MUST.
The fixup no-ops on non-macOS (sys.platform guard), so the new regression tests must carry the same @pytest.mark.macos_only marker as their siblings (test_relaunchable_fixup_falls_back_to_legacy_adhoc_on_failure). Without it the legacy-adhoc test failed on the Linux CI runner where the fixup returns True before reaching the reset path.
Addresses round-2 review feedback on #90961. The previous commits scoped the keychain deletion to the legacy ad-hoc fallback, but the reviewer correctly held the blocker: the fallback ran codesign with check=False, ignored the result, and unconditionally deleted 'Hermes Safe Storage' — permanently orphaning gateway and native OAuth credentials even when signing failed or a configured identity had failed and routed into the fallback. This commit removes the deletion entirely: - _desktop_macos_reset_keychain_safe_storage is gone; no code path touches the keychain item anymore. - The legacy fallback now checks the codesign result and runs codesign --verify --deep --strict; any failure leaves the item untouched and prints a warning. - The keychain prompt after an ad-hoc re-sign is recoverable (Always Allow updates the ACL partition list and preserves the key); deletion is not. The durable proof-carrying migration belongs in Electron (safeStorage can read the old key) and is tracked as a follow-up. Tests: 4 witnesses (stable path, default no-config success, fallback failure, fallback success) all mutation-verified against both the deletion regression and the ignored-codesign-result regression.
…cceed The legacy ad-hoc fallback signed and verified successfully but still fell through to return False, contradicting the fixup's documented contract. The success witness codified the contradiction. Return True on the verified success path; the caller ignores the return value, so no behavior change beyond the contract correction.
૮ >ﻌ< ა ci reviewran on 14430b5 — fix(desktop): return True when fallback sign + strict verifi
|
alt-glitch
added
type/bug
This was referenced Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The macOS updater can no longer destroy saved logins: the desktop re-sign path never touches the "Hermes Safe Storage" keychain item, and the legacy ad-hoc codesign fallback now checks its own result and runs strict verification instead of silently reporting success. Salvage of #90961 by @DavidMetcalfe (all commits cherry-picked, authorship preserved — the branch history walks through and then removes an earlier deletion-based approach; the final tree contains zero keychain-touching code).
Context: since keychain encryption became opt-in (#95015), this protects opted-in users — for them the keychain item is the only key to their gateway token + native OAuth ciphertext, and deleting it (the earlier draft's approach, blocked twice in review) would orphan those credentials unrecoverably. The residual re-prompt for opted-in users is tracked in the re-scoped #91115 (ACL is cdhash-bound; proof-carrying rotation is the durable fix).
Changes
hermes_cli/main.py(@DavidMetcalfe):_desktop_macos_relaunchable_fixupfallback path — replaces the blindcodesign --force --deep --sign - (check=False)with result check →codesign --verify --deep --strictgate →return Trueonly on verified success; warn +return Falseotherwise. Never touches the keychain on any path.tests/hermes_cli/test_gui_command.py(@DavidMetcalfe): 4 mutation-verified macos_only witnesses (stable path never deletes, default no-config success never deletes, failed fallback never deletes/verifies, successful fallback verifies strictly) + flips the pre-existing fallback test to the new contract.Validation
--verify --deep --strictgatedtests/hermes_cli/test_gui_command.py: 41 passed, 9 skipped (macos_only witnesses platform-gated)check=Falsewith no verification); the reviewer thread on fix(desktop): never delete safeStorage keychain item during macOS re-sign; verify fallback strictly #90961 carries 4 rounds of review with mutation-verified witnesses and exact-head green CI at the salvaged revision.Credit: @DavidMetcalfe (author), @andrexibiza (review rounds that caught the deletion hazard). Closes #90961. Refs #90959, #91115.
Infographic