refactor: migrate to new path utils package#241
Conversation
* Added import for `PathUtilsBaseError` to handle specific errors. * Updated `handleError` function to throw `PathUtilsBaseError` when encountered. * Improved error message handling for unexpected errors.
…orts * Deleted `resolveSafePath` and `isWithinBase` functions from `utils.ts`. * Removed `BridgePathTraversal` error class from `errors.ts`. * Updated imports in `define.ts` and `index.ts` to reflect the changes.
🦋 Changeset detectedLatest commit: 8193e70 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
WalkthroughMigrates fs-bridge to use @ucdjs/path-utils for path resolution and error handling, removes local utils, updates errors (removes BridgePathTraversal, adds BridgeSetupError), revises define.ts control flow and error propagation, updates exports, adjusts tests accordingly, tweaks path-utils normalization and logs, and updates dependencies and changesets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant BridgeFactory as defineFileSystemBridge
participant Impl as fsBridge (impl)
participant Setup as fsBridge.setup
participant Ops as fsBridge.operations
participant PathUtils as @ucdjs/path-utils
App->>BridgeFactory: defineFileSystemBridge(fsBridge)
activate BridgeFactory
BridgeFactory->>Setup: setup(options, state)
note right of Setup: try { ... } catch { throw BridgeSetupError(cause) }
Setup-->>BridgeFactory: { operations, capabilities, state }
deactivate BridgeFactory
App->>Ops: operation(args)
alt returns Promise
Ops-->>App: Promise
App->>Ops: await
Ops->>PathUtils: resolveSafePath(...) (internally)
PathUtils-->>Ops: ok or PathUtilsBaseError
alt PathUtilsBaseError
Ops-->>App: throw PathUtilsBaseError
else Other error
Ops-->>App: throw BridgeGenericError("operation")
end
else returns value
Ops-->>App: value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
github-actions
bot
added
the
pkg: fs-bridge
* Introduced `BridgeSetupError` extending `BridgeBaseError`. * Includes optional `originalError` parameter for better context.
* Introduced `BridgeSetupError` for better error management when setting up the file system bridge. * Updated error messages to provide clearer context, including quotes around operation names.
* Refactor tests for better organization and clarity. * Add error handling tests for setup and async operations. * Ensure all filesystem operations are validated and throw appropriate errors. * Improve assertions for option inference and state management.
- Updated exports in `index.ts` to explicitly list error classes. - Improves clarity on available error types for consumers of the module.
* Changed the method of resolving the input path from `pathe.resolve` to `pathe.normalize` to ensure consistent path formatting. * This adjustment improves the handling of input paths, particularly in edge cases.
Preview Deployment for WebThe Web worker has been deployed successfully. Preview URL: https://https://ucdjs-dev.luxass.workers.dev This preview was built from commit 8193e70 🤖 This comment will be updated automatically when you push new commits to this PR. |
Preview Deployment for ApiThe Api worker has been deployed successfully. Preview URL: https://preview.api.ucdjs.dev This preview was built from commit 8193e70 🤖 This comment will be updated automatically when you push new commits to this PR. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the fs-bridge package to use the new @ucdjs/path-utils package for centralized path handling and security. The migration removes local path utility functions and error classes in favor of the shared package.
- Removes local
utils.tsfile with duplicate path handling functions - Updates imports to use the centralized @ucdjs/path-utils package
- Enhances error handling with new BridgeSetupError class
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ucd-store/test/file-operations/get-file.test.ts | Updates test to use PathTraversalError from path-utils package |
| packages/path-utils/src/security.ts | Fixes path resolution logic and adds debugging console.error statements |
| packages/fs-bridge/test/utils.test.ts | Removes entire test file for local path utilities |
| packages/fs-bridge/test/define.test.ts | Reorganizes tests and adds comprehensive error handling coverage |
| packages/fs-bridge/src/utils.ts | Removes entire utils file with local path handling functions |
| packages/fs-bridge/src/index.ts | Updates exports to remove utils and add specific error class exports |
| packages/fs-bridge/src/errors.ts | Adds BridgeSetupError class and removes BridgePathTraversal |
| packages/fs-bridge/src/define.ts | Updates imports and adds error handling for setup function |
| packages/fs-bridge/package.json | Adds @ucdjs/path-utils dependency |
| .changeset/wet-regions-reply.md | Documents path resolution fix in path-utils |
| .changeset/forty-meals-smell.md | Documents fs-bridge migration to path-utils |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| console.error("[resolveSafePath] Path traversal detected:", { | ||
| basePath: normalizedBasePath, | ||
| accessedPath: resolvedPath, | ||
| }); |
There was a problem hiding this comment.
Console.error calls in production code can impact performance and expose internal path information. Consider using a proper logging framework or removing these debug statements before release.
| console.error("[resolveSafePath] Path traversal detected:", { | |
| basePath: normalizedBasePath, | |
| accessedPath: resolvedPath, | |
| }); |
| console.error("[resolveSafePath#windows] Path traversal detected:", { | ||
| basePath: normalizedBasePath, | ||
| accessedPath: normalizedDecodedPath, | ||
| }); |
There was a problem hiding this comment.
Console.error calls in production code can impact performance and expose internal path information. Consider using a proper logging framework or removing these debug statements before release.
| console.error("[resolveSafePath#windows] Path traversal detected:", { | |
| basePath: normalizedBasePath, | |
| accessedPath: normalizedDecodedPath, | |
| }); |
| assert(fileError != null, "Expected error for path traversal"); | ||
| expect(fileError.message).toBe("Path traversal detected: attempted to access path outside of allowed scope: ../outside.txt"); | ||
| expect(fileError).toBeInstanceOf(PathTraversalError); | ||
| // todo: FIX |
There was a problem hiding this comment.
The TODO comment is unclear about what needs to be fixed. Please provide more specific details about the issue or remove the comment if it's no longer relevant.
| // todo: FIX |
github-actions
bot
added
pkg: ucd-store
![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
🧹 Nitpick comments (13)
packages/path-utils/src/security.ts (5)
129-131: Remove unused mutation of inputPath
inputPathis modified but never read afterward (the flow usesdecodedPath). Drop this to avoid confusion.Apply:
- inputPath = isWindowsDrivePath(inputPath) - ? inputPath - : prependLeadingSlash(inputPath);
144-174: Rename misleading variable: not absolute anymoreAfter switching to
pathe.normalize,absoluteInputPathis no longer guaranteed to be absolute. Rename for clarity.Apply:
- const absoluteInputPath = pathe.normalize(decodedPath); + const normalizedInputPath = pathe.normalize(decodedPath); @@ - if (isAbsoluteInput && isWithinBase(normalizedBasePath, absoluteInputPath)) { + if (isAbsoluteInput && isWithinBase(normalizedBasePath, normalizedInputPath)) { @@ - const normalizedInput = pathe.normalize(absoluteInputPath); + const normalizedInput = pathe.normalize(normalizedInputPath); @@ - return pathe.normalize(absoluteInputPath); + return pathe.normalize(normalizedInputPath);
176-181: Gate Windows-drive handling on the path shape, not host OSTo make behavior consistent in cross-platform environments (e.g., receiving
C:\...on non-Windows), consider relying solely onisWindowsDrivePath(decodedPath).Apply:
- // If either the process.platform is win32, or we are a case insensitive platform, that isn't darwin. - const isWindows = osPlatform === "win32" || (!isCaseSensitive && osPlatform !== "darwin"); - - if (isWindows && isWindowsDrivePath(decodedPath)) { + if (isWindowsDrivePath(decodedPath)) { return internal_resolveWindowsPath(basePath, decodedPath); }
194-199: Avoid noisy console.error in library codeUnconditional error logs can flood consumers. Guard behind env or a debug flag.
Apply:
- console.error("[resolveSafePath] Path traversal detected:", { - basePath: normalizedBasePath, - accessedPath: resolvedPath, - }); + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.error("[resolveSafePath] Path traversal detected:", { + basePath: normalizedBasePath, + accessedPath: resolvedPath, + }); + }
235-239: Same: guard Windows-path traversal logsMirror the logging guard for Windows branch.
Apply:
- console.error("[resolveSafePath#windows] Path traversal detected:", { - basePath: normalizedBasePath, - accessedPath: normalizedDecodedPath, - }); + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.error("[resolveSafePath#windows] Path traversal detected:", { + basePath: normalizedBasePath, + accessedPath: normalizedDecodedPath, + }); + }.changeset/wet-regions-reply.md (1)
11-17: Update example variable name for accuracyExample still uses
absoluteInputPathafter switching tonormalize. Rename tonormalizedInputPathto avoid confusion.Apply:
-const absoluteInputPath = pathe.resolve(decodedPath); +const normalizedInputPath = pathe.resolve(decodedPath);and
-const absoluteInputPath = pathe.normalize(decodedPath); +const normalizedInputPath = pathe.normalize(decodedPath);.changeset/forty-meals-smell.md (1)
21-28: Call out breaking surface explicitlyRemoving
resolveSafePath/isWithinBasefrom fs-bridge is a breaking change. Add a “BREAKING” note to aid consumers (even under 0.x).Apply:
**Key changes:** - Removed local `utils.ts` file with `resolveSafePath` and `isWithinBase` functions @@ - Added `BridgeSetupError` for better error handling during bridge setup + +**BREAKING:** +- `resolveSafePath` and `isWithinBase` are no longer re-exported from `@ucdjs/fs-bridge`. + Import them from `@ucdjs/path-utils` instead.packages/fs-bridge/src/define.ts (2)
36-49: Tighten setup block and typingUse
constand inline the try result; avoids nullablebridgeand narrows types.Apply:
- let bridge = null; - try { - bridge = fsBridge.setup({ + let bridge; + try { + bridge = fsBridge.setup({ options, state: (state ?? {}) as TState, resolveSafePath, }); } catch (err) { throw new BridgeSetupError( "Failed to setup file system bridge", err instanceof Error ? err : undefined, ); }
71-80: Normalize thenables via Promise.resolve instead of requiring .catchSome thenables implement only
then. Wrap withPromise.resolveand attach.catchto be robust.Apply:
- if (result && typeof (result as PromiseLike<unknown>)?.then === "function") { - if (!("catch" in (result as Promise<unknown>))) { - throw new BridgeGenericError( - `The promise returned by '${String(property)}' operation does not support .catch()`, - ); - } - - return (result as Promise<unknown>).catch((err: unknown) => handleError(property, err)); - } + if (result && typeof (result as PromiseLike<unknown>)?.then === "function") { + return Promise.resolve(result).catch((err: unknown) => + handleError(property, err), + ); + }packages/fs-bridge/test/define.test.ts (4)
164-189: Clarify test name/comment: it passes an empty object, not “no options”This test passes {}, not undefined. Rename for precision (avoids confusion with the undefined-arg path, which is invalid for object schemas).
Apply:
- it("should handle bridge with optionsSchema but no options passed", () => { + it("should handle bridge with optionsSchema and empty options object", () => { @@ - // options should be an empty object when no arguments passed + // options should be an empty object when an empty object is passed
251-252: Make assertion resilient to message changesAssert on error type and that the message mentions the op, rather than relying on exact message text.
Apply:
- expect(() => operations.read!("undefined")).toThrowError(new BridgeUnsupportedOperation("read")); + const call = () => operations.read!("undefined"); + expect(call).toThrowError(BridgeUnsupportedOperation); + expect(call).toThrowError(/read/);
259-262: Silence Biome “noThenProperty” for intentional thenableLinter error stems from intentionally crafting a then-only object. Add a scoped ignore with rationale.
Apply:
read: vi.fn().mockImplementation(() => { - return { then: p.then.bind(p) }; + // biome-ignore lint/suspicious/noThenProperty: Intentionally returning a thenable without .catch() to assert error handling + return { then: p.then.bind(p) }; }),
74-105: Optional: add a targeted test for resolveSafePath injectionSince define() now injects resolveSafePath into setup, consider a test that asserts it’s provided and usable (e.g., call resolveSafePath on a benign path and assert no error) to lock in the contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
.changeset/forty-meals-smell.md(1 hunks).changeset/wet-regions-reply.md(1 hunks)packages/fs-bridge/package.json(1 hunks)packages/fs-bridge/src/define.ts(4 hunks)packages/fs-bridge/src/errors.ts(1 hunks)packages/fs-bridge/src/index.ts(1 hunks)packages/fs-bridge/src/utils.ts(0 hunks)packages/fs-bridge/test/define.test.ts(2 hunks)packages/fs-bridge/test/utils.test.ts(0 hunks)packages/path-utils/src/security.ts(3 hunks)packages/ucd-store/test/file-operations/get-file.test.ts(2 hunks)
💤 Files with no reviewable changes (2)
- packages/fs-bridge/src/utils.ts
- packages/fs-bridge/test/utils.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-29T06:02:32.422Z
Learnt from: luxass
PR: ucdjs/ucd#175
File: packages/fs-bridge/test/bridges/node.test.ts:0-0
Timestamp: 2025-07-29T06:02:32.422Z
Learning: In the NodeFileSystemBridge security validation, path traversal detection occurs before critical system paths checks, which means absolute paths to critical system directories will always throw "Path traversal detected" rather than "Critical system path access denied", making it impossible to meaningfully test the critical system paths validation logic.
Applied to files:
packages/ucd-store/test/file-operations/get-file.test.tspackages/path-utils/src/security.tspackages/fs-bridge/src/errors.ts
🧬 Code graph analysis (3)
packages/fs-bridge/src/errors.ts (1)
packages/fs-bridge/src/index.ts (2)
BridgeSetupError(8-8)BridgeBaseError(4-4)
packages/fs-bridge/test/define.test.ts (2)
packages/fs-bridge/src/types.ts (1)
FileSystemBridgeOperations(26-72)packages/fs-bridge/src/define.ts (1)
defineFileSystemBridge(17-99)
packages/fs-bridge/src/define.ts (2)
packages/fs-bridge/src/errors.ts (2)
BridgeSetupError(22-32)BridgeGenericError(10-20)packages/path-utils/src/index.ts (1)
PathUtilsBaseError(6-6)
🪛 Biome (2.1.2)
packages/fs-bridge/test/define.test.ts
[error] 260-260: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (11)
packages/ucd-store/test/file-operations/get-file.test.ts (1)
4-5: LGTM: migrate assertions to PathTraversalError and use patheType-based assertion is the right move with the new path-utils; the pathe import is appropriate.
packages/fs-bridge/package.json (1)
46-46: Add path-utils dependency — LGTMAlignment with the migration; no issues spotted.
packages/fs-bridge/src/index.ts (2)
3-10: Named error re-exports — LGTMExplicit export list improves API clarity and treeshaking.
1-17: Safe removal confirmed No imports ofresolveSafePathorisWithinBasefound in the repo; removal won’t break downstreams.packages/fs-bridge/src/errors.ts (1)
22-32: BridgeSetupError addition — LGTMConsistent with error taxonomy and preserves original error via
cause.packages/fs-bridge/src/define.ts (1)
115-129: Error routing — LGTMDirectly rethrowing
BridgeBaseErrorandPathUtilsBaseErrorkeeps semantics intact; generic wrapper is fine.packages/fs-bridge/test/define.test.ts (5)
4-4: LGTM: imports and API surface look correctImporting assertCapability and the new error types from "../src" matches the updated public exports.
49-56: LGTM: capability assertion and full operation coverageGood demonstration of both single and multi-capability checks plus end-to-end invocation/argument assertions.
Also applies to: 58-71
74-105: LGTM: option schema inference pathTests correctly validate Zod parsing and option propagation to setup.
271-293: LGTM: setup error wrapping preserves original errorAccurately asserts BridgeSetupError shape and originalError propagation.
295-307: LGTM: async/sync operation error wrappingGood coverage of both rejection and throw paths with the expected standardized message.
Also applies to: 309-324
| read: vi.fn().mockResolvedValue("file content"), | ||
| write: vi.fn().mockResolvedValue(undefined), | ||
| exists: vi.fn().mockResolvedValue(true), | ||
| listdir: vi.fn().mockResolvedValue(["file1.txt", "file2.txt"]), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify listdir mock return type matches FSEntry
Tests stub listdir with string arrays, but the public type is Promise<FSEntry[]>. If FSEntry isn’t a string alias, this will type-check by accident only if the suite is lax. Align stubs to the actual FSEntry shape to avoid future breakage.
Run to confirm the FSEntry definition and update the mocks accordingly:
Also applies to: 39-39
🏁 Script executed:
#!/bin/bash
# Locate FSEntry type/interface and show nearby context.
rg -nC3 -g '!**/dist/**' -P '\b(type|interface)\s+FSEntry\b'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Search for FSEntry definition and nearby context in TypeScript files
rg -nC3 '\binterface\s+FSEntry\b' -t ts || true
rg -nC3 '\btype\s+FSEntry\b' -t ts || trueLength of output: 338
Align listdir mocks to return proper FSEntry objects
Tests currently stub listdir with string arrays (["file1.txt", "file2.txt"]) but listdir should return Promise<FSEntry[]>. Update the mocks in packages/fs-bridge/test/define.test.ts (lines 14 and 39) to resolve with objects matching:
{ type: "file"; name: string; path: string; }This ensures mocks conform to FSEntry and prevents future type errors.
🤖 Prompt for AI Agents
In packages/fs-bridge/test/define.test.ts around lines 14 and 39, the listdir
mocks currently resolve to string arrays but should return Promise<FSEntry[]>;
update both vi.fn().mockResolvedValue calls to resolve to arrays of objects with
shape { type: "file"; name: string; path: string } (e.g. for "file1.txt" create
{ type: "file", name: "file1.txt", path: "/some/path/file1.txt" } and similarly
for file2) so the mocks match the FSEntry type expected by the code.
| expect(fileError).toBeInstanceOf(PathTraversalError); | ||
| // todo: FIX | ||
| expect((fileError as PathTraversalError).accessedPath).toBe(pathe.normalize(pathe.join(storePath, "15.0.0", "../../outside.txt"))); | ||
| expect((fileError as PathTraversalError).basePath).toBe(pathe.normalize(storePath)); |
There was a problem hiding this comment.
Fix expected paths: basePath should be the version directory; use resolve for robustness
The security boundary for getFile is the version folder (storePath/15.0.0), not the store root. Also prefer pathe.resolve over normalize(join(...)) for clarity and cross-platform stability. Remove the TODO and adjust expectations accordingly.
- // todo: FIX
- expect((fileError as PathTraversalError).accessedPath).toBe(pathe.normalize(pathe.join(storePath, "15.0.0", "../../outside.txt")));
- expect((fileError as PathTraversalError).basePath).toBe(pathe.normalize(storePath));
+ const expectedAccessed = pathe.resolve(storePath, "15.0.0", "../../outside.txt");
+ const expectedBase = pathe.resolve(storePath, "15.0.0");
+ expect((fileError as PathTraversalError).accessedPath).toBe(expectedAccessed);
+ expect((fileError as PathTraversalError).basePath).toBe(expectedBase);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(fileError).toBeInstanceOf(PathTraversalError); | |
| // todo: FIX | |
| expect((fileError as PathTraversalError).accessedPath).toBe(pathe.normalize(pathe.join(storePath, "15.0.0", "../../outside.txt"))); | |
| expect((fileError as PathTraversalError).basePath).toBe(pathe.normalize(storePath)); | |
| expect(fileError).toBeInstanceOf(PathTraversalError); | |
| - // todo: FIX | |
| - expect((fileError as PathTraversalError).accessedPath).toBe(pathe.normalize(pathe.join(storePath, "15.0.0", "../../outside.txt"))); | |
| const expectedAccessed = pathe.resolve(storePath, "15.0.0", "../../outside.txt"); | |
| const expectedBase = pathe.resolve(storePath, "15.0.0"); | |
| expect((fileError as PathTraversalError).accessedPath).toBe(expectedAccessed); | |
| expect((fileError as PathTraversalError).basePath).toBe(expectedBase); |
🤖 Prompt for AI Agents
In packages/ucd-store/test/file-operations/get-file.test.ts around lines 113 to
116, update the test expectations so the security boundary is the version
directory rather than the store root: remove the TODO and assert that (fileError
as PathTraversalError).accessedPath equals pathe.resolve(storePath, "15.0.0",
"../../outside.txt") and that (fileError as PathTraversalError).basePath equals
pathe.resolve(storePath, "15.0.0"); keep the existing cast to PathTraversalError
and use pathe.resolve for robust, cross-platform path comparison.
1 participant
🔗 Linked issue
resolves #236
resolves #232
📚 Description
Summary by CodeRabbit
New Features
Bug Fixes
Refactor