Skip to content

fix: check uri.path is string in pathToFsPath #3751

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 12, 2021
Merged

fix: check uri.path is string in pathToFsPath #3751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/node/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import * as path from "path"
import safeCompare from "safe-compare"
import * as util from "util"
import xdgBasedir from "xdg-basedir"
import { getFirstString } from "../common/util"

export interface Paths {
data: string
Expand Down Expand Up @@ -457,10 +458,17 @@ 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, keepDriveLetterCasing = false): string {
export function pathToFsPath(path: string | string[], keepDriveLetterCasing = false): string {
const isWindows = process.platform === "win32"
const uri = { authority: undefined, 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}.`,
)
}
Comment on lines +466 to +470
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFirstString can only return a string or undefined so this could be something like:

if (!uri.path) {
  throw new Error("no path was provided")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do


if (uri.authority && uri.path.length > 1 && uri.scheme === "file") {
// unc path: file://shares/c$/far/boo
value = `//${uri.authority}${uri.path}`
Expand Down
41 changes: 41 additions & 0 deletions test/unit/node/util.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,3 +453,44 @@ describe("escapeHtml", () => {
)
})
})

describe("pathToFsPath", () => {
it("should convert a path to a file system path", () => {
expect(util.pathToFsPath("/foo/bar/baz")).toBe("/foo/bar/baz")
})
it("should lowercase drive letter casing by default", () => {
expect(util.pathToFsPath("/C:/far/boo")).toBe("c:/far/boo")
})
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this function can take an object we should update the types on it I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't.

I was just testing this logic

  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}.`,
    )
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops I mean remove @ts-expect-error then update pathToFsPath

export function pathToFsPath(path: string | string[] | object, keepDriveLetterCasing = false): string

Or in other words if pathToFsPath can accept an object the types should reflect that.

Although I don't think we should make it accept a object so it probably makes more sense to remove this test. We don't currently have anywhere in the code where this arg might be an object (I think! I could be wrong.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry! I think my first comment was confusing. I added this piece of logic

  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}.`,
    )
  }

to make sure we handled the scenario where something other than a string was being passed in. I think there is somewhere in the codebase where we're passing in something that we typecast as a string but I'm not sure 🤔

In order to test that, I need to make path something other than a string. I chose object. And that's why I'm using
// @ts-expect-error We need to check other types.

Copy link
Member

@code-asher code-asher Jul 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! What I mean is that there are two scenarios:

  1. You should be able to pass non-string arguments to pathToFsPath.
  2. You should not be able to pass non-string arguments to pathToFsPath.

If the first is true then the types in the function signature should reflect this (with any or whatever the list of possible types are). So this would entail changing the signature to something like path: any instead of path: string. Otherwise, if we can't trust our types then we'd need to add type guards in every function! (And that means testing every function with invalid arguments too---that'd be rough lol.)

If the second is true then the code that is calling pathToFsPath with invalid arguments is incorrect/broken and we will need to fix it. Usually TypeScript catches this for us but there are some cases where the data at some boundary comes in as any so we have to look out for those. I don't think that's the case here though.

In our case the data comes in as string | string[] | qs.ParsedQs | qs.ParsedQs[] | undefined. We then check that the argument is a string before calling pathToFsPath so the second option should suit our purposes:

https://github.com/cdr/code-server/blob/717eaa64704f69c763d6fa6502d7b1c1ba9d628f/src/node/routes/static.ts#L19-L22

https://github.com/cdr/code-server/blob/ddee4f748c693c26b19af964994907c7eeef6828/src/node/routes/vscode.ts#L66-L68

https://github.com/cdr/code-server/blob/ddee4f748c693c26b19af964994907c7eeef6828/src/node/routes/vscode.ts#L76-L78

So why do we get an error? I'm not 100% sure but I think what's happening is the tool is thinking that since req.query is an object it can be manipulated in between the if and the function call which can turn it into an invalid argument (like if someone did req.query.path = {}).

TypeScript is able to detect this and show an error but whatever tool reported this apparently can't.

If that's the case then we'll need to extract it to a constant first, e.g.:

const path = getFirstString(req.query.path)
if (path) { 
  res.set("Content-Type", getMediaMime(path)) 
  res.send(await fs.readFile(pathToFsPath(path)))

Then our pathToFsPath can purely accept string without having to do any argument manipulation itself.

.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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can remove.

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")
})
it("should replace / with \\ on Windows", () => {
let ORIGINAL_PLATFORM = process.platform

Object.defineProperty(process, "platform", {
value: "win32",
})

expect(util.pathToFsPath("/C:/far/boo")).toBe("c:\\far\\boo")

Object.defineProperty(process, "platform", {
value: ORIGINAL_PLATFORM,
})
})
})