fix(keychain): keep completed signature validations fresh

[jeffloo886]

A successful trusted-application validation could be reused for 30 seconds after a sealed bundle resource or non-version Info.plist field changed. The cache fingerprint only observes executable and selected bundle metadata, so that later preflight could incorrectly report .allowed, defeating the extra protection against legacy Keychain prompts.

This follow-up builds on the cache implementation landed in #3867 and preserves the original PR history. Completed successes now revalidate on the next operation. Concurrent requests still share an in-flight validation, confirmed CSSMERR_CSP_VERIFY_FAILED rejections retain their 64-entry/five-minute cache, and transient results remain uncached. The existing short synchronous generic-password operation memo remains intact; its scope is not widened across asynchronous refreshes or deferred tasks.

Thanks @jeffloo886 for identifying the sealed-resource freshness issue and updating the original proposal. Human contributor credit is retained in the fix commit and changelog.

Validation

• New synthetic resource and non-version plist regressions failed four assertions against the previous code, then passed with the fix. They verify a fresh validator call and a blocked simulated background gate.
• 171 focused tests across 34 suites passed, covering Keychain policy, browser gates, short-operation reuse, concurrent sharing, rejection expiry/invalidation, capacity, and transient statuses.
• make check passed with zero violations in 2,570 Swift files.
• Independent review of the final change is scoped-clean through P2.
• Full make test passed: all 1,334 selections in 120 groups succeeded on the first pass, with zero retries or timeouts.
• Exact-head CI passed on d535daba16e119e0421d1cd52bf03c1e26310add, including both macOS shards and every Linux CLI target. GitGuardian also passed.

The new regressions inject signature results and Keychain preflight outcomes. No real SecItem access, account credentials, browser-cookie imports, or live Keychain prompts were used. This demonstrates stale preflight/prompt risk; it does not claim an OS credential-authorization bypass or a reproduced live prompt.

Refs #3837.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed September 22, 2026, 3:31 PM ET / 19:31 UTC (Revision 7).

ClawSweeper review

What this changes

The PR stops caching completed successful Keychain signature checks while preserving concurrent sharing and rejection caching, and updates regression tests and documentation.

Merge readiness

⛔ Blocked before merge - 2 items remain

Keep open: current main still retains successful validations for 30 seconds, and the owner explicitly preserved this follow-up. No actionable patch defect remains, but the native after-fix proof requested in the previous review is still outstanding.

Priority: P2
Reviewed head: d535daba16e119e0421d1cd52bf03c1e26310add

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The focused patch and regression coverage are sound, but real native after-fix evidence remains a merge gate.
Proof confidence	🦪 silver shellfish (2/6)	Needs real behavior proof before merge: The changed validation memo is exercised through injected signature results and a simulated preflight feeding the real browser gate. This verifies cache behavior but does not show the production native validator accepting a valid fixture and freshly rejecting it after sealed-resource mutation. The earlier async-scope proof request no longer applies. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Needs proof	Needs real behavior proof before merge: The changed validation memo is exercised through injected signature results and a simulated preflight feeding the real browser gate. This verifies cache behavior but does not show the production native validator accepting a valid fixture and freshly rejecting it after sealed-resource mutation. The earlier async-scope proof request no longer applies. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed	8 items	Pinned production change: The introduced delta removes successLifetime and stores only confirmed signature rejections. Flight completion, concurrent sharing, cache capacity, and transient-result handling remain intact. The verified test-merge tree has no difference from the reviewed head. Still needed on main: The pinned main implementation caches errSecSuccess for 30 seconds and keys bundle identity using selected metadata, without sealed-resource contents. It therefore does not implement this follow-up's completed-success freshness. Production boundary and security safeguards: trustedApplication invokes the native validator through the memo. ACL evaluation still distinguishes success, rejection, and indeterminate results; browser imports require an allowed preflight and retain the noninteractive read wrapper. The patch shortens retained approval lifetime without adding credentials, permissions, dependencies, or persistent storage.
Findings	None	None.
Security	None	None.

How this fits together

CodexBar checks whether a Keychain item's trusted applications permit its executable before attempting background credential access. The validation memo shares signature checks, and its results feed the browser-cookie gate that allows or skips imports.

flowchart TD
  A[Background cookie import] --> B[Keychain ACL preflight]
  B --> C[Trusted application and executable]
  C --> D[Signature validation memo]
  D --> E[Share pending check or validate afresh]
  E --> F[Allow or skip import]
  F --> G[Noninteractive credential read]

Loading

Before merge

