diff options
author | Nayeem Rahman <nayeemrmn99@gmail.com> | 2023-08-30 18:52:01 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-08-30 18:52:01 +0100 |
commit | 1cce3060227f7bc53a8d5ad938f092362cf78855 (patch) | |
tree | 9abfe9f27372291b9f04e11a4e4127bb2daf39af | |
parent | d28384c3deec1497d28f0f6bd16cf51de832e572 (diff) |
fix(runtime/permissions): Resolve executable specifiers in allowlists and queries (#14130)
Closes #14122.
Adds two extensions to `--allow-run` behaviour:
- When `--allow-run=foo` is specified and `foo` is found in the `PATH`
at startup, `RunDescriptor::Path(which("foo"))` is added to the
allowlist alongside `RunDescriptor::Name("foo")`. Currently only the
latter is.
- When run permission for `foo` is queried and `foo` is found in the
`PATH` at runtime, either `RunDescriptor::Path(which("foo"))` or
`RunDescriptor::Name("foo")` would qualify in the allowlist. Currently
only the latter does.
-rw-r--r-- | Cargo.lock | 1 | ||||
-rw-r--r-- | cli/tests/integration/run_tests.rs | 5 | ||||
-rw-r--r-- | cli/tests/testdata/allow_run_allowlist_resolution.ts | 66 | ||||
-rw-r--r-- | cli/tests/testdata/allow_run_allowlist_resolution.ts.out | 15 | ||||
-rw-r--r-- | runtime/Cargo.toml | 1 | ||||
-rw-r--r-- | runtime/permissions/mod.rs | 140 |
6 files changed, 183 insertions, 45 deletions
diff --git a/Cargo.lock b/Cargo.lock index d565cbb23..dd12b8851 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1515,6 +1515,7 @@ dependencies = [ "tokio", "tokio-metrics", "uuid", + "which", "winapi", "winres", ] diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 22096cb60..e7ff19954 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -3617,6 +3617,11 @@ itest!(followup_dyn_import_resolved { output: "run/followup_dyn_import_resolves/main.ts.out", }); +itest!(allow_run_allowlist_resolution { + args: "run --quiet --unstable -A allow_run_allowlist_resolution.ts", + output: "allow_run_allowlist_resolution.ts.out", +}); + itest!(unhandled_rejection { args: "run --check run/unhandled_rejection.ts", output: "run/unhandled_rejection.ts.out", diff --git a/cli/tests/testdata/allow_run_allowlist_resolution.ts b/cli/tests/testdata/allow_run_allowlist_resolution.ts new file mode 100644 index 000000000..c7369d928 --- /dev/null +++ b/cli/tests/testdata/allow_run_allowlist_resolution.ts @@ -0,0 +1,66 @@ +// Testing the following (but with `deno` instead of `echo`): +// | `deno run --allow-run=echo` | `which path == "/usr/bin/echo"` at startup | `which path != "/usr/bin/echo"` at startup | +// |-------------------------------------|--------------------------------------------|--------------------------------------------| +// | **`Deno.Command("echo")`** | ✅ | ✅ | +// | **`Deno.Command("/usr/bin/echo")`** | ✅ | ❌ | + +// | `deno run --allow-run=/usr/bin/echo | `which path == "/usr/bin/echo"` at runtime | `which path != "/usr/bin/echo"` at runtime | +// |-------------------------------------|--------------------------------------------|--------------------------------------------| +// | **`Deno.Command("echo")`** | ✅ | ❌ | +// | **`Deno.Command("/usr/bin/echo")`** | ✅ | ✅ | + +const execPath = Deno.execPath(); +const execPathParent = execPath.replace(/[/\\][^/\\]+$/, ""); + +const testUrl = `data:application/typescript;base64,${ + btoa(` + console.log(await Deno.permissions.query({ name: "run", command: "deno" })); + console.log(await Deno.permissions.query({ name: "run", command: "${ + execPath.replaceAll("\\", "\\\\") + }" })); + Deno.env.set("PATH", ""); + console.log(await Deno.permissions.query({ name: "run", command: "deno" })); + console.log(await Deno.permissions.query({ name: "run", command: "${ + execPath.replaceAll("\\", "\\\\") + }" })); +`) +}`; + +const process1 = await new Deno.Command(Deno.execPath(), { + args: [ + "run", + "--quiet", + "--allow-env", + "--allow-run=deno", + testUrl, + ], + stderr: "null", + env: { "PATH": execPathParent }, +}).output(); +console.log(new TextDecoder().decode(process1.stdout)); + +const process2 = await new Deno.Command(Deno.execPath(), { + args: [ + "run", + "--quiet", + "--allow-env", + "--allow-run=deno", + testUrl, + ], + stderr: "null", + env: { "PATH": "" }, +}).output(); +console.log(new TextDecoder().decode(process2.stdout)); + +const process3 = await new Deno.Command(Deno.execPath(), { + args: [ + "run", + "--quiet", + "--allow-env", + `--allow-run=${execPath}`, + testUrl, + ], + stderr: "null", + env: { "PATH": execPathParent }, +}).output(); +console.log(new TextDecoder().decode(process3.stdout)); diff --git a/cli/tests/testdata/allow_run_allowlist_resolution.ts.out b/cli/tests/testdata/allow_run_allowlist_resolution.ts.out new file mode 100644 index 000000000..16ba6754a --- /dev/null +++ b/cli/tests/testdata/allow_run_allowlist_resolution.ts.out @@ -0,0 +1,15 @@ +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "granted", onchange: null } + +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "prompt", onchange: null } +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "prompt", onchange: null } + +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "granted", onchange: null } +PermissionStatus { state: "prompt", onchange: null } +PermissionStatus { state: "granted", onchange: null } + diff --git a/runtime/Cargo.toml b/runtime/Cargo.toml index d4401effd..5ab50714d 100644 --- a/runtime/Cargo.toml +++ b/runtime/Cargo.toml @@ -109,6 +109,7 @@ termcolor = "1.1.3" tokio.workspace = true tokio-metrics.workspace = true uuid.workspace = true +which = "4.2.5" [target.'cfg(windows)'.dependencies] fwdansi.workspace = true diff --git a/runtime/permissions/mod.rs b/runtime/permissions/mod.rs index a87ca309f..77f44b813 100644 --- a/runtime/permissions/mod.rs +++ b/runtime/permissions/mod.rs @@ -27,6 +27,7 @@ use std::path::PathBuf; use std::str::FromStr; use std::string::ToString; use std::sync::Arc; +use which::which; mod prompter; use prompter::permission_prompt; @@ -261,6 +262,9 @@ pub trait Descriptor: Eq + Clone { fn stronger_than(&self, other: &Self) -> bool { self == other } + fn aliases(&self) -> Vec<Self> { + vec![] + } } #[derive(Clone, Debug, Eq, PartialEq)] @@ -326,33 +330,43 @@ impl<T: Descriptor + Hash> UnaryPermission<T> { desc: &Option<T>, allow_partial: AllowPartial, ) -> PermissionState { - if self.is_flag_denied(desc) || self.is_prompt_denied(desc) { - PermissionState::Denied - } else if self.is_granted(desc) { - match allow_partial { - AllowPartial::TreatAsGranted => PermissionState::Granted, - AllowPartial::TreatAsDenied => { - if self.is_partial_flag_denied(desc) { - PermissionState::Denied - } else { - PermissionState::Granted + let aliases = desc.as_ref().map_or(vec![], T::aliases); + for desc in [desc] + .into_iter() + .chain(&aliases.into_iter().map(Some).collect::<Vec<_>>()) + { + let state = if self.is_flag_denied(desc) || self.is_prompt_denied(desc) { + PermissionState::Denied + } else if self.is_granted(desc) { + match allow_partial { + AllowPartial::TreatAsGranted => PermissionState::Granted, + AllowPartial::TreatAsDenied => { + if self.is_partial_flag_denied(desc) { + PermissionState::Denied + } else { + PermissionState::Granted + } } - } - AllowPartial::TreatAsPartialGranted => { - if self.is_partial_flag_denied(desc) { - PermissionState::GrantedPartial - } else { - PermissionState::Granted + AllowPartial::TreatAsPartialGranted => { + if self.is_partial_flag_denied(desc) { + PermissionState::GrantedPartial + } else { + PermissionState::Granted + } } } + } else if matches!(allow_partial, AllowPartial::TreatAsDenied) + && self.is_partial_flag_denied(desc) + { + PermissionState::Denied + } else { + PermissionState::Prompt + }; + if state != PermissionState::Prompt { + return state; } - } else if matches!(allow_partial, AllowPartial::TreatAsDenied) - && self.is_partial_flag_denied(desc) - { - PermissionState::Denied - } else { - PermissionState::Prompt } + PermissionState::Prompt } fn request_desc( @@ -402,7 +416,12 @@ impl<T: Descriptor + Hash> UnaryPermission<T> { fn revoke_desc(&mut self, desc: &Option<T>) -> PermissionState { match desc.as_ref() { - Some(desc) => self.granted_list.retain(|v| !v.stronger_than(desc)), + Some(desc) => { + self.granted_list.retain(|v| !v.stronger_than(desc)); + for alias in desc.aliases() { + self.granted_list.retain(|v| !v.stronger_than(&alias)); + } + } None => { self.granted_global = false; // Revoke global is a special case where the entire granted list is @@ -469,7 +488,11 @@ impl<T: Descriptor + Hash> UnaryPermission<T> { ) { match desc { Some(desc) => { + let aliases = desc.aliases(); list.insert(desc); + for alias in aliases { + list.insert(alias); + } } None => *list_global = true, } @@ -580,7 +603,11 @@ impl AsRef<str> for EnvDescriptor { #[derive(Clone, Eq, PartialEq, Hash, Debug)] pub enum RunDescriptor { + /// Warning: You may want to construct with `RunDescriptor::from()` for case + /// handling. Name(String), + /// Warning: You may want to construct with `RunDescriptor::from()` for case + /// handling. Path(PathBuf), } @@ -592,19 +619,41 @@ impl Descriptor for RunDescriptor { fn name(&self) -> Cow<str> { Cow::from(self.to_string()) } -} -impl FromStr for RunDescriptor { - type Err = (); + fn aliases(&self) -> Vec<Self> { + match self { + RunDescriptor::Name(name) => match which(name) { + Ok(path) => vec![RunDescriptor::Path(path)], + Err(_) => vec![], + }, + RunDescriptor::Path(_) => vec![], + } + } +} - fn from_str(s: &str) -> Result<Self, Self::Err> { +impl From<String> for RunDescriptor { + fn from(s: String) -> Self { + #[cfg(windows)] + let s = s.to_lowercase(); let is_path = s.contains('/'); #[cfg(windows)] - let is_path = is_path || s.contains('\\') || Path::new(s).is_absolute(); + let is_path = is_path || s.contains('\\') || Path::new(&s).is_absolute(); if is_path { - Ok(Self::Path(resolve_from_cwd(Path::new(s)).unwrap())) + Self::Path(resolve_from_cwd(Path::new(&s)).unwrap()) } else { - Ok(Self::Name(s.to_string())) + Self::Name(s) + } + } +} + +impl From<PathBuf> for RunDescriptor { + fn from(p: PathBuf) -> Self { + #[cfg(windows)] + let p = PathBuf::from(p.to_string_lossy().to_string().to_lowercase()); + if p.is_absolute() { + Self::Path(p) + } else { + Self::Path(resolve_from_cwd(&p).unwrap()) } } } @@ -905,19 +954,19 @@ impl UnaryPermission<SysDescriptor> { impl UnaryPermission<RunDescriptor> { pub fn query(&self, cmd: Option<&str>) -> PermissionState { self.query_desc( - &cmd.map(|c| RunDescriptor::from_str(c).unwrap()), + &cmd.map(|c| RunDescriptor::from(c.to_string())), AllowPartial::TreatAsPartialGranted, ) } pub fn request(&mut self, cmd: Option<&str>) -> PermissionState { - self.request_desc(&cmd.map(|c| RunDescriptor::from_str(c).unwrap()), || { + self.request_desc(&cmd.map(|c| RunDescriptor::from(c.to_string())), || { Some(cmd?.to_string()) }) } pub fn revoke(&mut self, cmd: Option<&str>) -> PermissionState { - self.revoke_desc(&cmd.map(|c| RunDescriptor::from_str(c).unwrap())) + self.revoke_desc(&cmd.map(|c| RunDescriptor::from(c.to_string()))) } pub fn check( @@ -926,7 +975,7 @@ impl UnaryPermission<RunDescriptor> { api_name: Option<&str>, ) -> Result<(), AnyError> { self.check_desc( - &Some(RunDescriptor::from_str(cmd).unwrap()), + &Some(RunDescriptor::from(cmd.to_string())), false, api_name, || Some(format!("\"{}\"", cmd)), @@ -1594,19 +1643,20 @@ fn parse_sys_list( fn parse_run_list( list: &Option<Vec<String>>, ) -> Result<HashSet<RunDescriptor>, AnyError> { + let mut result = HashSet::new(); if let Some(v) = list { - v.iter() - .map(|x| { - if x.is_empty() { - Err(AnyError::msg("Empty path is not allowed")) - } else { - Ok(RunDescriptor::from_str(x).unwrap()) - } - }) - .collect() - } else { - Ok(HashSet::new()) + for s in v { + if s.is_empty() { + return Err(AnyError::msg("Empty path is not allowed")); + } else { + let desc = RunDescriptor::from(s.to_string()); + let aliases = desc.aliases(); + result.insert(desc); + result.extend(aliases); + } + } } + Ok(result) } fn escalation_error() -> AnyError { |