fix: check uri.path is string in pathToFsPath #3751
Conversation
jsjoeio
changed the title
Codecov Report
@@ Coverage Diff @@
## main #3751 +/- ##
==========================================
+ Coverage 62.01% 62.89% +0.87%
==========================================
Files 35 35
Lines 1835 1838 +3
Branches 370 371 +1
==========================================
+ Hits 1138 1156 +18
+ Misses 588 577 -11
+ Partials 109 105 -4
Continue to review full report at Codecov.
|
jsjoeio
force-pushed
the
jsjoeio-fix-type-confusion
branch
2 times, most recently
from
d989df0 to
6835d2d
Compare
|
Or alternatively if we want to accept arrays (to make it easier to call) we can change the type of |
|
Oh and if we allow arrays we could use |
|
I'll go with your second approach: change type to accept |
There's a chance this function can be called with a path that is not a string. To catch that, we check if path is of a different type and throw an error if it is. This also adds a couple tests for this function.
jsjoeio
force-pushed
the
jsjoeio-fix-type-confusion
branch
from
92ba58b to
7ce9ee0
Compare
There was a problem hiding this comment.
Sorry to review after merge.
Actually I'm a bit confused because we already check if the type is a string before calling pathToFsPath. Is the checker confused because of the object? If so we probably need to extract it into a variable instead, for example:
const path = getFirstString(req.query.path)
if (path) {
pathToFsPath(path)
}
| ) | ||
| }) | ||
| it("should not throw an error for a string array", () => { | ||
| // @ts-expect-error We need to check other types |
| 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 |
There was a problem hiding this comment.
If this function can take an object we should update the types on it I think.
There was a problem hiding this comment.
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}.`,
)
}There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Right! What I mean is that there are two scenarios:
- You should be able to pass non-string arguments to
pathToFsPath. - 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:
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.
| 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}.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
getFirstString can only return a string or undefined so this could be something like:
if (!uri.path) {
throw new Error("no path was provided")
}
This PR adds an extra check to the
pathToFsPathfunction to check thaturi.pathis indeed a string.We do this because CodeQL believes there is currently type confusion through parameter tampering earlier up in the codebase where this is used. See security warning here.
Fixes #3750