Add Deno and macOS Keychain smoke coverage#847
Open
bjesuiter wants to merge 26 commits into
Open
Conversation
Autoreview found regressions in the KeychainManager ownership rewrite for fix-access. The ownership path read and deleted items from an explicit keychain, then recreated them in the default keychain. It also rejected valid service-only refs, could delete matching internet-password items while rewriting a generic password, and used a lossy security CLI fallback that could strip trailing newlines from secrets. Batch fix-access also unlocked all keychains before processing any item, so one stale or unavailable keychain could abort the whole batch instead of failing only that ref. Fix this by preserving keychainName on generic-password writes, resolving account-less refs only when unambiguous, reading exact secret data through SecItemCopyMatching for ownership, deleting only the targeted generic-password item, falling back to legacy ACL handling for unsupported cases, and moving batch unlock into per-item handling. Validation: bun run lint:fix; bun run --filter varlock build:binary; autoreview --mode local --engine codex reported no accepted/actionable findings.
Add Deno execution support to the shared smoke-test runner so the existing smoke tests can be reused under deno run -A instead of maintaining a separate deno-only test file. Also teach varlock completion generation how to emit shell completion scripts when invoked through Deno, and route keychain-set through the shared runner so it participates in the Deno smoke path.
When takeOwnership is scoped to a named keychain, ownership reads and deletes must not fall back to the default keychain search list if that name cannot be resolved. A typo in --keychain could otherwise read or delete a matching item from another keychain before failing during recreation.\n\nResolve the requested keychain explicitly for both the ownership read and delete paths, and throw keychainNotFound when resolution fails.
takeOwnership has to delete and recreate the generic password item so VarlockEnclave becomes the owner. If recreation failed after the delete, fix-access could permanently lose the user's secret.\n\nCatch recreation failures, immediately attempt to write the saved service/account/value back, and return a specific ownershipTransferFailed error that reports whether the secret value was restored. This keeps fix-access from silently turning a repair attempt into destructive migration.
Change takeOwnership to avoid deleting the original item until a replacement has been created and verified. The flow now creates a temporary generic-password item with the same secret, reads it back to verify it is usable, deletes the original item only after that check succeeds, renames the temporary item to the original service/account, then verifies the final item.\n\nIf the swap fails after the original delete, keep the previous recovery behavior: restore the saved secret value at the original service/account when possible, and report whether that recovery succeeded.
This was referenced Jun 27, 2026
Restore fix-access to the legacy ACL-editing behavior so it preserves existing per-app Keychain access on existing items. Move the delete-and-recreate ownership rewrite behind a separate keychain take-ownership command. The new command is explicit about the ACL-reset tradeoff and still preserves named-keychain targeting when it recreates generic-password items. Validation: bun run lint:fix; bun run --filter varlock build:binary; bun run --filter varlock typecheck; autoreview --mode local --engine codex reported no accepted/actionable findings.
Add a macOS Keychain smoke test for the explicit take-ownership path and include it in the keychain smoke scripts. Validation: bun run lint:fix; bun run --filter varlock build:binary; VARLOCK_RUN_KEYCHAIN_SMOKE=1 bunx vitest run tests/keychain-ownership.test.ts from smoke-tests.
Before running keychain fix-access, assert that a restricted smoke-test item is not readable yet. Add timeout support to the smoke-test runner so the pre-fix read cannot hang indefinitely on Keychain authorization. Validation: bun run lint:fix; bun run --filter varlock build:binary; VARLOCK_RUN_KEYCHAIN_SMOKE=1 bunx vitest run tests/keychain-fix.test.ts from smoke-tests.
This reverts commit 8b1c78d.
Add a useFallback option to keychain() reads. It defaults to true to preserve the existing /usr/bin/security fallback, but useFallback=false now requires the Swift daemon to read through Apple's framework APIs directly. Use the option in the keychain fix-access smoke fixture so the test proves fix-access grants VarlockEnclave direct framework access instead of succeeding via the security CLI fallback. Validation: bun run lint:fix; bun run --filter varlock typecheck; bun run --filter varlock build:binary; VARLOCK_RUN_KEYCHAIN_SMOKE=1 bunx vitest run tests/keychain-fix.test.ts from smoke-tests; autoreview --mode local --engine codex.
Kill and clear Varlock daemon state before and after every macOS Keychain smoke test so stale daemon access state cannot affect prompt behavior or test results. Validation: bun run lint:fix; bun run --filter varlock build:binary; VARLOCK_RUN_KEYCHAIN_SMOKE=1 bunx vitest run tests/keychain-fix.test.ts from smoke-tests.
Replace the separate post-test keychain daemon cleanup helper with resetVarlockDaemon in both beforeEach and afterEach. The important behavior is killing the daemon and clearing pid/socket state; directory removal is unnecessary. Validation: bun run lint:fix; bun run --filter varlock build:binary; VARLOCK_RUN_KEYCHAIN_SMOKE=1 bunx vitest run tests/keychain-fix.test.ts from smoke-tests.
bjesuiter
force-pushed
the
deno-compatibility
branch
from
5e42121 to
bdd1a0a
Compare
Member
|
I'm not sure about deferring to Also I had thought that the main reason to read from existing keychain items is the case where a user is already using that item in other contexts, or for example where an IT department is managing those items for the user. In this case I dont think recreating the item will be a good idea. One thing to note - it could be that you are seeing more prompts than would actually occur since you are not signing the built binary whereas the real one will be signed. |
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
2 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
keychain fix-accesstake ownership of imported/external items instead of editing legacy ACLsSmoke tests
VARLOCK_RUN_KEYCHAIN_SMOKE=1and cover:keychain setwriting and reading a daemon-owned itemkeychain importmigrating plaintext.envvalues into Keychain refs, then deleting the source.envbefore readbackKeychain reasoning
Legacy ACL editing (
SecKeychainItemCopyAccess/SecKeychainItemSetAccess) kept causing repeated macOS password prompts and was hard to explain.fix-accessnow treats the item as an external secret to migrate: read current value, delete original item, recreate through VarlockEnclave. That makes the daemon the item owner and avoids repeated future password prompts.Reads still try the daemon path first;
/usr/bin/securityis fallback and is used by take-ownership to read externally-owned originals reliably.Validation
bun run lint:fixbun run --filter varlock typecheckbun run --filter @varlock/encryption-binary-swift build:currentVARLOCK_RUN_KEYCHAIN_SMOKE=1 bun run --cwd smoke-tests test tests/keychain-fix.test.ts