From fb9bb51fcbb8183f7f939f74d1fca7f1ea074df5 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Mon, 8 Jul 2019 09:55:24 +0200 Subject: [PATCH] core: clearly define when module lookup is path-based vs URL-based The rules are now as follows: * In `import` statements, as mandated by the WHATWG specification, the import specifier is always treated as a URL. If it is a relative URL, it must start with either / or ./ or ../ * A script name passed to deno as a command line argument may be either an absolute URL or a local path. - If the name starts with a valid URI scheme followed by a colon, e.g. 'http:', 'https:', 'file:', 'foo+bar:', it always interpreted as a URL (even if Deno doesn't support the indicated protocol). - Otherwise, the script name is interpreted as a local path. The local path may be relative, and operating system semantics determine how it is resolved. Prefixing a relative path with ./ is not required. --- cli/compiler.rs | 4 +- cli/deno_error.rs | 3 +- cli/flags.rs | 4 +- cli/import_map.rs | 4 +- cli/msg.fbs | 3 +- cli/ops.rs | 2 +- cli/state.rs | 5 +- cli/worker.rs | 10 +- core/module_specifier.rs | 304 ++++++++++++++++-- core/modules.rs | 10 +- tests/error_011_bad_module_specifier.ts.out | 2 +- ...or_012_bad_dynamic_import_specifier.ts.out | 2 +- 12 files changed, 308 insertions(+), 45 deletions(-) diff --git a/cli/compiler.rs b/cli/compiler.rs index 872ed28a23c306..c7adbe390f193e 100644 --- a/cli/compiler.rs +++ b/cli/compiler.rs @@ -249,7 +249,7 @@ mod tests { tokio_util::init(|| { let specifier = "./tests/002_hello.ts"; use deno::ModuleSpecifier; - let module_name = ModuleSpecifier::resolve_root(specifier) + let module_name = ModuleSpecifier::resolve_url_or_path(specifier) .unwrap() .to_string(); @@ -296,7 +296,7 @@ mod tests { fn test_bundle_async() { let specifier = "./tests/002_hello.ts"; use deno::ModuleSpecifier; - let module_name = ModuleSpecifier::resolve_root(specifier) + let module_name = ModuleSpecifier::resolve_url_or_path(specifier) .unwrap() .to_string(); diff --git a/cli/deno_error.rs b/cli/deno_error.rs index 2e683c504e6ac8..6f11a68795684d 100644 --- a/cli/deno_error.rs +++ b/cli/deno_error.rs @@ -110,7 +110,8 @@ impl DenoError { use ModuleResolutionError::*; match err { InvalidUrl(err) | InvalidBaseUrl(err) => Self::url_error_kind(err), - ImportPathPrefixMissing => ErrorKind::ImportPathPrefixMissing, + InvalidPath => ErrorKind::InvalidPath, + ImportPrefixMissing => ErrorKind::ImportPrefixMissing, } } Repr::Diagnostic(ref _err) => ErrorKind::Diagnostic, diff --git a/cli/flags.rs b/cli/flags.rs index 4d68ac726128da..b3fc11380ce673 100644 --- a/cli/flags.rs +++ b/cli/flags.rs @@ -630,7 +630,9 @@ pub enum DenoSubcommand { fn get_default_bundle_filename(source_file: &str) -> String { use deno::ModuleSpecifier; - let url = ModuleSpecifier::resolve_root(source_file).unwrap().to_url(); + let url = ModuleSpecifier::resolve_url_or_path(source_file) + .unwrap() + .to_url(); let path_segments = url.path_segments().unwrap(); let last = path_segments.last().unwrap(); String::from(last.trim_end_matches(".ts").trim_end_matches(".js")) diff --git a/cli/import_map.rs b/cli/import_map.rs index 4321cacadfd4e5..44525f7a2e5007 100644 --- a/cli/import_map.rs +++ b/cli/import_map.rs @@ -178,9 +178,7 @@ impl ImportMap { continue; } - let normalized_address = ModuleSpecifier::resolve(&url_string, ".") - .expect("Address should be valid module specifier"); - normalized_addresses.push(normalized_address); + normalized_addresses.push(url.into()); } normalized_addresses diff --git a/cli/msg.fbs b/cli/msg.fbs index da181af435d989..df85a407293747 100644 --- a/cli/msg.fbs +++ b/cli/msg.fbs @@ -102,6 +102,7 @@ enum ErrorKind: byte { WouldBlock, InvalidInput, InvalidData, + InvalidPath, TimedOut, Interrupted, WriteZero, @@ -141,7 +142,7 @@ enum ErrorKind: byte { NoAsyncSupport, NoSyncSupport, ImportMapError, - ImportPathPrefixMissing, + ImportPrefixMissing, // other kinds Diagnostic, diff --git a/cli/ops.rs b/cli/ops.rs index 0664d8077f2c1d..218fb996e77a61 100644 --- a/cli/ops.rs +++ b/cli/ops.rs @@ -2053,7 +2053,7 @@ fn op_create_worker( err_check(worker.execute("denoMain()")); err_check(worker.execute("workerMain()")); - let module_specifier = ModuleSpecifier::resolve_root(specifier)?; + let module_specifier = ModuleSpecifier::resolve_url_or_path(specifier)?; let op = worker diff --git a/cli/state.rs b/cli/state.rs index fd209a0f23daaf..0562265112fab9 100644 --- a/cli/state.rs +++ b/cli/state.rs @@ -170,7 +170,8 @@ impl Loader for ThreadSafeState { } } - ModuleSpecifier::resolve(specifier, referrer).map_err(DenoError::from) + ModuleSpecifier::resolve_import(specifier, referrer) + .map_err(DenoError::from) } /// Given an absolute url, load its source code. @@ -252,7 +253,7 @@ impl ThreadSafeState { None } else { let root_specifier = argv_rest[1].clone(); - match ModuleSpecifier::resolve_root(&root_specifier) { + match ModuleSpecifier::resolve_url_or_path(&root_specifier) { Ok(specifier) => Some(specifier), Err(e) => { // TODO: handle unresolvable specifier diff --git a/cli/worker.rs b/cli/worker.rs index 65da666e87dbc7..1cf38e295102e8 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -141,7 +141,7 @@ mod tests { #[test] fn execute_mod_esm_imports_a() { let module_specifier = - ModuleSpecifier::resolve_root("tests/esm_imports_a.js").unwrap(); + ModuleSpecifier::resolve_url_or_path("tests/esm_imports_a.js").unwrap(); let argv = vec![String::from("./deno"), module_specifier.to_string()]; let state = ThreadSafeState::new( flags::DenoFlags::default(), @@ -169,7 +169,7 @@ mod tests { #[test] fn execute_mod_circular() { let module_specifier = - ModuleSpecifier::resolve_root("tests/circular1.js").unwrap(); + ModuleSpecifier::resolve_url_or_path("tests/circular1.js").unwrap(); let argv = vec![String::from("./deno"), module_specifier.to_string()]; let state = ThreadSafeState::new( flags::DenoFlags::default(), @@ -197,7 +197,7 @@ mod tests { #[test] fn execute_006_url_imports() { let module_specifier = - ModuleSpecifier::resolve_root("tests/006_url_imports.ts").unwrap(); + ModuleSpecifier::resolve_url_or_path("tests/006_url_imports.ts").unwrap(); let argv = vec![String::from("deno"), module_specifier.to_string()]; let mut flags = flags::DenoFlags::default(); flags.reload = true; @@ -327,7 +327,7 @@ mod tests { // "foo" is not a valid module specifier so this should return an error. let mut worker = create_test_worker(); let module_specifier = - ModuleSpecifier::resolve_root("does-not-exist").unwrap(); + ModuleSpecifier::resolve_url_or_path("does-not-exist").unwrap(); let result = worker.execute_mod_async(&module_specifier, false).wait(); assert!(result.is_err()); }) @@ -340,7 +340,7 @@ mod tests { // tests). let mut worker = create_test_worker(); let module_specifier = - ModuleSpecifier::resolve_root("./tests/002_hello.ts").unwrap(); + ModuleSpecifier::resolve_url_or_path("./tests/002_hello.ts").unwrap(); let result = worker.execute_mod_async(&module_specifier, false).wait(); assert!(result.is_ok()); }) diff --git a/core/module_specifier.rs b/core/module_specifier.rs index 8b8ccf4a675ee7..116e377d421da9 100644 --- a/core/module_specifier.rs +++ b/core/module_specifier.rs @@ -1,3 +1,4 @@ +use std::env::current_dir; use std::error::Error; use std::fmt; use url::ParseError; @@ -8,7 +9,8 @@ use url::Url; pub enum ModuleResolutionError { InvalidUrl(ParseError), InvalidBaseUrl(ParseError), - ImportPathPrefixMissing, + InvalidPath, + ImportPrefixMissing, } use ModuleResolutionError::*; @@ -28,7 +30,8 @@ impl fmt::Display for ModuleResolutionError { InvalidBaseUrl(ref err) => { write!(f, "invalid base URL for relative import: {}", err) } - ImportPathPrefixMissing => { + InvalidPath => write!(f, "invalid module path"), + ImportPrefixMissing => { write!(f, "relative import path not prefixed with / or ./ or ../") } } @@ -46,7 +49,7 @@ impl ModuleSpecifier { /// Resolves module using this algorithm: /// https://html.spec.whatwg.org/multipage/webappapis.html#resolve-a-module-specifier - pub fn resolve( + pub fn resolve_import( specifier: &str, base: &str, ) -> Result { @@ -64,7 +67,7 @@ impl ModuleSpecifier { || specifier.starts_with("./") || specifier.starts_with("../")) => { - Err(ImportPathPrefixMissing)? + Err(ImportPrefixMissing)? } // 3. Return the result of applying the URL parser to specifier with base @@ -76,7 +79,7 @@ impl ModuleSpecifier { // If parsing the specifier as a URL failed for a different reason than // it being relative, always return the original error. We don't want to - // return `ImportPathPrefixMissing` or `InvalidBaseUrl` if the real + // return `ImportPrefixMissing` or `InvalidBaseUrl` if the real // problem lies somewhere else. Err(err) => Err(InvalidUrl(err))?, }; @@ -84,25 +87,70 @@ impl ModuleSpecifier { Ok(ModuleSpecifier(url)) } - /// Takes a string representing a path or URL to a module, but of the type - /// passed through the command-line interface for the main module. This is - /// slightly different than specifiers used in import statements: "foo.js" for - /// example is allowed here, whereas in import statements a leading "./" is - /// required ("./foo.js"). This function is aware of the current working - /// directory and returns an absolute URL. - pub fn resolve_root( - root_specifier: &str, + /// Converts a string representing an absulute URL into a ModuleSpecifier. + pub fn resolve_url( + url_str: &str, ) -> Result { - let url = match Url::parse(root_specifier) { - Ok(url) => url, - Err(..) => { - let cwd = std::env::current_dir().unwrap(); - let base = Url::from_directory_path(cwd).unwrap(); - base.join(&root_specifier).map_err(InvalidUrl)? - } - }; + Url::parse(url_str) + .map(ModuleSpecifier) + .map_err(ModuleResolutionError::InvalidUrl) + } - Ok(ModuleSpecifier(url)) + /// Takes a string representing either an absolute URL or a file path, + /// as it may be passed to deno as a command line argument. + /// The string is interpreted as a URL if it starts with a valid URI scheme, + /// e.g. 'http:' or 'file:' or 'git+ssh:'. If not, it's interpreted as a + /// file path; if it is a relative path it's resolved relative to the current + /// working directory. + pub fn resolve_url_or_path( + specifier: &str, + ) -> Result { + if Self::specifier_has_uri_scheme(specifier) { + Self::resolve_url(specifier) + } else { + Self::resolve_path(specifier) + } + } + + /// Converts a string representing a relative or absolute path into a + /// ModuleSpecifier. A relative path is considered relative to the current + /// working directory. + fn resolve_path( + path_str: &str, + ) -> Result { + let path = current_dir().unwrap().join(path_str); + Url::from_file_path(path) + .map(ModuleSpecifier) + .map_err(|()| ModuleResolutionError::InvalidPath) + } + + /// Returns true if the input string starts with a sequence of characters + /// that could be a valid URI scheme, like 'https:', 'git+ssh:' or 'data:'. + /// + /// According to RFC 3986 (https://tools.ietf.org/html/rfc3986#section-3.1), + /// a valid scheme has the following format: + /// scheme = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) + /// + /// We additionally require the scheme to be at least 2 characters long, + /// because otherwise a windows path like c:/foo would be treated as a URL, + /// while no schemes with a one-letter name actually exist. + fn specifier_has_uri_scheme(specifier: &str) -> bool { + let mut chars = specifier.chars(); + let mut len = 0usize; + // THe first character must be a letter. + match chars.next() { + Some(c) if c.is_ascii_alphabetic() => len += 1, + _ => return false, + } + // Second and following characters must be either a letter, number, + // plus sign, minus sign, or dot. + loop { + match chars.next() { + Some(c) if c.is_ascii_alphanumeric() || "+-.".contains(c) => len += 1, + Some(':') if len >= 2 => return true, + _ => return false, + } + } } } @@ -123,3 +171,215 @@ impl PartialEq for ModuleSpecifier { &self.to_string() == other } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_resolve_import() { + let tests = vec![ + ( + "/dev/core/tests/005_more_imports.ts", + "file:///home/yeti", + "file:///dev/core/tests/005_more_imports.ts", + ), + ( + "//zombo.com/1999.ts", + "https://cherry.dev/its/a/thing", + "https://zombo.com/1999.ts", + ), + ( + "http://deno.land/this/url/is/valid", + "base is clearly not a valid url", + "http://deno.land/this/url/is/valid", + ), + ( + "//server/some/dir/file", + "file:///home/yeti/deno", + "file://server/some/dir/file", + ), + // This test is disabled because the url crate does not follow the spec, + // dropping the server part from the final result. + // ( + // "/another/path/at/the/same/server", + // "file://server/some/dir/file", + // "file://server/another/path/at/the/same/server", + // ), + ]; + + for (specifier, base, expected_url) in tests { + let url = ModuleSpecifier::resolve_import(specifier, base) + .unwrap() + .to_string(); + assert_eq!(url, expected_url); + } + } + + #[test] + fn test_resolve_import_error() { + use url::ParseError::*; + use ModuleResolutionError::*; + + let tests = vec![ + ( + "https://eggplant:b/c", + "http://deno.land/core/tests/006_url_imports.ts", + InvalidUrl(InvalidPort), + ), + ( + "https://eggplant@/c", + "http://deno.land/core/tests/006_url_imports.ts", + InvalidUrl(EmptyHost), + ), + ( + "./foo.ts", + "/relative/base/url", + InvalidBaseUrl(RelativeUrlWithoutBase), + ), + ]; + + for (specifier, base, expected_err) in tests { + let err = ModuleSpecifier::resolve_import(specifier, base).unwrap_err(); + assert_eq!(err, expected_err); + } + } + + #[test] + fn test_resolve_url_or_path() { + // Absolute URL. + let mut tests: Vec<(&str, String)> = vec![]; + + // The local path tests assume that the cwd is the deno repo root. + let cwd = current_dir().unwrap(); + let cwd_str = cwd.to_str().unwrap(); + + if cfg!(target_os = "windows") { + // Absolute local path. + let expected_url = "file:///C:/deno/tests/006_url_imports.ts"; + tests.extend(vec![ + ( + r"C:/deno/tests/006_url_imports.ts", + expected_url.to_string(), + ), + ( + r"C:\deno\tests\006_url_imports.ts", + expected_url.to_string(), + ), + ( + r"C:\deno\tests\006_url_imports.ts", + expected_url.to_string(), + ), + ( + r"\\?\C:\deno\tests\006_url_imports.ts", + expected_url.to_string(), + ), + // Not supported: `Url::from_file_path()` fails. + // (r"\\.\C:\deno\tests\006_url_imports.ts", expected_url.to_string()), + // Not supported: `Url::from_file_path()` performs the wrong conversion. + // (r"//./C:/deno/tests/006_url_imports.ts", expected_url.to_string()), + ]); + + // Rooted local path without drive letter. + let expected_url = format!( + "file:///{}:/deno/tests/006_url_imports.ts", + cwd_str.get(..1).unwrap(), + ); + tests.extend(vec![ + (r"/deno/tests/006_url_imports.ts", expected_url.to_string()), + (r"\deno\tests\006_url_imports.ts", expected_url.to_string()), + ]); + + // Relative local path. + let expected_url = format!( + "file:///{}/tests/006_url_imports.ts", + cwd_str.replace("\\", "/") + ); + tests.extend(vec![ + (r"tests/006_url_imports.ts", expected_url.to_string()), + (r"tests\006_url_imports.ts", expected_url.to_string()), + (r"./tests/006_url_imports.ts", expected_url.to_string()), + (r".\tests\006_url_imports.ts", expected_url.to_string()), + ]); + + // UNC network path. + let expected_url = "file://server/share/deno/cool"; + tests.extend(vec![ + (r"\\server\share\deno\cool", expected_url.to_string()), + (r"\\server/share/deno/cool", expected_url.to_string()), + // Not supported: `Url::from_file_path()` performs the wrong conversion. + // (r"//server/share/deno/cool", expected_url.to_string()), + ]); + } else { + // Absolute local path. + let expected_url = "file:///deno/tests/006_url_imports.ts"; + tests.extend(vec![ + ("/deno/tests/006_url_imports.ts", expected_url.to_string()), + ("//deno/tests/006_url_imports.ts", expected_url.to_string()), + ]); + + // Relative local path. + let expected_url = format!("file://{}/tests/006_url_imports.ts", cwd_str); + tests.extend(vec![ + ("tests/006_url_imports.ts", expected_url.to_string()), + ("./tests/006_url_imports.ts", expected_url.to_string()), + ]); + } + + for (specifier, expected_url) in tests { + let url = ModuleSpecifier::resolve_url_or_path(specifier) + .unwrap() + .to_string(); + assert_eq!(url, expected_url); + } + } + + #[test] + fn test_resolve_url_or_path_error() { + use url::ParseError::*; + use ModuleResolutionError::*; + + let mut tests = vec![ + ("https://eggplant:b/c", InvalidUrl(InvalidPort)), + ("https://:8080/a/b/c", InvalidUrl(EmptyHost)), + ]; + if cfg!(target_os = "windows") { + tests.push((r"\\.\c:/stuff/deno/script.ts", InvalidPath)); + } + + for (specifier, expected_err) in tests { + let err = ModuleSpecifier::resolve_url_or_path(specifier).unwrap_err(); + assert_eq!(err, expected_err); + } + } + + #[test] + fn test_specifier_has_uri_scheme() { + let tests = vec![ + ("http://foo.bar/etc", true), + ("HTTP://foo.bar/etc", true), + ("http:ftp:", true), + ("http:", true), + ("hTtP:", true), + ("ftp:", true), + ("mailto:spam@please.me", true), + ("git+ssh://git@github.com/denoland/deno", true), + ("blob:https://whatwg.org/mumbojumbo", true), + ("abc.123+DEF-ghi:", true), + ("abc.123+def-ghi:@", true), + ("", false), + (":not", false), + ("http", false), + ("c:dir", false), + ("X:", false), + ("./http://not", false), + ("1abc://kinda/but/no", false), + ("schluẞ://no/more", false), + ]; + + for (specifier, expected) in tests { + let result = ModuleSpecifier::specifier_has_uri_scheme(specifier); + assert_eq!(result, expected); + } + } +} diff --git a/core/modules.rs b/core/modules.rs index 8a229e7706a90e..c1fa5d7336564a 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -643,11 +643,11 @@ mod tests { eprintln!(">> RESOLVING, S: {}, R: {}", specifier, referrer); - let output_specifier = match ModuleSpecifier::resolve(specifier, referrer) - { - Ok(specifier) => specifier, - Err(_e) => return Err(MockError::ResolveErr), - }; + let output_specifier = + match ModuleSpecifier::resolve_import(specifier, referrer) { + Ok(specifier) => specifier, + Err(..) => return Err(MockError::ResolveErr), + }; if mock_source_code(&output_specifier.to_string()).is_some() { Ok(output_specifier) diff --git a/tests/error_011_bad_module_specifier.ts.out b/tests/error_011_bad_module_specifier.ts.out index d5a3efd7e2aefb..9eec893073431e 100644 --- a/tests/error_011_bad_module_specifier.ts.out +++ b/tests/error_011_bad_module_specifier.ts.out @@ -1,4 +1,4 @@ -[WILDCARD]error: Uncaught ImportPathPrefixMissing: relative import path not prefixed with / or ./ or ../ +[WILDCARD]error: Uncaught ImportPrefixMissing: relative import path not prefixed with / or ./ or ../ [WILDCARD] js/errors.ts:[WILDCARD] at DenoError (js/errors.ts:[WILDCARD]) at maybeError (js/errors.ts:[WILDCARD]) diff --git a/tests/error_012_bad_dynamic_import_specifier.ts.out b/tests/error_012_bad_dynamic_import_specifier.ts.out index 1a20b60a1750bf..3f88b4ee3a26cc 100644 --- a/tests/error_012_bad_dynamic_import_specifier.ts.out +++ b/tests/error_012_bad_dynamic_import_specifier.ts.out @@ -1,4 +1,4 @@ -[WILDCARD]error: Uncaught ImportPathPrefixMissing: relative import path not prefixed with / or ./ or ../ +[WILDCARD]error: Uncaught ImportPrefixMissing: relative import path not prefixed with / or ./ or ../ [WILDCARD] js/errors.ts:[WILDCARD] at DenoError (js/errors.ts:[WILDCARD]) at maybeError (js/errors.ts:[WILDCARD])