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

feat(permission): support suffix wildcards in --allow-env flag #25255

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
085b111
Add wildcard support to --allow-env flag for environment variable pat…
yazan-abdalrahman Aug 28, 2024
5e463d3
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Aug 28, 2024
1dae2e4
fix, test
yazan-abdalrahman Aug 28, 2024
849e194
Merge remote-tracking branch 'origin/Enhance---allow-env-to-Support-P…
yazan-abdalrahman Aug 28, 2024
574f292
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 3, 2024
8ae53a6
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 4, 2024
7d0b807
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 4, 2024
32af9f7
New solution with support env.get and set
yazan-abdalrahman Sep 4, 2024
ac39fc9
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 5, 2024
8603674
fmt
yazan-abdalrahman Sep 5, 2024
981a9ce
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 5, 2024
a7469b9
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 5, 2024
7a93a1a
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 5, 2024
8df48c3
fix
yazan-abdalrahman Sep 5, 2024
7ed8376
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 5, 2024
4c31cdd
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 5, 2024
b5c8bb8
fix new solution
yazan-abdalrahman Sep 5, 2024
24e2ac5
Merge remote-tracking branch 'origin/Enhance---allow-env-to-Support-P…
yazan-abdalrahman Sep 5, 2024
3076ca9
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 8, 2024
e49e0ef
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 10, 2024
6a368f9
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 16, 2024
afff2c8
Merge branch 'refs/heads/main' into Enhance---allow-env-to-Support-Pr…
yazan-abdalrahman Sep 18, 2024
22b568f
fmt
yazan-abdalrahman Sep 18, 2024
6153314
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Sep 24, 2024
9ef3509
Merge branch 'refs/heads/main' into Enhance---allow-env-to-Support-Pr…
yazan-abdalrahman Oct 9, 2024
c828c7c
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Oct 17, 2024
525cdbc
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Oct 21, 2024
63b3e22
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Oct 30, 2024
37c49be
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
yazan-abdalrahman Nov 12, 2024
372266b
wip
bartlomieju Nov 17, 2024
600d042
Merge branch 'env_wildcard' into Enhance---allow-env-to-Support-Prefi…
bartlomieju Nov 17, 2024
4b2b357
cleanup
bartlomieju Nov 17, 2024
365525e
test for wildcards in workers
bartlomieju Nov 20, 2024
6d12ba3
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
bartlomieju Nov 20, 2024
522a4f3
move definition beside impl
dsherret Nov 20, 2024
78d9617
allow doing a subset of a prefix when creating child perms
dsherret Nov 20, 2024
0bc4cd7
Merge branch 'main' into Enhance---allow-env-to-Support-Prefix,-Suffi…
bartlomieju Nov 20, 2024
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
23 changes: 16 additions & 7 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.

use super::flags_net;
use crate::args::resolve_no_prompt;
use crate::util::fs::canonicalize_path;
use std::collections::HashSet;
use std::env;
use std::ffi::OsString;
Expand Down Expand Up @@ -36,15 +38,11 @@ use deno_runtime::colors;
use deno_runtime::deno_permissions::parse_sys_kind;
use deno_runtime::deno_permissions::PermissionsOptions;
use log::debug;
use log::error;
use log::Level;
use serde::Deserialize;
use serde::Serialize;

use crate::args::resolve_no_prompt;
use crate::util::fs::canonicalize_path;

use super::flags_net;

#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub enum ConfigFlag {
#[default]
Expand Down Expand Up @@ -4956,7 +4954,18 @@ fn permission_args_parse(flags: &mut Flags, matches: &mut ArgMatches) {
}

if let Some(env_wl) = matches.remove_many::<String>("allow-env") {
flags.permissions.allow_env = Some(env_wl.collect());
let env_permissions: Vec<String> = env_wl.collect();
for env_var in &env_permissions {
if env_var.contains('*') {
Copy link
Member

Choose a reason for hiding this comment

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

--allow-env=* makes no sense because it's equal to --allow-env. If we are to support "wildcards" in env var names it should only be at the begging or end (AWS_*, *_DENO)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I will remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--allow-env=* makes no sense because it's equal to --allow-env. If we are to support "wildcards" in env var names it should only be at the begging or end (AWS_*, *_DENO)

done

if let Err(e) =
deno_runtime::deno_permissions::add_wildcard_permission(env_var)
{
error!("Failed to add wildcard permission: {}", e);
}
}
}

flags.permissions.allow_env = Some(env_permissions);
debug!("env allowlist: {:#?}", &flags.permissions.allow_env);
}

Expand Down
10 changes: 8 additions & 2 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -395,8 +395,14 @@ pub fn main() {
};

match create_and_run_current_thread_with_maybe_metrics(future) {
Ok(exit_code) => std::process::exit(exit_code),
Err(err) => exit_for_error(err),
Ok(exit_code) => {
let _ = deno_runtime::deno_permissions::clear_wildcard_permissions();
std::process::exit(exit_code)
}
Err(err) => {
let _ = deno_runtime::deno_permissions::clear_wildcard_permissions();
exit_for_error(err)
}
}
}

Expand Down
55 changes: 55 additions & 0 deletions runtime/permissions/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use deno_core::url::Url;
use deno_core::ModuleSpecifier;
use deno_terminal::colors;
use fqdn::FQDN;
use log::error;
use once_cell::sync::Lazy;
use std::borrow::Cow;
use std::collections::HashSet;
Expand All @@ -32,6 +33,7 @@ use std::path::PathBuf;
use std::str::FromStr;
use std::string::ToString;
use std::sync::Arc;
use std::sync::RwLock;

pub mod prompter;
use prompter::permission_prompt;
Expand Down Expand Up @@ -181,6 +183,29 @@ impl PermissionState {
.map(|info| { format!(" to {info}") })
.unwrap_or_default(),
);

if name == "env" {
let env_var_name = info()
.map(|info| {
let info_str = info.trim_start_matches('"').trim_end_matches('"');
info_str.to_string()
})
.unwrap_or_default();
for env_var in crate::get_wildcard_permissions() {
if let Some(suffix) = env_var.strip_prefix('*') {
if env_var_name.ends_with(suffix) {
return (Ok(()), true, false);
}
} else if let Some(prefix) = env_var.strip_suffix('*') {
if env_var_name.starts_with(prefix) {
return (Ok(()), true, false);
}
} else if env_var.contains('*') {
return (Ok(()), true, false);
}
}
}

match permission_prompt(&msg, name, api_name, true) {
PromptResponse::Allow => {
Self::log_perm_access(name, info);
Expand Down Expand Up @@ -2317,6 +2342,36 @@ pub fn is_standalone() -> bool {
IS_STANDALONE.is_raised()
}

static WILDCARD_PERMISSIONS: Lazy<RwLock<Vec<String>>> =
Lazy::new(|| RwLock::new(Vec::new()));

pub fn add_wildcard_permission(permission: &str) -> Result<(), String> {
let mut permissions = WILDCARD_PERMISSIONS.write().map_err(|e| {
error!("Failed to acquire write lock: {}", e);
"Failed to add wildcard permission".to_string()
})?;
if !permissions.contains(&permission.to_string()) {
permissions.push(permission.to_string());
}
Ok(())
}

pub fn get_wildcard_permissions() -> Vec<String> {
let permissions = WILDCARD_PERMISSIONS
.read()
.expect("Failed to acquire read lock");
permissions.clone()
}

pub fn clear_wildcard_permissions() -> Result<(), String> {
let mut permissions = WILDCARD_PERMISSIONS.write().map_err(|e| {
error!("Failed to acquire write lock: {}", e);
"Failed to clear wildcard permissions".to_string()
})?;
permissions.clear();
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the purpose of this code? What's the reason for having a static with a lock just to match permissions?

Copy link
Contributor Author

@yazan-abdalrahman yazan-abdalrahman Sep 5, 2024

Choose a reason for hiding this comment

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

@bartlomieju

The code uses a RwLock to handle wildcard permissions dynamically. When a wildcard is passed (e.g., --allow-env=MYAPP_*), the logic captures and stores these patterns. Before checking an environment variable's permission, the stored patterns are matched to grant access. The lock ensures thread-safe reads/writes while allowing concurrent reads. This dynamic approach helps manage permissions for env variables set by the user (via Deno.env.set), even if they weren’t present at the script’s start. It addresses the issue where wildcard permissions don’t automatically allow newly set env variables like FOOBAR_X

Copy link
Member

Choose a reason for hiding this comment

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

That is not correct. You want to solve the same way --allow-net permissions are stored - you store an enum of either a concrete value (--allow-env=NODE_DEBUG) or a wildcard match (--allow-env=AWS_*, --allow-env=*_DENO). Then when the permission is checked you either perform an exact match on "concrete value" enum variant, or a wildcard match on the other variant. The RwLock is not needed in here.

So you need to update EnvDescriptor to something like:

#[derive(Clone, Eq, PartialEq, Hash, Debug)]
pub enum EnvDescriptor {
  Concrete(EnvVarName),
  Wildcard(...)
}

and then impl UnaryPermission<EnvDescriptor> to apply that logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bartlomieju ,
its cant be Enum maybe have many suffixes like this AWS_,FOOBAR_ , many prefix *_DENO, *_EXALT,
how theses random different name can be stores in Enum

Copy link
Member

Choose a reason for hiding this comment

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

You store one element in a single enum - then multiple enums are stored here:

pub struct UnaryPermission<T: Descriptor + Hash> {
granted_global: bool,
granted_list: HashSet<T>,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You store one element in a single enum - then multiple enums are stored here:

pub struct UnaryPermission<T: Descriptor + Hash> {
granted_global: bool,
granted_list: HashSet<T>,

@bartlomieju
done check latest commit


#[cfg(test)]
mod tests {
use super::*;
Expand Down
34 changes: 34 additions & 0 deletions tests/specs/permission/process_env_permissions/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"tempDir": true,
"tests": {
"deno_env_wildcard_tests": {
"envs": {
"MYAPP_HELLO": "Hello\tworld,",
"MYAPP_GOODBYE": "farewell",
"OTHER_VAR": "ignore"
},
"steps": [
{
"args": "run --allow-env=MYAPP_* main.js",
"output": "Hello\tworld,\nfarewell\ndone\n"
},
{
"args": "run --allow-env=*_HELLO,*_GOODBYE,*_DONE,*_TEST main.js",
"output": "Hello\tworld,\nfarewell\ndone\n"
},
{
"args": "run --allow-env=* main.js",
"output": "Hello\tworld,\nfarewell\ndone\n"
},
{
"args": "run --allow-env main.js",
"output": "Hello\tworld,\nfarewell\ndone\n"
},
{
"args": "run --allow-env=MYAPP_HELLO,MYAPP_GOODBYE,MYAPP_TEST,MYAPP_DONE main.js",
"output": "Hello\tworld,\nfarewell\ndone\n"
}
]
}
}
}
5 changes: 5 additions & 0 deletions tests/specs/permission/process_env_permissions/main.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
console.log(Deno.env.get("MYAPP_HELLO"));
console.log(Deno.env.get("MYAPP_GOODBYE"));
Deno.env.set("MYAPP_TEST", "done");
Deno.env.set("MYAPP_DONE", "done");
console.log(Deno.env.get("MYAPP_DONE"));