Skip to content

Block background Chromium Keychain access - #2225

Merged
steipete merged 2 commits into
steipete:mainfrom
Yuxin-Qiao:agent/block-background-browser-keychain
Jul 16, 2026
Merged

Block background Chromium Keychain access#2225
steipete merged 2 commits into
steipete:mainfrom
Yuxin-Qiao:agent/block-background-browser-keychain

Conversation

@Yuxin-Qiao

Copy link
Copy Markdown
Contributor

Summary

  • block Chromium browser-cookie imports during background provider refreshes
  • preserve browser-cookie access for user-initiated refreshes and the existing Keychain pre-alert flow
  • update gate tests to prove background paths do not even perform the Keychain preflight

Root cause

Chromium cookie decryption reads Chrome Safe Storage from Keychain. The previous background path could proceed into the browser importer whenever an attribute-only preflight appeared allowed, even though the later password-data read could require user interaction.

Impact

Automatic refreshes no longer initiate the Chromium Keychain path. A user-initiated refresh remains the consent boundary for browser-cookie authentication.

Validation

  • targeted test changes use KeychainAccessPreflight test overrides and do not touch a real Keychain item
  • git diff --check
  • the full local SwiftPM test executable was blocked by unrelated auxiliary-target timestamp churn after the modified core module compiled; this is recorded rather than treated as a passing test

Follow-up

SweetCookieKit is being hardened separately so no host can implicitly promote a no-UI Safe Storage read to an interactive one. Once released, CodexBar should bump that dependency.

Fixes #2213
Related: #2214

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jul 16, 2026
@clawsweeper

clawsweeper Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 16, 2026, 2:11 PM ET / 18:11 UTC.

Summary
Blocks Chromium browser-cookie imports during background refresh before Keychain preflight while preserving user-initiated imports and updating gate tests.

Reproducibility: no. high-confidence packaged reproduction is available. Source inspection establishes the prompt-capable path and override tests exercise the gate, but neither the original screenshot nor the current tests demonstrate the real macOS behavior end to end.

Review metrics: 1 noteworthy metric.

  • Patch surface: 2 files; 23 additions, 11 deletions. The diff is small, but its shared authentication gate can affect every provider that falls back to Chromium cookies.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Post redacted packaged-app logs or a recording showing that automatic refresh performs no Chromium Keychain preflight or prompt while manual refresh still imports cookies.
  • Confirm the standalone CLI Chromium-cookie path remains functional and document the observed command output without exposing account or credential data.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Only override-based tests are supplied; add redacted packaged-macOS logs or a recording showing background suppression and successful explicit app and CLI access, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

  • [P1] Existing users who rely on Chromium cookies for automatic provider refresh may receive stale usage or authentication failures until they perform a manual refresh; this intentional compatibility change needs upgrade proof and maintainer visibility.
  • [P1] Because the gate is shared, a standalone CLI path that retains the default background interaction context could also lose Chromium cookie imports even though the stated boundary is app-background refresh.

Maintainer options:

  1. Prove every interaction boundary (recommended)
    Before merge, provide redacted packaged-macOS evidence that background refresh avoids Chromium Keychain access while manual app refresh and standalone CLI cookie authentication remain functional.

Next step before merge

  • [P1] Do not queue an automated repair: no definite code defect is established, while contributor-supplied packaged-macOS proof and explicit app/CLI compatibility confirmation remain required before merge.

Security
Cleared: The diff narrows Keychain access and introduces no dependency, permission, secret-handling, downloaded-code, or supply-chain expansion.

Review details

Best possible solution:

Keep one shared no-prompt background guard, explicitly classify manual app refresh and standalone CLI invocations as interactive, cover all three contexts with focused tests, and demonstrate the packaged macOS result without accessing private credentials in automated tests.

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

No high-confidence packaged reproduction is available. Source inspection establishes the prompt-capable path and override tests exercise the gate, but neither the original screenshot nor the current tests demonstrate the real macOS behavior end to end.

Is this the best way to solve the issue?

Yes, conditionally: placing the restriction at the shared browser-cookie gate is narrow and maintainable, provided explicit app and CLI operations are confirmed to use a user-initiated context and upgrade behavior is proven.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against f9993821856e.

Label changes

Label justifications:

  • P2: This is a bounded privacy and authentication-flow correction with limited but cross-provider user impact.
  • merge-risk: 🚨 compatibility: The shared gate intentionally removes automatic Chromium-cookie access and may also affect explicit paths if their interaction context is not set correctly.
  • merge-risk: 🚨 auth-provider: Suppressing Chromium cookie imports can change provider authentication fallback and cause stale or unavailable usage data.
  • 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: Only override-based tests are supplied; add redacted packaged-macOS logs or a recording showing background suppression and successful explicit app and CLI access, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.
Evidence reviewed

What I checked:

  • Shared gate change: The branch adds an early return for Chromium-class browsers whenever the interaction context is not user initiated, before the Keychain preflight runs. (Sources/CodexBarCore/BrowserCookieAccessGate.swift:85, cc661d7ecce5)
  • Focused regression coverage: The tests now distinguish background and user-initiated contexts and assert that background Chromium access skips the preflight while Safari remains available. (Tests/CodexBarTests/BrowserDetectionTests.swift:249, cc661d7ecce5)
  • Proof remains simulated: The PR body reports TaskLocal and Keychain preflight overrides rather than a packaged app run, and the latest discussion contains no contributor-supplied runtime artifact after the prior proof request. (cc661d7ecce5)
  • Repository validation policy: Repository guidance prohibits unattended prompt-capable Keychain validation and requires separate packaged-host evidence before claiming that a reported prompt issue is resolved. (AGENTS.md, f9993821856e)
  • Related scope: The branch is a candidate fix for the unexpected prompt report, while the linked provider-neutral credential-policy proposal remains a broader and distinct product discussion.

Likely related people:

  • steipete: The repository owner marked the revised branch ready for review and is the strongest routing candidate for the intended app-versus-CLI interaction boundary. (role: repository owner and recent review participant; confidence: high; files: Sources/CodexBarCore/BrowserCookieAccessGate.swift, Sources/CodexBarCore/ProviderInteractionContext.swift)
  • manikv12: Merged history credits this contributor with substantial Claude OAuth, Keychain, and background-refresh work adjacent to the credential-access policy affected here. (role: prior keychain-flow contributor; confidence: medium; files: Sources/CodexBarCore, Sources/CodexBar)
  • artuskg: Merged history credits this contributor with strengthening no-UI Keychain reads, which is directly adjacent to the prompt-suppression invariant in this PR. (role: prior no-UI keychain contributor; confidence: medium; files: Sources/CodexBarCore)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.
Review history (2 earlier review cycles)
  • reviewed 2026-07-16T14:57:24.114Z sha 17c8729 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-16T16:53:17.812Z sha cc661d7 :: needs real behavior proof before merge. :: none

@steipete
steipete marked this pull request as ready for review July 16, 2026 18:05
@steipete
steipete merged commit bad4beb into steipete:main Jul 16, 2026
8 checks passed
@steipete

Copy link
Copy Markdown
Owner

Merged as bad4beb.

Verification:

  • Audited the complete two-file diff. The guard only blocks background access for browsers whose cookie decryption uses Keychain; Safari, Firefox, and Zen remain allowed, and user-initiated Chromium access remains allowed.
  • Ran swift test --filter BrowserDetectionTests: 28 tests passed, 0 failures.
  • Autoreview completed with no actionable findings.
  • Exact head cc661d7 passed GitGuardian, lint, changes, both Linux CLI jobs, lint-build-test, and both macOS shards. macOS shard durations: 27m09s and 27m17s.
  • GitHub reported MERGEABLE/CLEAN immediately before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

why will codexbar request my keychain permission?

2 participants