fix(modernjs): harden SSR bundle serving against path traversal - #4983
fix(modernjs): harden SSR bundle serving against path traversal#4983tonoizer wants to merge 2 commits into
Conversation
Confine /bundles and SSR JSON middleware to their roots and set Content-Length from UTF-8 byte length so non-ASCII federated chunks are not truncated. Co-authored-by: Cursor <cursoragent@cursor.com>
🦋 Changeset detectedLatest commit: 54021d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 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 |
![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: 8903f302b1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8903f302b1
ℹ️ 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".
@module-federation/devtools
@module-federation/cli
create-module-federation
@module-federation/dts-plugin
@module-federation/enhanced
@module-federation/error-codes
@module-federation/esbuild
@module-federation/managers
@module-federation/manifest
@module-federation/metro
@module-federation/metro-plugin-rnc-cli
@module-federation/metro-plugin-rnef
@module-federation/metro-plugin-rock
@module-federation/modern-js
@module-federation/modern-js-v3
@module-federation/native-federation-tests
@module-federation/native-federation-typescript
@module-federation/nextjs-mf
@module-federation/node
@module-federation/observability-plugin
@module-federation/playground
@module-federation/retry-plugin
@module-federation/rsbuild-plugin
@module-federation/rspack
@module-federation/rspress-plugin
@module-federation/rstest
@module-federation/runtime
@module-federation/runtime-core
@module-federation/runtime-tools
@module-federation/sdk
@module-federation/storybook-addon
@module-federation/third-party-dts-extractor
@module-federation/treeshake-frontend
@module-federation/treeshake-server
@module-federation/typescript
@module-federation/utilities
@module-federation/webpack-bundler-runtime
@module-federation/bridge-react
@module-federation/bridge-react-webpack-plugin
@module-federation/bridge-shared
@module-federation/bridge-vue3
@module-federation/inject-external-runtime-core-plugin
commit: |
Bundle Size Report8 package(s) changed, 35 unchanged. Package dist + ESM entry
Bundle targets
Consumer scenarios
Total dist (raw): 35.88 MB (+2.0 kB (+0.0%)) Bundle sizes are generated with rslib (Rspack). Package-root metrics preserve the historical report. Tracked subpath exports such as |
|
Thanks for working on this. The path traversal and UTF-8 Content-Length issues addressed by this PR are valid. However, the current PR does not pass the @module-federation/modern-js-v3 test suite. In ssrPlugin.spec.ts, dev.setupMiddlewares is configured as an array of handlers, but the test harness config.dev.setupMiddlewares(middlewares); This causes all three ssrPlugin tests to fail with TypeError: config.dev.setupMiddlewares is not a function. Separately, we are currently planning the future direction of the Modern.js Module Federation integration. This includes reconsidering the ownership and design of its SSR asset-serving We will revisit these security and correctness fixes as part of that broader planning. For now, we are going to close this PR. Thanks again for identifying and working on these issues. |
2 participants
Summary
/bundlesstatic middleware to the bundles root (reject../ escapes) in both@module-federation/modern-jsand@module-federation/modern-js-v3..jsonextension, strip query/hash, and keep reads underdist/.Content-Lengthfrom UTF-8 byte length (not JS string length) so non-ASCII federated chunks are not truncated, and charge the file cache by byte size.Testing
pnpm --filter @module-federation/modern-js exec rstest src/server/staticMiddleware.spec.ts(15/15)pnpm --filter @module-federation/modern-js-v3 exec rstest src/server/staticMiddleware.spec.ts(15/15)ESLINT_USE_FLAT_CONFIG=false)Risks
path.resolveunder the bundles/dist roots.Related
Content-Lengthbytes formodern-js-v3. This PR includes that fix for bothmodern-jsandmodern-js-v3, plus path-traversal confinement for/bundlesand the SSR JSON middleware, and file-cache byte sizing.