From f7b306beb8725a967ceab3bd209bacc482296999 Mon Sep 17 00:00:00 2001 From: Marcin Obel <7283716+marcinobel@users.noreply.github.com> Date: Fri, 18 Sep 2026 18:39:00 +0200 Subject: [PATCH] fix(auth): keep the browser open for interactive sign-in (#24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `revcli auth` closed the window as soon as Google Maps rendered whenever the profile already held a Google session — "already signed in, no action needed" — which made the one command whose job is credential entry useless for signing in or switching accounts. The only way through was `revcli auth logout` first. The window now stays open either way: for credential entry when signed out, and for account switching when signed in. It closes when sign-in is detected or when the user closes it. `waitForSignIn()` returns `SignInOutcome` ("signed-in" | "browser-closed") instead of `void`, so each caller can treat a closed window differently: fatal for a scrape, which cannot continue without a session, but a normal end for `auth`. `waitForBrowserClose()` covers the signed-in case, polling `pages()` as well as the context `close` event because closing the last window on macOS leaves the process alive. Also fixed while in the flow: the 5-minute timeout threw out of an async Commander action, which `program.parse()` does not await, so it surfaced as an unhandled rejection with a raw stack trace. It now reports the timeout and exits 1. Verified on the real command: the window stayed open past 12s where it previously closed after ~4s, and the CLI exited cleanly ~2s after the window was closed. 10 new tests in tests/auth.test.ts (287 -> 297) pin both end states of each waiter; AGENTS.md, README and CHANGELOG updated. --- AGENTS.md | 9 +-- CHANGELOG.md | 7 ++ README.md | 2 + src/commands/auth.ts | 45 +++++++++--- src/scraper/auth.ts | 86 +++++++++++++++++++---- tests/auth.test.ts | 160 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 285 insertions(+), 24 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0d85bf2..5128c34 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -104,7 +104,7 @@ CLI (src/index.ts) ─── commander routes to 4 commands: src/scraper/scrape-location.ts ── shared orchestrator, accepts ParsedUrl (used by both commands); delegates final payload construction to exported pure helper `assembleScrapeResult()` which reconciles `business.totalReviews` with `reviews.length` │ ├── browser.ts ── persistent Chromium context (~/.revcli/chrome-profile/), anti-detection, SIGINT cleanup -├── auth.ts ── isSignedIn(), hasLimitedView(), waitForUserAuth(), waitForSignIn() – Google auth detection; decideSignedIn()/GOOGLE_AUTH_COOKIES make the cookie check authoritative +├── auth.ts ── isSignedIn(), hasLimitedView(), waitForUserAuth(), waitForSignIn(), waitForBrowserClose() – Google auth detection; decideSignedIn()/GOOGLE_AUTH_COOKIES make the cookie check authoritative ├── navigator.ts ── orchestrates state eviction → consent → locale → limited-view check → tab → sort; verifies loaded placeId against parsed.placeId ├── consent.ts ── Google consent handling, hl=en enforcement, g_ep/entry stripping ├── business-extractor.ts ── business name/rating/totalReviews/address extraction; page.evaluate() returns raw DOM strings only, parsing happens Node-side via parseReviewCount + parseRatingText; PII-scrubbed debug logging of candidate strings @@ -138,8 +138,8 @@ URL input → `parseGoogleMapsInput()` validates → `scrapeLocation(parsed)` la ### Patterns to know - **Persistent browser profile**: `launchBrowser()` uses `chromium.launchPersistentContext()` with `~/.revcli/chrome-profile/`. Google auth cookies survive between CLI runs. The `PROFILE_DIR` constant is exported from `browser.ts`. `ensureProfileDir()` in `browser.ts` creates the directory with owner-only `0700` permissions – it is a **credential store** holding your Google session cookies, so it must never be shared, backed up, or committed. The profile's spoofed user-agent and `navigator.webdriver` masking are ToS-sensitive (see the anti-automation note in README's Legal notice); removing them would break unauthenticated scraping. -- **Authentication flow**: Google Maps shows a "limited view" (no Reviews tab) to unauthenticated EEA users. `revcli auth` opens a browser for manual sign-in. The scrape command also detects limited view inline and can prompt for auth in headed mode. -- **Cookie-authoritative sign-in detection**: `auth.ts` defines `GOOGLE_AUTH_COOKIES` (the definitive set of Google session cookies: SID/HSID/SSID/APISID/SAPISID/`__Secure-*PSID`). `decideSignedIn(cookieCheckSucceeded, hasAuthCookies, signInButtonVisible)` treats a successful cookie check as authoritative – when `cookieCheckSucceeded` is true, `hasAuthCookies` decides, and the DOM "Sign in" button is only a fallback when the cookie check failed. `hasGoogleAuthSession()` checks the cookie set directly; `waitForSignIn()` polls and throws a typed `UnrecoverableError` with kind `"AUTH_POLL"` on timeout. +- **Authentication flow**: Google Maps shows a "limited view" (no Reviews tab) to unauthenticated EEA users. `revcli auth` opens a browser for manual sign-in and never short-circuits on an existing session – the window stays open (until the user closes it) so the profile can be switched to a different account. The scrape command also detects limited view inline and can prompt for auth in headed mode. +- **Cookie-authoritative sign-in detection**: `auth.ts` defines `GOOGLE_AUTH_COOKIES` (the definitive set of Google session cookies: SID/HSID/SSID/APISID/SAPISID/`__Secure-*PSID`). `decideSignedIn(cookieCheckSucceeded, hasAuthCookies, signInButtonVisible)` treats a successful cookie check as authoritative – when `cookieCheckSucceeded` is true, `hasAuthCookies` decides, and the DOM "Sign in" button is only a fallback when the cookie check failed. `hasGoogleAuthSession()` checks the cookie set directly; `waitForSignIn()` polls and throws a typed `UnrecoverableError` with kind `"AUTH_POLL"` on timeout, returning `SignInOutcome` (`"signed-in"` | `"browser-closed"`) otherwise – `waitForUserAuth()` treats a closed window as fatal for the scrape, while `revcli auth` reports it as a normal end. `waitForBrowserClose()` resolves once the window is gone, polling `pages()` as well as the `close` event because closing the last window on macOS leaves the process (and context) alive. - **Default headed mode**: The browser shows by default so users can observe scraping. Use `--headless` to hide it. This is the opposite of most scrapers – it's intentional because auth requires a visible browser. - **URL normalization**: `appendHlParam()` in `consent.ts` always forces `hl=en` and strips `g_ep`/`entry` tracking params that trigger Google's limited view. This runs before every navigation. - **Centralized selectors**: All Google Maps CSS class selectors live in `src/scraper/selectors.ts`. Google obfuscates these names and changes them periodically. When scraping breaks, check selectors first. See [docs/selector-maintenance.md](docs/selector-maintenance.md) for the full update procedure. @@ -170,7 +170,7 @@ URL input → `parseGoogleMapsInput()` validates → `scrapeLocation(parsed)` la - ESM-only (`"type": "module"`, `.js` extensions in imports) - Node 22+ required - Strict TypeScript, zero `any` types -- Tests use vitest (287 tests across 17 files) – pure-function tests for parser, schema, URL, CSV, JSON, retry, rate-limiter, consent, unrecoverable, batch-utils, validate, scroller, storage-types, scrape-location, extractor-selectors, extractor, auth; Playwright-dependent modules are not unit tested +- Tests use vitest (297 tests across 17 files) – pure-function tests for parser, schema, URL, CSV, JSON, retry, rate-limiter, consent, unrecoverable, batch-utils, validate, scroller, storage-types, scrape-location, extractor-selectors, extractor, auth; browser-coupled code is otherwise not unit tested, except where a narrow structural type makes it fakes-only (`ClosableBrowser` for `waitForBrowserClose`) - `parseInputFile()`, `slugify()`, and `deduplicateFilename()` in batch.ts are exported for testability - `parseReview()`, `parseReviewCount()`, and `detectLanguage()` in parser.ts are exported for testability - `parseRatingText()` in business-extractor.ts is exported for testability (called Node-side after `page.evaluate()` returns the raw aria-label string) @@ -179,4 +179,5 @@ URL input → `parseGoogleMapsInput()` validates → `scrapeLocation(parsed)` la - `extractPlaceIdFromUrl()`, `placeIdsMatch()`, and `canVerifyPlaceIdFormat()` in url.ts are exported for testability (direct tests cover priority, encoding, edge cases, null handling, case-insensitivity, and ChIJ vs 0x format discrimination) - `VOLATILE_STORAGE_TYPES` in `src/scraper/storage-types.ts` is a pure constant kept separate from `browser.ts` so tests can pin the cookies-excluded invariant without pulling in Playwright - `calculateStaleDelay()` and `shouldContinueScrolling()` in scroller.ts are exported for testability +- `waitForBrowserClose()` in auth.ts takes the narrow structural `ClosableBrowser` (not `BrowserContext`) so its poll/close-event behaviour is testable against a fake, and `waitForSignIn()` returns the `SignInOutcome` union instead of `void` so each caller's closed-window policy is explicit and pinned by tests - `extractOriginalLanguageFromButtonText()` in extractor.ts is exported for testability – pure helper that parses the human-readable language name out of the translated-review toggle button text ("See original (Polish)" → "Polish", handles nested parens like "Chinese (Simplified)") diff --git a/CHANGELOG.md b/CHANGELOG.md index 2250ba4..4f3b504 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- `revcli auth` no longer shuts the browser down as soon as Google Maps renders. It used to exit + immediately whenever the profile already held a signed-in session, which made it impossible to + sign in or switch accounts. The window now stays open – for credential entry, or for account + switching when a session already exists – and closes when sign-in is detected or when you close + it. The 5-minute timeout also reports a message instead of an unhandled-rejection stack trace. + ## [0.1.4] - 2026-09-18 ### Changed diff --git a/README.md b/README.md index d73890c..3fa44cc 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,8 @@ revcli auth logout # Clear session **How it works:** revcli uses a persistent Chrome profile at `~/.revcli/chrome-profile/`. Sign in once with `revcli auth`, and all subsequent scrapes reuse that session. Google auth cookies persist between CLI runs. The profile is a **credential store** holding your Google session cookies — it is created with owner-only (`0700`) permissions. Do not share it or back it up; anyone with read access to it can act as your signed-in session. +`revcli auth` always leaves the browser window open: it closes once sign-in is detected, or when you close it yourself. If the profile is already signed in, the window stays open anyway so you can sign out and sign in with a different account — close it when you're done. + ## Output schema
diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 77361c4..e665d7c 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -11,8 +11,10 @@ import { isSignedIn, hasLimitedView, waitForSignIn, + waitForBrowserClose, } from "../scraper/auth.js"; import { handleConsent } from "../scraper/consent.js"; +import { UnrecoverableError } from "../core/errors.js"; export async function authLoginCommand(): Promise { logger.info("Opening Google Maps in Chrome..."); @@ -28,10 +30,7 @@ export async function authLoginCommand(): Promise { await handleConsent(page); await page.waitForTimeout(3000); - if (await isSignedIn(page)) { - logger.success("Already signed in to Google Maps. No action needed."); - return; - } + const alreadySignedIn = await isSignedIn(page); console.log(` Sign in to your Google account: @@ -41,13 +40,43 @@ export async function authLoginCommand(): Promise { 3. Complete any 2FA prompts if required 4. Wait until Google Maps fully loads - The browser will close automatically once sign-in is detected. + The session is saved to ${PROFILE_DIR} and reused by every later run. `); - logger.info("Waiting for sign-in... (timeout: 5 minutes)"); + // This command is the credential-entry step, so it must never short-circuit + // on an existing session: the window has to stay open for the user to sign + // in, switch accounts, or sign in again. + if (alreadySignedIn) { + logger.info( + "Already signed in. The browser stays open so you can switch accounts – close the window when you're done.", + ); + await waitForBrowserClose(context); + logger.success("Browser closed. Google session saved for later runs."); + return; + } + + logger.info( + "Waiting for sign-in... The browser closes automatically once sign-in is detected (timeout: 5 minutes).", + ); - await waitForSignIn(context, page); - logger.success("Signed in to Google Maps. Session saved."); + const outcome = await waitForSignIn(context, page); + if (outcome === "browser-closed") { + logger.warn( + "Browser closed before sign-in was detected. Run `revcli auth status` to check the saved session.", + ); + return; + } + logger.success("Signed in to Google Maps. Session saved for later runs."); + } catch (err) { + // Commander does not await async action handlers, so an escaping + // UnrecoverableError (the 5-minute timeout) would surface as an unhandled + // rejection with a raw stack trace. + if (err instanceof UnrecoverableError) { + logger.error(err.message); + process.exitCode = 1; + return; + } + throw err; } finally { await closeBrowser(context); } diff --git a/src/scraper/auth.ts b/src/scraper/auth.ts index 59fe72b..8420206 100644 --- a/src/scraper/auth.ts +++ b/src/scraper/auth.ts @@ -120,25 +120,36 @@ export interface SignInWaitOptions { pollIntervalMs?: number; } +/** How a `waitForSignIn` poll loop ended. */ +export type SignInOutcome = "signed-in" | "browser-closed"; + /** * Poll until the user has signed in, re-resolving the active page each tick * (sign-in can open a new tab/window or close the original page) and using the * authoritative `isSignedIn`/`isGoogleAuthUrl` checks. Shared by * `waitForUserAuth` and the `auth login` command so the two poll loops never - * diverge. Throws a typed `UnrecoverableError("AUTH_POLL", …)` on a closed - * context or timeout instead of leaking a raw "Target closed" error. + * diverge. + * + * Resolves `"signed-in"` once an authenticated Maps page is seen, or + * `"browser-closed"` when the user closes the window first – the caller + * decides whether that is a failure (the scrape flow, which cannot continue) + * or a normal end (the `auth` command). Throws a typed + * `UnrecoverableError("AUTH_POLL", …)` on timeout instead of leaking a raw + * "Target closed" error. */ export async function waitForSignIn( context: BrowserContext, initialPage: Page, options: SignInWaitOptions = {}, -): Promise { +): Promise { const { timeoutMs = 300000, pollIntervalMs = 2000 } = options; const startTime = Date.now(); while (Date.now() - startTime < timeoutMs) { try { const pages = context.pages(); + // The user closed the browser window instead of signing in. + if (pages.length === 0) return "browser-closed"; const activePage = pages[pages.length - 1] ?? initialPage; await activePage.waitForTimeout(pollIntervalMs); @@ -151,7 +162,7 @@ export async function waitForSignIn( url.includes("myaccount.google.com") ) { if (url.includes("google.com/maps")) { - if (await isSignedIn(activePage)) return; + if (await isSignedIn(activePage)) return "signed-in"; continue; } // Sign-in completed on another Google page – return to Maps to confirm. @@ -161,15 +172,10 @@ export async function waitForSignIn( }); await handleConsent(activePage); await activePage.waitForTimeout(3000); - if (await isSignedIn(activePage)) return; + if (await isSignedIn(activePage)) return "signed-in"; } } catch (err) { - if (isClosedContext(err)) { - throw new UnrecoverableError( - "AUTH_POLL", - `Sign-in poll aborted: ${err instanceof Error ? err.message : String(err)}`, - ); - } + if (isClosedContext(err)) return "browser-closed"; logger.debug( `Sign-in poll error (will retry): ${err instanceof Error ? err.message : String(err)}`, ); @@ -182,6 +188,54 @@ export async function waitForSignIn( ); } +/** + * Minimal structural view of a browser context. `waitForBrowserClose` only + * needs to observe whether any page is still open and to react to the + * context's `close` event; narrowing the parameter keeps the helper testable + * without a real browser. `BrowserContext` satisfies it structurally. + */ +export interface ClosableBrowser { + pages(): readonly unknown[]; + once(event: "close", listener: () => void): unknown; + off(event: "close", listener: () => void): unknown; +} + +/** + * Resolves once the user has closed the browser window. + * + * Used by `revcli auth` when the profile is already authenticated: there is no + * sign-in to detect, so the window stays open for switching accounts or + * signing in again, and the user closing it is the only end state. Polls + * `pages()` in addition to the `close` event because closing the last window + * on macOS leaves the browser process – and therefore the context – alive. + */ +export async function waitForBrowserClose( + browser: ClosableBrowser, + pollIntervalMs = 1000, +): Promise { + let closed = false; + const onClose = () => { + closed = true; + }; + browser.once("close", onClose); + + try { + while (!closed && !hasNoPages(browser)) { + await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)); + } + } finally { + browser.off("close", onClose); + } +} + +function hasNoPages(browser: ClosableBrowser): boolean { + try { + return browser.pages().length === 0; + } catch { + return true; + } +} + /** * Waits for the user to sign in to Google Maps interactively. * Opens the Google sign-in page and polls until authentication is detected. @@ -203,6 +257,14 @@ export async function waitForUserAuth(page: Page): Promise { timeout: 30000, }); - await waitForSignIn(page.context(), page); + const outcome = await waitForSignIn(page.context(), page); + // A scrape cannot continue without a session, so a closed window is fatal + // here even though `revcli auth` treats it as a normal end. + if (outcome === "browser-closed") { + throw new UnrecoverableError( + "AUTH_POLL", + "Browser closed before sign-in was detected.", + ); + } logger.success("Sign-in detected – continuing scrape."); } diff --git a/tests/auth.test.ts b/tests/auth.test.ts index e9c7c11..433e4a9 100644 --- a/tests/auth.test.ts +++ b/tests/auth.test.ts @@ -1,10 +1,16 @@ import { describe, it, expect } from "vitest"; +import type { BrowserContext, Page } from "playwright"; import { isGoogleAuthUrl, hasGoogleAuthSession, decideSignedIn, GOOGLE_AUTH_COOKIES, + waitForBrowserClose, + waitForSignIn, + waitForUserAuth, + type ClosableBrowser, } from "../src/scraper/auth.js"; +import { UnrecoverableError } from "../src/core/errors.js"; describe("isGoogleAuthUrl", () => { it("treats the sign-in identifier page as auth", () => { @@ -112,3 +118,157 @@ describe("decideSignedIn", () => { expect(decideSignedIn(false, false, true)).toBe(false); }); }); + +/** + * Stand-in for a BrowserContext covering only the surface the waiters touch. + * `ClosableBrowser` exists as a narrow structural type precisely so this needs + * no cast and no real browser. + */ +class FakeBrowser implements ClosableBrowser { + private pageCount: number; + private listeners = new Set<() => void>(); + + constructor(pageCount: number) { + this.pageCount = pageCount; + } + + pages(): readonly unknown[] { + return Array.from({ length: this.pageCount }, () => ({})); + } + + once(_event: "close", listener: () => void): unknown { + this.listeners.add(listener); + return this; + } + + off(_event: "close", listener: () => void): unknown { + this.listeners.delete(listener); + return this; + } + + /** Simulate the user closing the browser window. */ + closeWindow(): void { + this.pageCount = 0; + } + + /** Simulate the context's own `close` event firing. */ + emitClose(): void { + for (const listener of this.listeners) listener(); + } + + get listenerCount(): number { + return this.listeners.size; + } +} + +/** Fake page + context pair for driving waitForSignIn. */ +function makePage(url: string, cookieNames: string[] = []) { + const page = { + waitForTimeout: async () => {}, + url: () => url, + context: () => ({ + cookies: async () => cookieNames.map((name) => ({ name })), + }), + }; + return { + page: page as unknown as Page, + context: { pages: () => [page] } as unknown as BrowserContext, + }; +} + +describe("waitForBrowserClose", () => { + it("resolves immediately when no window is open", async () => { + await expect(waitForBrowserClose(new FakeBrowser(0), 5)).resolves.toBeUndefined(); + }); + + it("resolves once the last page is gone", async () => { + const browser = new FakeBrowser(1); + const waited = waitForBrowserClose(browser, 5); + setTimeout(() => browser.closeWindow(), 15); + await expect(waited).resolves.toBeUndefined(); + }); + + it("resolves when the context close event fires", async () => { + const browser = new FakeBrowser(1); + const waited = waitForBrowserClose(browser, 5); + setTimeout(() => browser.emitClose(), 15); + await expect(waited).resolves.toBeUndefined(); + }); + + it("keeps waiting while a page is still open", async () => { + const browser = new FakeBrowser(1); + let resolved = false; + const waited = waitForBrowserClose(browser, 5).then(() => { + resolved = true; + }); + + await new Promise((resolve) => setTimeout(resolve, 40)); + expect(resolved).toBe(false); + + browser.closeWindow(); + await waited; + expect(resolved).toBe(true); + }); + + it("treats a context whose pages() throws as closed", async () => { + const browser: ClosableBrowser = { + pages: () => { + throw new Error("context has been closed"); + }, + once: () => {}, + off: () => {}, + }; + await expect(waitForBrowserClose(browser, 5)).resolves.toBeUndefined(); + }); + + it("removes its close listener once it resolves", async () => { + const browser = new FakeBrowser(1); + const waited = waitForBrowserClose(browser, 5); + browser.closeWindow(); + await waited; + expect(browser.listenerCount).toBe(0); + }); +}); + +describe("waitForSignIn", () => { + it("returns signed-in for an authenticated Maps page", async () => { + const { page, context } = makePage( + "https://www.google.com/maps?hl=en", + ["SID"], + ); + await expect( + waitForSignIn(context, page, { timeoutMs: 1000, pollIntervalMs: 5 }), + ).resolves.toBe("signed-in"); + }); + + it("returns browser-closed when the user closed the window", async () => { + const context = { pages: () => [] } as unknown as BrowserContext; + await expect( + waitForSignIn(context, {} as unknown as Page), + ).resolves.toBe("browser-closed"); + }); + + it("throws a typed AUTH_POLL timeout when sign-in never completes", async () => { + const { page, context } = makePage("https://example.com/"); + const err = await waitForSignIn(context, page, { + timeoutMs: 30, + pollIntervalMs: 5, + }).catch((e: unknown) => e); + + expect(err).toBeInstanceOf(UnrecoverableError); + expect((err as UnrecoverableError).kind).toBe("AUTH_POLL"); + }); +}); + +describe("waitForUserAuth", () => { + it("treats a closed window as fatal for the scrape flow", async () => { + const page = { + goto: async () => {}, + context: () => ({ pages: () => [] }), + } as unknown as Page; + + const err = await waitForUserAuth(page).catch((e: unknown) => e); + expect(err).toBeInstanceOf(UnrecoverableError); + expect((err as UnrecoverableError).kind).toBe("AUTH_POLL"); + }); +});