Skip to content
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.

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

Import data: from data: should be allowed #53992

Closed
inca opened this issue Jul 22, 2024 · 2 comments
Closed

Import data: from data: should be allowed #53992

inca opened this issue Jul 22, 2024 · 2 comments
Labels
experimental Issues and PRs related to experimental features. loaders Issues and PRs related to ES module loaders

Comments

@inca
Copy link

inca commented Jul 22, 2024

Version

20.15.1 (LTS) up to 22.5.1

Platform

Darwin Boriss-MacBook-Pro.local 23.1.0 Darwin Kernel Version 23.1.0: Mon Oct  9 21:27:24 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T6000 arm64

Subsystem

No response

What steps will reproduce the bug?

Minimal reproducible scenario:

// test.mjs
function codeToUrl(code) {
    return `data:text/javascript;base64,${btoa(unescape(encodeURIComponent(code)))}`;
}

const code1 = `console.log("Hello")`;

const code2 = `
await import("${codeToUrl(code1)}");
`

await import(codeToUrl(code2));

Running this without --experimental-network-imports works fine (and is expected).

$ node test.mjs
Hello

However, adding --experimental-network-imports throws ERR_NETWORK_IMPORT_DISALLOWED even though no network import occurs.

$ node --experimental-network-imports test.mjs 
(node:70234) ExperimentalWarning: Network Imports is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/modules/esm/resolve:1114
      throw new ERR_NETWORK_IMPORT_DISALLOWED(
            ^

Error [ERR_NETWORK_IMPORT_DISALLOWED]: import of 'data:text/javascript;base64,Y29uc29sZS5sb2coIkhlbGxvIik=' by data:text/javascript;base64,CmF3YWl0IGltcG9ydCgiZGF0YTp0ZXh0L2phdmFzY3JpcHQ7YmFzZTY0LFkyOXVjMjlzWlM1c2IyY29Ja2hsYkd4dklpaz0iKTsK is not supported: import data: from a non file: is not allowed

The issue clearly comes from this code branch which specifically checks for:

  • data: protocol in the import() statement
  • file: protocol in the parent module
  • existence of --experimental-network-imports

Based on the subsequent branch it seems like importing data: is supposed to be allowed everywhere (of course http: and https: sources being only accepted with the flag), and it looks like the case where data: imports from another data: was overlooked.

How often does it reproduce? Is there a required condition?

Consistently reproducible with the steps described above.

What is the expected behavior? Why is that the expected behavior?

The expectation is that the code snippet works with and without --experimental-network-imports.

What do you see instead?

ERR_NETWORK_IMPORT_DISALLOWED as per above.

Additional information

Importing data: from another data: may not be the most practical use case, but it's rather important from the correctness PoV. I personally don't see any security implications (e.g. as long as there are no relative imports occurring inside data: modules — but this issue here appears to be unrelated to those).

@RedYetiDev RedYetiDev added loaders Issues and PRs related to ES module loaders experimental Issues and PRs related to experimental features. labels Jul 22, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jul 28, 2024

Thanks for the report.

I personally don't see any security implications

There are unfortunately. We cannot support both "importing data: URLs from data: modules" and "importing data: URLs from https: modules" while blocking "accessing node: modules from https: modules" – or at least I couldn't find a way.

Anyways, --experimental-network-imports has been removed in #53822, so this issue no longer applies. Closing.

@aduh95 aduh95 closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2024
@inca
Copy link
Author

inca commented Jul 28, 2024

Thank you, appreciate your time looking into it.

If node: modules would map to file: internally, then it all boils down to just one rule: file: can only be imported from file: (which means, importing file: is forbidden from either https: or data:). On the other hand, importing data: should be allowed from everywhere (file:, https:, other data:). This also matches how browser imports would work.

Anyways, looks like dropping support for importing https: would be a much larger impact for our use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issues and PRs related to experimental features. loaders Issues and PRs related to ES module loaders
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants