feat(auth): associate OAuth buckets with browser profiles (Fixes #1045)#2526
Merged
Conversation
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — PR #2526
|
Contributor
LLxprt PR Review – PR #2526Issue AlignmentThe implementation directly resolves #1045. OAuth bucket associations now persist a Side Effects
Code QualityStrengths:
Concerns:
Tests and CoverageCoverage impact: increase New/modified test files:
No mock-theater concerns: tests assert observable behavior and command output, not internal implementation details. VerdictReady The PR is well-tested, backward-compatible, and directly implements the requested feature. The Chrome-only CLI limitation is a minor scope constraint, not a blocker. All new behavior is guarded by validation and has meaningful automated tests. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
OAuth previously opened whatever browser was in the foreground. Now buckets
can be associated with a specific browser binary and profile directory
(Playwright-style launch), so each bucket opens the correct browser/profile
every time.
- openBrowserSecurely accepts optional {browser, profileDirectory} and
launches the specific browser binary with --profile-directory instead of
the OS default; full backward compatibility when no options are given
- browser-profile-discovery reads Chrome/Firefox profile display names and
directory names per platform (Local State / profiles.ini)
- BrowserProfileAssociationStore persists bucket->browser/profile mappings
- Anthropic and Codex providers look up the association for the active
bucket (via the runtime accessor bridge) and launch that browser/profile;
fall back to the OS default when no association exists
- AuthFlowOrchestrator calls provider.setAuthContext({bucket}) before
initiateAuth so the provider knows which bucket is being authenticated
- /auth <provider> create <bucket> discovers profiles and associates one;
/auth <provider> profile <bucket> manages associations
The auth command imported discoverBrowserProfiles and validateProfileDirectory from deep core subpaths (@vybestack/llxprt-code-core/utils/*.js). Those deep paths are not part of the published core package exports, so the consumer smoke test (npm pack -> install -> --version) failed to resolve the module, and the CLI import-boundary guard flagged them as violations. Import both from the public @vybestack/llxprt-code-core barrel instead, and update the test mock to target that specifier via importActual.
…n store Address automated review feedback on the browser-profile feature: - secure-browser-launcher: On Windows, Start-Process -ArgumentList was built as a single unquoted string, so PowerShell split it on whitespace and a Chrome profile directory named 'Profile 1' was truncated to 'Profile'. Build -ArgumentList as a PowerShell array of individually single-quoted elements via shared helpers (powershellQuote + buildStartProcessCommand) so spaces are preserved for both Chrome and Firefox. Add tests covering the space case and Firefox on Windows, and strengthen the macOS/Linux launch assertions to exact-order toEqual so argument-order regressions are caught. - browser-profile-association-store: tighten isAssociationFileData to validate each entry has the required browser + profileDirectory fields (malformed entries are now rejected instead of silently passing); extract a cloneAssociation helper to deduplicate the object-cloning in set/get/listAssociations; add a test for malformed-entry rejection; fix the createInMemoryFsMalformed naming typo.
… Linux browser fallbacks Address the second round of automated review feedback on the browser-profile feature: - secure-browser-launcher: extract a shared browserExecOptions constant used by both the default-browser and specific-browser paths so the two cannot diverge. Route the Start-Process executable through powershellQuote() for consistency and future safety. Add a Linux/BSD fallback chain for Chrome (google-chrome-stable, chromium, chromium-browser) and Firefox (firefox-esr) via a tryLinuxFallbackBinaries helper, so the feature works on distros that ship Chromium instead of google-chrome. Add a test covering the fallback walk. - browser-profile-association-store: validate the required numeric 'version' field in isAssociationFileData so persisted data missing it is rejected. Add a test for the missing-version case. - oauth-runtime-accessors: add explicit parameter types to getBrowserProfileAssociation to match the interface signature.
… BrowserKind, harden tests Address the third round of automated review feedback: - secure-browser-launcher: on Windows the specific-browser path now spawns the Chrome/Firefox executable directly via execFileAsync instead of constructing a PowerShell Start-Process command string, eliminating the string-based invocation boundary entirely for targeted launches. Removed the now-unused powershellQuote/buildStartProcessCommand helpers. Extracted a shared isLinuxLike(platform) helper used by both the default-browser and specific-browser fallback paths. Typed browserExecOptions as a precise BrowserExecOptions (ExecFileOptions plus the spawn-only detached/stdio fields) so the options object is type-checked. Safari test now also asserts no profile flags leak into the launch args. Windows Chrome/Firefox tests updated for the direct-executable invocation. - browser-profile-association-store: isAssociationEntry now validates the persisted browser against the actual BrowserKind union (chrome/firefox/ safari) via a SUPPORTED_BROWSER_KINDS set, so malformed persisted data with an unknown browser is rejected on read instead of failing later at launch time. Added a test for the unsupported-browser-kind case. - tests: the in-memory fs mock now sets error.code = 'ENOENT' to mirror a real Node ErrnoException; the missing-version test now asserts the file is not overwritten; a create-without-bucket test name now reflects that no bucket was passed.
…r, bucket default
Address the fourth round of automated review feedback (real correctness
bugs and test-coverage gaps):
- browser-profile-discovery: Firefox discovery now returns the profile Name
(the [Profile*] 'Name=' key) as directoryName instead of the Path, because
Firefox's -P flag selects profiles by Name. When Name and Path differ
(renamed profiles, relocated profile dirs), passing Path would fail to
select the intended profile. Falls back to Path only for entries that omit
Name entirely. Updated the discovery test accordingly.
- secure-browser-launcher: the Windows specific-browser path now probes
typical install locations (Program Files, PROGRAMFILES(X86), LOCALAPPDATA
for Chrome; Program Files / PROGRAMFILES(X86) for Firefox) when a bare
chrome.exe/firefox.exe is not on PATH, restoring the resilience the old
PowerShell Start-Process path had. Added tryWindowsFallbackBinaries plus
windowsBrowserFallbackPaths helpers mirroring the Linux fallback design,
and two tests (successful fallback + all-candidates-missing).
- browser-profile-association-store: writeData calls mkdir({recursive:true})
unconditionally, removing the redundant existsFn check and the TOCTOU
window between check and write.
- oauth-runtime-accessors: getBrowserProfileAssociation now passes
bucket ?? 'default' to the store so an explicit undefined bucket preserves
the default-bucket behavior deterministically.
- anthropic-oauth-provider.browser-profile.spec: extracted a DeviceFlowTestHarness
interface so the internal deviceFlow cast is checked against a documented
contract. Added coverage for (a) shouldLaunchBrowser=false skipping the
browser launch entirely, and (b) an openBrowserSecurely rejection being
tolerated (graceful degradation) rather than failing initiateAuth.
- browser-profile-discovery: removed the unused gaia_name field from
ChromeLocalState.
…LOCALAPPDATA fallback
Address the fifth round of automated review feedback:
- browser-profile-discovery + secure-browser-launcher: Windows env-var
fallbacks (LOCALAPPDATA, APPDATA, PROGRAMFILES, PROGRAMFILES(X86)) now use
an envOr(value, fallback) helper with a truthiness check instead of ??,
so an explicitly-empty environment variable does not bypass the fallback
and produce a malformed partial path.
- browser-profile-discovery: the default branch in chromeUserDataDir and
firefoxProfileRoot now throws 'Unsupported platform' instead of silently
falling back to Linux-style paths, so platform-detection issues surface
explicitly instead of causing confusing launch failures.
- secure-browser-launcher: validateProfileDirectory switched from a strict
allowlist regex (which rejected legitimate profile display names containing
parentheses, ampersands, or Unicode/CJK characters) to a denylist that
blocks only path separators (/, \), control characters (\p{Cc}), and
traversal sequences (..). This is safe because the launcher uses execFile
(no shell), passing the profile name as a discrete argv element, so shell
metacharacters are not an injection vector. Added tests for the newly
accepted names and for bare-traversal/backslash rejection.
- secure-browser-launcher: the Windows LOCALAPPDATA Chrome fallback now
resolves to %USERPROFILE%\AppData\Local (the real per-user location)
instead of C:\Users\Public, so the probe is useful for per-user installs.
- tests: the launcher test suite now saves/restores process.env.PROGRAMFILES
in beforeEach/afterEach so a test that sets it cannot leak into siblings;
replaced String.fromCharCode(10) with a readable '\n' literal in the
auth-command expected-content join.
… clarify tests Address the sixth round of automated review feedback: - browser-profile-discovery: parseChromeProfiles now treats a JSON value that is not a plain object (e.g. 'null', a number, or an array) as empty instead of dereferencing it, avoiding a TypeError on corrupted Local State files. The parsed value is typed as unknown and narrowed with a typeof-object check before being cast to ChromeLocalState. Added a test for the null-parsed case. - browser-profile-association-store.spec: aligned the in-memory mkdir mock signature with the AssociationStoreFs interface ((_p, _opts?) => void) so the mock cannot silently diverge if the implementation starts using the arguments; moved the createInMemoryFsMalformed helper above its first usage for readability; renamed the default-bucket test to reflect that it exercises getAssociation's default-bucket resolution. - secure-browser-launcher.test: renamed the 'shell metacharacters' test to 'path separators' since the value is actually rejected for its '/' separator (the launcher allows shell metacharacters because execFile uses no shell).
…e-only Windows fallback
Address the seventh round of automated review feedback:
- Extracted the duplicated envOr helper into a shared
packages/core/src/utils/env.ts module imported by both
browser-profile-discovery and secure-browser-launcher (DRY).
- discoverBrowserProfiles now catches the Unsupported-platform error thrown
by chromeUserDataDir/firefoxProfileRoot and degrades to an empty list, so
calling the public discovery entry point on an unsupported platform never
crashes the /auth command. Added a test for the graceful degradation.
- tryWindowsFallbackBinaries now gates candidate paths with statSync().isFile()
instead of existsSync(), so a directory present at a candidate path is not
mistaken for a browser binary and passed to execFileAsync. Updated the
Windows fallback test to mock statSync consistently with existsSync.
- browser-profile-association-store.spec: the unsupported-browser-kind test
now asserts the invalid file content is preserved after the failed read
(matching the missing-version test); the listAssociations('codex') test
now asserts the returned association data, not just the length.
- authCommand.test: refined the create-without-bucket test name to make
clear the action is present but the bucket argument is omitted.
# Conflicts: # packages/cli/src/ui/commands/authCommand.ts
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
acoliver
added a commit
that referenced
this pull request
Jul 14, 2026
Integrate PR #2526's OAuth browser profile associations while preserving the ACP stack. Accept the target branch's existing .llxprt tree unchanged.
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
1 participant
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
Fixes #1045.
OAuth previously opened whatever browser happened to be in the foreground ("browser roulette"). This PR lets each OAuth bucket be associated with a specific browser binary and profile directory, so OAuth always launches the correct browser/profile for that bucket — Playwright-style, instead of relying on the OS default foreground browser.
What changed
Browser launcher (
packages/core)openBrowserSecurely(url, options?)now accepts an optional{ browser, profileDirectory }. When supplied, it launches the named browser binary directly (Google Chrome,Firefox,Safari) with--profile-directory(Chrome) /-P(Firefox), instead of the OS-defaultopen/xdg-open/ PowerShell path.validateProfileDirectory()guards against path traversal / shell injection in the profile-directory argument (strict allowlist). URL validation is unchanged.browser-profile-discovery.tsreads installed browser profile display names + directory names per platform:Local State(profile.info_cache+Default)profiles.iniAssociation storage (
packages/providers)BrowserProfileAssociationStorepersists{ provider, bucket } -> { browser, profileDirectory, displayName }mappings to a JSON file in the app data dir.OAuthManagerexposes facade methods:setBrowserProfileAssociation,getBrowserProfileAssociation,clearBrowserProfileAssociation,listBrowserProfileAssociations.Provider integration
OAuthProviderinterface gains an optionalsetAuthContext({ bucket })method.AuthFlowOrchestrator.doInitiateAuthcallsprovider.setAuthContext?.({ bucket })beforeinitiateAuth(), so the provider knows which bucket is authenticating.AnthropicOAuthProviderandCodexOAuthProviderlook up the association for the active bucket (via the runtime accessor bridge) and pass{ browser, profileDirectory }toopenBrowserSecurely. When no association exists they fall back to the previous OS-default behavior.getBrowserProfileAssociationaccessor (returnsundefinedwhen no runtime is registered or on error — it is called during browser launch, an early code path).Commands (
packages/cli)/auth <provider> create <bucket>— discovers browser profiles, associates one with the bucket, then authenticates. Works for thedefaultbucket too./auth <provider> profile <bucket> [--set <profileDir> | --clear | --browser <chrome|firefox|safari>]— manage an existing bucket's browser/profile association.Why this approach
Per the issue discussion, the goal is to "pick a browser or profile and use that one" — i.e. launch the specific binary+profile directly (as Playwright does), rather than fighting over the foreground window. The association is stored per-bucket so lazy auth (token refresh / re-auth) uses the same browser/profile automatically.
Firefox and Safari are structurally supported by the launcher and discovery; the primary implementation path is Chrome (the most common multi-profile case), with Firefox/Safari launch args in place.
Test plan
packages/core—secure-browser-launcher.test.ts(32 tests: profile launch, validation, backward compat, platform-specific args, injection prevention) +browser-profile-discovery.test.ts(12 tests: Chrome/Firefox/Safari discovery, missing/malformed files, platform dirs)packages/providers—browser-profile-association-store.spec.ts(11 tests: set/get/clear/list, persistence, malformed-file guard) +anthropic-oauth-provider.browser-profile.spec.ts(3 tests: association drives launch options, no-association falls back, accessor-throws is safe)packages/cli—authCommand.test.ts(34 tests, +7 new:createaction discovery/association,profileset/clear, error paths)npm run lint(0 errors),npm run lint:eslint-guard(passed),npm run typecheck(clean),npm run format(clean),npm run build(core/providers/cli)🤖 Generated with LLxprt Code