Skip to content

fix: resolve correctness and security findings from full-repo review#248

Merged
danjdewhurst merged 3 commits into
mainfrom
claude/repo-review-fixes-mmemn8
Jun 11, 2026
Merged

fix: resolve correctness and security findings from full-repo review#248
danjdewhurst merged 3 commits into
mainfrom
claude/repo-review-fixes-mmemn8

Conversation

@danjdewhurst

Copy link
Copy Markdown
Contributor

Full-repo review fixes

A systematic review of the codebase (core daemon, config/plugins, flow machinery, all 79 command handlers, scripts/packaging, docs) surfaced ~35 high-confidence defects. This PR fixes all of them except one architectural item noted at the bottom.

Crashes and broken commands

  • browse diff crashed on every invocation: the handler declared _deps but the body referenced deps (ReferenceError at runtime; tsc isn't in CI so nothing caught it).
  • Stealth mode broke real pages (two bugs): chrome.runtime.getVersion() referenced an undefined chromeVersion identifier, and the getComputedStyle proxy returned unbound native methods so style.getPropertyValue(...) threw Illegal invocation on every stealth page — itself a bot-detection signal.
  • extract table @ref used the 0-based nthMatch with 1-based :nth-of-type, selecting the wrong table (or none). Now indexes querySelectorAll results directly.
  • Crawl globs: --include/--exclude patterns containing (, [, + threw mid-crawl, and . acted as a wildcard (*.example.com* matched xexamplexcom.evil). Metacharacters are now escaped.

Daemon robustness

  • Accepted sockets had no error listener — an abrupt client disconnect (ECONNRESET) raised an uncaught exception and killed the daemon, orphaning the browser.
  • The idle timer could shut the daemon down mid-command; it now defers while commands are active.
  • browse batch file.json --json was ignored: the batch-level json flag never propagated because entries always parsed to a concrete false.
  • wait url/text/visible/hidden poll loops never terminated — the outer timeout only abandons the promise. Timed-out waits polled the page every 100 ms for the daemon's entire lifetime. Loops are now bounded by the request timeout.
  • record start/stop leaked a framenavigated listener per cycle, duplicating goto steps in saved flows.
  • Smaller leaks/races: withTimeout timers now cleared on settle; scratch-page creation caches the in-flight promise; trace state moved to a WeakMap; per-session video state released on session close; auth token compared with timingSafeEqual (relevant for --listen tcp://).

Wrong results

  • browse compliance on the current page always passed the cookie audit — it audited a freshly created (empty) context. With a URL, the tracker check read the main session's network buffer instead of the audited page's requests. Both fixed.
  • Tracker cookie classification: blanket prefix matching flagged first-party cookies like front_session as Facebook (fr) and IDENTITY as DoubleClick (IDE). Prefix matching is now limited to dynamic-suffix prefixes (_ga_, _hj, …).
  • Login could report success without verifying anything: config validation accepted textVisible/elementNotVisible success conditions that waitForSuccess doesn't implement (it fell through and resolved immediately). Validation now only accepts the three implemented variants.
  • Recorded flows that fell back to CSS selectors could never replay — replay treated the selector string as an accessible name. The recorder now emits the { selector } step form the flow schema already supports.
  • a11y headings collected headings grouped by level, masking real skip detection; gesture ignored .nth() disambiguation for duplicate-name refs; crawl pagination could exceed --max-pages by ~2x; URL normalization stripped slashes out of query strings; vrt passed its pass/fail percentage as a per-pixel colour tolerance; cookies --domain badexample.com matched .example.com; throttle --upload/--latency without --download was rejected; subscribe silently accepted (and defaulted to!) network/download events it never listened for; page-eval re-executed side-effectful code on runtime errors; do ignored HTTP error statuses and double-appended /v1 to OPENAI_BASE_URL; trace clean/video clean documented flags were rejected by daemon flag validation; shell completions were missing four commands and several flags.

Security hardening

  • compileSafePattern claimed ReDoS mitigation but only capped length(a+)* is 6 chars and hangs the daemon's event loop (flows are an untrusted input via flow-share install). It now rejects nested unbounded quantifiers (documented as a heuristic).
  • stealth.ts interpolated the config-supplied browser channel into a shell command (execSync); now execFileSync with an argument vector.

Packaging / docs

  • Dockerfile: the runtime image (playwright:v1.54.1) had neither Chrome nor the chromium revision the embedded patchright 1.58.2 expects — the published image could only run --help. It now installs a matching Chrome.
  • install.sh installed browsers via upstream playwright, whose browser revisions the patchright-based release binary can't find; switched to patchright (matching setup.sh/CI).
  • docs/plugins.md told plugin authors to import from "browse/plugin", which doesn't exist (package is private/unpublished); now points at the relative-path pattern the bundled examples use.

Tests

  • New: test/safe-pattern.test.ts, test/tracker-database.test.ts, plus regression tests for glob escaping, query-string normalization, and batch json fallback.
  • bun test: 1358 pass, 0 fail (was 1343); biome check clean; binary compiles and runs.

Known limitation (intentionally not fixed here)

The ref registry (src/refs.ts) is module-global while sessions/tabs execute concurrently: one session's snapshot/navigation can invalidate or remap another session's @e refs. Fixing it properly means moving the registry into TabState and threading it through ~36 call sites and ~200 test call sites — a refactor that deserves its own PR rather than riding along here. Current behavior is conservatively safe for single-session use (any navigation marks refs stale).

https://claude.ai/code/session_018W3Am9a8RWjW8drwJCBNHd


Generated by Claude Code

Crashes and broken commands:
- diff: rename _deps parameter to deps — every invocation threw
  ReferenceError: deps is not defined
- stealth: chrome.runtime.getVersion() referenced an undefined
  chromeVersion identifier, throwing in the page
- stealth: bind getComputedStyle proxy methods to the underlying
  CSSStyleDeclaration so getPropertyValue() no longer throws
  "Illegal invocation" on every stealth page
- extract: table @ref used 0-based nthMatch with 1-based
  :nth-of-type, selecting the wrong (or no) table
- crawl: escape regex metacharacters in --include/--exclude globs;
  patterns with (, [, + crashed mid-crawl and . matched any char

Daemon robustness:
- add a socket error handler so an abrupt client disconnect
  (ECONNRESET) no longer kills the daemon
- defer idle shutdown while a command is still executing
- propagate batch-level json flag to entries that don't set it
- bound wait url/text/visible/hidden poll loops with the request
  timeout — abandoned loops previously polled forever
- record: detach the framenavigated listener on record stop;
  listeners stacked across sessions and duplicated goto steps
- clear withTimeout's timer once the command settles
- cache in-flight scratch-page creation to avoid leaking pages
  under concurrent calls
- use a WeakMap for per-context trace state and release per-session
  video state on session close
- compare auth tokens with timingSafeEqual

Correct results:
- compliance: only use a fresh context when a URL is given (cookie
  audit of the current page always saw an empty jar), and capture
  the fresh page's requests instead of the main session's buffer
- tracker-database: restrict cookie prefix matching to dynamic-
  suffix prefixes; front_session and similar first-party cookies
  were reported as Facebook/DoubleClick trackers
- a11y headings: collect in document order so skip detection works
- gesture: apply .nth() disambiguation to refs like other commands
- recorder: emit selector fallbacks as { selector } steps so
  recorded flows replay; require a path boundary in base-url
  rewriting
- crawl: pagination respects the remaining --max-pages budget and
  trailing-slash normalization no longer mangles query strings
- config: validate login successCondition against the three keys
  login actually implements — textVisible et al. silently skipped
  verification
- vrt: stop passing the pass/fail percentage threshold as
  compareScreenshots' per-pixel colour tolerance
- cookies: fix --domain suffix matching (badexample.com no longer
  matches .example.com)
- throttle: allow --upload/--latency without --download
- subscribe: implement the advertised network/download events and
  honour --status
- page-eval: only fall back to the unwrapped form on construction
  errors — runtime failures re-executed side-effectful code
- do: check HTTP response status and align OPENAI_BASE_URL
  semantics with assert-ai (base URL includes /v1)
- back/forward/intercept: swallow late rejections from the losing
  Promise.race branch and route.fulfill
- trace/video clean: allow the documented --older-than/--dry-run
  flags through daemon flag validation
- completions: add missing perf/security/responsive/extract
  commands and --webhook/--metrics flags

Security hardening:
- safe-pattern: reject nested unbounded quantifiers (e.g. (a+)*)
  instead of relying on a length cap that doesn't mitigate ReDoS
- stealth: use execFileSync instead of shell interpolation when
  probing the configured browser binary for its version

Packaging/docs:
- Dockerfile: install a patchright-matching Chrome in the runtime
  image — the published image could only run --help
- install.sh: install browsers via patchright (not upstream
  playwright, whose browser revisions the binary can't find)
- docs/plugins.md: fix import paths ("browse/plugin" is not
  published; use a relative path like the bundled examples)

https://claude.ai/code/session_018W3Am9a8RWjW8drwJCBNHd
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@danjdewhurst, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 54 minutes and 43 seconds. Learn how PR review limits work.

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9ca6a8dc-0051-4996-bd78-0d115a34eb4c

📥 Commits

Reviewing files that changed from the base of the PR and between 59f1bd2 and b7fdf83.

📒 Files selected for processing (35)
  • .github/workflows/ci.yml
  • Dockerfile
  • docs/plugins.md
  • install.sh
  • src/commands/a11y.ts
  • src/commands/back.ts
  • src/commands/compliance.ts
  • src/commands/cookies.ts
  • src/commands/diff.ts
  • src/commands/do.ts
  • src/commands/extract.ts
  • src/commands/forward.ts
  • src/commands/gesture.ts
  • src/commands/intercept.ts
  • src/commands/page-eval.ts
  • src/commands/record.ts
  • src/commands/session.ts
  • src/commands/subscribe.ts
  • src/commands/throttle.ts
  • src/commands/vrt.ts
  • src/commands/wait.ts
  • src/completions.ts
  • src/config.ts
  • src/crawl-engine.ts
  • src/daemon.ts
  • src/protocol.ts
  • src/recorder.ts
  • src/safe-pattern.ts
  • src/stealth.ts
  • src/timeout.ts
  • src/tracker-database.ts
  • test/crawl.test.ts
  • test/protocol.test.ts
  • test/safe-pattern.test.ts
  • test/tracker-database.test.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

claude added 2 commits June 11, 2026 06:23
dl.google.com is serving GitHub macOS runners a 138-byte stub, and
patchright's installer deletes the preinstalled Chrome before it
downloads — failing the install step and leaving no browser behind.
Use the image's Chrome when it exists and only download when missing.

https://claude.ai/code/session_018W3Am9a8RWjW8drwJCBNHd
@danjdewhurst danjdewhurst merged commit ca32366 into main Jun 11, 2026
5 of 6 checks passed
@danjdewhurst danjdewhurst deleted the claude/repo-review-fixes-mmemn8 branch June 11, 2026 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants