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/CHANGELOG.md b/CHANGELOG.md index ebcdbb88..8dd2e927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,31 @@ 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. 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. 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. + +### 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/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/USAGE.md b/docs/USAGE.md index 07892b63..fd022478 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1787,6 +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, 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 7d97ae67..efda3e37 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,19 @@ 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 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 c70c1329..6bcdbc08 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 @@ -27,6 +28,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 +63,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, @@ -75,6 +81,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 @@ -86,6 +93,7 @@ BrowserSessionClosedError, ConfigurationError, ContentPolicyError, + FlowAccountChooserError, FlowApiError, # re-exported via gflow_cli.api.__init__ FlowHostMigratedError, NetworkError, @@ -783,6 +791,121 @@ 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) -> bool: + """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 "" + # 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. + # 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 + + # 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( + detail=( + f"Google sign-in/chooser displayed at {url} but no account is recorded " + f"in this profile to auto-select." + ) + ) + + # 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. + # 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(re.compile(rf"^{re.escape(email)}$", re.IGNORECASE)) + count = await row.count() + + if count == 0: + raise FlowAccountChooserError( + detail=( + f"Account chooser displayed at {url} but recorded account '{email}' " + f"was not found among selectable accounts." + ) + ) + + # The row is the account's entry; verify we actually leave the chooser. + await row.first.click() + # `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: + # 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 — it is at {landed}." + ) + ) from exc + logger.info( + "client.account_chooser_autoselected", + account=redact_sensitive_text(email), + ) + return True + async def _bootstrap_and_resolve_locale(self) -> None: """Navigate the bootstrap page and settle the account locale (#580, #587). @@ -819,6 +942,20 @@ 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. + # 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, 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 13563a49..8d6c579b 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() @@ -261,7 +262,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 +290,28 @@ def auth_login(profile: str | None, browser: str | None) -> None: try: pdir = asyncio.run(auth_mod.login(name, browser=selected_browser)) + if account: + 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", + # 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=( + 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: console.print(f"[red]{e}[/red]") if e.remediation_hint: diff --git a/src/gflow_cli/data/redaction.py b/src/gflow_cli/data/redaction.py index 6e9e4997..f9e55344 100644 --- a/src/gflow_cli/data/redaction.py +++ b/src/gflow_cli/data/redaction.py @@ -16,17 +16,39 @@ # 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. + ( + # 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"(?", ), ) # Any whitespace-delimited token carrying signed-query material — covers full @@ -85,11 +107,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/src/gflow_cli/diagnostics.py b/src/gflow_cli/diagnostics.py index 72c23261..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()): diff --git a/src/gflow_cli/errors.py b/src/gflow_cli/errors.py index 0989a5ae..44180eab 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 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. + """ + + 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 while signed in as the recorded 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 38 distinguishes account chooser stall + # from generic errors (1) without parsing stderr. + 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/src/gflow_cli/profile_store.py b/src/gflow_cli/profile_store.py index 5359ae9c..1ed5dfc6 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, @@ -350,13 +350,27 @@ 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.""" +def read_account_file(profile_path: Path) -> str | None: + """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_bootstrap_chooser.py b/tests/api/test_bootstrap_chooser.py new file mode 100644 index 00000000..73182d38 --- /dev/null +++ b/tests/api/test_bootstrap_chooser.py @@ -0,0 +1,240 @@ +"""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 playwright.async_api import TimeoutError as PlaywrightTimeoutError + +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.""" + 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?continue=flow.google.com", + row_count=1, + ) + # `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) + assert res is True + # 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, + # 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 +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, 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) + + assert "recorded@example.com" in str(exc_info.value) + assert EXIT_CODE_MAP[FlowAccountChooserError] == 38 + # 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 +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) + 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, + ) + # 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) + 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 +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) + 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() + + +@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) 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 new file mode 100644 index 00000000..948bfc1b --- /dev/null +++ b/tests/auth/test_account_autoselect.py @@ -0,0 +1,192 @@ +"""Tests for FlowAccountChooserError (exit code 38) and account auto-selection.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +import structlog + +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 "complete the account chooser" in err.remediation_hint + + +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] == 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 == 38 + + +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 38.""" + 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 == 38 + 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 + + +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 new file mode 100644 index 00000000..81f8f988 --- /dev/null +++ b/tests/e2e/test_account_chooser_e2e.py @@ -0,0 +1,297 @@ +"""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() + + +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() + + +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/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/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__ 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) diff --git a/website/docs/USAGE.md b/website/docs/USAGE.md index 19fa9c27..143fe3de 100644 --- a/website/docs/USAGE.md +++ b/website/docs/USAGE.md @@ -1787,6 +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, 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: