feat: sync browser_drop, evaluate, and network request tools#122
feat: sync browser_drop, evaluate, and network request tools#122JustasMonkev wants to merge 5 commits into
Conversation
Bring the browser toolset in line with upstream playwright-mcp: - browser_drop: drop files or MIME-typed data onto an element, with file paths sandboxed to the client root / output dir. - browser_evaluate: accept a plain expression as well as a function, await promises, and surface errors instead of throwing raw. - browser_network_requests: numbered list, hides successful static resources by default, adds static/filter/filename params. - browser_network_request: inspect headers or body for one indexed request, with sensitive-header redaction, size caps, and safe handling of compressed/binary bodies. Adds harness coverage for the two new tools and updates README/SKILL docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a new ChangesBrowser drop, evaluate rewrite, and network request inspection
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…er-tool-sync # Conflicts: # .claude/run-mcp-direct-harness.mjs
Resolve harness conflict by keeping main's fixture-based browser_drop and
browser_network_request tests; drop the duplicate tests this branch added.
Two follow-on fixes so the merged harness passes:
- codegen.formatObject now quotes object keys that are not valid JS
identifiers (e.g. "text/plain"), so browser_drop generates valid code.
- network harness tests pass { static: true } since the top-level document
request is a non-fetch resource hidden by default.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/run-mcp-direct-harness.mjs:
- Around line 155-158: The test is using a hard-coded request index and a
too-loose assertion, which can inspect the wrong browser request. In
browser_network_request within run-mcp-direct-harness.mjs, resolve the
/audit-site entry from browser_network_requests first and pass that index into
callTool, then tighten the assertText check so it requires the expected detail
markers from the selected request rather than accepting a generic match.
In `@src/tools/evaluate.ts`:
- Around line 51-63: The normalization logic in evaluate() is still using
page-context eval inside both the locator.evaluate and tab.page.evaluate
branches, which can break CSP-restricted pages for plain expressions like
document.title. Move the function-vs-expression detection outside the page realm
in src/tools/evaluate.ts, then pass either a callable or a simple expression
path into the existing evaluate flow without using eval in the page context. Add
a CSP regression test covering the browser_evaluate path to ensure expressions
work when unsafe-eval is blocked.
In `@src/tools/network.ts`:
- Around line 359-361: The default binary filename built in the network download
flow uses new Date().toISOString() inside the defaultPrefix filename, which
introduces colon characters that break Windows paths when filename is omitted.
Update the filename generation in the relevant helper in network.ts to sanitize
the timestamp before composing defaultFilename, and keep the change localized
around the extension/defaultFilename/outputFile logic so the fallback path
remains valid across platforms.
In `@tests/e2e-tools-smoke.test.ts`:
- Around line 61-65: The async drop handling in browser_drop is not being
awaited, so assertions can run before dropzone.dataset.dropped is updated. Add a
promise or other synchronization in the drop listener that resolves after the
awaited file.text() completes, and have browser_drop wait on it before
returning. Apply the same fix for both file-drop assertion paths in browser_drop
and the later helper used around the second drop assertion, so browser_evaluate
always sees the final dropped value.
In `@tests/tools-evaluate.test.ts`:
- Around line 107-110: The locator mocks in tests/tools-evaluate.test.ts define
normalize more than once, which makes the mock object ambiguous and can break
TypeScript compilation. Clean up the mock object passed to mockTab.refLocator so
it has only one normalize property, and apply the same fix to the second locator
mock mentioned in the review. Use the refLocator setup and locator mock
definitions as the target points for the duplicate removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45fe73ce-044f-4e1a-87b1-266da8e3b439
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
.claude/run-mcp-direct-harness.mjsREADME.mdSKILL.mdpackage.jsonsrc/tools/evaluate.tssrc/tools/files.tssrc/tools/network.tstests/e2e-tools-smoke.test.tstests/tools-evaluate.test.tstests/tools-files.test.tstests/tools-network.test.ts
| evalResult = await locator.evaluate(async (element, source) => { | ||
| const value = eval(`(${source})`); | ||
| const isFunction = typeof value === 'function'; | ||
| const result = await (isFunction ? value(element) : value); | ||
| return { result, isFunction }; | ||
| }, params.function); | ||
| } else { | ||
| evalResult = await tab.page.evaluate(async source => { | ||
| const value = eval(`(${source})`); | ||
| const isFunction = typeof value === 'function'; | ||
| const result = await (isFunction ? value() : value); | ||
| return { result, isFunction }; | ||
| }, params.function); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether evaluate tests cover CSP pages and where eval is introduced.
rg -nP '\beval\s*\(' src tests
rg -nP 'Content-Security-Policy|unsafe-eval|CSP' tests srcRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## evaluate.ts\n'
cat -n src/tools/evaluate.ts | sed -n '1,220p'
printf '\n## tests mentioning evaluate/browser_evaluate/CSP\n'
rg -n -C 2 'browser_evaluate|evaluate\(|Content-Security-Policy|unsafe-eval|CSP' tests src || trueRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 14819
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## file inventory\n'
git ls-files | rg '(^|/)(test|tests|__tests__)/|evaluate\.ts$|axe|auditSite|auditKeyboard'
printf '\n## evaluate.ts outline\n'
ast-grep outline src/tools/evaluate.ts --view expanded || trueRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 1448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## src/tools/evaluate.ts (focused)\n'
sed -n '1,220p' src/tools/evaluate.ts | cat -n
printf '\n## test files with evaluate/tool coverage\n'
fd -a -t f '.*\.(test|spec)\.(ts|tsx|js|jsx)$' . | sed 's#^\./##' | sort | head -200
printf '\n## search in tests for evaluate-related paths\n'
rg -n -C 2 'evaluate\.ts|browser_evaluate|evalResult|params\.function|locator\.evaluate|page\.evaluate|Content-Security-Policy|unsafe-eval' . || trueRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 23663
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('src/tools/evaluate.ts')
print(p.read_text())
PYRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 3490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## src/tools/evaluate.ts'
cat -n src/tools/evaluate.ts | sed -n '1,180p'
echo
echo '## test files'
fd -a -t f '.*\.(test|spec)\.(ts|tsx|js|jsx)$' . | sed 's#^\./##' | sort | head -200
echo
echo '## CSP / evaluate references'
rg -n -C 2 'browser_evaluate|params\.function|locator\.evaluate|page\.evaluate|Content-Security-Policy|unsafe-eval|CSP' src tests . || trueRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 29978
Avoid page-context eval when normalizing browser_evaluate input. CSP-restricted pages can block unsafe-eval, so plain expressions like document.title may fail even though the public page.evaluate call would succeed. Detect function-vs-expression outside the page realm and add a CSP regression test for this path.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 51-51: Avoid eval with expressions
Context: eval((${source}))
Note: [CWE-95] Improper Neutralization of Directives in Dynamically Evaluated Code ('Eval Injection').
(detect-eval-with-expression-typescript)
[warning] 58-58: Avoid eval with expressions
Context: eval((${source}))
Note: [CWE-95] Improper Neutralization of Directives in Dynamically Evaluated Code ('Eval Injection').
(detect-eval-with-expression-typescript)
🪛 Biome (2.5.1)
[error] 52-52: eval() exposes to security risks and performance issues.
(lint/security/noGlobalEval)
[error] 59-59: eval() exposes to security risks and performance issues.
(lint/security/noGlobalEval)
🪛 OpenGrep (1.23.0)
[ERROR] 52-52: eval() with dynamic input can execute arbitrary code. Avoid dynamic code evaluation entirely, or use a safe alternative.
(coderabbit.code-injection.eval-js)
[ERROR] 59-59: eval() with dynamic input can execute arbitrary code. Avoid dynamic code evaluation entirely, or use a safe alternative.
(coderabbit.code-injection.eval-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/evaluate.ts` around lines 51 - 63, The normalization logic in
evaluate() is still using page-context eval inside both the locator.evaluate and
tab.page.evaluate branches, which can break CSP-restricted pages for plain
expressions like document.title. Move the function-vs-expression detection
outside the page realm in src/tools/evaluate.ts, then pass either a callable or
a simple expression path into the existing evaluate flow without using eval in
the page context. Add a CSP regression test covering the browser_evaluate path
to ensure expressions work when unsafe-eval is blocked.
Source: Linters/SAST tools
| const extension = mime.getExtension(contentType) ?? 'bin'; | ||
| const defaultFilename = `${defaultPrefix}-${new Date().toISOString()}.${extension}`; | ||
| const outputPath = await tab.context.outputFile(filename ?? defaultFilename); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Sanitize the generated binary filename timestamp.
toISOString() includes : characters, so the default binary save path can fail on Windows when no filename is provided.
Proposed fix
- const defaultFilename = `${defaultPrefix}-${new Date().toISOString()}.${extension}`;
+ const timestamp = new Date().toISOString().replace(/[:.]/g, '-');
+ const defaultFilename = `${defaultPrefix}-${timestamp}.${extension}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const extension = mime.getExtension(contentType) ?? 'bin'; | |
| const defaultFilename = `${defaultPrefix}-${new Date().toISOString()}.${extension}`; | |
| const outputPath = await tab.context.outputFile(filename ?? defaultFilename); | |
| const extension = mime.getExtension(contentType) ?? 'bin'; | |
| const timestamp = new Date().toISOString().replace(/[:.]/g, '-'); | |
| const defaultFilename = `${defaultPrefix}-${timestamp}.${extension}`; | |
| const outputPath = await tab.context.outputFile(filename ?? defaultFilename); |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 361-361: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.promises.writeFile(outputPath, body)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/network.ts` around lines 359 - 361, The default binary filename
built in the network download flow uses new Date().toISOString() inside the
defaultPrefix filename, which introduces colon characters that break Windows
paths when filename is omitted. Update the filename generation in the relevant
helper in network.ts to sanitize the timestamp before composing defaultFilename,
and keep the change localized around the extension/defaultFilename/outputFile
logic so the fallback path remains valid across platforms.
| dropzone.addEventListener('drop', async event => { | ||
| event.preventDefault(); | ||
| const file = event.dataTransfer.files[0]; | ||
| dropzone.dataset.dropped = file ? await file.text() : event.dataTransfer.getData('text/plain'); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the test file and inspect the relevant regions with line numbers.
wc -l tests/e2e-tools-smoke.test.ts
sed -n '1,140p' tests/e2e-tools-smoke.test.ts
printf '\n--- later section ---\n'
sed -n '250,320p' tests/e2e-tools-smoke.test.ts
# Search for the specific browser_drop / browser_evaluate pattern and any existing sync helpers.
rg -n "browser_drop|browser_evaluate|dropzoneDropComplete|dataset\.dropped|addEventListener\('drop'" tests/e2e-tools-smoke.test.tsRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 8099
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the browser_drop / browser_evaluate implementations and inspect the relevant slices.
rg -n "browser_drop|browser_evaluate|drop" src tests -g '!**/node_modules/**'
printf '\n--- browser server backend outline ---\n'
ast-grep outline src/browserServerBackend.ts --view expanded || true
printf '\n--- search in browser server backend ---\n'
rg -n "browser_drop|browser_evaluate|callTool|evaluate|drop" src/browserServerBackend.ts src -g '!**/node_modules/**'
# Read the implementation regions if the file exists.
if [ -f src/browserServerBackend.ts ]; then
printf '\n--- browser server backend slice 1 ---\n'
sed -n '1,260p' src/browserServerBackend.ts
printf '\n--- browser server backend slice 2 ---\n'
sed -n '260,520p' src/browserServerBackend.ts
fiRepository: JustasMonkev/mcp-accessibility-scanner
Length of output: 18475
Wait for the async drop handler before asserting. browser_drop finishes before the drop listener’s awaited file.text() resolves, so the following browser_evaluate can read a stale value. Synchronize on a promise from the handler before both file-drop assertions here and at 282-286.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e-tools-smoke.test.ts` around lines 61 - 65, The async drop handling
in browser_drop is not being awaited, so assertions can run before
dropzone.dataset.dropped is updated. Add a promise or other synchronization in
the drop listener that resolves after the awaited file.text() completes, and
have browser_drop wait on it before returning. Apply the same fix for both
file-drop assertion paths in browser_drop and the later helper used around the
second drop assertion, so browser_evaluate always sees the final dropped value.
| mockTab.refLocator.mockResolvedValue({ | ||
| evaluate: locatorEvaluate, | ||
| _resolveSelector: async () => ({ resolvedSelector: 'internal:role=button' }), | ||
| normalize: async () => ({ toString: () => `locator('#button')` }), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate normalize mock property.
Both locator mock objects define normalize twice, which can fail TypeScript compilation and makes the mock shape ambiguous.
Proposed fix
mockTab.refLocator.mockResolvedValue({
evaluate: locatorEvaluate,
- normalize: async () => ({ toString: () => `locator('`#button`')` }),
normalize: async () => ({ toString: () => `locator('`#button`')` }),
});Apply the same cleanup to the second locator mock.
Also applies to: 131-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tools-evaluate.test.ts` around lines 107 - 110, The locator mocks in
tests/tools-evaluate.test.ts define normalize more than once, which makes the
mock object ambiguous and can break TypeScript compilation. Clean up the mock
object passed to mockTab.refLocator so it has only one normalize property, and
apply the same fix to the second locator mock mentioned in the review. Use the
refLocator setup and locator mock definitions as the target points for the
duplicate removal.
1 participant
Summary
Syncs the browser toolset with upstream playwright-mcp (issue #116):
browser_drop— drop files or MIME-typed data onto an element, as if dragged from outside the page. File paths are sandboxed to the client filesystem root / configured output dir.browser_evaluate— now accepts a plain expression as well as a function, awaits returned promises, and reports errors instead of throwing raw.browser_network_requests— numbered list with stable indexes; hides successful static resources by default; addsstatic,filter, andfilenameparams.browser_network_request(new) — inspect headers or a single body part for one indexed request. Redacts sensitive headers by default, caps inline/total body sizes, saves binary bodies byte-for-byte, and fails closed on compressed/unbounded bodies.Docs (README, SKILL.md) updated for the new/changed tools.
Testing
npm run typecheck— cleannpm run test— 394 passednpm run test:mcp— 29 passed, 1 skipped (browser_install); added harness coverage forbrowser_dropandbrowser_network_request🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation