feat(permission): support suffix wildcards in --allow-env flag
#25255
Conversation
yazan-abdalrahman
changed the title
…x,-and-Wildcard-Matching
|
Could @lucacasonato @dsherret @bartlomieju Pls review the change? |
…refix,-Suffix,-and-Wildcard-Matching' into Enhance---allow-env-to-Support-Prefix,-Suffix,-and-Wildcard-Matching
@lucacasonato This edge case requires separate tickets. Maybe we need to verify permission before prompt, and if we have a wildcard, we need to change the permission_list or u can use |
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
|
@lucacasonato |
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
runtime/permissions/lib.rs
Outdated
| 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(()) | ||
| } |
There was a problem hiding this comment.
Can you explain the purpose of this code? What's the reason for having a static with a lock just to match permissions?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
You store one element in a single enum - then multiple enums are stored here:
deno/runtime/permissions/lib.rs
Lines 338 to 340 in b01578a
There was a problem hiding this comment.
You store one element in a single enum - then multiple enums are stored here:
deno/runtime/permissions/lib.rs
Lines 338 to 340 in b01578a
@bartlomieju
done check latest commit
cli/args/flags.rs
Outdated
| for env_var in &env_permissions { | ||
| if env_var.contains('*') { |
There was a problem hiding this comment.
--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)
There was a problem hiding this comment.
OK I will remove it
There was a problem hiding this comment.
--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
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
…refix,-Suffix,-and-Wildcard-Matching' into Enhance---allow-env-to-Support-Prefix,-Suffix,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
|
@lucacasonato @bartlomieju |
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
…efix,-Suffix,-and-Wildcard-Matching # Conflicts: # runtime/permissions/lib.rs
|
@dsherret |
…x,-and-Wildcard-Matching
…efix,-Suffix,-and-Wildcard-Matching # Conflicts: # cli/args/flags.rs
|
Please review this change. |
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
|
Please review this change. |
…x,-and-Wildcard-Matching
…x,-and-Wildcard-Matching
|
@dsherret Please review this change. |
There was a problem hiding this comment.
What happens when someone queries for if permisisons are allowed via the Deno.permissions.query API? It seems like this wouldn't handle that?
I feel like probably we should introduce an EnvQueryDescriptor on EnvDescriptor's AllowDesc that could be an explicit name or pattern, then we could compare EnvDescriptor to it.
…x,-Suffix,-and-Wildcard-Matching
bartlomieju
changed the title
--allow-env flag
…x,-and-Wildcard-Matching
runtime/permissions/lib.rs
Outdated
| match other { | ||
| Self::AllowDesc::Name(n) => match &self.0 { | ||
| EnvQueryDescriptorInner::Name(env_var_name) => n == env_var_name, | ||
| EnvQueryDescriptorInner::PrefixPattern(env_var_name) => { | ||
| env_var_name.as_ref().starts_with(n.as_ref()) | ||
| } | ||
| }, | ||
| Self::AllowDesc::PrefixPattern(p) => match &self.0 { | ||
| EnvQueryDescriptorInner::Name(env_var_name) => { | ||
| env_var_name.as_ref().starts_with(p.as_ref()) | ||
| } | ||
| EnvQueryDescriptorInner::PrefixPattern(env_var_name) => { | ||
| p == env_var_name | ||
| } | ||
| }, | ||
| } |
There was a problem hiding this comment.
I updated it (see the new test)
…x,-and-Wildcard-Matching
#24847