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

Dynamic import should respect permissions #2764

Merged
merged 3 commits into from
Aug 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions cli/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,12 @@ fn op_fetch_source_file(
let specifier = inner.specifier().unwrap();
let referrer = inner.referrer().unwrap();

let resolved_specifier = state.resolve(specifier, referrer, false)?;
// TODO(ry) Maybe a security hole. Only the compiler worker should have access
// to this. Need a test to demonstrate the hole.
let is_dyn_import = false;

let resolved_specifier =
state.resolve(specifier, referrer, false, is_dyn_import)?;

let fut = state
.file_fetcher
Expand Down Expand Up @@ -750,7 +755,7 @@ fn op_fetch(
let req = msg_util::deserialize_request(header, body)?;

let url_ = url::Url::parse(url).map_err(ErrBox::from)?;
state.check_net_url(url_)?;
state.check_net_url(&url_)?;

let client = http_util::get_client();

Expand Down
4 changes: 2 additions & 2 deletions cli/permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ impl DenoPermissions {
}
}

pub fn check_net_url(&self, url: url::Url) -> Result<(), ErrBox> {
pub fn check_net_url(&self, url: &url::Url) -> Result<(), ErrBox> {
let msg = &format!("network access to \"{}\"", url);
match self.allow_net.get_state() {
PermissionAccessorState::Allow => {
Expand Down Expand Up @@ -627,7 +627,7 @@ mod tests {

for (url_str, is_ok) in url_tests.iter() {
let u = url::Url::parse(url_str).unwrap();
assert_eq!(*is_ok, perms.check_net_url(u).is_ok());
assert_eq!(*is_ok, perms.check_net_url(&u).is_ok());
}

for (domain, is_ok) in domain_tests.iter() {
Expand Down
36 changes: 34 additions & 2 deletions cli/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::compilers::JsCompiler;
use crate::compilers::JsonCompiler;
use crate::compilers::TsCompiler;
use crate::deno_dir;
use crate::deno_error::permission_denied;
use crate::file_fetcher::SourceFileFetcher;
use crate::flags;
use crate::global_timer::GlobalTimer;
Expand Down Expand Up @@ -119,6 +120,7 @@ impl Loader for ThreadSafeState {
specifier: &str,
referrer: &str,
is_main: bool,
is_dyn_import: bool,
) -> Result<ModuleSpecifier, ErrBox> {
if !is_main {
if let Some(import_map) = &self.import_map {
Expand All @@ -128,8 +130,14 @@ impl Loader for ThreadSafeState {
}
}
}
let module_specifier =
ModuleSpecifier::resolve_import(specifier, referrer)?;

ModuleSpecifier::resolve_import(specifier, referrer).map_err(ErrBox::from)
if is_dyn_import {
self.check_dyn_import(&module_specifier)?;
}

Ok(module_specifier)
}

/// Given an absolute url, load its source code.
Expand Down Expand Up @@ -294,7 +302,7 @@ impl ThreadSafeState {
}

#[inline]
pub fn check_net_url(&self, url: url::Url) -> Result<(), ErrBox> {
pub fn check_net_url(&self, url: &url::Url) -> Result<(), ErrBox> {
self.permissions.check_net_url(url)
}

Expand All @@ -303,6 +311,30 @@ impl ThreadSafeState {
self.permissions.check_run()
}

pub fn check_dyn_import(
self: &Self,
module_specifier: &ModuleSpecifier,
) -> Result<(), ErrBox> {
let u = module_specifier.as_url();
match u.scheme() {
"http" | "https" => {
self.check_net_url(u)?;
Ok(())
}
"file" => {
let filename = u
.to_file_path()
.unwrap()
.into_os_string()
.into_string()
.unwrap();
self.check_read(&filename)?;
Ok(())
}
_ => Err(permission_denied()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A "unsupported import url" or something error would be more appropriate here.

}
}

#[cfg(test)]
pub fn mock(argv: Vec<String>) -> ThreadSafeState {
ThreadSafeState::new(
Expand Down
31 changes: 23 additions & 8 deletions core/modules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub trait Loader: Send + Sync {
specifier: &str,
referrer: &str,
is_main: bool,
is_dyn_import: bool,
) -> Result<ModuleSpecifier, ErrBox>;

/// Given ModuleSpecifier, load its source code.
Expand Down Expand Up @@ -125,12 +126,15 @@ impl<L: Loader> RecursiveLoad<L> {

fn add_root(&mut self) -> Result<(), ErrBox> {
let module_specifier = match self.state {
State::ResolveMain(ref specifier) => {
self.loader.resolve(specifier, ".", true)?
}
State::ResolveImport(ref specifier, ref referrer) => {
self.loader.resolve(specifier, referrer, false)?
}
State::ResolveMain(ref specifier) => self.loader.resolve(
specifier,
".",
true,
self.dyn_import_id().is_some(),
)?,
State::ResolveImport(ref specifier, ref referrer) => self
.loader
.resolve(specifier, referrer, false, self.dyn_import_id().is_some())?,
_ => unreachable!(),
};

Expand Down Expand Up @@ -160,7 +164,12 @@ impl<L: Loader> RecursiveLoad<L> {
referrer: &str,
parent_id: deno_mod,
) -> Result<(), ErrBox> {
let module_specifier = self.loader.resolve(specifier, referrer, false)?;
let module_specifier = self.loader.resolve(
specifier,
referrer,
false,
self.dyn_import_id().is_some(),
)?;
let module_name = module_specifier.as_str();

let mut modules = self.modules.lock().unwrap();
Expand Down Expand Up @@ -277,7 +286,12 @@ impl<L: Loader> ImportStream for RecursiveLoad<L> {
|specifier: &str, referrer_id: deno_mod| -> deno_mod {
let modules = self.modules.lock().unwrap();
let referrer = modules.get_name(referrer_id).unwrap();
match self.loader.resolve(specifier, &referrer, is_main) {
match self.loader.resolve(
specifier,
&referrer,
is_main,
self.dyn_import_id().is_some(),
) {
Ok(specifier) => modules.get_id(specifier.as_str()).unwrap_or(0),
// We should have already resolved and Ready this module, so
// resolve() will not fail this time.
Expand Down Expand Up @@ -675,6 +689,7 @@ mod tests {
specifier: &str,
referrer: &str,
_is_root: bool,
_is_dyn_import: bool,
) -> Result<ModuleSpecifier, ErrBox> {
let referrer = if referrer == "." {
"file:///"
Expand Down
2 changes: 1 addition & 1 deletion tests/013_dynamic_import.test
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
args: tests/013_dynamic_import.ts --reload
args: tests/013_dynamic_import.ts --reload --allow-read
output: tests/013_dynamic_import.ts.out
2 changes: 1 addition & 1 deletion tests/014_duplicate_import.test
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
args: tests/014_duplicate_import.ts --reload
args: tests/014_duplicate_import.ts --reload --allow-read
output: tests/014_duplicate_import.ts.out
2 changes: 1 addition & 1 deletion tests/015_duplicate_parallel_import.test
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
args: tests/015_duplicate_parallel_import.js --reload
args: tests/015_duplicate_parallel_import.js --reload --allow-read
output: tests/015_duplicate_parallel_import.js.out
2 changes: 1 addition & 1 deletion tests/042_dyn_import_evalcontext.test
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
args: run --reload tests/042_dyn_import_evalcontext.ts
args: run --allow-read --reload tests/042_dyn_import_evalcontext.ts
output: tests/042_dyn_import_evalcontext.ts.out
2 changes: 1 addition & 1 deletion tests/error_014_catch_dynamic_import_error.test
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
args: tests/error_014_catch_dynamic_import_error.js --reload
args: tests/error_014_catch_dynamic_import_error.js --reload --allow-read
output: tests/error_014_catch_dynamic_import_error.js.out
3 changes: 3 additions & 0 deletions tests/error_015_dynamic_import_permissions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
(async () => {
await import("http://localhost:4545/tests/subdir/mod4.js");
})();
1 change: 1 addition & 0 deletions tests/error_015_dynamic_import_permissions.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error: Uncaught TypeError: permission denied
4 changes: 4 additions & 0 deletions tests/error_015_dynamic_import_permissions.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
args: --reload --no-prompt tests/error_015_dynamic_import_permissions.js
output: tests/error_015_dynamic_import_permissions.out
check_stderr: true
exit_code: 1
5 changes: 5 additions & 0 deletions tests/error_016_dynamic_import_permissions2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// If this is executed with --allow-net but not --allow-read the following
// import should cause a permission denied error.
(async () => {
await import("http://localhost:4545/tests/subdir/evil_remote_import.js");
})();
2 changes: 2 additions & 0 deletions tests/error_016_dynamic_import_permissions2.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[WILDCARD]
error: Uncaught TypeError: permission denied
5 changes: 5 additions & 0 deletions tests/error_016_dynamic_import_permissions2.test
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# We have an allow-net flag but not allow-read, it should still result in error.
args: --no-prompt --reload --allow-net tests/error_016_dynamic_import_permissions2.js
output: tests/error_016_dynamic_import_permissions2.out
check_stderr: true
exit_code: 1
4 changes: 4 additions & 0 deletions tests/subdir/evil_remote_import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// We expect to get a permission denied error if we dynamically
// import this module without --allow-read.
export * from "file:///c:/etc/passwd";
console.log("Hello from evil_remote_import.js");