Rollup adapter is not resolving virtual modules correctly in windows environments

[giamir]

I have bumped into an issue where the rollupAdapter is not resolving virtual modules due to the presence of a conventional null byte which messes up with few guard conditions and tricks the adapter to believe that the import specifier (source in the snippet below) is a relative url.

  const injectedFilePath = path.normalize(source).startsWith(rootDir);
 
 if (!injectedFilePath && !path.isAbsolute(source) && whatwgUrl.parseURL(source) != null) {
  // don't resolve relative and valid urls
  return source;
}

In the above snippet of code when the source is something like "\x00C:\\Users\\giamir\\Code\\MyRepo\\node_modules\\@testing-library\\dom\\dist\\helpers.js?commonjs-exports" then:

• injectedFilePath evaluates to false
• path.isAbsolute(source) evaluates to false
• whatwgUrl.parseURL(source) evaluates to an object (C:\Users\etc is considered a valid url by the whatwgUrl package)

This prevent the virtual module from being resolved correctly by the dev server (instead the server try to access a file directly from disk C:\\Users\\giamir\\Code\\MyRepo\\node_modules\\@testing-library\\dom\\dist\\helpers.js?commonjs-exports)

The only reason why this is not happening in POSIX systems is beacuse whatwgUrl.parseURL(source) evaluate to null (/Users/etc is not considered a valid url by the whatwgUrl package).

I am planning to open a small PR which performs the absolute path check in the import specifier (source) stripped out of the null byte prefix:

path.isAbsolute(source.replace('\0', '')

[thepassle]

I had to revert this change because it seems to have broken things for non-windows environments 🙃

[giamir]

Thanks @thepassle for merging the PR in the first place.
How this small fix is breaking non-windows environments? Is there an issue I can look at?
I tested the fix across macos, ubuntu and windows for our test suite and it all worked fine.

[thepassle]

My colleague pinpointed the issue down to this change in the adapter, im not at my laptop rn but ive asked him to reply to you 🙂

[thepassle]

Apparently the issue had to do with data URIs: "data:text/javascript;base64,<etc>"

[giamir]

Ok understood. Then the solution is to relax the regexp to accept data schemes too (^(https?|data)$).
I can go ahead and prepare a small PR for that.
Do you or your colleague know if there is any other scheme apart from https, http, and data that should be supported?

[akrawitz]

Thanks for working on this - I'm also hitting this issue.

Another approach would be to check for a starting null byte:

if (!injectedFilePath && !path.isAbsolute(source) && whatwgUrl.parseURL(source) != null && !source.startsWith('\0')) {
  // don't resolve relative and valid urls
  return source;
}

As I understand it, in this context, the null byte is a clear signal that this is internal to the plugin, and not a "relative and valid url". This would avoid the need to enumerate url schemes.

[giamir]

@akrawitz That would work too and it is more explicit than enumerating schemes in my opinion (tagging @43081j since that was their initial suggestion).
We could even go a step further and create a local variable for additional readability

const isPluginPath = source.startsWith('\0');

@akrawitz How do you feel about opening a small PR?

[43081j]

makes sense to me, i had wrongly assumed the source would only ever be a http(s).

although to be honest, that whole condition is a little flaky IMO.

you can tell whoever originally wrote it used whatwg as a kind of shortcut to being able to pass through URLs (rather than having to tag them higher up the stack explicitly somehow). adding conditions to it might make it even more flaky.

e.g. what if in future someone comes along and passes a windows path through without a null byte? lets say some plugin decided to do that, even if its unconventional its out of our hands (in that it is possible).

though maybe its impossible to get a non-prefixed windows path to that point, even via a plugin, im not sure.

if we wanted to maintain the same amount of coverage the original condition had, it may be worth just checking for windows paths explicitly and keeping the original logic:

const isWindowsPath = /^[A-Za-z]:[\\\]/;
if (!injectedFilePath && !path.isAbsolute(source) && whatwgUrl.parseURL(source) != null && !isWindowsPath.test(source)) {

but maybe im overthinking it and you can't get a windows path to that point without a null byte anyway (so the suggestion further up is fine).

[akrawitz]

If the path is absolute and not prefixed, then !path.isAbsolute(source) will correctly return false on Windows and *nix. If the path is absolute and is prefixed, then on *nix, whatwgUrl.parseURL(source) != null returns false, but on Windows it returns true, because the leading null byte is eaten and C: is interpreted as a URL scheme.

So the "loophole" is specifically the case with a null byte followed by an absolute path on Windows. As far as I can tell, the other cases are already handled correctly. (With the caveat that I don't claim to fully understand how this code works - it's crazy how many times the same references pass back through here - first as bare specifiers, then as full paths, then as virtual paths, then with postfixes like ?commonjs-proxy and ?commonjs-module, etc... - the whole thing seems Rube-Goldberg-esque IMO).

So, the check would need to be for an absolute windows path prefixed with a null byte. But really, in this context, anything prefixed with a null byte is, by definition, not a "relative and valid url" but rather some sort of internal Rollup plugin business.

I would also recommend avoiding any "hand-coded" path logic, because it seems to invite issues (for example, what if in the future Windows allows non-ASCII drive letters, etc...).

@giamir Good idea for local variable, that does make it clearer.

I will get a PR together...

[43081j]

the whole thing seems Rube-Goldberg-esque

hah this gave me a good chuckle.

they probably built it that way so each handler of each step can be a plugin, i imagine. otherwise, they'd need some separate plugin hooks or a monolithic one which does all of this.

i agree though its not ideal. the thing holding this architectural design back is the fact that we're storing state in these paths basically.

So the "loophole" is specifically the case with a null byte followed by an absolute path on Windows

it is, that we're aware of.

i was pointing out that it may be possible plugins could cause windows paths to be passed along here without the null byte, and they would successfully be parsed by whatwg-url. so if a plugin wanted to later intercept that windows path, it probably couldn't (unless it puts a null byte in).

its probably a non-issue, i was more pointing out/questioning if its possible and worth taking account of.