From 1a4b23eaa182c6481d7eb0451ed525c3841be51f Mon Sep 17 00:00:00 2001 From: stgmt Date: Tue, 8 Sep 2026 23:04:25 +0300 Subject: [PATCH 01/12] fix(auth): autoselect recorded account at post-migration chooser Signed-off-by: stgmt --- CHANGELOG.md | 10 +++ src/gflow_cli/api/client.py | 52 +++++++++++++++ src/gflow_cli/cli.py | 18 +++++- src/gflow_cli/errors.py | 23 +++++++ src/gflow_cli/profile_store.py | 5 ++ tests/api/test_bootstrap_chooser.py | 63 +++++++++++++++++++ tests/auth/test_account_autoselect.py | 91 +++++++++++++++++++++++++++ 7 files changed, 261 insertions(+), 1 deletion(-) create mode 100644 tests/api/test_bootstrap_chooser.py create mode 100644 tests/auth/test_account_autoselect.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ebcdbb88..50576794 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 exit 23 — which told the user to file a frontend-drift bug about a frontend that was behaving correctly. +- **Account auto-selection at post-migration sign-in chooser** + ([#763](https://github.com/ffroliva/gflow-cli/issues/763)). When Google + Flow hands the session over to `flow.google.com` and redirects to an account + chooser, `FlowApiClient` now auto-selects the profile's recorded account from + `.gflow_account`. If the recorded account is absent or cannot be selected, the + client raises a dedicated, non-retryable `FlowAccountChooserError` (exit code 38), + avoiding generic `UnexpectedError` or selector drift stalls. `gflow auth login` + gains an optional `--account ` option to assert that login authenticates + as the required account. + ### Changed - **`gflow auth login` closes the browser for you.** It drives your real Google Chrome diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index c70c1329..6221d7da 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -86,6 +86,7 @@ BrowserSessionClosedError, ConfigurationError, ContentPolicyError, + FlowAccountChooserError, FlowApiError, # re-exported via gflow_cli.api.__init__ FlowHostMigratedError, NetworkError, @@ -783,6 +784,55 @@ async def _enter_setup(self) -> None: # S1 can share this context rather than opening its own. await self._setup_transport() + async def _handle_account_chooser(self, page: Any, account_email: str | None = None) -> bool: + """Select the recorded Google account on accountchooser if encountered (#750). + + Returns True if an account was clicked, False if not on chooser. + Raises FlowAccountChooserError if on chooser but account is missing/not selectable. + """ + from gflow_cli.profile_store import read_account_file + + url = getattr(page, "url", "") or "" + # Check if URL looks like accountchooser or Google sign-in + is_chooser = "accounts.google.com" in url and ("accountchooser" in url or "signin" in url) + # Also check for presence of chooser DOM or landing page redirect + if not is_chooser: + return False + + email = account_email or read_account_file(self.profile_dir) + if not email: + raise FlowAccountChooserError( + detail=( + f"Google sign-in/chooser displayed at {url} but no account is recorded " + f"in this profile to auto-select." + ) + ) + + # Match row by email attribute, text, or data-email + # Google account chooser rows typically have data-email, or text containing the email + selector = ( + f"div[data-email='{email}'], [aria-label*='{email}'], " + f"[role='link']:has-text('{email}'), [role='button']:has-text('{email}')" + ) + locator = page.locator(selector) + count = await locator.count() + if count == 0: + # Fallback broader text search + locator = page.locator(f"text={email}") + count = await locator.count() + + if count == 0: + raise FlowAccountChooserError( + detail=( + f"Account chooser displayed at {url} but recorded account '{email}' " + f"was not found among selectable accounts." + ) + ) + + # Click the row + await locator.first.click() + return True + async def _bootstrap_and_resolve_locale(self) -> None: """Navigate the bootstrap page and settle the account locale (#580, #587). @@ -807,6 +857,8 @@ async def _bootstrap_and_resolve_locale(self) -> None: wait_until="domcontentloaded", timeout=60_000, ) + # Check if landing redirected to account chooser + await self._handle_account_chooser(self._page) # #639: NOT_REDIRECTED means "there is no redirect to wait for". It must not # ALSO mean "do not read the locale" — which is what returning here made it # mean, and that made the state ABSORBING: `_resolve_account_locale` is the diff --git a/src/gflow_cli/cli.py b/src/gflow_cli/cli.py index 13563a49..1b5c5729 100644 --- a/src/gflow_cli/cli.py +++ b/src/gflow_cli/cli.py @@ -261,7 +261,12 @@ def _maybe_rename_first_profile( help="Browser strategy for login. 'chrome' bypasses Google secure blocks.", envvar="GFLOW_CLI_AUTH_BROWSER", ) -def auth_login(profile: str | None, browser: str | None) -> None: +@click.option( + "--account", + default=None, + help="Assert that login authenticates as this exact Google account (email address).", +) +def auth_login(profile: str | None, browser: str | None, account: str | None = None) -> None: """One-time interactive sign-in. Opens a browser window.""" from gflow_cli.browser_manager import is_chrome_available from gflow_cli.errors import EXIT_CODE_MAP, GFlowError @@ -284,6 +289,17 @@ def auth_login(profile: str | None, browser: str | None) -> None: try: pdir = asyncio.run(auth_mod.login(name, browser=selected_browser)) + if account: + actual_account = profile_store.read_account_file(pdir) + if actual_account and actual_account.lower() != account.strip().lower(): + from gflow_cli.errors import FlowAccountChooserError + + raise FlowAccountChooserError( + detail=( + f"Login completed but verified account '{actual_account}' does not " + f"match required --account '{account}'." + ) + ) except GFlowError as e: console.print(f"[red]{e}[/red]") if e.remediation_hint: diff --git a/src/gflow_cli/errors.py b/src/gflow_cli/errors.py index 0989a5ae..19847edf 100644 --- a/src/gflow_cli/errors.py +++ b/src/gflow_cli/errors.py @@ -30,6 +30,7 @@ "FlowAgentUiError", "FlowApiError", "FlowAppError", + "FlowAccountChooserError", "FlowHostMigratedError", "FrameExtractionError", "GFlowError", @@ -756,6 +757,23 @@ class FlowHostMigratedError(GFlowError): ) +class FlowAccountChooserError(GFlowError): + """Raised when Google Flow lands on an account chooser or sign-in hop + and the profile's recorded Google account cannot be selected automatically. + + **Not retryable** (exit code 33). Retrying with the same profile and recorded + account into a signed-out or missing chooser row cannot succeed without + manual operator interaction via gflow auth login. + """ + + problem_type = "https://gflow-cli.dev/errors/flow-account-chooser" + title = "Recorded Google account not selectable" + _default_remediation = ( + "Run `gflow auth login --profile ` and complete the account chooser " + "manually, or pass --account ." + ) + + class UiModeUnavailableError(GFlowError): """Raised when the Flow UI arm a command REQUIRES (``--ui-mode`` / ``GFLOW_CLI_UI_MODE``, or inferred — e.g. ``-i`` instructions force agentic) @@ -1245,6 +1263,11 @@ def __init__( # frontend" (per-account, not retryable) from genuine selector drift # (23), which it used to masquerade as. FlowHostMigratedError: 36, + # FlowAccountChooserError: Google Flow landed on account chooser + # and the recorded account row could not be selected automatically. + # Direct GFlowError subclass; exit 33 distinguishes account chooser stall + # from generic errors (1) without parsing stderr. + FlowAccountChooserError: 33, # UiModeUnavailableError (issue #299): a command's required arm (--ui-mode / # inferred) couldn't be reached after a best-effort switch. Direct GFlowError # subclass — retryable policy abort, distinct from FlowAgentUiError (25). diff --git a/src/gflow_cli/profile_store.py b/src/gflow_cli/profile_store.py index 5359ae9c..123e6dba 100644 --- a/src/gflow_cli/profile_store.py +++ b/src/gflow_cli/profile_store.py @@ -350,6 +350,11 @@ def account_locale_for(profile_name: str) -> str | None: return cached +def read_account_file(profile_path: Path) -> str | None: + """Read the Google account email from the profile's .gflow_account file.""" + return _read_account_file(profile_path) + + def _read_account_file(profile_path: Path) -> str | None: """Read the Google account email from the profile's .gflow_account file.""" account_file = profile_path / ACCOUNT_FILE diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py new file mode 100644 index 00000000..d8d755df --- /dev/null +++ b/tests/api/test_bootstrap_chooser.py @@ -0,0 +1,63 @@ +"""Tests for bootstrap account chooser auto-selection in FlowApiClient.""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from gflow_cli.errors import FlowAccountChooserError + + +@pytest.mark.asyncio +async def test_bootstrap_detects_chooser_and_autoselects_account(tmp_path: Path) -> None: + """When bootstrap hits account chooser, it clicks the row matching .gflow_account.""" + from gflow_cli.api.client import FlowApiClient + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("user@example.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page = MagicMock() + page.url = "https://accounts.google.com/v3/signin/accountchooser" + + # Mock locator for the email row + account_row = AsyncMock() + account_row.count.return_value = 1 + page.locator.return_value = account_row + + # Helper method on client or transport + res = await client._handle_account_chooser(page, "user@example.com") + assert res is True + account_row.first.click.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_bootstrap_chooser_absent_account_raises_flow_account_chooser_error( + tmp_path: Path, +) -> None: + """When recorded account is not found on chooser, FlowAccountChooserError is raised.""" + from gflow_cli.api.client import FlowApiClient + from gflow_cli.errors import EXIT_CODE_MAP + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("recorded@example.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page = MagicMock() + page.url = "https://accounts.google.com/v3/signin/accountchooser" + + account_row = AsyncMock() + account_row.count.return_value = 0 + page.locator.return_value = account_row + + with pytest.raises(FlowAccountChooserError) as exc_info: + await client._handle_account_chooser(page, "recorded@example.com") + + assert "recorded@example.com" in str(exc_info.value) + assert EXIT_CODE_MAP[FlowAccountChooserError] == 33 diff --git a/tests/auth/test_account_autoselect.py b/tests/auth/test_account_autoselect.py new file mode 100644 index 00000000..1798c8b2 --- /dev/null +++ b/tests/auth/test_account_autoselect.py @@ -0,0 +1,91 @@ +"""Tests for FlowAccountChooserError (exit code 33) and account auto-selection.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from gflow_cli.errors import ( + EXIT_CODE_MAP, + FlowAccountChooserError, + GFlowError, + is_retryable, +) + + +def test_flow_account_chooser_error_class_invariants() -> None: + """FlowAccountChooserError is a non-retryable GFlowError with RFC 9457 attributes.""" + err = FlowAccountChooserError( + detail=( + "Account chooser displayed but recorded account 'user@example.com' was not selectable." + ) + ) + assert isinstance(err, GFlowError) + assert not is_retryable(err) + assert err.problem_type == "https://gflow-cli.dev/errors/flow-account-chooser" + assert err.title == "Recorded Google account not selectable" + assert "gflow auth login" in err.remediation_hint + assert "--account" in err.remediation_hint + + +def test_flow_account_chooser_error_exit_code_33() -> None: + """FlowAccountChooserError maps to exit code 33 in EXIT_CODE_MAP.""" + err = FlowAccountChooserError(detail="test") + assert EXIT_CODE_MAP[FlowAccountChooserError] == 33 + # Check isinstance walk correctly resolves to 33 + code = next(c for cls, c in EXIT_CODE_MAP.items() if isinstance(err, cls)) + assert code == 33 + + +def test_exit_code_map_ordering_with_flow_account_chooser_error() -> None: + """Most-specific classes MUST appear before parent classes in EXIT_CODE_MAP.""" + seen: list[type] = [] + for cls in EXIT_CODE_MAP: + for prior in seen: + assert not issubclass(cls, prior), ( + f"{cls.__name__} is a subclass of {prior.__name__} but appears AFTER it; " + f"swap their order in EXIT_CODE_MAP." + ) + seen.append(cls) + + +def test_read_account_file_returns_email(tmp_path: Path) -> None: + """read_account_file helper returns stripped email or None.""" + from gflow_cli.profile_store import ACCOUNT_FILE, read_account_file + + profile_dir = tmp_path / "profile_test" + profile_dir.mkdir() + assert read_account_file(profile_dir) is None + + (profile_dir / ACCOUNT_FILE).write_text(" User.Test@Gmail.Com \n", encoding="utf-8") + assert read_account_file(profile_dir) == "User.Test@Gmail.Com" + + +def test_auth_login_with_account_mismatch_raises( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """gflow auth login --account asserts against verified session email and fails with exit 33.""" + from click.testing import CliRunner + + from gflow_cli.cli import main as cli + + # Mock login to write one email, but caller asked for a different account + async def _mock_login(name: str, browser: str = "auto", headless: bool = False) -> Path: + pdir = tmp_path / f"profile_{name}" + pdir.mkdir(parents=True, exist_ok=True) + (pdir / ".gflow_account").write_text("actual@example.com", encoding="utf-8") + return pdir + + monkeypatch.setattr("gflow_cli.auth.login", _mock_login) + monkeypatch.setenv("GFLOW_CLI_HOME", str(tmp_path)) + + runner = CliRunner() + result = runner.invoke( + cli, + ["auth", "login", "--profile", "test", "--account", "expected@example.com"], + ) + assert result.exit_code == 33 + assert "Recorded Google account not selectable" in result.output or ( + "does not match" in result.output + ) From 0759be451cbe1428ddb8e450c285df76d47ca458 Mon Sep 17 00:00:00 2001 From: stgmt Date: Wed, 9 Sep 2026 01:22:24 +0300 Subject: [PATCH 02/12] fix(auth): use exit 38 (not 33) for FlowAccountChooserError 33 is already claimed by the gflow doctor verdict (a successful diagnosis, not an error class) in EXIT_CODE_MAP documentation; the new typed chooser error must not collide. 38 is free and sits adjacent to the UI-cluster codes. Downstream presentation-reels client keys on class/38/retryable=false. Signed-off-by: stgmt --- AGENTS.md | 2 +- docs/USAGE.md | 3 ++- src/gflow_cli/errors.py | 6 +++--- tests/api/test_bootstrap_chooser.py | 2 +- tests/auth/test_account_autoselect.py | 16 ++++++++-------- website/docs/USAGE.md | 3 ++- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a188aa2e..f051faae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,7 +106,7 @@ the five other mirror axes), which no command here can check and no CI gate can - Type hints everywhere; `pyright` strict on `src/gflow_cli`. - Structured logging only (`structlog`) — **never** raw `print()` or `import logging` in `src/`. -- Errors as RFC 9457 Problem Details with stable per-class exit codes (3–37, e.g. 11 is `ConfigurationError` — including `ProfileLockedError` for same-profile lease contention, 16 is the `DataStoreError` family, 19 `SceneConcatError`, 20 `FrameExtractionError`, 21 `ChainPartialError`, 22 `UpscaleUnavailableError`, 25 `FlowAgentUiError`, 28 `UiModeUnavailableError`, 29 `MentionIndexUnavailableError`, 30 `QueueSchemaError`, 37 `InsufficientCreditsError`). See `src/gflow_cli/errors.py::EXIT_CODE_MAP` for the complete mapping. Exit 33 is reserved outside that map: `gflow doctor` findings-present — a successful diagnosis, not an error class. +- Errors as RFC 9457 Problem Details with stable per-class exit codes (3–38, e.g. 11 is `ConfigurationError` — including `ProfileLockedError` for same-profile lease contention, 16 is the `DataStoreError` family, 19 `SceneConcatError`, 20 `FrameExtractionError`, 21 `ChainPartialError`, 22 `UpscaleUnavailableError`, 25 `FlowAgentUiError`, 28 `UiModeUnavailableError`, 29 `MentionIndexUnavailableError`, 30 `QueueSchemaError`, 37 `InsufficientCreditsError`). See `src/gflow_cli/errors.py::EXIT_CODE_MAP` for the complete mapping. Exit 33 is reserved outside that map: `gflow doctor` findings-present — a successful diagnosis, not an error class. - 100-char line length, `ruff` configured. Imports sorted by `ruff` (isort rules). - **YAGNI / least-code**: prefer the smallest change that works. No speculative abstractions (interface/factory with one implementation), no config or flags nobody sets, no dead constants/helpers, no reinventing the stdlib. Review carries this as its own lens — the **D14 over-engineering** dimension of [`pr-council-review`](skills/pr-council-review/SKILL.md) (baseline, always runs). Its rubric is portable; the `ponytail` plugin (see CONTRIBUTING) is an optional accelerant, not a dependency. - **MCP & CLI Schema Symmetry**: Any updates or additions to user-facing CLI command parameters (e.g., `gflow image t2i`, `gflow video`) must be mirrored in the corresponding MCP tool definitions. Never add option/argument fields to Click commands without updating the MCP server implementation. This symmetry is enforced programmatically in CI via `tests/mcp/test_cli_parity.py` (every CLI leaf command needs a mapped MCP tool or an explicit, reasoned exemption) plus the schema checks in `tests/mcp/test_server.py`. diff --git a/docs/USAGE.md b/docs/USAGE.md index 07892b63..060ac87d 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1786,7 +1786,8 @@ shell scripts can branch on the failure mode without parsing stderr. | `34` | `SyncPartialError` | `gflow data sync` failed on some projects but succeeded on others — completed writes stay committed | Retryable: re-run the same command; it resumes with what is still nameless (see [`gflow data sync`](#gflow-data-sync)) | | `35` | `ExtendUnavailableError` | No Veo extend model is orderable for this account and aspect — the extend family is tier-gated and there is no square variant. **Never auto-retry**: a tier gate does not clear on its own. | | `36` | `FlowHostMigratedError` | Flow served the project from `flow.google.com` and the request could not be represented by the migrated composer, or `GFLOW_CLI_FLOW_HOST=labs.google` disabled it. Supported today: `video t2v`; local-file video i2v/r2v; `image t2i`; and local-file `image i2i`. Image UUID/entity/instruction/Imagen-4 forms, `image batch`, and the `3:4` image aspect remain unsupported. Not selector drift (23) | **Not retryable.** Use one of the supported forms — `--project` is required for images as well as video — or the REST surface (`gflow project list`, `gflow data …`); follow #639 for the remaining matrix | -| `37` | `InsufficientCreditsError` | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | +| `37` | `InsufficientCreditsError` +| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | | `130`| SIGINT | User-interrupted (Ctrl-C) | — | **Exit code 16 — data store / migration error.** Fires when: diff --git a/src/gflow_cli/errors.py b/src/gflow_cli/errors.py index 19847edf..f84957d8 100644 --- a/src/gflow_cli/errors.py +++ b/src/gflow_cli/errors.py @@ -761,7 +761,7 @@ class FlowAccountChooserError(GFlowError): """Raised when Google Flow lands on an account chooser or sign-in hop and the profile's recorded Google account cannot be selected automatically. - **Not retryable** (exit code 33). Retrying with the same profile and recorded + **Not retryable** (exit code 38). Retrying with the same profile and recorded account into a signed-out or missing chooser row cannot succeed without manual operator interaction via gflow auth login. """ @@ -1265,9 +1265,9 @@ def __init__( FlowHostMigratedError: 36, # FlowAccountChooserError: Google Flow landed on account chooser # and the recorded account row could not be selected automatically. - # Direct GFlowError subclass; exit 33 distinguishes account chooser stall + # Direct GFlowError subclass; exit 38 distinguishes account chooser stall # from generic errors (1) without parsing stderr. - FlowAccountChooserError: 33, + FlowAccountChooserError: 38, # UiModeUnavailableError (issue #299): a command's required arm (--ui-mode / # inferred) couldn't be reached after a best-effort switch. Direct GFlowError # subclass — retryable policy abort, distinct from FlowAgentUiError (25). diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index d8d755df..5860a51b 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -60,4 +60,4 @@ async def test_bootstrap_chooser_absent_account_raises_flow_account_chooser_erro await client._handle_account_chooser(page, "recorded@example.com") assert "recorded@example.com" in str(exc_info.value) - assert EXIT_CODE_MAP[FlowAccountChooserError] == 33 + assert EXIT_CODE_MAP[FlowAccountChooserError] == 38 diff --git a/tests/auth/test_account_autoselect.py b/tests/auth/test_account_autoselect.py index 1798c8b2..a8f4658d 100644 --- a/tests/auth/test_account_autoselect.py +++ b/tests/auth/test_account_autoselect.py @@ -1,4 +1,4 @@ -"""Tests for FlowAccountChooserError (exit code 33) and account auto-selection.""" +"""Tests for FlowAccountChooserError (exit code 38) and account auto-selection.""" from __future__ import annotations @@ -29,13 +29,13 @@ def test_flow_account_chooser_error_class_invariants() -> None: assert "--account" in err.remediation_hint -def test_flow_account_chooser_error_exit_code_33() -> None: - """FlowAccountChooserError maps to exit code 33 in EXIT_CODE_MAP.""" +def test_flow_account_chooser_error_exit_code_38() -> None: + """FlowAccountChooserError maps to exit code 38 in EXIT_CODE_MAP.""" err = FlowAccountChooserError(detail="test") - assert EXIT_CODE_MAP[FlowAccountChooserError] == 33 - # Check isinstance walk correctly resolves to 33 + assert EXIT_CODE_MAP[FlowAccountChooserError] == 38 + # Check isinstance walk correctly resolves to 38 code = next(c for cls, c in EXIT_CODE_MAP.items() if isinstance(err, cls)) - assert code == 33 + assert code == 38 def test_exit_code_map_ordering_with_flow_account_chooser_error() -> None: @@ -65,7 +65,7 @@ def test_read_account_file_returns_email(tmp_path: Path) -> None: def test_auth_login_with_account_mismatch_raises( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """gflow auth login --account asserts against verified session email and fails with exit 33.""" + """gflow auth login --account asserts against verified session email and fails with exit 38.""" from click.testing import CliRunner from gflow_cli.cli import main as cli @@ -85,7 +85,7 @@ async def _mock_login(name: str, browser: str = "auto", headless: bool = False) cli, ["auth", "login", "--profile", "test", "--account", "expected@example.com"], ) - assert result.exit_code == 33 + assert result.exit_code == 38 assert "Recorded Google account not selectable" in result.output or ( "does not match" in result.output ) diff --git a/website/docs/USAGE.md b/website/docs/USAGE.md index 19fa9c27..c6615611 100644 --- a/website/docs/USAGE.md +++ b/website/docs/USAGE.md @@ -1786,7 +1786,8 @@ shell scripts can branch on the failure mode without parsing stderr. | `34` | `SyncPartialError` | `gflow data sync` failed on some projects but succeeded on others — completed writes stay committed | Retryable: re-run the same command; it resumes with what is still nameless (see [`gflow data sync`](#gflow-data-sync)) | | `35` | `ExtendUnavailableError` | No Veo extend model is orderable for this account and aspect — the extend family is tier-gated and there is no square variant. **Never auto-retry**: a tier gate does not clear on its own. | | `36` | `FlowHostMigratedError` | Flow served the project from `flow.google.com` and the request could not be represented by the migrated composer, or `GFLOW_CLI_FLOW_HOST=labs.google` disabled it. Supported today: `video t2v`; local-file video i2v/r2v; `image t2i`; and local-file `image i2i`. Image UUID/entity/instruction/Imagen-4 forms, `image batch`, and the `3:4` image aspect remain unsupported. Not selector drift (23) | **Not retryable.** Use one of the supported forms — `--project` is required for images as well as video — or the REST surface (`gflow project list`, `gflow data …`); follow #639 for the remaining matrix | -| `37` | `InsufficientCreditsError` | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | +| `37` | `InsufficientCreditsError` +| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | | `130`| SIGINT | User-interrupted (Ctrl-C) | — | **Exit code 16 — data store / migration error.** Fires when: From c12e5aabd89997ee62e9b2545b43fa929d7acc2e Mon Sep 17 00:00:00 2001 From: stgmt Date: Wed, 9 Sep 2026 01:55:23 +0300 Subject: [PATCH 03/12] fix(docs): repair USAGE exit-code table, add 38 row The previous commit split the 37 row and dropped the 38 row from both docs copies; the table now carries the full 37 row followed by the new 38 row (FlowAccountChooserError). Signed-off-by: stgmt --- docs/USAGE.md | 4 ++-- website/docs/USAGE.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/USAGE.md b/docs/USAGE.md index 060ac87d..6477bbc2 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1786,8 +1786,8 @@ shell scripts can branch on the failure mode without parsing stderr. | `34` | `SyncPartialError` | `gflow data sync` failed on some projects but succeeded on others — completed writes stay committed | Retryable: re-run the same command; it resumes with what is still nameless (see [`gflow data sync`](#gflow-data-sync)) | | `35` | `ExtendUnavailableError` | No Veo extend model is orderable for this account and aspect — the extend family is tier-gated and there is no square variant. **Never auto-retry**: a tier gate does not clear on its own. | | `36` | `FlowHostMigratedError` | Flow served the project from `flow.google.com` and the request could not be represented by the migrated composer, or `GFLOW_CLI_FLOW_HOST=labs.google` disabled it. Supported today: `video t2v`; local-file video i2v/r2v; `image t2i`; and local-file `image i2i`. Image UUID/entity/instruction/Imagen-4 forms, `image batch`, and the `3:4` image aspect remain unsupported. Not selector drift (23) | **Not retryable.** Use one of the supported forms — `--project` is required for images as well as video — or the REST surface (`gflow project list`, `gflow data …`); follow #639 for the remaining matrix | -| `37` | `InsufficientCreditsError` -| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | +| `37` | `InsufficientCreditsError` | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | +| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | | `130`| SIGINT | User-interrupted (Ctrl-C) | — | **Exit code 16 — data store / migration error.** Fires when: diff --git a/website/docs/USAGE.md b/website/docs/USAGE.md index c6615611..9ac750f4 100644 --- a/website/docs/USAGE.md +++ b/website/docs/USAGE.md @@ -1786,8 +1786,8 @@ shell scripts can branch on the failure mode without parsing stderr. | `34` | `SyncPartialError` | `gflow data sync` failed on some projects but succeeded on others — completed writes stay committed | Retryable: re-run the same command; it resumes with what is still nameless (see [`gflow data sync`](#gflow-data-sync)) | | `35` | `ExtendUnavailableError` | No Veo extend model is orderable for this account and aspect — the extend family is tier-gated and there is no square variant. **Never auto-retry**: a tier gate does not clear on its own. | | `36` | `FlowHostMigratedError` | Flow served the project from `flow.google.com` and the request could not be represented by the migrated composer, or `GFLOW_CLI_FLOW_HOST=labs.google` disabled it. Supported today: `video t2v`; local-file video i2v/r2v; `image t2i`; and local-file `image i2i`. Image UUID/entity/instruction/Imagen-4 forms, `image batch`, and the `3:4` image aspect remain unsupported. Not selector drift (23) | **Not retryable.** Use one of the supported forms — `--project` is required for images as well as video — or the REST surface (`gflow project list`, `gflow data …`); follow #639 for the remaining matrix | -| `37` | `InsufficientCreditsError` -| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | +| `37` | `InsufficientCreditsError` | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | +| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | | `130`| SIGINT | User-interrupted (Ctrl-C) | — | **Exit code 16 — data store / migration error.** Fires when: From 41c4ae2046446c2fd5928edf79a204cb92cc71ec Mon Sep 17 00:00:00 2001 From: stgmt Date: Wed, 9 Sep 2026 04:26:05 +0300 Subject: [PATCH 04/12] fix(auth): address council review on chooser autoselect Exact data-email row match plus exact-text fallback (a substring superset can no longer win and bill the wrong account); chooser check moved after the locale settle; click-through verified by wait_for_url to the editor; bot-rejection hop excluded from the chooser heuristic; --account raises when the identity is unreadable and warns what the profile now holds on mismatch; remediation names only gflow auth login; chooser wired into diagnostics capture triggers; exit-38 carve-out recorded in the selector memory; e2e_auth bootstrap test added. Signed-off-by: stgmt --- docs/AUTHENTICATION.md | 12 ++ docs/DEBUGGING.md | 2 +- .../memory/ui-selector-drift-error-exit-23.md | 12 ++ src/gflow_cli/api/client.py | 55 +++++--- src/gflow_cli/cli.py | 19 ++- src/gflow_cli/diagnostics.py | 4 + src/gflow_cli/errors.py | 2 +- src/gflow_cli/profile_store.py | 7 +- tests/api/test_bootstrap_chooser.py | 121 +++++++++++++++--- tests/auth/test_account_autoselect.py | 14 +- tests/e2e/test_auth_verification_e2e.py | 24 ++++ website/docs/AUTHENTICATION.md | 12 ++ website/docs/DEBUGGING.md | 2 +- 13 files changed, 224 insertions(+), 62 deletions(-) diff --git a/docs/AUTHENTICATION.md b/docs/AUTHENTICATION.md index 8771cc69..ea4fd1f6 100644 --- a/docs/AUTHENTICATION.md +++ b/docs/AUTHENTICATION.md @@ -174,6 +174,18 @@ Set ffroliva as default profile. If a profile named after the email local-part already exists, the rename is skipped and the profile keeps the name `default`. +#### `--account ` + +Asserts that the login authenticates as one exact Google account. After the +session verifies, the CLI compares the verified email against `--account` +(case-insensitive) and fails with `FlowAccountChooserError` (exit 38) on a +mismatch — including when no verified email was recorded at all, since an +identity assertion that cannot read the identity must not pass. + +A mismatch means the profile now holds the *other* account's session: re-run +`gflow auth login --profile --account ` while signed in as the +required account. + #### `--browser [auto|chrome|internal]` | Value | Browser used | When to use | diff --git a/docs/DEBUGGING.md b/docs/DEBUGGING.md index 74490be1..89be6cac 100644 --- a/docs/DEBUGGING.md +++ b/docs/DEBUGGING.md @@ -107,7 +107,7 @@ at command startup). Captured: `FlowAppError` (31), `FlowAgentUiError` (25), `FlowHostMigratedError` (36), `UiModeUnavailableError` (28), -`UiSelectorDriftError` (23), +`UiSelectorDriftError` (23), `FlowAccountChooserError` (38), `TransportTimeoutError` (9), `BrowserSessionClosedError` (15), `WireFormatError` (7), `WafRejectionError` (10), `NetworkError` (6), unexpected exceptions while a page is alive, and `ProfileLockedError` (11) diff --git a/docs/superpowers/memory/ui-selector-drift-error-exit-23.md b/docs/superpowers/memory/ui-selector-drift-error-exit-23.md index 7d97ae67..17705046 100644 --- a/docs/superpowers/memory/ui-selector-drift-error-exit-23.md +++ b/docs/superpowers/memory/ui-selector-drift-error-exit-23.md @@ -16,3 +16,15 @@ description: "Selector-probe failures = typed UiSelectorDriftError exit 23, neve **Remediation contract updated by PR #504 (2026-08-13, #493):** `UiSelectorDriftError._default_remediation` now asks for "the diagnostics JSON and/or debug screenshot referenced in this message, plus the incident bundle's report.md" — the old "debug screenshot from this message" was a false promise on the mode-switch probe, which writes `diag_mode_switch_miss.json` ONLY (no screenshot; the full-page screenshot lives in the incident bundle's `sensitive/`). The exit-23 mode-switch fall-through detail additionally names the unrecognized-new-variant hypothesis. See [[issue-493-third-editor-variant-predict-stop]]. See [[pr-184-e2e-drift-sim-results]], [[flow-library-ui-drift-174]], [[exit-code-map-ordering-invariant-test-pitfall]]. + +**Carve-out recorded by PR #764 (2026-09-08, #763):** a selector-cascade miss on +`accounts.google.com` (the Google account chooser after the post-migration hop) +raises `FlowAccountChooserError` (exit 38), NOT `UiSelectorDriftError` (exit 23). +The chooser is Google-auth UI, not the Flow editor: reporting it as drift would +tell users to file a frontend bug about a working chooser, and the exit-23 +remediation (attach diagnostics, check for a release) cannot fix a missing +account row. The miss is evidence about the *recorded account* (absent row, +signed-out row, bot-rejection hop), so the typed error carries the observed URL +kind and names `gflow auth login --profile ` as the recovery. Precedent: +exits 36 (`FlowHostMigratedError`) and 37 (`InsufficientCreditsError`) each got +the same carve-out recorded when introduced. diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index 6221d7da..3fcd18b6 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -785,18 +785,22 @@ async def _enter_setup(self) -> None: await self._setup_transport() async def _handle_account_chooser(self, page: Any, account_email: str | None = None) -> bool: - """Select the recorded Google account on accountchooser if encountered (#750). + """Select the recorded Google account on accountchooser if encountered (#763). Returns True if an account was clicked, False if not on chooser. Raises FlowAccountChooserError if on chooser but account is missing/not selectable. + + Matching is exact on the account row only: the chooser's loose surfaces + ("Remove ", "Sign out of ", signed-in-as subtitle) would + otherwise win a substring match and click the wrong account, billing it. """ from gflow_cli.profile_store import read_account_file url = getattr(page, "url", "") or "" - # Check if URL looks like accountchooser or Google sign-in - is_chooser = "accounts.google.com" in url and ("accountchooser" in url or "signin" in url) - # Also check for presence of chooser DOM or landing page redirect - if not is_chooser: + # accounts.google.com is a sign-in surface. Anything else is the caller's + # page, not a chooser; the rejected-browser hop is not a chooser either + # and must surface as its own error rather than a missing account. + if "accounts.google.com" not in url or "v3/signin/rejected" in url: return False email = account_email or read_account_file(self.profile_dir) @@ -808,18 +812,14 @@ async def _handle_account_chooser(self, page: Any, account_email: str | None = N ) ) - # Match row by email attribute, text, or data-email - # Google account chooser rows typically have data-email, or text containing the email - selector = ( - f"div[data-email='{email}'], [aria-label*='{email}'], " - f"[role='link']:has-text('{email}'), [role='button']:has-text('{email}')" - ) - locator = page.locator(selector) - count = await locator.count() + # Exact row match only (D3): data-email is the chooser's stable per-account + # anchor. A substring/text-engine fallback would match "Remove " + # or "Sign out of " and click a DOM-order-first wrong account. + row = page.locator(f'[data-email="{email}"]') + count = await row.count() if count == 0: - # Fallback broader text search - locator = page.locator(f"text={email}") - count = await locator.count() + row = page.get_by_text(email, exact=True) + count = await row.count() if count == 0: raise FlowAccountChooserError( @@ -829,8 +829,19 @@ async def _handle_account_chooser(self, page: Any, account_email: str | None = N ) ) - # Click the row - await locator.first.click() + # The row is the account's entry; verify we actually leave the chooser. + await row.first.click() + if "project" not in (await page.wait_for_url("**/project/**", timeout=30_000) or ""): + raise FlowAccountChooserError( + detail=( + f"Clicked recorded account '{email}' on the chooser but the session " + f"did not reach the Flow editor." + ) + ) + logger.info( + "client.account_chooser_autoselected", + account=redact_sensitive_text(email), + ) return True async def _bootstrap_and_resolve_locale(self) -> None: @@ -857,8 +868,6 @@ async def _bootstrap_and_resolve_locale(self) -> None: wait_until="domcontentloaded", timeout=60_000, ) - # Check if landing redirected to account chooser - await self._handle_account_chooser(self._page) # #639: NOT_REDIRECTED means "there is no redirect to wait for". It must not # ALSO mean "do not read the locale" — which is what returning here made it # mean, and that made the state ABSORBING: `_resolve_account_locale` is the @@ -871,6 +880,12 @@ async def _bootstrap_and_resolve_locale(self) -> None: self._account_locale, from_url = await self._resolve_account_locale( self._page, settle=settle ) + # #763: the chooser hop lands through the same post-goto redirect chain as + # the locale hop, so it is observable only after the settle above. On a + # chooser the settle reads off accounts.google.com — safe: + # write_account_locale runs only on the redirected branch below, which + # the chooser never reaches. + await self._handle_account_chooser(self._page) if not settle: # Kept (not merged into account_locale_state) because field reports key # on this event to tell "the settle was skipped" from "it timed out". diff --git a/src/gflow_cli/cli.py b/src/gflow_cli/cli.py index 1b5c5729..11566915 100644 --- a/src/gflow_cli/cli.py +++ b/src/gflow_cli/cli.py @@ -36,6 +36,7 @@ from gflow_cli.observability import DEBUG_LEVEL, configure_logging from gflow_cli.update_check import UpdateNotice, maybe_notify_update +logger = structlog.get_logger(__name__) console = Console() @@ -290,14 +291,22 @@ def auth_login(profile: str | None, browser: str | None, account: str | None = N try: pdir = asyncio.run(auth_mod.login(name, browser=selected_browser)) if account: - actual_account = profile_store.read_account_file(pdir) - if actual_account and actual_account.lower() != account.strip().lower(): - from gflow_cli.errors import FlowAccountChooserError + from gflow_cli.errors import FlowAccountChooserError + actual_account = profile_store.read_account_file(pdir) + if actual_account is None or actual_account.lower() != account.strip().lower(): + held = actual_account or "nothing recorded" + logger.warning( + "auth.account_assert_failed", + required=account.strip(), + held=actual_account or None, + ) raise FlowAccountChooserError( detail=( - f"Login completed but verified account '{actual_account}' does not " - f"match required --account '{account}'." + f"Login completed but the profile now holds '{held}', which does not " + f"match required --account '{account}'. Re-run " + f"`gflow auth login --profile {name} --account {account.strip()}` " + f"while signed in as the required account." ) ) except GFlowError as e: diff --git a/src/gflow_cli/diagnostics.py b/src/gflow_cli/diagnostics.py index 72c23261..2c990e83 100644 --- a/src/gflow_cli/diagnostics.py +++ b/src/gflow_cli/diagnostics.py @@ -1847,6 +1847,7 @@ def _validate_overlay(raw: dict[str, object]) -> dict[str, object]: def _capture_triggers() -> tuple[type[BaseException], ...]: from gflow_cli.errors import ( BrowserSessionClosedError, + FlowAccountChooserError, FlowAgentUiError, FlowAppError, FlowHostMigratedError, @@ -1859,6 +1860,7 @@ def _capture_triggers() -> tuple[type[BaseException], ...]: ) return ( + FlowAccountChooserError, FlowAppError, FlowAgentUiError, # #639: this arm REPLACED UiSelectorDriftError on the migrated frontend. @@ -1877,6 +1879,7 @@ def _capture_triggers() -> tuple[type[BaseException], ...]: def _screenshot_triggers() -> tuple[type[BaseException], ...]: from gflow_cli.errors import ( + FlowAccountChooserError, FlowAgentUiError, FlowAppError, FlowHostMigratedError, @@ -1886,6 +1889,7 @@ def _screenshot_triggers() -> tuple[type[BaseException], ...]: ) return ( + FlowAccountChooserError, FlowAppError, FlowAgentUiError, FlowHostMigratedError, diff --git a/src/gflow_cli/errors.py b/src/gflow_cli/errors.py index f84957d8..44180eab 100644 --- a/src/gflow_cli/errors.py +++ b/src/gflow_cli/errors.py @@ -770,7 +770,7 @@ class FlowAccountChooserError(GFlowError): title = "Recorded Google account not selectable" _default_remediation = ( "Run `gflow auth login --profile ` and complete the account chooser " - "manually, or pass --account ." + "manually while signed in as the recorded account." ) diff --git a/src/gflow_cli/profile_store.py b/src/gflow_cli/profile_store.py index 123e6dba..c3eddccf 100644 --- a/src/gflow_cli/profile_store.py +++ b/src/gflow_cli/profile_store.py @@ -107,7 +107,7 @@ def list_profiles() -> list[ProfileMeta]: name = entry.name[len(PROFILE_DIR_PREFIX) :] s = status(name) last_used = _last_modified(entry) - google_account = _read_account_file(entry) + google_account = read_account_file(entry) out.append( ProfileMeta( name=name, @@ -351,11 +351,6 @@ def account_locale_for(profile_name: str) -> str | None: def read_account_file(profile_path: Path) -> str | None: - """Read the Google account email from the profile's .gflow_account file.""" - return _read_account_file(profile_path) - - -def _read_account_file(profile_path: Path) -> str | None: """Read the Google account email from the profile's .gflow_account file.""" account_file = profile_path / ACCOUNT_FILE try: diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index 5860a51b..6d637402 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -10,6 +10,22 @@ from gflow_cli.errors import FlowAccountChooserError +def _chooser_page(url: str, row_count: int) -> tuple[MagicMock, AsyncMock]: + """Build a chooser page whose data-email row has the given count. + + The exact-row locator is the first ``page.locator`` call; the exact-text + fallback uses ``page.get_by_text``. ``wait_for_url`` defaults to an + un-awaited MagicMock — tests that reach it must override it. + """ + page = MagicMock() + page.url = url + row = AsyncMock() + row.count = AsyncMock(return_value=row_count) + row.first = AsyncMock() + page.locator.return_value = row + return page, row + + @pytest.mark.asyncio async def test_bootstrap_detects_chooser_and_autoselects_account(tmp_path: Path) -> None: """When bootstrap hits account chooser, it clicks the row matching .gflow_account.""" @@ -21,18 +37,18 @@ async def test_bootstrap_detects_chooser_and_autoselects_account(tmp_path: Path) (profile / ACCOUNT_FILE).write_text("user@example.com\n", encoding="utf-8") client = FlowApiClient(profile_dir=profile) - page = MagicMock() - page.url = "https://accounts.google.com/v3/signin/accountchooser" - - # Mock locator for the email row - account_row = AsyncMock() - account_row.count.return_value = 1 - page.locator.return_value = account_row + page, row = _chooser_page( + "https://accounts.google.com/v3/signin/accountchooser?continue=flow.google.com", + row_count=1, + ) + page.wait_for_url = AsyncMock(return_value="https://flow.google.com/project/p1") - # Helper method on client or transport res = await client._handle_account_chooser(page, "user@example.com") assert res is True - account_row.first.click.assert_awaited_once() + # The exact row selector is used, and it is clicked + assert page.locator.call_args[0][0] == '[data-email="user@example.com"]' + row.first.click.assert_awaited_once() + page.wait_for_url.assert_awaited_once_with("**/project/**", timeout=30_000) @pytest.mark.asyncio @@ -49,15 +65,90 @@ async def test_bootstrap_chooser_absent_account_raises_flow_account_chooser_erro (profile / ACCOUNT_FILE).write_text("recorded@example.com\n", encoding="utf-8") client = FlowApiClient(profile_dir=profile) - page = MagicMock() - page.url = "https://accounts.google.com/v3/signin/accountchooser" - - account_row = AsyncMock() - account_row.count.return_value = 0 - page.locator.return_value = account_row + page, row = _chooser_page( + "https://accounts.google.com/v3/signin/accountchooser?continue=flow.google.com", + row_count=0, + ) + # The exact-text fallback also matches nothing. + page.get_by_text = MagicMock(return_value=MagicMock(count=AsyncMock(return_value=0))) with pytest.raises(FlowAccountChooserError) as exc_info: await client._handle_account_chooser(page, "recorded@example.com") assert "recorded@example.com" in str(exc_info.value) assert EXIT_CODE_MAP[FlowAccountChooserError] == 38 + page.get_by_text.assert_called_once_with("recorded@example.com", exact=True) + + +@pytest.mark.asyncio +async def test_bootstrap_chooser_exact_match_never_clicks_superset_account( + tmp_path: Path, +) -> None: + """A superset address on the chooser must not be clicked (billing safety). + + Regression for the substring-match defect: with ``an@corp.com`` recorded and + only ``ryan@corp.com`` present, neither the exact data-email row nor the + exact-text fallback may match — the handler must raise, never click. + """ + from gflow_cli.api.client import FlowApiClient + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("an@corp.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page, row = _chooser_page( + "https://accounts.google.com/v3/signin/accountchooser", + row_count=0, + ) + page.get_by_text = MagicMock(return_value=MagicMock(count=AsyncMock(return_value=0))) + + with pytest.raises(FlowAccountChooserError): + await client._handle_account_chooser(page, "an@corp.com") + row.first.click.assert_not_awaited() + page.wait_for_url.assert_not_called() + + +@pytest.mark.asyncio +async def test_bootstrap_chooser_click_no_editor_raises_flow_account_chooser_error( + tmp_path: Path, +) -> None: + """Click-through that never reaches the editor raises FlowAccountChooserError.""" + from gflow_cli.api.client import FlowApiClient + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("user@example.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page, row = _chooser_page( + "https://accounts.google.com/v3/signin/accountchooser", + row_count=1, + ) + page.wait_for_url = AsyncMock( + return_value="https://accounts.google.com/v3/signin/accountchooser" + ) + + with pytest.raises(FlowAccountChooserError) as exc_info: + await client._handle_account_chooser(page, "user@example.com") + assert "did not reach the Flow editor" in str(exc_info.value) + + +@pytest.mark.asyncio +async def test_bootstrap_rejected_browser_hop_is_not_a_chooser(tmp_path: Path) -> None: + """The bot-rejection hop must surface as its own error, never a missing account.""" + from gflow_cli.api.client import FlowApiClient + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("user@example.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page, row = _chooser_page("https://accounts.google.com/v3/signin/rejected", row_count=0) + + res = await client._handle_account_chooser(page, "user@example.com") + assert res is False + page.locator.assert_not_called() diff --git a/tests/auth/test_account_autoselect.py b/tests/auth/test_account_autoselect.py index a8f4658d..a531a08e 100644 --- a/tests/auth/test_account_autoselect.py +++ b/tests/auth/test_account_autoselect.py @@ -26,7 +26,7 @@ def test_flow_account_chooser_error_class_invariants() -> None: assert err.problem_type == "https://gflow-cli.dev/errors/flow-account-chooser" assert err.title == "Recorded Google account not selectable" assert "gflow auth login" in err.remediation_hint - assert "--account" in err.remediation_hint + assert "complete the account chooser" in err.remediation_hint def test_flow_account_chooser_error_exit_code_38() -> None: @@ -38,18 +38,6 @@ def test_flow_account_chooser_error_exit_code_38() -> None: assert code == 38 -def test_exit_code_map_ordering_with_flow_account_chooser_error() -> None: - """Most-specific classes MUST appear before parent classes in EXIT_CODE_MAP.""" - seen: list[type] = [] - for cls in EXIT_CODE_MAP: - for prior in seen: - assert not issubclass(cls, prior), ( - f"{cls.__name__} is a subclass of {prior.__name__} but appears AFTER it; " - f"swap their order in EXIT_CODE_MAP." - ) - seen.append(cls) - - def test_read_account_file_returns_email(tmp_path: Path) -> None: """read_account_file helper returns stripped email or None.""" from gflow_cli.profile_store import ACCOUNT_FILE, read_account_file diff --git a/tests/e2e/test_auth_verification_e2e.py b/tests/e2e/test_auth_verification_e2e.py index b9027c0d..c927f123 100644 --- a/tests/e2e/test_auth_verification_e2e.py +++ b/tests/e2e/test_auth_verification_e2e.py @@ -88,3 +88,27 @@ class BrowserCookieError(Exception): assert status.outcome is FlowSessionOutcome.AUTHENTICATED assert isinstance(status.user_email, str) and status.user_email + + +async def test_e2e_bootstrap_completes_with_chooser_callsite_present( + e2e_profile_dir: Path, +) -> None: + """Bootstrap still completes with the chooser-autoselect callsite wired in (#763). + + Zero credits: enters the client (bootstrap + locale settle + the new + ``_handle_account_chooser`` call) and asserts the page lands on the Flow + editor, not on an account chooser. On a signed-in profile no chooser + renders, so this pins the no-op path; a maintainer can run it against a + profile signed out of Flow to exercise the click-through branch. + """ + from gflow_cli.profile_store import ACCOUNT_FILE + + account_file = e2e_profile_dir / ACCOUNT_FILE + recorded = account_file.read_text(encoding="utf-8").strip() if account_file.exists() else "" + + async with FlowApiClient(profile_dir=e2e_profile_dir, transport="evaluate_fetch") as client: + assert await client.health_check() is True + url = getattr(client._page, "url", "") or "" + assert "accounts.google.com" not in url, ( + f"bootstrap stalled on the account chooser for recorded account {recorded!r}" + ) diff --git a/website/docs/AUTHENTICATION.md b/website/docs/AUTHENTICATION.md index 95e4647a..b4f508bd 100644 --- a/website/docs/AUTHENTICATION.md +++ b/website/docs/AUTHENTICATION.md @@ -174,6 +174,18 @@ Set ffroliva as default profile. If a profile named after the email local-part already exists, the rename is skipped and the profile keeps the name `default`. +#### `--account ` + +Asserts that the login authenticates as one exact Google account. After the +session verifies, the CLI compares the verified email against `--account` +(case-insensitive) and fails with `FlowAccountChooserError` (exit 38) on a +mismatch — including when no verified email was recorded at all, since an +identity assertion that cannot read the identity must not pass. + +A mismatch means the profile now holds the *other* account's session: re-run +`gflow auth login --profile --account ` while signed in as the +required account. + #### `--browser [auto|chrome|internal]` | Value | Browser used | When to use | diff --git a/website/docs/DEBUGGING.md b/website/docs/DEBUGGING.md index 74490be1..89be6cac 100644 --- a/website/docs/DEBUGGING.md +++ b/website/docs/DEBUGGING.md @@ -107,7 +107,7 @@ at command startup). Captured: `FlowAppError` (31), `FlowAgentUiError` (25), `FlowHostMigratedError` (36), `UiModeUnavailableError` (28), -`UiSelectorDriftError` (23), +`UiSelectorDriftError` (23), `FlowAccountChooserError` (38), `TransportTimeoutError` (9), `BrowserSessionClosedError` (15), `WireFormatError` (7), `WafRejectionError` (10), `NetworkError` (6), unexpected exceptions while a page is alive, and `ProfileLockedError` (11) From 5a24751e02f37cc9e87a9f55568d03be18867461 Mon Sep 17 00:00:00 2001 From: Flavio Oliva Date: Wed, 9 Sep 2026 08:22:47 +0100 Subject: [PATCH 05/12] fix(auth): make the chooser click-through verification actually verifiable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Page.wait_for_url` is annotated `-> None`: it returns nothing and signals a miss by raising. So `(await page.wait_for_url(...) or "")` was always `""`, `"project" not in ""` was always True, and every SUCCESSFUL chooser click raised FlowAccountChooserError. The success path — the logger.info and `return True` beneath it — was unreachable in production, and a real stall raised an uncaught Playwright TimeoutError, surfacing as the same opaque exit 1 that #763 is about. The landing predicate also changes. `**/project/**` cannot match the bootstrap URL `labs.google/fx/tools/flow`, and only the migrated origin serves /project/, so the glob encoded one cohort's shape as universal. `flow_host_kind` is the codebase's exact-host classifier and answers for both cohorts; a substring test would match any URL merely carrying the host in a ?continue= parameter. The happy-path test asserted `AsyncMock(return_value=".../project/p1")` — a value Playwright cannot produce — which is why CI stayed green over an inert fix. It now pins the real contract: wait_for_url returns None, raises on a miss, and the predicate accepts both Flow cohorts while rejecting the chooser itself. --- src/gflow_cli/api/client.py | 24 ++++++++++++++++++++---- tests/api/test_bootstrap_chooser.py | 25 +++++++++++++++++++------ 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index 3fcd18b6..887045fb 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -27,6 +27,7 @@ import structlog from playwright.async_api import BrowserContext, Page, Playwright, async_playwright +from playwright.async_api import TimeoutError as PlaywrightTimeoutError from gflow_cli.api import routes, video_extend from gflow_cli.api._engine import ( @@ -61,7 +62,11 @@ make_transport, resolve_transport_name, ) -from gflow_cli.api.transports._common import await_url_settled, raise_if_migrated +from gflow_cli.api.transports._common import ( + await_url_settled, + flow_host_kind, + raise_if_migrated, +) from gflow_cli.api.transports.base import ( FlowTransportStrategy, SupportsTransportSetup, @@ -831,13 +836,24 @@ async def _handle_account_chooser(self, page: Any, account_email: str | None = N # The row is the account's entry; verify we actually leave the chooser. await row.first.click() - if "project" not in (await page.wait_for_url("**/project/**", timeout=30_000) or ""): + # `wait_for_url` returns None and signals a miss by RAISING, so its return value + # is falsy on success as well as failure — testing it inverted the check and made + # every successful click raise. Catch the raise instead. + # + # The landing predicate is "on any Flow host", not a `**/project/**` glob: the + # bootstrap URL is `labs.google/fx/tools/flow` with no /project/ segment, and only + # the migrated origin serves /project/. `flow_host_kind` is the codebase's + # exact-host classifier (a substring test matches any URL merely mentioning the + # host in a ?continue= param), and it answers for both cohorts. + try: + await page.wait_for_url(lambda u: flow_host_kind(u) is not None, timeout=30_000) + except PlaywrightTimeoutError as exc: raise FlowAccountChooserError( detail=( f"Clicked recorded account '{email}' on the chooser but the session " - f"did not reach the Flow editor." + f"did not reach Flow within 30s." ) - ) + ) from exc logger.info( "client.account_chooser_autoselected", account=redact_sensitive_text(email), diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index 6d637402..6bb70b6f 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -6,6 +6,7 @@ from unittest.mock import AsyncMock, MagicMock import pytest +from playwright.async_api import TimeoutError as PlaywrightTimeoutError from gflow_cli.errors import FlowAccountChooserError @@ -41,14 +42,24 @@ async def test_bootstrap_detects_chooser_and_autoselects_account(tmp_path: Path) "https://accounts.google.com/v3/signin/accountchooser?continue=flow.google.com", row_count=1, ) - page.wait_for_url = AsyncMock(return_value="https://flow.google.com/project/p1") + # `Page.wait_for_url` is annotated `-> None`: it returns nothing and signals a miss + # by raising. A mock that returns a URL string encodes a contract Playwright does + # not have, and it hid an inverted check that made every successful click raise. + page.wait_for_url = AsyncMock(return_value=None) res = await client._handle_account_chooser(page, "user@example.com") assert res is True # The exact row selector is used, and it is clicked assert page.locator.call_args[0][0] == '[data-email="user@example.com"]' row.first.click.assert_awaited_once() - page.wait_for_url.assert_awaited_once_with("**/project/**", timeout=30_000) + page.wait_for_url.assert_awaited_once() + # The landing predicate accepts BOTH Flow cohorts and rejects the chooser itself, + # so it cannot be satisfied by simply still being on accounts.google.com. + predicate = page.wait_for_url.call_args[0][0] + assert predicate("https://labs.google/fx/tools/flow?hl=en") is True + assert predicate("https://flow.google.com/project/p1") is True + assert predicate("https://accounts.google.com/v3/signin/accountchooser") is False + assert page.wait_for_url.call_args[1]["timeout"] == 30_000 @pytest.mark.asyncio @@ -127,13 +138,15 @@ async def test_bootstrap_chooser_click_no_editor_raises_flow_account_chooser_err "https://accounts.google.com/v3/signin/accountchooser", row_count=1, ) - page.wait_for_url = AsyncMock( - return_value="https://accounts.google.com/v3/signin/accountchooser" - ) + # A landing that never happens is a Playwright TimeoutError out of wait_for_url — + # the real failure signal, not a returned URL. + page.wait_for_url = AsyncMock(side_effect=PlaywrightTimeoutError("timed out")) with pytest.raises(FlowAccountChooserError) as exc_info: await client._handle_account_chooser(page, "user@example.com") - assert "did not reach the Flow editor" in str(exc_info.value) + assert "did not reach Flow" in str(exc_info.value) + # The Playwright timeout is chained, not swallowed, so the bundle keeps the cause. + assert isinstance(exc_info.value.__cause__, PlaywrightTimeoutError) @pytest.mark.asyncio From 4d9471f4dc35cb97847459d4ca504349fdece461 Mon Sep 17 00:00:00 2001 From: Flavio Oliva Date: Wed, 9 Sep 2026 08:33:08 +0100 Subject: [PATCH 06/12] test(auth): cover the chooser click-through against a real Playwright page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The defect the previous commit fixes shipped through a green unit suite because that suite mocked `page.wait_for_url` as returning a URL string — a contract Playwright does not have. A mock cannot falsify a belief about the mocked thing, so this adds tests that drive a real Chromium: real locator engine, real click, real navigation, real wait_for_url. Zero cost and no Google account: the chooser and both Flow landings are served by route interception, which also makes it deterministic — a real signed-out chooser cannot be staged on demand. Covers the branch production actually uses (the .gflow_account read, which every unit test bypasses by passing the address in), both host cohorts, the real-timeout failure path, and the wrong-account hazard that motivates the exact [data-email=] match. A/B: 4 passed against the fix; 3 failed against the original client.py, the click-through cases dying on an uncaught Playwright TimeoutError — `waiting for navigation to "**/project/**"` — which is the #763 symptom. Also types `_handle_account_chooser(page: Page)`. It was `page: Any`, which is what let the inverted check past `pyright src` in the first place; with a real type the landing predicate's parameter is known and the gate has something to check. --- tests/e2e/test_account_chooser_e2e.py | 178 ++++++++++++++++++++++++++ 1 file changed, 178 insertions(+) create mode 100644 tests/e2e/test_account_chooser_e2e.py diff --git a/tests/e2e/test_account_chooser_e2e.py b/tests/e2e/test_account_chooser_e2e.py new file mode 100644 index 00000000..c2bdec9f --- /dev/null +++ b/tests/e2e/test_account_chooser_e2e.py @@ -0,0 +1,178 @@ +"""E2E for the post-migration account-chooser autoselect path (#763). + +**Why this is an e2e and not one more unit test.** The defect this file pins +shipped through a fully green unit suite: the suite mocked ``page.wait_for_url`` +as returning a URL string, so ``(await page.wait_for_url(...) or "")`` looked +like it could be truthy. Playwright's ``wait_for_url`` is annotated ``-> None`` +and signals a miss by *raising*, so the mock encoded a contract the real API does +not have, and the inverted check beneath it — which made every *successful* +chooser click raise — was invisible to every assertion. A mock cannot falsify a +belief about the mocked thing. Only a real ``Page`` can. + +So these tests drive a **real Playwright page** through the real locator engine, +a real click, a real navigation and the real ``wait_for_url``. + +**Cost: zero.** Both the chooser and the Flow landing are served by Playwright +route interception, so no request reaches Google, no Flow credit is spent, and +no authenticated profile is required — see +``docs/superpowers/memory/credit-free-route-abort-verification.md``. That also +makes the test deterministic: the real Google chooser cannot be staged on demand. + +The DOM here is a stand-in for Google's markup, so this proves the *mechanism* +(guard → locate → click → land), not that ``[data-email]`` is still the live +chooser's anchor. Selector drift on the real page is a separate question, and +``/gflow:live-verify`` on a signed-out account is what answers it. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from playwright.async_api import Route, async_playwright + +from gflow_cli.api.client import FlowApiClient +from gflow_cli.errors import FlowAccountChooserError +from gflow_cli.profile_store import ACCOUNT_FILE + +pytestmark = [pytest.mark.e2e, pytest.mark.e2e_auth] + +CHOOSER_URL = "https://accounts.google.com/v3/signin/accountchooser?continue=flow" +LABS_LANDING = "https://labs.google/fx/tools/flow?hl=en" +MIGRATED_LANDING = "https://flow.google.com/project/e2e-project" +ACCOUNT = "e2e-chooser@example.com" +OTHER_ACCOUNT = "someone-else@example.com" + + +def _chooser_html(target_href: str) -> str: + """A chooser carrying two rows, the recorded account second. + + The decoy is first in DOM order on purpose: ``.first`` must resolve within + the *matched* set, so a selector that over-matches would click the wrong + account — and on a real chooser that signs in, and bills, the wrong person. + """ + return f""" + + +""" + + +async def _serve(page: object, chooser_html: str) -> None: + """Route the chooser and both Flow cohorts to local HTML — nothing leaves the box.""" + + async def _chooser(route: Route) -> None: + await route.fulfill(status=200, content_type="text/html", body=chooser_html) + + async def _flow(route: Route) -> None: + await route.fulfill( + status=200, + content_type="text/html", + body="flow", + ) + + await page.route("https://accounts.google.com/**", _chooser) # type: ignore[attr-defined] + await page.route("https://labs.google/**", _flow) # type: ignore[attr-defined] + await page.route("https://flow.google.com/**", _flow) # type: ignore[attr-defined] + + +@pytest.mark.parametrize("landing", [LABS_LANDING, MIGRATED_LANDING], ids=["labs", "migrated"]) +async def test_e2e_chooser_autoselect_lands_on_either_cohort(tmp_path: Path, landing: str) -> None: + """A real click on the recorded row returns True and leaves the chooser. + + Parameterised over both cohorts because the landing predicate is the part + this fix changed: a ``**/project/**`` glob describes only the migrated + origin, while the labs bootstrap URL has no ``/project/`` segment at all. + Whichever host an account resolves to, leaving the chooser must count. + + ``account_email`` is deliberately NOT passed, so this drives the branch + production actually uses — the ``.gflow_account`` read — which every unit + test bypasses by passing the address in. + """ + profile = tmp_path / "profile_e2e" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text(f"{ACCOUNT}\n", encoding="utf-8") + + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + await _serve(page, _chooser_html(landing)) + await page.goto(CHOOSER_URL, wait_until="domcontentloaded") + + client = FlowApiClient(profile_dir=profile) + assert await client._handle_account_chooser(page) is True + + # Landed on a Flow host, and specifically the one the row pointed at. + assert page.url.startswith(landing.split("?")[0]), ( + f"expected to land on {landing}, still at {page.url}" + ) + assert "accounts.google.com" not in page.url + finally: + await browser.close() + + +async def test_e2e_chooser_click_that_never_lands_raises_exit_38(tmp_path: Path) -> None: + """A click that does not leave the chooser raises FlowAccountChooserError. + + The row's href is a same-page anchor, so the click is real and lands + nowhere. That makes ``wait_for_url`` raise a genuine Playwright + ``TimeoutError`` — the failure signal the code must catch and translate. + Before the fix this branch was unreachable for the opposite reason: the + check was inverted, so it fired on success and the real timeout escaped + uncaught as a generic exit 1, which is the #763 symptom itself. + + Costs the handler's full 30 s wait; that is the price of proving the real + timeout rather than a mocked one. + """ + profile = tmp_path / "profile_e2e" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text(f"{ACCOUNT}\n", encoding="utf-8") + + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + await _serve(page, _chooser_html("#stay-put")) + await page.goto(CHOOSER_URL, wait_until="domcontentloaded") + + client = FlowApiClient(profile_dir=profile) + with pytest.raises(FlowAccountChooserError) as exc_info: + await client._handle_account_chooser(page) + + assert ACCOUNT in str(exc_info.value) + assert "did not reach Flow" in str(exc_info.value) + finally: + await browser.close() + + +async def test_e2e_chooser_absent_row_raises_before_any_click(tmp_path: Path) -> None: + """A recorded account with no row raises without clicking anything. + + The wrong-account hazard is the reason the selector is an exact + ``[data-email=]`` match: this chooser offers a different address, and a + substring or text-engine fallback that matched it would sign in — and bill — + the wrong person. Nothing here may be clickable. + """ + profile = tmp_path / "profile_e2e" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("not-on-this-chooser@example.com\n", encoding="utf-8") + + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + await _serve(page, _chooser_html(LABS_LANDING)) + await page.goto(CHOOSER_URL, wait_until="domcontentloaded") + + client = FlowApiClient(profile_dir=profile) + with pytest.raises(FlowAccountChooserError) as exc_info: + await client._handle_account_chooser(page) + + assert "not-on-this-chooser@example.com" in str(exc_info.value) + # Still on the chooser: no row was clicked, so no wrong account was picked. + assert "accounts.google.com" in page.url + finally: + await browser.close() From 8a8793025de3d348875c76de3f2a8f9332439dfd Mon Sep 17 00:00:00 2001 From: Flavio Oliva Date: Wed, 9 Sep 2026 09:09:43 +0100 Subject: [PATCH 07/12] fix(auth): type the chooser page so pyright can see the predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `page: Any` erased `wait_for_url`'s signature, so the landing predicate's parameter was unknown and `pyright src` failed with reportUnknownLambdaType — the same looseness that let the original inverted check through the gate. This was claimed in 970d9aef and was not in it: an A/B control there restored client.py from HEAD, which at that moment predated the edit, so the change was silently reverted before the commit. CI caught it on all three Python versions. --- src/gflow_cli/api/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index 887045fb..07c4da7f 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -789,7 +789,7 @@ async def _enter_setup(self) -> None: # S1 can share this context rather than opening its own. await self._setup_transport() - async def _handle_account_chooser(self, page: Any, account_email: str | None = None) -> bool: + async def _handle_account_chooser(self, page: Page, account_email: str | None = None) -> bool: """Select the recorded Google account on accountchooser if encountered (#763). Returns True if an account was clicked, False if not on chooser. From fbbbaf307701faf1a2072816fdc7e4e5fa238544 Mon Sep 17 00:00:00 2001 From: stgmt Date: Wed, 9 Sep 2026 15:22:27 +0300 Subject: [PATCH 08/12] fix(auth): address round-5 council review Exact-host chooser gate via urlsplit (substring ?continue= can no longer misfire); rejected-browser route imported from its single source instead of an inline literal; USAGE row 38 names only gflow auth login; memory carve-out corrected to match the code (no bot-rejection inclusion, no URL-kind claim); account addresses get their own pattern in the redaction module with call sites wired; session locale re-read from the editor after click-through; account_email test seam removed; --account success-path test added. Signed-off-by: stgmt --- docs/USAGE.md | 2 +- .../memory/ui-selector-drift-error-exit-23.md | 10 ++++-- src/gflow_cli/api/client.py | 30 ++++++++++------- src/gflow_cli/cli.py | 5 +-- src/gflow_cli/data/redaction.py | 32 ++++++++++++------- tests/api/test_bootstrap_chooser.py | 10 +++--- tests/auth/test_account_autoselect.py | 26 +++++++++++++++ website/docs/USAGE.md | 2 +- 8 files changed, 83 insertions(+), 34 deletions(-) diff --git a/docs/USAGE.md b/docs/USAGE.md index 6477bbc2..fd022478 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1787,7 +1787,7 @@ shell scripts can branch on the failure mode without parsing stderr. | `35` | `ExtendUnavailableError` | No Veo extend model is orderable for this account and aspect — the extend family is tier-gated and there is no square variant. **Never auto-retry**: a tier gate does not clear on its own. | | `36` | `FlowHostMigratedError` | Flow served the project from `flow.google.com` and the request could not be represented by the migrated composer, or `GFLOW_CLI_FLOW_HOST=labs.google` disabled it. Supported today: `video t2v`; local-file video i2v/r2v; `image t2i`; and local-file `image i2i`. Image UUID/entity/instruction/Imagen-4 forms, `image batch`, and the `3:4` image aspect remain unsupported. Not selector drift (23) | **Not retryable.** Use one of the supported forms — `--project` is required for images as well as video — or the REST surface (`gflow project list`, `gflow data …`); follow #639 for the remaining matrix | | `37` | `InsufficientCreditsError` | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | -| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | +| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, while signed in as the recorded account (re-run `gflow auth login` if the chooser offers a different session) | | `130`| SIGINT | User-interrupted (Ctrl-C) | — | **Exit code 16 — data store / migration error.** Fires when: diff --git a/docs/superpowers/memory/ui-selector-drift-error-exit-23.md b/docs/superpowers/memory/ui-selector-drift-error-exit-23.md index 17705046..efda3e37 100644 --- a/docs/superpowers/memory/ui-selector-drift-error-exit-23.md +++ b/docs/superpowers/memory/ui-selector-drift-error-exit-23.md @@ -23,8 +23,12 @@ raises `FlowAccountChooserError` (exit 38), NOT `UiSelectorDriftError` (exit 23) The chooser is Google-auth UI, not the Flow editor: reporting it as drift would tell users to file a frontend bug about a working chooser, and the exit-23 remediation (attach diagnostics, check for a release) cannot fix a missing -account row. The miss is evidence about the *recorded account* (absent row, -signed-out row, bot-rejection hop), so the typed error carries the observed URL -kind and names `gflow auth login --profile ` as the recovery. Precedent: +account row. The miss is evidence about the *recorded account* (absent row or +a click-through that never reaches the editor). Each raise site interpolates the +observed chooser URL verbatim — there is no URL-kind taxonomy. Explicitly out +of scope: the bot-rejection hop (`.../v3/signin/rejected`) is excluded from the +chooser gate and surfaces as its own error, never as a missing account. +Recovery is `gflow auth login --profile ` while signed in as the recorded +account. Precedent: exits 36 (`FlowHostMigratedError`) and 37 (`InsufficientCreditsError`) each got the same carve-out recorded when introduced. diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index 07c4da7f..a4abdfbe 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -80,6 +80,7 @@ parse_video_status, ) from gflow_cli.api.video_extend import ExtendStarted +from gflow_cli.auth.internal_chromium import GOOGLE_REJECTED_BROWSER_ROUTE from gflow_cli.browser_manager import channel_for_profile from gflow_cli.config import BrowserEngine, Settings from gflow_cli.diagnostics import IncidentRecorder, run_retention, validated_incidents_root @@ -789,7 +790,7 @@ async def _enter_setup(self) -> None: # S1 can share this context rather than opening its own. await self._setup_transport() - async def _handle_account_chooser(self, page: Page, account_email: str | None = None) -> bool: + async def _handle_account_chooser(self, page: Page) -> bool: """Select the recorded Google account on accountchooser if encountered (#763). Returns True if an account was clicked, False if not on chooser. @@ -802,13 +803,17 @@ async def _handle_account_chooser(self, page: Page, account_email: str | None = from gflow_cli.profile_store import read_account_file url = getattr(page, "url", "") or "" - # accounts.google.com is a sign-in surface. Anything else is the caller's - # page, not a chooser; the rejected-browser hop is not a chooser either - # and must surface as its own error rather than a missing account. - if "accounts.google.com" not in url or "v3/signin/rejected" in url: + # Exact host match, never a substring test: a Flow URL merely carrying + # accounts.google.com in a ?continue= param must not read as a chooser. + # The rejected-browser hop is not a chooser either and must surface as + # its own error rather than a missing account. + parts = urlsplit(url) + host = (parts.hostname or "").lower() + is_accounts_host = parts.scheme == "https" and host == "accounts.google.com" + if not is_accounts_host or GOOGLE_REJECTED_BROWSER_ROUTE in url: return False - email = account_email or read_account_file(self.profile_dir) + email = read_account_file(self.profile_dir) if not email: raise FlowAccountChooserError( detail=( @@ -897,11 +902,14 @@ async def _bootstrap_and_resolve_locale(self) -> None: self._page, settle=settle ) # #763: the chooser hop lands through the same post-goto redirect chain as - # the locale hop, so it is observable only after the settle above. On a - # chooser the settle reads off accounts.google.com — safe: - # write_account_locale runs only on the redirected branch below, which - # the chooser never reaches. - await self._handle_account_chooser(self._page) + # the locale hop, so it is observable only after the settle above. + # write_account_locale below runs whenever settle is True — chooser or + # not — but only folds a non-None from_url, which a chooser page never + # yields, so the on-disk cache is safe. self._account_locale is NOT: + # it would carry accounts.google.com's for the rest of + # the run, so on a click-through it is re-read from the editor below. + if await self._handle_account_chooser(self._page): + self._account_locale, _ = await self._resolve_account_locale(self._page, settle=False) if not settle: # Kept (not merged into account_locale_state) because field reports key # on this event to tell "the settle was skipped" from "it timed out". diff --git a/src/gflow_cli/cli.py b/src/gflow_cli/cli.py index 11566915..29dad141 100644 --- a/src/gflow_cli/cli.py +++ b/src/gflow_cli/cli.py @@ -34,6 +34,7 @@ from gflow_cli.cli_video import video as _video_group from gflow_cli.config import get_settings, warn_if_removed_gemini_key_set from gflow_cli.observability import DEBUG_LEVEL, configure_logging +from gflow_cli.redaction import redact_sensitive_text from gflow_cli.update_check import UpdateNotice, maybe_notify_update logger = structlog.get_logger(__name__) @@ -298,8 +299,8 @@ def auth_login(profile: str | None, browser: str | None, account: str | None = N held = actual_account or "nothing recorded" logger.warning( "auth.account_assert_failed", - required=account.strip(), - held=actual_account or None, + required=redact_sensitive_text(account.strip()), + held=redact_sensitive_text(actual_account) if actual_account else None, ) raise FlowAccountChooserError( detail=( diff --git a/src/gflow_cli/data/redaction.py b/src/gflow_cli/data/redaction.py index 6e9e4997..8f142de6 100644 --- a/src/gflow_cli/data/redaction.py +++ b/src/gflow_cli/data/redaction.py @@ -16,17 +16,27 @@ # exception message ("HTTP 403: ... Bearer ya29.xxx") would pass through it # verbatim — these patterns cover the prose case. All case-insensitive: header # dumps are frequently lowercased ("cookie: sapisid=..."). -_SECRET_TEXT_PATTERNS = ( - re.compile(r"Bearer\s+[A-Za-z0-9._~+/=-]+", re.IGNORECASE), - re.compile(r"SAPISIDHASH\s+\S+", re.IGNORECASE), +_SECRET_TEXT_PATTERNS: tuple[tuple[re.Pattern[str], str], ...] = ( + (re.compile(r"Bearer\s+[A-Za-z0-9._~+/=-]+", re.IGNORECASE), ""), + (re.compile(r"SAPISIDHASH\s+\S+", re.IGNORECASE), ""), # Google auth cookie pairs — both header ("SAPISID=x") and equals forms. # SAPISIDHASH before SAPISID before the bare SID family so the longest # name wins; \b keeps bare SID from firing inside unrelated words. - re.compile( - r"\b(?:__Secure-(?:next-auth\.session-token|[13]PSID[A-Z]*)" - r"|SAPISIDHASH|SAPISID|APISID|SSID|HSID|OSID|LSID|SID)" - r"\s*=\s*\S+", - re.IGNORECASE, + ( + re.compile( + r"\b(?:__Secure-(?:next-auth\.session-token|[13]PSID[A-Z]*)" + r"|SAPISIDHASH|SAPISID|APISID|SSID|HSID|OSID|LSID|SID)" + r"\s*=\s*\S+", + re.IGNORECASE, + ), + "", + ), + # Account addresses (chooser/login identity in error details and logs). + # Kept distinct from : an address is correlatable PII, + # not a credential, and operators triage chooser failures by cohort. + ( + re.compile(r"[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}", re.IGNORECASE), + "", ), ) # Any whitespace-delimited token carrying signed-query material — covers full @@ -85,11 +95,11 @@ def redact_error_detail(detail: str) -> str: """Scrub a free-text error detail before it is persisted to the DB (#341). Applied to ``GFlowError.to_problem_details()['detail']`` on the FAILED - operation write path. Scrubs bearer/SAPISIDHASH/cookie-pair secrets, drops + operation write path. Scrubs bearer/SAPISIDHASH/cookie-pair secrets and account addresses, drops URLs carrying signed-query material, and truncates post-redaction as defense-in-depth against a scrub bypass. """ - for pattern in _SECRET_TEXT_PATTERNS: - detail = pattern.sub("", detail) + for pattern, replacement in _SECRET_TEXT_PATTERNS: + detail = pattern.sub(replacement, detail) detail = _SIGNED_QUERY_TOKEN_PATTERN.sub("", detail) return detail[:ERROR_DETAIL_MAX_CHARS] diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index 6bb70b6f..db5bb9f4 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -47,7 +47,7 @@ async def test_bootstrap_detects_chooser_and_autoselects_account(tmp_path: Path) # not have, and it hid an inverted check that made every successful click raise. page.wait_for_url = AsyncMock(return_value=None) - res = await client._handle_account_chooser(page, "user@example.com") + res = await client._handle_account_chooser(page) assert res is True # The exact row selector is used, and it is clicked assert page.locator.call_args[0][0] == '[data-email="user@example.com"]' @@ -84,7 +84,7 @@ async def test_bootstrap_chooser_absent_account_raises_flow_account_chooser_erro page.get_by_text = MagicMock(return_value=MagicMock(count=AsyncMock(return_value=0))) with pytest.raises(FlowAccountChooserError) as exc_info: - await client._handle_account_chooser(page, "recorded@example.com") + await client._handle_account_chooser(page) assert "recorded@example.com" in str(exc_info.value) assert EXIT_CODE_MAP[FlowAccountChooserError] == 38 @@ -116,7 +116,7 @@ async def test_bootstrap_chooser_exact_match_never_clicks_superset_account( page.get_by_text = MagicMock(return_value=MagicMock(count=AsyncMock(return_value=0))) with pytest.raises(FlowAccountChooserError): - await client._handle_account_chooser(page, "an@corp.com") + await client._handle_account_chooser(page) row.first.click.assert_not_awaited() page.wait_for_url.assert_not_called() @@ -143,7 +143,7 @@ async def test_bootstrap_chooser_click_no_editor_raises_flow_account_chooser_err page.wait_for_url = AsyncMock(side_effect=PlaywrightTimeoutError("timed out")) with pytest.raises(FlowAccountChooserError) as exc_info: - await client._handle_account_chooser(page, "user@example.com") + await client._handle_account_chooser(page) assert "did not reach Flow" in str(exc_info.value) # The Playwright timeout is chained, not swallowed, so the bundle keeps the cause. assert isinstance(exc_info.value.__cause__, PlaywrightTimeoutError) @@ -162,6 +162,6 @@ async def test_bootstrap_rejected_browser_hop_is_not_a_chooser(tmp_path: Path) - client = FlowApiClient(profile_dir=profile) page, row = _chooser_page("https://accounts.google.com/v3/signin/rejected", row_count=0) - res = await client._handle_account_chooser(page, "user@example.com") + res = await client._handle_account_chooser(page) assert res is False page.locator.assert_not_called() diff --git a/tests/auth/test_account_autoselect.py b/tests/auth/test_account_autoselect.py index a531a08e..3e479dc8 100644 --- a/tests/auth/test_account_autoselect.py +++ b/tests/auth/test_account_autoselect.py @@ -77,3 +77,29 @@ async def _mock_login(name: str, browser: str = "auto", headless: bool = False) assert "Recorded Google account not selectable" in result.output or ( "does not match" in result.output ) + + +def test_auth_login_with_account_match_succeeds( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """gflow auth login --account passes when verified email matches (success path).""" + from click.testing import CliRunner + + from gflow_cli.cli import main as cli + + async def _mock_login(name: str, browser: str = "auto", headless: bool = False) -> Path: + pdir = tmp_path / f"profile_{name}" + pdir.mkdir(parents=True, exist_ok=True) + (pdir / ".gflow_account").write_text("actual@example.com", encoding="utf-8") + return pdir + + monkeypatch.setattr("gflow_cli.auth.login", _mock_login) + monkeypatch.setenv("GFLOW_CLI_HOME", str(tmp_path)) + + runner = CliRunner() + result = runner.invoke( + cli, + ["auth", "login", "--profile", "test", "--account", "Actual@Example.com"], + ) + assert result.exit_code == 0, result.output + assert "Session saved" in result.output diff --git a/website/docs/USAGE.md b/website/docs/USAGE.md index 9ac750f4..143fe3de 100644 --- a/website/docs/USAGE.md +++ b/website/docs/USAGE.md @@ -1787,7 +1787,7 @@ shell scripts can branch on the failure mode without parsing stderr. | `35` | `ExtendUnavailableError` | No Veo extend model is orderable for this account and aspect — the extend family is tier-gated and there is no square variant. **Never auto-retry**: a tier gate does not clear on its own. | | `36` | `FlowHostMigratedError` | Flow served the project from `flow.google.com` and the request could not be represented by the migrated composer, or `GFLOW_CLI_FLOW_HOST=labs.google` disabled it. Supported today: `video t2v`; local-file video i2v/r2v; `image t2i`; and local-file `image i2i`. Image UUID/entity/instruction/Imagen-4 forms, `image batch`, and the `3:4` image aspect remain unsupported. Not selector drift (23) | **Not retryable.** Use one of the supported forms — `--project` is required for images as well as video — or the REST surface (`gflow project list`, `gflow data …`); follow #639 for the remaining matrix | | `37` | `InsufficientCreditsError` | The account's balance is short **for the model it asked for**, so Flow **replaced** the submit control with its `Insufficient credits warning` instead of disabling it. Short, not necessarily empty: measured 2026-09-07, an account holding **50** credits requesting `--model veo-quality` (**100**) rendered the warning. Explicitly **not** selector drift (23): reporting it as drift told users to file a frontend bug over a credit shortfall | Check the balance with `gflow credits user`, then pick a cheaper `--model` (`veo-lite` costs 10), top up, or wait for the allowance to reset. Nothing was submitted, so no credit was spent. `gflow image` draws on a separate daily quota and may still work | -| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, or pass `--account ` | +| `38` | `FlowAccountChooserError` | The post-migration hop landed on Google's account chooser and the profile's recorded account (`.gflow_account`) could not be selected automatically (row absent, click-through did not return to the editor, or `--account` mismatch) | **Not retryable**: run `gflow auth login --profile ` and complete the chooser manually, while signed in as the recorded account (re-run `gflow auth login` if the chooser offers a different session) | | `130`| SIGINT | User-interrupted (Ctrl-C) | — | **Exit code 16 — data store / migration error.** Fires when: From 02fbe979355fa190203e80a92038e0823afed6af Mon Sep 17 00:00:00 2001 From: stgmt Date: Wed, 9 Sep 2026 16:24:29 +0300 Subject: [PATCH 09/12] fix(auth): make chooser gate total for non-string page URLs The exact-host urlsplit gate raised TypeError on mocked pages (url is a MagicMock), reddening every suite that enters the client context. Mirror flow_host_kind discipline: non-string or unparseable URLs return False so a probe error never displaces the real bootstrap failure. Regression test pins it. Signed-off-by: stgmt --- src/gflow_cli/api/client.py | 12 ++++++++++-- tests/api/test_bootstrap_chooser.py | 20 ++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index a4abdfbe..2cf7d158 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -807,8 +807,16 @@ async def _handle_account_chooser(self, page: Page) -> bool: # accounts.google.com in a ?continue= param must not read as a chooser. # The rejected-browser hop is not a chooser either and must surface as # its own error rather than a missing account. - parts = urlsplit(url) - host = (parts.hostname or "").lower() + # Total by construction (same discipline as flow_host_kind): a probe + # error must never displace the real bootstrap failure, and suites + # drive this path with mocked pages whose url is not a string. + if not isinstance(url, str): + return False + try: + parts = urlsplit(url) + host = (parts.hostname or "").lower() + except ValueError: + return False is_accounts_host = parts.scheme == "https" and host == "accounts.google.com" if not is_accounts_host or GOOGLE_REJECTED_BROWSER_ROUTE in url: return False diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index db5bb9f4..ebb7eea3 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -165,3 +165,23 @@ async def test_bootstrap_rejected_browser_hop_is_not_a_chooser(tmp_path: Path) - res = await client._handle_account_chooser(page) assert res is False page.locator.assert_not_called() + + +@pytest.mark.asyncio +async def test_bootstrap_chooser_non_string_url_is_not_a_chooser( + tmp_path: Path, +) -> None: + """A mocked page whose url is not a string must not raise (probe totality).""" + from gflow_cli.api.client import FlowApiClient + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("user@example.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page = MagicMock() + page.url = MagicMock(name="mock.url") + + assert await client._handle_account_chooser(page) is False + page.locator.assert_not_called() From 56c71017b30beb050999094f914d0d384c870d77 Mon Sep 17 00:00:00 2001 From: Flavio Oliva Date: Wed, 9 Sep 2026 19:10:28 +0100 Subject: [PATCH 10/12] fix(auth): name where the chooser click-through actually landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The landing-timeout raise reported only "did not reach Flow within 30s" and omitted the one datum that diagnoses it: the URL the click left the session on. Its sibling raise above interpolates the chooser URL for exactly this reason. This branch fired live on 2026-09-09 against a real Google chooser on profile `ffroliva` — the row was found and clicked, the 30s wait timed out, and the error named no URL, so "a challenge needs a human", "a consent screen needs a click" and "the click never navigated" collapsed into one indistinguishable exit 38. `flow_host_kind` is a host-only match that accepts every Flow landing this codebase knows (`/about` included — verified empirically), so a timeout here is never the predicate being too narrow: the session is still on a Google surface, and which surface is the whole question. The persisted operation row stays clean — redact_error_detail's pattern scrubs an address carried in a ?continue= param. Signed-off-by: Flavio Oliva --- CHANGELOG.md | 4 ++- src/gflow_cli/api/client.py | 12 +++++++- tests/api/test_bootstrap_chooser.py | 43 +++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50576794..5ad177e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,7 +35,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 client raises a dedicated, non-retryable `FlowAccountChooserError` (exit code 38), avoiding generic `UnexpectedError` or selector drift stalls. `gflow auth login` gains an optional `--account ` option to assert that login authenticates - as the required account. + as the required account. When the click-through does not reach Flow, the error + names the URL the session actually landed on, so a Google challenge that needs a + human is distinguishable from a click that never navigated. ### Changed diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index 2cf7d158..29ad9ad2 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -861,10 +861,20 @@ async def _handle_account_chooser(self, page: Page) -> bool: try: await page.wait_for_url(lambda u: flow_host_kind(u) is not None, timeout=30_000) except PlaywrightTimeoutError as exc: + # Where the click left us IS the diagnosis, so the detail has to carry it + # (its sibling raise above interpolates the chooser URL for the same + # reason). `flow_host_kind` is a host-only match that accepts every Flow + # landing this codebase knows, `/about` included, so a timeout here is + # never the predicate being too narrow — the session is still on a Google + # surface. WHICH surface is the whole question: a challenge needs a human, + # a consent screen needs a click, and a URL still equal to `url` above + # means the click never navigated at all. Shipped without this, the branch + # fired live on 2026-09-09 and said only "did not reach Flow within 30s". + landed = page.url raise FlowAccountChooserError( detail=( f"Clicked recorded account '{email}' on the chooser but the session " - f"did not reach Flow within 30s." + f"did not reach Flow within 30s — it is at {landed}." ) ) from exc logger.info( diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index ebb7eea3..c065cf97 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -185,3 +185,46 @@ async def test_bootstrap_chooser_non_string_url_is_not_a_chooser( assert await client._handle_account_chooser(page) is False page.locator.assert_not_called() + + +@pytest.mark.asyncio +async def test_bootstrap_chooser_landing_timeout_names_where_the_page_landed( + tmp_path: Path, +) -> None: + """A landing timeout must report the URL the click actually left us on. + + The sibling raise above interpolates the chooser URL; this branch shipped + without it and fired live on 2026-09-09 saying only "did not reach Flow + within 30s". `flow_host_kind` is a host-only match that accepts every known + Flow landing (including `/about`), so a timeout means the session is still on + a Google surface — and *which* surface is the whole diagnosis: a password + challenge needs a human, a consent screen needs a click, and an unchanged + chooser URL means our click never navigated at all. Without the URL those + are one indistinguishable exit 38. + """ + from gflow_cli.api.client import FlowApiClient + from gflow_cli.profile_store import ACCOUNT_FILE + + profile = tmp_path / "profile_p1" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text("user@example.com\n", encoding="utf-8") + + client = FlowApiClient(profile_dir=profile) + page, row = _chooser_page( + "https://accounts.google.com/v3/signin/accountchooser", + row_count=1, + ) + interstitial = "https://accounts.google.com/signin/v2/challenge/pwd" + + async def _click_moves_to_interstitial(*_args: object, **_kwargs: object) -> None: + page.url = interstitial + + row.first.click = AsyncMock(side_effect=_click_moves_to_interstitial) + page.wait_for_url = AsyncMock(side_effect=PlaywrightTimeoutError("timed out")) + + with pytest.raises(FlowAccountChooserError) as exc_info: + await client._handle_account_chooser(page) + + # The CURRENT url, not the chooser url captured on entry: reporting the entry + # url would claim "still on the chooser" for a click that did navigate. + assert interstitial in str(exc_info.value) From 7a40c6f43d361394106e608c944927278b65f82d Mon Sep 17 00:00:00 2001 From: Flavio Oliva Date: Wed, 9 Sep 2026 19:49:36 +0100 Subject: [PATCH 11/12] fix(auth): match the recorded account case-insensitively on both tiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gflow auth login --account` compares with `.lower()` on both sides (cli.py: `actual_account.lower() != account.strip().lower()`), but the chooser row match did not: a CSS attribute selector is case-sensitive unless given the `i` flag, `get_by_text(..., exact=True)` is case-sensitive, and `read_account_file` normalises nothing. An address recorded in one case and rendered by Google in another therefore passed the `--account` assertion and then missed its row, raising "recorded account was not found among selectable accounts" — exit 38 telling the operator to re-login while the account sits on the chooser. That is the exact false negative this feature exists to remove. Relaxing case must not relax the anti-substring discipline, so the text fallback becomes an ANCHORED case-insensitive pattern rather than a bare one: an unanchored relaxation would match the chooser's "Remove " and "Sign out of " rows, and clicking those signs the operator out instead of in. Proven on a real locator engine, because CSS matching semantics are exactly what a mock cannot model: two new route-intercepted e2e cases (zero credits, no account) — one that a case variant selects its row, one that a superset row is still refused. The two unit tests that asserted the old call shape are updated, and the fallback assertion is now behavioural (matches a case variant; refuses both superset forms) rather than a comparison against call arguments, so an over-broad implementation fails it. Signed-off-by: Flavio Oliva --- CHANGELOG.md | 5 +- src/gflow_cli/api/client.py | 14 ++++- tests/api/test_bootstrap_chooser.py | 16 ++++-- tests/e2e/test_account_chooser_e2e.py | 74 +++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ad177e1..d8a0f3e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 gains an optional `--account ` option to assert that login authenticates as the required account. When the click-through does not reach Flow, the error names the URL the session actually landed on, so a Google challenge that needs a - human is distinguishable from a click that never navigated. + human is distinguishable from a click that never navigated. Account matching is + case-insensitive on both tiers, matching `--account`'s own comparison, so a + recorded address whose case differs from Google's rendering still selects its + row instead of reporting the account as absent. ### Changed diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index 29ad9ad2..ec50b456 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -17,6 +17,7 @@ import base64 import json import os +import re import sys import time import uuid @@ -833,10 +834,19 @@ async def _handle_account_chooser(self, page: Page) -> bool: # Exact row match only (D3): data-email is the chooser's stable per-account # anchor. A substring/text-engine fallback would match "Remove " # or "Sign out of " and click a DOM-order-first wrong account. - row = page.locator(f'[data-email="{email}"]') + # Case-insensitive on BOTH tiers, because `gflow auth login --account` + # already compares with `.lower()` and `read_account_file` normalises + # nothing: an address recorded in one case and rendered by Google in + # another otherwise passes the --account assertion and then misses the + # row, raising "not found among selectable accounts" while the account + # sits on the chooser. CSS attribute matching is case-sensitive unless + # the `i` flag is given; the text fallback stays ANCHORED so relaxing + # case does not start matching "Remove " / "Sign out of " + # — clicking those signs the operator out instead of in. + row = page.locator(f'[data-email="{email}" i]') count = await row.count() if count == 0: - row = page.get_by_text(email, exact=True) + row = page.get_by_text(re.compile(rf"^{re.escape(email)}$", re.IGNORECASE)) count = await row.count() if count == 0: diff --git a/tests/api/test_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py index c065cf97..73182d38 100644 --- a/tests/api/test_bootstrap_chooser.py +++ b/tests/api/test_bootstrap_chooser.py @@ -49,8 +49,10 @@ async def test_bootstrap_detects_chooser_and_autoselects_account(tmp_path: Path) res = await client._handle_account_chooser(page) assert res is True - # The exact row selector is used, and it is clicked - assert page.locator.call_args[0][0] == '[data-email="user@example.com"]' + # The exact row selector is used, and it is clicked. The trailing `i` is the CSS + # case-insensitivity flag: `--account` compares with `.lower()`, so the row match + # must too, or a case variant passes the assert and then misses its row. + assert page.locator.call_args[0][0] == '[data-email="user@example.com" i]' row.first.click.assert_awaited_once() page.wait_for_url.assert_awaited_once() # The landing predicate accepts BOTH Flow cohorts and rejects the chooser itself, @@ -88,7 +90,15 @@ async def test_bootstrap_chooser_absent_account_raises_flow_account_chooser_erro assert "recorded@example.com" in str(exc_info.value) assert EXIT_CODE_MAP[FlowAccountChooserError] == 38 - page.get_by_text.assert_called_once_with("recorded@example.com", exact=True) + # The fallback is an ANCHORED case-insensitive pattern, not `exact=True`. Assert the + # anchoring behaviourally rather than by repr: an unanchored relaxation would match + # the chooser's "Sign out of " row, and clicking that signs the operator out + # instead of in — the precise hazard `exact=True` was there to prevent. + page.get_by_text.assert_called_once() + pattern = page.get_by_text.call_args[0][0] + assert pattern.search("RECORDED@example.com"), "must match a case variant" + assert not pattern.search("Sign out of recorded@example.com"), "must refuse a superset" + assert not pattern.search("Remove recorded@example.com"), "must refuse a superset" @pytest.mark.asyncio diff --git a/tests/e2e/test_account_chooser_e2e.py b/tests/e2e/test_account_chooser_e2e.py index c2bdec9f..c705adef 100644 --- a/tests/e2e/test_account_chooser_e2e.py +++ b/tests/e2e/test_account_chooser_e2e.py @@ -176,3 +176,77 @@ async def test_e2e_chooser_absent_row_raises_before_any_click(tmp_path: Path) -> assert "accounts.google.com" in page.url finally: await browser.close() + + +async def test_e2e_chooser_matches_recorded_account_case_insensitively(tmp_path: Path) -> None: + """A case variant of the recorded address still selects the row. + + `gflow auth login --account` compares case-insensitively (`cli.py`: + ``actual_account.lower() != account.strip().lower()``), but the row match is + a CSS attribute selector and an exact-text fallback, both case-SENSITIVE, and + ``read_account_file`` normalises nothing. So an address recorded in one case + and rendered by Google in another passes the `--account` assertion and then + misses the row — surfacing as "recorded account was not found among + selectable accounts", exit 38, telling the operator to re-login while the + account sits right there on the chooser. That is the exact false negative + this feature exists to remove. + + Only a real locator engine can settle this: CSS attribute matching is + case-sensitive by default and case-insensitive only with the `i` flag, which + no mock can model. Zero credits — route interception, as above. + """ + profile = tmp_path / "profile_e2e" + profile.mkdir() + # Chooser renders ACCOUNT lowercase; the profile records a case variant. + recorded = "E2E-Chooser@Example.com" + assert recorded.lower() == ACCOUNT, "variant must differ only by case" + (profile / ACCOUNT_FILE).write_text(f"{recorded}\n", encoding="utf-8") + + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + await _serve(page, _chooser_html(MIGRATED_LANDING)) + await page.goto(CHOOSER_URL, wait_until="domcontentloaded") + + client = FlowApiClient(profile_dir=profile) + assert await client._handle_account_chooser(page) is True + assert "accounts.google.com" not in page.url + finally: + await browser.close() + + +async def test_e2e_chooser_case_insensitive_match_still_refuses_a_superset_row( + tmp_path: Path, +) -> None: + """Relaxing case must not relax the anti-substring discipline. + + The chooser's loose surfaces ("Remove ", "Sign out of ") are + why the match is exact. A case-insensitive match implemented with an + unanchored regex would start selecting those, and clicking "Sign out of" on + a real chooser signs the operator out instead of in. Here the ONLY row + carrying the address is a superset string, so a correct implementation finds + no exact row and raises rather than clicking it. + """ + profile = tmp_path / "profile_e2e" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text(f"{ACCOUNT}\n", encoding="utf-8") + + superset_only = f""" + + +""" + + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + await _serve(page, superset_only) + await page.goto(CHOOSER_URL, wait_until="domcontentloaded") + + client = FlowApiClient(profile_dir=profile) + with pytest.raises(FlowAccountChooserError) as exc_info: + await client._handle_account_chooser(page) + assert "not found among selectable accounts" in str(exc_info.value) + finally: + await browser.close() From e69fde18335f89a292d039055c696d2a2cb82407 Mon Sep 17 00:00:00 2001 From: Flavio Oliva Date: Wed, 9 Sep 2026 21:17:23 +0100 Subject: [PATCH 12/12] fix(auth): six defects found by an xhigh review of the chooser path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review pass over this branch found six real defects, verified before acting on each. Four are in the feature's own commits; two were latent and are reached by the new code. 1. An expired session now exits 3 again, not 38. The host gate accepts EVERY accounts.google.com landing, so an email form, a password challenge or a consent interstitial — none of which offer anything to pick — reached the row lookup, found nothing and raised exit 38 claiming the recorded account was "not found among selectable accounts". That both misdirects the operator and changes an exit code callers branch on: the path previously continued to the transport's 401 and classified as AuthExpiredError (exit 3). A chooser is now identified positively, by Google's chooser path OR the presence of account rows; neither matching returns False, exactly as before the feature existed. Two signals because each covers the other's blind spot — a renamed path, or a chooser whose rows carry no data-email. 2. A chooser hop no longer demotes a learned locale. The post-click resolve discarded its from_url, so the fold below ran next_locale_state(cached, None) — which returns PROVISIONAL for a committed segment — and wrote that to disk on every chooser hop (#643's bug class). The code logged its own bug: `client.account_locale_state now=? was=pt`. The comment above it claimed "the on-disk cache is safe"; it was not. The editor's segment is folded now. 3. FlowAccountChooserError no longer triggers incident capture. It was in both _capture_triggers() and _screenshot_triggers(), and by construction it fires only while the page is on accounts.google.com — so a bundle would carry a DOM dump and a full-page screenshot of a Google auth surface (every signed-in identity's address, name and avatar; on a sign-in form, the input and hidden-field DOM) into the artifact users are prompted to attach to GitHub issues. diagnostics.py already excludes AuthExpiredError as "deterministic operator remediation", and docs/DEBUGGING.md says so; this has the same remediation and now the same rule. 4. The pattern no longer eats filesystem paths. It matched `x@y.co` inside `C:/x@y.co/path` and `/var/x@y.io/cache`, and it runs on every persisted error detail, transport snippet and worker payload — silently rewriting the one artifact left for debugging a failure. Bounded by lookaround so a match cannot start after a path separator or continue into a path segment; `?Email=me@x.com` still scrubs. It shipped with no test at all. 5. The mismatch log event says something again. redact_sensitive_text maps every address to one constant, so `required=` and `held=` were two identical tokens and no signal — while the console prints both in the clear on the next line. Replaced with held_recorded, the distinction the event exists to make. 6. .gflow_account is treated as untrusted input. Its value is interpolated into a CSS attribute selector, where a double quote closes the attribute early and makes locator.count() raise a raw Playwright parse error past every typed handler — a generic exit 1, the symptom class #763 exists to remove. The reader also decoded UTF-8 while catching only OSError, so a damaged file broke list_profiles() for every profile. Guarded once in the shared reader, which fixes both callers and any I did not enumerate. Full suite 4168 passed / 7 skipped / 92% coverage; pyright unchanged from the develop baseline; ruff, hygiene, doc-links, website-mirror and PII gates green. Signed-off-by: Flavio Oliva --- CHANGELOG.md | 10 +++ src/gflow_cli/api/client.py | 30 +++++++-- src/gflow_cli/cli.py | 8 ++- src/gflow_cli/data/redaction.py | 14 ++++- src/gflow_cli/diagnostics.py | 13 ++-- src/gflow_cli/profile_store.py | 20 +++++- tests/api/test_client_locale_cache.py | 34 +++++++++++ tests/auth/test_account_autoselect.py | 87 +++++++++++++++++++++++++++ tests/data/test_redaction.py | 25 +++++++- tests/e2e/test_account_chooser_e2e.py | 45 ++++++++++++++ tests/test_diagnostics_recorder.py | 10 +++ 11 files changed, 276 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8a0f3e8..8dd2e927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 recorded address whose case differs from Google's rendering still selects its row instead of reporting the account as absent. +### Fixed + +- **`gflow auth list` no longer fails on a profile whose `.gflow_account` is + damaged.** The reader decoded as UTF-8 and caught only `OSError`, so a + non-UTF-8 or truncated file raised out of `list_profiles()` and broke the + listing for *every* profile, not just the damaged one. The value is also + interpolated into a DOM attribute selector, where a stray quote produced an + untyped failure; unusable content now reads as "no account recorded", which + every caller already handles. + ### Changed - **`gflow auth login` closes the browser for you.** It drives your real Google Chrome diff --git a/src/gflow_cli/api/client.py b/src/gflow_cli/api/client.py index ec50b456..6bcdbc08 100644 --- a/src/gflow_cli/api/client.py +++ b/src/gflow_cli/api/client.py @@ -822,6 +822,19 @@ async def _handle_account_chooser(self, page: Page) -> bool: if not is_accounts_host or GOOGLE_REJECTED_BROWSER_ROUTE in url: return False + # Identify a chooser POSITIVELY. The host gate accepts every + # accounts.google.com landing, and most are not choosers — the email form, a + # password challenge, a consent interstitial — where there is nothing to pick. + # Reporting those as a chooser misdirects the operator and changes an exit code + # callers branch on: they otherwise reach the transport's 401 and classify as + # AuthExpiredError (exit 3). Two independent signals, because each covers the + # other's blind spot — Google renaming the path, or a chooser whose rows carry + # no data-email. Neither matching means we return False, which is exactly how + # this path behaved before the feature existed. + on_chooser_path = parts.path.rstrip("/").endswith("accountchooser") + if not on_chooser_path and await page.locator("[data-email]").count() == 0: + return False + email = read_account_file(self.profile_dir) if not email: raise FlowAccountChooserError( @@ -931,13 +944,18 @@ async def _bootstrap_and_resolve_locale(self) -> None: ) # #763: the chooser hop lands through the same post-goto redirect chain as # the locale hop, so it is observable only after the settle above. - # write_account_locale below runs whenever settle is True — chooser or - # not — but only folds a non-None from_url, which a chooser page never - # yields, so the on-disk cache is safe. self._account_locale is NOT: - # it would carry accounts.google.com's for the rest of - # the run, so on a click-through it is re-read from the editor below. + # BOTH outputs of the first resolve are the chooser's, and both must be + # replaced. `self._account_locale` would otherwise carry + # accounts.google.com's for the rest of the run. `from_url` + # is subtler and was wrong: a chooser yields None, and + # `next_locale_state(cached="pt", observed=None)` returns PROVISIONAL, so + # the fold below wrote a DEMOTION of a committed locale on every chooser + # hop (#643's bug class). The post-click resolve holds the editor's real + # segment — fold that. if await self._handle_account_chooser(self._page): - self._account_locale, _ = await self._resolve_account_locale(self._page, settle=False) + self._account_locale, from_url = await self._resolve_account_locale( + self._page, settle=False + ) if not settle: # Kept (not merged into account_locale_state) because field reports key # on this event to tell "the settle was skipped" from "it timed out". diff --git a/src/gflow_cli/cli.py b/src/gflow_cli/cli.py index 29dad141..8d6c579b 100644 --- a/src/gflow_cli/cli.py +++ b/src/gflow_cli/cli.py @@ -34,7 +34,6 @@ from gflow_cli.cli_video import video as _video_group from gflow_cli.config import get_settings, warn_if_removed_gemini_key_set from gflow_cli.observability import DEBUG_LEVEL, configure_logging -from gflow_cli.redaction import redact_sensitive_text from gflow_cli.update_check import UpdateNotice, maybe_notify_update logger = structlog.get_logger(__name__) @@ -299,8 +298,11 @@ def auth_login(profile: str | None, browser: str | None, account: str | None = N held = actual_account or "nothing recorded" logger.warning( "auth.account_assert_failed", - required=redact_sensitive_text(account.strip()), - held=redact_sensitive_text(actual_account) if actual_account else None, + # Not the addresses: redact_sensitive_text maps every address to + # one constant, so those two fields were identical tokens and no + # signal, while the console prints both in the clear below. What + # the event can carry is the distinction it exists to make. + held_recorded=actual_account is not None, ) raise FlowAccountChooserError( detail=( diff --git a/src/gflow_cli/data/redaction.py b/src/gflow_cli/data/redaction.py index 8f142de6..f9e55344 100644 --- a/src/gflow_cli/data/redaction.py +++ b/src/gflow_cli/data/redaction.py @@ -35,7 +35,19 @@ # Kept distinct from : an address is correlatable PII, # not a credential, and operators triage chooser failures by cohort. ( - re.compile(r"[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}", re.IGNORECASE), + # Bounded so a path is not mistaken for an address: `C:/x@y.co/path` and + # `/var/x@y.io/cache` are ordinary paths, and this pattern runs on EVERY + # persisted error detail, transport snippet and worker payload — the one + # artifact left for debugging a failure, where a silent rewrite cannot be + # told apart from the original text. The lookbehind rejects a match starting + # mid-token or right after a path separator; the lookahead rejects one that + # continues into a path segment. + re.compile( + r"(?", ), ) diff --git a/src/gflow_cli/diagnostics.py b/src/gflow_cli/diagnostics.py index 2c990e83..42e55718 100644 --- a/src/gflow_cli/diagnostics.py +++ b/src/gflow_cli/diagnostics.py @@ -1019,6 +1019,7 @@ def should_capture(self, exc: BaseException) -> bool: from gflow_cli.errors import ( AuthExpiredError, ContentPolicyError, + FlowAccountChooserError, GFlowError, ProfileLockedError, ) @@ -1027,8 +1028,12 @@ def should_capture(self, exc: BaseException) -> bool: return False if not isinstance(exc, Exception): return False # cancellation/KeyboardInterrupt/SystemExit are not incidents - if isinstance(exc, (ContentPolicyError, AuthExpiredError)): - return False # deterministic operator remediation; DOM adds nothing + if isinstance(exc, (ContentPolicyError, AuthExpiredError, FlowAccountChooserError)): + # Deterministic operator remediation; DOM adds nothing. The chooser error + # additionally fires ONLY while the page is on accounts.google.com, so a + # bundle would carry a DOM dump and a full-page screenshot of a Google auth + # surface into the artifact users are prompted to attach to GitHub issues. + return False if isinstance(exc, ProfileLockedError): return True # metadata-only incident if isinstance(exc, _capture_triggers()): @@ -1847,7 +1852,6 @@ def _validate_overlay(raw: dict[str, object]) -> dict[str, object]: def _capture_triggers() -> tuple[type[BaseException], ...]: from gflow_cli.errors import ( BrowserSessionClosedError, - FlowAccountChooserError, FlowAgentUiError, FlowAppError, FlowHostMigratedError, @@ -1860,7 +1864,6 @@ def _capture_triggers() -> tuple[type[BaseException], ...]: ) return ( - FlowAccountChooserError, FlowAppError, FlowAgentUiError, # #639: this arm REPLACED UiSelectorDriftError on the migrated frontend. @@ -1879,7 +1882,6 @@ def _capture_triggers() -> tuple[type[BaseException], ...]: def _screenshot_triggers() -> tuple[type[BaseException], ...]: from gflow_cli.errors import ( - FlowAccountChooserError, FlowAgentUiError, FlowAppError, FlowHostMigratedError, @@ -1889,7 +1891,6 @@ def _screenshot_triggers() -> tuple[type[BaseException], ...]: ) return ( - FlowAccountChooserError, FlowAppError, FlowAgentUiError, FlowHostMigratedError, diff --git a/src/gflow_cli/profile_store.py b/src/gflow_cli/profile_store.py index c3eddccf..1ed5dfc6 100644 --- a/src/gflow_cli/profile_store.py +++ b/src/gflow_cli/profile_store.py @@ -351,12 +351,26 @@ def account_locale_for(profile_name: str) -> str | None: def read_account_file(profile_path: Path) -> str | None: - """Read the Google account email from the profile's .gflow_account file.""" + """Read the Google account email from the profile's .gflow_account file. + + The file is untrusted input — a truncated write, a hand edit, a Google + display string. Its value is interpolated into a CSS attribute selector + (``[data-email="{email}" i]``), where a double quote closes the attribute + early and makes ``locator.count()`` raise a raw Playwright parse error that + escapes every typed handler as a generic exit 1. Guarding here rather than + at the call site fixes every caller at once: the chooser raises its own + "nothing recorded" error, and ``list_profiles`` keeps working on a profile + whose file is damaged (it decodes as UTF-8, so a non-UTF-8 file otherwise + breaks ``gflow auth list`` for every profile, not just the damaged one). + """ account_file = profile_path / ACCOUNT_FILE try: - return account_file.read_text(encoding="utf-8").strip() or None - except OSError: + raw = account_file.read_text(encoding="utf-8").strip() + except (OSError, UnicodeDecodeError): + return None + if not raw or "@" not in raw or '"' in raw or any(c.isspace() for c in raw): return None + return raw def _last_modified(path: Path) -> datetime | None: diff --git a/tests/api/test_client_locale_cache.py b/tests/api/test_client_locale_cache.py index 3162a84a..4f35b0be 100644 --- a/tests/api/test_client_locale_cache.py +++ b/tests/api/test_client_locale_cache.py @@ -439,3 +439,37 @@ async def test_the_settle_wait_is_skipped_when_the_url_already_answered( assert client._account_locale == "pt" assert page.lang_probed is False, "the URL answered; do not touch " assert page.lang_waited is False, "the URL answered; do not pay the settle-wait" + + +async def test_chooser_hop_does_not_demote_a_learned_locale( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A chooser click-through must fold the EDITOR's locale, not the chooser's. + + The chooser page yields no locale segment, so the first resolve returns + ``from_url=None``. `next_locale_state(cached="pt", observed=None)` returns + PROVISIONAL — a demotion — and it is written to disk. The post-click resolve + holds the real segment and used to discard it (`self._account_locale, _ =`), + so every chooser hop quietly downgraded a committed locale, which is the bug + class of #643. The comment above the call claimed "the on-disk cache is safe"; + it was not. + """ + from unittest.mock import MagicMock + + write_account_locale(tmp_path, "pt") + + page = MagicMock() + page.goto = AsyncMock(return_value=None) + client = FlowApiClient(tmp_path) + client._page = page # type: ignore[assignment] + + # First resolve runs on the chooser: no segment. Second runs on the editor. + resolves = iter([("en", None), ("pt", "pt")]) + monkeypatch.setattr( + client, "_resolve_account_locale", AsyncMock(side_effect=lambda *a, **k: next(resolves)) + ) + monkeypatch.setattr(client, "_handle_account_chooser", AsyncMock(return_value=True)) + + await client._bootstrap_and_resolve_locale() + + assert read_account_locale(tmp_path) == "pt", "a chooser hop must not demote a learned locale" diff --git a/tests/auth/test_account_autoselect.py b/tests/auth/test_account_autoselect.py index 3e479dc8..948bfc1b 100644 --- a/tests/auth/test_account_autoselect.py +++ b/tests/auth/test_account_autoselect.py @@ -5,6 +5,7 @@ from pathlib import Path import pytest +import structlog from gflow_cli.errors import ( EXIT_CODE_MAP, @@ -103,3 +104,89 @@ async def _mock_login(name: str, browser: str = "auto", headless: bool = False) ) assert result.exit_code == 0, result.output assert "Session saved" in result.output + + +def test_account_mismatch_event_carries_signal_not_two_identical_tokens( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + install_log_capture: structlog.testing.LogCapture, +) -> None: + """The mismatch event must say something a log-only operator can act on. + + `redact_sensitive_text` maps EVERY address to the one constant + ````, so logging `required=` and `held=` through it emitted + two identical tokens — the event could not answer the only question it + exists for: was a different account held, or none at all? Meanwhile the + console prints both addresses in the clear on the very next line, so the + redaction bought nothing and cost the field its meaning. + """ + from click.testing import CliRunner + + from gflow_cli.cli import main as cli + + async def _mock_login(name: str, browser: str = "auto", headless: bool = False) -> Path: + pdir = tmp_path / f"profile_{name}" + pdir.mkdir(parents=True, exist_ok=True) + (pdir / ".gflow_account").write_text("actual@example.com", encoding="utf-8") + return pdir + + monkeypatch.setattr("gflow_cli.auth.login", _mock_login) + monkeypatch.setenv("GFLOW_CLI_HOME", str(tmp_path)) + # The CLI reconfigures structlog on entry, which would replace the capture + # processor installed by the fixture and swallow every event. + monkeypatch.setattr("gflow_cli.cli.configure_logging", lambda *a, **k: None) + + CliRunner().invoke( + cli, ["auth", "login", "--profile", "test", "--account", "expected@example.com"] + ) + + events = [ + e for e in install_log_capture.entries if e.get("event") == "auth.account_assert_failed" + ] + assert events, "a mismatch must be logged" + event = events[0] + + identical = [v for v in event.values() if v == ""] + assert len(identical) < 2, f"two identical tokens carry no signal: {event}" + assert event["held_recorded"] is True, "must distinguish held-someone-else from held-nothing" + + +class TestAccountFileIsUntrustedInput: + """`.gflow_account` is a file on disk, so its content is untrusted. + + It is interpolated into a CSS attribute selector + (`[data-email="{email}" i]`). A value containing a double quote produces + `[data-email="a"b@x.com" i]`, and `locator.count()` then raises a raw + Playwright parse error that escapes `_handle_account_chooser` past every + FlowAccountChooserError handler — a generic exit 1, which is the symptom + class #763 exists to remove. Guarding the shared reader fixes every caller + at once: the chooser raises its own typed "nothing recorded" error, and + `gflow auth list` keeps working. + """ + + def test_a_quote_bearing_value_reads_as_absent(self, tmp_path: Path) -> None: + from gflow_cli.profile_store import ACCOUNT_FILE, read_account_file + + (tmp_path / ACCOUNT_FILE).write_text('a"b@x.com', encoding="utf-8") + assert read_account_file(tmp_path) is None + + def test_non_utf8_content_reads_as_absent(self, tmp_path: Path) -> None: + from gflow_cli.profile_store import ACCOUNT_FILE, read_account_file + + # read_text catches only OSError today, so this raises UnicodeDecodeError + # out of every caller — including list_profiles(), breaking `gflow auth + # list` for every profile, not just the damaged one. + (tmp_path / ACCOUNT_FILE).write_bytes(b"\xff\xfe not utf 8") + assert read_account_file(tmp_path) is None + + def test_a_value_with_no_at_sign_reads_as_absent(self, tmp_path: Path) -> None: + from gflow_cli.profile_store import ACCOUNT_FILE, read_account_file + + (tmp_path / ACCOUNT_FILE).write_text("truncated-write", encoding="utf-8") + assert read_account_file(tmp_path) is None + + def test_an_ordinary_address_still_reads(self, tmp_path: Path) -> None: + from gflow_cli.profile_store import ACCOUNT_FILE, read_account_file + + (tmp_path / ACCOUNT_FILE).write_text("me@example.com\n", encoding="utf-8") + assert read_account_file(tmp_path) == "me@example.com" diff --git a/tests/data/test_redaction.py b/tests/data/test_redaction.py index fd06e026..763bc85d 100644 --- a/tests/data/test_redaction.py +++ b/tests/data/test_redaction.py @@ -1,4 +1,4 @@ -from gflow_cli.data.redaction import prompt_fields, redact_metadata +from gflow_cli.data.redaction import prompt_fields, redact_error_detail, redact_metadata def test_prompt_fields_store_mode_stores_text_and_hash() -> None: @@ -56,3 +56,26 @@ def test_session_id_is_redacted() -> None: # The project id is a plain resource identifier and stays readable — it is # what makes a bundle diagnosable at all. assert ctx["projectId"] == "7d3d6bd9-a39f-4c2d-b772-146e73e539cf" + + +class TestEmailRedactionScope: + """The `` pattern must scrub addresses without eating paths. + + Added with the account-chooser feature so chooser errors do not persist an + operator's address, and shipped with no test. It runs on EVERY persisted + error detail (`data/recorder.py`), every transport response snippet + (`transports/_common.py`, `transports/batchexecute.py`) and both worker paths + (`worker/codec.py`, `worker/daemon.py`) — so an over-match silently rewrites + the one artifact left for debugging a failure, with nothing to distinguish a + redaction from the original text. + """ + + def test_a_real_address_is_scrubbed(self) -> None: + assert redact_error_detail("contact me@example.com now") == ("contact now") + + def test_a_filesystem_path_is_not_an_address(self) -> None: + # Verified failing before the fix: '' replaced 'x@y.co'. + assert redact_error_detail("file at C:/x@y.co/path") == "file at C:/x@y.co/path" + + def test_a_posix_path_is_not_an_address(self) -> None: + assert redact_error_detail("read /var/x@y.io/cache") == "read /var/x@y.io/cache" diff --git a/tests/e2e/test_account_chooser_e2e.py b/tests/e2e/test_account_chooser_e2e.py index c705adef..81f8f988 100644 --- a/tests/e2e/test_account_chooser_e2e.py +++ b/tests/e2e/test_account_chooser_e2e.py @@ -250,3 +250,48 @@ async def test_e2e_chooser_case_insensitive_match_still_refuses_a_superset_row( assert "not found among selectable accounts" in str(exc_info.value) finally: await browser.close() + + +async def test_e2e_signin_page_is_not_reported_as_a_chooser(tmp_path: Path) -> None: + """An ordinary expired session must not be misreported as a missing account row. + + The host gate accepts ANY https accounts.google.com landing, so an expired + session redirected to the email form — no remembered accounts, nothing to + pick — reaches the row lookup, finds nothing, and raises + FlowAccountChooserError: exit 38, "recorded account was not found among + selectable accounts". Two things are wrong with that. It asserts a chooser + listing other accounts when there is no chooser at all, pointing the operator + at the wrong remediation; and it *changes an exit code callers branch on* — + this path previously continued to the transport's HTTP 401 and surfaced as + AuthExpiredError (exit 3), which is also deliberately excluded from incident + capture. Scripts keyed on exit 3 for re-auth would silently stop matching. + + A chooser is identified structurally, by having account rows at all — not by + its URL, which is Google's to change. Same discipline as the host gate: + parse, never pattern-match a label. + """ + profile = tmp_path / "profile_e2e" + profile.mkdir() + (profile / ACCOUNT_FILE).write_text(f"{ACCOUNT}\n", encoding="utf-8") + + signin_form = """ + +
+""" + + async with async_playwright() as pw: + browser = await pw.chromium.launch(headless=True) + try: + page = await (await browser.new_context()).new_page() + await _serve(page, signin_form) + await page.goto( + "https://accounts.google.com/v3/signin/identifier?continue=flow", + wait_until="domcontentloaded", + ) + + client = FlowApiClient(profile_dir=profile) + # False = "not a chooser", so the caller carries on and the real + # auth failure classifies itself downstream. + assert await client._handle_account_chooser(page) is False + finally: + await browser.close() diff --git a/tests/test_diagnostics_recorder.py b/tests/test_diagnostics_recorder.py index fb159745..4cc7863c 100644 --- a/tests/test_diagnostics_recorder.py +++ b/tests/test_diagnostics_recorder.py @@ -16,6 +16,7 @@ AuthExpiredError, ConfigurationError, ContentPolicyError, + FlowAccountChooserError, FlowAgentUiError, FlowAppError, NetworkError, @@ -91,6 +92,15 @@ def test_trigger_classification_matches_design(self, tmp_path: Path) -> None: ContentPolicyError("expected"), AuthExpiredError("expected"), ConfigurationError("usage"), + # Same policy class as AuthExpiredError: deterministic operator + # remediation, and it fires ONLY while the page is on + # accounts.google.com — so a bundle would carry a DOM dump and a + # full-page screenshot of a Google auth surface (every signed-in + # identity's address, name and avatar; on a sign-in form, the input + # and hidden-field DOM) into the artifact users are told to attach + # to GitHub issues. docs/DEBUGGING.md: "Never captured: ... ordinary + # AuthExpiredError". This has the same remediation, so same rule. + FlowAccountChooserError(detail="expected"), ): assert not rec.should_capture(exc), type(exc).__name__