Fix bug in package URL resolution due to startsWith containment check. #902
Conversation
If a dependency name began with the name of the package it was being
resolved from, we would not resolve the dependency to the components
directory, because we were checking for containment naively with string
startsWith.
Real life example:
- "/repos/iron-icons/a.html" imports "../iron-iconset-svg/b.html"
- We test "/repos/iron-iconset-svg/b.html".startsWith("/repos/iron-icons")
and did not rewrite the url into the components directory, because
it looked like it was already inside the package.
aomarks
force-pushed
the
package-resolve-bug
branch
from
7c42935 to
4227989
Compare
| @@ -30,15 +30,13 @@ export class FSUrlLoader implements UrlLoader { | |||
| root: string; | |||
|
|
|||
| constructor(root: string = '') { | |||
| if (root.endsWith('/')) { | |||
There was a problem hiding this comment.
Seemed like this was both wrong (shouldn't it be not endsWith?) and also unnecessary, because the resolve right after normalizes the slash away anyway.
| return url.startsWith('file://') && | ||
| isPathInside(this.root, Uri.parse(url).fsPath); | ||
| const parsed = Uri.parse(url); | ||
| return parsed.scheme === 'file' && !parsed.authority && |
There was a problem hiding this comment.
Previously this was testing using startsWith, so if there was a host/authority (which we have a test for), there would be a missing slash, and it worked out. Now we need to explicitly check there is no authority.
src/core/utils.ts
Outdated
| // If the path from the directory to the file does not require traversing up, | ||
| // then it must be either a descendent or the same directory. Note this is | ||
| // case-insensitive on Windows (which is what we want). | ||
| return !pathlib.relative(directory, file).startsWith('..'); |
There was a problem hiding this comment.
A small issue that probably won't come up:
I think isPathInside('c:\\foo', 'd:\\bar') returns true right now. Maybe we should add an extra check if we're on windows?
There was a problem hiding this comment.
Good catch. I've just switched to pull in path-is-inside, which already handles this case correctly. Note that it has the parameters in reverse order (which is the more intuitive order to me anyway). Also filed domenic/path-is-inside#7 to add typings there, but added custom ones here for now.
| @@ -93,6 +93,7 @@ | |||
| "jsonschema": "^1.1.0", | |||
| "minimatch": "^3.0.4", | |||
| "parse5": "^4.0.0", | |||
| "path-is-inside": "^1.0.2", | |||
There was a problem hiding this comment.
Cool, zero dependencies and written by Domenic
If a dependency name began with the name of the package it was being resolved from, we would not resolve the dependency to the components directory, because we were checking for containment naively with string
startsWith.Real life example:
/repos/iron-icons/a.htmlimports../iron-iconset-svg/b.html"/repos/iron-iconset-svg/b.html".startsWith("/repos/iron-icons")and did not rewrite the url into the components directory, because it looked like it was already inside the package.