Skip to content

fix: look through all matching permissions in is_path_allowed #10679

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: look through all matching permissions in is_path_allowed #10679

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d4b883
fix: use `find_all_permissions` in `is_path_allowed`
rubydusa May 31, 2025
96055e2
chore: `rustfmt`
rubydusa May 31, 2025
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
50 changes: 49 additions & 1 deletion crates/config/src/fs_permissions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ impl FsPermissions {
/// Caution: This should be called with normalized paths if the `allowed_paths` are also
/// normalized.
pub fn is_path_allowed(&self, path: &Path, kind: FsAccessKind) -> bool {
self.find_permission(path).map(|perm| perm.is_granted(kind)).unwrap_or_default()
self.find_all_permissions(path).iter().any(|perm| perm.is_granted(kind))
}

/// Returns the permission for the matching path.
Expand Down Expand Up @@ -66,6 +66,39 @@ impl FsPermissions {
permission.map(|perm| perm.access)
}

/// Returns all permissions for the matching path.
///
/// This finds the longest matching paths with resolved sym links, e.g. if we have the following
/// permissions:
///
/// `./out` = `read`
/// `./out/contracts` = `read`
/// `./out/contracts` = `write`
///
/// And we check for `./out/contracts/MyContract.sol`, we will get both `read` and
/// `write` permissions.
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
Comment on lines +80 to +99
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

find_permission also accounts for length:

// the longest path takes precedence
if perm.path < active_perm.path {
    continue;
}

https://github.com/foundry-rs/foundry/pull/10679/files/3d4b883ffbe6b57093d594b7a636295cac49af6e#diff-7a174b097c5cb093b0f560f9cb870fa36e01b702f3fa1cb5496868b791bf5f44R58-R61

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

What do you agree on? not touching find_permission, or creating a third function for shared logic?
I tried refactoring the functions to share logic while trying to make sure find_permissions implementation changes as little as possible to ensure it's logically consistent - but I couldn't do it any nice way that does not complicate things further

I would consider again simply having find_all_permissions be used onces for this case, and if in the future it's apparent there are more places where this functionality is needed - then it should be refactored

}

/// Updates all `allowed_paths` and joins ([`Path::join`]) the `root` with all entries
pub fn join_all(&mut self, root: &Path) {
self.permissions.iter_mut().for_each(|perm| {
Expand Down Expand Up @@ -270,4 +303,19 @@ mod tests {
let permission = permissions.find_permission(Path::new("./out/MyContract.sol")).unwrap();
assert_eq!(FsAccessPermission::Write, permission);
}

#[test]
fn find_all_permissions() {
let permissions = FsPermissions::new(vec![
PathPermission::read("./out"),
PathPermission::read("./out/contracts"),
PathPermission::write("./out/contracts"),
]);

let found_permissions =
permissions.find_all_permissions(Path::new("./out/contracts/MyContract.sol"));
assert_eq!(found_permissions.len(), 2);
assert!(found_permissions.contains(&FsAccessPermission::Write));
assert!(found_permissions.contains(&FsAccessPermission::Read));
}
}
Loading