Remove unpkg.com dependency and serve manifold assets locally#396
Open
tracygardner wants to merge 3 commits intomainfrom
Open
Remove unpkg.com dependency and serve manifold assets locally#396tracygardner wants to merge 3 commits intomainfrom
tracygardner wants to merge 3 commits intomainfrom
Conversation
manifold-3d's Emscripten-compiled JS calls locateFile for both the .wasm binary and any .js companion files (e.g. worker scripts). The .wasm was already redirected to the local wasm/ directory, but the .js file fell through and was resolved against the package's original unpkg.com base URL, requiring https://unpkg.com in both script-src and connect-src. Fix: - Copy node_modules/manifold-3d/manifold.js to wasm/ at build time via viteStaticCopy (alongside the existing manifold.wasm copy). - Extend locateFile in api/shapes.js to redirect .js files to wasm/<name> as well, so all manifold-3d assets are served from self. - Remove https://unpkg.com from script-src and connect-src in the CSP (vite.config.mjs, index.html, docs/CSP_POLICY.md). https://claude.ai/code/session_01AemiiYusMHWjBimfoovWBp
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTightens Content-Security-Policy entries (removes unpkg.com) and exposes a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant Shapes as api/shapes.js
participant Wasm as Manifold WASM
participant Flock as flock.js
rect rgba(200,230,201,0.5)
App->>Shapes: import { getManifold }
end
rect rgba(187,222,251,0.5)
Shapes->>Wasm: instantiate/preload manifold WASM
Wasm-->>Shapes: return manifoldInstance, manifoldMeshInstance
end
rect rgba(255,224,178,0.5)
App->>Flock: call InitializeCSG2Async()
Flock->>Shapes: getManifold()
Shapes-->>Flock: { manifoldInstance, manifoldMeshInstance }
Flock->>Wasm: InitializeCSG2Async({ manifoldInstance, manifoldMeshInstance })
Wasm-->>Flock: initialization complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/CSP_POLICY.md`:
- Line 39: The CSP connect-src documented in CSP_POLICY.md is inconsistent with
the runtime policy emitted by the build (the Vite config and index HTML include
a broad "https:" scheme); either update the documented connect-src line in
CSP_POLICY.md to include the https: scheme alongside the existing hosts, or
remove the generic "https:" allowance from the runtime policy in the Vite
configuration and from the meta tag in index HTML so the implementation matches
the restrictive list in the doc—locate the connect-src declaration in
CSP_POLICY.md and the corresponding connect-src values produced by the Vite
config and index HTML and make the values identical.
In `@vite.config.mjs`:
- Line 13: The CSP string in CSP_META_POLICY contains a broad connect-src token
"https:" which allows any HTTPS origin; update the CSP_META_POLICY definition to
remove the "https:" scheme from the connect-src directive and instead list only
the explicit required origins (e.g., https://www.googletagmanager.com,
https://www.google-analytics.com, https://region1.google-analytics.com,
https://stats.g.doubleclick.net) or, if you intentionally want to permit
arbitrary HTTPS endpoints, add a clear comment above CSP_META_POLICY documenting
that decision and its risks; locate and edit the CSP_META_POLICY constant in
vite.config.mjs and adjust the connect-src clause accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70d948f0-ee70-4765-8a2c-15ab55e2c4c2
📒 Files selected for processing (4)
api/shapes.jsdocs/CSP_POLICY.mdindex.htmlvite.config.mjs
| img-src 'self' data: blob: https://www.google-analytics.com https://www.googletagmanager.com; | ||
| font-src 'self' data:; | ||
| connect-src 'self' https://www.googletagmanager.com https://www.google-analytics.com https://region1.google-analytics.com https://stats.g.doubleclick.net https://unpkg.com; | ||
| connect-src 'self' https://www.googletagmanager.com https://www.google-analytics.com https://region1.google-analytics.com https://stats.g.doubleclick.net; |
There was a problem hiding this comment.
Documentation does not match actual CSP implementation.
The documented connect-src directive differs from what's defined in vite.config.mjs (line 13) and index.html (line 7). The implementation includes a broad https: scheme allowance that is not reflected in this documentation:
| Source | connect-src value |
|---|---|
| This doc | 'self' https://www.googletagmanager.com https://www.google-analytics.com ... |
| vite.config.mjs / index.html | 'self' https: https://www.googletagmanager.com ... |
Please either update the documentation to reflect the actual policy, or remove the https: from the implementation files to match this more restrictive documented policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CSP_POLICY.md` at line 39, The CSP connect-src documented in
CSP_POLICY.md is inconsistent with the runtime policy emitted by the build (the
Vite config and index HTML include a broad "https:" scheme); either update the
documented connect-src line in CSP_POLICY.md to include the https: scheme
alongside the existing hosts, or remove the generic "https:" allowance from the
runtime policy in the Vite configuration and from the meta tag in index HTML so
the implementation matches the restrictive list in the doc—locate the
connect-src declaration in CSP_POLICY.md and the corresponding connect-src
values produced by the Vite config and index HTML and make the values identical.
|
|
||
| // `frame-ancestors` is only enforced from HTTP headers (ignored in CSP meta tags). | ||
| const CSP_META_POLICY = "default-src 'self'; base-uri 'self'; form-action 'self'; object-src 'none'; script-src 'self' 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval' https://www.googletagmanager.com https://unpkg.com; style-src 'self' 'unsafe-inline'; img-src 'self' data: blob: https://www.google-analytics.com https://www.googletagmanager.com; font-src 'self' data:; connect-src 'self' https: https://www.googletagmanager.com https://www.google-analytics.com https://region1.google-analytics.com https://stats.g.doubleclick.net https://unpkg.com; media-src 'self' data: blob:; worker-src 'self' blob:; frame-src 'self'; manifest-src 'self'"; | ||
| const CSP_META_POLICY = "default-src 'self'; base-uri 'self'; form-action 'self'; object-src 'none'; script-src 'self' 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval' https://www.googletagmanager.com; style-src 'self' 'unsafe-inline'; img-src 'self' data: blob: https://www.google-analytics.com https://www.googletagmanager.com; font-src 'self' data:; connect-src 'self' https: https://www.googletagmanager.com https://www.google-analytics.com https://region1.google-analytics.com https://stats.g.doubleclick.net; media-src 'self' data: blob:; worker-src 'self' blob:; frame-src 'self'; manifest-src 'self'"; |
There was a problem hiding this comment.
connect-src includes overly broad https: scheme allowance.
The CSP policy contains connect-src 'self' https: ... which permits connections to any HTTPS endpoint. This undermines the security benefit of removing unpkg.com, since the policy now allows fetching from any external HTTPS origin.
If the intent is to allow arbitrary HTTPS connections (e.g., for user-provided URLs), this is acceptable but should be documented. Otherwise, consider removing https: and explicitly listing only the required origins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.mjs` at line 13, The CSP string in CSP_META_POLICY contains a
broad connect-src token "https:" which allows any HTTPS origin; update the
CSP_META_POLICY definition to remove the "https:" scheme from the connect-src
directive and instead list only the explicit required origins (e.g.,
https://www.googletagmanager.com, https://www.google-analytics.com,
https://region1.google-analytics.com, https://stats.g.doubleclick.net) or, if
you intentionally want to permit arbitrary HTTPS endpoints, add a clear comment
above CSP_META_POLICY documenting that decision and its risks; locate and edit
the CSP_META_POLICY constant in vite.config.mjs and adjust the connect-src
clause accordingly.
Instead of redirecting manifold-3d's .js companion files through locateFile (which served a raw copy that still had the hardcoded unpkg.com scriptDirectory, breaking WASM loading in workers), strip the hardcoded URL at import time with a Vite transform plugin. The manifoldUnpkgPatchPlugin transform hook matches node_modules/manifold-3d/ manifold.js and replaces 'https://unpkg.com/manifold-3d...' with '' so Emscripten falls back to deriving scriptDirectory from the script's own URL (local server in dev, the bundle chunk in prod). This means no fetch ever leaves the origin for manifold-3d assets, making the https://unpkg.com CSP allowance unnecessary. Also reverts the broken locateFile .js redirect (caused 'ManifoldMesh is not a constructor' because the raw worker copy tried to load WASM from unpkg.com which was then blocked by CSP) and removes the now-unneeded raw manifold.js static copy from viteStaticCopy. https://claude.ai/code/session_01AemiiYusMHWjBimfoovWBp
Root cause: BABYLON.InitializeCSG2Async() with no arguments fetches manifold-3d from https://unpkg.com/manifold-3d@3.3.0 at runtime. With unpkg.com removed from the CSP, that fetch is blocked, leaving BabylonJS without an initialized manifold module. Any subsequent CSG2 operation then throws "ManifoldMesh is not a constructor". Fix: InitializeCSG2Async accepts an options object with manifoldInstance and manifoldMeshInstance. We pre-load the wasm module via the existing getManifold() helper in api/shapes.js (which already serves the WASM from the locally-hosted /wasm/manifold.wasm via locateFile) and inject the resulting Manifold and Mesh constructors into BabylonJS, so it never makes an external request. Also removes the manifoldUnpkgPatchPlugin Vite transform that was targeting the wrong file — the unpkg.com load was coming from BabylonJS, not from the manifold-3d npm package itself. https://claude.ai/code/session_01AemiiYusMHWjBimfoovWBp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR removes the external dependency on unpkg.com for manifold-3d assets by serving both the WASM binary and its JavaScript companion from a local
wasm/directory. This improves security by eliminating an external origin from the Content Security Policy and reduces reliance on external CDNs.Key Changes
https://unpkg.comfrom bothscript-srcandconnect-srcdirectivesapi/shapes.jsto serve bothmanifold.wasmandmanifold.jsfrom the localwasm/directory instead of fetching from unpkg.comvite.config.mjsto copymanifold.jsalongside the existingmanifold.wasmfile during the build processImplementation Details
The
getManifold()function now intercepts both.wasmand.jsfile requests from the manifold-3d module and redirects them to the localwasm/directory. This ensures that the manifold library's initialization files are served locally, eliminating the need for external network access to unpkg.com while maintaining full functionality.https://claude.ai/code/session_01AemiiYusMHWjBimfoovWBp
Summary by CodeRabbit