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

#6981: fs_permissions find permission also sym links #7022

Merged
merged 4 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ alloy-primitives = { workspace = true, features = ["serde"] }
revm-primitives = { workspace = true, default-features = false, features = ["std"] }

dirs-next = "2"
dunce = "1"
eyre.workspace = true
figment = { version = "0.10", features = ["toml", "env"] }
globset = "0.4"
Expand Down
6 changes: 4 additions & 2 deletions crates/config/src/fs_permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ impl FsPermissions {

/// Returns the permission for the matching path.
///
/// This finds the longest matching path, e.g. if we have the following permissions:
/// This finds the longest matching path with resolved sym links, e.g. if we have the following
/// permissions:
///
/// `./out` = `read`
/// `./out/contracts` = `read-write`
Expand All @@ -53,7 +54,8 @@ impl FsPermissions {
pub fn find_permission(&self, path: &Path) -> Option<FsAccessPermission> {
let mut permission: Option<&PathPermission> = None;
for perm in &self.permissions {
if path.starts_with(&perm.path) {
let permission_path = dunce::canonicalize(&perm.path).unwrap_or(perm.path.clone());
if path.starts_with(permission_path) {
Comment on lines +57 to +58
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually not sure why this is the right fix, because the path should already be canonicalized here

/// Returns true if the given path is allowed, if any path `allowed_paths` is an ancestor of the
/// path
///
/// We only allow paths that are inside allowed paths. To prevent path traversal
/// ("../../etc/passwd") we canonicalize/normalize the path first. We always join with the
/// configured root directory.
pub fn is_path_allowed(&self, path: impl AsRef<Path>, kind: FsAccessKind) -> bool {
self.is_normalized_path_allowed(&self.normalized_path(path), kind)
}

Copy link
Collaborator Author

@grandizzy grandizzy Feb 8, 2024

Choose a reason for hiding this comment

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

I'm actually not sure why this is the right fix, because the path should already be canonicalized here

/// Returns true if the given path is allowed, if any path `allowed_paths` is an ancestor of the
/// path
///
/// We only allow paths that are inside allowed paths. To prevent path traversal
/// ("../../etc/passwd") we canonicalize/normalize the path first. We always join with the
/// configured root directory.
pub fn is_path_allowed(&self, path: impl AsRef<Path>, kind: FsAccessKind) -> bool {
self.is_normalized_path_allowed(&self.normalized_path(path), kind)
}

that's the normalized path to the file desired to be loaded in test (by using vm.readFile), mind that is passed to is_normalized_path_allowed function which checks it against configured fs_permissions path (that are not normalized)

pub fn is_path_allowed(&self, path: impl AsRef<Path>, kind: FsAccessKind) -> bool {
self.is_normalized_path_allowed(&self.normalized_path(path), kind)
}
fn is_normalized_path_allowed(&self, path: &Path, kind: FsAccessKind) -> bool {
self.fs_permissions.is_path_allowed(path, kind)
}

Copy link
Member

Choose a reason for hiding this comment

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

ah now I see, it, we're canonicalizing the permission path, got it ty!

if let Some(active_perm) = permission.as_ref() {
// the longest path takes precedence
if perm.path < active_perm.path {
Expand Down
81 changes: 79 additions & 2 deletions crates/forge/tests/cli/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use foundry_cli::utils as forge_utils;
use foundry_compilers::artifacts::{OptimizerDetails, RevertStrings, YulDetails};
use foundry_config::{
cache::{CachedChains, CachedEndpoints, StorageCachingConfig},
Config, FuzzConfig, InvariantConfig, SolcReq,
fs_permissions::{FsAccessPermission, PathPermission},
Config, FsPermissions, FuzzConfig, InvariantConfig, SolcReq,
};
use foundry_evm::opts::EvmOpts;
use foundry_test_utils::{
Expand All @@ -14,7 +15,11 @@ use foundry_test_utils::{
};
use path_slash::PathBufExt;
use pretty_assertions::assert_eq;
use std::{fs, path::PathBuf, str::FromStr};
use std::{
fs,
path::{Path, PathBuf},
str::FromStr,
};

// tests all config values that are in use
forgetest!(can_extract_config_values, |prj, cmd| {
Expand Down Expand Up @@ -625,3 +630,75 @@ forgetest_init!(can_skip_remappings_auto_detection, |prj, cmd| {
assert_eq!(config.remappings.len(), 1);
assert_eq!("remapping/=lib/remapping/", config.remappings[0].to_string());
});

forgetest_init!(can_parse_default_fs_permissions, |_prj, cmd| {
let config = cmd.config();

assert_eq!(config.fs_permissions.len(), 1);
let out_permission = config.fs_permissions.find_permission(Path::new("out")).unwrap();
assert_eq!(FsAccessPermission::Read, out_permission);
});

forgetest_init!(can_parse_custom_fs_permissions, |prj, cmd| {
// explicitly set fs permissions
let custom_permissions = FsPermissions::new(vec![
PathPermission::read("./read"),
PathPermission::write("./write"),
PathPermission::read_write("./write/contracts"),
]);

let config = Config { fs_permissions: custom_permissions, ..Default::default() };
prj.write_config(config);

let config = cmd.config();

assert_eq!(config.fs_permissions.len(), 3);

// check read permission
let permission = config.fs_permissions.find_permission(Path::new("./read")).unwrap();
assert_eq!(permission, FsAccessPermission::Read);
// check nested write permission
let permission =
config.fs_permissions.find_permission(Path::new("./write/MyContract.sol")).unwrap();
assert_eq!(permission, FsAccessPermission::Write);
// check nested read-write permission
let permission = config
.fs_permissions
.find_permission(Path::new("./write/contracts/MyContract.sol"))
.unwrap();
assert_eq!(permission, FsAccessPermission::ReadWrite);
// check no permission
let permission =
config.fs_permissions.find_permission(Path::new("./bogus")).unwrap_or_default();
assert_eq!(permission, FsAccessPermission::None);
});

#[cfg(not(target_os = "windows"))]
forgetest_init!(can_resolve_symlink_fs_permissions, |prj, cmd| {
// write config in packages/files/config.json
let config_path = prj.root().join("packages").join("files");
fs::create_dir_all(&config_path).unwrap();
fs::write(config_path.join("config.json"), "{ enabled: true }").unwrap();

// symlink packages/files dir as links/
std::os::unix::fs::symlink(
Path::new("./packages/../packages/../packages/files"),
prj.root().join("links"),
)
.unwrap();

// write config, give read access to links/ symlink to packages/files/
let permissions =
FsPermissions::new(vec![PathPermission::read(Path::new("./links/config.json"))]);
let config = Config { fs_permissions: permissions, ..Default::default() };
prj.write_config(config);

let config = cmd.config();
let mut fs_permissions = config.fs_permissions;
fs_permissions.join_all(prj.root());
assert_eq!(fs_permissions.len(), 1);

// read permission to file should be granted through symlink
let permission = fs_permissions.find_permission(&config_path.join("config.json")).unwrap();
assert_eq!(permission, FsAccessPermission::Read);
});
Loading