fix(F-869): get_instance_state reports storage truthfully; the storage reader no longer hashes nodriver's deep-serialized nodes - #103
Merged
Conversation
…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
… 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.
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.
…oth Unreleased entries
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect (F-869)
get_instance_stateon a real page (www.google.com, 2.1.5 live backend) returnedlocal_storage: {},session_storage: {},partial: falsewhile the backend log saidStorage access unavailable for <id>: unhashable type: 'dict'at INFO. A PythonTypeErrorin our code was caught by a broadexcept, logged with the message written for opaque origins, and reported to the caller as a clean empty result.Root cause: nodriver 0.47
Tab.evaluatealways requests deep serialization and returnsdeep_serialized_value.valueraw, soObject.keys(localStorage)arrives as[{'type':'string','value':'alpha'}, …]; the reader used those dicts as dict keys. Measured with a real headless Chrome (probe deleted). Full write-up:audit/stage2/finding_F869_get_instance_state_swallows_storage_typeerror.md.The fix
New leaf
embedded/page_storage.py, THE one home for reading a page's localStorage/sessionStorage through CDP: one evaluate returning a JSON string (no deep-serialized array crosses the boundary),StorageBlockedErrorfor an opaque origin (empty storage is then true,partialstays false),StorageReadErrorfor everything else, whichget_instance_stateturns intopartial: True+detail_error. The broadexceptand theexcept (RuntimeError, ConnectionError)that also let{}through are gone.browser_manager.py1529 → 1528 (grandfather row ratcheted down).debug_logger.log_warningforwardsexc_infoso the durable log carries the traceback.Messages never carry page values. Review caught that the first draft's error messages interpolated the page's own storage entries (where session tokens live) into the log,
detail_error, and Sentry breadcrumbs. All messages now report shape and count only; 17 pins assert a JWT-shaped secret in a malformed answer never reaches the raised message,detail_error, or any formatted log record.Evidence
assert {} == {'alpha': '1', 'beta': '2'}with the production INFO line reproduced hermetically;assert state["partial"] is True→False. GREEN after.browser_manager.py.Not fixed here (named)
tab.evaluatecallers expecting non-primitives (F-844 class) are not swept; the cloner's nested-array case is F-872 (measured separately) and owns the decision on the one home for the idiom.console_logsis never populated byget_page_state; not investigated.