Repository navigation
fix(tool-server): keep Chromium fill within field limits and report dialogs and cross-origin frames - #1487
fix(tool-server): keep Chromium fill within field limits and report dialogs and cross-origin frames#1487latekvo wants to merge 8 commits into
Conversation
…ialogs and cross-origin frames - Refuse text over a maxlength, and text a number or email input would rewrite, with FILL_UNSUPPORTED_FIELD before touching the field. The value-setter fallback only runs when the page left the old text, so it no longer overrides the browser's own rewrite of an edit. - Send a no-op first: a page blocked by a JavaScript dialog gets nothing that writes and fails with FILL_NOT_VERIFIED (timeout) instead of the generic CDP timeout; a fill that times out midway says the field may still change. - Focus in a cross-origin or sandboxed frame is FILL_UNSUPPORTED_FIELD, not FILL_NO_FOCUSED_FIELD; a <frame> is followed like an <iframe>. - Tool description and the tools reference list the new cases.
…ect> frames - The value-setter fallback also requires that the edit fired no input event, so a browser rewrite that equals the old text (an IDN email on a retry) is no longer forced over. - A textarea's maxlength check counts CRLF and CR as the one LF it stores. - A same-origin <object> document is followed like an <iframe>; a cross-origin one is FILL_UNSUPPORTED_FIELD; an <object> with no document (an image) stays FILL_NO_FOCUSED_FIELD. - The page-not-answering error also names a crashed page and says when to restart the app. - Docs and the tool description name the number and email cases exactly. - Tests pin search, url, tel and email in the maxlength type list.
latekvo
left a comment
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: Review of 4024a90: claims vs code, nearest twin, non-happy paths, inputs, reachability, an absence pass and 40 mutations. Every comment below was reproduced with headless Chrome 155 and the PR's tool-server (POST /tools/fill), with fields read back over raw CDP and compared against real typing (CDP Input.insertText) or main.
…G-hosted fields - Building the CDP service under a JavaScript dialog times out on its read-only setup; fill now reports that as FILL_NOT_VERIFIED "changed nothing", like the no-op probe, instead of the generic CDP timeout. - execCommand throws in an SVG document; a text control there falls back to the value setter instead of the generic "could not run" error. - The CRLF count matches CRLF only (a lone CR keeps its length). - Tests pin the unchanged-text guard, both sides of the CRLF count, and the SVG fallback for controls and contenteditables.
…romium fill - Compare the read-back against the text with CR and CRLF stored as LF, as a textarea or editable element stores them, so such text verifies. - Clear a number input that shows text it cannot parse, and do not verify it while it still does. - Watch for the edit's input in the window's capture phase, so a page listener that stops it cannot let the setter override the page's rewrite. - Refuse focus in an <embed>, which fill cannot reach, instead of asking to tap the field again. - Tell the caller to ask the user to close a dialog, which argent cannot do, and note connection errors beside the fill codes in the docs. - Pin maxlength 0, single-address email probing and the untouched selection of a refused fill in tests.
latekvo
left a comment
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: Review of cfff563: the six review moves, the absence pass and about 30 mutations, each checked with headless Chrome 155 through the PR's tool-server. Fields were read back over raw CDP and compared against real typing (CDP Input.insertText) and the merge-base build 91542bd. All comments are low severity. A focused field inside a closed shadow root gets the same looping "tap first" answer and cannot be reached from the page. It is outside this diff and filed as #1662.
…ocuments in Chromium fill In an SVG or XML document createElement makes an element in no namespace, so the probe never refused and the value setter fallback wiped a number field or trimmed an email one. Also: restart advice when a page crashes mid-fill, the frame-window test the input listener lacked, and docs and a comment that overstated what fill covers.
latekvo
left a comment
There was a problem hiding this comment.
[Diplomat, Opus 5.5]: Review of 1f17de3: the six review moves, an absence pass and about 60 mutations, each checked with headless Chrome 155 through the PR's tool-server (POST /tools/fill). Fields were read back over raw CDP and compared against real typing (CDP Input.insertText) and the merge-base build 0064663. Every row of the description's table was re-run and matches. All comments below are low severity. Two problems outside this diff are filed: #1746 (a trailing or double space in a contenteditable never verifies, since Chrome stores it as ) and #1747 (fill "" reports an image-only contenteditable as cleared and leaves the image).
fillon Chromium (#1444) forced text past field limits and answeredverified: true, wiped a number input it could not fill, failed under a JS alert with the generic "restart the app" CDP timeout and then wrote the field when the alert closed, and told the caller to "tap the field first" for focus in a cross-origin frame, which loops.Fix
FILL_UNSUPPORTED_FIELDbefore anything changes: overmaxlength(only the input types it applies to; a textarea's CRLF counts as one), or text a number or email input's own value rules rewrite (checked on a detached XHTML input of the same type, also in an SVG or XML document).input(watched in the window's capture phase; only a page listener added there earlier can hide it) and left the old text (a refused edit), never over the browser's or the page's own rewrite, even one equal to the old text.FILL_NOT_VERIFIED/timeout, as on iOS and Android.execCommandthrows, a text control gets the value setter.<embed>, isFILL_UNSUPPORTED_FIELD. A<frame>or same-origin<object>is followed like an<iframe>.fillfootnote inreference/tools.mdxlist the cases. Android is unchanged.maxlength=3field, fillabcdefverified: true, fieldabcdefFILL_UNSUPPORTED_FIELD, field unchanged42, fillabcFILL_NOT_VERIFIED, field""FILL_UNSUPPORTED_FIELD, field4242in an SVG or XML document, fillabc42FILL_UNSUPPORTED_FIELD, field42a@b.coFILL_NOT_VERIFIED, fielda@b.coFILL_UNSUPPORTED_FIELD, field unchangeda@ąę.pl(typing givesa@xn--2da4b.pl)verified: true, fielda@ąę.plFILL_NOT_VERIFIED, fielda@xn--2da4b.pla@xn--2da4b.plverified: true, fielda@ąę.pl(invalid)FILL_NOT_VERIFIED, field unchangedalert()open, filldlgDEBUGGER_CDP_REQUEST_TIMEOUT; fielddlgonce the alert closesFILL_NOT_VERIFIED"changed nothing"; field stays""DEBUGGER_CDP_REQUEST_TIMEOUTFILL_NOT_VERIFIED"changed nothing"inputhandler opens an alertDEBUGGER_CDP_REQUEST_TIMEOUTFILL_NOT_VERIFIED"may still change when the page resumes"FILL_NO_FOCUSED_FIELDFILL_UNSUPPORTED_FIELD<frame>of a frameset, or a same-origin<object>FILL_NO_FOCUSED_FIELDverified: true<embed>FILL_NO_FOCUSED_FIELDFILL_UNSUPPORTED_FIELDa\r\nbFILL_NOT_VERIFIED, fielda\nbverified: true, fielda\nb1., fill""verified: true, field still shows1.verified: true, field empty12with a mask that stopsinputand keeps digits, fill12averified: true, field12aFILL_NOT_VERIFIED, field12Repro and E2E (before = origin/main 91542bd, after = this branch)
Both bundles built from this worktree (
npm run build -w @swmansion/argent), driven throughargent run <tool>under a sandbox HOME. Every field read back over raw CDP, not argent. Error codes from the tool-server event log. Each build ran 2+ times on Electron 148 and on real Chrome 155 (port 9383): every run of a build matched its other runs, and Electron matched Chrome. The retry,<object>and first-call rows, and the controls below, were re-run on headless Chrome 155 after the follow-up commits.Unchanged controls (both builds): plain input, text at exactly
maxlength, clearing amaxlengthfield, valid number, clearing a number,type=number maxlength=2with123(maxlength does not apply), valid email, url with surrounding spaces (typed input keeps them), password at its limit, a page whoseexecCommandrefuses (setter fallback still fills it), same-origin iframe,keyboardinto the cross-origin frame.Browser semantics checked first over raw CDP: maxlength counts UTF-16 units (
😀😀into 3 leaves😀);select()andinsertTextwork ontype=number; numberabc/+5/1,5and email whitespace are rewritten typed and set alike, url whitespace only when set; analert()blocks everyRuntime.evaluateand a latePage.enabledoes not report the open dialog (Electron), hence the no-op probe instead of dialog events.Tests and local CI
fill-tool.test.ts+fill-chromium-script.test.ts, 104 tests). Mutations of each new rule (each type in the maxlength list, CRLF count and read-back,maxlength=0, single-address email, capture-phase listener, numberbadInput,<embed>,>vs>=, probetype/multipleand namespace, email probe, the frame's own window,<frame>,<object>and itscontentWindowcheck, unchanged-text andinputguards, listener removal, SVG fallback, connect and timeout mapping, no-op probe, rethrow) each fail at least one test.e2e-tool-aliases(81 vs 83), which fails the same way locally onmainand passes in CI; argent, argent-tools-client, registry suites andtest:scriptspass.npm run build,typecheck:tests,typecheck:scripts,prettier --check ., eslint (run per package), knip on an unbuilt tree, docstypecheckanddocusaurus build, and the SpiderShield description gate (9.08 / 9.0) pass.Left out
chromium-storageset/remove/clear(and the Chromium clipboard route) write throughRuntime.evaluatethe same way: under an open alertchromium-storage setfails after 10 s withDEBUGGER_CDP_REQUEST_TIMEOUTand the value lands once the alert closes (2/2 runs). That predatesfill, has its own error contract and wants a CDP-layer fix, so it is not changed here (#1562). A dialog that is already open before the first Chromium call still waits out the 60 s service build every Chromium tool shares (#894, #1476).