Conversation
🦋 Changeset detectedLatest commit: 40cab87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Type Coverage ReportCoverage Comparison
Files with Most Type Issues (Top 15)
This report was generated by the Type Coverage GitHub Action |
| } catch (err: any) { | ||
| if (err.message) { | ||
| } catch (err: unknown) { | ||
| if (err instanceof Error && err.message) { |
There was a problem hiding this comment.
Pretty minor, but best to avoid changing implementation where possible. If we change impl, it needs to be feature flagged and becomes a whole thing
There was a problem hiding this comment.
I don't entirely disagree, but I think this is the standard pattern in typescript for dealing with errors. I don't think catch (err: Error) is allowed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // support for source maps is better with eval. | ||
| (0, eval)(asset.output); | ||
| if (asset.output) { | ||
| (0, eval)(asset.output); |
Check failure
Code scanning / CodeQL
Code injection Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
To fix this issue, we should avoid blindly evaluating arbitrary code received from a network source (even in a local development context). The safest mitigation is to eliminate or strictly limit dynamic code execution via eval. For HMR systems, one standard solution is to validate both the type and contents of incoming assets, and to only allow code that matches expected patterns, or ideally, to load updated modules via <script> tags (allowing built-in CSP and browser script loading protections) rather than evaluating strings.
For the context here:
- Instead of
(0, eval)(asset.output);, inject a<script>tag with the code if it is JavaScript and is from a trusted source. - At minimum, verify that the WebSocket origin is trusted before processing updates.
- If dynamic eval is absolutely required for HMR (which is commonly the case only in development), make sure to restrict communications to authenticated channels and/or validate the format of
asset.outputto only correspond to bundles you expect. - If using the
<script>method:- Create a
<script>element - Set text content to
asset.output - Set nonce/integrity if appropriate
- Add to
document.head - Remove after execution
- Create a
Necessary changes:
- Replace the direct call to
eval(asset.output)at line 579 in packages/runtimes/hmr/src/loaders/hmr-runtime.ts with a script injection pattern. - Add a helper function for safe script injection if necessary.
- Ensure that the asset type is 'js' before any code-injection logic is performed (already checked, but can reinforce).
- Document that this change is only a partial mitigation, and the overall HMR protocol should have authentication/integrity checks.
| @@ -576,7 +576,13 @@ | ||
| // Global eval. We would use `new Function` here but browser | ||
| // support for source maps is better with eval. | ||
| if (asset.output) { | ||
| (0, eval)(asset.output); | ||
| // safer: inject script tag rather than direct eval | ||
| const script = document.createElement('script'); | ||
| script.textContent = asset.output; | ||
| script.type = 'text/javascript'; | ||
| // Optionally: set nonce and/or other attributes as per CSP requirements | ||
| document.head.appendChild(script); | ||
| document.head.removeChild(script); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It's worth checking if HMR still works (you can setup a simple standalone project and use atlaspack-link) as I believe this file ends up running in the browser - this would be why the original used comment based flow types. Or at least you'd need to make sure that the client side is "compiled" so that all of the type checks are removed.
| } catch (err: any) { | ||
| if (err.message) { | ||
| } catch (err: unknown) { | ||
| if (err instanceof Error && err.message) { |
There was a problem hiding this comment.
I don't entirely disagree, but I think this is the standard pattern in typescript for dealing with errors. I don't think catch (err: Error) is allowed.
4 participants
Motivation
Remove ts-expect-errors in hmr-runtime.ts
Changes
This is all rovodev's work with Marcin's prompt
Checklist
docs/folder