Repository navigation
Conversation
could_be_plugin is a cheap pre-filter that decides whether a specifier is worth running plugin regex filters against. It looked at the byte after the final '.' and only accepted ASCII letters (or non-ASCII), so absolute paths like /app/foo.1 or /app/video.3gp returned false and the onLoad/onResolve regex was never consulted. The module then fell through to the built-in file loader. The only goal of the check was to reject './', '../', '..' (and their Windows backslash forms). Those produce an empty extension or one that starts with a path separator, so accept anything else. Fixes #4609
|
Warning Review limit reached
Next review available in: 4 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM. The inline note on extract_namespace is a pre-existing off-by-one adjacent to this change, not something this PR introduced — worth a follow-up but not a blocker here.
Extended reasoning...
Overview
This PR loosens the PluginRunner::could_be_plugin pre-filter in src/bundler/transpiler.rs so that file extensions beginning with a digit (.1, .3gp, .h2) are allowed through to the plugin regex filters. Previously the check only accepted extensions whose first byte was an ASCII letter or >127, so absolute resolved paths like /app/foo.1 returned false and run_on_load_plugins was skipped entirely. The fix accepts any non-empty extension whose first byte is not a path separator, preserving the original intent of cheaply rejecting ./, ../, .., .\\ while letting everything else reach the real regex match.
Security risks
None. This is a pure byte-classification change in a pre-filter; the widening only causes more specifiers to be tested against user-registered onLoad regex filters, which is the intended behavior. No new inputs reach parsing, allocation, or filesystem code that weren't already reachable via letter-extension paths.
Level of scrutiny
Low-to-moderate. The function is a cheap fast-path guard whose false positives are harmless (they just fall through to the regex filter). I traced all four call sites (ModuleLoader.rs:603, VirtualMachine.rs:4246, jsc_hooks.rs:5139, linker.rs:482) and confirmed they all use this only as a gate before regex matching / extract_namespace. I walked the relative-navigation cases the comment says it must reject — ./, ../, .., .\\, ..\\foo — and each still produces an empty extension or one starting with //\\, so they still fall through to the !is_absolute && has_colon check unchanged.
Other factors
The new test in plugins.test.ts follows harness conventions (tempDir, bunEnv/bunExe, concurrent pipe drain, stdout asserted before exitCode, issue URL comment, it.concurrent), covers three digit-extension variants plus a letter-extension control, and exercises both import and computed require. The PR description reports it fails on main and passes with the fix, and that the full plugins/bundler_plugin suites pass. The one inline finding is an off-by-one in the adjacent extract_namespace drive-letter guard — pre-existing (git-blame attributes it to an earlier commit) and correctly flagged as such; it slightly widens in reach with this change but was already reachable for letter-extension paths on A:/Z: drives.
The strict inequalities excluded a/z/A/Z so a resolved path like Z:\share\file.js had 'Z' mis-parsed as a plugin namespace on Windows. Use the canonical bun_paths::resolve_path::is_drive_letter helper (inclusive bounds) instead of the hand-rolled range.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 9:33 PM PT - Jul 31st, 2026
✅ @robobun, your commit c02e8bda96973c400f9cf55ead2bc2f4125c953d passed in 🧪 To try this PR locally: bunx bun-pr 36592That installs a local version of the PR into your bun-36592 --bun |
There was a problem hiding this comment.
LGTM — the widened pre-filter is strictly safer (false positives just run the regex filters; the ./ ../ ..\ cases are still rejected via the separator/empty check), and the extract_namespace follow-up now uses the shared inclusive is_drive_letter helper as suggested.
What was reviewed:
- Traced
./,../,..,.\, and trailing-.through the newcould_be_plugin— all still fall through to the absolute/colon fallback as before. - Checked all four call sites (
linker.rs,VirtualMachine.rs,ModuleLoader.rs,jsc_hooks.rs) — none rely on the narrower old semantics;trueonly means "run the regex filters". - Confirmed
bun_paths::resolve_path::is_drive_letter(u8)is the inclusivea..=z | A..=Zcheck. - Test follows harness conventions (tempDir, concurrent pipe drain, stdout-before-exitCode) and covers import + require with a letter-extension control.
Extended reasoning...
Overview
Two-line semantic change in src/bundler/transpiler.rs plus a subprocess test in test/js/bun/plugin/plugins.test.ts. PluginRunner::could_be_plugin previously required the byte after the final . to be an ASCII letter or non-ASCII, which silently excluded digit-leading extensions (.1, .3gp) from ever reaching runtime plugin onLoad filters. The new check accepts any non-empty extension whose first byte is not a path separator, which is exactly what the original comment said the check was for. The second hunk swaps a hand-rolled strict-inequality drive-letter range in extract_namespace for the shared bun_paths::resolve_path::is_drive_letter helper — the follow-up to my earlier inline comment on this PR, now resolved.
Security risks
None. This is a pure byte-level pre-filter over module specifiers. Widening it only causes more specifiers to be tested against user-registered plugin filter regexes; the actual match decision is still made by those regexes. No new untrusted-input parsing, allocation, or filesystem access.
Level of scrutiny
Low-to-moderate. could_be_plugin is a hot-path fast-reject, so the concern is behavioral regression rather than safety. I hand-traced the specifiers the old comment named — ./, ../, .., and Windows \\ variants — and each still yields either an empty extension or one starting with //\\, so they still fall through to the !is_absolute && has_colon branch and return false exactly as before. Everything newly returning true (digits, other ASCII punctuation) is harmless: the caller just runs the plugin regex filters, which either match (correct) or don't (same outcome as before, one regex test slower). The four call sites were checked and none depend on the narrower old behavior.
Other factors
Both prior review threads (the drive-letter off-by-one and the comment-cop note) are resolved in the current diff. The test is well-formed per REVIEW.md: issue URL comment, tempDir/bunEnv/bunExe, concurrent stdout/stderr/exited drain, exact-value toEqual on parsed JSON before the exit-code assertion, it.concurrent, and it exercises both the ESM (await import) and CJS (require with a computed specifier to defeat static rewriting) paths alongside a letter-extension control that would have passed on main. The PR description states it fails on main and passes with the fix, and that the full plugins.test.ts and bundler_plugin.test.ts suites pass.
There was a problem hiding this comment.
LGTM — the extract_namespace off-by-one is now fixed via is_drive_letter, and the redundant inline comment was dropped.
What was reviewed:
could_be_pluginwidening: traced..,./,../,.\, lone., and trailing-dot inputs through the new check — all still fall through to the!is_absolute && has_colonpath as before.- Confirmed
is_drive_letter_tuses inclusive bounds, soA:/Z:now correctly return an empty namespace on Windows. - Checked all
could_be_plugincallers (ModuleLoader, VirtualMachine, jsc_hooks, linker) — widening only means more specifiers reach the regex filter loop, which is the intended behavior; no path skips normal resolution on filter miss.
Extended reasoning...
Overview
Two-line change to PluginRunner::could_be_plugin in src/bundler/transpiler.rs that relaxes the extension pre-filter from "starts with an ASCII letter or non-ASCII byte" to "non-empty and does not start with a path separator". This lets file extensions beginning with a digit (.1, .3gp) reach onLoad plugin filters. A follow-up commit also replaced the adjacent hand-rolled drive-letter range in extract_namespace with the shared bun_paths::resolve_path::is_drive_letter helper (fixing an off-by-one that excluded a/z/A/Z). One new subprocess test in test/js/bun/plugin/plugins.test.ts.
Security risks
None. This is a pure byte-level pre-filter that only decides whether to run user-registered plugin regex filters; it does not touch auth, crypto, network, or filesystem writes. Widening the pre-filter cannot bypass anything — the actual plugin filter regex still has to match.
Level of scrutiny
Low-to-moderate. The change is a strict widening of a fast-path guard whose only documented purpose is to reject ./, ../, and absolute paths cheaply. I walked every relative-navigation form (., .., ./, ../, .\, ..\) through the new predicate and they all still return false. Newly-accepted inputs (digit/symbol-leading extensions) simply proceed to the regex filter loop; on no match, callers fall through to normal resolution exactly as before, so there is no behavior change for specifiers without a matching plugin.
Other factors
The test follows harness conventions closely (tempDir, bunEnv/bunExe, concurrent pipe drain, stdout asserted before exit code, issue-URL comment, it.concurrent, letter-extension control case, both import and require paths). Both prior review comments (mine on the drive-letter bounds, comment-cop on the inline comment) were addressed in follow-up commits and the threads are resolved. The is_drive_letter helper was verified to use inclusive <= bounds.
|
Heads-up from #40465. It routes every resolved absolute path to the |
This branch has not been deployed
2 participants
Fixes #4609.
Repro
bun --preload ./preload.ts index.tsfires the.customfilter but never fires the.1filter;ok.yaml.1falls through to the built-infileloader.Cause
PluginRunner::could_be_pluginis the cheap pre-filter that decides whether to run plugin regex filters at all. It looked at the byte immediately after the final.and only returned true for ASCII letters (or non-ASCII bytes). For an absolute resolved path like/app/foo.1or/app/video.3gpthat byte is a digit, so the function returned false andrun_on_load_pluginswas skipped entirely.The comment on the check says its only purpose is to rule out
./,../,..(and their Windows\\forms). Those cases produce either an empty extension or one that starts with a path separator.Fix
Accept any non-empty extension that does not start with
/or\\. This keeps the intended fast-path rejection of relative-navigation specifiers while letting numeric (.1,.h2,.3gp) and other legal extensions through to the regex filters.Verification
New test in
test/js/bun/plugin/plugins.test.tscovers.1,.txt.2,.3gp(via bothimportandrequire) alongside a letter-extension control. Fails on main with the digit-extension values coming back undefined, passes with this change. Fullplugins.test.ts(36 tests) andbundler_plugin.test.ts(52 tests) pass.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file