feat: allow file reads outside IDE workspace (w/ permission) #8298
Conversation
Patrick-Erichsen
requested review from
tingwai
and removed request for
a team
dosubot
bot
added
the
size:XL
Patrick-Erichsen
requested review from
RomneyDa
and removed request for
tingwai
- Combined resolveInputPath approach with new ContinueError system - Updated error handling to use ContinueError with appropriate error reasons - Maintained improved error messages from pe/read-file-errs branch
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 13 files
Prompt for AI agents (all 5 issues)
Understand the root cause of the following 5 issues and fix them.
<file name="core/tools/implementations/viewSubdirectory.ts">
<violation number="1" location="core/tools/implementations/viewSubdirectory.ts:11">
resolveInputPath returns a truthy result even when the absolute directory does not exist, so this branch no longer throws DirectoryNotFound and generateRepoMap later fails while walking the missing path.</violation>
</file>
<file name="extensions/cli/src/util/pathResolver.ts">
<violation number="1" location="extensions/cli/src/util/pathResolver.ts:15">
Home-relative paths that use "~\" (common on Windows shells) are not expanded, so resolveInputPath incorrectly rejects valid Windows home paths.</violation>
</file>
<file name="core/util/pathResolver.ts">
<violation number="1" location="core/util/pathResolver.ts:26">
Workspace containment currently matches on raw prefix, so paths such as `/workspace-other/file` are incorrectly classified as inside `/workspace`, which can bypass the outside-workspace permission gate. Please ensure the comparison enforces real directory boundaries.</violation>
</file>
<file name="core/util/pathResolver.test.ts">
<violation number="1" location="core/util/pathResolver.test.ts:80">
The expected URI is constructed with path.join, which yields backslashes and omits the third slash on Windows, so this assertion fails on Windows even though resolveInputPath returns the correct normalized URI.</violation>
<violation number="2" location="core/util/pathResolver.test.ts:201">
This assertion hardcodes a POSIX path; on Windows normalizeDisplayPath yields `~\Documents\file.txt`, so the test fails despite the implementation being correct on that platform.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
There was a problem hiding this comment.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 5 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="core/util/pathResolver.test.ts">
<violation number="1" location="core/util/pathResolver.test.ts:38">
The new findUriInDirs mock treats file:///workspace-subdir/... as within the workspace because it only checks uri.startsWith(dir). Please ensure the mock only matches when the URI actually sits under the workspace path, mirroring the real helper.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Reviewed changes from recent commits (found 1 issue).
1 issue found across 3 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="core/tools/implementations/viewSubdirectory.vitest.ts">
<violation number="1" location="core/tools/implementations/viewSubdirectory.vitest.ts:7">
The mocked extras.ide object is missing getWorkspaceDirs, so resolveInputPath throws a TypeError before viewSubdirectoryImpl can raise ContinueError, causing the test to fail.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
dosubot
bot
added
size:L
core/util/pathResolver.ts
Outdated
|
|
||
| // Expand tilde paths | ||
| let expandedPath = trimmedPath; | ||
| if (trimmedPath.startsWith("~/")) { |
There was a problem hiding this comment.
noting no tilde support for windows, probably fine since tilde is usually unix, but e.g. ~\this\that would be missed here
There was a problem hiding this comment.
i dont think that is a valid windows path
Windows doesn't interpret ~ as the home directory like Unix/Linux systems do. Valid Windows paths typically start with:
- A drive letter: C:\this\that
- A UNC path: \\server\share\this\that
- A relative path: this\that or .\this\that
To reference the user's home directory in Windows, you'd use environment variables like %USERPROFILE%\this\that or %HOMEPATH%\this\that.
core/util/pathResolver.ts
Outdated
|
|
||
| if (isAbsolute) { | ||
| // For Windows network paths, handle specially | ||
| if (expandedPath.startsWith("\\\\")) { |
There was a problem hiding this comment.
this check is duplicate, can move windows network path handling outside/above other absolute paths
core/util/pathResolver.ts
Outdated
| // For Windows network paths, handle specially | ||
| if (expandedPath.startsWith("\\\\")) { | ||
| const networkPath = expandedPath.replace(/\\/g, "/"); | ||
| const uri = "file:" + networkPath; // file://server/share format |
There was a problem hiding this comment.
note networkPath is not normalized to URI path style, probably fine but could add URI.normalize here
| } | ||
|
|
||
| // Fall back to relative path resolution within workspace | ||
| const workspaceUri = await resolveRelativePathInDir(expandedPath, ide); |
There was a problem hiding this comment.
One issue here is that resolveRelativePathInDir checks if file exists while isUriWithinWorkspace does not. e.g. isUriWithinWorkspace is just doing string parsing, while resolveRelativePathInDir has a file exists check. This means you will not get file exists error if not in workspace (or there will be duplicate exists checks, not sure)
There was a problem hiding this comment.
This is the only blocking feedback
|
|
||
| // Expand tilde paths | ||
| let expandedPath = trimmedPath; | ||
| if (trimmedPath.startsWith("~/") || trimmedPath.startsWith("~\\")) { |
There was a problem hiding this comment.
Nitpick but could merge this section of logic with a expandTildePaths util or similar
|
After pulling in |
There was a problem hiding this comment.
Reviewed changes from recent commits (found 2 issues).
2 issues found across 5 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="core/tools/implementations/viewSubdirectory.vitest.ts">
<violation number="1" location="core/tools/implementations/viewSubdirectory.vitest.ts:9">
This test no longer covers the resolveInputPath-null scenario because the mocked extras cause fileExists to trigger the failure instead, so it will not detect regressions in that branch.</violation>
</file>
<file name="core/util/pathResolver.ts">
<violation number="1" location="core/util/pathResolver.ts:59">
UNC file paths are no longer converted correctly: pathToFileURL on POSIX hosts turns \\server\share paths into URIs like file:///mnt/workspace/%5Cserversharedir, breaking access to network shares.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
Note that I added a |
2 participants
closes CON-4324 CON-3616 CON-4465
Summary by cubic
Allow reading and listing files outside the IDE workspace with explicit permission. Adds robust path resolution for absolute, tilde (~/), and file:// paths, and enforces stricter access policies for non-workspace paths.