Skip to content

security: replace new Function() eval with direct ESM import#92

Open
xiaolai wants to merge 1 commit intopbakaus:mainfrom
xiaolai:fix/replace-new-function-with-import
Open

security: replace new Function() eval with direct ESM import#92
xiaolai wants to merge 1 commit intopbakaus:mainfrom
xiaolai:fix/replace-new-function-with-import

Conversation

@xiaolai
Copy link
Copy Markdown
Contributor

@xiaolai xiaolai commented Apr 10, 2026

Summary

  • Replaces 3 instances of new Function() code evaluation with direct ESM imports of the already-exported ANTIPATTERNS array
  • Eliminates an arbitrary code execution surface at build time and dev-server startup
  • Net reduction of 21 lines; no new dependencies

Motivation

Three build scripts (scripts/build.js, scripts/build-extension.js, scripts/lib/sub-pages-data.js) extract the ANTIPATTERNS array from src/detect-antipatterns.mjs via regex, then evaluate it with new Function(). Since ANTIPATTERNS is already exported from the module (line 3577), this indirection is unnecessary.

The new Function() pattern means any commit that modifies the matched regex region in detect-antipatterns.mjs gets arbitrary code execution at:

  • bun run build (build.js + build-extension.js)
  • bun run dev (sub-pages-data.js runs at server boot)

With a direct ESM import, a malformed ANTIPATTERNS entry produces a parse error at import time rather than silent code execution.

Changes

File Before After
scripts/build.js regex + new Function() in validateAntipatternRules() import { ANTIPATTERNS } from source
scripts/build-extension.js regex + new Function() to generate antipatterns.json import { ANTIPATTERNS } from source
scripts/lib/sub-pages-data.js readAntipatternRules() with regex + new Function() readAntipatternRules() returns imported ANTIPATTERNS directly

Test plan

  • bun run build completes without errors
  • bun run dev starts without errors
  • node scripts/build-extension.js generates extension/detector/antipatterns.json with correct rule count
  • Built output matches pre-change output (diff dist/ before and after)

ANTIPATTERNS is already exported from src/detect-antipatterns.mjs, so the
three build scripts that previously extracted it via regex and evaluated it
with new Function() can simply import it directly.

This eliminates an arbitrary code execution surface at build time and
dev-server startup: a tampered detect-antipatterns.mjs could previously
execute arbitrary code silently during the regex-to-eval path.
@pbakaus
Copy link
Copy Markdown
Owner

pbakaus commented Apr 29, 2026

Thanks again, this one would also be a clean improvement. Quick note before we can land it:

The branch needs a rebase against main, and the conflict resolution drops one of the three changes. After the v3.0 build refactor:

  1. scripts/build.js: the validateAntipatternRules function this PR modified was removed entirely. generateCounts now scans src/detect-antipatterns.mjs with a different regex that only counts id: '...' lines, no new Function. So please drop the scripts/build.js change (no import needed).
  2. scripts/build-extension.js: still has new Function() at line 62. PR fix applies cleanly.
  3. scripts/lib/sub-pages-data.js: still has new Function() at line 152. PR fix applies with minor context drift.

If you can rebase to keep just (2) and (3), happy to merge.

One framing note: I'd retitle from security: to something like refactor: or chore:. Build-time code from your own repo's source files isn't really an attack surface (anyone with commit access can already run anything at build), so the value here is code quality (less brittle, removes the no-new-func lint suppression), not security. Tiny thing, just helps the changelog read accurately later.

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