fix: look through all matching permissions in is_path_allowed
#10679
Conversation
rubydusa
requested review from
DaniPopes,
klkvr,
mattsse,
grandizzy,
yash-atreya and
zerosnacks
as code owners
| pub fn find_all_permissions(&self, path: &Path) -> Vec<FsAccessPermission> { | ||
| let mut matching_permissions = Vec::new(); | ||
| let mut max_path_len = 0; | ||
|
|
||
| // First pass: find all matching permissions and determine the maximum path length | ||
| for perm in &self.permissions { | ||
| let permission_path = dunce::canonicalize(&perm.path).unwrap_or(perm.path.clone()); | ||
| if path.starts_with(&permission_path) { | ||
| let path_len = permission_path.components().count(); | ||
| if path_len > max_path_len { | ||
| max_path_len = path_len; | ||
| matching_permissions.clear(); | ||
| matching_permissions.push(perm.access); | ||
| } else if path_len == max_path_len { | ||
| matching_permissions.push(perm.access); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| matching_permissions |
There was a problem hiding this comment.
we now have both find_permission and find_all_permissions
I think we only want to keep one impl for this to ensure we always use this behaviour.
the internal check is slightly different from what we have in find_permission, why do we need to account for length here
There was a problem hiding this comment.
find_permission also accounts for length:
// the longest path takes precedence
if perm.path < active_perm.path {
continue;
}It's needed because you could have something like this:
fs_permissions = [
{ access = "read-write", path = "./artifacts" },
{ access = "read", path = "./artifacts/sensitive" }
]you shouldn't be able to write to ./artifacts/sensitive/data.json
There was a problem hiding this comment.
I think we only want to keep one impl for this to ensure we always use this behaviour.
Maybe we can refactor both functions to use a third function for shared logic? I just didn't want to touch find_permission since I didn't know all the places where it's used
There was a problem hiding this comment.
Maybe we can refactor both functions to use a third function for shared logic? I just didn't want to touch find_permission since I didn't know all the places where it's used
@rubydusa I think this makes sense, we could also rename current find_permission to find_path_permission for clarity
Motivation
Closes #10677
Solution
Solution proposed in #10677 (comment)
(iterate through all matching longest paths)
PR Checklist