Repository navigation
feat(preview): agents fill logins from an rbw vault without seeing the values - #561
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
the
vouch:trusted
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Independent review round 1: changes required at 2c716703aaf2513750fbd6323d2ec77c96d6f6ec (feature 3f1a12741e4a9c039304323755ef9b61fd287756).
Why: Real-browser probes reproduce password exposure in snapshot pixels and encoded results, plaintext username/TOTP exposure, and an evaluate bypass through a popup; navigation state also bypasses redaction.
So what: Fix the five blocking findings below and review the replacement head before merge; the required live Shopify proof remains outstanding.
Elon record:
- Requirements and who asked: The user requested a read-only independent security review against #557, its approved Design proposal and Design simplification, and the pinned PR head.
- Deleted: No implementation, production access, real-vault reads, merge, deployment, or repo-wide checks; no speculative origin-bypass finding.
- Bottleneck: The no-secret guarantee must hold at every browser output and document lifetime boundary, beyond the fill result itself.
- Checked myself: Read the full PR diff/body, all three Issue comments and Issue body, exact-SHA source and repository rules; ran 316 existing focused tests plus three disposable real-browser probes, and inspected the returned screenshot pixels.
Blocking findings, ranked by impact:
-
P1: Passwords are visible in returned screenshot pixels after reveal, or when filled into a text input. ServerBrowserPage.ts:425 captures the raw viewport; CredentialFill.ts:256 deliberately excludes the screenshot. The password field accepts ordinary text inputs too. I filled a fake password, clicked a normal Show password button using the production click helper, took a production snapshot, decoded its PNG and visually read the complete password. The textual snapshot was scrubbed. CredentialFill.test.ts:246 exercises reveal but only searches serialized base64 for the plaintext sentinel, which cannot establish pixel secrecy. The approved screenshot exception covers usernames and codes, not passwords. Password-bearing image/recording output needs protection and actual pixel-level proof.
-
P1: The result scrubber leaks URL-encoded passwords and trusts arbitrary object structure. CredentialFill.ts:129 uses raw/JSON-style patterns, without URL/form encoding. A real GET form containing fake password
REVIEW-Password+&=#?% 42producedREVIEW-Password%2B%26%3D%23%3F%25+42, which survived the production scrubber unchanged in URL/network-shaped results. ServerBrowser.test.ts:1710 uses a sentinel unchanged by encodeURIComponent, so misses this. Separately, CredentialFill.ts:254 never scrubs object keys and skips every property namedscreenshot, even an evaluate result{screenshot: password}. Both raw-key and fake-screenshot leaks reproduced. Failure mapping at ServerBrowser.ts:2041 also forwardserror.detailwithout scrubbing. Restrict exemptions to trusted image payloads and cover structured/encoded output before the broker boundary. -
P1: GET-form secrets enter renderer-facing thread session state outside handleRequest. ServerBrowser.ts:808 reports the raw request URL, and ServerBrowser.ts:588 reports the raw loaded URL/title. Manager.ts:276 installs this in the session snapshot and emits a navigated event with that snapshot. This never crosses the new handleRequest scrubber. The synthetic GET submission confirmed the secret-bearing URL; the state/event propagation is established by source inspection. The acknowledged address-bar risk therefore violates the renderer/thread-state acceptance criterion, even after fixing finding 2. Scrub before session state and event publication, not only before MCP responses.
-
P1: Username and TOTP plaintext leak through snapshots, including console entries. CredentialFill.ts:118 retains only passwords for general scrubbing. Element masking does not cover text/console/network echoes, and the snapshot returns console entries at ServerBrowserPage.ts:478. Input listeners that logged the fake username and code caused both complete values to appear in the returned production snapshot. Also, preview_press Backspace changed the code to five digits, which appeared on the input's own tree line because masking matches the original value. The approved design deliberately limits substring scrubbing, but its only explicit exposure waiver is screenshot pixels. This listed risk must be resolved against the tool-result/transcript requirement before merge. Existing tests echo only the password and never exercise edited credential values.
-
P1: A popup bypasses the evaluate read-back guard while the filled document remains alive. ServerBrowser.ts:775 creates fresh, unarmed credentials for each tab; ServerBrowser.ts:1181 adopts popups while preserving window.opener. ServerBrowser.ts:1997 checks only the evaluated tab. A normal same-origin popup opened by preview_click was unarmed and production evaluate could read
opener.document.querySelector('#pw').valueas a character array, bypassing exact-value scrubbing;{screenshot: ...}returned the complete raw string too. No new main-frame document loaded in the filled tab. The guard must account for reachable live credential documents, with a regression through the server/broker seam.
Other checks and risk decisions:
- Main-frame/iframe resolution, https refusal, exact/dot-boundary matching, and the post-read origin check looked sound in the paths checked. Existing real-browser cross-origin iframe and wrong-origin tests passed. An added probe navigated to an unrelated origin during vault.read: originMismatch, replacement field empty. Lookalike suffixes, userinfo tricks, Unicode-lookalike hostname, encoded-dot suffix and unrelated IP were rejected. This is host matching: non-default ports and an exact configured IP are intentionally accepted by the current code/tests; it is not strict scheme/host/port origin equality. No unrestricted race-freedom claim is made.
- rbw uses an argument array, shell:false, ignored stdin,
--before the validated item, bounded output and a 15-second per-process timeout. Exit 69 mapping and fixed error messages are covered by passing fake-spawner tests. stdout is not logged; stderr is classified internally, with only command/reason/exitCode logged. Audit success/error tests pass and mode is 0600. No secret was found in the normal fill request or outcome. - credentialVault defaults to none, the local variant requires an absolute rbwPath, and the general settings patch does not enable it. No real settings were changed.
- protocolLogging and fillFailed are justified fixed reasons; tabId is harmless audit context; the fixed PreviewAutomationExecutionError for evaluate is acceptable. Split OTP boxes are an honest unsupported case and not a merge blocker. GET exposure and non-pixel username/TOTP exposure are blockers as above.
- Downloads have no credential-content protection: an added real attachment-response probe exported the actual filled fake password and read it unchanged from the download stream; ServerBrowser saves downloads unchanged at line 956 and advertises their local paths. This requires a page/server exporting the value and subsequent file access, rather than evaluate. Treat it as an explicit remaining boundary limitation, not evidence that the guard prevents all read-back.
Executed on the exact head in a detached temporary worktree /tmp/chromeria-561-review, Node 22.23.1, pnpm 11.10.0, cached Playwright headless shell 1223:
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/CredentialVault.test.ts apps/server/src/preview/CredentialFill.test.ts apps/server/src/preview/ServerBrowser.test.ts apps/server/src/preview/ServerBrowserPage.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/mcp/toolkits/preview/handlers.test.ts packages/contracts/src/settings.test.ts
# 7 files passed, 316 tests passed, 7.45 seconds.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561Probe.test.ts
# Final disposable probes: 1 file passed, 3 tests passed.The disposable tests assert the observed exposure, not the desired security behavior. One intermediate blob-download fixture timed out waiting for a download; the final probe used a real attachment response and passed. No candidate source was edited. The temporary worktree is removed after the review; probe source/logs remain outside tracked source under /tmp.
Not checked: live Shopify login, real rbw/wrapper execution, real-vault stdout/stderr, actual subprocess timeout/kill behavior, a full server/client session-state integration reproduction, desktop/mobile behavior, exhaustive navigation races, full CI/typecheck/lint, or repo-wide checks. No real rbw get or rbw code, checkout/switch/reset/pull in either protected checkout, push, merge or deployment was performed.
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
…session state and popups Review round 1 on #561 found five leaks. While a filled document lives, snapshots of the tab and of tabs linked to it by opener return a 1x1 placeholder instead of a screenshot, and evaluate and recording are refused across that opener group. All three filled values are scrubbed from every result, error message and error detail, in raw, URL, form, JSON and HTML forms, keys included; only the snapshot's PNG data is exempt. Navigation status and popup URLs are scrubbed before they reach Manager session state. Element masking now masks a filled input's current value, so an edited code is covered. Refs #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Independent review round 2: CHANGES REQUIRED at 290cb2704d22910e4ed07157bfc25d248e689c8c (base e760e6af8be7762948211a8f39306fda3ab1cc89).
Why: Real Chromium probes still expose the password after GET navigation, through a concurrent popup recording, and in ordinary encoded strings; global scrubbing also breaks unrelated threads' response structure.
So what: Address the three P1 findings and the P2 scope/structure regression below, then independently review the replacement head; the live Shopify proof remains outstanding.
Elon record:
- Requirements and who asked: The user requested a read-only second independent review against #557, review 5578161274, the planner's five repair requirements, and this exact head, including actual browser pixels and collateral masking.
- Deleted: No implementation changes, real-vault reads, account access, deployment, merge, push, repo-wide checks, or speculative history-restoration finding.
- Bottleneck: A successful fill must remain protected across output boundaries, asynchronous operations, and transitions that still leave the value reachable.
- Checked myself: Read the full
gh pr diff 561 --repo toolboxmd/chromeria, PR body/comments, round-1 review, Issue body/comments, exact-head source and repository rules; reran the author's focused 524-test scope, typechecks and lint, and four independent probes through real Chromium plus production ServerBrowser/Broker/Manager, including decoded screenshot and recording pixels.
The user's goal is autonomous sign-in without returning the filled values or damaging unrelated browser work. The smallest corrections are to keep protection while credentials remain reachable, make recording/fill exclusion hold across asynchronous work in the opener group, cover the missing conventional encodings, and scope data redaction without rewriting trusted protocol keys.
Findings
-
P1: A new main-frame document is not proof that the password is gone. CredentialFill.ts:147-152 unconditionally disarms on any main-frame document. Consequently ServerBrowser.ts:1996 returns real pixels and ServerBrowser.ts:2051 permits evaluate immediately after a GET login submission, although the password is still in
location.href. Through the actual broker I filled a fake password, submitted a real GET form with production click, and calledpreview_evaluatewithArray.from(new URL(location.href).searchParams.get('pw')): the returned character array reconstructs the entire password. No secret was supplied in that expression. With the destination reflecting the submitted value into its heading, the production snapshot returned a 1280x800 PNG with the complete password legible; its text and session URL/title were correctly scrubbed. This requires no download or real vault. Keep the protection across a navigation that still carries or displays credentials, rather than treating the navigation event alone as a safe reset. Add the real GET destination and pixel/readback checks, not only a mock event followed by expected unblocking. -
P1: Recording exclusion races with a pending fill in a related tab. ServerBrowser.ts:2033-2047 checks for recording before awaiting the vault, while CredentialFill.ts:265-269 arms only after that wait. Each tab has its own action queue. Deterministic reproduction: open a same-origin popup; begin filling the opener and hold the fake vault response; start recording on the popup; release the vault response. Both
recordingStartand the fill succeed. A pre-existing popup button that displaysopener.document.getElementById('pw').value, clicked through the production helper, then places the password in the actual recording. I stopped it through the broker, decoded the returned video with ffmpeg, and visually read the full fake password in its last frame. A snapshot of that same popup was correctly replaced by the grey pixel. Sequential recording refusal works; it does not close this asynchronous window. Reserve/coordinate the incompatible operations over the group for the full fill/start transition, with a regression that holds the vault response while starting recording in another tab. -
P1: The encoding enumeration still misses conventional URL and HTML representations. CredentialFill.ts:81-99 handles fully uppercase or fully lowercase percent escapes, but not mixed case; its three HTML combinations omit
"together with'. Using fake passwordREVIEW-Password+&=#?% 42<"'>, both of these strings passed unchanged through the actual ServerBrowser/Broker result scrubber after navigation:REVIEW-Password%2b%26%3D%23%3F%25%2042%3C%22'%3EREVIEW-Password+&=#?% 42<"'>
Both decode to the complete password; the latter is the quote/apostrophe combination used by the installed React DOM server serializer (verified in its local source). These were produced from the page's existing query value without putting the password in the evaluate request. An independent unit probe also confirms that all-numeric HTML entities survive. Cover conventional mixed hex/entity variants and test literal expected forms independently of the production enumeration. The existing keys, arbitraryscreenshotproperties, raw errors, and ordinary uppercase/lowercase/form cases are fixed; this is the remaining part of round-1 finding 2.
-
P2: Scrubbing crosses every thread for the tab's entire lifetime and can destroy response schemas. ServerBrowser.ts:573-575 collects values from every tab, and CredentialFill.ts:157-167 never expires them on navigation or code expiry. CredentialFill.ts:311-312 also rewrites trusted transport keys. Real-browser reproduction: fill username
datain thread A, navigate A to a new document, open an unrelated tab in thread B, snapshot B. Its screenshot has keys["mimeType","••••••••","width","height"];screenshot.datais missing. This is a broken tool response, beyond masking an unrelated label. Expired six-digit codes likewise keep masking matching strings across every thread while the original tab remains. There is a 30-minute idle sweep for agent-owned tabs without viewers, but no absolute retention bound: activity refreshes it, and viewed/human tabs can remain indefinitely. Scope remembered-value redaction to the affected tab/opener group, preserve trusted protocol field names while scrubbing untrusted object keys, and give stale username/TOTP matching an explicit lifetime once retained buffers are safe. Do not simply expire the only protection while raw historical console/network buffers remain.
Round-1 findings checked
- 1, Show-password pixels: the exact original action is fixed while armed. I filled the real page, clicked its ordinary Show password control, confirmed the input really became text with the fake password, decoded the returned PNG and inspected its single grey pixel. Popup snapshots also return that placeholder. Protection is incomplete across navigation and concurrent recording as findings 1 and 2 show.
- 2, structured/encoded results and errors: raw keys and nested
screenshotproperties are scrubbed through the broker; only the snapshot image data exemption applies. A real evaluate exception containing the fake password returned a scrubbed error. Existing tests cover error-shaped recursive objects. Missing conventional encodings remain finding 3. - 3, renderer-facing navigation state: fixed on the exercised path. A real GET submission and a popup URL containing the encoded fake password produced scrubbed Manager session snapshots and actual
opened/navigatedevents. I awaited the destination'sSuccessevent; both its query URL and password-bearing title were masked. This does not fix the new document's evaluate/pixel exposure. - 4, console echoes and edited OTP input: fixed for the requested probes. Full username and code console echoes were absent from the returned snapshot, and Backspace-shortening the code did not expose its five digits on the input's tree line. A partial code in a separate console echo still remains the explicitly declared limitation.
- 5, same-document popup evaluate: fixed on the exercised path. The production broker refused the original
Array.from(opener.document.querySelector('#pw').value)expression while the opener remained armed; popup pixels were withheld too. Both directions of the group linkage are present in source. The lifetime reset and recording race above remain separate reachable paths.
Executed proof
Detached worktree /tmp/chromeria-561-r2, created exactly with the requested worktree command after fetch. Final HEAD is the reviewed SHA; tracked files were unchanged. Node 24.18.0 for the final runs, pnpm 11.10.0, cached Playwright headless shell 1223. The initial seven-suite run passed 321 tests; Node 24.18.0 was selected explicitly on PATH for the final runs.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# 22 files passed, 1 skipped; 524 tests passed, 1 skipped; 20.91 seconds.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R2.test.ts
# Final disposable probes: 1 file passed, 4 tests passed; 3.52 seconds.
# These tests assert the observed behavior, including exposures, not security success.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# Exit 0; Effect diagnostic suggestions were emitted.
pnpm exec vp lint <all 17 changed TypeScript files>
# Exit 0.
git diff e760e6af8be7762948211a8f39306fda3ab1cc89...HEAD --check
# Exit 0.The production browser/context implementation, input helpers, ServerBrowser, PreviewAutomationBroker, and Manager were used in the independent browser probes. The vault alone returned fake sentinels; all HTTPS fixture responses were fulfilled in isolated temporary browser contexts. There was no running full web client. The probe uses event/promise milestones, including a held vault read, rather than sleep/poll loops. The initial independent harness run selected the uncached full-Chromium path and failed before the probes; using the already cached headless shell corrected the harness. An initial history-return probe restored an empty password field, so no history-restoration leak is claimed. Video verification used the final painted frames; an immediate early stop did not yet include the changed page.
Test quality: the new complex password sentinel, real GET form, edited-input masking, trusted image-path exception and broker-level key assertions are useful regressions. The server regression is still a mocked browser/CDP test; its synthetic main-frame event asserts that screenshots/evaluate resume without proving the replacement document is safe. The real-browser fill test still checks serialized base64 for a plaintext sentinel rather than decoding pixels. Its encoding helper enumerates essentially the same forms as production and misses the conventional counterexamples above. The added server test also contains Effect.sleep("5 millis") polling loops despite AGENTS.md's event/milestone rule. None of the current regressions exercises concurrent recording versus a pending fill.
Not checked: live Shopify login; real rbw/wrapper, real vault stdout/stderr, or subprocess timeout/kill behavior; desktop/mobile/full-client flows; exhaustive navigation/concurrency schedules; arbitrary transformations beyond the stated cases; repo-wide CI. Downloads remain the previously reported, explicitly declared boundary. No production settings, canonical checkout contents, account state, or candidate tracked source were changed; no push or merge occurred.
Disposable probe source, observations, images/video and command logs are retained outside tracked source at /tmp/chromeria-561-r2-*; the review worktree was removed after verification, and its path is absent from Git's worktree list. These are fake-value evidence only.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
…clean Review round 2 on #561. The opener group of a filled tab stays protected (evaluate and recording refused, screenshots withheld) until every tab in it has loaded a document since the fill whose URL and title hold no filled value; a GET login destination no longer resets it. A fill and a recording start each claim their tab before their first await, so neither can begin while the other is pending in the same group. The scrubber decodes each line every way URL, form, HTML and JSON writers encode, two layers deep, and replaces a matching line whole. Scrubbing covers only the thread that holds the values, keeps trusted field names, and the values are dropped, with the group's stored entries scrubbed, once the group is clean. Refs #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Independent review round 3: CHANGES REQUIRED at 100cc53e40cf9d31c6a8010117a0ab01de558969, base e760e6af8be7762948211a8f39306fda3ab1cc89.
Why: Independent real-browser probes reproduce four credential leaks and a snapshot timeout; the current CI Lint job also fails on three exports introduced by this PR.
So what: Fix the six findings below and independently review the replacement head; the planner's live Shopify proof remains outstanding.
Elon record
- Requirements and who asked: The user requested a read-only third independent review of this exact head against #557, both prior reviews in full, the planner's protection-lifetime, atomic exclusion, decoding and scope decisions, and the original probes.
- Deleted: No implementation changes, real-vault reads, production settings, account login, push, merge, deployment, repo-wide local checks, or demands to decode base64/split values.
- Bottleneck: Protection must survive release, concurrent group changes and tab closure without losing the values needed to scrub an already captured response.
- Checked myself: Read both complete prior reviews, Issue body/comments, PR body/comments, complete candidate diff and relevant consumers; ran the 539-test focused scope, three typechecks, lint on all 17 changed TypeScript files, and 15 disposable probes using real Chromium with production ServerBrowser, Broker, Manager and recorder; decoded PNG/video artifacts and inspected the pixels; read the actual failing CI job log.
Blocking findings
1. P1: The snapshot that triggers lazy release returns historical plaintext
ServerBrowser.ts:2031-2038, release at :623-634.
Reproduction through the real broker: fill a password on a page with an ordinary input-event console echo; confirm an armed snapshot scrubs it; navigate to a clean document; make snapshot the first operation that invokes the clean-group guard. ServerBrowserPage.snapshot has already copied the raw console/network/action arrays before groupArmed replaces the stored entries and clears the values. The response scrubber then sees no values. The first clean snapshot returned consoleEntries[0].text containing the entire fake password; the next snapshot returned the mask. This is an immediate transcript leak, beyond the accepted delay in dropping values. Keep redaction available for the response already captured, and regress the first snapshot after clean navigation with real historical entries.
2. P1: An asynchronous clean check can erase a concurrent new fill
ServerBrowser.ts:616-634, CredentialFill.ts:270-280.
groupArmed snapshots the group, checks filling once, then awaits document-title reads and clears credentials without revalidating the group's state. documentClean likewise reads navigatedSinceFill before its await. Reproduction: fill the opener, navigate opener and popup to clean documents, then invoke evaluate on the popup. Hold the opener's title response at the last clean check; complete a new credential fill in the opener; release the title response. The popup evaluate succeeds and Array.from(opener.document.querySelector('#pw').value) reconstructs the full new password. A subsequent evaluate in the opener also returns it, and snapshots resume at 1280x800 although the newly filled document has never navigated. The held method delegates to the real Page.title after release; browser, vault-fill execution, per-tab queues and broker are otherwise production code with a fake vault. Make release conditional on the same unchanged group/fill state that was checked, and add this cross-tab interleaving regression. The new fill/recording claims close the original race but do not protect this release transition.
3. P1: Closing an unfilled middle popup disconnects protected descendants
ServerBrowser.ts:758-765, group traversal at :591-608.
The close path transfers only the closing tab's own values. For opener A, popup B and B's popup C, fill A and use a pre-existing control in C to display the raw value reachable through opener.opener. C correctly withholds pixels before closure. Close B, which has no values of its own: C is disconnected from A's group, receives no inherited values, and immediately permits evaluate and real screenshots without navigating. Through production click/close/evaluate/snapshot, the returned character array reconstructed the complete password; I decoded and visually inspected the returned 1280x800 PNG and read the complete password in its heading. Preserve protection across a group split even when the removed bridge tab was not itself filled. The direct filled-parent closure probe passes in a persistent test profile, so that narrower success does not cover this case.
4. P1: Ordinary named Unicode HTML entities still bypass scrubbing
CredentialFill.ts:72-105, lookup at :116-121.
The decoder's named-entity table covers ASCII punctuation only. With fake value REVIEW-café-42, a page input listener serializing é as é produces REVIEW-café-42. Both the actual scrubber and an armed snapshot through the real broker return that complete reversible value unchanged, alongside the correctly masked raw echo. The screenshot remains withheld, so this is a text-boundary bypass while protection is active. This is one layer of a normal named HTML entity, within the planner's explicit HTML-decoding scope; it is neither base64 nor a split/deeply encoded value. Cover valid named entities for the supported credential character set and add literal Unicode-entity counterexamples.
5. P2: Invalid numeric entities make protected snapshots time out
String.fromCodePoint throws for untrusted references such as � and �. The scrubber evaluates these even in unrelated text. A page input listener emitting unrelated � entry poisons the console buffer after fill: a production broker snapshot times out instead of responding because result scrubbing throws before broker.respond. A control snapshot with the same 1500 ms budget succeeds before the echo, and independent unit probes confirm the RangeError for both references. Handle invalid numeric references without throwing, and test the tool response with malformed page text. No mutual-lock deadlock was observed in the exercised schedules; this is a reproducible unanswered-request failure.
6. P2: The current CI Lint gate fails on newly exported internals
CredentialVault.ts:90, ServerBrowserPage.ts:320, ServerBrowserPage.ts:362.
The actual Lint job fails in vp run knip:check with Unused exports (3): make, dottedPattern, maskTree, and exits 1. Its log identifies merge candidate 2e0355dae8de305c0f4e531176d44a454475173b, merging this exact head into the stated base. Source search confirms the two page helpers have only local consumers; the vault factory is consumed locally by its layer. Remove unintended exports or use the repository's intentional-public-API convention where justified. Targeted vp lint passing does not establish this separate gate.
Prior findings and stated boundaries
- Show-password pixels: the original armed-tab reveal probe passes. The real input becomes text with the fake password, while the returned PNG decodes to the fixed 1x1 grey pixel; popup snapshots likewise withhold pixels. GET destinations remain withheld too. Finding 3 demonstrates a new lifetime escape.
- GET navigation: the real submitted URL contains the form-encoded password and the destination reflects it into title/body. Evaluate and recording remain refused and the PNG remains 1x1. Actual Manager opened/navigated events, sessions and status are scrubbed. A clean opener alone does not release an unclean popup; navigating both clean permits evaluate.
- Recording race: the held-vault/popup-recording reproduction now refuses recording; it remains refused after fill completes. I also held real recorder encoder startup and confirmed the reverse fill is refused before recording finishes starting. After clean navigation, the actual recorder starts/stops, yields a valid H.264 2560x1600 video, and a later fill succeeds after stop. Decoded video pixels show the clean page with empty inputs. No recorder was mocked; only startup was paused in the reverse-order probe.
- Encoded/structured outputs: the round-2 literal mixed-case percent escapes,
"', all-numeric entities, double-percent and JSON forms now scrub. Real input-listener echoes of mixed escapes, entities and JSON objects with secret keys/nested screenshot properties cross the snapshot boundary as masks. Recursive error-shaped values scrub at the direct helper seam. The original post-GET evaluate/error expressions are now refused before executing. Named Unicode entities remain finding 4. - Username/TOTP and edited input: complete raw username/code console echoes are scrubbed while armed; a Backspace-shortened code stays masked on its input's tree line.
- Scope and protocol keys: another thread receives
{data: 'order 826493'}unchanged while that code is armed elsewhere. Both affected/unrelated snapshots keepscreenshot.datawhen a username isdata. Attempts by the other thread to snapshot, evaluate, click or fill the protected tab fail with tab-not-found. This agrees with thread-qualified tab lookup, rather than assuming that request-thread redaction alone establishes isolation. - Closure/late tabs: a popup opened after fill is protected. Closing the filled parent transfers protection to the surviving popup in a persistent temporary profile, and a subsequent clean document releases it. The unfilled-bridge case is finding 3.
- Origin checks: the prior navigation-during-vault-read probe still returns originMismatch and leaves the replacement input empty; lookalike host/userinfo/encoded-dot/Unicode-lookalike probes remain refused. Existing real cross-origin iframe tests pass.
- Accepted limits: lazy release itself, whole-line collateral masking, more than two decoding layers, base64/split-value transforms by a hostile page, partial codes outside inputs, human live views, split OTP inputs and exported downloads remain the stated boundaries. I reran the real attachment-download probe and confirmed the exported fake value is unchanged; no new download blocker is asserted. The lifecycle leaks above occur within the planner's required protection rules.
Commands and results
Fetched with git fetch origin main feat/557-preview-fill-credential, then created exactly the requested detached worktree with:
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria worktree add /tmp/chromeria-561-r3 100cc53e40
pnpm install --offline --frozen-lockfile --ignore-scripts
# Exit 0. Ephemeral worktree dependencies only.Node v24.18.0 and pnpm 11.10.0; cached Chromium headless shell 1223. Commands below ran from that worktree, with /opt/homebrew/opt/node@24/bin first on PATH:
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# 22 files passed, 1 skipped; 539 tests passed, 1 skipped; 11.33 seconds.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R3.test.ts
# Final independent probes: 1 file passed, 15 tests passed; 7.80 seconds.
# OBSERVED tests assert the actual vulnerabilities, not security success.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# Exit 0 for the unmodified candidate, with the disposable probe removed.
git diff --name-only e760e6af8b...HEAD -- '*.ts' '*.tsx' > /tmp/chromeria-561-r3-lint-files.txt
xargs pnpm exec vp lint < /tmp/chromeria-561-r3-lint-files.txt
# Exit 0, all 17 changed TypeScript files.
git diff e760e6af8b...HEAD --check
# Exit 0.
ffprobe -v error -show_entries stream=codec_name,width,height -show_entries format=duration,size -of json /tmp/chromeria-561-r3-clean.webm
ffmpeg -y -v error -sseof -0.05 -i /tmp/chromeria-561-r3-clean.webm -frames:v 1 /tmp/chromeria-561-r3-clean-last.png
# Exit 0; decoded recording and returned screenshot PNGs inspected visually.
gh pr checks 561 --repo toolboxmd/chromeria
gh api --allow-escape-sequences repos/toolboxmd/chromeria/actions/jobs/118338398795/logs
# Lint failure verified from the real job log, as finding 6.The tracked candidate was never edited. Independent probes use isolated temporary browser contexts/HTTPS fixtures, a fake vault and event/promise milestones; the clean-check race additionally delays one real title read, and the reverse recording-order probe delays scratch-page startup. No full web client was running. Intermediate probe failures were fixture/oracle corrections: comparing a JSON-escaped password to raw text, and closing an isolated parent's entire context when trying to retain its child. The final closure probe uses a temporary persistent profile. An initial typecheck included the disposable probe and reported two probe typing errors; the candidate-only rerun passes.
Test quality: the literal encoding regressions, held vault read, unrelated-thread response and all-tabs-clean checks are useful. The server tests still mock page/CDP/recording; they do not assert the first snapshot's historical buffers after release, a concurrent re-fill during an awaited clean check, an unfilled bridge closing, Unicode named entities, or invalid code points. The real fill test's serialization checks cannot prove screenshot pixel secrecy. I did not repeat the author's source mutation experiments; the independent probes above establish the listed behavior directly.
Not checked: live admin.shopify.com login; real rbw/wrapper execution, real-vault stdout/stderr, subprocess timeout/kill behavior; desktop/mobile/full-client/relay flows; exhaustive navigation/concurrency schedules; every HTML entity or transform; repo-wide local CI. At inspection the forge still showed Test Server 8 pending, so no complete green-CI claim is made. No real rbw get/rbw code, protected-checkout checkout/switch/reset/pull, settings change, push or merge occurred.
Disposable fake-value probe source, logs, observations, PNGs and video remain outside tracked source at /tmp/chromeria-561-r3-*. The review worktree was removed after proof; its path is absent from Git's worktree list. Canonical checkout HEAD and tracked contents remain unchanged.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
3c1020d to
836ba18
Compare
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Request changes on 836ba184ccb508adcb72b95a8eb12fad5b504b71: five P1 privacy defects remain in the login quarantine.
Why: Live-origin metadata still exports a filled value, and several asynchronous/lifecycle transitions bypass or erase the quarantine.
So what: The implementation owner should fix these paths and obtain another exact-head review before merge; the ordinary three-step sign-in fixture works, but CI has no result on this head.
Elon record
Requirements and who asked: The owner requested independent correctness/privacy review of #557/#561: agents sign into the owner's accounts without receiving vault values; preserve normal multi-step sign-in.
Deleted: No implementation changes or replacement design; superseded scrubber requirements and the explicitly accepted oracle/live-view/incognito/download/split-code limits are not treated as defects.
Bottleneck: Quarantine lifetime transitions and content-bearing responses outside the central read guard.
Checked myself: Full PR diff/body and redesign comment; real Chromium with fake vault values through the production browser service/broker; actual MCP result/audit probe; focused suites, three scoped typechecks, and live GitHub check/merge state.
Blocking findings
1. P1: A later allowed fill exports and logs a prior value through origin
CredentialFill.ts:152, handlers.ts:248.
The fill result's origin is live page content, even when the group already holds a secret. With fake password review-secret-42, a preinstalled input listener navigates to https://<input.value>.example.test/. Snapshots remain refused, but the next allowed username fill returns origin: "https://review-secret-42.example.test". I confirmed the complete prior password in both content[0].text and structuredContent of the actual preview_fill_credential MCP response, and in the actual credential-fills.jsonl audit line. The item authorizes example.test, so the origin check passes normally. This is a direct response and persistent log leak, without ending the login or using a matching oracle. Do not export or audit a live page origin after a value has entered the group; cover both success metadata and credential-error origin metadata.
2. P1: A status read started before quarantine can return the newly filled title
statusWithTitle checks tab.quarantine before awaiting page.title(), and status bypasses the queue/running-operation drain. I held the first status title read before its real Playwright call, completed a real password fill whose input listener copies the value into document.title, then released the title read. The broker status response contains the complete password in title and has no loginInProgress field. The fill does not wait for this read. Revalidate quarantine at the response boundary, including reads already in flight when filling starts.
3. P1: A failed follow-up fill unlocks popups created during that attempt
After a successful fill, another fill replaces the group's login id. A popup opened during its vault read inherits that replacement id but is absent from previous. If the read fails before typing, rollback assigns the popup previous.get(member) ?? null, making it readable despite its filled opener remaining locked. With the second fake-vault read held, I opened and attached a real popup, verified it initially refused a snapshot, failed the read, and then reconstructed the complete original password using the popup's production evaluate call through opener.document. Its snapshot also returned full-size pixels. Preserve the existing login for tabs joining during a failed follow-up fill.
4. P1: Closing a filled opener while its popup attaches loses the popup's lock
Popup inheritance consults the opener's entry in tabs only after asynchronous attachment/setup. I filled a persistent-profile parent, opened its popup with the fake password in the query, held the popup's real CDP attachment, closed the parent through the broker, and completed attachment. The opener is now absent, so the popup receives quarantine: null. Its evaluate reconstructs the entire password from its URL; its normal full-size snapshot also returns that secret-bearing URL. The surviving popup was never closed or ended. Carry protection across pending popup creation and opener closure rather than relying on a later live-map lookup.
5. P1: Desktop reconnection resets quarantine on the same surviving document
ServerBrowser.ts:2624, reset at :786-789.
Desktop detach drops the ServerTab without closing the desktop page/session; reconnect creates a non-adopted tab with quarantine: null. A filled document therefore becomes readable when its debugger connection returns, for example after DevTools closes. I exercised the production detach/drop/findTab/createTab path with a real filled Chromium page and a fake desktop attachment boundary returning that same page: the page remained open, and subsequent evaluate returned the full password while status returned its secret-bearing title without a login flag. The actual desktop host's detach path likewise preserves its webContents. Preserve quarantine across connection lifetime changes. This probe does not claim a full Electron UI run.
Answers and cleared paths
- All reads protected? No, because of the findings above. With a normally attached, continuously locked group, the central guard refused all 13 probed operations, including snapshot text/pixels, evaluate, readPage, recording start and coordinate clicks. Console/network/action history are behind snapshot; status withheld downloads, file chooser, URL/title and dialog text. Real dialogs could be dismissed without returning their text; a real secret-bearing download was saved while status returned no download metadata.
- Only close/end-login releases it? No. Failed follow-up fill, pending-popup attachment after opener close, and desktop reconnect release surviving tabs. In the ordinary path, navigation and a failed fill after typing retained the lock. Another provider session/thread did not get page content in the exercised denial paths. Cross-environment transport was not exercised.
- Values stored/logged/returned? Yes, through the live-origin result/audit path. I found no deliberate retained credential-value store in the new fill implementation; normal fill errors were fixed/content-free in the exercised paths. Existing page console/network buffers can naturally contain echoes but are unreadable while correctly quarantined.
- Recording exclusion? Passed the real-recorder probes: a held vault read prevented a popup recording, and a held popup recorder startup prevented a related-tab fill; after recording stopped, filling worked.
- Fresh normal signed-in tab? Passed with an explicitly selected persistent
defaultprofile. End-login closed the group; a fresh tab evaluated/snapshotted normally and sent the fixture's signed-in cookie. Incognito loss remains accepted. - Three-step sign-in? Passed a real email page -> password page -> single-input OTP page using fills, locator clicks and wait-for only. The final cookie authenticated a fresh tab. Explicit navigation and coordinates were unnecessary. This establishes the fixture flow, not a live Shopify login.
- CI green? No complete CI result exists for this head. At 2026-10-19 23:04 UTC, GitHub reported
mergeable: CONFLICTING/mergeStateStatus: DIRTYagainst main5898397a657e9e1df162d2ddc48dbdf6a748d1c0. Only the three successful size/vouch metadata checks existed; no CI workflow/Checkresult was present. This is an absent merge gate, not a claimed CI test failure.
Commands and results
Reviewed head 836ba184ccb508adcb72b95a8eb12fad5b504b71, diff base 1e83a3169f77d5dbbd6727c929ffb8b0c1118754. Read gh pr diff 561 --repo toolboxmd/chromeria, the full PR body and #557 comment 6278773329. Fetched origin. The requested /tmp/chromeria-561-r4 already existed at this SHA, so I preserved it and created my own detached /tmp/chromeria-561-r4-independent; dependencies were installed there with pnpm install --offline --frozen-lockfile --ignore-scripts (exit 0). Node v24.18.0, pnpm 11.10.0, cached Chromium headless shell 1223.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# Final: 22 files passed, 1 skipped; 525 tests passed, 1 skipped; 15.41 seconds.
# Initial: 524 passed, 1 failed, 1 skipped. The unchanged settings test
# "reloads when a dangling settings link gets its destination" timed out at 2s.
# That test passed alone, then the complete focused rerun above passed.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R4.test.ts
# Final independent probes: 10 passed; 9.25 seconds.
# Six OBSERVED counterexamples assert five vulnerabilities, not privacy success.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# All exit 0, after removing the disposable probe from the candidate.
git diff 1e83a3169f...HEAD --check
# Exit 0.
gh pr checks 561 --repo toolboxmd/chromeria
# Size/vouch checks only, independent review pending during inspection.
# Exact-head check-runs/actions APIs corroborated absence of CI.All commands used /opt/homebrew/opt/node@24/bin first on PATH. Probes use real browser operations, isolated fake HTTPS fixtures and fake vault values, with promise/event milestones for races. The desktop transport boundary is simulated; the underlying page/CDP and quarantine paths are real. Intermediate probe failures were reviewer fixture/oracle errors, including holding a same-tab queued recorder, comparing URL escape spellings, expecting text the fixture had replaced, and requiring a particular denial tag. These were corrected without editing the candidate. No mutation of tracked candidate code was made, and no baseline red/green repair cycle is claimed.
Not checked: live Shopify/account login, real vault or rbw wrapper execution, full Electron/mobile/web-client/relay/cross-environment operation, exhaustive concurrency schedules, or repo-wide local CI. The PR's focused tests contain mocked browser/quarantine paths; the real fill-only tests and mocked MCP bridge do not cover the five production transitions above.
The disposable probe source and fake-value observations/logs remain outside tracked source under /tmp/chromeria-561-r4-*. My review worktree was removed after checks; the pre-existing requested worktree was preserved. The canonical checkout remains clean at its original d74c023f7816d12282e5cb22fed83db7bc1618b4; no push, merge, settings change or real-vault read occurred.
Verdict: request changes, with review/independent=failure on the reviewed SHA. This review is posted as a comment review because the authenticated GitHub account is also the PR author and cannot submit its own formal changes-requested review.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
…tine
preview_fill_credential names a vault item and a field (username, password,
totp) plus a locator. The server reads the value from rbw only after the
input's own frame is an https page of the item's sites, re-checks the origin,
types the value, verifies it and drops it; the result is only
{filled, item, field, origin, tabId}. The fill puts the tab's opener group,
including popups opened later, in a login: every read is refused with one
fixed error and session state shows "Login in progress" without URL or
title, until preview_end_login closes those tabs. Each attempt appends a
value-free line to credential-fills.jsonl.
Closes #557
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… leave Review round 4 on #561. A login is recorded by preview session and CDP target, not on the ServerTab object, so a desktop detach and reattach of the same page keeps it; it ends only with preview_end_login or when the session closes. A fill into a group already in a login never lifts it, and a popup takes its opener's login as the browser reports it, with a record of closed openers for popups still attaching. Every result is checked once more as it leaves: status-shaped results are taken again and other reads of a tab now in a login are refused, so a status read in flight during a fill cannot return the filled title. Fill results and audit lines name the matched vault host as `site`, never the live page origin, and refusals name no origin once the group holds a value. Refs #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
836ba18 to
c74169f
Compare
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Request changes on c74169f59dde62e4145853e71fa346510446fdc6: three P1 privacy defects remain, and the new integration suite fails in CI.
Why: Login commitment is still tracked per fill invocation, popup inheritance is read before asynchronous setup finishes, and refusal-origin permission is captured before overlapping fills can introduce a value.
So what: The implementation owner should fix these paths and the CI setup, then obtain another exact-head independent review before merge.
Elon record
Requirements and who asked: The owner requested defensive pre-merge review of #557/#561: agents sign into the owner's accounts without receiving or logging vault values; preserve three-step sign-in and cookie reuse, and verify the five round-4 findings and current CI.
Deleted: No implementation changes or replacement architecture; the accepted matching-oracle, human live-view, incognito, saved-download and split-code limits are not treated as findings.
Bottleneck: Concurrent fills and asynchronous tab construction still separate the protection decision from the page/group lifetime and the returned or audited data.
Checked myself: Full PR body/diff, round-4 review 5578803129 and redesign comment 6278773329; real Chromium through production ServerBrowser, broker, Manager and MCP with fake vault values; 533 focused tests, 10 adapted prior probes, three new privacy reproductions plus one refused lifecycle probe, three scoped typechecks, and completed exact-head GitHub CI/logs.
Blocking findings
1. P1: A failed first fill unlocks a group after a concurrent related fill has succeeded
ServerBrowser.ts:2123, with concurrent-fill tracking at 2115.
existing and typed describe this invocation, not whether another fill has committed a value to the same login. Fills in separate tabs have separate queues, and fills are deliberately excluded from running. Start the first fill in parent A and hold its fake-vault read; fill its already attached popup B successfully; then fail A's read. A still has existing.length === 0 and typed === false, so its rollback clears both tabs and their session/target records, including B's successful login.
The real-Chromium probe confirmed B refused a snapshot after its successful fill, then became readable after A failed. Production evaluate returned the complete fake password from B's input, and production status returned that password as B's title without loginInProgress. Neither tab was closed or ended. This violates invariant A; rollback must depend on the group's current commitment, not only this fill's local state.
2. P1: A popup can miss a login that starts between inheritance lookup and tab publication
ServerBrowser.ts:846, asynchronous setup at 876, publication at 979.
Open a popup before the parent is filled. Hold the popup's real setViewportSize call, after quarantine has been calculated as null but before tabs.set. Fill the parent successfully, then finish the popup's setup. The popup was absent from the fill's group scan and keeps the earlier null protection decision. No inheritance check runs when it is published.
The parent still refused snapshots, but the popup's production evaluate reconstructed the complete fake password through opener.document.getElementById('pw').value; its normal snapshot also returned 1280x800 pixels. The new response check cannot repair this: the popup itself has no lock, and a read started after the fill sees no epoch change. This violates invariants A and B even while the parent remains open. Inheritance must hold through publication, including logins begun during setup.
3. P1: An overlapping first fill still writes a prior value to the audit through refusal origin
CredentialFill.ts:116, 154, captured permission at ServerBrowser.ts:2111, audit sink at handlers.ts:270.
Start the first fill on A through the actual MCP tool and hold its vault read. Its revealOrigin is true. Fill related popup B; a preinstalled input listener navigates A to https://<B's value>.example.test/. Resume A's read. A's origin revalidation refuses, but named(originOf(frame)) still permits the live origin because A itself has not typed. executeTabOperation preserves this credential-error detail, and the handler writes it to the audit as site.
The actual credential-fills.jsonl line contained "site":"https://review-secret-44.example.test", the entire fake password. The MCP error text itself remained fixed and did not contain it. Thus the successful-result rename works, but invariant C is still false for concurrent refusal/audit paths. This persists a value before any later evaluate or snapshot is requested, independently of the readback consequence in finding 1.
4. P2: The new integration suite fails on the configured CI runner before exercising a regression
CredentialLogin.integration.test.ts:197.
The suite now finds the installed Chromium shell, but launches through the production sandboxed context without satisfying the runner's sandbox prerequisites. On this exact head, all seven tests fail opening their first tab in Test Server 1: This host blocks the sandbox T3's browser runs in (AppArmor on Ubuntu 23.10+). The job reports 7 failed, 821 passed, 10 skipped; the aggregate Check also fails. Make the fixture/CI browser setup runnable on the declared Ubuntu runner while retaining the regression assertions. This is a CI setup defect, not evidence that the seven tested privacy behaviors failed on Linux after a successful browser launch.
Round-4 disposition and requested answers
- The five original schedules now pass their privacy assertions. The sequential success-result/audit origin probe returns only the vault host; held status returns a redacted status; a failed follow-up fill keeps its new popup locked; a popup reported after filling stays locked when its opener closes during CDP attach; desktop detach/reattach of the same page remains locked. The author's seven real-Chromium integration tests also passed locally. This closes the demonstrated original schedules, but findings 1–3 show that the broader monotonicity, attachment and refusal-origin guarantees are not closed.
- Can a locked group return page content or release early? Yes. Findings 1 and 2 return the complete fake password. For a continuously attached, correctly locked group, the prior probes still refuse all 13 disallowed operations, with content-free status, dialog handling, downloads and allowed actions. Recording and filling excluded one another in both tested start orders.
- Values stored, logged or in errors? Yes, in the actual persistent audit through finding 3. The internal credential refusal carries the live origin. Its MCP text is fixed. I found no deliberate long-lived vault-value store in the fill/vault implementation; page console/network buffers naturally retain echoes, and their access depends on the quarantine. The rbw adapter maps failures to fixed reasons and does not deliberately log stdout/stderr, but real rbw/wrapper execution was not tested.
- Three-step sign-in and cookie reuse? Passed twice: the author's integration fixture and the prior independent fixture both completed email -> password -> single-input OTP using fills, locator clicks and waits, then
endLogin; a fresh persistent-profile tab was signed in. The independent fixture also obtained a normal fresh-tab snapshot. This is fixture proof, not a live account login. - Closed-opener record: I inspected the session/target maps and bounded closed-opener fallback. An additional attempt to expire the opener record by closing 65 later filled sessions did not unlock the attaching popup. The Manager event mirror serializes behind that attachment, so this attempt does not establish that the bounded fallback itself was exercised. No additional cache-eviction defect is claimed.
- Session versus target identity: the original same-document desktop reattach probe passes, and source inspection confirms both session and target records are restored. This does not remove the publication gap in finding 2 or the deletion of both records in finding 1. Cross-browser target collisions and cross-environment transports were not exercised.
- CI green? No. Waited for completion. CI run 39341119418 completed with
failureonc74169f59dde62e4145853e71fa346510446fdc6;Test Server 1and aggregateCheckfail. Build and Typecheck, Lint (including knip), Test, Test Web, server shards 2–8, Release Smoke, Fork Stack Model and Native fingerprint diff pass. The PR is mergeable butUNSTABLEagainst base5898397a657e9e1df162d2ddc48dbdf6a748d1c0.
Commands and results
Read gh pr view 561 --repo toolboxmd/chromeria, gh pr diff 561 --repo toolboxmd/chromeria, the full round-4 review and redesign comment via gh api. Fetched origin and created my own detached /tmp/chromeria-561-r5 at the requested head. Node v24.18.0 for tests/typechecks, pnpm 11.10.0, Playwright Chromium headless shell 1223 on macOS. Dependency setup pnpm install --offline --frozen-lockfile --ignore-scripts exited 0; that install initially used the ambient Node v26 and printed an engine warning, then all proof commands used Node 24.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# Exit 0: 23 files passed, 1 skipped; 533 tests passed, 1 skipped; 10.62s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R4.test.ts
# Final exit 0: 10 prior independent probes passed; 9.33s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R5.test.ts
# Final exit 0: 4 probes passed; 12.21s.
# Three assert the observed P1 counterexamples above, not privacy success.
# The fourth asserts refusal after the closed-opener churn attempt.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# All exit 0, after removing disposable probe files from the candidate.
git diff 5898397a65 HEAD --check
# Exit 0.
gh pr checks 561 --repo toolboxmd/chromeria
gh run view 39341119418 --repo toolboxmd/chromeria --json headSha,status,conclusion
gh api repos/toolboxmd/chromeria/actions/jobs/118362686516/logs --allow-escape-sequences
# CI completed/failure on exact head; downloaded and read the failed shard log.The original round-4 probe file asserted that vulnerabilities were present. A first verbatim rerun failed as those assertions stopped holding and then contaminated later fixture globals. I changed the six obsolete leak/readback assertions to privacy assertions, retained the production paths and race milestones, and ensured fixture cleanup on Effect failures; the final ten all pass. For the extra cache lead, the initial leak hypothesis was rejected by loginInProgress, so the final probe checks that refusal. No candidate production code or tracked test was changed; no baseline mutation test or new baseline red/green cycle is claimed.
Disposable probe sources, fake-value observations and logs are retained outside tracked source under /tmp/chromeria-561-r5-*. The review worktree was removed after checks; the pre-existing round-4 worktree was preserved. The canonical checkout remains clean at its starting d74c023f7816d12282e5cb22fed83db7bc1618b4. No push, merge, real-vault access, account login or machine configuration change occurred.
Not checked: real vault/rbw wrapper execution, live Shopify login, a full Electron/mobile/web-client/relay or cross-environment run, exhaustive concurrency schedules or target identity aliasing, and repo-wide local CI. Desktop reconnect proof uses a simulated transport boundary around a real Chromium document. Linux CI did not get past browser launch for the new integration suite. Local scoped proof is not claimed as the complete merge gate.
Verdict: request changes, with review/independent=failure on the exact reviewed SHA. Posted as a comment review because the authenticated account is also the PR author and cannot request changes on its own PR.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
…d logs no page data Review round 5 on #561. Delete the mechanisms that raced instead of locking them: a fill quarantines its group as it starts and nothing lifts that but preview_end_login or the session closing, even a fill that fails before typing. A new tab, popup or not, is checked at publication, with nothing awaited before it joins the tab list: if any tab of its browser context is in a login, it starts in that login, so no opener lookup or capture is needed. Refusals carry only a reason and the tab, never the page origin, and the audit line keeps time, environment, thread, item, field, the matched vault host and the result. The real-Chromium tests use the server's sandbox opt-out, as the other real-browser tests launch unsandboxed, so they run on CI's Ubuntu runner. Refs #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Independent review round 6: request changes on 40c519b3ad5a65454891bd150aa443c3b74fb333; three P1 privacy paths remain.
Why: End-login misses pending tabs, desktop context identity changes across connections, and popup metadata is published before the new context quarantine applies.
So what: The implementation owner should close these paths and obtain another exact-head review before merge; the round-5 reproductions and ordinary sign-in pass, and the main CI workflow succeeds.
Elon record
Requirements and who asked: The owner requested defensive pre-merge review of #557/#561 on this head, using fake values only: verify no rollback, context-wide quarantine of newly created tabs, content-free fill metadata/audit, earlier findings, three-step sign-in, cookie reuse and completed CI. This supports the Objective's agent working surface by protecting account sign-in.
Deleted: No implementation changes, replacement architecture, real-vault/account access, push or merge; no reintroduction of superseded scrubber requirements or findings against the accepted matching-oracle, human live-view, incognito, saved-download and split-code limits.
Bottleneck: Quarantine must cover a page from creation through attachment and final closure, and profile identity must survive separate desktop connections; protecting only a published ServerTab leaves gaps.
Checked myself: Full PR body/diff, redesign comment and all five prior review bodies; 536 focused tests, 14 adapted prior independent probes and three additional real-Chromium counterexamples; three candidate-only typechecks; exact-head completed CI and the actual browser-shard and size-labeller logs. No production source or tracked test was edited.
Blocking findings
1. P1: End-login releases a secret-bearing popup that is still attaching
ServerBrowser.ts:1963, immediate record deletion at 1977, inheritance at 973.
Fill parent A in a persistent profile; click its existing popup control, which opens /child?pw=<encoded input value>; hold the popup's real CDP attachment before publication. Call endLogin on A, then release attachment. The closing set contains only published tabs, so B is absent, and forgetLogin immediately removes A's context/session/target protection. B then publishes with no matching login and remains open.
The production broker returned closed: [A], then allowed B's evaluate expression Array.from(new URL(location.href).searchParams.get('pw')) to reconstruct the entire fake password. B's status returned the secret-bearing URL and no loginInProgress. This reproduced twice before the final combined run, which reproduced it again. The earlier ordinary close probe remains protected because its event mirror waits behind attachment; endLogin bypasses that ordering by deleting the record itself. End-login must cover pending members and must not leave a surviving page readable.
2. P1: Desktop tabs in one profile do not share the context quarantine
ServerBrowser.ts:539, consumed at 976; separate connections at ServerBrowserContexts.ts:224.
contextLogin compares Playwright BrowserContext objects with ===. Each desktop tab connects through a separate CDP endpoint and chromium.connectOverCDP call, which creates a different wrapper even when the underlying pages share a browser profile/storage context. Session and target keys preserve an individual page, but do not identify a newly created different page in that profile.
The probe used the production connectDesktopPage and production desktop createCdpRelayConnection, backed by real Chromium pages in the same actual context, with two loopback endpoints standing in for Electron's per-tab debugger channels. It asserted the underlying contexts were identical and the connected wrappers different. A page input listener copied A's filled fake password to localStorage. While A still refused snapshots, a newly opened B in the same default profile was readable: production evaluate Array.from(localStorage.reviewEcho) returned the complete value. Status simultaneously showed A locked and B unlocked. Repeated successfully in three runs. This is a failure of the explicit new-tab profile rule, not a claim that page storage itself is a vault adapter log. The full Electron app was not launched.
3. P1: A context-quarantined popup exports its URL through Manager state before it is locked
ServerBrowser.ts:1267, publication at 1273.
Open unrelated A and B in the same persistent profile before filling A. A's input listener copies the value to shared site storage; B has an ordinary popup button that puts that stored value in the popup URL. Fill A, then click B's button. B is a pre-existing unrelated tab, so the chosen model permits it to stay unlocked; its newly created popup C must nevertheless start quarantined under the context-wide rule.
adoptPopup hides the initial URL only when the opener is quarantined. It therefore passes C's live secret-bearing URL to manager.open, which stores and publishes it before createTab applies contextLogin. The real Manager opened event contained the complete reversible fake password in snapshot.navStatus.url; manager.list still contained that URL after C had attached. At the same point C correctly refused evaluate and its MCP status had url: null, loginInProgress: true. Thus the read guard does not repair the already emitted and retained renderer-facing state. Apply the context quarantine before content-bearing session metadata is published, as well as before tab reads.
Requested answers and prior-findings disposition
- Round 5's three demonstrated privacy schedules are closed on this head. The failed-first/concurrent-success schedule stays locked; the popup held at
setViewportSizejoins the login after a fill begins; the overlapping refusal's actual MCP result and audit contain no fake password, with auditsite: null. The rollback code and live-origin error field are removed. The adapted four round-5 probes pass, including the prior closed-opener churn refusal. - Round 5's Chromium CI launch failure is closed. The fixture opts out through the existing
T3CODE_SERVER_BROWSER_SANDBOX=0operator setting, matching the repository's unsandboxed Playwright test behavior. The UbuntuTest Server 1log shows all 11 CredentialLogin.integration tests passing, with 832 passed and 10 skipped in that shard. This establishes successful Linux execution, not just a local browser run. - Earlier demonstrated round-4 schedules stay closed: all 10 adapted probes pass, covering success-result/audit origin, held status, failed follow-up fill with a late popup, ordinary opener close during popup attach, same-document desktop detach/reattach, fixed errors and normal quarantine. This does not establish closure of every lifetime variant: findings 1 and 3 expose new attachment/publication schedules, and finding 2 is a previously unexercised desktop profile boundary.
- Rounds 1–3's scrubber/lazy-release/encoding/entity machinery is gone. Normal locked reads are refused rather than decoded or scrubbed, so those old mechanisms no longer exist. The prior independent probes still refuse all 13 disallowed operations across a multi-tab opener chain, including snapshot pixels/text, evaluate, readPage, recording and coordinate clicks. Closing an intermediate tab, navigation, dialog handling, downloads, failed post-typing fills, and both tested recording/fill start orders preserve the ordinary lock. These are bounded verified paths, not a universal absence-of-leaks claim.
- Can protected/related tabs still return content or release early? Yes: findings 1–3 demonstrate it. The first releases a surviving related popup, the second never marks a new desktop tab, and the third exports a locked popup's live URL outside the MCP response guard.
- Stored/logged/returned values: fill success metadata uses a vault host; refusals and the audit no longer use live origins. I found no deliberate long-lived vault-value store in the fill/vault implementation and reproduced no remaining audit leak. Browser/site storage and page console/network/action buffers can contain echoes; finding 2 reads a site-stored echo, finding 3 retains a value-bearing URL in Manager session state, and findings 1–2 return values through normal broker operations. Fixed MCP errors and the tested audit remain value-free. Real rbw/wrapper execution was not tested.
- Three-step sign-in plus cookie reuse passes twice: the candidate integration fixture and independent prior fixture both complete email → password → single-input OTP using fills, locator clicks and waits, then end-login. A fresh persistent-profile tab sends the signed-in cookie and can read the account page; the independent fixture also gets a normal screenshot. This is fixture proof, not live Shopify/account proof.
CI on the reviewed head
Waited for completion and checked gh pr checks 561 --repo toolboxmd/chromeria plus run identity. CI run 39343258453 completed success on 40c519b3ad5a65454891bd150aa443c3b74fb333: Build and Typecheck, Lint, Test, Test Web, all eight server shards, Release Smoke and aggregate Check pass. Fork Stack Model and Native fingerprint diff also pass; the listed mobile/Rust/preview jobs are skipped.
The complete PR check list is not all green: the separate Label PR size job failed with HttpError: Unexpected end of JSON input while adding size:XL. I read its log; this is the labeller's GitHub API failure, not a browser/test failure. Independent review is separately failure because of the three P1 findings above.
Commands and results
Read gh pr view 561 --repo toolboxmd/chromeria, gh pr diff 561 --repo toolboxmd/chromeria, redesign comment 6278773329 and the full five prior reviews using gh api; inspected the complete diff and affected consumers. Base 5898397a657e9e1df162d2ddc48dbdf6a748d1c0, head 40c519b3ad5a65454891bd150aa443c3b74fb333.
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria fetch origin
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria worktree add /tmp/chromeria-561-r6 40c519b3ad
pnpm install --offline --frozen-lockfile --ignore-scripts
# Exit 0. Dependencies only in my detached review worktree.
# All test/typecheck commands used /opt/homebrew/opt/node@24/bin first on PATH.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# Exit 0: 23 files passed, 1 skipped; 536 tests passed, 1 skipped; 11.37s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R4.test.ts apps/server/src/preview/Review561R5.test.ts
# Exit 0: 14 adapted prior probes passed; 12.36s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R6.test.ts
# Final exit 0: 3 counterexample probes passed; 7.09s.
# These assert the three vulnerabilities above, not privacy success.
# The first two also reproduced in two earlier successful runs.
# Disposable test files were removed before these candidate-only checks.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# All exit 0, no TS errors; existing Effect suggestions were printed.
git diff 5898397a65 HEAD --check
# Exit 0.
gh pr checks 561 --repo toolboxmd/chromeria
gh run view 39343258453 --repo toolboxmd/chromeria --json headSha,status,conclusion
gh api repos/toolboxmd/chromeria/actions/jobs/118369714183/logs --allow-escape-sequences
gh api repos/toolboxmd/chromeria/actions/jobs/118369651413/logs
# Main CI completed/success on exact head; read the passing Chromium shard and failed labeller logs.Node v24.18.0, pnpm 11.10.0, cached Playwright Chromium headless shell 1223 on macOS. Initial dependency installation used ambient Node v26 and printed its engine warning; proof used Node 24. vp performed its own workspace dependency preparation before testing. The first new desktop probe could not resolve a direct ws import; I pointed the disposable probe at the already installed package. The new popup-metadata probe initially compared two equivalent URL escape spellings; I corrected its independent oracle to decode the query and compare the complete value, then it passed. Neither correction changed candidate code or privacy expectations. No new baseline mutation/red-green cycle is claimed.
Probe sources, fake-value observations and logs remain outside tracked source under /tmp/chromeria-561-r6-*. The review worktree was removed after proof; the canonical checkout stays clean at its starting d74c023f7816d12282e5cb22fed83db7bc1618b4. No canonical source edit, push, merge, real-vault access, account login or machine configuration change occurred.
Not checked: real vault/rbw/wrapper execution and subprocess timeout/kill behavior, live Shopify/account login, full Electron UI/mobile/web-client/relay-to-another-environment flows, exhaustive concurrency/target-alias schedules, or repo-wide local checks. The new desktop probe uses production CDP connection and relay code backed by Chromium, but substitutes loopback endpoints for Electron's actual debugger channel. Focused local proof is not the complete merge gate; GitHub CI supplies the broader checks described above.
Verdict: request changes, review/independent=failure on this exact SHA. Posted as a comment review because the authenticated GitHub account is also the PR author and cannot formally request changes on its own PR.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
…ins on end Review round 6 on #561. Simplify by scope. Fills run only in server browser tabs; a desktop-hosted tab refuses with serverBrowserOnly, so the desktop quarantine plumbing (session and CDP target records) is gone. A login is keyed by profile id, not by tab, context object or opener: a fill puts the whole profile in it, every existing tab's session state is republished as "Login in progress" at once, and every tab created or still attaching there joins it. A popup in a profile with a login publishes no URL. preview_end_login marks the profile ending and closes its tabs; a popup that finishes attaching while it ends is closed, and the login lifts only when the profile has no tabs and no popup still attaching. Refs #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Independent review round 7: request changes on 9131aa6639d252b71f654c050a7bd0e1e38bde5a; the profile quarantine still lifts before its browser pages have drained.
Why: The drain check counts published tabs and pending popups, but misses ordinary pending attachments and forgets closing pages before their actual close finishes.
So what: The implementation owner should fix the two reproduced drain schedules below and obtain another exact-head review before merge; the demonstrated round-6 schedules and ordinary sign-in pass.
Elon record
Requirements and who asked: The owner requested defensive pre-merge review of #557/#561, using fake values only: server-browser-only fills, profile-id quarantine across existing/attaching/future tabs, redacted published state, no early release, earlier regressions, three-step sign-in/cookie reuse, other agents' paused tabs, and completed exact-head CI. This protects account sign-in on the Objective's agent working surface.
Deleted: No implementation changes, replacement architecture, real vault/account access, push or merge; no revived scrubber/encoding requirements or findings against the accepted matching-oracle, human-live-view, incognito, saved-download and split-code limits.
Bottleneck: The quarantine's lifetime must follow actual browser pages and all attachments, rather than only the published tabs registry.
Checked myself: Full PR diff/body, redesign comment, full round-6 review and its probe source, earlier review bodies; 538 focused tests, 14 adapted prior probes, six additional real-Chromium probes, three candidate-only typechecks, completed exact-head CI and the actual Linux Chromium-shard log. Two probes reproduce the drain defect, three verify the revised protection boundaries, and one clears a late-action-error hypothesis.
Blocking finding
P1: Profile quarantine can lift before all pages and attachments have drained
Drain check: ServerBrowser.ts:530.
Early removal: ServerBrowser.ts:744, asynchronous page close at 753.
Ordinary pending attachments: ServerBrowser.ts:1316; only popups enter attaching at 1267.
Two independently reproduced schedules violate the same drain invariant:
-
A closing page remains alive after the login has been removed. Before filling A in persistent profile
default, the fake site installs a normalBroadcastChannellistener that answers with its password input. Fill A, hold the realPage.closeAPI boundary, and call productionendLogin.dropTabremoves A and callsliftIfDrainedbefore starting/awaiting its page close. End-login returnsclosed: [A], althoughA.isClosed()is still false. Open fresh B in that profile: its status has nologinInProgress, and production evaluate can request the channel response and return a character array reconstructing the entire fake password from still-live A. This does not rely on persisted localStorage, a real vault, a password in the evaluate request, or a matching oracle. The promise gate makes the close-latency schedule deterministic; it does not establish its uncontrolled frequency. The close is released and the real page closes during cleanup. The newendingflag cannot help after its profile entry has already been deleted. -
A normal tab already attaching before end-login survives and publishes unlocked. Fill A, start
preview_openfor B in the samedefaultprofile, and hold B's realcontext.newCDPSessionbefore publication. End A's login, then release attachment. B is inpendingTabs, but not inattaching, so the drain check deletes the profile record. B's publication-timeendingcheck now sees nothing, the pending open succeeds, B stays open, and evaluate succeeds with no login flag. In the fixture, evaluate returns the complete fake value copied by A's input listener to shared site storage. B was initially about:blank until its setup completed; the storage echo alone is not a claim that a legitimate fresh tab must erase all site-persisted data. The defect is that a pre-end pending tab survives the required close and the profile lifts while that attachment still exists.
These are two paths through one incomplete lifetime accounting rule. Preserve quarantine through actual page closure and every pending server-profile attachment; closing the Manager session or removing a published ServerTab is not proof that its page has gone. No candidate code was changed.
Requested answers and prior findings
- The three demonstrated round-6 schedules are closed on this head. The held secret-bearing popup closes after end-login and cannot be evaluated. Desktop fill refuses with fixed
serverBrowserOnlybefore typing, including after same-document detach/reattach, so the old separate-CDP-wrapper fill premise is removed. A popup from a pre-existing other tab in the same server profile publishes no secret URL, while both existing tabs are already quarantined; Manager events/list state and MCP status are content-free. The broader end-login claim is still false for the two schedules above. - Earlier demonstrated schedules stay closed. All 14 adapted round-4/5 probes pass: success metadata and actual MCP audit use the vault host; held status is redacted; failed follow-up and failed-first/concurrent-success fills stay locked; popup publication after a fill and ordinary opener-close during popup attach stay protected; origin-bearing refusals do not reach the audit. The obsolete desktop successful-fill probe was adapted to assert refusal and empty input before/after reattachment. The closed-opener churn probe still refuses.
- Rounds 1-3's old scrubber, entity-decoder and lazy-release mechanisms are gone. The prior integrated probes still refuse all 13 disallowed operations, including text/pixel snapshot, evaluate, readPage, recording and coordinate clicks, across a multi-tab chain. Bridge closure, navigation, failed post-typing fills, dialog dismissal and downloads retain the ordinary quarantine, and recording/fill exclusion passes in both tested start orders. This does not establish absence of every lifecycle escape, as the P1 shows.
- Can a value still reach an agent result or quarantine lift early? Yes. The still-live-page channel probe returns the full fake password through production broker evaluate after early release. The normal pending-tab probe also demonstrates early release. I found no new independent audit leak or retained vault-value store in the fill/vault code. Page console/network buffers and site storage naturally contain echoes; their secrecy depends on the lifetime guard. Normal fixed errors and tested audit lines remain content-free. A separate probe held a real Playwright click timeout whose call log contains the fake password until after end-login; the returned error still excludes it because the general conversion keeps only the first line, so that hypothesis is cleared.
- Three-step sign-in and fresh-tab cookie reuse pass twice. Both the candidate integration fixture and independent fixture complete email -> password -> single-input OTP using fills, locator clicks and waits, then end-login; a fresh explicitly selected persistent-profile tab sends the signed-in cookie and reads the account page. The independent fixture also takes a normal fresh-tab screenshot. This is fake-site fixture proof, not live Shopify proof.
- Other agents' paused tabs behave cleanly in the tested settled-lock case. A second thread/session in the same profile receives fixed
loginInProgressfor snapshot, evaluate, navigation and recording; status has no URL/title, and end-login closes both threads' tabs. These probes complete without a hung request. The user guide and tool descriptions explain server-only scope, profile-wide pause, failed fills and closure/cookie reuse. In-flight operations and every possible human-control/attachment ordering were not exhaustively tested. - PR body: read twice while reviewing; both fetched bodies still describe/prove round-5/6 head
40c519b3adand the older opener-group/context model. I used the owner's round-7 instructions and candidate code as this round's acceptance, rather than treating those stale body claims as exact-head evidence. The author is already updating it.
CI
Waited for completion and checked gh pr checks 561 --repo toolboxmd/chromeria and run identity. CI run 39345636110 is completed/success on exact head 9131aa6639d252b71f654c050a7bd0e1e38bde5a: aggregate Check, Build and Typecheck, Lint, Test, Test Web, all eight server shards and Release Smoke pass. Fork Stack Model, Native fingerprint diff and both labellers also pass; Rust, Mobile Native Static Analysis and preview jobs are skipped. The size labeller is not a code finding. I read the actual Test Server 1 log: all 13 CredentialLogin.integration tests pass, with 834 passed and 10 skipped in the shard. Independent review remains failure because of the P1 lifetime defect above.
Commands and results
Base 5898397a657e9e1df162d2ddc48dbdf6a748d1c0, reviewed head 9131aa6639d252b71f654c050a7bd0e1e38bde5a. Read gh pr view 561 --repo toolboxmd/chromeria, gh pr diff 561 --repo toolboxmd/chromeria, review 5579139789 in full with its three probes, earlier review bodies, and design comment 6278773329 using gh api.
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria fetch origin
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria worktree add /tmp/chromeria-561-r7 9131aa6639
pnpm install --offline --frozen-lockfile --ignore-scripts
# Exit 0, dependencies in my detached review worktree only.
# Proof commands used /opt/homebrew/opt/node@24/bin first on PATH.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# Exit 0: 23 files passed, 1 skipped; 538 tests passed, 1 skipped; 11.84s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R4.test.ts apps/server/src/preview/Review561R5.test.ts
# Exit 0: 14 adapted prior probes passed; 12.48s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R7.test.ts
# Final exit 0: six probes passed; 8.27s.
# Two assert the observed drain defects, not privacy success.
# Disposable test files removed before these candidate-only checks.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# All exit 0; existing Effect suggestions were printed.
git diff 5898397a657e9e1df162d2ddc48dbdf6a748d1c0 HEAD --check
# Exit 0; review-worktree tracked state clean.
gh pr checks 561 --repo toolboxmd/chromeria
gh run view 39345636110 --repo toolboxmd/chromeria --json headSha,status,conclusion
gh api repos/toolboxmd/chromeria/actions/jobs/118377483639/logs --allow-escape-sequences
# The actual Linux log runs all 13 CredentialLogin.integration tests successfully:
# Test Server 1: 834 passed, 10 skipped.
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria worktree remove /tmp/chromeria-561-r7
# Exit 0; path absent from Git worktree list, canonical checkout clean at original HEAD.Node v24.18.0, pnpm 11.10.0, cached Chromium headless shell 1223 on macOS. Installation initially used ambient Node v26 and printed its engine warning; proof used Node 24. The initial status mutation used the short SHA and GitHub rejected it; the full SHA succeeded. The worktree command was initially blocked until I loaded the implementation reference, then succeeded. The first late-error probe expected exposure but the real error conversion disproved it; the final test checks that safe result. One popup-state oracle initially required an already-published redacted Success state while its safe initial Idle state was still current; I permitted that content-free initial state and retained the secret-absence, existing-tab re-publication and actual popup-read refusal assertions. T3's watch tool refused because this is a child thread; the checks were verified completed before posting. Candidate code and tracked tests were never edited. No new baseline mutation/red-green cycle is claimed.
Not checked: real vault/rbw/wrapper execution and subprocess timeout/kill behavior; live Shopify/account login; full Electron UI, web/mobile client, remote relay or another-environment flows; exhaustive concurrency, close-failure, human-control, or attachment schedules; repo-wide local checks. Focused proof is not the complete merge gate. The known human-live-view, saved-download and site-persistence limits are unchanged. No real vault, production settings, push, merge, or installation was accessed or performed.
Probe sources, fake-value observations and logs remain outside tracked source under /tmp/chromeria-561-r7-*. My review worktree was removed and its path is absent from Git's worktree list. The canonical checkout remains clean at its starting d74c023f7816d12282e5cb22fed83db7bc1618b4. Pre-existing worktrees were preserved.
Verdict: request changes, review/independent=failure on the exact reviewed SHA. Posted as a comment review because the authenticated account is also the PR author and GitHub does not permit its own formal changes-requested review.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
Review round 7 on #561. Remove the counting instead of refining it: preview_end_login closes the profile's whole browser context and waits for the close to finish, and the login lifts only then. Every page and every tab still attaching goes with the context, so no per-page close tracking or pending-attach counter decides anything; the popup attach holds and drain check are deleted. While the context closes, the profile's tabs refuse reads, and a new tab request waits for the close, then opens a fresh context from the same profile directory, signed in through cookies with an expiry. Any other close of the context also ends its login. Refs #557 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
lukemaj
left a comment
lukemaj
left a comment
There was a problem hiding this comment.
What: Independent review round 8 passes on f2613fbe88216d6ea378e84a687aa501d8cd2f98, with no supported blocking finding in the inspected scope.
Why: Whole-context closure removes both demonstrated round-7 lifetime escapes, and the adapted earlier privacy probes still pass.
So what: The implementation owner can use this exact-head review for the independent-review gate; the separately planned live account proof and human merge approval remain outstanding.
Elon record
Requirements and who asked: The owner requested defensive pre-merge review of #557/#561 using fake values only, against the planner's server-browser-only, profile-id quarantine and whole-context-close decision: verify both round-7 schedules, prior findings, output privacy, close/request ordering, cookie reuse, other agents' refusals and completed exact-head CI. This supports protected sign-in on the Objective's agent working surface.
Deleted: No implementation changes, replacement design, per-page drain/attach-count requirement, real vault/account access, push or merge; no revival of the removed scrubber or findings against accepted matching-oracle, human-live-view, site-persistence, incognito, saved-download and split-code limits.
Bottleneck: Whether the old browser context can survive release, or a new request can bypass the close barrier.
Checked myself: Full PR diff/body, design comment, seven prior review bodies and round-7 probe source; 540 focused tests; 22 independent real-Chromium probes; three candidate-only typechecks; exact-head CI and the actual Linux Chromium-shard log. Both round-7 schedules and two additional close/request schedules pass.
Findings by severity
No supported P0, P1 or P2 finding remains from this review. This is a bounded review result, not proof of absence of all defects.
Round-7 P1: closed in both demonstrated schedules
Relevant implementation: ServerBrowser.ts:1962-1983, ServerBrowserContexts.ts:237-253, attachment publication at ServerBrowser.ts:964-975.
- Closing page latency: The original probe still holds the real
Page.closeboundary and installs a BroadcastChannel responder before filling. ProductionendLoginnow closes the context, so the original page is alreadyisClosed() === truewhen end-login returns, despite the held page-close call. The fresh tab uses a different context and receives no channel response. No old page remains to return the fake password. - Ordinary pending attachment: Start a same-profile ordinary open, hold its real
newCDPSessionboundary, then end the login. The pending page and filled parent both close with the context. Releasing attachment produces a content-free failed open, rather than a surviving readable old page; a subsequent open uses a different context and evaluates normally.
Neither proof relies on page counts, attach counters or deletion from the published-tab registry deciding release. The probes preserve production browser/Manager/broker code and pause only the named real API boundaries; frequency under uncontrolled scheduling is not measured.
Close ordering and other agents
- Hold context closure before the real close, and also hold both published pages' individual close calls so the old pages genuinely remain alive. A second thread/session's snapshot, evaluate, navigate, recordingStart, locator click, credential fill and waitFor all finish with content-free
PreviewAutomationTabNotFoundErrorwhile the close gate is still held. End-login has closed their Manager sessions, so these requests do not enter the old context or wait for that gate. - Start three separate agent-session opens in that profile while the gate is held. All reach context lookup, none completes before release, and end-login also remains pending. After release, both old pages are closed and all three opens complete in one shared fresh context, with no login flag. No duplicate persistent-context launch or profile-directory lock failure occurred in this tested concurrent schedule.
- Separately hold the close promise after the real browser close has completed and its close event has fired. The new open and end-login still wait for that promise. The implementation clears the login record in its context-close callback, but by then the old pages are dead; the
clearingbarrier still prevents relaunch until the close promise resolves. Thus the callback timing does not expose the round-7 read or relaunch escape in the tested path. - A secret-bearing popup held during attachment also closes with the context. Reads after its pending attachment settles do not return the old value. The tested pre-existing other-tab popup publishes only content-free Manager events/state and status.
Earlier findings and output boundaries
- All 14 adapted round-4/5 probes pass: held status revalidation, success metadata and actual MCP audit using the vault host, failed follow-up/failed-first and concurrent-success fills retaining quarantine, publication after a fill begins, ordinary opener close during popup attach, closed-opener churn, and content-free origin-bearing refusals. Desktop fill refuses before typing and after same-document reattachment, preserving the accepted server-only scope.
- The former scrubber, HTML/entity decoder, lazy-release and group-transfer machinery from rounds 1-3 is absent. The integrated prior probe refuses all 13 disallowed operations across a multi-tab chain, including text/pixel snapshot, evaluate, readPage, recording and coordinate clicks. Closing a bridge, navigation, failed post-typing fills, dialogs and downloads preserve the ordinary lock; real recording/fill exclusion passes in both tested start orders.
- Normal fill outcomes, actual MCP success/error responses and audit lines contain no fake value or live page origin in the tested paths. Manager events/list state and status remain redacted during the tested profile/popup schedules. A real Playwright click timeout whose internal call log contains the fake value still produces a content-free late error after end-login through first-line conversion.
- I found no deliberate long-lived vault-value store in the fill/vault implementation and no remaining demonstrated result/audit/renderer-state leak within the inspected scope. Page console/network buffers and site storage can naturally retain echoes; this review does not claim those are erased from the persistent profile or saved files. The accepted human live view and matching oracles remain.
Sign-in and cookie reuse
The candidate integration test and independent prior fixture both complete email -> password -> single-input OTP using credential fills, locator clicks and waits, then end-login. A fresh persistent-profile tab is signed in and the independent fixture takes a normal screenshot. I adapted the independent fixture's cookie to Max-Age=3600: an expiring cookie survives the browser restart, whereas a session-only cookie does not. The changed tool description and user guide disclose this limitation, including incognito session loss. This is fake-site proof, not live Shopify proof.
The PR body was read at the start and again after proof. Both fetched versions still describe the older tab/attachment drain model and cite 9131aa6639; I used the owner's explicit round-8 decision and this candidate's code as acceptance. Its author is already updating that description; no exact-head proof is borrowed from its old claims.
CI
Waited for completion. CI run 39347782218 is completed/success on exact head f2613fbe88216d6ea378e84a687aa501d8cd2f98: aggregate Check, Build and Typecheck, Lint, Test, Test Web, all eight server shards and Release Smoke pass. Fork Stack Model, Native fingerprint diff and both labellers pass; the listed Rust, Mobile Native Static Analysis and deployment-preview jobs are skipped. The unrelated size labeller is not a code finding. I read the actual Test Server 1 log: all 15 CredentialLogin.integration tests pass, with 836 passed, 10 skipped in that shard.
Commands and results
Base 5898397a657e9e1df162d2ddc48dbdf6a748d1c0, reviewed head f2613fbe88216d6ea378e84a687aa501d8cd2f98. All local proof used Node v24.18.0, pnpm 11.10.0 and cached Playwright Chromium headless shell 1223 on macOS, with /opt/homebrew/opt/node@24/bin first on PATH.
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria fetch origin
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria worktree add /tmp/chromeria-561-r8 f2613fbe88
pnpm install --offline --frozen-lockfile --ignore-scripts
# Exit 0; dependencies only in my detached review worktree.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts
# Exit 0: 23 files passed, 1 skipped; 540 tests passed, 1 skipped; 13.37s.
PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview/Review561R4.test.ts apps/server/src/preview/Review561R5.test.ts apps/server/src/preview/Review561R8.test.ts
# Final exit 0: 3 files, 22 independent probes passed; 13.27s.
# Disposable probes removed before candidate-only checks.
pnpm exec tsc --noEmit -p apps/server
pnpm exec tsc --noEmit -p packages/contracts
pnpm exec tsc --noEmit -p packages/shared
# All exit 0; existing Effect suggestions were printed, no TS errors.
git diff 5898397a657e9e1df162d2ddc48dbdf6a748d1c0 HEAD --check
git status --short
# Exit 0, no whitespace errors; tracked/untracked worktree state clean.
gh pr view 561 --repo toolboxmd/chromeria
gh pr diff 561 --repo toolboxmd/chromeria
gh api repos/toolboxmd/chromeria/pulls/561/reviews/5579292619
gh api repos/toolboxmd/chromeria/issues/comments/6278773329
gh pr checks 561 --repo toolboxmd/chromeria
gh run watch 39347782218 --repo toolboxmd/chromeria --exit-status --interval 30
gh run view 39347782218 --repo toolboxmd/chromeria --json headSha,status,conclusion
gh api repos/toolboxmd/chromeria/actions/jobs/118384563008/logs --allow-escape-sequences
# Browser shard: all 15 CredentialLogin.integration tests pass; 836 passed, 10 skipped.
git -C /Users/lukaszmaj/dev/toolboxmd/chromeria worktree remove /tmp/chromeria-561-r8
# Exit 0; path absent from Git worktree list, canonical checkout clean at original HEAD.Probe corrections: the initial adapted popup probe subscribed to close after end-login, when this candidate had already closed the page, so its test-only event wait hung; I stopped that run and asserted the already-closed page instead. The first two new close-request probes accidentally wrapped a mutable spy in itself and failed with Maximum call stack size exceeded; capturing its implementation and restoring that implementation fixed the probe. The final combined 22-probe run passes. T3's watch tool returned This thread is a subagent, so it cannot watch pull requests; I waited for the CI workflow with gh run watch instead. Candidate source and tracked tests were never edited. No new baseline mutation/red-green cycle is claimed. vp performed workspace dependency preparation and printed missing-bin/peer warnings, then ran successfully.
Limits and cleanup
Not checked: real vault/rbw/wrapper execution or subprocess timeout/kill behavior; live Shopify/account login; full Electron UI, web/mobile clients, remote relay or cross-environment execution; exhaustive close-failure, overlapping end-login, human-control or lifecycle schedules; repo-wide local checks. Focused local proof is not the complete merge gate. Only fake vault values and isolated HTTPS fixtures were used.
The review worktree was removed and its path is absent from Git's worktree list. The canonical checkout remains clean at its starting d74c023f7816d12282e5cb22fed83db7bc1618b4; pre-existing worktrees were preserved. Probe sources, fake-value observations and logs remain outside tracked source under /tmp/chromeria-561-r8-*. No canonical source edit, push, merge, real-vault access, account login, production setting or machine configuration change occurred.
Verdict: pass, no blocking findings in the inspected scope, review/independent=success on the exact reviewed SHA. Posted as a comment review because the authenticated account is also the PR author and GitHub cannot accept its formal self-approval.
Reviewer: GPT-6.1 Sol, high effort, Codex harness in T3 Code.
What changed: Agents sign in from the environment's rbw vault with
preview_fill_credentialin server browser tabs. A fill puts the whole browser profile in a login: no tab in it can be read untilpreview_end_logincloses the profile's browser context and that close has finished, and no result or audit line carries page data.Why: The only way to sign an agent in was
preview_type, which writes the password into the agent's session log (#557). Round 7 showed that counting pages and attachments let the login lift early; closing the context removes the counting.So what: CI is green and independent review round 8 passed on
f2613fbe88. Next are the planner's live admin.shopify.com proof on cavallo and the user's merge approval.How it works
fillCredentialandendLogin.{filled, item, field, site, tabId}, wheresiteis the matched vault host. End-login result{closed}.PreviewCredentialErrorhas a fixedreasonand thetabId, never an origin.loginInProgress.credentialVault: {kind:"none"} | {kind:"local", rbwPath}(default none, settings.json only).preview/CredentialVault.tsspawnsrbwPathwith an argument array (shell: false, stdin ignored), runsrbw unlockedfirst, thenrbw get --field uris|username|password -- <item>orrbw code -- <item>. Exit 69 and spawn failures map tovaultUnavailable; stdout is never logged. It refuses whileDEBUGenablespw:protocolorpw:api.preview/CredentialFill.tsmatches the input frame's live https host to the item's hosts, reads the value, re-checks the origin, types, verifies and drops the value. Refusals carry only reason and tab.serverBrowserOnly.ServerBrowser.ts), keyed by profile id:profile:<id>, an isolated context's own key for it and its popups, or a separate key per desktop tab.Login in progress.preview_end_loginmarks the login ending, closes the profile's Manager sessions, and callsServerBrowserContexts.closeContext, which closes the browser context and waits.docs/user/browser-import.md: profile-wide pause, keep logins short, end-login closes the profile's browser, session cookies need a new sign-in. Fork registry listsServerBrowserContexts.tsas shared.Elon record
dropTab, and the lift-when-drained logic.Review round 7: what changed per finding
BroadcastChannelwith its password input; hold the context's realcloseduring end-login. The tab refuses reads while the close is held, and a new tab request waits. After release, end-login returns, the old page is closed, and the fresh tab's channel request gets no answer.preview_openin the same profile, hold its CDP attach, end the login, release. The open fails and its page is closed.Proof (head
f2613fbe88, base5898397a65)PLAYWRIGHT_BROWSERS_PATH=/tmp/pw-browsers pnpm exec vp test run apps/server/src/preview apps/server/src/mcp/toolkits/preview apps/server/src/mcp/McpHttpServer.test.ts apps/server/src/mcp/PreviewAutomationBroker.test.ts apps/server/src/serverSettings.test.ts packages/contracts/src/settings.test.ts packages/shared/src/t3McpToolPresentation.test.ts: 23 files passed, 1 skipped; 540 tests passed, 1 skipped.apps/server,packages/contracts,packages/shared,apps/web,apps/mobile), lint on changed files,pnpm run knip:check(exit 0),node scripts/fork-features.mjs check,scripts/fork-check.sh,git diff --check origin/main: clean.f2613fbe88: every job passes. That includesCheck,Lint,Build and Typecheck,Test,Test Web, all 8 server shards (Test Server 1 ran the 15 real-Chromium tests: 15 passed),Release Smoke,Fork Stack Model,Native fingerprint diff, and both labellers.review/independent=successonf2613fbe88(pullrequestreview-5579427710).rbw getorrbw codeagainst the real vault.Website impact: none; Chromeria has no product website. The user guide section is the documentation change.
Deviations
protocolLogging,fillFailed,loginInProgress,serverBrowserOnly.site(matched vault host).preview_dialogstays allowed with redacted status; coordinate clicks are refused.Open risks
preview_end_login, because the context relaunches; such a site needs a new sign-in. The live Shopify proof will show whether Shopify's admin session survives.Model and harness: Claude Opus 5.5 in Claude Code (T3 Code).
🤖 Generated with Claude Code