diff --git a/CHANGELOG.md b/CHANGELOG.md index ca700ba..3089115 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ ## 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. + ### Fixed — `status` reported a dead sibling's record as "the backend" (F-868) On a machine whose `server.json` recorded three display contexts — two dead diff --git a/CLAUDE.md b/CLAUDE.md index 87a99bc..13fb6be 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -81,7 +81,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. **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` | @@ -126,7 +127,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..81b9eb5 --- /dev/null +++ b/audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md @@ -0,0 +1,392 @@ +# 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. + +`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 +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. + +**(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. 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): + +* `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, 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. +* `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 +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. + +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**, 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) | +| `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. +* **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 + 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..bdb9c2b 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. 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", + 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..2badfae --- /dev/null +++ b/src/stealth_chrome_devtools_mcp/embedded/page_storage.py @@ -0,0 +1,202 @@ +"""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. + +**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 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. +""" + +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 + +#: 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 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. + """ + + +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 _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): + raise StorageReadError( + f"{store}: record is {type(record).__name__}, not an object" + ) + if not record.get("ok"): + # 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") + entries: dict[str, str] = {} + 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): + 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 + + +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` 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__}, 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 2fede91..d84e3a1 100644 --- a/tests/test_instance_state_cookies.py +++ b/tests/test_instance_state_cookies.py @@ -71,14 +71,29 @@ "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*. 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"]]},' + '"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). - "JSON.stringify": '{"width":1280,"height":720,"devicePixelRatio":1}', + "innerWidth": '{"width":1280,"height":720,"devicePixelRatio":1}', } @@ -178,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 new file mode 100644 index 0000000..5db4349 --- /dev/null +++ b/tests/test_page_state_storage.py @@ -0,0 +1,453 @@ +"""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 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 + 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``. +* :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. +* :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 + +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. + + 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, + "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. + "read('localStorage')": storage_answer, + "innerWidth": 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) + + +# --------------------------------------------------------------------------- +# (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", + ), +] + + +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". + + ``tab.evaluate`` answers a JS throw with an ``ExceptionDetails`` husk rather + 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) diff --git a/tools/check_file_budgets.py b/tools/check_file_budgets.py index 0ac3f88..66dbd1a 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