Skip to content

Commit a65d166

Browse files
committed
refactor: only accept string in pathToFsPath
CodeQL caught a path where we were passing in req.query.path to pathToFsPath, which may not have been a string. So we refactored some things to ensure we only pass it a string which also let us change the parameter type to string instead of string | string[].
1 parent f96bcdc commit a65d166

File tree

3 files changed

+11
-26
lines changed

3 files changed

+11
-26
lines changed

src/node/routes/vscode.ts

+8-6
Original file line numberDiff line numberDiff line change
@@ -63,19 +63,21 @@ router.get("/", async (req, res) => {
6363
* TODO: Might currently be unused.
6464
*/
6565
router.get("/resource(/*)?", ensureAuthenticated, async (req, res) => {
66-
if (typeof req.query.path === "string") {
67-
res.set("Content-Type", getMediaMime(req.query.path))
68-
res.send(await fs.readFile(pathToFsPath(req.query.path)))
66+
const path = getFirstString(req.query.path)
67+
if (path) {
68+
res.set("Content-Type", getMediaMime(path))
69+
res.send(await fs.readFile(pathToFsPath(path)))
6970
}
7071
})
7172

7273
/**
7374
* Used by VS Code to load files.
7475
*/
7576
router.get("/vscode-remote-resource(/*)?", ensureAuthenticated, async (req, res) => {
76-
if (typeof req.query.path === "string") {
77-
res.set("Content-Type", getMediaMime(req.query.path))
78-
res.send(await fs.readFile(pathToFsPath(req.query.path)))
77+
const path = getFirstString(req.query.path)
78+
if (path) {
79+
res.set("Content-Type", getMediaMime(path))
80+
res.send(await fs.readFile(pathToFsPath(path)))
7981
}
8082
})
8183

src/node/util.ts

+2-8
Original file line numberDiff line numberDiff line change
@@ -458,17 +458,11 @@ enum CharCode {
458458
* Taken from vs/base/common/uri.ts. It's not imported to avoid also importing
459459
* everything that file imports.
460460
*/
461-
export function pathToFsPath(path: string | string[], keepDriveLetterCasing = false): string {
461+
export function pathToFsPath(path: string, keepDriveLetterCasing = false): string {
462462
const isWindows = process.platform === "win32"
463-
const uri = { authority: undefined, path: getFirstString(path), scheme: "file" }
463+
const uri = { authority: undefined, path: getFirstString(path) || "", scheme: "file" }
464464
let value: string
465465

466-
if (typeof uri.path !== "string") {
467-
throw new Error(
468-
`Could not compute fsPath from given uri. Expected path to be of type string, but was of type ${typeof uri.path}.`,
469-
)
470-
}
471-
472466
if (uri.authority && uri.path.length > 1 && uri.scheme === "file") {
473467
// unc path: file://shares/c$/far/boo
474468
value = `//${uri.authority}${uri.path}`

test/unit/node/util.test.ts

+1-12
Original file line numberDiff line numberDiff line change
@@ -464,19 +464,8 @@ describe("pathToFsPath", () => {
464464
it("should keep drive letter casing when set to true", () => {
465465
expect(util.pathToFsPath("/C:/far/bo", true)).toBe("C:/far/bo")
466466
})
467-
it("should throw an error if a non-string is passed in for path", () => {
468-
expect(() =>
469-
util
470-
// @ts-expect-error We need to check other types
471-
.pathToFsPath({}),
472-
).toThrow(`Could not compute fsPath from given uri. Expected path to be of type string, but was of type undefined.`)
473-
})
474-
it("should not throw an error for a string array", () => {
475-
// @ts-expect-error We need to check other types
476-
expect(() => util.pathToFsPath(["/hello/foo", "/hello/bar"]).not.toThrow())
477-
})
478467
it("should use the first string in a string array", () => {
479-
expect(util.pathToFsPath(["/hello/foo", "/hello/bar"])).toBe("/hello/foo")
468+
expect(util.pathToFsPath("/hello/foo")).toBe("/hello/foo")
480469
})
481470
it("should replace / with \\ on Windows", () => {
482471
let ORIGINAL_PLATFORM = process.platform

0 commit comments

Comments
 (0)