[udsy19]

Fixes #654.

Impact: silent-wrong-result

Who reaches this: any user of the CLI or MCP scan_skill entry point scanning a skill that bundles
a .claude/settings.json or .claude/settings.local.json, via the bundled_execution_surface
analyzer's BH3 rule (src/skillspector/nodes/analyzers/bundled_execution_surface.py).

Triggered when: the bundled settings file sets env.NODE_OPTIONS to a value containing -r,
Node's documented short alias for --require (e.g. "-r ./malicious.js"). _env_node_options_runs_code()
checks the value's whitespace-separated tokens against _NODE_OPTIONS_CODE_FLAGS, which only listed
the long-form flags (--require, --import, --loader, --experimental-loader). -r is absent,
so the token check never matches.

What the user observes: scan_skill reports the skill as SAFE with no BH3 finding, even though
NODE_OPTIONS=-r ./malicious.js makes Node load and execute the attacker's module on every
subsequent Node invocation in that environment — the identical runtime effect as
NODE_OPTIONS=--require ./malicious.js, which the same rule already correctly flags HIGH.
Confirmed against a live Node v22.23.1 install (process.allowedNodeEnvironmentFlags.has('-r') is
true, and NODE_OPTIONS="-r ./preload.js" node -e "..." actually runs the preload module); see
the linked issue for the full reproduction.

Fix

Add -r to _NODE_OPTIONS_CODE_FLAGS. This is the only NODE_OPTIONS-recognized short alias among
the flags this set models — Node has no short form for --import/--loader/--experimental-loader
— so no other flag needs adding.

Tests

Added two cases to tests/nodes/analyzers/test_bundled_execution_surface.py::test_bh3_flags_risky_settings_surface:
NODE_OPTIONS=-r ./.claude/preload.js alone, and combined with a benign flag
(--max-old-space-size=512 -r ./.claude/hook.js), both asserting the existing BH3/HIGH/
env_code_execution contract this suite already uses for the long-form flags.

Negative control: reverted bundled_execution_surface.py to its pristine state and re-ran the two
new cases:

FAILED tests/nodes/analyzers/test_bundled_execution_surface.py::test_bh3_flags_risky_settings_surface[document10-HIGH-env_code_execution]
FAILED tests/nodes/analyzers/test_bundled_execution_surface.py::test_bh3_flags_risky_settings_surface[document11-HIGH-env_code_execution]
AssertionError: assert [] == ['BH3']
2 failed in 2.70s

With the fix restored, the full tests/nodes/analyzers/test_bundled_execution_surface.py suite
passes: 154 passed (152 pre-existing + 2 new), confirming the asymmetry and no regressions.
ruff check, ruff format --check, and mypy are clean on both changed files.

Signed-off-by: Udaya Tejas udayatejas2004@gmail.com

[rng1995]

[SkillSpector Review]

Hi @udsy19, thank you for closing the -r gap in the BH3 NODE_OPTIONS check and for the clear reproduction in #654!

Value and readiness: Before this change, a bundled settings file with NODE_OPTIONS=-r ./x.js scanned as SAFE, even though Node preloads the module exactly as it does with --require. Adding -r to _NODE_OPTIONS_CODE_FLAGS closes that gap with a one-token change. The two new parametrized cases assert the existing BH3 / HIGH / env_code_execution contract, one with -r alone and one after a benign flag. The existing benign cases (--max-old-space-size=4096, --dns-result-order=ipv4first, empty) are unaffected. It is ready for final maintainer review.

Material findings

1. [Non-blocking, follow-up] src/skillspector/nodes/analyzers/bundled_execution_surface.py:1377-1382 (code this PR does not change): Node removes double quotes when it parses NODE_OPTIONS. As a result, "-r" ./x.js, "--require" ./x.js and --req""uire ./x.js all preload the module, but none of those tokens equals a flag in the set, so none is reported. This bypass predates the PR and also affects the long flags, so it does not block this fix. It is worth a follow-up issue: split NODE_OPTIONS the way Node does (strip " and handle \ escapes) before comparing tokens.

PIC tradeoffs: None identified.

Verification and gaps:

• I traced _env_value_is_flagged → _env_node_options_runs_code at head f72d8f8. -r now matches as a standalone token.
• -r=… also matches, through the = branch. Node rejects that spelling in NODE_OPTIONS ("is not allowed"), so this is harmless over-matching.
• --import, --loader and --experimental-loader have no short aliases, as the PR states.
• _NODE_OPTIONS_CODE_FLAGS is the only NODE_OPTIONS check in src/. It was introduced by #604 (89e9087), and this file has not changed on main since the PR's base. The PR merges cleanly with current main.
• I checked Node's handling of -r ./preload.js, -r=./preload.js and the quoted forms with a local Node v26.4.0 and a scratch preload script that I wrote. No contributor code was run.
• CI: all 6 checks are green. Per policy, I did not run the tests locally.

---

Decision: Approved (reviewed head f72d8f8cc8ff955f9d25c67613dc02c247f55e62)