From b20a779f7b65c18afdb9d0378e56c7fdf838b888 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 30 Sep 2021 15:50:59 -0400 Subject: [PATCH] fix: worker environment permissions should accept an array (#12250) --- .../testdata/workers/env_read_check_worker.js | 14 +++ cli/tests/unit/worker_permissions_test.ts | 43 ++++++++ cli/tests/unit/worker_types.ts | 2 +- runtime/js/11_workers.js | 3 +- runtime/ops/worker_host.rs | 11 +- runtime/permissions.rs | 104 ++++++++++-------- 6 files changed, 123 insertions(+), 54 deletions(-) create mode 100644 cli/tests/testdata/workers/env_read_check_worker.js create mode 100644 cli/tests/unit/worker_permissions_test.ts diff --git a/cli/tests/testdata/workers/env_read_check_worker.js b/cli/tests/testdata/workers/env_read_check_worker.js new file mode 100644 index 0000000000..72ad31df22 --- /dev/null +++ b/cli/tests/testdata/workers/env_read_check_worker.js @@ -0,0 +1,14 @@ +onmessage = async ({ data }) => { + const permissions = []; + for (const name of data.names) { + const { state } = await Deno.permissions.query({ + name: "env", + variable: name, + }); + permissions.push(state === "granted"); + } + + postMessage({ + permissions, + }); +}; diff --git a/cli/tests/unit/worker_permissions_test.ts b/cli/tests/unit/worker_permissions_test.ts new file mode 100644 index 0000000000..35cb2d04d5 --- /dev/null +++ b/cli/tests/unit/worker_permissions_test.ts @@ -0,0 +1,43 @@ +// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. +import { assertEquals, deferred, unitTest } from "./test_util.ts"; + +unitTest( + { permissions: { env: true, read: true } }, + async function workerEnvArrayPermissions() { + const promise = deferred(); + + const worker = new Worker( + new URL( + "../testdata/workers/env_read_check_worker.js", + import.meta.url, + ).href, + { + type: "module", + deno: { + namespace: true, + permissions: { + env: ["test", "OTHER"], + }, + }, + }, + ); + + worker.onmessage = ({ data }) => { + promise.resolve(data.permissions); + }; + + worker.postMessage({ + names: ["test", "TEST", "asdf", "OTHER"], + }); + + const permissions = await promise; + worker.terminate(); + + if (Deno.build.os === "windows") { + // windows ignores case + assertEquals(permissions, [true, true, false, true]); + } else { + assertEquals(permissions, [true, false, false, true]); + } + }, +); diff --git a/cli/tests/unit/worker_types.ts b/cli/tests/unit/worker_types.ts index 0fd2c32485..0f99e15df5 100644 --- a/cli/tests/unit/worker_types.ts +++ b/cli/tests/unit/worker_types.ts @@ -5,7 +5,7 @@ unitTest( { permissions: { read: true } }, function utimeSyncFileSuccess() { const w = new Worker( - new URL("../workers/worker_types.ts", import.meta.url).href, + new URL("../testdata/workers/worker_types.ts", import.meta.url).href, { type: "module" }, ); assert(w); diff --git a/runtime/js/11_workers.js b/runtime/js/11_workers.js index d7d153098a..2b908b9f84 100644 --- a/runtime/js/11_workers.js +++ b/runtime/js/11_workers.js @@ -58,6 +58,7 @@ } /** + * @param {"inherit" | boolean} value * @param {string} permission * @return {boolean} */ @@ -127,7 +128,7 @@ write = "inherit", }) { return { - env: parseUnitPermission(env, "env"), + env: parseArrayPermission(env, "env"), hrtime: parseUnitPermission(hrtime, "hrtime"), net: parseArrayPermission(net, "net"), ffi: parseUnitPermission(ffi, "ffi"), diff --git a/runtime/ops/worker_host.rs b/runtime/ops/worker_host.rs index f749e495c0..c1ce782da3 100644 --- a/runtime/ops/worker_host.rs +++ b/runtime/ops/worker_host.rs @@ -223,7 +223,10 @@ fn merge_env_permission( ) -> Result, AnyError> { if let Some(worker) = worker { if (worker.global_state < main.global_state) - || !worker.granted_list.iter().all(|x| main.check(&x.0).is_ok()) + || !worker + .granted_list + .iter() + .all(|x| main.check(x.as_ref()).is_ok()) { return Err(custom_error( "PermissionDenied", @@ -448,11 +451,7 @@ where Ok(Some(UnaryPermission:: { global_state: value.global_state, - granted_list: value - .paths - .into_iter() - .map(|env| EnvDescriptor(env.to_uppercase())) - .collect(), + granted_list: value.paths.into_iter().map(EnvDescriptor::new).collect(), ..Default::default() })) } diff --git a/runtime/permissions.rs b/runtime/permissions.rs index 0776b3a41a..081268aa9a 100644 --- a/runtime/permissions.rs +++ b/runtime/permissions.rs @@ -159,26 +159,48 @@ impl UnitPermission { } } -#[derive(Clone, Debug, Default, Deserialize, PartialEq)] +/// A normalized environment variable name. On Windows this will +/// be uppercase and on other platforms it will stay as-is. +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] +struct EnvVarName { + inner: String, +} + +impl EnvVarName { + pub fn new(env: impl AsRef) -> Self { + Self { + inner: if cfg!(windows) { + env.as_ref().to_uppercase() + } else { + env.as_ref().to_string() + }, + } + } +} + +impl AsRef for EnvVarName { + fn as_ref(&self) -> &str { + self.inner.as_str() + } +} + +#[derive(Clone, Debug, Default, PartialEq)] pub struct UnaryPermission { - #[serde(skip)] pub name: &'static str, - #[serde(skip)] pub description: &'static str, pub global_state: PermissionState, pub granted_list: HashSet, pub denied_list: HashSet, - #[serde(skip)] pub prompt: bool, } -#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, Deserialize)] +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] pub struct ReadDescriptor(pub PathBuf); -#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, Deserialize)] +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] pub struct WriteDescriptor(pub PathBuf); -#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, Deserialize)] +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] pub struct NetDescriptor(pub String, pub Option); impl NetDescriptor { @@ -203,13 +225,25 @@ impl fmt::Display for NetDescriptor { } } -#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, Deserialize)] -pub struct EnvDescriptor(pub String); +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] +pub struct EnvDescriptor(EnvVarName); -#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, Deserialize)] +impl EnvDescriptor { + pub fn new(env: impl AsRef) -> Self { + Self(EnvVarName::new(env)) + } +} + +impl AsRef for EnvDescriptor { + fn as_ref(&self) -> &str { + self.0.as_ref() + } +} + +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] pub struct RunDescriptor(pub String); -#[derive(Clone, Eq, PartialEq, Hash, Debug, Default, Deserialize)] +#[derive(Clone, Eq, PartialEq, Hash, Debug, Default)] pub struct FfiDescriptor(pub String); impl UnaryPermission { @@ -585,21 +619,18 @@ impl UnaryPermission { impl UnaryPermission { pub fn query(&self, env: Option<&str>) -> PermissionState { - #[cfg(windows)] - let env = env.map(|env| env.to_uppercase()); - #[cfg(windows)] - let env = env.as_deref(); + let env = env.map(EnvVarName::new); if self.global_state == PermissionState::Denied - && match env { + && match env.as_ref() { None => true, - Some(env) => self.denied_list.iter().any(|env_| env_.0 == env), + Some(env) => self.denied_list.iter().any(|env_| &env_.0 == env), } { PermissionState::Denied } else if self.global_state == PermissionState::Granted - || match env { + || match env.as_ref() { None => false, - Some(env) => self.granted_list.iter().any(|env_| env_.0 == env), + Some(env) => self.granted_list.iter().any(|env_| &env_.0 == env), } { PermissionState::Granted @@ -610,23 +641,18 @@ impl UnaryPermission { pub fn request(&mut self, env: Option<&str>) -> PermissionState { if let Some(env) = env { - let env = if cfg!(windows) { - env.to_uppercase() - } else { - env.to_string() - }; - let state = self.query(Some(&env)); + let state = self.query(Some(env)); if state == PermissionState::Prompt { if permission_prompt(&format!("env access to \"{}\"", env)) { - self.granted_list.insert(EnvDescriptor(env)); + self.granted_list.insert(EnvDescriptor::new(env)); PermissionState::Granted } else { - self.denied_list.insert(EnvDescriptor(env)); + self.denied_list.insert(EnvDescriptor::new(env)); self.global_state = PermissionState::Denied; PermissionState::Denied } } else if state == PermissionState::Granted { - self.granted_list.insert(EnvDescriptor(env)); + self.granted_list.insert(EnvDescriptor::new(env)); PermissionState::Granted } else { state @@ -650,9 +676,7 @@ impl UnaryPermission { pub fn revoke(&mut self, env: Option<&str>) -> PermissionState { if let Some(env) = env { - #[cfg(windows)] - let env = env.to_uppercase(); - self.granted_list.remove(&EnvDescriptor(env.to_string())); + self.granted_list.remove(&EnvDescriptor::new(env)); } else { self.granted_list.clear(); } @@ -663,8 +687,6 @@ impl UnaryPermission { } pub fn check(&mut self, env: &str) -> Result<(), AnyError> { - #[cfg(windows)] - let env = &env.to_uppercase(); let (result, prompted) = self.query(Some(env)).check( self.name, Some(&format!("\"{}\"", env)), @@ -672,9 +694,9 @@ impl UnaryPermission { ); if prompted { if result.is_ok() { - self.granted_list.insert(EnvDescriptor(env.to_string())); + self.granted_list.insert(EnvDescriptor::new(env)); } else { - self.denied_list.insert(EnvDescriptor(env.to_string())); + self.denied_list.insert(EnvDescriptor::new(env)); self.global_state = PermissionState::Denied; } } @@ -976,17 +998,7 @@ impl Permissions { global_state: global_state_from_option(state), granted_list: state .as_ref() - .map(|v| { - v.iter() - .map(|x| { - EnvDescriptor(if cfg!(windows) { - x.to_uppercase() - } else { - x.clone() - }) - }) - .collect() - }) + .map(|v| v.iter().map(EnvDescriptor::new).collect()) .unwrap_or_else(HashSet::new), denied_list: Default::default(), prompt,