From 5c61318592d3ef818b033343b4a38728c6bddbb6 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 19 Jul 2021 14:22:42 -0700 Subject: [PATCH 1/2] 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[]. --- src/node/routes/vscode.ts | 14 ++++++++------ src/node/util.ts | 10 ++-------- test/unit/node/util.test.ts | 13 +------------ 3 files changed, 11 insertions(+), 26 deletions(-) diff --git a/src/node/routes/vscode.ts b/src/node/routes/vscode.ts index ffd66ab9c..160f5a432 100644 --- a/src/node/routes/vscode.ts +++ b/src/node/routes/vscode.ts @@ -63,9 +63,10 @@ router.get("/", async (req, res) => { * TODO: Might currently be unused. */ router.get("/resource(/*)?", ensureAuthenticated, async (req, res) => { - if (typeof req.query.path === "string") { - res.set("Content-Type", getMediaMime(req.query.path)) - res.send(await fs.readFile(pathToFsPath(req.query.path))) + const path = getFirstString(req.query.path) + if (path) { + res.set("Content-Type", getMediaMime(path)) + res.send(await fs.readFile(pathToFsPath(path))) } }) @@ -73,9 +74,10 @@ router.get("/resource(/*)?", ensureAuthenticated, async (req, res) => { * Used by VS Code to load files. */ router.get("/vscode-remote-resource(/*)?", ensureAuthenticated, async (req, res) => { - if (typeof req.query.path === "string") { - res.set("Content-Type", getMediaMime(req.query.path)) - res.send(await fs.readFile(pathToFsPath(req.query.path))) + const path = getFirstString(req.query.path) + if (path) { + res.set("Content-Type", getMediaMime(path)) + res.send(await fs.readFile(pathToFsPath(path))) } }) diff --git a/src/node/util.ts b/src/node/util.ts index 2a010f0e9..1216601ef 100644 --- a/src/node/util.ts +++ b/src/node/util.ts @@ -458,17 +458,11 @@ enum CharCode { * Taken from vs/base/common/uri.ts. It's not imported to avoid also importing * everything that file imports. */ -export function pathToFsPath(path: string | string[], keepDriveLetterCasing = false): string { +export function pathToFsPath(path: string, keepDriveLetterCasing = false): string { const isWindows = process.platform === "win32" - const uri = { authority: undefined, path: getFirstString(path), scheme: "file" } + const uri = { authority: undefined, path: getFirstString(path) || "", scheme: "file" } let value: string - if (typeof uri.path !== "string") { - throw new Error( - `Could not compute fsPath from given uri. Expected path to be of type string, but was of type ${typeof uri.path}.`, - ) - } - if (uri.authority && uri.path.length > 1 && uri.scheme === "file") { // unc path: file://shares/c$/far/boo value = `//${uri.authority}${uri.path}` diff --git a/test/unit/node/util.test.ts b/test/unit/node/util.test.ts index 8b3923fb4..30cd4672c 100644 --- a/test/unit/node/util.test.ts +++ b/test/unit/node/util.test.ts @@ -464,19 +464,8 @@ describe("pathToFsPath", () => { it("should keep drive letter casing when set to true", () => { expect(util.pathToFsPath("/C:/far/bo", true)).toBe("C:/far/bo") }) - it("should throw an error if a non-string is passed in for path", () => { - expect(() => - util - // @ts-expect-error We need to check other types - .pathToFsPath({}), - ).toThrow(`Could not compute fsPath from given uri. Expected path to be of type string, but was of type undefined.`) - }) - it("should not throw an error for a string array", () => { - // @ts-expect-error We need to check other types - expect(() => util.pathToFsPath(["/hello/foo", "/hello/bar"]).not.toThrow()) - }) it("should use the first string in a string array", () => { - expect(util.pathToFsPath(["/hello/foo", "/hello/bar"])).toBe("/hello/foo") + expect(util.pathToFsPath("/hello/foo")).toBe("/hello/foo") }) it("should replace / with \\ on Windows", () => { let ORIGINAL_PLATFORM = process.platform From 6e33dccb403c651fa733282863fcebd69907f138 Mon Sep 17 00:00:00 2001 From: Joe Previte Date: Mon, 19 Jul 2021 15:00:32 -0700 Subject: [PATCH 2/2] feat: add tests for isFile --- test/unit/node/util.test.ts | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/test/unit/node/util.test.ts b/test/unit/node/util.test.ts index 30cd4672c..e765be6cf 100644 --- a/test/unit/node/util.test.ts +++ b/test/unit/node/util.test.ts @@ -1,6 +1,9 @@ import * as cp from "child_process" +import * as path from "path" +import { promises as fs } from "fs" import { generateUuid } from "../../../src/common/util" import * as util from "../../../src/node/util" +import { tmpdir } from "../../../src/node/constants" describe("getEnvPaths", () => { describe("on darwin", () => { @@ -464,9 +467,6 @@ describe("pathToFsPath", () => { it("should keep drive letter casing when set to true", () => { expect(util.pathToFsPath("/C:/far/bo", true)).toBe("C:/far/bo") }) - it("should use the first string in a string array", () => { - expect(util.pathToFsPath("/hello/foo")).toBe("/hello/foo") - }) it("should replace / with \\ on Windows", () => { let ORIGINAL_PLATFORM = process.platform @@ -481,3 +481,23 @@ describe("pathToFsPath", () => { }) }) }) + +describe("isFile", () => { + const testDir = path.join(tmpdir, "tests", "isFile") + let pathToFile = "" + + beforeEach(async () => { + pathToFile = path.join(testDir, "foo.txt") + await fs.mkdir(testDir, { recursive: true }) + await fs.writeFile(pathToFile, "hello") + }) + afterEach(async () => { + await fs.rm(testDir, { recursive: true, force: true }) + }) + it("should return false if the path doesn't exist", async () => { + expect(await util.isFile(testDir)).toBe(false) + }) + it("should return true if is file", async () => { + expect(await util.isFile(pathToFile)).toBe(true) + }) +})