summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Sherret <dsherret@users.noreply.github.com>2023-08-17 12:14:22 -0400
committerGitHub <noreply@github.com>2023-08-17 16:14:22 +0000
commitf343391a9f97d29ad287f247c06aa370eb7cab50 (patch)
treeeeed398c9506fde5c20d3e552edd8a432bc5a2ae
parent6082e51094e0b5835d7c83586766cc611da2a382 (diff)
fix(unstable): disable importing from the vendor directory (#20067)
Some people might get think they need to import from this directory, which could cause confusion and duplicate dependencies. Additionally, the `vendor` directory has special behaviour in the language server, so importing from the folder will definitely cause confusion and issues there.
-rw-r--r--cli/factory.rs12
-rw-r--r--cli/lsp/documents.rs13
-rw-r--r--cli/resolver.rs48
-rw-r--r--cli/tests/integration/lsp_tests.rs42
-rw-r--r--cli/tests/integration/run_tests.rs13
-rw-r--r--cli/tools/vendor/test.rs12
-rw-r--r--test_util/src/lsp.rs1
7 files changed, 121 insertions, 20 deletions
diff --git a/cli/factory.rs b/cli/factory.rs
index 6a99bb2da..dbf9bd95b 100644
--- a/cli/factory.rs
+++ b/cli/factory.rs
@@ -38,6 +38,7 @@ use crate::npm::NpmPackageFsResolver;
use crate::npm::NpmResolution;
use crate::npm::PackageJsonDepsInstaller;
use crate::resolver::CliGraphResolver;
+use crate::resolver::CliGraphResolverOptions;
use crate::standalone::DenoCompileBinaryWriter;
use crate::tools::check::TypeChecker;
use crate::util::progress_bar::ProgressBar;
@@ -424,13 +425,18 @@ impl CliFactory {
.resolver
.get_or_try_init_async(async {
Ok(Arc::new(CliGraphResolver::new(
- self.options.to_maybe_jsx_import_source_config()?,
- self.maybe_import_map().await?.clone(),
- self.options.no_npm(),
self.npm_api()?.clone(),
self.npm_resolution().await?.clone(),
self.package_json_deps_provider().clone(),
self.package_json_deps_installer().await?.clone(),
+ CliGraphResolverOptions {
+ maybe_jsx_import_source_config: self
+ .options
+ .to_maybe_jsx_import_source_config()?,
+ maybe_import_map: self.maybe_import_map().await?.clone(),
+ maybe_vendor_dir: self.options.vendor_dir_path(),
+ no_npm: self.options.no_npm(),
+ },
)))
})
.await
diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs
index 439cf547d..8227d2e4c 100644
--- a/cli/lsp/documents.rs
+++ b/cli/lsp/documents.rs
@@ -21,6 +21,7 @@ use crate::npm::CliNpmRegistryApi;
use crate::npm::NpmResolution;
use crate::npm::PackageJsonDepsInstaller;
use crate::resolver::CliGraphResolver;
+use crate::resolver::CliGraphResolverOptions;
use crate::util::glob;
use crate::util::path::specifier_to_file_path;
use crate::util::text_encoding;
@@ -1241,13 +1242,19 @@ impl Documents {
Arc::new(PackageJsonDepsProvider::new(maybe_package_json_deps));
let deps_installer = Arc::new(PackageJsonDepsInstaller::no_op());
self.resolver = Arc::new(CliGraphResolver::new(
- maybe_jsx_config,
- options.maybe_import_map,
- false,
options.npm_registry_api,
options.npm_resolution,
deps_provider,
deps_installer,
+ CliGraphResolverOptions {
+ maybe_jsx_import_source_config: maybe_jsx_config,
+ maybe_import_map: options.maybe_import_map,
+ maybe_vendor_dir: options
+ .maybe_config_file
+ .and_then(|c| c.vendor_dir_path())
+ .as_ref(),
+ no_npm: false,
+ },
));
self.imports = Arc::new(
if let Some(Ok(imports)) =
diff --git a/cli/resolver.rs b/cli/resolver.rs
index 6fa8eaabe..f78f31e8d 100644
--- a/cli/resolver.rs
+++ b/cli/resolver.rs
@@ -1,6 +1,7 @@
// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
use deno_core::anyhow::anyhow;
+use deno_core::anyhow::bail;
use deno_core::error::AnyError;
use deno_core::futures::future;
use deno_core::futures::future::LocalBoxFuture;
@@ -16,6 +17,7 @@ use deno_npm::registry::NpmRegistryApi;
use deno_runtime::deno_node::is_builtin_node_module;
use deno_semver::npm::NpmPackageReq;
use import_map::ImportMap;
+use std::path::PathBuf;
use std::sync::Arc;
use crate::args::package_json::PackageJsonDeps;
@@ -102,6 +104,7 @@ pub struct CliGraphResolver {
mapped_specifier_resolver: MappedSpecifierResolver,
maybe_default_jsx_import_source: Option<String>,
maybe_jsx_import_source_module: Option<String>,
+ maybe_vendor_specifier: Option<ModuleSpecifier>,
no_npm: bool,
npm_registry_api: Arc<CliNpmRegistryApi>,
npm_resolution: Arc<NpmResolution>,
@@ -125,8 +128,9 @@ impl Default for CliGraphResolver {
maybe_import_map: Default::default(),
package_json_deps_provider: Default::default(),
},
- maybe_default_jsx_import_source: Default::default(),
- maybe_jsx_import_source_module: Default::default(),
+ maybe_default_jsx_import_source: None,
+ maybe_jsx_import_source_module: None,
+ maybe_vendor_specifier: None,
no_npm: false,
npm_registry_api,
npm_resolution,
@@ -137,27 +141,37 @@ impl Default for CliGraphResolver {
}
}
+pub struct CliGraphResolverOptions<'a> {
+ pub maybe_jsx_import_source_config: Option<JsxImportSourceConfig>,
+ pub maybe_import_map: Option<Arc<ImportMap>>,
+ pub maybe_vendor_dir: Option<&'a PathBuf>,
+ pub no_npm: bool,
+}
+
impl CliGraphResolver {
pub fn new(
- maybe_jsx_import_source_config: Option<JsxImportSourceConfig>,
- maybe_import_map: Option<Arc<ImportMap>>,
- no_npm: bool,
npm_registry_api: Arc<CliNpmRegistryApi>,
npm_resolution: Arc<NpmResolution>,
package_json_deps_provider: Arc<PackageJsonDepsProvider>,
package_json_deps_installer: Arc<PackageJsonDepsInstaller>,
+ options: CliGraphResolverOptions,
) -> Self {
Self {
mapped_specifier_resolver: MappedSpecifierResolver {
- maybe_import_map,
+ maybe_import_map: options.maybe_import_map,
package_json_deps_provider,
},
- maybe_default_jsx_import_source: maybe_jsx_import_source_config
+ maybe_default_jsx_import_source: options
+ .maybe_jsx_import_source_config
.as_ref()
.and_then(|c| c.default_specifier.clone()),
- maybe_jsx_import_source_module: maybe_jsx_import_source_config
+ maybe_jsx_import_source_module: options
+ .maybe_jsx_import_source_config
.map(|c| c.module),
- no_npm,
+ maybe_vendor_specifier: options
+ .maybe_vendor_dir
+ .and_then(|v| ModuleSpecifier::from_directory_path(v).ok()),
+ no_npm: options.no_npm,
npm_registry_api,
npm_resolution,
package_json_deps_installer,
@@ -219,7 +233,7 @@ impl Resolver for CliGraphResolver {
referrer: &ModuleSpecifier,
) -> Result<ModuleSpecifier, AnyError> {
use MappedResolution::*;
- match self
+ let result = match self
.mapped_specifier_resolver
.resolve(specifier, referrer)?
{
@@ -232,7 +246,21 @@ impl Resolver for CliGraphResolver {
}
None => deno_graph::resolve_import(specifier, referrer)
.map_err(|err| err.into()),
+ };
+
+ // When the user is vendoring, don't allow them to import directly from the vendor/ directory
+ // as it might cause them confusion or duplicate dependencies. Additionally, this folder has
+ // special treatment in the language server so it will definitely cause issues/confusion there
+ // if they do this.
+ if let Some(vendor_specifier) = &self.maybe_vendor_specifier {
+ if let Ok(specifier) = &result {
+ if specifier.as_str().starts_with(vendor_specifier.as_str()) {
+ bail!("Importing from the vendor directory is not permitted. Use a remote specifier instead or disable vendoring.");
+ }
+ }
}
+
+ result
}
}
diff --git a/cli/tests/integration/lsp_tests.rs b/cli/tests/integration/lsp_tests.rs
index d540c5f37..d2cb36806 100644
--- a/cli/tests/integration/lsp_tests.rs
+++ b/cli/tests/integration/lsp_tests.rs
@@ -8958,5 +8958,47 @@ fn lsp_vendor_dir() {
);
assert_eq!(diagnostics.all().len(), 2);
+ // now try doing a relative import into the vendor directory
+ client.write_notification(
+ "textDocument/didChange",
+ json!({
+ "textDocument": {
+ "uri": local_file_uri,
+ "version": 2
+ },
+ "contentChanges": [
+ {
+ "range": {
+ "start": { "line": 0, "character": 0 },
+ "end": { "line": 2, "character": 0 },
+ },
+ "text": "import { returnsHi } from './vendor/subdir/mod1.ts';\nconst test: string = returnsHi();\nconsole.log(test);"
+ }
+ ]
+ }),
+ );
+
+ let diagnostics = client.read_diagnostics();
+
+ assert_eq!(
+ json!(
+ diagnostics
+ .messages_with_file_and_source(local_file_uri.as_str(), "deno")
+ .diagnostics
+ ),
+ json!([
+ {
+ "range": {
+ "start": { "line": 0, "character": 26 },
+ "end": { "line": 0, "character": 51 }
+ },
+ "severity": 1,
+ "code": "resolver-error",
+ "source": "deno",
+ "message": "Importing from the vendor directory is not permitted. Use a remote specifier instead or disable vendoring."
+ }
+ ]),
+ );
+
client.shutdown();
}
diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs
index 2ac6ed985..0c39c2b72 100644
--- a/cli/tests/integration/run_tests.rs
+++ b/cli/tests/integration/run_tests.rs
@@ -4580,4 +4580,17 @@ console.log(returnsHi());"#,
.args("run --vendor http://localhost:4545/subdir/CAPITALS/hello_there.ts")
.run()
.assert_matches_text("hello there\n");
+
+ // now try importing directly from the vendor folder
+ temp_dir.write(
+ "main.ts",
+ r#"import { returnsHi } from './vendor/http_localhost_4545/subdir/mod1.ts';
+console.log(returnsHi());"#,
+ );
+ deno_run_cmd
+ .run()
+ .assert_matches_text("error: Importing from the vendor directory is not permitted. Use a remote specifier instead or disable vendoring.
+ at [WILDCARD]/main.ts:1:27
+")
+ .assert_exit_code(1);
}
diff --git a/cli/tools/vendor/test.rs b/cli/tools/vendor/test.rs
index 0bf6f84f3..c00cc654d 100644
--- a/cli/tools/vendor/test.rs
+++ b/cli/tools/vendor/test.rs
@@ -26,6 +26,7 @@ use crate::cache::ParsedSourceCache;
use crate::npm::CliNpmRegistryApi;
use crate::npm::NpmResolution;
use crate::resolver::CliGraphResolver;
+use crate::resolver::CliGraphResolverOptions;
use super::build::VendorEnvironment;
@@ -290,7 +291,7 @@ impl VendorTestBuilder {
}
fn build_resolver(
- jsx_import_source_config: Option<JsxImportSourceConfig>,
+ maybe_jsx_import_source_config: Option<JsxImportSourceConfig>,
original_import_map: Option<ImportMap>,
) -> CliGraphResolver {
let npm_registry_api = Arc::new(CliNpmRegistryApi::new_uninitialized());
@@ -300,13 +301,16 @@ fn build_resolver(
None,
));
CliGraphResolver::new(
- jsx_import_source_config,
- original_import_map.map(Arc::new),
- false,
npm_registry_api,
npm_resolution,
Default::default(),
Default::default(),
+ CliGraphResolverOptions {
+ maybe_jsx_import_source_config,
+ maybe_import_map: original_import_map.map(Arc::new),
+ maybe_vendor_dir: None,
+ no_npm: false,
+ },
)
}
diff --git a/test_util/src/lsp.rs b/test_util/src/lsp.rs
index 949dd25d6..8a1f24b32 100644
--- a/test_util/src/lsp.rs
+++ b/test_util/src/lsp.rs
@@ -998,6 +998,7 @@ impl CollectedDiagnostics {
.unwrap()
}
+ #[track_caller]
pub fn messages_with_file_and_source(
&self,
specifier: &str,