#6981: fs_permissions find permission also sym links

[grandizzy]

Motivation

see #6981

Solution

• resolve configured fs_permissions symlinks paths when finding permissions

Tests

• could write a test for unix family that creates sym link in temp dir and check permissions, didn't find this approach in project, pls advise

[onbjerg]

I'd like to see some x-platform tests on this because Path::canonicalize also transforms paths into the abysmal Windows extended path format which has historically messed up a lot of path-related things in Foundry. For example, the directory separator becomes a backslash instead of a forward slash, which might mean this actually breaks on Windows.

You can see some examples of integration tests that set up foundry.toml and projects in https://github.com/foundry-rs/foundry/tree/master/crates/forge/tests/cli

[mattsse]

I'd like to see an additional non-windows test for this

[grandizzy]

I'd like to see an additional non-windows test for this

added here 250bbc5#diff-289d6a2d36ab0e59edc4f4baeeaa5026ecae1002e13a5fc80e033e46d612bc7bR676

[grandizzy]

I'd like to see some x-platform tests on this because Path::canonicalize also transforms paths into the abysmal Windows extended path format which has historically messed up a lot of path-related things in Foundry. For example, the directory separator becomes a backslash instead of a forward slash, which might mean this actually breaks on Windows.

You can see some examples of integration tests that set up foundry.toml and projects in https://github.com/foundry-rs/foundry/tree/master/crates/forge/tests/cli

thanks for hint, I added couple of them (some mimics the unit tests) with 250bbc5, using dunce::canonicalize as suggested should address further concerns

[onbjerg]

lgtm, pending @mattsse

[mattsse]

can you elaborate on the fix, unclear why we need to do this

we also want an actual solidity test example, see repro in the issue

[mattsse]

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

foundry/crates/cheatcodes/src/config.rs

Lines 72 to 80 in 5ef0d6f

	/// 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)
	}

[grandizzy]

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

foundry/crates/cheatcodes/src/config.rs

Lines 72 to 80 in 5ef0d6f

	/// 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)

foundry/crates/cheatcodes/src/config.rs

Lines 78 to 84 in 5ef0d6f

	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)
	}

[mattsse]

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

[grandizzy]

can you elaborate on the fix, unclear why we need to do this

sure, so using repo in #6981 with following structure (mind that the config.json file to be read in foundry test is outside foundry repo)

foundry-symlink-issue-example/
├── package.json
├── packages
│   ├── contracts
│   │   ├── foundry.toml
│   │   ├── node_modules
│   │   │   └── @repo
│   │   │       └── imported -> ../../../imported
│   │   ├── package.json
│   │   ├── remappings.txt
│   │   ├── test
│   │   │   └── ImportTest.t.sol
│   │   └── yarn.lock
│   └── imported
│       ├── files
│       │   └── config.json
│       └── package.json
├── README.md
└── yarn.lock

with configured permission

fs_permissions = [
  { access = "read", path = "./node_modules/@repo/imported/files/config.json" },
]

when trying to read the file as

        string memory jsonString = vm.readFile(
            "node_modules/@repo/imported/files/config.json"
        );

the "node_modules/@repo/imported/files/config.json" path is joined with root dir and normalized in is_path_allowed hence transformed to "~/foundry-symlink-issue-example/packages/imported/files/config.json". Then this path is checked for permissions (by checking if it starts with any configured permission paths, but this one not canonicalized)

foundry/crates/config/src/fs_permissions.rs

Lines 45 to 56 in 5ef0d6f

	/// Returns the permission for the matching path.
	///
	/// This finds the longest matching path, e.g. if we have the following permissions:
	///
	/// `./out` = `read`
	/// `./out/contracts` = `read-write`
	///
	/// And we check for `./out/contracts/MyContract.sol` we will get `read-write` as permission.
	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) {

so comparing if ~/foundry-symlink-issue-example/packages/imported/files/config.json starts with
~/foundry-symlink-issue-example/packages/contracts/./node_modules/@repo/imported/files/config.json which is false

Proposed fix is to normalize the configured path = "./node_modules/@repo/imported/files/config.json" to "~/foundry-symlink-issue-example/packages/imported/files/config.json"

we also want an actual solidity test example, see repro in the issue

I made the tests against repo provided in #6981 (which I mimicked in can_resolve_symlink_fs_permissions test) , can add solidity in tests too

[mattsse]

thanks for explaining, now I got it

lgtm

[mattsse]

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