diff --git a/hermes_cli/browser_connect.py b/hermes_cli/browser_connect.py index 911cb4b78f..a5ef1497fa 100644 --- a/hermes_cli/browser_connect.py +++ b/hermes_cli/browser_connect.py @@ -630,26 +630,36 @@ def _copy_auth_file(src_file: str, dst_file: str) -> bool: """ os.makedirs(os.path.dirname(dst_file), exist_ok=True) if os.path.basename(src_file) in _SQLITE_AUTH_DBS: - try: - import sqlite3 - - # Read-only URI + immutable-free: we want a consistent committed - # snapshot, not to fight the writer. Short busy timeout so a truly - # wedged DB fails fast rather than hanging the launch. - source = sqlite3.connect(f"file:{src_file}?mode=ro", uri=True, timeout=5) + # On a live Chrome on macOS the profile holds + # its DBs in a state where mode=ro WITHOUT immutable=1 can hang the + # connect/backup indefinitely (the sqlite busy-timeout never fires + # because the block happens inside lock negotiation). immutable=1 + # reads instantly and is correct here: we want a committed snapshot of + # a file another process owns, not coordinated writes. A torn read + # raises → falls through to the plain-copy fallback below. + for uri in ( + f"file:{src_file}?mode=ro&immutable=1", + f"file:{src_file}?mode=ro", + ): try: - out = sqlite3.connect(dst_file) + import sqlite3 + + # Short busy timeout so a truly wedged DB fails fast rather + # than hanging the launch. + source = sqlite3.connect(uri, uri=True, timeout=5) try: - with out: - source.backup(out) + out = sqlite3.connect(dst_file) + try: + with out: + source.backup(out) + finally: + out.close() finally: - out.close() - finally: - source.close() - return True - except Exception as e: - logger.debug("real-profile: sqlite-backup of %s failed (%s); trying raw copy", - src_file, e) + source.close() + return True + except Exception as e: + logger.debug("real-profile: sqlite-backup of %s failed (%s); trying next mode", + src_file, e) # Non-DB file, or DB whose backup failed: raw copy. try: shutil.copy2(src_file, dst_file) @@ -722,6 +732,53 @@ def _profile_is_locked(src: str, source_profile: str) -> bool: return False +def _real_profile_pin() -> str | None: + """Pinned source profile dir name from ``browser.real_profile_pin``. + + Natively the snapshot follows Chrome's + ``profile.last_used`` — whichever profile the user touched last. On a + machine with a work profile (HM) and a personal profile, that roulette + can silently give the agent the wrong identity. When set (e.g. + ``"Profile 2"``), the snapshot ALWAYS copies that profile regardless of + last_used. Unset → native last_used behavior, unchanged. + """ + try: + from hermes_cli.config import read_raw_config + + cfg = read_raw_config() + browser_cfg = cfg.get("browser", {}) + if isinstance(browser_cfg, dict): + pin = browser_cfg.get("real_profile_pin") + if isinstance(pin, str) and pin.strip(): + return pin.strip() + except Exception as e: + logger.debug("could not read real_profile_pin: %s", e) + return None + + +def _resolve_source_profile(src: str) -> tuple[str | None, str | None]: + """Resolve which source profile to copy: pin first, else last_used. + + Returns ``(profile_dir_name, error)``. A configured pin that does not + exist under ``src`` FAILS CLOSED with a fixable message — falling back + to last_used would silently browse as the wrong identity, which is the + exact wrong-principal bug this pin exists to prevent. + """ + pin = _real_profile_pin() + if pin: + if os.path.isdir(os.path.join(src, pin)): + return pin, None + return None, ( + f"browser.real_profile_pin is set to '{pin}' but that profile " + f"directory does not exist under {src!r}. Fix the pin (run: " + "`python3 -c \"import json; " + "print(json.load(open(input()))['profile']['info_cache'])\" " + "against '/Local State' to list profiles) or " + "remove it to fall back to last-used." + ) + return _last_used_profile(src), None + + def _real_profile_autoclose() -> bool: """Whether browser.real_profile_autoclose consent is on (config read). @@ -822,7 +879,9 @@ def close_browser_holding_profile(src: str, timeout: float = 15.0) -> tuple[bool psutil.wait_procs(alive, timeout=3.0) # The lock releases slightly after the process exits on Windows; poll. - source_profile = _last_used_profile(src) + source_profile, _resolve_err = _resolve_source_profile(src) + if not source_profile: + source_profile = _last_used_profile(src) deadline = time.monotonic() + timeout while time.monotonic() < deadline: if not _profile_is_locked(src, source_profile): @@ -859,8 +918,10 @@ def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | N f"profile directory for '{browser}' was not found ({src!r}). " "Launch that browser at least once, or turn browser.use_real_profile off." ) + source_profile, resolve_err = _resolve_source_profile(src) + if resolve_err or not source_profile: + return None, resolve_err dst = real_profile_copy_dir(browser) - source_profile = _last_used_profile(src) # Fast lock probe BEFORE any copy: a running browser holds the cookie DB # deny-all (Windows), and a blocking file op on it can hang the launch for # minutes. On POSIX this never trips (no mandatory locking) so @@ -910,12 +971,38 @@ def snapshot_real_profile(browser: str, src: str | None = None) -> tuple[str | N # Base user-data-dir file the browser reads at startup. Cheap; always # re-synced so last_used etc. stay current. ls_src = os.path.join(src, "Local State") + ls_dst = os.path.join(dst, "Local State") if os.path.isfile(ls_src): try: - shutil.copy2(ls_src, os.path.join(dst, "Local State")) + shutil.copy2(ls_src, ls_dst) except OSError as e: logger.debug("real-profile snapshot: skipped Local State: %s", e) + # The copy contains ONLY the mirrored Default + # dir (that is where the pinned/active profile's auth was mirrored + # into), but a verbatim Local State still names the SOURCE profile + # (e.g. last_used="Profile 2", info_cache listing Profile 2/4/7). + # Chrome therefore opens a missing profile dir and starts SIGNED OUT + # (verified live: 3 anonymous cookies instead of the ~4000 copied). + # Rewrite Local State so the copy's only profile is Default and it is + # the last-used one. + try: + import json as _json + + with open(ls_dst, encoding="utf-8") as fh: + state = _json.load(fh) + prof = state.get("profile") + if isinstance(prof, dict): + cache = prof.get("info_cache") + if isinstance(cache, dict) and "Default" in cache: + prof["info_cache"] = {"Default": cache["Default"]} + prof["last_used"] = "Default" + prof["last_active_profiles"] = ["Default"] + with open(ls_dst, "w", encoding="utf-8") as fh: + _json.dump(state, fh) + except (OSError, ValueError) as e: + logger.debug("real-profile snapshot: could not normalize Local State: %s", e) + if not populated: # Fresh (or torn-and-rebuilding): drop any partial Default and copy # the ACTIVE profile's full dir (minus caches AND the locked auth diff --git a/tests/tools/test_browser_real_profile.py b/tests/tools/test_browser_real_profile.py index 1635e60c47..68ff002fa7 100644 --- a/tests/tools/test_browser_real_profile.py +++ b/tests/tools/test_browser_real_profile.py @@ -176,10 +176,21 @@ class TestRealProfileCdpLaunch: def test_launch_returns_http_cdp(self, tmp_path): import tools.browser_tool as bt self._reset() - proc = Mock(returncode=0, stdout="", stderr="") + proc = Mock(return_value=None, returncode=0, stdout="", stderr="") + + class FakeChrome: + def poll(self): + return None + + def fake_popen(argv, **kw): + (tmp_path / "DevToolsActivePort").write_text("41000\n/devtools/browser/x\n") + return FakeChrome() + with patch.object(bt, "_use_real_profile", return_value=True), \ patch("hermes_cli.browser_connect.detect_default_chromium", return_value="chrome"), \ patch("hermes_cli.browser_connect.snapshot_real_profile", return_value=(str(tmp_path), None)), \ + patch("hermes_cli.browser_connect.chromium_executable", return_value="/usr/bin/chrome"), \ + patch.object(bt.subprocess, "Popen", side_effect=fake_popen), \ patch.object(bt, "_agent_browser_get_cdp", side_effect=[None, "http://127.0.0.1:41000"]), \ patch.object(bt, "_find_agent_browser", return_value="/usr/bin/agent-browser"), \ @@ -191,19 +202,36 @@ class TestRealProfileCdpLaunch: self._reset() def test_launch_never_passes_headless(self, tmp_path): - """--headless would use a separate cookie store → 0 real cookies.""" + """--headless would use a separate cookie store → 0 real cookies. + + We launch the REAL Chrome binary + ourselves (no mock-keychain switches — those break macOS cookie + decryption) and agent-browser attaches via --cdp. So the agent-browser + argv must contain --cdp (attach, not launch) and must NOT contain + --headless or --profile (launch-mode switches). + """ import tools.browser_tool as bt self._reset() - proc = Mock(returncode=0, stdout="", stderr="") + proc = Mock(return_value=None, returncode=0, stdout="", stderr="") captured = {} def fake_run(argv, **kw): captured["argv"] = argv return proc + class FakeChrome: + def poll(self): + return None + + def fake_popen(argv, **kw): + (tmp_path / "DevToolsActivePort").write_text("41000\n/devtools/browser/x\n") + return FakeChrome() + with patch.object(bt, "_use_real_profile", return_value=True), \ patch("hermes_cli.browser_connect.detect_default_chromium", return_value="chrome"), \ patch("hermes_cli.browser_connect.snapshot_real_profile", return_value=(str(tmp_path), None)), \ + patch("hermes_cli.browser_connect.chromium_executable", return_value="/usr/bin/chrome"), \ + patch.object(bt.subprocess, "Popen", side_effect=fake_popen), \ patch.object(bt, "_agent_browser_get_cdp", side_effect=[None, "http://127.0.0.1:41000"]), \ patch.object(bt, "_find_agent_browser", return_value="/usr/bin/agent-browser"), \ @@ -211,19 +239,30 @@ class TestRealProfileCdpLaunch: patch.object(bt, "_is_headed_mode", return_value=False): bt._real_profile_cdp() assert "--headless" not in captured["argv"] - assert "--profile" in captured["argv"] - assert str(tmp_path) in captured["argv"] + assert "--profile" not in captured["argv"] + assert "--cdp" in captured["argv"] self._reset() def test_reuses_only_session_on_our_copy_dir(self, tmp_path): """A live session on a DIFFERENT dir (stale/throwaway) is closed, not reused.""" import tools.browser_tool as bt self._reset() - proc = Mock(returncode=0, stdout="", stderr="") + proc = Mock(return_value=None, returncode=0, stdout="", stderr="") closed = {"n": 0} + + class FakeChrome: + def poll(self): + return None + + def fake_popen(argv, **kw): + (tmp_path / "DevToolsActivePort").write_text("41000\n/devtools/browser/x\n") + return FakeChrome() + with patch.object(bt, "_use_real_profile", return_value=True), \ patch("hermes_cli.browser_connect.detect_default_chromium", return_value="chrome"), \ patch("hermes_cli.browser_connect.snapshot_real_profile", return_value=(str(tmp_path), None)), \ + patch("hermes_cli.browser_connect.chromium_executable", return_value="/usr/bin/chrome"), \ + patch.object(bt.subprocess, "Popen", side_effect=fake_popen), \ patch.object(bt, "_agent_browser_get_cdp", side_effect=["http://127.0.0.1:5000", "http://127.0.0.1:41000"]), \ patch.object(bt, "_cdp_http_ready", return_value=True), \ diff --git a/tests/tools/test_browser_real_profile_pin.py b/tests/tools/test_browser_real_profile_pin.py new file mode 100644 index 0000000000..d0803b32e5 --- /dev/null +++ b/tests/tools/test_browser_real_profile_pin.py @@ -0,0 +1,94 @@ +"""Tests for the LOCAL real_profile_pin patch (browser.real_profile_pin). + +Native behavior: snapshot copies whichever Chromium profile was last used +(Local State -> profile.last_used). The pin lets a machine with a work +profile and a personal profile lock each Hermes install to one identity so +last-used roulette can never give the agent the wrong principal. + +Invariants under test: +- pin set + exists -> pinned profile is copied, last_used ignored +- pin set + missing -> FAIL CLOSED (error), never silently last_used +- pin unset -> native last_used behavior, byte-for-byte +""" +import json +import os + +import pytest + + +class TestRealProfilePin: + def _make_profile(self, root, last_used="Profile 2"): + """Synthetic Chromium user-data-dir with two profiles + last_used.""" + for prof in ("Default", "Profile 2", "Profile 4"): + (root / prof / "Network").mkdir(parents=True) + (root / prof / "Cookies").write_text(f"cookies-{prof}") + (root / prof / "Login Data").write_text(f"logins-{prof}") + (root / prof / "Preferences").write_text("{}") + (root / "Crashpad").mkdir() + (root / "Local State").write_text( + json.dumps({"os_crypt": {}, "profile": {"last_used": last_used}}) + ) + return root + + def test_pin_wins_over_last_used(self, tmp_path, monkeypatch): + import hermes_cli.browser_connect as bc + + src = self._make_profile(tmp_path / "real", last_used="Profile 4") + home = tmp_path / "hermes-home" + monkeypatch.setattr(bc, "get_hermes_home", lambda: home) + monkeypatch.setattr(bc, "_real_profile_pin", lambda: "Profile 2") + + dst, err = bc.snapshot_real_profile("chrome", src=str(src)) + assert err is None and dst + got = (home / "browser-profile" / "chrome" / "Default" / "Cookies").read_text() + assert got == "cookies-Profile 2", "pin must override last_used" + + def test_bad_pin_fails_closed(self, tmp_path, monkeypatch): + import hermes_cli.browser_connect as bc + + src = self._make_profile(tmp_path / "real") + monkeypatch.setattr(bc, "get_hermes_home", lambda: tmp_path / "hh") + monkeypatch.setattr(bc, "_real_profile_pin", lambda: "Profile 99") + + dst, err = bc.snapshot_real_profile("chrome", src=str(src)) + assert dst is None + assert err and "real_profile_pin" in err and "Profile 99" in err + # Nothing may have been copied when the pin failed closed + assert not (tmp_path / "hh" / "browser-profile" / "chrome" / "Default").exists() + + def test_no_pin_keeps_native_last_used(self, tmp_path, monkeypatch): + import hermes_cli.browser_connect as bc + + src = self._make_profile(tmp_path / "real", last_used="Profile 4") + home = tmp_path / "hermes-home" + monkeypatch.setattr(bc, "get_hermes_home", lambda: home) + monkeypatch.setattr(bc, "_real_profile_pin", lambda: None) + + dst, err = bc.snapshot_real_profile("chrome", src=str(src)) + assert err is None and dst + got = (home / "browser-profile" / "chrome" / "Default" / "Cookies").read_text() + assert got == "cookies-Profile 4", "no pin = native last_used" + + def test_re_sync_respects_pin_when_last_used_flips(self, tmp_path, monkeypatch): + """The wrong-principal regression: session 2 with different last_used + must NOT overlay a different profile's auth onto the pinned copy.""" + import hermes_cli.browser_connect as bc + + src = self._make_profile(tmp_path / "real", last_used="Profile 2") + home = tmp_path / "hermes-home" + monkeypatch.setattr(bc, "get_hermes_home", lambda: home) + monkeypatch.setattr(bc, "_real_profile_pin", lambda: "Profile 2") + + dst1, err1 = bc.snapshot_real_profile("chrome", src=str(src)) + assert err1 is None + + # User browses HM (Profile 4) in between; last_used flips. + (src / "Local State").write_text( + json.dumps({"os_crypt": {}, "profile": {"last_used": "Profile 4"}}) + ) + (src / "Profile 2" / "Cookies").write_text("cookies-Profile 2-v2") + + dst2, err2 = bc.snapshot_real_profile("chrome", src=str(src)) + assert err2 is None and dst2 == dst1 + got = (home / "browser-profile" / "chrome" / "Default" / "Cookies").read_text() + assert got == "cookies-Profile 2-v2", "auth re-sync must stay on the pin" diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 5adc71d17d..8d7ee6a395 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -1454,6 +1454,7 @@ def _use_real_profile() -> bool: _REAL_PROFILE_SESSION = "hermes-real-profile" _real_profile_cdp_lock = threading.Lock() _real_profile_cdp_cache: dict = {} +_real_profile_chrome_procs: list = [] # Popen handles of directly-launched real browsers def _agent_browser_argv(browser_cmd: str) -> list: @@ -1653,9 +1654,87 @@ def _real_profile_cdp() -> tuple: return None, f"browser.use_real_profile is on, but {err}" copy_dir = snap_dir - # Launch agent-browser's packaged Chromium on the profile COPY. This is - # the same launch path Hermes' built-in local browsing already uses, - # just pointed at the copied user-data-dir — no bespoke Chrome launch. + # Launch the user's REAL browser binary directly on the profile COPY. + # agent-browser 0.35's own + # launch path force-adds --use-mock-keychain/--password-store=basic + # (and --headless=new), which makes macOS Chrome treat every + # keychain-encrypted cookie as undecryptable and drop it — the copied + # profile launches signed out. Launching the real binary ourselves with + # NO mock-keychain switches keeps the OS keychain path intact, exactly + # as the snapshot design intends; agent-browser attaches to it after + # via --auto-connect (--cdp ). + from hermes_cli.browser_connect import chromium_executable + + real_binary = chromium_executable(browser) + if real_binary is None: + return None, ( + "browser.use_real_profile is on, but the real browser binary for " + f"'{browser}' could not be found. Reinstall it or turn the toggle off." + ) + import tempfile + + port_file = os.path.join(copy_dir, "DevToolsActivePort") + try: + os.unlink(port_file) # stale port from a previous launch confuses reuse probes + except OSError: + pass + chrome_argv = [ + real_binary, + f"--user-data-dir={copy_dir}", + "--remote-debugging-port=0", + "--no-first-run", + "--no-default-browser-check", + "--disable-background-networking", + "--disable-component-update", + "--disable-default-apps", + "--disable-hang-monitor", + "--disable-popup-blocking", + "--disable-prompt-on-repost", + "--disable-sync", + "--disable-features=Translate", + "--no-startup-window", + ] + try: + chrome_proc = subprocess.Popen( + chrome_argv, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + stdin=subprocess.DEVNULL, + start_new_session=True, + env=_build_browser_env(), + ) + except (subprocess.SubprocessError, OSError) as e: + return None, f"browser.use_real_profile is on, but the launch failed: {e}" + _real_profile_chrome_procs.append(chrome_proc) + + # Wait for DevToolsActivePort to appear (Chrome picks a free port). + import time as _time + + deadline = _time.monotonic() + 30.0 + port = None + while _time.monotonic() < deadline: + try: + with open(port_file, encoding="utf-8") as fh: + line = fh.readline().strip() + if line.isdigit(): + port = int(line) + break + except OSError: + pass + if chrome_proc.poll() is not None: + return None, ( + "browser.use_real_profile is on, but Chrome exited during " + "startup (another instance may hold the profile copy)." + ) + _time.sleep(0.25) + if port is None: + return None, ( + "browser.use_real_profile is on, but the real-profile browser " + "did not expose a debug port in time. Retry, or turn the toggle off." + ) + + # Tell agent-browser to ATTACH to the running Chrome instead of + # launching its own (its own launch injects mock-keychain flags). try: browser_cmd = _find_agent_browser() except FileNotFoundError as e: @@ -1666,16 +1745,9 @@ def _real_profile_cdp() -> tuple: argv = [ *_agent_browser_argv(browser_cmd), "--session", _REAL_PROFILE_SESSION, - "--profile", copy_dir, + "--cdp", str(port), + "open", "about:blank", ] - # Do NOT pass agent-browser's ``--headless``: it maps to Chrome's legacy - # headless mode, which uses a SEPARATE cookie store and loads none of the - # copied profile's cookies (verified: --headless → 0 cookies, default → - # full jar). agent-browser's default already runs windowless on a - # server (no DISPLAY) while reading the real cookie store, which is - # exactly what real-profile browsing needs. Headed mode is a superset - # (visible window) and equally fine, so no flag either way. - argv += ["open", "about:blank"] try: proc = subprocess.run( argv, capture_output=True, text=True, @@ -1698,6 +1770,19 @@ def _real_profile_cdp() -> tuple: ) cdp = _agent_browser_get_cdp(_REAL_PROFILE_SESSION) + # The daemon may answer with the endpoint of a + # browser IT spawned (throwaway temp profile) instead of the real + # Chrome we launched on the copy. The DevToolsActivePort file OUR + # Chrome wrote is the authoritative endpoint of the logged-in browser; + # if the daemon disagrees, trust ours. + try: + with open(port_file, encoding="utf-8") as fh: + our_port = fh.readline().strip() + m = re.search(r":(\d+)", cdp or "") + if m and m.group(1) != our_port: + cdp = f"http://127.0.0.1:{our_port}" + except (OSError, ValueError): + pass if not cdp: return None, ( "browser.use_real_profile is on, but the real-profile browser "