• Add real behavior proof - Needs real behavior proof before merge: The changed validation memo is exercised through injected signature results and a simulated preflight feeding the real browser gate. This verifies cache behavior but does not show the production native validator accepting a valid fixture and freshly rejecting it after sealed-resource mutation. The earlier async-scope proof request no longer applies. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
• Complete next step (P2) - Add native after-fix proof using a disposable signed fixture through the production validation path, showing success followed by fresh rejection after resource mutation without credential reads or Keychain prompts. Terminal output or redacted logs are sufficient; a terminal screenshot or recording is also welcome. Redact private paths, endpoints, identifiers, and secrets. Updating the PR body should trigger re-review; otherwise ask a maintainer to comment @clawsweeper re-review.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Validation reuse expectations	100 serial calls → 100 validations; 20 concurrent calls → 1	The regression tests explicitly document the intended freshness tradeoff while preserving concurrent sharing; these are synthetic expectations, not runtime performance measurements.

Technical review

Best possible solution:

Keep successful validations fresh between completed operations while retaining concurrent sharing, bounded rejection caching, and the existing noninteractive credential-read boundary.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: main can reuse a completed success after a resource change that leaves its metadata key unchanged. The synthetic regressions isolate this mechanism; a native current-main reproduction was not executed.

Is this the best way to solve the issue?

Yes. Removing retained successes is a narrow repair because the existing metadata key cannot establish complete sealed-resource freshness, and the patch preserves the existing concurrency and rejection mechanisms.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 173afc88a417.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This is a bounded correction to background Keychain preflight freshness, with no established credential-authorization bypass or widespread outage.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The changed validation memo is exercised through injected signature results and a simulated preflight feeding the real browser gate. This verifies cache behavior but does not show the production native validator accepting a valid fixture and freshly rejecting it after sealed-resource mutation. The earlier async-scope proof request no longer applies. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

• Pinned production change: The introduced delta removes successLifetime and stores only confirmed signature rejections. Flight completion, concurrent sharing, cache capacity, and transient-result handling remain intact. The verified test-merge tree has no difference from the reviewed head. (Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift:69, d535daba16e1)
• Still needed on main: The pinned main implementation caches errSecSuccess for 30 seconds and keys bundle identity using selected metadata, without sealed-resource contents. It therefore does not implement this follow-up's completed-success freshness. (Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift:70, 173afc88a417)
• Production boundary and security safeguards: trustedApplication invokes the native validator through the memo. ACL evaluation still distinguishes success, rejection, and indeterminate results; browser imports require an allowed preflight and retain the noninteractive read wrapper. The patch shortens retained approval lifetime without adding credentials, permissions, dependencies, or persistent storage. (Sources/CodexBarCore/KeychainAccessPreflight.swift:412, d535daba16e1)
• Regression coverage and proof boundary: The new resource and plist tests use unsigned synthetic files and injected signature statuses, then exercise the real ACL outcome mapper and browser gate through a preflight override. The body reports four failing assertions before the fix, 171 focused tests, make check, and the full suite passing afterward; these are contributor-reported supplemental results, not native runtime proof. No tests or builds were executed during this read-only review. (Tests/CodexBarTests/KeychainAccessValidationMemoTests.swift:85, d535daba16e1)
• Maintainer disposition and review continuity: The owner explicitly reopened this PR to preserve the newer freshness concern: fix(keychain): keep completed signature validations fresh #3838 (comment). The supplied previous review covers the identical head and has no findings, with one remaining request for native sealed-resource rejection proof. Current source has only the short synchronous operation memo; the earlier async-scope proposal is absent. The complete supplied body and current live body agree about using injected validation outcomes. (d535daba16e1)
• Inspected media does not cover signature validation: Both prepared images were inspected locally. They show synthetic Antigravity quota charts attached to fix: correct usage history, account routing, and quotas #3867, not this PR's native signature-validation behavior. The captured review source identity is 43d90e8ee05bccf58df4e52a6b239497fa8b8350274a5569191a3439af7876b3.

Likely related people:

• Peter Steinberger: Raw commit 2f96556 adds Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift:8 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 2f96556accb4; files: Sources/CodexBarCore/KeychainAccessPreflight+ValidationMemo.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Provide redacted native output showing successful validation followed by fresh rejection after a sealed-resource change through the changed production path, using disposable fixtures without real credential reads.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (6 earlier review cycles)

• reviewed 2026-09-21T17:27:57.500Z sha 91d8589 :: needs real behavior proof before merge. :: [P1] Key cached verdicts by the actual signing requirement | [P1] Invalidate cached results when validated bundle resources change
• reviewed 2026-09-22T01:46:35.482Z sha 2350385 :: needs real behavior proof before merge. :: [P1] [P1] Invalidate successful verdicts when sealed resources change | [P2] [P2] Leave indeterminate validator failures uncached
• reviewed 2026-09-22T02:09:34.308Z sha bd7ab6b :: needs real behavior proof before merge. :: [P1] [P1] Invalidate successful verdicts when sealed resources change
• reviewed 2026-09-22T06:16:55.968Z sha 7881bce :: needs real behavior proof before merge. :: [P1] [P1] Expire the shared memo when the async refresh scope ends
• reviewed 2026-09-22T18:45:55.965Z sha 7881bce :: needs real behavior proof before merge. :: [P1] [P1] Expire the shared memo when the async refresh scope ends
• reviewed 2026-09-22T19:11:47.643Z sha d535dab :: needs real behavior proof before merge. :: none

[jeffloo886]

Update: fixed both P1 findings

Verified both against Apple's actual open-source implementation (apple-oss-distributions/security, OSX/libsecurity_keychain/lib/SecTrustedApplication.cpp and TrustedApplication.cpp), not just the review's characterization, and reproduced each defect directly before fixing it.

Finding 1 — cache-key collision (confirmed and fixed)

SecTrustedApplicationCopyData's implementation is exactly:

const char *path = TrustedApplication::required(appRef)->path();
Required(dataRef) = CFDataCreate(NULL, (const UInt8 *)path, strlen(path) + 1);

— only the stored path, never the embedded SecRequirementRef (or legacy hash) that verifyToDisk actually evaluates via SecStaticCodeCheckValidity(ondisk, kSecCSDefaultFlags, requirement).

Reproduced the exact collision with two real SecTrustedApplications built from the same path but different SecRequirements:

CopyData (old key):
  requirement A: /Applications/SharedInstallPath.app/Contents/MacOS/Shared
  requirement B: /Applications/SharedInstallPath.app/Contents/MacOS/Shared
  COLLIDE: True

CopyExternalRepresentation (new key):
  requirement A: len=136 sha=07bed273477149f4
  requirement B: len=136 sha=21bb04c6e60e321b
  DISTINGUISHES: True

Fix: the cache key now uses SecTrustedApplicationCopyExternalRepresentation, which serializes the full ACL subject (requirement included), instead of CopyData.

New regression test same path with different signing requirements is not a cache collision builds this exact scenario. Falsification check: temporarily reverted just the key source back to CopyData (keeping everything else) — the test fails as expected (callCount == 1, wanted 2); on the real fix it passes (callCount == 2), and all 6 other tests in the suite were unaffected by that revert, confirming the test isolates only this defect.

Finding 2 — resource-tamper staleness (bounded, not eliminated)

SecStaticCodeCheckValidity's default flags validate the bundle's full sealed-resource envelope, not just the executable file, so a resource edited without touching the executable left the identity-based key unchanged. Cheaply fingerprinting the whole envelope would mean re-doing the expensive walk the cache exists to avoid.

Added a 5-minute TTL on cache entries — matching the retry deadline #3301 already established in this file for the adjacent rejected-ACL cooldown — so this gap (and an ACL repaired after a rejection) is bounded to a fixed window instead of the life of the process. Executable-identity invalidation is unaffected and still fires immediately for the common case (a replaced binary).

New regression test a cached verdict expires after the TTL and is revalidated covers the boundary (still cached at TTL-1s, revalidated past TTL+1s).

Verification

• swift test --filter 'KeychainTrustedApplicationValidationCacheTests|KeychainCacheApplicationPathTests|KeychainNoUIQueryTests|KeychainAccessPreflightRetryTests' — 24 tests, 4 suites, passed.
• swift test --filter 'Keychain|BrowserCookie|CookieImporter' — 221 tests, 40 suites, passed (no regressions in the broader surface this touches).
• ./Scripts/lint.sh lint — 0 violations, 2549 files.

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[jeffloo886]

Update (commit bd7ab6b): fixed both new findings from Revision 2

P1 — stale success no longer possible for up to 5 minutes

Agreed the risk asymmetry is real and shouldn't be papered over with one shared TTL: a rejection surviving past its truth only delays a legitimate read (the exact tradeoff #3301's cooldown already accepted for this file's adjacent case), but a success surviving past its truth wrongly authorizes a preflight that should now fail. Those are not the same risk and don't belong on the same clock.

Considered and rejected three other options first:

