Conversation
briantstephan
requested review from
asanehisa,
benlife5,
jwartofsky-yext and
mkilpatrick
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request updates license metadata and dependency versions across the project. The rollup package is bumped from version 4.27.2 to 4.59.0 in package.json and license notices. Third-party license files are updated to reflect corrected author attributions (including 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pages/package.json`:
- Line 79: Upgrade to Rollup v4.59.0 can error if generated output paths escape
the configured output directory; run the project build with the updated "rollup"
dependency and verify no rollup validation errors occur by checking all places
that configure output names or emit files—specifically review usages of
output.entryFileNames, output.chunkFileNames, output.assetFileNames,
manualChunks, any plugin calls to emitFile(), and functions like
preserveEntrySignatures or sanitizeFileName—to ensure none produce "../" or
paths that resolve outside the output dir; if any do, change those patterns to
safe relative names inside the output directory or sanitize/normalize emitted
file names so they no longer escape the configured output path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f82e41f5-89d6-4f81-810c-0379c0fc0477
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
THIRD-PARTY-NOTICESpackages/pages/THIRD-PARTY-NOTICESpackages/pages/package.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/pages/src/vite-plugin/build/build.ts (1)
83-104: Consider centralizing duplicated subpath sanitization logic.This helper is effectively duplicated from
packages/pages/src/common/src/assets/getAssetsFilepath.ts(sanitizeAssetsDir). Extracting a shared utility would reduce divergence risk in path-safety behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages/src/vite-plugin/build/build.ts` around lines 83 - 104, The sanitization logic in sanitizeOutputSubpath duplicates sanitizeAssetsDir; extract the common behavior into a shared utility (e.g., export a new sanitizeSubpath function) and have sanitizeOutputSubpath and the existing sanitizeAssetsDir call that shared function instead; ensure the shared function preserves the exact normalization steps (trim, strip Windows drive, remove leading slashes, posix normalize, remove leading ./ and reject empty/. or ..) and update imports in build.ts and the module that currently defines sanitizeAssetsDir to use the new utility so both call the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pages/src/common/src/assets/getAssetsFilepath.ts`:
- Around line 27-29: The docstring for getAssetsFilepath.ts incorrectly states
that absolute asset paths "fall back", but the implementation in
getAssetsFilepath actually normalizes/sanitizes absolute paths into safe
relative paths instead of falling back; update the comment above the
getAssetsFilepath function to describe the real behavior: that empty values or
paths that would escape the output dir fall back to the default, while absolute
paths are converted/sanitized into a safe relative subpath for Rollup output
(include mention of the normalization/sanitization step so the doc matches the
code).
In `@packages/pages/src/vite-plugin/modules/plugin.ts`:
- Line 68: The current sanitizeModuleEntryName drops parent segments (using
basename) causing different moduleName values to collide; update the logic
around sanitizeModuleEntryName and its usage where safeModuleEntryName is
created so sanitized output filenames remain unique by incorporating a
deterministic short identifier derived from the original moduleName (for example
append a short hash or base64 of moduleName) or preserve path segments (replace
path separators with safe characters) instead of only using basename; ensure the
change is applied to the places that compute safeModuleEntryName and the code
path that currently strips parents so generated filenames cannot overwrite one
another.
---
Nitpick comments:
In `@packages/pages/src/vite-plugin/build/build.ts`:
- Around line 83-104: The sanitization logic in sanitizeOutputSubpath duplicates
sanitizeAssetsDir; extract the common behavior into a shared utility (e.g.,
export a new sanitizeSubpath function) and have sanitizeOutputSubpath and the
existing sanitizeAssetsDir call that shared function instead; ensure the shared
function preserves the exact normalization steps (trim, strip Windows drive,
remove leading slashes, posix normalize, remove leading ./ and reject empty/. or
..) and update imports in build.ts and the module that currently defines
sanitizeAssetsDir to use the new utility so both call the single implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e144f235-c36b-4c8a-b21f-c3716d146f68
📒 Files selected for processing (4)
packages/pages/src/common/src/assets/getAssetsFilepath.test.tspackages/pages/src/common/src/assets/getAssetsFilepath.tspackages/pages/src/vite-plugin/build/build.tspackages/pages/src/vite-plugin/modules/plugin.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32a79369-ed3d-4c79-b4c7-64f7f5cce39c
📒 Files selected for processing (4)
packages/pages/src/common/src/assets/getAssetsFilepath.tspackages/pages/src/common/src/assets/sanitizeSubpath.tspackages/pages/src/vite-plugin/build/build.tspackages/pages/src/vite-plugin/modules/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/pages/src/vite-plugin/build/build.ts
- packages/pages/src/common/src/assets/getAssetsFilepath.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/pages/src/vite-plugin/modules/plugin.ts (1)
229-236: HardensanitizeModuleEntryNameagainst filesystem-reserved characters.Current normalization removes traversal segments, but names can still contain characters invalid on common filesystems (for example
:or*). Consider replacing disallowed characters to keep output names portable.🛠️ Suggested hardening
const sanitizeModuleEntryName = (moduleName: string): string => { const normalized = path.posix.normalize(moduleName.replace(/\\/g, "/")).replace(/^(\.\/)+/, ""); const safeName = normalized .split("/") .filter((segment) => segment !== "" && segment !== "." && segment !== "..") - .join("-"); + .join("-") + .replace(/[<>:"/\\|?*\x00-\x1F]/g, "-") + .replace(/-+/g, "-") + .replace(/^-|-$/g, ""); return safeName.length > 0 ? safeName : "module"; };
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pages/src/common/src/assets/getAssetsFilepath.ts`:
- Around line 7-10: Update the docstring in getAssetsFilepath to accurately
describe the implemented behavior: replace references to "vite.config.json" and
"assetDir" with "vite.config.js" and "build.assetsDir", and keep the notes about
falling back to the default "assets", rejecting empty values or paths that
escape the output directory, and sanitizing absolute paths into safe relative
subpaths for Rollup output; ensure the function name getAssetsFilepath is
mentioned so readers can find the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f8c0b12-eaf0-4570-8e98-ccf772e50ecc
📒 Files selected for processing (6)
packages/pages/src/common/src/assets/getAssetsFilepath.tspackages/pages/src/common/src/assets/sanitizeSubpath.test.tspackages/pages/src/common/src/assets/sanitizeSubpath.tspackages/pages/src/vite-plugin/build/build.tspackages/pages/src/vite-plugin/modules/plugin.test.tspackages/pages/src/vite-plugin/modules/plugin.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/pages/src/common/src/assets/sanitizeSubpath.ts
- packages/pages/src/vite-plugin/build/build.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pages/src/vite-plugin/modules/plugin.ts`:
- Around line 49-50: The check using "in" on the filepaths object can match
prototype keys and skip valid module names; update the existence test around
resolvedModuleName in plugin.ts to use an own-property check (e.g.,
Object.prototype.hasOwnProperty.call(filepaths, resolvedModuleName) or
filepaths.hasOwnProperty resolved via safe call) before setting
filepaths[resolvedModuleName] = { path: filepath, name: name } so prototype keys
like "toString" or "constructor" don't cause false positives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4541b710-a44d-43a6-b541-06bca83269e2
📒 Files selected for processing (1)
packages/pages/src/vite-plugin/modules/plugin.ts
3 participants
https://yext.atlassian.net/browse/VULN-42410