fix(workerd): guard fileURLToPath(import.meta.url) for bundled modules - #2910
fix(workerd): guard fileURLToPath(import.meta.url) for bundled modules#2910JamesbbBriz wants to merge 5 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 544edb2eae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5d9b461 to
72ffa69
Compare
|
@james-elicx I rebased this onto the latest main, resolved the index.ts conflict, and updated the build integration coverage for the current split/minified server output. The Codex P2 feedback remains addressed; targeted checks pass locally (18/18). Could you please kick off a Big Bonk review when you have a moment? Thanks! |
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk |
|
CI follow-up: the failed prefetch test assumed async RSC URL generation completed in call order, so it always treated dashboard-4 as the queued request. The new regression deliberately completes the first digest last, identifies the actual queued cache entry from the four issued request URLs, and still asserts navigation promotes exactly that fifth request. Local verification: deterministic old 4-vs-5 failure, focused test 10/10 repeated, full prefetch-cache file 76/76, and scoped check/diff-check pass. Waiting for CI; not requesting re-review yet. |
workerd does not provide import.meta.url for bundled modules (it is
undefined), so libraries that resolve paths from it at module init —
e.g. findUp-style package.json discovery — crash with "The path argument
must be of type string or an instance of URL". Node never sees this
(import.meta.url is always defined there).
Adds a narrow transform plugin that guards only the two crashing call
shapes — fileURLToPath(import.meta.url) and createRequire(import.meta.url)
— rewriting the argument to `import.meta.url ?? "file:///"`. All other
import.meta.url usage is untouched, and the guard is a no-op on Node.
Verified under local workerd (same binary as real Workers): unguarded
fileURLToPath(undefined) crashes module init; guarded, fileURLToPath("file:///")
yields "/" and findUp terminates safely. End-to-end: a full CMS stack
(Payload) passes its 25-check API suite with the app-level workaround
removed — 7 guard sites applied in the bundle, 0 missed.
Adversarial verification found the bounded 80-char window between the callee and the argument silently misses calls separated by long block comments (3+ lines), leaving them unguarded in real builds. Widen the filter to an unbounded window — the AST handler remains authoritative, so the cost is only an extra parseAst on false positives. Adds a filter-level regression test for long-comment forms.
… guard Adds a fixture with a dependency that calls fileURLToPath(import.meta.url) behind a long block comment (the window-escape shape), and a build-level test asserting the guard lands in the emitted server bundle while no bare call remains. Covers the filter+handler pipeline end to end — the unit suite alone bypasses the filter gate.
8c2377f to
008f7a9
Compare
|
Rebase update: latest main now contains the authoritative queued-prefetch stabilization from #2920 (�dbb322f), which fixes the same async setup-order assumption. During rebase our duplicate local test commit conflicted with that upstream fix, so I dropped the duplicate and kept #2920's simpler four-slots-first regression. Rebased #2910 head verification: queued promotion target passes; workerd guard/build tests 20/20; deploy/build matrix 467 pass with the single existing Windows symlink EPERM host limitation; scoped checks and diff-check pass. CI should now run from the conflict-free head. No re-review request yet. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 008f7a9e43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@james-elicx I think this is ready for another look now. I fixed the workerd issue from the latest review, and CI is green. Thanks! |
1 participant
What & why
While dogfooding a CMS on vinext + Cloudflare Workers, we hit a hard crash: workerd exposes
import.meta.urlas the non-file sentinelworkerfor bundled modules. Libraries that resolve paths from it at module init therefore throw infileURLToPath(import.meta.url)orcreateRequire(import.meta.url)and fail to load.Node receives a normal
file:URL, which is why this is workerd-specific and worth handling centrally instead of patching applications one by one.What it does
fileURLToPath(import.meta.url)andcreateRequire(import.meta.url).file:URLs and falls back tofile:///for workerd''sworkersentinel or a missing value.import.meta.urlsemantics remain untouched.import.meta.urlusage is untouched.Next.js research and deliberate divergence
Searched Next.js canary under
packages/next/src,test/e2e, andtest/unit. The tree contains Node-oriented direct usages such asmodule.createRequire(import.meta.url)and fixtures usingfileURLToPath(import.meta.url), but no workerd guard or equivalent transform. This is an intentional Cloudflare/workerd-specific divergence: vinext must keep bundled module initialization alive when workerd supplies a non-file module identifier.Verification
vp test run tests/workerd-import-meta-url-guard.test.ts tests/workerd-import-meta-url-build.test.ts: 24 passed.EPERM). Ubuntu CI is running the authoritative matrix.vp checkon all changed files andgit diff --checkpass.import.meta.url === worker, verifies the fallback, preserves a normalfile:URL, and verifies client exclusion.Known boundaries
fileURLToPathgets the same rewrite.import.meta.url as string) and computed access are intentionally out of scope.file:///fallback is only meaningful under workerd''s POSIX file URL handling.