esm: do not truncate data: URLs for evaluation

[targos]

Fixes: #53775

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/60273/

[LiviaMedeiros]

Perhaps we're lacking tests for such cases, but this change breaks the URL interpretation that is supposed to work with any imported URLs:

$ node-main -e 'import(`data:text/javascript,const u = new URL(import.meta.url); console.log(u, u.searchParams.get("a"), u.hash);?a=b&c=d#h`)'
data: b #h

$ node-pr53778 -e 'import(`data:text/javascript,const u = new URL(import.meta.url); console.log(u, u.searchParams.get("a"), u.hash);?a=b&c=d#h`)'
data:text/javascript,const u = new URL(import.meta.url); console.log(u, u.searchParams.get("a"), u.hash);?a=b&c=d#h:1
const u = new URL(import.meta.url); console.log(u, u.searchParams.get("a"), u.hash);?a=b&c=d#h
                                                                                    ^

SyntaxError: Unexpected token '?'
    at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:164:18)
    at callTranslator (node:internal/modules/esm/loader:439:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:445:30)
    at async ModuleJob._link (node:internal/modules/esm/module_job:106:19)

Node.js v23.0.0-pre

I'm -1 on this change even as semver-major PRs that contain breaking changes and should be released in the next major version. .

I think, we should be strict and keep treating URLs as per-spec URLs, rather than just a capricious string that likes prefix with colon, percent-encoding, and truncates everything after ? or #.

[targos]

So you think web browsers are wrong?

I don't really care about which behavior we have, but I think either the spec or the browsers must be fixed if we don't make this change.

[targos]

Your snippet gives the same syntax error in both Chrome and Firefox.

[LiviaMedeiros]

Browsers are both wrong and inconsistent here, unfortunately.

> await import('data:text/javascript,console.log(new URL(import.meta.url).hash)#123')
#123

> await import('data:text/javascript,console.log(new URL(import.meta.url).search)?123')
[errors in both Chrome and Firefox]

$ node-pr53778 -e 'import(`data:text/javascript,console.log(new URL(import.meta.url).hash)#123`)'
[same SyntaxError]

[aduh95]

Browsers are both wrong and inconsistent here, unfortunately.

> await import('data:text/javascript,console.log(new URL(import.meta.url).hash)#123')
#123

> await import('data:text/javascript,console.log(new URL(import.meta.url).search)?123')
[errors in both Chrome and Firefox]

$ node-pr53778 -e 'import(`data:text/javascript,console.log(new URL(import.meta.url).hash)#123`)'
[same SyntaxError]

I can confirm the same happens in Safari. However, I'm not sure browsers are inconsistant, e.g. if you try the following:

> await import('data:text/javascript,console.log(new URL(import.meta.url).search)?123:45')
?123:45

I think that can be explained by the fact they use the full URL (including "query" and "hash") as part of the script, and if it parses to valid JS (which works because # is interpreted as a single line comment, I assume). Then you use WHATWG URL class to parse it, and that API treats data: URLs as other protocols (i.e. it reports a hash for the URL).

[LiviaMedeiros]

The inconsistency is that they treat search part as code but hash part as throwaway suffix.

which works because # is interpreted as a single line comment, I assume

I can assume the same as well as historical reasons behind it, but I can't think of any spec that lines up with this. AFAIK the only thing that is interpreted like that in JS is #! in the very beginning of script. eval(123#123) or just 123 # am comment would yield syntax error.

Nowadays # is a part of JS syntax (private properties), and attempting to use it raw would give syntax error as well. Even a /* */ comment with # inside would. IMHO allowing ? :/?./??/??= without percent-encoding just endorses a footgun here, the code must be always urlencoded.

[jasnell]

It is important to note that per the fetch spec data URLs carry no semantics for query strings at all and are explicitly required to ignore hash fragments. The interpretation of the data portion of the URL is entirely dependent on the mime type established. Given the spec, this PR is definitely not quite correct in that it really should ignore the hash fragment and omit that from the result. The query string, however, should be ignored and passed through as part of the data. For the example in the test to be correct here, the hash in the test should be percent encoded. If it is not percent encoded, it should be properly interpreted as an ignored hash fragment.

    const urlWithUnescapedQueryAndHash = 'data:text/javascript,export const value="?query#hash"';
    const { value } = await import(urlWithUnescapedQueryAndHash);
    // This should be an error with invalid syntax
    assert.strictEqual(value, '?query#hash');

    const urlWithUnescapedQueryAndHash = 'data:text/javascript,export const value="?query%23hash"';
    const { value } = await import(urlWithUnescapedQueryAndHash);
    // This is valid and should work...
    assert.strictEqual(value, '?query#hash');

Accidentally hit approve. Meant Request Changes

[jasnell]

I'm in agreement that this change is not quite correct. Data URLs should be required to have the proper percent encoding and interpreted to spec.

[aduh95]

The inconsistency is that they treat search part as code but hash part as throwaway suffix.

Yeah you're right, the fragment is not part of the script, we can see it with e.g.

await import("data:text/javascript,export default '#hash'")

This produces a syntax error, and if you click on the link to see the source we can see that the script doesn't contain the #hash'.
I assume it has to do with hashes not being sent to the servers, only the query, and browser engine is emulating that as well, but I'm only guessing at this point

[targos]

Anyone feel free to open another PR if there's a fix to do on our side.