fix(server): include external workspace package roots in fs allow#21703
fix(server): include external workspace package roots in fs allow#21703Vishnuuuu24 wants to merge 2 commits intovitejs:mainfrom
Conversation
|
Local verification completed after latest changes:\n- pnpm vitest run src/node/server/tests/search-root.spec.ts (7/7 passing)\n- pnpm run typecheck (packages/vite)\n- pnpm run typecheck (repo root)\n\nThis covers the new pnpm and package.json workspace-parent-directory regression cases. |
|
Updated PR with a lockfile fix for CI formatting:\n- added missing importer entries in pnpm-lock.yaml for new fixture package.json files\n\nThis addresses the failing "Check formatting" job diff on pnpm-lock.yaml. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where Vite's searchForWorkspaceRoot function incorrectly blocked access to workspace packages located outside the workspace root directory. The fix parses workspace configuration files (pnpm-workspace.yaml and package.json workspaces field) to extract package patterns, computes their base directories, and determines the common ancestor of all workspace packages as the true workspace root.
Changes:
- Enhanced
searchForWorkspaceRootto parse workspace configurations and compute the common ancestor of all package locations - Added comprehensive YAML parsing for
pnpm-workspace.yamlwithout external dependencies - Added JSON parsing for
package.jsonworkspaces field with support for both array and object formats - Added test cases and fixtures for pnpm and npm workspaces with external packages
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vite/src/node/server/searchRoot.ts | Core implementation: added workspace pattern parsing, base directory extraction, and common ancestor computation |
| packages/vite/src/node/server/tests/search-root.spec.ts | Added test cases for pnpm and npm workspaces with packages outside the workspace directory |
| packages/vite/src/node/server/tests/fixtures/pnpm-outside/workspace/pnpm-workspace.yaml | Test fixture for pnpm workspace with external packages |
| packages/vite/src/node/server/tests/fixtures/pnpm-outside/workspace/my-app/package.json | Test fixture package.json for nested app |
| packages/vite/src/node/server/tests/fixtures/pnpm-outside/packages/my-lib/package.json | Test fixture package.json for external library |
| packages/vite/src/node/server/tests/fixtures/npm-outside/workspace/package.json | Test fixture for npm/yarn workspace with external packages |
| packages/vite/src/node/server/tests/fixtures/npm-outside/workspace/my-app/package.json | Test fixture package.json for nested app |
| packages/vite/src/node/server/tests/fixtures/npm-outside/packages/my-lib/package.json | Test fixture package.json for external library |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| return root | ||
| } | ||
|
|
||
| if (basename(base).includes('.')) { |
There was a problem hiding this comment.
The heuristic basename(base).includes('.') assumes that paths with dots in the basename are files rather than directories. While this works for typical cases, directories can have dots in their names (e.g., packages/v1.0/). This could cause the function to incorrectly use the parent directory. Consider checking if the path actually exists on the filesystem and is a directory, or documenting this limitation. However, the impact is limited: it would just make the allow list slightly more permissive than necessary, which is safe.
| if (basename(base).includes('.')) { | |
| const basePath = resolve(root, base) | |
| let isDirectory = false | |
| try { | |
| isDirectory = fs.statSync(basePath).isDirectory() | |
| } catch { | |
| // If the path does not exist or can't be stat'ed, fall back to the heuristic below. | |
| } | |
| if (!isDirectory && basename(base).includes('.')) { |
| function getCommonAncestor(paths: string[]): string { | ||
| if (!paths.length) { | ||
| return '' | ||
| } | ||
|
|
||
| let ancestor = resolve(paths[0]) | ||
| for (const path of paths.slice(1).map((path) => resolve(path))) { | ||
| while (ancestor !== dirname(ancestor)) { | ||
| const relation = relative(ancestor, path) | ||
| if ( | ||
| relation === '' || | ||
| (!relation.startsWith('..') && !isAbsolute(relation)) | ||
| ) { | ||
| break | ||
| } | ||
| ancestor = dirname(ancestor) | ||
| } | ||
| } |
There was a problem hiding this comment.
On Windows, if workspace packages are located on different drives, the function may return incorrect results. When paths are on different drives, relative() returns an absolute path, and the algorithm will eventually terminate at the root of the first path's drive, even though that drive doesn't contain the other paths. While this is an extremely rare edge case in practice, consider adding a check or documenting this limitation.
1 participant
Fixes #21700.\n\nsearchForWorkspaceRoot now resolves workspace package patterns from pnpm-workspace.yaml and package.json workspaces/workspaces.packages, computes their common ancestor, and uses that as workspace root. This allows valid layouts with ../packages/* to be included in fs allow.\n\nIncludes new regression fixtures/tests for pnpm and npm-style workspaces that reference parent directories.\n\nValidation:\n- pnpm vitest run src/node/server/tests/search-root.spec.ts (packages/vite)\n- pnpm run typecheck (packages/vite)\n- pnpm run typecheck (repo root)