fix(shims): avoid jsx-runtime named exports in error boundary#45
fix(shims): avoid jsx-runtime named exports in error boundary#45liuxiaopai-ai wants to merge 2 commits intocloudflare:mainfrom
Conversation
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The core fix is correct — replacing JSX syntax with React.createElement() in error-boundary.tsx eliminates the implicit dependency on named exports from react/jsx-runtime, which is the root cause of #25. The test is solid and the approach is sound.
However, this fix is incomplete. The same vulnerability exists in 6 other .tsx shim files that still use JSX syntax:
| File | JSX usage |
|---|---|
link.tsx |
<LinkStatusContext.Provider>, <a> |
metadata.tsx |
80+ JSX elements (<meta>, <title>, <link>, <>) |
legacy-image.tsx |
<Image> |
form.tsx |
<form> |
document.tsx |
<html>, <head>, <body>, <div> |
image.tsx |
<img>, <UnpicImage> |
All of these will fail with the same SyntaxError: does not provide an export named 'jsx' under the same conditions described in #25. The fix should either be applied consistently across all shim files, or (better) the root cause should be addressed at the Vite config level so JSX compilation works regardless of user tsconfig.
Two inline comments below on the test.
| "../packages/vinext/src/shims/error-boundary.tsx", | ||
| ); | ||
|
|
||
| await fs.writeFile(jsxRuntimePath, "export default {};\n"); |
There was a problem hiding this comment.
This mock doesn't faithfully reproduce the failure scenario from #25. The issue is that react/jsx-runtime exposes jsx/jsxs/Fragment only via a default export (CJS interop), not as named exports. An empty default export (export default {};) won't trigger the same code path — if the module loaded without JSX, there's nothing that would call jsx() on this object.
A more faithful mock would export a default with the expected shape but no named exports, e.g.:
| await fs.writeFile(jsxRuntimePath, "export default {};\n"); | |
| await fs.writeFile(jsxRuntimePath, "const jsx = () => null; const jsxs = () => null; const Fragment = 'div'; export default { jsx, jsxs, Fragment };\n"); |
That said, the current test still validates what matters (module loads without crashing), so this is a minor nit.
There was a problem hiding this comment.
Fixed in 5847f3d. Updated the mock to export a default object with jsx/jsxs/Fragment to better match the failure mode.
| }); | ||
|
|
||
| const mod = await server.ssrLoadModule(shimPath); | ||
| expect(typeof mod.ErrorBoundary).toBe("function"); |
There was a problem hiding this comment.
Consider also asserting that instantiation/rendering works, not just that the exports are functions. For example, you could verify that React.createElement(mod.ErrorBoundary, { fallback: () => null }, 'child') doesn't throw. Right now, if someone accidentally broke the constructor signature, this test would still pass.
There was a problem hiding this comment.
Good call — also addressed in 5847f3d. Added an instantiation assertion for ErrorBoundary in the runtime-compat test, in addition to export-shape checks.
|
Review posted successfully. Here's a summary of the feedback: The fix itself is correct — converting JSX to Key concern: the fix is incomplete. Six other Minor test feedback:
|
|
Thanks for the detailed review — I pushed a follow-up commit to address the two inline test nits:
Commit: 5847f3d On scope: agreed this pattern can exist in other shim If you'd prefer, I can also expand this PR to cover all affected shim files directly. |
|
Thanks @liuxiaopai-ai! Closing in favor of #155 |
4 participants
Addresses #25.