From 978bc1c8c451bec9ccf1317b6ebad4b3fe9f4f11 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Mon, 14 Sep 2026 23:52:50 -0400 Subject: [PATCH 1/3] fix(F-869): get_instance_state told the truth about storage it never read A real-transport smoke on 2.1.5 (Windows 11, headless Chrome 152, backend pid 53836) navigated to https://www.google.com/ and asked get_instance_state. It got 28 cookies, "local_storage": {}, "session_storage": {} and "partial": false. The backend log for that same call says: 2026-09-14 23:34:56,804 INFO 53836 [c5e09043b5d9] stealth.backend: browser_manager.get_page_state: Storage access unavailable for 886a408c-4a8f-41ec-a096-8d06a1c1fee3: unhashable type: 'dict' google.com has localStorage entries, so the record was untrue; and "unhashable type: 'dict'" is a TypeError in THIS package, not the opaque-origin condition that INFO line claims. nodriver's Tab.evaluate always sends deep SerializationOptions and returns deep_serialized_value.value RAW (DeepSerializedValue.from_json keeps json["value"] verbatim), so a string primitive arrives plain but an ARRAY arrives as BiDi nodes. Measured against Chrome 152 over a real http origin: >>> await tab.evaluate("Object.keys(localStorage)") [{'type': 'string', 'value': 'alpha'}, {'type': 'string', 'value': 'beta'}] local_storage[key] = value then hashed a dict. This is the trap F-844 closed for the viewport object literal eleven lines below in the same function; F-844's own residuals named this read and deferred it for want of LOC in browser_manager.py. The read moves to embedded/page_storage.py, THE one home for it: one JSON.stringify round trip for both stores - a string primitive survives deep serialization, the same idiom, not a second one. Nothing is interpolated into the JS any more, so a key containing a quote is no longer a syntax error and page-controlled data is no longer script, and a 200-key page costs 1 CDP call instead of 402. The error policy is the other half. StorageBlockedError - Chrome throwing INSIDE the page on an opaque origin / data: URL / blocked storage - keeps the existing INFO line and empty dicts, because such a page really has no readable storage. Everything else propagates, is logged at WARNING with exc_info, and becomes get_instance_state's partial: True + detail_error: the one degradation shape, reached by raising, exactly as get_page_state's docstring already promised. The except (RuntimeError, ConnectionError) branch is deleted rather than kept: a connection failing mid-collection is a degraded record too. debug_logger.log_warning grows an optional error= forwarded as exc_info; the in-memory ring shape is unchanged, so get_debug_view stays byte-stable. browser_manager.py 1529 -> 1528 LOC; the grandfather row ratchets DOWN to the actual. tests/test_instance_state_cookies.py is updated deliberately, with the justification inline. Its fixture answered Object.keys(localStorage) with ["ls-key"] - a hand-shaped list of plain strings modelling the assumption the product got wrong, and the reason this defect stayed green through F-844's own live-driven fix. Its assertions are unchanged. Finding: audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md --- CHANGELOG.md | 25 ++ CLAUDE.md | 5 +- ...stance_state_swallows_storage_typeerror.md | 317 ++++++++++++++++++ .../embedded/browser_manager.py | 43 ++- .../embedded/debug_logger.py | 11 +- .../embedded/page_storage.py | 133 ++++++++ tests/test_instance_state_cookies.py | 22 +- tests/test_page_state_storage.py | 307 +++++++++++++++++ tools/check_file_budgets.py | 13 +- 9 files changed, 845 insertions(+), 31 deletions(-) create mode 100644 audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md create mode 100644 src/stealth_chrome_devtools_mcp/embedded/page_storage.py create mode 100644 tests/test_page_state_storage.py diff --git a/CHANGELOG.md b/CHANGELOG.md index dd81c56..8f5bbc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,30 @@ # Changelog +## Unreleased + +### Fixed — `get_instance_state` reported empty storage as if it were the truth (F-869) + +On any page that actually has localStorage or sessionStorage entries, +`get_instance_state` (and the `browser://{id}/state` and `browser://{id}/console` +resources) returned `"local_storage": {}`, `"session_storage": {}` and +`"partial": false`. Measured on 2.1.5 against `https://www.google.com/`: 28 +cookies came back, both stores came back empty, and the record declared itself +complete. `nodriver`'s `Tab.evaluate` always sends deep `SerializationOptions` and +hands back `deep_serialized_value.value` raw, so `Object.keys(localStorage)` +arrives as `[{'type': 'string', 'value': 'alpha'}, …]` — measured against Chrome +152 — and the per-key loop raised `TypeError: unhashable type: 'dict'` when it used +one of those nodes as a dict key. An `except Exception` then logged it at INFO as +"Storage access unavailable", the sentence meant for opaque origins, and let the +empty record through; INFO is not error-reported, so the failure reached neither +the caller nor error reporting. The read now lives in `embedded/page_storage.py` +and asks the page for one `JSON.stringify` of both stores — the same idiom F-844 +applied to the viewport eleven lines below — which also retires the +`localStorage.getItem('{key}')` string interpolation and 2N+2 CDP round trips. Only +a page that genuinely refuses the read still reports empty storage; anything else +propagates and `get_instance_state` answers with `partial: true` and a +`detail_error`, as its docstring always promised, with a WARNING and a traceback in +the backend log. + ## 2.1.5 ### Fixed — the backend escapes the MCP client's Job Object (F-867) diff --git a/CLAUDE.md b/CLAUDE.md index 0fbf3b6..2c6fae6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -80,7 +80,8 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec **Browser & interaction** | File | Owns | |---|---| -| `browser_manager.py` | `BrowserManager` — spawn/list/close instances; `close_instance` offloaded teardown | +| `browser_manager.py` | `BrowserManager` — spawn/list/close instances; `close_instance` offloaded teardown. `get_page_state` owns the page-state COMPOSITION and its error POLICY, not the storage read (F-869): `page_storage.StorageBlockedError` is the page refusing (INFO, empty dicts, still `partial: false`), and every other exception propagates so `get_instance_state` can answer its `partial: True` + `detail_error` record — the one degradation shape, reached by raising, exactly as this method's docstring always said | +| `page_storage.py` | **THE one home for "read a page's localStorage/sessionStorage"** (F-869) — the ONE `JSON.stringify` round trip that reads both stores (`READ_JS`, `read`), and the two outcomes it distinguishes: `StorageBlockedError` (Chrome threw *inside the page* on an opaque origin / `data:` URL / policy-blocked storage — the expected answer) and `StorageReadError` (the evaluate did not answer with the promised JSON, so empty storage would be a lie). It is `JSON.stringify` for the same reason the viewport read next to it is (F-844): `tab.evaluate` always sends deep `SerializationOptions` and returns `deep_serialized_value.value` RAW, so `Object.keys(localStorage)` arrives as `[{'type':'string','value':'k'}, …]` — measured on Chrome 152 — and the old per-key loop hashed a `dict`. One round trip also retires the `f"localStorage.getItem('{key}')"` interpolation (a quote in a key was a syntax error; `');…` was injection) and 2N+2 CDP calls. A leaf: imports no other embedded module, takes the tab as an argument | | `dom_handler.py` | DOM manipulation + element interaction | | `element_resolution.py` | selector resolution that survives CDP document-node invalidation (route ALL selector resolution through here — never `tab.select`/`find` directly) | | `proxy_forwarder.py` | authenticated egress-proxy forwarding + `_free_port` | @@ -124,7 +125,7 @@ dependency order — each imports only the ones above it, and none imports `serv | `cdp_params.py` | **THE one home for "a caller's JSON, as the type a CDP wrapper declares"** (F-861) — `typed`, which builds each `execute_cdp_command` argument into the type nodriver's generated wrapper declares for it (`from_json` for the generated classes, through `Optional[..]` and `List[..]`), read from the wrapper's own type hints so nothing is typed by hand; primitives and already-typed values pass through untouched, and a value the type cannot take raises `ToolError` naming the param and the type. Called from the ONE site `cdp_function_executor.build_cdp_call`, after F-816's name folding. A leaf: imports only `tool_errors` | | `response_handler.py` | large-response handling + file fallbacks; **the one home for "can this payload survive the transport"** — `json_safe` (serializable, F-822) and `surrogate_safe` (utf-8-encodable, F-823) | | `in_memory_storage.py` | `InMemoryStorage` — deliberately non-durable instance cross-check | -| `debug_logger.py` | in-memory debug log ring/view; `log_tool_failure` is the ring entry point for a failed tool call (ring only — the durable/Sentry-bridged log line is deliberately NOT written, F-835/F-782) | +| `debug_logger.py` | in-memory debug log ring/view; `log_tool_failure` is the ring entry point for a failed tool call (ring only — the durable/Sentry-bridged log line is deliberately NOT written, F-835/F-782). `log_warning` takes an optional `error=` (F-869) forwarded as `exc_info`, so a warning ABOUT a caught exception carries its traceback; the ring shape is unchanged either way, which is what keeps `get_debug_view`'s contract byte-stable | ### Tombstones — do NOT route a change to these (they were removed) diff --git a/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md new file mode 100644 index 0000000..96bb1be --- /dev/null +++ b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md @@ -0,0 +1,317 @@ +# F-869 (MED, live-evidenced) — `get_instance_state` reported EMPTY storage with `partial: false` while a `TypeError` from our own code was logged at INFO + +**Status:** FIXED on `fix/F869-page-state-storage-typeerror`. +**Found:** 2.1.5, by hand over the real stdio transport. +**Predicted:** by F-844's own residuals section, which named this exact read and +deferred it ("costs LOC this file does not have"). It was right about the shape +and wrong about the cost: on a page that really has storage the read does not +degrade to `{}`, it *raises*, and the raise is swallowed. + +## 1. What was observed + +Measured 2026-09-14 23:34:56 local, version 2.1.5, live backend pid 53836, +Windows 11, headless Chrome 152. A real-transport smoke did +`spawn_browser(headless=True)` → `navigate("https://www.google.com/")` +(success, title "Google") → `get_instance_state(instance_id)`. + +The tool returned a full-looking record: 28 cookies, `"local_storage": {}`, +`"session_storage": {}`, `"console_logs": []`, `"partial": false`. + +The backend log for that same call (correlation id `c5e09043b5d9`) says: + +``` +2026-09-14 23:34:56,804 INFO 53836 [c5e09043b5d9] stealth.backend: browser_manager.get_page_state: Storage access unavailable for 886a408c-4a8f-41ec-a096-8d06a1c1fee3: unhashable type: 'dict' +``` + +Three things are wrong with that pair of facts: + +* `www.google.com` has localStorage entries, so `"local_storage": {}` is untrue. +* `"partial": false` asserts the record is complete. It was not. +* `unhashable type: 'dict'` is a **Python `TypeError` in this package**, not the + "page blocks storage access" condition (`about:blank`, an opaque origin, a + `data:` URL) that the INFO message claims. INFO records are not error-reported + — `observability.py:599` initialises `LoggingIntegration(event_level=logging.ERROR)` + — so the defect was invisible from *both* ends: the caller was told everything + was fine, and nothing was ever shipped anywhere. + +## 2. Cause (a) — the deep-serialized key array + +`browser_manager.get_page_state`, 2.1.5, `browser_manager.py:1435-1447`: + +```python +local_storage = {} +session_storage = {} + +try: + local_storage_keys = await tab.evaluate("Object.keys(localStorage)") + for key in local_storage_keys: + value = await tab.evaluate(f"localStorage.getItem('{key}')") + local_storage[key] = value # <-- :1442 the TypeError +``` + +`nodriver.core.tab.Tab.evaluate` (`.venv/Lib/site-packages/nodriver/core/tab.py:812-848`) +**always** sends deep serialization options and never asks for the value itself: + +```python +ser = cdp.runtime.SerializationOptions( + serialization="deep", max_depth=10, + additional_parameters={"maxNodeDepth": 10, "includeShadowTree": "all"}, +) +... + if remote_object.deep_serialized_value: + return remote_object.deep_serialized_value.value +``` + +and `cdp.runtime.DeepSerializedValue.from_json` +(`.venv/Lib/site-packages/nodriver/cdp/runtime.py:90-97`) keeps the payload +**raw** — `value=json['value']` — so it never walks into the graph: + +```python +@classmethod +def from_json(cls, json: T_JSON_DICT) -> DeepSerializedValue: + return cls( + type_=str(json['type']), + value=json['value'] if json.get('value', None) is not None else None, + ... +``` + +A *primitive* therefore arrives plain, but an **array arrives as a list of BiDi +nodes**. Measured (not assumed) against Chrome 152 with the pinned nodriver 0.47, +over a real `http://127.0.0.1` origin with two localStorage entries set: + +``` +REPR Object.keys(localStorage): [{'type': 'string', 'value': 'alpha'}, {'type': 'string', 'value': 'beta'}] +TYPE: +REPR getItem: '1' +HASH RAISED: TypeError unhashable type: 'dict' +``` + +That is the whole mechanism. `key` is a `dict`; `local_storage[key] = value` +hashes it; `TypeError: unhashable type: 'dict'` — the exact text in the live log +line. The `getItem` half looked fine (a string primitive survives deep +serialization), which is why only half this code path was ever suspected. + +This is the **same trap F-844 closed for the viewport object literal eleven lines +below, in the same function**, whose fix comment still stands at +`browser_manager.py:1449-1451`: + +```python +# ``JSON.stringify``, not a bare object literal: nodriver always +# sends deep serialization options, so an object comes back as +# ``[[key, {type,value}], …]`` — return_by_value cannot undo it. +``` + +F-844's finding named this follow-up explicitly +(`audit/stage2/finding_F844_get_instance_state_cookie_list_attributeerror.md`, +Residuals): *"`Object.keys(localStorage)` is an array, so `evaluate` returns +Chrome's deep serialization of it, not a list of strings… Converting them to the +same `JSON.stringify` idiom is the obvious follow-up."* It also recorded why the +live run did not catch it: *"`local_storage: {}` on a page that had no storage +anyway, so the shape is unproven either way."* An empty store never enters the +loop, so the defect is invisible on exactly the pages that were tested. + +Two further properties of the old loop, both closed by the same fix: + +* **Interpolation.** `f"localStorage.getItem('{key}')"` built JS out of + page-controlled data. A key containing `'` is a syntax error; a key containing + `');…` is script injection. +* **2N+2 round trips.** A page with 200 keys cost 402 CDP calls inside + `get_instance_state`'s `browser_state_timeout_seconds` budget. + +## 3. Cause (b) — the error policy that made it invisible + +`browser_manager.py:1448-1461` (2.1.5): + +```python +except (RuntimeError, ConnectionError) as e: + debug_logger.log_warning( + "browser_manager", "get_page_state", + f"Storage access failed (connection issue) for {instance_id}: {e}", + ) +except Exception as e: + # Pages may block storage access (cross-origin, opaque origins, + # security policies) + debug_logger.log_info( + "browser_manager", "get_page_state", + f"Storage access unavailable for {instance_id}: {e}", + ) +``` + +The comment states a narrow, legitimate condition; the `except Exception` +implements an unconditional one. Any exception whatsoever — including a defect in +this package — became the sentence "storage is unavailable on this page", at INFO, +with empty dicts flowing on into a record that then declared itself complete. + +This contradicts the function's own docstring, three lines above the `try` +(`browser_manager.py:1415-1419`): + +> Raises on a collection failure — `get_instance_state` is what turns that into +> its `partial` record. + +`get_instance_state` does have that machinery and it is the project's named +sub-field degradation shape (F-746): `tool_sections/browser_management.py:313-330` +turns any exception out of `get_page_state` into +`{"partial": True, "detail_error": "Failed to collect full page state: …"}`. +The storage read was the one thing that never reached it. + +It also passes `tests/test_no_silent_excepts.py` — the handler *does* log — which +is the limit of that AST census: it checks that something was said, not that the +right thing was said at the right level. A `log_info` with no `exc_info` on a +`TypeError` is not silence, but it is not a report either. + +## 4. Blast radius + +* **Which tools share the helper.** `get_page_state` has three callers: + `get_instance_state` (`tool_sections/browser_management.py:292`) and two + resources, `browser://{id}/state` and `browser://{id}/console` + (`embedded/server.py:249`, `:299`). All three reported empty storage on every + page that has any. +* **What a caller sees.** Nothing. `partial: false`, two empty dicts. An agent + reading `local_storage: {}` on a logged-in app concludes the app keeps no local + state, which is a wrong answer delivered with full confidence — worse than an + error, because it is actionable. +* **Whether Sentry ever hears.** No. `LoggingIntegration(event_level=logging.ERROR)` + (`observability.py:599`) ships ERROR records as events and reduces WARNING/INFO + to breadcrumbs, which only travel attached to some *other* event. There was no + other event: the exception never escaped. Per the memory note *external users on + PyPI*, this is not a single-machine concern. +* **How long.** The loop predates F-844 (2.0.8) and is unchanged since; F-844 + fixed the two raising statements *around* it and left this one, documented, in + place. + +## 5. The fix + +**(a) One read, one round trip, a JSON *string*.** The read moves to a new leaf, +`embedded/page_storage.py` — THE one home for "read a page's localStorage / +sessionStorage" — which asks the page for + +```js +JSON.stringify((function(){ + function read(name){ + try{return {ok:true,entries:Object.entries(window[name])};} + catch(e){return {ok:false,reason:String((e&&e.message)||e)};} + } + return {local:read('localStorage'),session:read('sessionStorage')}; +})()) +``` + +A string primitive survives deep serialization intact — the same idiom F-844 +applied to the viewport, not a second one. Nothing is interpolated into the JS, so +the injection and the quote-in-a-key syntax error are structurally gone. +`window[name]` is read *inside* the `try` because the property access is what +throws on a blocked origin. + +It is a leaf: it imports no other embedded module and takes the tab as an +argument. It exists as its own module for two reasons — `browser_manager.py` had +**zero** headroom under its 1529-LOC grandfather row, and the deep-serialization +argument is a paragraph that belongs with the JS rather than in the middle of +`get_page_state`. + +**(b) Two outcomes, and they are told apart by the page, not by us.** + +* `page_storage.StorageBlockedError` — Chrome itself threw while the page touched + `window.localStorage` / `window.sessionStorage`. Measured message, from a + `data:` URL on Chrome 152: *"Failed to read the 'localStorage' property from + 'Window': Storage is disabled inside 'data:' URLs."* This is the condition the + old comment described, and it keeps exactly the old treatment: the existing + INFO line, empty dicts, `partial: false`. An opaque origin really has no + readable storage. +* **Everything else propagates.** `page_storage.StorageReadError` (the answer was + not the JSON this module asked for — `tab.evaluate` hands back an + `ExceptionDetails` husk rather than raising, so "it returned something" is not + evidence it worked), a dying connection, or a `TypeError` from a future defect. + It reaches `get_page_state`'s outer handler, which now logs **at WARNING with + `exc_info`** and re-raises, and `get_instance_state` turns it into its + `partial: True` + `detail_error` record. + +That is one degradation shape, the one that already existed, reached by raising — +which is what convention 2 and the function's own docstring already said. No +`{"success": False}` dict, no new field, no widened `except`, and the `except +Exception` that stays is narrower in effect than the one it replaces because the +expected condition now has its own named type in front of it. + +`debug_logger.log_warning` grows one optional keyword, `error: Exception | None`, +forwarded as `exc_info` to the durable line. The in-memory ring shape is +unchanged, so `get_debug_view`'s tool contract is byte-stable. + +The `except (RuntimeError, ConnectionError)` branch is **deleted**, not kept. It +swallowed a dying connection into the same untrue `{}` + `partial: false`; a +connection that is failing mid-collection is a degraded record by any reading. + +## 6. Tests + +`tests/test_page_state_storage.py` (new, hermetic, no browser): + +* `test_storage_comes_back_from_the_shape_chrome_really_sends` — the deep + serialized shape through `get_page_state`, asserting the **values**, not merely + that nothing raised. RED before the fix with `assert {} == {'alpha': '1', + 'beta': '2'}`, and the run emitted + `INFO stealth.backend: browser_manager.get_page_state: Storage access + unavailable for i1: unhashable type: 'dict'` — the production log line, + reproduced hermetically. +* `test_the_deep_serialized_key_array_is_never_hashed` — the mechanism isolated + against `page_storage.read`. +* `test_a_key_with_a_quote_in_it_survives` — the interpolation half. +* `test_a_page_that_blocks_storage_is_still_not_partial` — the named tolerance, + unchanged and now reached only by its own condition. +* `test_an_unexpected_storage_failure_is_reported_not_swallowed` — the policy: + `partial: True`, the exception text in `detail_error`, and a WARNING record + carrying `exc_info`. RED before the fix with `assert False is True` on + `state["partial"]`. +* `test_a_blocked_page_is_logged_at_info_and_carries_no_traceback` — the two + conditions must not collapse into one level. +* `test_an_answer_that_is_not_the_promised_json_is_an_error` — a husk is a read + failure, never "no storage". + +`DEEP_KEYS` in that module is the literal `repr` printed by the Chrome 152 probe, +per the memory notes *mocked fakes can encode the bug* and *fixtures from the same +serializer cannot fail*. + +**`tests/test_instance_state_cookies.py` (F-844's home) is updated deliberately**, +in this commit, with the justification inline. Its `PAGE_JS` answered +`Object.keys(localStorage)` with `["ls-key"]` — a hand-shaped list of plain +strings, modelling the assumption the product got wrong. That fixture is the +reason this defect was green through F-844's own live-driven fix, and it is +exactly the failure mode that module's docstring warns about. It now answers the +one-shot read with a JSON string; its assertions (`{"ls-key": "ls-value"}`) are +unchanged. + +## 7. Files changed + +| File | Δ | +|---|---| +| `embedded/page_storage.py` | **new**, 133 LOC (leaf) | +| `embedded/browser_manager.py` | +21 / −22 → **1528 LOC** | +| `embedded/debug_logger.py` | +10 / −1 (`log_warning(error=…)`) | +| `tools/check_file_budgets.py` | grandfather row **1529 → 1528** (ratchet DOWN, cap == actual) | +| `tests/test_page_state_storage.py` | **new** | +| `tests/test_instance_state_cookies.py` | +18 / −4 (SOFT golden, justified inline) | +| `CLAUDE.md`, `CHANGELOG.md` | navigation-map row + Unreleased entry | + +## 8. Residuals (deliberately out of scope) + +* **A collection failure still does not reach Sentry.** It is now in the durable + log *with a traceback* and in the caller's `detail_error`, which is the whole + finding. But `get_instance_state` catches the raise and returns `partial` (the + F-746 contract), and `LoggingIntegration`'s `event_level` is ERROR, so no event + is created. Raising the level, or capturing explicitly the way + `observability.capture_lifecycle` does for proxy transitions, is a policy + decision about the F-746 contract and belongs with whoever owns it. +* **`tests/test_no_silent_excepts.py` cannot see this class of defect.** It + asserts a handler said *something*; it cannot assert the level matched the + severity, or that a caught exception carried `exc_info`. A census of + "`except Exception` whose only log is INFO/DEBUG" would have found this one, and + probably others. Not attempted here — it is a new gate, not a fix. +* **`tab.evaluate` remains a shape hazard at every other call site.** F-844 said + so and it is still true; this finding closes the third instance of it in one + function. `DOMHandler.execute_script` (F-832) is the one seam that asks Chrome + for the value by value and is safe. A sweep of the remaining bare + `tab.evaluate` callers that expect a non-primitive is the durable fix and is + larger than this finding. +* **`console_logs: []` was not investigated.** The live record also carried an + empty `console_logs`, which `PageState` defaults to and nothing in + `get_page_state` ever populates. It may be a second untruthful field; it is a + different mechanism and has no evidence yet. +* **`viewport` is still read by a second `tab.evaluate`.** Folding it into the + same round trip as the storage read would save one CDP call, but it would move + F-844's fix for no behavioural gain. diff --git a/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py b/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py index 007afc6..8ed0b8b 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py +++ b/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py @@ -16,6 +16,7 @@ from stealth_chrome_devtools_mcp.embedded import ( desktop_launch, + page_storage, spawn_contention, spawn_exhaustion, spawn_leak, @@ -1432,32 +1433,20 @@ async def get_page_state(self, instance_id: str) -> PageState | None: # a ``{"cookies": [...]}`` envelope (F-844). PageState wants dicts. cookies = await tab.send(uc.cdp.network.get_cookies()) or [] - local_storage = {} - session_storage = {} - + local_storage: dict[str, str] = {} + session_storage: dict[str, str] = {} try: - local_storage_keys = await tab.evaluate("Object.keys(localStorage)") - for key in local_storage_keys: - value = await tab.evaluate(f"localStorage.getItem('{key}')") - local_storage[key] = value - - session_storage_keys = await tab.evaluate("Object.keys(sessionStorage)") - for key in session_storage_keys: - value = await tab.evaluate(f"sessionStorage.getItem('{key}')") - session_storage[key] = value - except (RuntimeError, ConnectionError) as e: - debug_logger.log_warning( - "browser_manager", - "get_page_state", - f"Storage access failed (connection issue) for {instance_id}: {e}", - ) - except Exception as e: - # Pages may block storage access (cross-origin, opaque origins, - # security policies) + local_storage, session_storage = await page_storage.read(tab) + except page_storage.StorageBlockedError as blocked: + # The ONLY condition this message was ever meant for: the PAGE + # itself refused (opaque origin, ``data:`` URL, storage disabled + # by policy), so empty dicts are the truth here. Anything else — + # F-869's TypeError among them — now reaches the handler below + # and becomes get_instance_state's honest partial record. debug_logger.log_info( "browser_manager", "get_page_state", - f"Storage access unavailable for {instance_id}: {e}", + f"Storage access unavailable for {instance_id}: {blocked}", ) # ``JSON.stringify``, not a bare object literal: nodriver always @@ -1482,6 +1471,16 @@ async def get_page_state(self, instance_id: str) -> PageState | None: ) except Exception as e: + # F-869: ONE place a collection failure is recorded, at WARNING with + # its traceback. It used to be INFO'd as "storage unavailable" and + # dropped, so a defect in this package was invisible to the caller + # and to error reporting alike. + debug_logger.log_warning( + "browser_manager", + "get_page_state", + f"Page state collection failed for {instance_id}: {e}", + error=e, + ) raise Exception(f"Failed to get page state: {e!s}") # noqa: B904 plan_M4ph1 async def cleanup_inactive(self, timeout_seconds: int | None = None) -> int: diff --git a/src/stealth_chrome_devtools_mcp/embedded/debug_logger.py b/src/stealth_chrome_devtools_mcp/embedded/debug_logger.py index f5894cf..f6848a1 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/debug_logger.py +++ b/src/stealth_chrome_devtools_mcp/embedded/debug_logger.py @@ -221,6 +221,7 @@ def log_warning( method: str, message: str, context: dict[str, Any] | None = None, + error: Exception | None = None, ): """ Log a warning. @@ -230,9 +231,17 @@ def log_warning( method (str): Name of the method where the warning occurred. message (str): Warning message. context (Optional[Dict[str, Any]]): Additional context for the warning. + error (Optional[Exception]): F-869 — the exception this warning is + about, when there is one. It is attached as ``exc_info`` so the + durable log line carries the TRACEBACK, which is the difference + between "something went wrong" and a locatable defect. Optional + because most warnings are conditions, not caught exceptions; the + in-memory ring shape is unchanged either way. """ with self._lock: - _backend_logger.warning("%s.%s: %s", component, method, message) + _backend_logger.warning( + "%s.%s: %s", component, method, message, exc_info=error + ) warning_entry = { "timestamp": datetime.now(tz=timezone.utc).isoformat(), diff --git a/src/stealth_chrome_devtools_mcp/embedded/page_storage.py b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py new file mode 100644 index 0000000..60aae00 --- /dev/null +++ b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py @@ -0,0 +1,133 @@ +"""THE one home for "read a page's localStorage/sessionStorage" (F-869). + +The read used to live inline in ``browser_manager.get_page_state`` as four +``tab.evaluate`` calls:: + + local_storage_keys = await tab.evaluate("Object.keys(localStorage)") + for key in local_storage_keys: + value = await tab.evaluate(f"localStorage.getItem('{key}')") + local_storage[key] = value + +``tab.evaluate`` always sends ``SerializationOptions(serialization="deep")`` and +hands back ``remote_object.deep_serialized_value.value`` **raw** — nodriver's +``DeepSerializedValue.from_json`` keeps ``json["value"]`` verbatim and never +walks into it. A primitive therefore arrives plain, but an *array* arrives as a +list of BiDi nodes. Measured against Chrome 152 over a real http origin:: + + >>> await tab.evaluate("Object.keys(localStorage)") + [{'type': 'string', 'value': 'alpha'}, {'type': 'string', 'value': 'beta'}] + +so ``local_storage[key] = value`` hashed a ``dict`` and raised +``TypeError: unhashable type: 'dict'`` on every page that has any storage at +all. This is the same deep-serialization trap F-844 hit with the viewport +object literal in the same function, and it is closed the same way: ask the page +for ``JSON.stringify(...)``, whose answer is a *string* primitive and therefore +survives deep serialization intact. + +Two further properties of that one round trip, both of which the per-key loop +lacked: + +* **No interpolation.** The old loop built ``localStorage.getItem('{key}')`` by + f-string, so a key containing ``'`` produced a syntax error and a key + containing ``');...`` was script injection from page-controlled data. +* **One evaluate, not 2N+2.** A page with 200 keys cost 402 CDP round trips + inside ``get_instance_state``'s ``browser_state_timeout_seconds`` budget. + +**The one degradation shape.** A page may legitimately refuse the read — an +opaque origin, a ``data:`` URL, third-party storage blocked by policy. Chrome +answers that with a ``SecurityError`` *thrown by the page*, which is not a +defect in this tool. That, and only that, is :class:`StorageBlockedError`; the caller +logs it and reports empty storage. Anything else — a bug here, a dying +connection, an answer that is not the JSON this module asked for — propagates, +and ``get_instance_state`` turns it into its ``partial: True`` + +``detail_error`` record (the named sub-field degradation, DESIGN §9). There is +no third outcome and no ``{"success": False}`` dict. + +A leaf: it imports no other embedded module and takes the tab as an argument. +""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + +if TYPE_CHECKING: # pragma: no cover - typing only, keeps this module a leaf + from nodriver import Tab + +#: Both stores in ONE round trip, as a JSON *string* (see the module docstring +#: for why a string and not an object). ``window[name]`` is read INSIDE the +#: ``try`` because it is the property *access* that throws on a blocked origin, +#: not the later ``Object.entries``. ``Object.entries`` on a ``Storage`` yields +#: its own enumerable string keys and their string values — the same set +#: ``Object.keys`` yielded, paired with the values the old loop fetched one by +#: one. +READ_JS = ( + "JSON.stringify((function(){" + "function read(name){" + "try{return {ok:true,entries:Object.entries(window[name])};}" + "catch(e){return {ok:false,reason:String((e&&e.message)||e)};}" + "}" + "return {local:read('localStorage'),session:read('sessionStorage')};" + "})())" +) + + +#: ``Object.entries`` yields ``[key, value]`` — two elements, always. +_PAIR = 2 + + +class StorageBlockedError(Exception): + """The PAGE refused the read — an expected answer, not a failure. + + Raised only when Chrome itself threw while the page touched + ``window.localStorage`` / ``window.sessionStorage``: an opaque origin, a + ``data:`` URL, storage disabled by policy. Measured message from Chrome 152 + on a ``data:`` URL:: + + Failed to read the 'localStorage' property from 'Window': + Storage is disabled inside 'data:' URLs. + """ + + +class StorageReadError(Exception): + """The evaluate did not answer with the JSON this module asked for. + + Deliberately distinct from :class:`StorageBlockedError`: this one means the read + itself went wrong (a JS throw outside the page's own ``try``, a CDP + ``ExceptionDetails`` husk where a string was promised), so the caller must + NOT report empty storage as if it were the truth. + """ + + +def _entries(record: object, store: str) -> dict[str, str]: + """One store's ``{ok, entries}`` record as a plain ``{key: value}`` dict.""" + if not isinstance(record, dict): + raise StorageReadError(f"{store}: unexpected record {record!r}") + if not record.get("ok"): + raise StorageBlockedError(f"{store}: {record.get('reason')}") + rows = record.get("entries") + if not isinstance(rows, list): + raise StorageReadError(f"{store}: unexpected entries {rows!r}") + entries: dict[str, str] = {} + for row in rows: + # Every row is checked rather than unpacked: a malformed answer must be + # a named StorageReadError, not a ValueError out of tuple unpacking. + if not isinstance(row, list) or len(row) != _PAIR: + raise StorageReadError(f"{store}: unexpected entry {row!r}") + entries[str(row[0])] = str(row[1]) + return entries + + +async def read(tab: Tab) -> tuple[dict[str, str], dict[str, str]]: + """``(local_storage, session_storage)`` for the page ``tab`` is showing. + + Raises :class:`StorageBlockedError` when the page itself refused the read, and + :class:`StorageReadError` when the answer was not the promised JSON. + """ + answer = await tab.evaluate(READ_JS) + if not isinstance(answer, str): + raise StorageReadError(f"evaluate answered {type(answer).__name__}: {answer!r}") + payload = json.loads(answer) + return _entries(payload.get("local"), "localStorage"), _entries( + payload.get("session"), "sessionStorage" + ) diff --git a/tests/test_instance_state_cookies.py b/tests/test_instance_state_cookies.py index 2fede91..1408674 100644 --- a/tests/test_instance_state_cookies.py +++ b/tests/test_instance_state_cookies.py @@ -71,10 +71,24 @@ "window.location.href": PAGE_URL, "document.title": "fixture-index-page", "document.readyState": "complete", - "Object.keys(localStorage)": ["ls-key"], - "localStorage.getItem": "ls-value", - "Object.keys(sessionStorage)": ["ss-key"], - "sessionStorage.getItem": "ss-value", + # F-869 (2026-09-14) UPDATES THIS FIXTURE, deliberately. It used to answer + # ``Object.keys(localStorage)`` with ``["ls-key"]`` — a list of plain + # strings — and ``localStorage.getItem`` with a value, modelling the per-key + # loop the product then ran. That answer was WRONG about nodriver in exactly + # the way this module's own docstring warns about two paragraphs up: deep + # serialization makes ``Object.keys`` come back as + # ``[{'type': 'string', 'value': 'ls-key'}, …]`` (measured, Chrome 152), so + # the live product raised ``TypeError: unhashable type: 'dict'`` here while + # this fixture stayed green. Storage is now ONE ``JSON.stringify`` read — + # the same trick F-844 applied to the viewport — so the fixture answers it + # the same way: a JSON *string*. Keyed by ``read('localStorage')`` and + # placed BEFORE the viewport entry because both expressions start with + # ``JSON.stringify`` and FakeTab takes the first matching substring. + # See tests/test_page_state_storage.py, which is F-869's home. + "read('localStorage')": ( + '{"local":{"ok":true,"entries":[["ls-key","ls-value"]]},' + '"session":{"ok":true,"entries":[["ss-key","ss-value"]]}}' + ), # A JSON *string*, because that is what the product now asks the page for # and what nodriver hands back for one. A dict here would model an # `evaluate` that returns plain objects — which it does not (see below). diff --git a/tests/test_page_state_storage.py b/tests/test_page_state_storage.py new file mode 100644 index 0000000..9857778 --- /dev/null +++ b/tests/test_page_state_storage.py @@ -0,0 +1,307 @@ +"""F-869 — ``get_instance_state`` reported EMPTY storage with ``partial: false`` +while a ``TypeError`` from our own code was logged at INFO and shipped nowhere. + +Live evidence (2.1.5, real stdio transport, Windows 11, headless Chrome 152, +backend pid 53836). ``spawn_browser(headless=True)`` → ``navigate( +"https://www.google.com/")`` (success, title "Google") → ``get_instance_state``. +The tool returned 28 cookies, ``"local_storage": {}``, ``"session_storage": {}`` +and ``"partial": false``. The backend log for that same call says:: + + 2026-09-14 23:34:56,804 INFO 53836 [c5e09043b5d9] stealth.backend: + browser_manager.get_page_state: Storage access unavailable for + 886a408c-4a8f-41ec-a096-8d06a1c1fee3: unhashable type: 'dict' + +``www.google.com`` has localStorage entries, so ``{}`` was untrue, and +``unhashable type: 'dict'`` is a Python bug in this package — not the +"page blocks storage" condition the INFO line claims. + +The shape, MEASURED (not assumed) against Chrome 152 over a real http origin +with the pinned nodriver 0.47:: + + >>> await tab.evaluate("Object.keys(localStorage)") + [{'type': 'string', 'value': 'alpha'}, {'type': 'string', 'value': 'beta'}] + >>> await tab.evaluate("localStorage.getItem('alpha')") + '1' + +``Tab.evaluate`` always sends ``SerializationOptions(serialization="deep")`` and +returns ``remote_object.deep_serialized_value.value`` raw; +``DeepSerializedValue.from_json`` keeps ``json["value"]`` verbatim and never +walks into it. A string primitive therefore arrives plain (which is why the +``getItem`` half looked fine) but an ARRAY arrives as a list of BiDi nodes. The +old loop then did ``local_storage[key] = value`` with a ``dict`` for ``key``. + +House rule (memory: *mocked fakes can encode the bug*, and *fixtures from the +same serializer cannot fail*): :data:`DEEP_KEYS` below is the literal ``repr`` +printed by that probe, not a hand-shaped ``["ls-key"]``. The pre-existing +``tests/test_instance_state_cookies.py`` fixture used exactly that hand-shaped +list of plain strings — which is precisely why this defect was green there +through F-844's own live-driven fix. + +The two pins: + +* :func:`test_storage_comes_back_from_the_shape_chrome_really_sends` drives the + real shape through the reader and asserts the VALUES arrive. Under the old + per-key loop this is the ``TypeError``. +* :func:`test_an_unexpected_storage_failure_is_reported_not_swallowed` asserts + the error policy: an unexpected exception makes the record say so + (``partial: True`` + ``detail_error`` carrying the exception text) and is + logged at WARNING **with a traceback**, instead of INFO + ``{}`` + + ``partial: false``. +""" + +from __future__ import annotations + +import json +import logging + +import pytest +from nodriver.cdp.network import Cookie + +from fakes import FakeBrowser, FakeTab, fake_instance +from stealth_chrome_devtools_mcp.embedded import page_storage +from stealth_chrome_devtools_mcp.embedded.browser_manager import BrowserManager + +INSTANCE_ID = "i1" +PAGE_URL = "https://fixture.test/index.html" + +#: Verbatim from the Chrome 152 probe — the answer ``Object.keys(localStorage)`` +#: really produces once nodriver's deep ``SerializationOptions`` are applied. +DEEP_KEYS = [{"type": "string", "value": "alpha"}, {"type": "string", "value": "beta"}] + +#: The one round trip the product now makes. ``ok`` false is Chrome's own +#: SecurityError, caught by the page inside the snippet. +STORAGE_OK = json.dumps( + { + "local": {"ok": True, "entries": [["alpha", "1"], ["beta", "2"]]}, + "session": {"ok": True, "entries": [["s", "9"]]}, + } +) +STORAGE_BLOCKED = json.dumps( + { + "local": { + "ok": False, + "reason": ( + "Failed to read the 'localStorage' property from 'Window': " + "Storage is disabled inside 'data:' URLs." + ), + }, + "session": {"ok": False, "reason": "Storage is disabled inside 'data:' URLs."}, + } +) + +CHROME_COOKIE_JSON = { + "name": "sid", + "value": "abc123", + "domain": "fixture.test", + "path": "/", + "expires": -1, + "size": 9, + "httpOnly": True, + "secure": False, + "session": True, + "sameSite": "Lax", + "priority": "Medium", + "sameParty": False, + "sourceScheme": "NonSecure", + "sourcePort": 80, +} + +VIEWPORT_JS = '{"width":1280,"height":720,"devicePixelRatio":1}' + + +def _page_js(storage_answer: str) -> dict[str, str]: + """The JS answers ``get_page_state`` reads, keyed by a substring of each + expression. Both the OLD per-key expressions and the NEW one-shot read are + answered, so this fixture is honest against either implementation — the pin + fails on the product's behaviour, never on the harness not knowing the JS. + """ + return { + "window.location.href": PAGE_URL, + "document.title": "fixture-index-page", + "document.readyState": "complete", + # OLD path: the deep-serialized array, exactly as Chrome answers it. + "Object.keys(localStorage)": DEEP_KEYS, + "localStorage.getItem": "1", + "Object.keys(sessionStorage)": DEEP_KEYS, + "sessionStorage.getItem": "9", + # NEW path (checked BEFORE the viewport read, which also starts with + # "JSON.stringify" — dict order is insertion order, so this entry wins). + "read('localStorage')": storage_answer, + "JSON.stringify": VIEWPORT_JS, + } + + +def _manager(storage_answer: str) -> tuple[BrowserManager, FakeTab]: + tab = FakeTab( + url=PAGE_URL, + evaluate_map=_page_js(storage_answer), + cdp_responses={"get_cookies": [Cookie.from_json(CHROME_COOKIE_JSON)]}, + ) + manager = BrowserManager() + manager._instances[INSTANCE_ID] = { + "browser": FakeBrowser(tabs=[tab]), + "tab": tab, + "instance": fake_instance(INSTANCE_ID), + "navigation_count": 0, + } + return manager, tab + + +# --------------------------------------------------------------------------- +# (a) the TypeError — the shape Chrome really sends must read correctly +# --------------------------------------------------------------------------- + + +async def test_storage_comes_back_from_the_shape_chrome_really_sends(): + """THE pin. Asserting the VALUES, not merely "no exception": a fix that + swallowed the TypeError and kept answering ``{}`` would pass a no-raise pin + while still being the defect this finding is about. + """ + manager, _tab = _manager(STORAGE_OK) + + state = await manager.get_page_state(INSTANCE_ID) + + assert state is not None + assert state.local_storage == {"alpha": "1", "beta": "2"} + assert state.session_storage == {"s": "9"} + + +async def test_the_deep_serialized_key_array_is_never_hashed(): + """The mechanism, isolated: the reader must not use a BiDi node as a key. + + Kept as its own pin because it names the trap rather than the symptom — the + same trap F-844 closed for the viewport object in the same function. + """ + tab = FakeTab(evaluate_map={"read('localStorage')": STORAGE_OK}) + + local, session = await page_storage.read(tab) + + assert local == {"alpha": "1", "beta": "2"} + assert session == {"s": "9"} + assert all(isinstance(k, str) for k in local) + + +async def test_a_key_with_a_quote_in_it_survives(): + """The old loop interpolated the key into ``localStorage.getItem('{key}')``. + + A key containing ``'`` produced a JS syntax error; a key containing + ``');...`` was script injection from page-controlled data. The one-shot read + interpolates nothing, so this is structurally impossible now — pinned so it + stays that way. + """ + key = "it's\n');alert(1)//" + hostile = json.dumps( + { + "local": {"ok": True, "entries": [[key, "kept"]]}, + "session": {"ok": True, "entries": []}, + } + ) + tab = FakeTab(evaluate_map={"read('localStorage')": hostile}) + + local, _session = await page_storage.read(tab) + + assert local == {key: "kept"} + + +async def test_a_page_that_blocks_storage_is_still_not_partial( + call_tool, patched_server +): + """The NAMED tolerance, unchanged and now reached only by its own condition. + + An opaque origin / ``data:`` URL really has no readable storage, so empty + dicts are the truth there and the call is not degraded. This is the branch + the INFO line was always meant for. + """ + manager, _tab = _manager(STORAGE_BLOCKED) + srv = patched_server(browser_manager=manager) + + state = await call_tool(srv, "get_instance_state", instance_id=INSTANCE_ID) + + assert state["local_storage"] == {} + assert state["session_storage"] == {} + assert state["partial"] is False + + +# --------------------------------------------------------------------------- +# (b) the error policy — an unexpected failure must be visible +# --------------------------------------------------------------------------- + + +async def test_an_unexpected_storage_failure_is_reported_not_swallowed( + call_tool, patched_server, caplog +): + """The second half of the finding, and the one that makes the record honest. + + On 2.1.5 this call returned ``local_storage: {}`` with ``partial: false`` + and one INFO line. INFO is not error-reported, so the ``TypeError`` reached + neither the caller nor Sentry: the defect was invisible from both ends. + """ + manager, tab = _manager(STORAGE_OK) + + async def boom(_expression, *args, **kwargs): + if "read('localStorage')" in _expression: + raise TypeError("unhashable type: 'dict'") + return tab._answer_for_js(_expression) + + tab.evaluate = boom + srv = patched_server(browser_manager=manager) + + with caplog.at_level(logging.WARNING, logger="stealth.backend"): + state = await call_tool(srv, "get_instance_state", instance_id=INSTANCE_ID) + + assert state["partial"] is True + assert "unhashable type: 'dict'" in state["detail_error"] + + warnings = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert warnings, "an unexpected failure must not be logged at INFO" + assert any("unhashable type: 'dict'" in r.getMessage() for r in warnings) + assert any(r.exc_info for r in warnings), ( + "a defect in our own code must carry its traceback" + ) + + +async def test_a_blocked_page_is_logged_at_info_and_carries_no_traceback(caplog): + """The two conditions must not collapse into one log level. + + If the blocked branch were also WARNING+exc_info, every opaque-origin page + would read as a defect and the level would stop meaning anything. + """ + manager, _tab = _manager(STORAGE_BLOCKED) + + with caplog.at_level(logging.INFO, logger="stealth.backend"): + await manager.get_page_state(INSTANCE_ID) + + blocked = [ + r for r in caplog.records if "Storage access unavailable" in r.getMessage() + ] + assert blocked, "the page-refused branch still says so" + assert all(r.levelno == logging.INFO for r in blocked) + assert all(r.exc_info is None for r in blocked) + + +@pytest.mark.parametrize( + "answer", + [ + pytest.param(object(), id="a-husk-instead-of-a-string"), + pytest.param('{"local": null, "session": null}', id="a-record-that-is-not-one"), + pytest.param( + '{"local":{"ok":true,"entries":[["only-a-key"]]},' + '"session":{"ok":true,"entries":[]}}', + id="an-entry-that-is-not-a-pair", + ), + pytest.param( + '{"local":{"ok":true,"entries":"nope"},"session":{"ok":true,"entries":[]}}', + id="entries-that-are-not-a-list", + ), + ], +) +async def test_an_answer_that_is_not_the_promised_json_is_an_error(answer): + """Not-the-JSON-we-asked-for is a read failure, never "no storage". + + ``tab.evaluate`` answers a JS throw with an ``ExceptionDetails`` husk rather + than raising, so "it returned something" is not evidence that it worked. + """ + tab = FakeTab(evaluate_map={"read('localStorage')": answer}) + + with pytest.raises(page_storage.StorageReadError): + await page_storage.read(tab) diff --git a/tools/check_file_budgets.py b/tools/check_file_budgets.py index e7059a8..cd29ce0 100644 --- a/tools/check_file_budgets.py +++ b/tools/check_file_budgets.py @@ -56,9 +56,18 @@ # Args:/Returns: blocks that only restated their own signatures # (_resolve_idle_timeout_seconds, touch_instance, spawn_browser, # get_instance) — the plan_F856 payment mechanism. Cap == actual. + # F-869 RATCHETS DOWN 1529 -> 1528. Reading a page's localStorage/ + # sessionStorage moved to the new page_storage.py leaf (the deep- + # serialization trap and the one-shot JSON.stringify read that closes it are + # a paragraph of justification that belongs WITH the JS, not in the middle + # of get_page_state). What this file keeps is the policy: which failure is + # the page refusing (INFO, empty storage) and which is a defect here + # (WARNING + traceback, propagate to get_instance_state's partial record). + # The row had zero headroom, so the extraction was the only way in; a + # one-line ratchet is small but it is the honest actual. Cap == actual. "embedded/browser_manager.py": ( - 1529, - "DEBT(F-702) + plan_M10a + plan_M7 + plan_M4ph1 + F-860", + 1528, + "DEBT(F-702) + plan_M10a + plan_M7 + plan_M4ph1 + F-860 + F-869", ), # plan_F808 Task 10 (F-808 fratricide), in two ratchets against one file: # 1054 -> 966 (step 10a) when the browser_pids.json schema, its lock and its From c4873754580132eb38370b477a7e7fb66aa8fc42 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Tue, 15 Sep 2026 00:11:50 -0400 Subject: [PATCH 2/3] fix(F-869): report shape, never the stored value, when a storage read is wrong Review of 978bc1c caught a leak the FIX introduced and the defect never had. page_storage's first draft diagnosed a malformed answer with f"{store}: unexpected entries {rows!r}" - and rows IS the page's localStorage, where a logged-in app keeps its session token. That message travels three ways at once: into the durable backend log, into get_instance_state's detail_error (i.e. to the MCP client), and into a Sentry breadcrumb, since LoggingIntegration(event_level=ERROR) reduces WARNINGs to breadcrumbs that ride out attached to a later event and _scrub_event strips emails and URL query strings, not a bare bearer token. Verified RED by reinstating the {rows!r} message: the JWT appeared in detail_error and in the WARNING line. Every message in page_storage now reports SHAPE and COUNT only - a type name, an entry index, a field count, a character count. The rule is stated in the module docstring, because it is a property of fixes of this shape rather than of this one line. The single page-supplied string still repeated is Chrome's own SecurityError text on StorageBlockedError, which describes the property ACCESS and is produced before anything is read. Three parametrized pins over seven malformed answers, each embedding a JWT-shaped SECRET, assert it reaches neither the raised message, nor detail_error, nor any FORMATTED log record - formatted rather than getMessage(), because the WARNING carries exc_info and the rendered traceback is what a log file and a breadcrumb actually hold. Each pin first asserts the fixture really carries the secret, so it cannot quietly become vacuous. Also, from the same review: - read() now wraps a non-JSON string (JSONDecodeError) and JSON that is not an object (AttributeError on payload.get) in StorageReadError. Both propagated correctly, but a module whose docstring says "there is no third outcome" and then has four is a claim its code does not keep. - browser_manager's WARNING comment no longer implies error reporting was fixed too; it points at the finding's SS8, which owns that residual. - Both fixtures key the viewport answer on innerWidth, not JSON.stringify. The storage read and the viewport read now both BEGIN with JSON.stringify and FakeTab returns the first substring that matches, so a shared key made dict insertion order decide which JSON the storage read received. No named home for the JSON.stringify idiom is created here: F-872 is measuring the fourth site (the cloner's nested BiDi nodes) and owns that decision, and a home chosen from three examples would pre-empt it. Noted in the finding's SS8. browser_manager.py stays at 1528 LOC; the grandfather row is unchanged and still cap == actual. --- ...stance_state_swallows_storage_typeerror.md | 62 +++++++- .../embedded/browser_manager.py | 4 +- .../embedded/page_storage.py | 55 ++++++- tests/test_instance_state_cookies.py | 11 +- tests/test_page_state_storage.py | 137 +++++++++++++++--- 5 files changed, 230 insertions(+), 39 deletions(-) diff --git a/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md index 96bb1be..8ef31f7 100644 --- a/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md +++ b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md @@ -224,6 +224,12 @@ argument is a paragraph that belongs with the JS rather than in the middle of `exc_info`** and re-raises, and `get_instance_state` turns it into its `partial: True` + `detail_error` record. +`StorageReadError` covers *every* other way the answer can be wrong, including the +two that would otherwise escape as their own types: a non-JSON string +(`JSONDecodeError`) and JSON that is not an object (`AttributeError` on +`payload.get`). Both propagated correctly, but a module that says "there is no +third outcome" and then has four is a claim its code does not keep. + That is one degradation shape, the one that already existed, reached by raising — which is what convention 2 and the function's own docstring already said. No `{"success": False}` dict, no new field, no widened `except`, and the `except @@ -238,6 +244,37 @@ The `except (RuntimeError, ConnectionError)` branch is **deleted**, not kept. It swallowed a dying connection into the same untrue `{}` + `partial: false`; a connection that is failing mid-collection is a degraded record by any reading. +**(c) The leak this fix must not introduce.** Caught in review of the first +commit, and worth its own section because it is a property of *fixes of this +shape*, not of this bug. + +Making a silent failure visible means writing a message, and the first draft of +`page_storage` wrote `f"{store}: unexpected entries {rows!r}"` and +`f"{store}: unexpected entry {row!r}"`. `rows` **is the page's localStorage** — +where a logged-in app keeps its session token. That message travels three ways at +once: + +* into the durable backend log (`~/.stealth-mcp/logs/`, retained by + `logging_setup.prune_old_logs`), +* into `get_instance_state`'s `detail_error`, i.e. into the MCP client's hands, +* into a **Sentry breadcrumb** — `LoggingIntegration(event_level=logging.ERROR)` + (`observability.py:599`) turns WARNING records into breadcrumbs that ride out + attached to any later event, and `observability._scrub_event` strips emails and + URL query strings, not a bare bearer token. Per the memory note *external users + on PyPI*, that is other people's machines. + +The defect being fixed never logged storage contents — it crashed before it could. +A fix that made the failure visible by quoting the data would have been strictly +worse than the bug. So `page_storage` states the rule in its module docstring and +every message reports **shape and count only**: a type name, an index, a field +count, a character count. The single page-supplied string it repeats is Chrome's +own `SecurityError` text on `StorageBlockedError`, which describes the property +*access* and is produced before anything is read. Three parametrized pins embed a +JWT-shaped `SECRET` in every malformed answer and assert it reaches neither the +raised message, nor `detail_error`, nor any **formatted** log record (formatted, +not `getMessage()`, because the WARNING carries `exc_info` and the rendered +traceback is what a log file and a breadcrumb actually hold). + ## 6. Tests `tests/test_page_state_storage.py` (new, hermetic, no browser): @@ -260,8 +297,14 @@ connection that is failing mid-collection is a degraded record by any reading. `state["partial"]`. * `test_a_blocked_page_is_logged_at_info_and_carries_no_traceback` — the two conditions must not collapse into one level. -* `test_an_answer_that_is_not_the_promised_json_is_an_error` — a husk is a read - failure, never "no storage". +* `test_an_answer_that_is_not_the_promised_json_is_an_error` — a husk, a non-JSON + string, a JSON scalar, a record that is not an object and three malformed entry + shapes are all read failures, never "no storage". +* `test_a_malformed_answer_never_quotes_the_storage_it_was_reading` and + `test_the_storage_value_reaches_neither_the_log_nor_detail_error` — §5(c): the + same seven malformed answers, each embedding a JWT-shaped `SECRET`, asserted + absent from the raised message, from `detail_error` and from every formatted + log record. `DEEP_KEYS` in that module is the literal `repr` printed by the Chrome 152 probe, per the memory notes *mocked fakes can encode the bug* and *fixtures from the same @@ -276,11 +319,18 @@ exactly the failure mode that module's docstring warns about. It now answers the one-shot read with a JSON string; its assertions (`{"ls-key": "ls-value"}`) are unchanged. +Both fixtures key the viewport answer on **`innerWidth`**, not `JSON.stringify`. +`FakeTab._answer_for_js` returns the first substring that matches, and the storage +read and the viewport read now both *begin* with `JSON.stringify`, so a shared key +would have made dict insertion order decide which JSON the storage read received — +a fixture that is correct by accident. Each expression is keyed on a token unique +to it. + ## 7. Files changed | File | Δ | |---|---| -| `embedded/page_storage.py` | **new**, 133 LOC (leaf) | +| `embedded/page_storage.py` | **new**, 172 LOC (leaf) | | `embedded/browser_manager.py` | +21 / −22 → **1528 LOC** | | `embedded/debug_logger.py` | +10 / −1 (`log_warning(error=…)`) | | `tools/check_file_budgets.py` | grandfather row **1529 → 1528** (ratchet DOWN, cap == actual) | @@ -308,6 +358,12 @@ unchanged. for the value by value and is safe. A sweep of the remaining bare `tab.evaluate` callers that expect a non-primitive is the durable fix and is larger than this finding. +* **There is deliberately NO named home for the `JSON.stringify` idiom here.** + Three sites now use it (the viewport, and this module's two stores), and a + fourth family — the cloner's nested BiDi nodes — is being measured under + **F-872**, which owns the decision about whether these collapse into one home + and where it lives. Extracting a shared helper in this branch would pre-empt + that with a home chosen from three examples instead of four. * **`console_logs: []` was not investigated.** The live record also carried an empty `console_logs`, which `PageState` defaults to and nothing in `get_page_state` ever populates. It may be a second untruthful field; it is a diff --git a/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py b/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py index 8ed0b8b..bdb9c2b 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py +++ b/src/stealth_chrome_devtools_mcp/embedded/browser_manager.py @@ -1473,8 +1473,8 @@ async def get_page_state(self, instance_id: str) -> PageState | None: except Exception as e: # F-869: ONE place a collection failure is recorded, at WARNING with # its traceback. It used to be INFO'd as "storage unavailable" and - # dropped, so a defect in this package was invisible to the caller - # and to error reporting alike. + # dropped. This reaches the caller (get_instance_state's partial + # record) and a post-mortem; NOT Sentry — see the finding's §8. debug_logger.log_warning( "browser_manager", "get_page_state", diff --git a/src/stealth_chrome_devtools_mcp/embedded/page_storage.py b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py index 60aae00..867c0de 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/page_storage.py +++ b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py @@ -43,6 +43,20 @@ ``detail_error`` record (the named sub-field degradation, DESIGN §9). There is no third outcome and no ``{"success": False}`` dict. +**No message here ever carries a stored VALUE.** A page's localStorage is where +its session tokens and JWTs live, and a :class:`StorageReadError` travels three +ways at once: into the durable backend log, into ``get_instance_state``'s +``detail_error`` (i.e. to the MCP client), and into a Sentry breadcrumb — +``LoggingIntegration(event_level=ERROR)`` reduces WARNINGs to breadcrumbs that +ride out attached to some later event, and ``observability._scrub_event`` strips +emails and URL query strings, not a bare bearer token. The defect this module +closes never logged storage contents; the fix must not introduce a leak the +defect did not have. So every message reports SHAPE and COUNT only — a type +name, an index, a field count, a character count — never a key and never a +value. The single exception is Chrome's own ``SecurityError`` text on +:class:`StorageBlockedError`, which describes the property ACCESS and is +produced before anything is read. + A leaf: it imports no other embedded module and takes the tab as an argument. """ @@ -102,18 +116,29 @@ class StorageReadError(Exception): def _entries(record: object, store: str) -> dict[str, str]: """One store's ``{ok, entries}`` record as a plain ``{key: value}`` dict.""" if not isinstance(record, dict): - raise StorageReadError(f"{store}: unexpected record {record!r}") + raise StorageReadError( + f"{store}: record is {type(record).__name__}, not an object" + ) if not record.get("ok"): + # Chrome's own SecurityError text, which describes the PROPERTY ACCESS + # and carries no stored value — the one page-supplied string this + # module repeats (see the no-values rule above). raise StorageBlockedError(f"{store}: {record.get('reason')}") rows = record.get("entries") if not isinstance(rows, list): - raise StorageReadError(f"{store}: unexpected entries {rows!r}") + raise StorageReadError(f"{store}: entries is {type(rows).__name__}, not a list") entries: dict[str, str] = {} - for row in rows: + for index, row in enumerate(rows): # Every row is checked rather than unpacked: a malformed answer must be # a named StorageReadError, not a ValueError out of tuple unpacking. - if not isinstance(row, list) or len(row) != _PAIR: - raise StorageReadError(f"{store}: unexpected entry {row!r}") + if not isinstance(row, list): + raise StorageReadError( + f"{store}: entry {index} is {type(row).__name__}, not a pair" + ) + if len(row) != _PAIR: + raise StorageReadError( + f"{store}: entry {index} has {len(row)} fields, not {_PAIR}" + ) entries[str(row[0])] = str(row[1]) return entries @@ -122,12 +147,26 @@ async def read(tab: Tab) -> tuple[dict[str, str], dict[str, str]]: """``(local_storage, session_storage)`` for the page ``tab`` is showing. Raises :class:`StorageBlockedError` when the page itself refused the read, and - :class:`StorageReadError` when the answer was not the promised JSON. + :class:`StorageReadError` for every other way the answer can be wrong — + including a non-JSON string and JSON that is not the object this module + asked for, which would otherwise escape as a ``JSONDecodeError`` / an + ``AttributeError`` and make the module's two outcomes into four. """ answer = await tab.evaluate(READ_JS) if not isinstance(answer, str): - raise StorageReadError(f"evaluate answered {type(answer).__name__}: {answer!r}") - payload = json.loads(answer) + raise StorageReadError( + f"evaluate answered {type(answer).__name__}, not the JSON string asked for" + ) + try: + payload = json.loads(answer) + except ValueError as bad_json: + raise StorageReadError( + f"evaluate answered {len(answer)} characters that are not JSON" + ) from bad_json + if not isinstance(payload, dict): + raise StorageReadError( + f"evaluate answered JSON {type(payload).__name__}, not an object" + ) return _entries(payload.get("local"), "localStorage"), _entries( payload.get("session"), "sessionStorage" ) diff --git a/tests/test_instance_state_cookies.py b/tests/test_instance_state_cookies.py index 1408674..d84e3a1 100644 --- a/tests/test_instance_state_cookies.py +++ b/tests/test_instance_state_cookies.py @@ -81,9 +81,10 @@ # the live product raised ``TypeError: unhashable type: 'dict'`` here while # this fixture stayed green. Storage is now ONE ``JSON.stringify`` read — # the same trick F-844 applied to the viewport — so the fixture answers it - # the same way: a JSON *string*. Keyed by ``read('localStorage')`` and - # placed BEFORE the viewport entry because both expressions start with - # ``JSON.stringify`` and FakeTab takes the first matching substring. + # the same way: a JSON *string*. Both expressions now begin with + # ``JSON.stringify`` and FakeTab returns the FIRST substring that matches, so + # each is keyed on a token unique to it — ``read('localStorage')`` here and + # ``innerWidth`` below — rather than on dict order. # See tests/test_page_state_storage.py, which is F-869's home. "read('localStorage')": ( '{"local":{"ok":true,"entries":[["ls-key","ls-value"]]},' @@ -92,7 +93,7 @@ # A JSON *string*, because that is what the product now asks the page for # and what nodriver hands back for one. A dict here would model an # `evaluate` that returns plain objects — which it does not (see below). - "JSON.stringify": '{"width":1280,"height":720,"devicePixelRatio":1}', + "innerWidth": '{"width":1280,"height":720,"devicePixelRatio":1}', } @@ -192,7 +193,7 @@ async def test_page_state_accepts_a_fractional_device_pixel_ratio(manager_and_ta ``int``: nobody wants ``"width": 1280.0``. """ manager, tab = manager_and_tab - tab._evaluate_map["JSON.stringify"] = ( + tab._evaluate_map["innerWidth"] = ( '{"width":1280,"height":720,"devicePixelRatio":1.25}' ) diff --git a/tests/test_page_state_storage.py b/tests/test_page_state_storage.py index 9857778..d1e4b0d 100644 --- a/tests/test_page_state_storage.py +++ b/tests/test_page_state_storage.py @@ -37,7 +37,7 @@ list of plain strings — which is precisely why this defect was green there through F-844's own live-driven fix. -The two pins: +The three pins: * :func:`test_storage_comes_back_from_the_shape_chrome_really_sends` drives the real shape through the reader and asserts the VALUES arrive. Under the old @@ -47,6 +47,13 @@ (``partial: True`` + ``detail_error`` carrying the exception text) and is logged at WARNING **with a traceback**, instead of INFO + ``{}`` + ``partial: false``. +* :func:`test_a_malformed_answer_never_quotes_the_storage_it_was_reading` and + :func:`test_the_storage_value_reaches_neither_the_log_nor_detail_error` pin the + thing the FIX could have broken. Making a failure visible means putting a + message in the durable log, in the client's ``detail_error`` and in a Sentry + breadcrumb; the data being read is a page's session tokens. Every malformed + answer in :data:`MALFORMED` embeds :data:`SECRET`, so a diagnostic built with + ``{rows!r}`` fails these — a leak the defect itself never had. """ from __future__ import annotations @@ -114,6 +121,13 @@ def _page_js(storage_answer: str) -> dict[str, str]: expression. Both the OLD per-key expressions and the NEW one-shot read are answered, so this fixture is honest against either implementation — the pin fails on the product's behaviour, never on the harness not knowing the JS. + + Every key is UNIQUE to one expression. ``FakeTab._answer_for_js`` returns the + first substring that matches, so keying two entries on ``JSON.stringify`` — + which both the storage read and the viewport read begin with — would make + dict order decide which JSON the storage read receives. ``innerWidth`` + appears only in the viewport expression and ``read('localStorage')`` only in + the storage one. """ return { "window.location.href": PAGE_URL, @@ -124,10 +138,9 @@ def _page_js(storage_answer: str) -> dict[str, str]: "localStorage.getItem": "1", "Object.keys(sessionStorage)": DEEP_KEYS, "sessionStorage.getItem": "9", - # NEW path (checked BEFORE the viewport read, which also starts with - # "JSON.stringify" — dict order is insertion order, so this entry wins). + # NEW path. "read('localStorage')": storage_answer, - "JSON.stringify": VIEWPORT_JS, + "innerWidth": VIEWPORT_JS, } @@ -279,29 +292,111 @@ async def test_a_blocked_page_is_logged_at_info_and_carries_no_traceback(caplog) assert all(r.exc_info is None for r in blocked) -@pytest.mark.parametrize( - "answer", - [ - pytest.param(object(), id="a-husk-instead-of-a-string"), - pytest.param('{"local": null, "session": null}', id="a-record-that-is-not-one"), - pytest.param( - '{"local":{"ok":true,"entries":[["only-a-key"]]},' - '"session":{"ok":true,"entries":[]}}', - id="an-entry-that-is-not-a-pair", - ), - pytest.param( - '{"local":{"ok":true,"entries":"nope"},"session":{"ok":true,"entries":[]}}', - id="entries-that-are-not-a-list", - ), - ], -) +# --------------------------------------------------------------------------- +# (c) the malformed-answer paths, and the leak the fix must not introduce +# --------------------------------------------------------------------------- + +#: A JWT-shaped value of the kind that really lives in a page's localStorage. +#: Every malformed answer below EMBEDS it, so a message built with ``{value!r}`` +#: anywhere on these paths puts it in the durable backend log, in +#: ``get_instance_state``'s ``detail_error`` (i.e. in the MCP client's hands) and +#: in a Sentry breadcrumb — ``_scrub_event`` strips emails and URL query strings, +#: not a bare bearer token. +SECRET = "eyJhbGciOiJIUzI1NiJ9.a-real-session-token.c2lnbmF0dXJl" + + +class _Husk: + """Stands in for the ``ExceptionDetails`` husk ``tab.evaluate`` returns + instead of raising — whose ``repr`` carries the JS error's own text.""" + + def __repr__(self) -> str: + return f"ExceptionDetails(description='storing {SECRET} failed')" + + +#: A well-formed, empty sessionStorage record, so each case below is malformed in +#: exactly ONE way. +_EMPTY: dict[str, object] = {"ok": True, "entries": []} + + +def _local(broken: object) -> str: + return json.dumps({"local": broken, "session": _EMPTY}) + + +MALFORMED = [ + pytest.param(_Husk(), id="a-husk-instead-of-a-string"), + pytest.param(f"not json at all: {SECRET}", id="a-string-that-is-not-json"), + pytest.param(json.dumps(SECRET), id="json-that-is-not-an-object"), + pytest.param(_local(["localStorage", SECRET]), id="a-record-that-is-not-an-object"), + pytest.param( + _local({"ok": True, "entries": SECRET}), id="entries-that-are-not-a-list" + ), + pytest.param( + _local({"ok": True, "entries": [SECRET]}), id="an-entry-that-is-not-a-pair" + ), + pytest.param( + _local({"ok": True, "entries": [["k", SECRET, "extra"]]}), + id="an-entry-with-three-fields", + ), +] + + +@pytest.mark.parametrize("answer", MALFORMED) async def test_an_answer_that_is_not_the_promised_json_is_an_error(answer): """Not-the-JSON-we-asked-for is a read failure, never "no storage". ``tab.evaluate`` answers a JS throw with an ``ExceptionDetails`` husk rather - than raising, so "it returned something" is not evidence that it worked. + than raising, so "it returned something" is not evidence that it worked. A + non-JSON string and a JSON scalar are covered here too: they used to escape + as `JSONDecodeError` / `AttributeError`, which propagate correctly but make a + module that promises two outcomes have four. """ tab = FakeTab(evaluate_map={"read('localStorage')": answer}) with pytest.raises(page_storage.StorageReadError): await page_storage.read(tab) + + +@pytest.mark.parametrize("answer", MALFORMED) +async def test_a_malformed_answer_never_quotes_the_storage_it_was_reading(answer): + """The fix must not introduce a leak the defect never had. + + The old per-key loop crashed before it could log anything about the page's + data. A diagnostic built with ``{rows!r}`` / ``{row!r}`` would be strictly + worse than the bug: `page_storage`'s messages therefore report SHAPE and + COUNT only — a type name, an index, a field count, a character count. + """ + assert SECRET in repr(answer), ( + "the fixture must carry the secret, or this pin asserts nothing" + ) + tab = FakeTab(evaluate_map={"read('localStorage')": answer}) + + with pytest.raises(page_storage.StorageReadError) as raised: + await page_storage.read(tab) + + assert SECRET not in str(raised.value) + assert SECRET not in repr(raised.value.args) + + +@pytest.mark.parametrize("answer", MALFORMED) +async def test_the_storage_value_reaches_neither_the_log_nor_detail_error( + answer, call_tool, patched_server, caplog +): + """The same rule at the two places the message actually travels to. + + `detail_error` goes to the MCP client; the WARNING record goes to the durable + backend log and rides out as a Sentry breadcrumb attached to a later event. + """ + manager, tab = _manager(STORAGE_OK) + tab._evaluate_map["read('localStorage')"] = answer + srv = patched_server(browser_manager=manager) + + with caplog.at_level(logging.DEBUG, logger="stealth.backend"): + state = await call_tool(srv, "get_instance_state", instance_id=INSTANCE_ID) + + assert state["partial"] is True + assert SECRET not in state["detail_error"] + # ``format`` rather than ``getMessage``: the WARNING carries exc_info, and the + # rendered traceback is what a log file and a breadcrumb actually hold. + rendered = logging.Formatter() + for record in caplog.records: + assert SECRET not in rendered.format(record) From a8d696b18d01be5918e63b52b62973b4dcb9eb8b Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Tue, 15 Sep 2026 00:23:45 -0400 Subject: [PATCH 3/3] fix(F-869): bound the page-authored refusal text repeated into the log page_storage repeats one page-supplied string: the refusal text on StorageBlockedError, which browser_manager logs at INFO. The comment called it "Chrome's own SecurityError text, which carries no stored value" - true about values, wrong about provenance. Measured on Chrome 152: window.localStorage is an OWN accessor with configurable: true, so a page can Object.defineProperty a throwing getter over it and author that string itself, at any length, straight into the durable backend log and a Sentry breadcrumb. It is now capped at BLOCKED_REASON_CHARS = 200 with a trailing ellipsis so a reader can tell a cut message from a short one. Chrome's own wording is 98 characters ("Failed to read the 'localStorage' property from 'Window': Storage is disabled inside 'data:' URLs."), so every genuine diagnostic survives whole - the reason the constant carries that measurement in its comment rather than a round number with no argument behind it. The comment at the raise site, the class docstring and the module's no-values paragraph now say page-CONTROLLED and bounded, not "carries no stored value": the wrong reassurance is worse than none, because it is the sentence a future reader would trust when deciding whether to widen this. Two pins, both halves: a reason longer than the budget is stored truncated and marked (RED without the cap - a 10 000-character reason came through whole), and Chrome's real 98-character message survives untouched, so the cap cannot quietly start costing a real diagnostic. --- CLAUDE.md | 2 +- ...stance_state_swallows_storage_typeerror.md | 25 +++++++-- .../embedded/page_storage.py | 52 ++++++++++++++---- tests/test_page_state_storage.py | 53 ++++++++++++++++++- 4 files changed, 116 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2c6fae6..3bbed5a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,7 @@ Package root: `src/stealth_chrome_devtools_mcp/`. Two console scripts (`pyprojec | File | Owns | |---|---| | `browser_manager.py` | `BrowserManager` — spawn/list/close instances; `close_instance` offloaded teardown. `get_page_state` owns the page-state COMPOSITION and its error POLICY, not the storage read (F-869): `page_storage.StorageBlockedError` is the page refusing (INFO, empty dicts, still `partial: false`), and every other exception propagates so `get_instance_state` can answer its `partial: True` + `detail_error` record — the one degradation shape, reached by raising, exactly as this method's docstring always said | -| `page_storage.py` | **THE one home for "read a page's localStorage/sessionStorage"** (F-869) — the ONE `JSON.stringify` round trip that reads both stores (`READ_JS`, `read`), and the two outcomes it distinguishes: `StorageBlockedError` (Chrome threw *inside the page* on an opaque origin / `data:` URL / policy-blocked storage — the expected answer) and `StorageReadError` (the evaluate did not answer with the promised JSON, so empty storage would be a lie). It is `JSON.stringify` for the same reason the viewport read next to it is (F-844): `tab.evaluate` always sends deep `SerializationOptions` and returns `deep_serialized_value.value` RAW, so `Object.keys(localStorage)` arrives as `[{'type':'string','value':'k'}, …]` — measured on Chrome 152 — and the old per-key loop hashed a `dict`. One round trip also retires the `f"localStorage.getItem('{key}')"` interpolation (a quote in a key was a syntax error; `');…` was injection) and 2N+2 CDP calls. A leaf: imports no other embedded module, takes the tab as an argument | +| `page_storage.py` | **THE one home for "read a page's localStorage/sessionStorage"** (F-869) — the ONE `JSON.stringify` round trip that reads both stores (`READ_JS`, `read`), and the two outcomes it distinguishes: `StorageBlockedError` (Chrome threw *inside the page* on an opaque origin / `data:` URL / policy-blocked storage — the expected answer) and `StorageReadError` (the evaluate did not answer with the promised JSON, so empty storage would be a lie). It is `JSON.stringify` for the same reason the viewport read next to it is (F-844): `tab.evaluate` always sends deep `SerializationOptions` and returns `deep_serialized_value.value` RAW, so `Object.keys(localStorage)` arrives as `[{'type':'string','value':'k'}, …]` — measured on Chrome 152 — and the old per-key loop hashed a `dict`. One round trip also retires the `f"localStorage.getItem('{key}')"` interpolation (a quote in a key was a syntax error; `');…` was injection) and 2N+2 CDP calls. **No message here ever carries a stored VALUE** — a page's localStorage is where its session tokens live and a `StorageReadError` reaches the durable log, `get_instance_state`'s `detail_error` AND a Sentry breadcrumb at once, so every message reports shape and count only (type name, entry index, field count, character count). The one page-supplied string repeated at all is the refusal text, kept because Chrome's wording is the diagnostic and **bounded** by `BLOCKED_REASON_CHARS` because `window.localStorage` is an own accessor with `configurable: true` (measured, Chrome 152) — a page can install its own throwing getter and author it. A leaf: imports no other embedded module, takes the tab as an argument | | `dom_handler.py` | DOM manipulation + element interaction | | `element_resolution.py` | selector resolution that survives CDP document-node invalidation (route ALL selector resolution through here — never `tab.select`/`find` directly) | | `proxy_forwarder.py` | authenticated egress-proxy forwarding + `_free_port` | diff --git a/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md index 8ef31f7..81b9eb5 100644 --- a/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md +++ b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md @@ -267,14 +267,29 @@ The defect being fixed never logged storage contents — it crashed before it co A fix that made the failure visible by quoting the data would have been strictly worse than the bug. So `page_storage` states the rule in its module docstring and every message reports **shape and count only**: a type name, an index, a field -count, a character count. The single page-supplied string it repeats is Chrome's -own `SecurityError` text on `StorageBlockedError`, which describes the property -*access* and is produced before anything is read. Three parametrized pins embed a +count, a character count. Three parametrized pins embed a JWT-shaped `SECRET` in every malformed answer and assert it reaches neither the raised message, nor `detail_error`, nor any **formatted** log record (formatted, not `getMessage()`, because the WARNING carries `exc_info` and the rendered traceback is what a log file and a breadcrumb actually hold). +**The one page-supplied string that IS repeated, and its bound.** +`StorageBlockedError` carries the refusal text, because Chrome's own wording is +the diagnostic — *"Storage is disabled inside `data:` URLs"* is the answer an +operator needs — and `browser_manager.py:1447` logs it at INFO. But it is not +Chrome's word: measured on Chrome 152, `window.localStorage` is an **own accessor +with `configurable: true`**, so a page can `Object.defineProperty` a throwing +getter over it and author that string itself, at any length, straight into the +durable log and a Sentry breadcrumb. It is therefore capped at +`page_storage.BLOCKED_REASON_CHARS = 200` — Chrome's real message is 98 +characters, so every genuine diagnostic survives whole — with a trailing `…` so a +reader can tell a cut message from a short one. Both halves are pinned: +`test_a_page_authored_refusal_is_truncated_to_the_budget` (RED without the cap: a +10 000-character reason came through whole) and +`test_chromes_own_refusal_survives_the_budget_whole`. The comment at the raise +site now says *page-controlled, bounded*, not "carries no stored value" — true but +the wrong reassurance. + ## 6. Tests `tests/test_page_state_storage.py` (new, hermetic, no browser): @@ -305,6 +320,10 @@ traceback is what a log file and a breadcrumb actually hold). same seven malformed answers, each embedding a JWT-shaped `SECRET`, asserted absent from the raised message, from `detail_error` and from every formatted log record. +* `test_a_page_authored_refusal_is_truncated_to_the_budget` and + `test_chromes_own_refusal_survives_the_budget_whole` — §5(c): the refusal text + is bounded at `BLOCKED_REASON_CHARS` with a visible `…`, and the cap costs + Chrome's own 98-character message nothing. `DEEP_KEYS` in that module is the literal `repr` printed by the Chrome 152 probe, per the memory notes *mocked fakes can encode the bug* and *fixtures from the same diff --git a/src/stealth_chrome_devtools_mcp/embedded/page_storage.py b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py index 867c0de..2badfae 100644 --- a/src/stealth_chrome_devtools_mcp/embedded/page_storage.py +++ b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py @@ -53,9 +53,12 @@ closes never logged storage contents; the fix must not introduce a leak the defect did not have. So every message reports SHAPE and COUNT only — a type name, an index, a field count, a character count — never a key and never a -value. The single exception is Chrome's own ``SecurityError`` text on -:class:`StorageBlockedError`, which describes the property ACCESS and is -produced before anything is read. +value. The single page-supplied string repeated at all is the refusal text on +:class:`StorageBlockedError` — kept because Chrome's own wording is the +diagnostic, and **bounded** (:data:`BLOCKED_REASON_CHARS`) because it is +page-CONTROLLED text, not Chrome's word: ``window.localStorage`` is an own +accessor with ``configurable: true``, so a page can define a throwing getter +over it and author that string itself. A leaf: it imports no other embedded module and takes the tab as an argument. """ @@ -89,17 +92,33 @@ #: ``Object.entries`` yields ``[key, value]`` — two elements, always. _PAIR = 2 +#: Cap on the refusal text repeated into :class:`StorageBlockedError`. Measured on +#: Chrome 152: ``window.localStorage`` is an OWN accessor with +#: ``configurable: true``, so a page can ``Object.defineProperty`` a throwing +#: getter over it and dictate this string verbatim — page-controlled, unbounded +#: text on the path to the durable log and a Sentry breadcrumb. Chrome's own +#: wording is 98 characters ("Failed to read the 'localStorage' property from +#: 'Window': Storage is disabled inside 'data:' URLs."), so 200 keeps every real +#: message whole while a hostile one is cut. +BLOCKED_REASON_CHARS = 200 + +#: Appended when the cap bit, so a reader can tell a cut message from a short one. +_TRUNCATED = "…" + class StorageBlockedError(Exception): """The PAGE refused the read — an expected answer, not a failure. - Raised only when Chrome itself threw while the page touched - ``window.localStorage`` / ``window.sessionStorage``: an opaque origin, a - ``data:`` URL, storage disabled by policy. Measured message from Chrome 152 - on a ``data:`` URL:: + Raised when the throw came from touching ``window.localStorage`` / + ``window.sessionStorage``: an opaque origin, a ``data:`` URL, storage + disabled by policy. Measured message from Chrome 152 on a ``data:`` URL:: Failed to read the 'localStorage' property from 'Window': Storage is disabled inside 'data:' URLs. + + The text is repeated into this error's message but capped at + :data:`BLOCKED_REASON_CHARS`: a page can install its own throwing getter, so + the wording is not necessarily Chrome's and its length is not its own to set. """ @@ -113,6 +132,18 @@ class StorageReadError(Exception): """ +def _bounded(reason: object) -> str: + """The page's refusal text, capped at :data:`BLOCKED_REASON_CHARS`. + + Not a value, but not trustworthy either: see that constant for why a page can + author this string. + """ + text = str(reason) + if len(text) <= BLOCKED_REASON_CHARS: + return text + return text[:BLOCKED_REASON_CHARS] + _TRUNCATED + + def _entries(record: object, store: str) -> dict[str, str]: """One store's ``{ok, entries}`` record as a plain ``{key: value}`` dict.""" if not isinstance(record, dict): @@ -120,10 +151,9 @@ def _entries(record: object, store: str) -> dict[str, str]: f"{store}: record is {type(record).__name__}, not an object" ) if not record.get("ok"): - # Chrome's own SecurityError text, which describes the PROPERTY ACCESS - # and carries no stored value — the one page-supplied string this - # module repeats (see the no-values rule above). - raise StorageBlockedError(f"{store}: {record.get('reason')}") + # PAGE-CONTROLLED text, repeated because Chrome's own wording is the + # diagnostic — and BOUNDED for exactly that reason (BLOCKED_REASON_CHARS). + raise StorageBlockedError(f"{store}: {_bounded(record.get('reason'))}") rows = record.get("entries") if not isinstance(rows, list): raise StorageReadError(f"{store}: entries is {type(rows).__name__}, not a list") diff --git a/tests/test_page_state_storage.py b/tests/test_page_state_storage.py index d1e4b0d..5db4349 100644 --- a/tests/test_page_state_storage.py +++ b/tests/test_page_state_storage.py @@ -37,7 +37,7 @@ list of plain strings — which is precisely why this defect was green there through F-844's own live-driven fix. -The three pins: +The four pins: * :func:`test_storage_comes_back_from_the_shape_chrome_really_sends` drives the real shape through the reader and asserts the VALUES arrive. Under the old @@ -54,6 +54,13 @@ breadcrumb; the data being read is a page's session tokens. Every malformed answer in :data:`MALFORMED` embeds :data:`SECRET`, so a diagnostic built with ``{rows!r}`` fails these — a leak the defect itself never had. +* :func:`test_a_page_authored_refusal_is_truncated_to_the_budget` covers the one + page-supplied string the module DOES repeat. ``window.localStorage`` is an own + accessor with ``configurable: true`` (measured, Chrome 152), so the refusal + text is not necessarily Chrome's and its length is not Chrome's to set; + ``BLOCKED_REASON_CHARS`` bounds it, and + :func:`test_chromes_own_refusal_survives_the_budget_whole` is the other half — + the cap may not cost a real diagnostic a character. """ from __future__ import annotations @@ -340,6 +347,50 @@ def _local(broken: object) -> str: ] +async def test_a_page_authored_refusal_is_truncated_to_the_budget(): + """A blocked reason is page-CONTROLLED text, so its length is not the page's + to choose. + + Measured on Chrome 152: ``window.localStorage`` is an own accessor with + ``configurable: true``, so a page can ``Object.defineProperty`` a throwing + getter over it and its message reaches ``StorageBlockedError`` verbatim — + from there to the INFO line in the durable backend log and to a Sentry + breadcrumb. Chrome's own wording is 98 characters; anything past + ``BLOCKED_REASON_CHARS`` is cut, and says so. + """ + hostile = "A" * (page_storage.BLOCKED_REASON_CHARS * 50) + tab = FakeTab( + evaluate_map={"read('localStorage')": _local({"ok": False, "reason": hostile})} + ) + + with pytest.raises(page_storage.StorageBlockedError) as raised: + await page_storage.read(tab) + + message = str(raised.value) + assert len(message) < len(hostile) + assert message.endswith("…"), ( + "a cut message must be distinguishable from a short one" + ) + assert message.count("A") == page_storage.BLOCKED_REASON_CHARS + + +async def test_chromes_own_refusal_survives_the_budget_whole(): + """The cap may not cost a real diagnostic a single character.""" + real = ( + "Failed to read the 'localStorage' property from 'Window': " + "Storage is disabled inside 'data:' URLs." + ) + tab = FakeTab( + evaluate_map={"read('localStorage')": _local({"ok": False, "reason": real})} + ) + + with pytest.raises(page_storage.StorageBlockedError) as raised: + await page_storage.read(tab) + + assert str(raised.value) == f"localStorage: {real}" + assert "…" not in str(raised.value) + + @pytest.mark.parametrize("answer", MALFORMED) async def test_an_answer_that_is_not_the_promised_json_is_an_error(answer): """Not-the-JSON-we-asked-for is a read failure, never "no storage".