Skip to content

fix(review): bound scroll GIF page operations - #3771

Merged
JSONbored merged 1 commit into
mainfrom
codex/fix-vulnerability-in-scroll-gif-capture
Jul 6, 2026
Merged

fix(review): bound scroll GIF page operations#3771
JSONbored merged 1 commit into
mainfrom
codex/fix-vulnerability-in-scroll-gif-capture

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent unbounded waits in the scroll-through GIF capture path where contributor-controlled preview pages can override or hang window.scrollTo/setTimeout/Promise in the page realm and stall review workers.

Description

  • Add a per-evaluate timeout constant SCROLL_EVALUATE_TIMEOUT_MS and a helper withScrollOperationTimeout to bound awaited page-realm evaluate calls.
  • Wrap the page.evaluate call that reads document.documentElement.scrollHeight and the page.evaluate call that invokes window.scrollTo with the timeout helper in src/review/visual/shot.ts.
  • Move the settle delay out of the page realm into a worker-side waitForScrollSettle() so the settle delay is enforced by the worker runtime rather than a page-controlled setTimeout.
  • Add a regression test test/unit/visual-shot-scroll-timeout.test.ts that verifies the normal capture path and asserts a never-settling page-realm evaluate is bounded and the browser is closed cleanly.

Testing

  • Ran the targeted unit tests with npm test -- test/unit/visual-shot-scroll-timeout.test.ts and the two tests in that file passed.
  • Ran npx vitest run test/unit/visual-shot-scroll-timeout.test.ts --coverage --reporter=verbose which passed the scoped tests but the run failed the repo-wide coverage threshold because it was scoped to a single test file.
  • Ran npm run typecheck (tsc --noEmit) and it succeeded.
  • Ran the full coverage job npm run test:coverage which exercised many tests but failed the repository global coverage thresholds and encountered unrelated long-running/timeouts in existing test suites (these failures are not caused by this scoped change).
  • Ran npm audit --audit-level=moderate but the environment registry audit returned 403 Forbidden, so the audit check could not complete.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.32%. Comparing base (9b1cfc9) to head (50fdc37).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3771   +/-   ##
=======================================
  Coverage   93.32%   93.32%           
=======================================
  Files         317      317           
  Lines       32397    32406    +9     
  Branches    11878    11878           
=======================================
+ Hits        30235    30244    +9     
  Misses       1530     1530           
  Partials      632      632           
Files with missing lines Coverage Δ
src/review/visual/shot.ts 90.27% <100.00%> (+0.64%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored force-pushed the codex/fix-vulnerability-in-scroll-gif-capture branch from d4f02ee to 50fdc37 Compare July 6, 2026 09:50
@JSONbored
JSONbored merged commit 120c5e2 into main Jul 6, 2026
10 checks passed
@JSONbored
JSONbored deleted the codex/fix-vulnerability-in-scroll-gif-capture branch July 6, 2026 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant