-
-
Notifications
You must be signed in to change notification settings - Fork 3
Hotfix capacitor versions and workspace server uncontrolled data used in path expression #93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| // Create a folder at the validated path | ||
| await fs.promises.mkdir(uri); | ||
| const safe = safeResolve(uri); | ||
| await fs.promises.mkdir(safe, { recursive: true }); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fully address the "uncontrolled data used in path expression" vulnerability, the code should ensure that after combining and resolving user-supplied paths, any symlinks or tricky filesystem artifacts are resolved via a canonicalization pass (e.g. using fs.realpathSync/fs.promises.realpath). Then, after full canonicalization, we must check that the resolved absolute path is indeed within the designated safe root (and was not escaped by symlinks or path tricks). The fix should be applied inside the safeResolve function in remote-workspace/src/servers/api-server/platform-api/handler.ts. This requires using async filesystem operations for consistency with the rest of the code (i.e. fs.promises.realpath rather than the synchronous fs.realpathSync). The function safeResolve should be updated to be asynchronous, and all its callers should be updated to await it and handle returned promises.
-
Copy modified line R13 -
Copy modified lines R18-R19 -
Copy modified lines R23-R29 -
Copy modified lines R32-R39 -
Copy modified line R158 -
Copy modified line R215 -
Copy modified line R221 -
Copy modified line R227 -
Copy modified line R229 -
Copy modified line R235
| @@ -10,21 +10,33 @@ | ||
|
|
||
| const settingsPath = path.join(SAFE_ROOT, "settings.json"); | ||
|
|
||
| function safeResolve(uri: string): string { | ||
| async function safeResolve(uri: string): Promise<string> { | ||
| if (!uri || typeof uri !== "string") { | ||
| throw new Error("Invalid path"); | ||
| } | ||
|
|
||
| // Canonicalize the SAFE_ROOT once for this function | ||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| // Canonicalize the SAFE_ROOT once for this function using realpath | ||
| const rootPath = await fs.promises.realpath(SAFE_ROOT); | ||
| // Combine and normalize the user input relative to the safe root | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
|
|
||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { | ||
| return candidate; | ||
| // Canonicalize (resolve symlinks) for the user-supplied path as well | ||
| let canonicalCandidate: string; | ||
| try { | ||
| canonicalCandidate = await fs.promises.realpath(candidate); | ||
| } catch (err) { | ||
| // Path does not exist yet (e.g. creating a folder): use the resolved candidate path. | ||
| canonicalCandidate = candidate; | ||
| } | ||
|
|
||
| // Check that canonicalCandidate is strictly under rootPath (or equal to rootPath) | ||
| if ( | ||
| canonicalCandidate === rootPath || | ||
| canonicalCandidate.startsWith(rootPath + path.sep) | ||
| ) { | ||
| return canonicalCandidate; | ||
| } | ||
|
|
||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
|
|
||
| @@ -152,7 +155,7 @@ | ||
| options: any, | ||
| baseUri: string | undefined = undefined, | ||
| ) { | ||
| const rootPath = safeResolve(uri); | ||
| const rootPath = await safeResolve(uri); | ||
| const files = await fs.promises.readdir(rootPath, { withFileTypes: true }); | ||
|
|
||
| const promise: Promise<any>[] = files | ||
| @@ -209,13 +212,13 @@ | ||
|
|
||
| async function handleCreateProject(uri: string) { | ||
| // Create a folder at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| await fs.promises.mkdir(safe, { recursive: true }); | ||
| } | ||
|
|
||
| async function handleDeleteProject(uri: string) { | ||
| // Delete the folder at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| await fs.promises.rm(safe, { recursive: true, force: true }); | ||
| } | ||
|
|
||
| @@ -226,15 +224,15 @@ | ||
| ctime?: Date; | ||
| }, | ||
| ) { | ||
| const safeOld = safeResolve(uri); | ||
| const safeOld = await safeResolve(uri); | ||
| const newPathCandidate = path.join(path.dirname(safeOld), updatedInfo.name); | ||
| const safeNew = safeResolve(newPathCandidate); | ||
| const safeNew = await safeResolve(newPathCandidate); | ||
| await fs.promises.rename(safeOld, safeNew); | ||
| } | ||
|
|
||
| async function handleCreateFolder(uri: string) { | ||
| // Create a folder at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| await fs.promises.mkdir(safe, { recursive: true }); | ||
| } | ||
|
|
| // Delete the folder at the validated path | ||
| await fs.promises.rm(uri, { recursive: true, force: true }); | ||
| const safe = safeResolve(uri); | ||
| await fs.promises.rm(safe, { recursive: true, force: true }); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fully mitigate the risk of uncontrolled user input affecting file operations via path traversal or symbolic links, modify the safeResolve function so that it uses fs.realpathSync to canonicalize both the root directory and the candidate path after resolution. This ensures that both paths represent their real filesystem locations, not just resolved strings. The comparison should then check that the resolved path starts with the canonical root (and optionally, allow for equality with the root for root operations). The code change affects only the implementation within remote-workspace/src/servers/api-server/platform-api/handler.ts—specifically, the safeResolve function. No new methods are needed, but the error handling for canonicalization must properly propagate filesystem errors (e.g., if the path does not exist, gracefully handle the error or create the path after validation for mkdir).
-
Copy modified lines R21-R38 -
Copy modified lines R41-R48
| @@ -15,16 +15,37 @@ | ||
| throw new Error("Invalid path"); | ||
| } | ||
|
|
||
| // Canonicalize the SAFE_ROOT once for this function | ||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| // Combine and normalize the user input relative to the safe root | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
|
|
||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { | ||
| return candidate; | ||
| // Canonicalize the SAFE_ROOT and the candidate path | ||
| const rootPathReal = fs.existsSync(SAFE_ROOT) | ||
| ? fs.realpathSync(SAFE_ROOT) | ||
| : path.resolve(SAFE_ROOT); | ||
|
|
||
| // The candidate may not exist yet (e.g. mkdir/rm calls), so we resolve the parent if it doesn't exist | ||
| let candidateReal; | ||
| if (fs.existsSync(candidate)) { | ||
| candidateReal = fs.realpathSync(candidate); | ||
| } else { | ||
| // For creating/deleting, candidate may not exist; resolve its parent as fs.realpathSync | ||
| const parent = path.dirname(candidate); | ||
| if (fs.existsSync(parent)) { | ||
| candidateReal = path.join(fs.realpathSync(parent), path.basename(candidate)); | ||
| } else { | ||
| // If parent does not exist, fallback to resolved path (all will be within SAFE_ROOT) | ||
| candidateReal = path.resolve(candidate); | ||
| } | ||
| } | ||
|
|
||
| // Check that candidate is strictly under rootPathReal (or equal to rootPathReal) | ||
| if ( | ||
| candidateReal === rootPathReal || | ||
| candidateReal.startsWith(rootPathReal + path.sep) | ||
| ) { | ||
| return candidateReal; | ||
| } | ||
|
|
||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
|
|
| const safeOld = safeResolve(uri); | ||
| const newPathCandidate = path.join(path.dirname(safeOld), updatedInfo.name); | ||
| const safeNew = safeResolve(newPathCandidate); | ||
| await fs.promises.rename(safeOld, safeNew); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fix this error, we must ensure that updatedInfo.name (the new folder/file name from the user) cannot be used to escape the intended directory hierarchy or introduce dangerous path elements. The best way is to strictly validate updatedInfo.name to restrict it to simple, platform-safe filenames with no slashes, traversal, null bytes, or special path components. This can be achieved by either using an allowlist pattern (such as only letters, numbers, underscores, hyphens, and periods) or by using the well-known sanitize-filename npm package. For best practices and broad safety, use sanitize-filename to clean any filename provided by the user before passing it to path.join. In addition, check after sanitization that the filename is non-empty.
Specifically, in handleUpdateProject, before using updatedInfo.name, sanitize it; if it is invalid after sanitization, throw an error. Insert the necessary import of sanitize-filename at the top. No changes need to be made outside the code shown in remote-workspace/src/servers/api-server/platform-api/handler.ts.
-
Copy modified line R4 -
Copy modified lines R230-R239
| @@ -1,7 +1,7 @@ | ||
| import fs from "fs"; | ||
| import ignore from "ignore"; | ||
| import path from "path"; | ||
|
|
||
| import sanitize from "sanitize-filename"; | ||
| // Define a safe root directory for projects. Can be overridden by env or configured as needed. | ||
| // All incoming URIs will be resolved and validated to ensure they don't escape this root. | ||
| const SAFE_ROOT = path.resolve( | ||
| @@ -227,7 +227,16 @@ | ||
| }, | ||
| ) { | ||
| const safeOld = safeResolve(uri); | ||
| const newPathCandidate = path.join(path.dirname(safeOld), updatedInfo.name); | ||
| // Sanitize the project name to ensure it's safe for use in a file path | ||
| const sanitizedName = sanitize(updatedInfo.name); | ||
| if ( | ||
| !sanitizedName || | ||
| sanitizedName !== updatedInfo.name || // Disallow any name changed by sanitization | ||
| sanitizedName.includes(path.sep) || sanitizedName.includes("/") | ||
| ) { | ||
| throw new Error("Invalid project name"); | ||
| } | ||
| const newPathCandidate = path.join(path.dirname(safeOld), sanitizedName); | ||
| const safeNew = safeResolve(newPathCandidate); | ||
| await fs.promises.rename(safeOld, safeNew); | ||
| } |
-
Copy modified lines R18-R19
| @@ -15,7 +15,8 @@ | ||
| "express": "^5.1.0", | ||
| "ignore": "^7.0.5", | ||
| "node-pty": "^1.1.0-beta37", | ||
| "ws": "^8.18.3" | ||
| "ws": "^8.18.3", | ||
| "sanitize-filename": "^1.6.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/express": "^5.0.3", |
| Package | Version | Security advisories |
| sanitize-filename (npm) | 1.6.3 | None |
| const safeOld = safeResolve(uri); | ||
| const newPathCandidate = path.join(path.dirname(safeOld), updatedInfo.name); | ||
| const safeNew = safeResolve(newPathCandidate); | ||
| await fs.promises.rename(safeOld, safeNew); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To securely use updatedInfo.name for renaming a project directory, the input should be strictly sanitized to ensure only valid filenames—not path components—are accepted, and that such filenames cannot escape their intended parent directory or cause unintended file overwrites. The best practice is to use the sanitize-filename npm package on updatedInfo.name before it's used to construct any filesystem path. Specifically:
- Add an import for
sanitize-filename(i.e.,import sanitize from "sanitize-filename";). - Before building
newPathCandidate, sanitizeupdatedInfo.nameusing the library. - If sanitization results in an empty string, treat it as invalid input and throw.
- Use the sanitized version when constructing
newPathCandidatefor path resolution.
These changes should only affect the relevant area inhandleUpdateProjectinremote-workspace/src/servers/api-server/platform-api/handler.ts. No changes are required elsewhere.
-
Copy modified line R4 -
Copy modified lines R231-R235
| @@ -1,6 +1,7 @@ | ||
| import fs from "fs"; | ||
| import ignore from "ignore"; | ||
| import path from "path"; | ||
| import sanitize from "sanitize-filename"; | ||
|
|
||
| // Define a safe root directory for projects. Can be overridden by env or configured as needed. | ||
| // All incoming URIs will be resolved and validated to ensure they don't escape this root. | ||
| @@ -227,7 +228,11 @@ | ||
| }, | ||
| ) { | ||
| const safeOld = safeResolve(uri); | ||
| const newPathCandidate = path.join(path.dirname(safeOld), updatedInfo.name); | ||
| const sanitizedName = sanitize(updatedInfo.name); | ||
| if (!sanitizedName) { | ||
| throw new Error("Invalid project name"); | ||
| } | ||
| const newPathCandidate = path.join(path.dirname(safeOld), sanitizedName); | ||
| const safeNew = safeResolve(newPathCandidate); | ||
| await fs.promises.rename(safeOld, safeNew); | ||
| } |
-
Copy modified lines R18-R19
| @@ -15,7 +15,8 @@ | ||
| "express": "^5.1.0", | ||
| "ignore": "^7.0.5", | ||
| "node-pty": "^1.1.0-beta37", | ||
| "ws": "^8.18.3" | ||
| "ws": "^8.18.3", | ||
| "sanitize-filename": "^1.6.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/express": "^5.0.3", |
| Package | Version | Security advisories |
| sanitize-filename (npm) | 1.6.3 | None |
| // Create a folder at the validated path | ||
| await fs.promises.mkdir(uri); | ||
| const safe = safeResolve(uri); | ||
| await fs.promises.mkdir(safe, { recursive: true }); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
The proper way to fix this issue is to fully canonicalize the target path by resolving all symbolic links (using fs.realpathSync or fs.promises.realpath), and then ensure that the final, resolved path is contained within the intended safe root. The best way here is to:
- Update
safeResolveto resolve the candidate path usingfs.realpathSync(or async variant withfs.promises.realpath), after combining and normalizing the user input. - Apply an additional check to ensure the real path is beneath
SAFE_ROOT(not just starts-with, but also with proper path separators). - As
safeResolveis used within several asynchronous functions, use the async variant (fs.promises.realpath) and convertsafeResolveto beasync. - Update all callers of
safeResolveto useawait. - No external packages are needed; Node.js core modules are sufficient.
Edits are required in remote-workspace/src/servers/api-server/platform-api/handler.ts:
- Change
safeResolveto beasync, usefs.promises.realpath, and recheck the root containment. - Update all invocations of
safeResolveto beawait safeResolve(...).
-
Copy modified line R13 -
Copy modified lines R24-R25 -
Copy modified lines R28-R44 -
Copy modified line R226 -
Copy modified line R232 -
Copy modified line R238 -
Copy modified line R240 -
Copy modified line R246 -
Copy modified line R252 -
Copy modified lines R259-R260
| @@ -10,7 +10,7 @@ | ||
|
|
||
| const settingsPath = path.join(SAFE_ROOT, "settings.json"); | ||
|
|
||
| function safeResolve(uri: string): string { | ||
| async function safeResolve(uri: string): Promise<string> { | ||
| if (!uri || typeof uri !== "string") { | ||
| throw new Error("Invalid path"); | ||
| } | ||
| @@ -21,11 +21,27 @@ | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
|
|
||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { | ||
| return candidate; | ||
| if (!(candidate === rootPath || candidate.startsWith(rootPath + path.sep))) { | ||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
|
|
||
| throw new Error("Can only access paths within the project home directory."); | ||
| // Canonicalize all symlinks | ||
| let realCandidate: string; | ||
| try { | ||
| realCandidate = await fs.promises.realpath(candidate); | ||
| } catch (err) { | ||
| // If the path doesn't exist yet (e.g., for creation), just use candidate | ||
| realCandidate = candidate; | ||
| } | ||
|
|
||
| // Final check after resolving symlinks | ||
| const realRoot = await fs.promises.realpath(rootPath); | ||
| if ( | ||
| !(realCandidate === realRoot || realCandidate.startsWith(realRoot + path.sep)) | ||
| ) { | ||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
| return realCandidate; | ||
| } | ||
|
|
||
| export async function handlePlatformAPIRequest( | ||
| @@ -209,13 +223,13 @@ | ||
|
|
||
| async function handleCreateProject(uri: string) { | ||
| // Create a folder at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| await fs.promises.mkdir(safe, { recursive: true }); | ||
| } | ||
|
|
||
| async function handleDeleteProject(uri: string) { | ||
| // Delete the folder at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| await fs.promises.rm(safe, { recursive: true, force: true }); | ||
| } | ||
|
|
||
| @@ -226,29 +235,29 @@ | ||
| ctime?: Date; | ||
| }, | ||
| ) { | ||
| const safeOld = safeResolve(uri); | ||
| const safeOld = await safeResolve(uri); | ||
| const newPathCandidate = path.join(path.dirname(safeOld), updatedInfo.name); | ||
| const safeNew = safeResolve(newPathCandidate); | ||
| const safeNew = await safeResolve(newPathCandidate); | ||
| await fs.promises.rename(safeOld, safeNew); | ||
| } | ||
|
|
||
| async function handleCreateFolder(uri: string) { | ||
| // Create a folder at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| await fs.promises.mkdir(safe, { recursive: true }); | ||
| } | ||
|
|
||
| async function handleCreateFile(uri: string) { | ||
| // Create a file at the validated path | ||
| const safe = safeResolve(uri); | ||
| const safe = await safeResolve(uri); | ||
| // ensure parent exists | ||
| await fs.promises.mkdir(path.dirname(safe), { recursive: true }); | ||
| await fs.promises.writeFile(safe, ""); | ||
| } | ||
|
|
||
| async function handleRename(oldUri: string, newUri: string) { | ||
| const safeOld = safeResolve(oldUri); | ||
| const safeNew = safeResolve(newUri); | ||
| const safeOld = await safeResolve(oldUri); | ||
| const safeNew = await safeResolve(newUri); | ||
| await fs.promises.rename(safeOld, safeNew); | ||
| } | ||
|
|
| async function handleDelete(uri: string) { | ||
| await fs.promises.rm(uri, { | ||
| const safe = safeResolve(uri); | ||
| await fs.promises.rm(safe, { |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
The best fix is to enhance the validation inside safeResolve to ensure that requests cannot access files outside the intended directory, even if directory symlinks are present within the root.
- In
safeResolve, after resolving bothSAFE_ROOTand the candidate path withpath.resolve, we should also callfs.realpathSyncto resolve any symlinks in both paths before performing the containment check. - This means constructing the candidate as before, then realpath-ing both, then checking for the containment relationship.
- The edit should be limited to the
safeResolveimplementation inremote-workspace/src/servers/api-server/platform-api/handler.ts. - You will need to convert the function to use
fs.realpathSync(for synchronous resolution), or (optionally) make it async and useawait fs.promises.realpath—for the purpose of the current code’s synchronous usage elsewhere, synchronous may be preferable. - No new imports are needed; the required methods are available via the existing
fsandpathimports.
-
Copy modified lines R18-R20 -
Copy modified line R22
| @@ -15,12 +15,11 @@ | ||
| throw new Error("Invalid path"); | ||
| } | ||
|
|
||
| // Canonicalize the SAFE_ROOT once for this function | ||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| // Combine and normalize the user input relative to the safe root | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
| // Canonicalize and resolve symlinks in SAFE_ROOT and candidate path | ||
| const rootPath = fs.realpathSync(path.resolve(SAFE_ROOT)); | ||
| const candidate = fs.realpathSync(path.resolve(SAFE_ROOT, uri)); | ||
|
|
||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| // Check that candidate path is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { | ||
| return candidate; | ||
| } |
| return fs.existsSync(uri); | ||
| try { | ||
| const safe = safeResolve(uri); | ||
| return fs.existsSync(safe); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fix the problem, we need to enhance the safeResolve function to ensure the path is fully canonicalized, including resolution of symbolic links, and then check that the canonical path is strictly inside the project root.
General method:
- Use
path.resolveto construct the target path from the root and user input. - Use
fs.realpathSyncon both the root and target candidate paths to fully resolve any symlinks. - Check that the canonicalized target path equals or is a subpath (with separator) of the canonicalized root. This avoids directory traversal and symlink attacks.
- Replace all uses of string prefix comparisons on potentially uncanonicalized paths.
File/lines to change:
- In
remote-workspace/src/servers/api-server/platform-api/handler.ts, edit thesafeResolvefunction at lines 13–29 to usefs.realpathSyncfor canonical path comparison. - Add error handling in
safeResolvein case the path does not exist (sincefs.realpathSyncthrows if the path is missing). To address this, we attempt to resolve the candidate path up to the first missing segment (i.e. resolve parent directory instead if needed, as is common in secure path checks). - Optionally, improve error messages.
Dependencies needed:
None. Node.js built-in libraries (fs, path) are sufficient.
-
Copy modified line R19 -
Copy modified lines R23-R35 -
Copy modified lines R38-R45
| @@ -16,15 +16,33 @@ | ||
| } | ||
|
|
||
| // Canonicalize the SAFE_ROOT once for this function | ||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| const rootRealPath = fs.realpathSync(SAFE_ROOT); // get symlink-resolved root | ||
| // Combine and normalize the user input relative to the safe root | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
|
|
||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { | ||
| return candidate; | ||
| let candidateRealPath: string; | ||
| try { | ||
| // If the candidate does not exist (e.g. for creation ops), resolve its parent | ||
| if (fs.existsSync(candidate)) { | ||
| candidateRealPath = fs.realpathSync(candidate); | ||
| } else { | ||
| // Resolve parent directory and reconstruct real path | ||
| const parent = path.dirname(candidate); | ||
| const parentRealPath = fs.realpathSync(parent); | ||
| candidateRealPath = path.join(parentRealPath, path.basename(candidate)); | ||
| } | ||
| } catch (e) { | ||
| throw new Error("Failed to resolve path: " + e?.message); | ||
| } | ||
|
|
||
| // Check that candidateRealPath is strictly under rootRealPath (or equal to rootRealPath) | ||
| if ( | ||
| candidateRealPath === rootRealPath || | ||
| candidateRealPath.startsWith(rootRealPath + path.sep) | ||
| ) { | ||
| return candidateRealPath; | ||
| } | ||
|
|
||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
|
|
| ); | ||
|
|
||
| const safe = safeResolve(uri); | ||
| const data = await fs.promises.readFile(safe, "utf-8"); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To fully mitigate path traversal and symlink-based attacks, safeResolve should resolve the user-supplied path to an absolute, normalized, and symlink-resolved path and then check that the result stays within SAFE_ROOT. This can be done by using fs.realpathSync() or (for async contexts) fs.promises.realpath. Update the validation logic so that after resolving the combined path with path.resolve, also apply fs.realpathSync (or await fs.promises.realpath if in an async context) before performing the "startsWith" root check. This change should only affect safeResolve in platform-api/handler.ts.
The steps are:
- Update the code inside
safeResolve(uri: string)to add symlink resolution to both the safe root and the candidate path. - Replace
path.resolvefor the candidate withfs.realpathSync(path.resolve(SAFE_ROOT, uri)). - Make sure the root path itself is also resolved with
fs.realpathSync. - All usages of
safeResolveremain unchanged, as the function now does the complete validation. - We do not need to add extra packages; only use built-in Node.js functionality.
-
Copy modified lines R18-R20
| @@ -15,10 +15,9 @@ | ||
| throw new Error("Invalid path"); | ||
| } | ||
|
|
||
| // Canonicalize the SAFE_ROOT once for this function | ||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| // Combine and normalize the user input relative to the safe root | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
| // Canonicalize and resolve any symlinks on SAFE_ROOT and the candidate path | ||
| const rootPath = fs.realpathSync(SAFE_ROOT); | ||
| const candidate = fs.realpathSync(path.resolve(SAFE_ROOT, uri)); | ||
|
|
||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { |
| ); | ||
| const safeFrom = safeResolve(from); | ||
| const safeTo = safeResolve(to); | ||
| await fs.promises.cp(safeFrom, safeTo, { recursive: true }); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
To comprehensively fix the issue, update the safeResolve function to use fs.realpathSync (or, in async versions, fs.promises.realpath) after resolving the input to produce the canonical, symlink-free path. After this normalization, re-check that the canonical path is inside SAFE_ROOT. This ensures that even if there are symlinks inside SAFE_ROOT pointing outwards, the resulting target of the path will still be validated.
Specifically:
- In
safeResolve, afterpath.resolve(SAFE_ROOT, uri), usefs.realpathSync(candidate)to eliminate symlinks. - Perform the root prefix check on this realpath.
- Be sure to handle cases where the file/directory does not exist (realpath will throw); for write/create/check commands, fallback to using the normalized path for non-existing files, but for read/copy, reject or handle errors when path does not exist.
- Since the existing usages of
safeResolveare both sync and async, update it to either have sync/async variants, or ensure calling code can use the appropriate version. - For
handleCopyFiles, use the async/promises versions everywhere and ensure that bothsafeFromandsafeToare checked as "real paths" within root.
-
Copy modified lines R23-R29 -
Copy modified lines R31-R32 -
Copy modified lines R38-R59 -
Copy modified lines R316-R317
| @@ -20,14 +20,43 @@ | ||
| // Combine and normalize the user input relative to the safe root | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
|
|
||
| let realCandidate: string; | ||
| try { | ||
| realCandidate = fs.realpathSync(candidate); | ||
| } catch (e) { | ||
| // If path doesn't exist yet (e.g., for write), use normalized candidate | ||
| realCandidate = candidate; | ||
| } | ||
| // Check that candidate is strictly under rootPath (or equal to rootPath) | ||
| if (candidate === rootPath || candidate.startsWith(rootPath + path.sep)) { | ||
| return candidate; | ||
| if (realCandidate === rootPath || realCandidate.startsWith(rootPath + path.sep)) { | ||
| return realCandidate; | ||
| } | ||
|
|
||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
|
|
||
| async function safeResolveAsync(uri: string): Promise<string> { | ||
| if (!uri || typeof uri !== "string") { | ||
| throw new Error("Invalid path"); | ||
| } | ||
|
|
||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| const candidate = path.resolve(SAFE_ROOT, uri); | ||
|
|
||
| let realCandidate: string; | ||
| try { | ||
| realCandidate = await fs.promises.realpath(candidate); | ||
| } catch (e) { | ||
| // If path doesn't exist (for write/copy destinations), use normalized candidate | ||
| realCandidate = candidate; | ||
| } | ||
| if (realCandidate === rootPath || realCandidate.startsWith(rootPath + path.sep)) { | ||
| return realCandidate; | ||
| } | ||
|
|
||
| throw new Error("Can only access paths within the project home directory."); | ||
| } | ||
|
|
||
| export async function handlePlatformAPIRequest( | ||
| data: { | ||
| operation: string; | ||
| @@ -290,8 +313,8 @@ | ||
|
|
||
| async function handleCopyFiles(from: string, to: string) { | ||
| // Copy the files from the validated from path to the validated to path | ||
| const safeFrom = safeResolve(from); | ||
| const safeTo = safeResolve(to); | ||
| const safeFrom = await safeResolveAsync(from); | ||
| const safeTo = await safeResolveAsync(to); | ||
| await fs.promises.cp(safeFrom, safeTo, { recursive: true }); | ||
| } | ||
|
|
| ); | ||
| const safeFrom = safeResolve(from); | ||
| const safeTo = safeResolve(to); | ||
| await fs.promises.cp(safeFrom, safeTo, { recursive: true }); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
The best way to fix this problem is to add additional checks inside the handleCopyFiles function to prevent the destination (to) from allowing certain risky operations, even if the path is contained within SAFE_ROOT. Specifically:
- Block copying into the project root itself: ensure
safeTois not equal to the root directory. - Prevent overwriting critical files such as
settings.jsonby disallowing destination paths that match that file. - Optionally, prevent recursive copy to an ancestor/parent folder of the source, or to source itself (e.g., avoid self-overwriting or broken recursion).
- Return a clear error message if an invalid path is detected.
Implementation steps:
- In
handleCopyFiles, after callingsafeResolveon bothfromandto, check thatsafeTois not equal to the project root, that it does not point tosettings.json, and that it does not resolve to the same assafeFromor its parent. - If any are true, throw an error before executing the copy.
- (No external libraries needed unless implementing pattern allow-list; only standard
pathand basic checks.)
Change only the code in handleCopyFiles region in remote-workspace/src/servers/api-server/platform-api/handler.ts.
-
Copy modified lines R295-R314
| @@ -292,6 +292,26 @@ | ||
| // Copy the files from the validated from path to the validated to path | ||
| const safeFrom = safeResolve(from); | ||
| const safeTo = safeResolve(to); | ||
|
|
||
| // Additional validation -- destination must NOT be project root, settings.json, or the source itself (or parent/ancestor of source) | ||
| const rootPath = path.resolve(SAFE_ROOT); | ||
| const settingsFilePath = path.join(rootPath, "settings.json"); | ||
|
|
||
| if (safeTo === rootPath) { | ||
| throw new Error("Cannot copy files directly into the root directory."); | ||
| } | ||
| if (safeTo === settingsFilePath) { | ||
| throw new Error("Cannot overwrite settings.json file."); | ||
| } | ||
| // Prevent copying from/to same path or ancestor self-copy | ||
| if (safeFrom === safeTo) { | ||
| throw new Error("Source and destination paths must be different."); | ||
| } | ||
| // Optionally, prevent copying into own ancestor (infinite recursion) | ||
| if (safeFrom.startsWith(safeTo + path.sep)) { | ||
| throw new Error("Cannot copy a folder to one of its ancestor paths."); | ||
| } | ||
|
|
||
| await fs.promises.cp(safeFrom, safeTo, { recursive: true }); | ||
| } | ||
|
|
No description provided.