From 425328e36126b1411b25fd100f965a3c33e315f6 Mon Sep 17 00:00:00 2001 From: Liohtml <158847046+Liohtml@users.noreply.github.com> Date: Sun, 12 Jul 2026 14:35:24 +0200 Subject: [PATCH 1/3] fix: release every held key and the mouse button on input failures #2347 fixed one instance of the unpaired-input defect in this file. I audited the rest of the input tools afterwards and found more, all with the same shape: a key or button is pressed down, and the release is only reachable on the success path. - press_key left the *main* key held. Puppeteer's Keyboard.press() is a bare down() followed by up() with no guard of its own, so a rejection between the two leaves the key logically held down. #2347 only released the modifiers. - drag left the *mouse button* held. ElementHandle.drag() presses the button down and only drop() releases it (drag interception is not enabled), so a failed drop leaves the button down for the rest of the session, breaking every later click. - type_text's submitKey went through the same unguarded press(). - The release loop added in #2347 was itself unguarded: a failing up() aborted the remaining releases and replaced the original error. Adds pressKeyReleasingHeldKeys(), which releases every key it pressed even if a key event rejects, guarding each release so one failure can neither abort the others nor mask the original error. Keys whose down() rejected are released too: Keyboard.down() latches the key on the client *before* it sends the CDP command and only up() clears it, so a latched modifier would otherwise be stamped onto every later keyboard and mouse event. Tests: a transient key up failure now leaves no key held, and a failed drop no longer leaves the mouse button down. Both fail on main. --- src/tools/input.ts | 80 ++++++++++++++++------ tests/tools/input.test.ts | 135 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 188 insertions(+), 27 deletions(-) diff --git a/src/tools/input.ts b/src/tools/input.ts index 53562eac6..a86386b4e 100644 --- a/src/tools/input.ts +++ b/src/tools/input.ts @@ -7,7 +7,7 @@ import {logger} from '../logger.js'; import type {McpContext} from '../McpContext.js'; import {zod} from '../third_party/index.js'; -import type {ElementHandle, KeyInput} from '../third_party/index.js'; +import type {ElementHandle, Keyboard, KeyInput} from '../third_party/index.js'; import type {TextSnapshotNode} from '../types.js'; import {parseKey} from '../utils/keyboard.js'; import type {WaitForEventsResult} from '../WaitForHelper.js'; @@ -33,6 +33,44 @@ const submitKeySchema = zod 'Optional key to press after typing. E.g., "Enter", "Tab", "Escape"', ); +/** + * Presses `key` while `modifiers` are held, and guarantees that every key it + * pressed down is released again — even if a key event rejects mid-sequence. + * + * Puppeteer's `Keyboard.press()` is a bare `down()` followed by `up()` with no + * guard of its own, so a rejection between the two leaves the key logically + * held down in the browser. `Keyboard.down()` also sets the client-side + * modifier bit *before* it sends the CDP command, and only `up()` clears that + * bit, so a key whose `down()` rejected still needs a matching `up()`. + */ +async function pressKeyReleasingHeldKeys( + keyboard: Keyboard, + key: KeyInput, + modifiers: KeyInput[] = [], +) { + const held: KeyInput[] = []; + try { + for (const modifier of modifiers) { + held.push(modifier); + await keyboard.down(modifier); + } + held.push(key); + await keyboard.down(key); + await keyboard.up(key); + held.pop(); + } finally { + for (const heldKey of held.toReversed()) { + try { + await keyboard.up(heldKey); + } catch (error) { + // A failed release must not abort the remaining releases, nor replace + // the original error with its own. + logger?.('failed to release key', heldKey, error); + } + } + } +} + function handleActionError(error: unknown, uid: string) { logger?.('failed to act using a locator', error); throw new Error( @@ -356,7 +394,8 @@ export const typeText = definePageTool({ const result = await page.waitForEventsAfterAction(async () => { await page.pptrPage.keyboard.type(request.params.text); if (request.params.submitKey) { - await page.pptrPage.keyboard.press( + await pressKeyReleasingHeldKeys( + page.pptrPage.keyboard, request.params.submitKey as KeyInput, ); } @@ -389,9 +428,24 @@ export const drag = definePageTool({ const toHandle = await request.page.getElementByUid(request.params.to_uid); try { const result = await request.page.waitForEventsAfterAction(async () => { - await fromHandle.drag(toHandle); - await new Promise(resolve => setTimeout(resolve, 50)); - await toHandle.drop(fromHandle); + let dropped = false; + try { + await fromHandle.drag(toHandle); + await new Promise(resolve => setTimeout(resolve, 50)); + await toHandle.drop(fromHandle); + dropped = true; + } finally { + if (!dropped) { + // `drag()` presses the mouse button down and only `drop()` releases + // it, so a failed drop leaves the button held down for the rest of + // the session, breaking every later click. + try { + await request.page.pptrPage.mouse.up(); + } catch (error) { + logger?.('failed to release the mouse button', error); + } + } + } }); response.appendResponseLine(`Successfully dragged an element`); response.attachWaitForResult(result); @@ -526,21 +580,7 @@ export const pressKey = definePageTool({ const [key, ...modifiers] = tokens; const result = await page.waitForEventsAfterAction(async () => { - const heldModifiers: KeyInput[] = []; - try { - for (const modifier of modifiers) { - await page.pptrPage.keyboard.down(modifier); - heldModifiers.push(modifier); - } - await page.pptrPage.keyboard.press(key); - } finally { - // Release every modifier that was successfully pressed, even if a - // later key event throws. Otherwise a failed press leaves modifiers - // logically held down in the browser (see #2309). - for (const modifier of heldModifiers.toReversed()) { - await page.pptrPage.keyboard.up(modifier); - } - } + await pressKeyReleasingHeldKeys(page.pptrPage.keyboard, key, modifiers); }); response.appendResponseLine( diff --git a/tests/tools/input.test.ts b/tests/tools/input.test.ts index bac71ccc9..025ff987b 100644 --- a/tests/tools/input.test.ts +++ b/tests/tools/input.test.ts @@ -14,6 +14,7 @@ import sinon from 'sinon'; import type {ParsedArguments} from '../../src/bin/chrome-devtools-mcp-cli-options.js'; import {McpResponse} from '../../src/McpResponse.js'; import {TextSnapshot} from '../../src/TextSnapshot.js'; +import type {KeyInput} from '../../src/third_party/index.js'; import { click, hover, @@ -1376,11 +1377,17 @@ describe('input', () => { context.getSelectedMcpPage(), ); - // Simulate the main key press failing mid-sequence (e.g. a CDP - // hiccup) after the modifiers have already been pressed down. + // Simulate the main key's key down failing (e.g. a CDP hiccup) after + // the modifiers have already been pressed down. + const realDown = page.keyboard.down.bind(page.keyboard); sinon - .stub(page.keyboard, 'press') - .throws(new Error('injected press failure')); + .stub(page.keyboard, 'down') + .callsFake(async (key: KeyInput): Promise => { + if (key === 'C') { + throw new Error('injected key down failure'); + } + return await realDown(key); + }); try { await assert.rejects( @@ -1399,16 +1406,130 @@ describe('input', () => { sinon.restore(); } - // The modifiers were pressed down; both must be released even though - // the main key press threw, otherwise the browser is left with the - // modifiers logically stuck down. + // The modifiers must be released even though the main key failed, + // otherwise they stay logically held down in the browser. + // + // "C" is released too, even though its key down failed: Keyboard.down() + // latches the key on the client *before* it sends the CDP command, and + // only up() clears it — a latched modifier would otherwise be stamped + // onto every later keyboard and mouse event. In a real transport + // failure this key up never reaches the page; here the stub only fails + // the key down, so the page observes it. assert.deepStrictEqual(await page.evaluate('logs'), [ 'dControl', 'dShift', + 'uC', 'uShift', 'uControl', ]); }); }); + + it('retries releasing the main key when its key up fails', async () => { + await withMcpContext(async (response, context) => { + const page = context.getSelectedMcpPage().pptrPage; + await page.setContent( + html``, + ); + context.getSelectedMcpPage().textSnapshot = await TextSnapshot.create( + context.getSelectedMcpPage(), + ); + + // A transient CDP failure on the main key's key up. The key down for + // "C" already reached the renderer, so if nothing retries the key up + // the browser is left with "C" logically held down. + const realUp = page.keyboard.up.bind(page.keyboard); + let failedOnce = false; + sinon + .stub(page.keyboard, 'up') + .callsFake(async (key: KeyInput): Promise => { + if (key === 'C' && !failedOnce) { + failedOnce = true; + throw new Error('transient CDP failure'); + } + return await realUp(key); + }); + + try { + await assert.rejects( + pressKey.handler( + { + params: {key: 'Control+Shift+C'}, + page: context.getSelectedMcpPage(), + }, + response, + context, + ), + ); + } finally { + sinon.restore(); + } + + const logs = (await page.evaluate('logs')) as string[]; + assert.ok( + logs.includes('uC'), + `expected "C" to be released, got ${JSON.stringify(logs)}`, + ); + }); + }); + }); + + describe('drag', () => { + it('releases the mouse button when the drop fails', async () => { + await withMcpContext(async (response, context) => { + const page = context.getSelectedMcpPage().pptrPage; + await page.setContent( + html` +
drag me
+
drop here
`, + ); + context.getSelectedMcpPage().textSnapshot = await TextSnapshot.create( + context.getSelectedMcpPage(), + ); + + // ElementHandle.drag() presses the mouse button down; only drop() + // releases it. If the drop fails, the button must not stay down. + const probe = await page.$('#to'); + assert.ok(probe); + const elementHandlePrototype = Object.getPrototypeOf(probe); + sinon + .stub(elementHandlePrototype, 'drop') + .rejects(new Error('drop failed')); + await probe.dispose(); + + try { + await assert.rejects( + drag.handler( + { + params: {from_uid: '1_1', to_uid: '1_2'}, + page: context.getSelectedMcpPage(), + }, + response, + context, + ), + ); + } finally { + sinon.restore(); + } + + const logs = (await page.evaluate('logs')) as string[]; + assert.ok( + logs.includes('down'), + `expected the drag to press the mouse down, got ${JSON.stringify(logs)}`, + ); + assert.ok( + logs.includes('up'), + `mouse button left held down after a failed drop, got ${JSON.stringify(logs)}`, + ); + }); + }); }); }); From 491ecae9dd559303146a97cb298203232cc72d3f Mon Sep 17 00:00:00 2001 From: Liohtml <158847046+Liohtml@users.noreply.github.com> Date: Mon, 13 Jul 2026 08:20:38 +0200 Subject: [PATCH 2/3] fix: keep Keyboard.press() for the main key press Calling keyboard.down(key) directly changed the CDP payload: press() passes {} to down(), which sends commands: undefined, while calling down(key) with no options uses Puppeteer's default and sends commands: []. Keep using press() so the wire payload is unchanged; the release guarantee is unaffected because the key is recorded as held before press() runs and only cleared once it resolves. Also documents why the drag path releases the mouse unconditionally. --- src/tools/input.ts | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/tools/input.ts b/src/tools/input.ts index a86386b4e..df190517c 100644 --- a/src/tools/input.ts +++ b/src/tools/input.ts @@ -39,9 +39,13 @@ const submitKeySchema = zod * * Puppeteer's `Keyboard.press()` is a bare `down()` followed by `up()` with no * guard of its own, so a rejection between the two leaves the key logically - * held down in the browser. `Keyboard.down()` also sets the client-side - * modifier bit *before* it sends the CDP command, and only `up()` clears that - * bit, so a key whose `down()` rejected still needs a matching `up()`. + * held down in the browser. + * + * Keys whose `down()` rejected are released too: `Keyboard.down()` latches the + * key on the client (`_modifiers |= bit`) *before* it sends the CDP command and + * only `up()` clears it, and `_modifiers` is stamped onto every subsequent + * keyboard, mouse and touch event. A stray key up is harmless; a latched + * modifier poisoning every later click is not. */ async function pressKeyReleasingHeldKeys( keyboard: Keyboard, @@ -55,8 +59,7 @@ async function pressKeyReleasingHeldKeys( await keyboard.down(modifier); } held.push(key); - await keyboard.down(key); - await keyboard.up(key); + await keyboard.press(key); held.pop(); } finally { for (const heldKey of held.toReversed()) { @@ -438,7 +441,11 @@ export const drag = definePageTool({ if (!dropped) { // `drag()` presses the mouse button down and only `drop()` releases // it, so a failed drop leaves the button held down for the rest of - // the session, breaking every later click. + // the session, breaking every later click. `drag()` can also throw + // after pressing the button (it resets its own dragging flag + // without releasing), and there is no way to ask whether the button + // is currently down — so release unconditionally. A stray mouse up + // is harmless; a stuck button is not. try { await request.page.pptrPage.mouse.up(); } catch (error) { From 51d5495fc46c06691035c35a4177825493a83546 Mon Sep 17 00:00:00 2001 From: Liohtml <158847046+Liohtml@users.noreply.github.com> Date: Mon, 13 Jul 2026 08:50:14 +0200 Subject: [PATCH 3/3] fix: release keys left held when a press_key key event fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrows this change to what is provably correct after a second audit. press_key still left the *main* key held: Puppeteer's Keyboard.press() is a bare down() followed by up() with no guard, so a rejecting up() leaves the key down in the browser. #2347 released only the modifiers. The press is now split into down()/up() so a failed release can be retried, without dispatching a key up for a key whose down() never landed. down(key, {}) keeps the CDP payload identical to press(). Each release is also guarded so a failing up() can neither abort the remaining releases nor replace the error that triggered them. type_text's submitKey goes through the same helper. Dropped from this change after review: - The drag fix. ElementHandle.drag() presses the mouse button and only drop() releases it, so a failed drop does leave the button down. But drop() clears page._isDragging only on success, so releasing the mouse without also clearing that flag makes the *next* drag skip mouse.down() and fail with "'left' is not pressed." — worse than the bug. Clearing it means reaching into a Puppeteer internal, so it needs its own change. - Keyboard.type() presses each character through the same unguarded press(). Locator.fill() types short values that way too, so fill and fill_form are exposed as well. Guarding it here would mean reimplementing type() on top of the non-public charIsKey(); the fix belongs in Puppeteer's press(). --- src/tools/input.ts | 97 +++++++++++++++++++++------------------ tests/tools/input.test.ts | 66 +------------------------- 2 files changed, 55 insertions(+), 108 deletions(-) diff --git a/src/tools/input.ts b/src/tools/input.ts index df190517c..ec8b774bb 100644 --- a/src/tools/input.ts +++ b/src/tools/input.ts @@ -34,42 +34,62 @@ const submitKeySchema = zod ); /** - * Presses `key` while `modifiers` are held, and guarantees that every key it - * pressed down is released again — even if a key event rejects mid-sequence. + * Releases `key`, swallowing any failure: a failed release must not abort the + * releases that follow it, nor replace the error that triggered the release. + * + * If the original `up()` reached the renderer before its promise rejected, this + * dispatches a second key up. That is a deliberate trade: a duplicate key up is + * inert for a key the renderer no longer considers pressed, whereas a key left + * held down is not. + */ +async function releaseKey(keyboard: Keyboard, key: KeyInput) { + try { + await keyboard.up(key); + } catch (error) { + logger?.('failed to release key', key, error); + } +} + +/** + * Presses `key` while `modifiers` are held, releasing every key that was + * actually pressed down even if a key event rejects mid-sequence. * * Puppeteer's `Keyboard.press()` is a bare `down()` followed by `up()` with no - * guard of its own, so a rejection between the two leaves the key logically - * held down in the browser. + * guard of its own, so a rejecting `up()` leaves the key held down in the + * browser. The press is split here so that a failed release can be retried + * without dispatching a key up for a key that never went down. * - * Keys whose `down()` rejected are released too: `Keyboard.down()` latches the - * key on the client (`_modifiers |= bit`) *before* it sends the CDP command and - * only `up()` clears it, and `_modifiers` is stamped onto every subsequent - * keyboard, mouse and touch event. A stray key up is harmless; a latched - * modifier poisoning every later click is not. + * Modifiers are released even when their own `down()` rejected. `Keyboard.down()` + * sets the client-side modifier bit *before* it sends the CDP command and only + * `up()` clears it, and that bit is stamped onto every subsequent keyboard, + * mouse and touch event — so a latched modifier would silently modify every + * later click. Non-modifier keys carry no such bit, which is why the main key is + * released only once its `down()` has actually succeeded. */ async function pressKeyReleasingHeldKeys( keyboard: Keyboard, key: KeyInput, modifiers: KeyInput[] = [], ) { - const held: KeyInput[] = []; + const heldModifiers: KeyInput[] = []; + let keyIsDown = false; try { for (const modifier of modifiers) { - held.push(modifier); + heldModifiers.push(modifier); await keyboard.down(modifier); } - held.push(key); - await keyboard.press(key); - held.pop(); + // Equivalent to keyboard.press(key), which calls down(key, {}) followed by + // up(key). Passing {} keeps the dispatched CDP payload identical. + await keyboard.down(key, {}); + keyIsDown = true; + await keyboard.up(key); + keyIsDown = false; } finally { - for (const heldKey of held.toReversed()) { - try { - await keyboard.up(heldKey); - } catch (error) { - // A failed release must not abort the remaining releases, nor replace - // the original error with its own. - logger?.('failed to release key', heldKey, error); - } + if (keyIsDown) { + await releaseKey(keyboard, key); + } + for (const modifier of heldModifiers.toReversed()) { + await releaseKey(keyboard, modifier); } } } @@ -395,6 +415,14 @@ export const typeText = definePageTool({ handler: async (request, response) => { const page = request.page; const result = await page.waitForEventsAfterAction(async () => { + // Note: Keyboard.type() presses each character through the same unguarded + // down()/up() pair, so a mid-string failure can still leave a character + // key held. Locator.fill() types short values through Keyboard.type() as + // well, so `fill` and `fill_form` are exposed the same way. Guarding it + // here would mean reimplementing type() on top of charIsKey(), which is + // not public API — the release belongs in Puppeteer's press(). A stuck + // character key is far less harmful than a stuck modifier: it carries no + // modifier bit, so it is not stamped onto subsequent events. await page.pptrPage.keyboard.type(request.params.text); if (request.params.submitKey) { await pressKeyReleasingHeldKeys( @@ -431,28 +459,9 @@ export const drag = definePageTool({ const toHandle = await request.page.getElementByUid(request.params.to_uid); try { const result = await request.page.waitForEventsAfterAction(async () => { - let dropped = false; - try { - await fromHandle.drag(toHandle); - await new Promise(resolve => setTimeout(resolve, 50)); - await toHandle.drop(fromHandle); - dropped = true; - } finally { - if (!dropped) { - // `drag()` presses the mouse button down and only `drop()` releases - // it, so a failed drop leaves the button held down for the rest of - // the session, breaking every later click. `drag()` can also throw - // after pressing the button (it resets its own dragging flag - // without releasing), and there is no way to ask whether the button - // is currently down — so release unconditionally. A stray mouse up - // is harmless; a stuck button is not. - try { - await request.page.pptrPage.mouse.up(); - } catch (error) { - logger?.('failed to release the mouse button', error); - } - } - } + await fromHandle.drag(toHandle); + await new Promise(resolve => setTimeout(resolve, 50)); + await toHandle.drop(fromHandle); }); response.appendResponseLine(`Successfully dragged an element`); response.attachWaitForResult(result); diff --git a/tests/tools/input.test.ts b/tests/tools/input.test.ts index 025ff987b..c3be1fdf4 100644 --- a/tests/tools/input.test.ts +++ b/tests/tools/input.test.ts @@ -1407,18 +1407,11 @@ describe('input', () => { } // The modifiers must be released even though the main key failed, - // otherwise they stay logically held down in the browser. - // - // "C" is released too, even though its key down failed: Keyboard.down() - // latches the key on the client *before* it sends the CDP command, and - // only up() clears it — a latched modifier would otherwise be stamped - // onto every later keyboard and mouse event. In a real transport - // failure this key up never reaches the page; here the stub only fails - // the key down, so the page observes it. + // otherwise they stay logically held down in the browser. "C" is not + // released: its key down never landed, so there is no key up to send. assert.deepStrictEqual(await page.evaluate('logs'), [ 'dControl', 'dShift', - 'uC', 'uShift', 'uControl', ]); @@ -1477,59 +1470,4 @@ describe('input', () => { }); }); }); - - describe('drag', () => { - it('releases the mouse button when the drop fails', async () => { - await withMcpContext(async (response, context) => { - const page = context.getSelectedMcpPage().pptrPage; - await page.setContent( - html` -
drag me
-
drop here
`, - ); - context.getSelectedMcpPage().textSnapshot = await TextSnapshot.create( - context.getSelectedMcpPage(), - ); - - // ElementHandle.drag() presses the mouse button down; only drop() - // releases it. If the drop fails, the button must not stay down. - const probe = await page.$('#to'); - assert.ok(probe); - const elementHandlePrototype = Object.getPrototypeOf(probe); - sinon - .stub(elementHandlePrototype, 'drop') - .rejects(new Error('drop failed')); - await probe.dispose(); - - try { - await assert.rejects( - drag.handler( - { - params: {from_uid: '1_1', to_uid: '1_2'}, - page: context.getSelectedMcpPage(), - }, - response, - context, - ), - ); - } finally { - sinon.restore(); - } - - const logs = (await page.evaluate('logs')) as string[]; - assert.ok( - logs.includes('down'), - `expected the drag to press the mouse down, got ${JSON.stringify(logs)}`, - ); - assert.ok( - logs.includes('up'), - `mouse button left held down after a failed drop, got ${JSON.stringify(logs)}`, - ); - }); - }); - }); });