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[].
This commit is contained in:
Joe Previte 2021-07-19 14:22:42 -07:00
parent 0f451524f9
commit 5c61318592
No known key found for this signature in database
GPG Key ID: 2C91590C6B742C24
3 changed files with 11 additions and 26 deletions

View File

@ -63,9 +63,10 @@ router.get("/", async (req, res) => {
* TODO: Might currently be unused. * TODO: Might currently be unused.
*/ */
router.get("/resource(/*)?", ensureAuthenticated, async (req, res) => { router.get("/resource(/*)?", ensureAuthenticated, async (req, res) => {
if (typeof req.query.path === "string") { const path = getFirstString(req.query.path)
res.set("Content-Type", getMediaMime(req.query.path)) if (path) {
res.send(await fs.readFile(pathToFsPath(req.query.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. * Used by VS Code to load files.
*/ */
router.get("/vscode-remote-resource(/*)?", ensureAuthenticated, async (req, res) => { router.get("/vscode-remote-resource(/*)?", ensureAuthenticated, async (req, res) => {
if (typeof req.query.path === "string") { const path = getFirstString(req.query.path)
res.set("Content-Type", getMediaMime(req.query.path)) if (path) {
res.send(await fs.readFile(pathToFsPath(req.query.path))) res.set("Content-Type", getMediaMime(path))
res.send(await fs.readFile(pathToFsPath(path)))
} }
}) })

View File

@ -458,17 +458,11 @@ enum CharCode {
* Taken from vs/base/common/uri.ts. It's not imported to avoid also importing * Taken from vs/base/common/uri.ts. It's not imported to avoid also importing
* everything that file imports. * 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 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 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") { if (uri.authority && uri.path.length > 1 && uri.scheme === "file") {
// unc path: file://shares/c$/far/boo // unc path: file://shares/c$/far/boo
value = `//${uri.authority}${uri.path}` value = `//${uri.authority}${uri.path}`

View File

@ -464,19 +464,8 @@ describe("pathToFsPath", () => {
it("should keep drive letter casing when set to true", () => { it("should keep drive letter casing when set to true", () => {
expect(util.pathToFsPath("/C:/far/bo", true)).toBe("C:/far/bo") 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", () => { 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", () => { it("should replace / with \\ on Windows", () => {
let ORIGINAL_PLATFORM = process.platform let ORIGINAL_PLATFORM = process.platform