• No TTL for success at all — reverts to the original, worse problem (unbounded staleness for the life of the process).
• A cheap full-bundle fingerprint — no such thing exists; the only accurate signal is re-running the expensive resource-hash walk itself, which is what the cache exists to avoid.
• Task-scoped caching per refresh cycle (extending the file's existing withMemoizedGenericPasswordChecks pattern) — the architecturally cleanest fix, but it means wiring ~29 provider call sites (or finding a single shared task-group entry point I don't have high confidence exists) instead of one file, which is a different-sized PR than a leaf-level fix.

Landed on: split the TTL by outcome. successCacheTTL = 30s, rejectionCacheTTL = 5 * 60s (unchanged, matches #3301). 30s is justified by evidence already in this PR's profiling — bursts measured under 20 seconds — with headroom, and it's well short of the 60-second refresh interval so a success can never bridge into the next scheduled refresh. New test a cached rejection uses the longer rejection TTL, not the success TTL proves the two are genuinely on separate clocks, not the same constant renamed twice.

I don't think this fully closes the "owner decision" you flagged — a 30-second window is still a window, and if the maintainer wants zero tolerance for stale success, the task-scoped refactor is the real answer. Flagging that explicitly rather than presenting 30s as if it settles the question.

P2 — indeterminate/transient statuses are never cached

Confirmed this was a real, unambiguous bug, missed in both earlier reviews. cacheTTL(for:) now returns nil for anything other than errSecSuccess or a confirmed CSSMERR_CSP_VERIFY_FAILED; a nil TTL means the outcome is returned to the caller but never written to the cache, so it always re-validates and never fights with checkGenericPasswordUncached's existing 3-attempt retry.

New test an unrecognized validator status is never cached reproduces this against a real (non-hypothetical) status the validator can actually return — pointing at a temp directory (exists on disk, so it has a valid filesystem identity and a cache key can form, but isn't a codesign-able executable) reliably produces a distinct OSStatus from either cacheable outcome; the test confirms it re-validates on every call.

Verification

Falsification checks on both new tests (temporarily reverted just the relevant logic, confirmed each fails, restored):

• an unrecognized validator status is never cached — fails under unconditional caching (count stays 1, never increments).

• a cached rejection uses the longer rejection TTL, not the success TTL — fails when both outcomes share one clock (expires early).

• swift test --filter 'KeychainTrustedApplicationValidationCacheTests' — 9 tests, 1 suite, passed.

• swift test --filter 'Keychain|BrowserCookie|CookieImporter' — 223 tests, 40 suites, passed. (One unrelated test, BrowserCookieAccessGateTests — a file this PR does not touch — flaked once on a real-clock timestamp comparison in the first run after this change; reran that suite in isolation 5 times, passed clean every time, consistent with pre-existing timing sensitivity rather than a regression.)

• ./Scripts/lint.sh lint — 0 violations, 2549 files.

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[clawsweeper]

🦞👀
Exact review queued.

Re-review progress:

• State: Waiting
• Detail: A newer exact-review queue owner superseded this run. This run completed as a no-op.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/35693570577
• Updated: 2026-09-22T06:12:52.557Z

[jeffloo886]

Addressed the stale-success concern from the previous review in commit 7881bce.

• Removed process-wide caching of successful trusted-application validation. SecStaticCodeCheckValidity covers sealed bundle resources, and executable metadata cannot reliably detect every resource mutation.
• Successful checks are now deduplicated only inside one complete provider refresh via the existing task-local generic-password memo; the memo is discarded when that refresh ends.
• Retained the bounded 5-minute cache only for confirmed CSSMERR_CSP_VERIFY_FAILED rejections. Transient/unrecognized statuses remain uncached.
• Verification: swift test --filter KeychainTrustedApplicationValidationCacheTests (9 passed), swift test --filter "Keychain|BrowserCookie|CookieImporter" (223 passed), and Scripts/lint.sh lint (2549 Swift files, 0 violations). No real Keychain prompts or credential reads were used.

@clawsweeper re-review

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

[steipete]

Rewritten in #3857 (dfccffeb1bef) with your contribution credited in the commit and changelog. The replacement coalesces concurrent misses, includes enclosing bundle metadata, leaves transient statuses uncached, and preserves other valid entries during eviction. Regression tests and CI passed. #3857 includes Closes #3838 and Fixes #3837. Thanks @jeffloo886.

[steipete]

Reopening this PR to preserve the newer 7881bcee revision. #3867 contains the replacement based on the previously inspected bd7ab6b snapshot, but its inherited closing keyword automatically closed this PR during the rebase merge despite the PR body keeping it open.

The newer sealed-resource invalidation concern still needs to be checked against the landed cache implementation. This revision remains under review.