From ba50be30431d8c30e1cb0829cdefb42ae62de99e Mon Sep 17 00:00:00 2001 From: tash-2s <81064017+tash-2s@users.noreply.github.com> Date: Thu, 28 Dec 2023 14:43:47 -0600 Subject: [PATCH] fix: create valid Standard JSON to verify for projects with symlinks (#35) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when a project includes symbolic links to Solidity files, `Project::standard_json_input` returns a Standard JSON Input struct that is unusable for contract verification on block explorers. `solc` cannot compile these contracts based on the json, leading to verification failures. This pull request addresses this issue. I encountered this problem when using the `forge verify-contract` command, which internally uses this function. While `forge build` completes successfully, the verification command fails, indicating that the block explorer cannot find a source file. This problem occurs in projects that contain symlinked Solidity files and when these files are imported from other files. My project, structured as a monorepo, uses external libraries installed via pnpm. These libraries, accessible from the `./node_modules/` directory, are set up with remappings in my Foundry project. However, directories under `./node_modules/` are symlinks pointing to other locations, leading to this issue. ## Reproduction I added a test named `can_create_standard_json_input_with_symlink` to demonstrate the issue within this repository. Also, the error can be reproduced using the `forge verify-contract` command and steps below: Environment: macOS (Apple silicon), forge 0.2.0 (`c312c0d` 2023-12-22T00:20:29.297186000Z) ```console $ mkdir repro && cd repro $ mkdir -p project/src project/node_modules dependency $ cat << EOF > project/src/A.sol // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.23; import "@dependency/B.sol"; contract A is B {} EOF $ cat << EOF > dependency/B.sol // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.23; import "./C.sol"; contract B is C {} EOF $ cat << EOF > dependency/C.sol // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.23; contract C {} EOF $ cat << EOF > project/foundry.toml [profile.default] remappings = ["@dependency/=node_modules/dependency/"] allow_paths = ["../dependency/"] EOF # Create a symbolic link $ cd project/node_modules $ ln -s ../../dependency dependency $ cd ../.. # Display the file structure $ tree . ├── dependency │   ├── B.sol │   └── C.sol └── project ├── foundry.toml ├── node_modules │   └── dependency -> ../../dependency └── src └── A.sol # `build` succeeds $ cd project $ forge build # `verify-contract` generates an unintended json (C.sol has a host absolute path name) $ forge verify-contract --show-standard-json-input 0x0000000000000000000000000000000000000000 A | jq . > A.json $ cat A.json { "language": "Solidity", "sources": { "src/A.sol": { "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\nimport \"@dependency/B.sol\";\ncontract A is B {}\n" }, "node_modules/dependency/B.sol": { "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\nimport \"./C.sol\";\ncontract B is C {}\n" }, "/Users/t/bench/repro/dependency/C.sol": { "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\ncontract C {}\n" } }, "settings": { "remappings": [ "@dependency/=node_modules/dependency/", "dependency/=node_modules/dependency/" ], "optimizer": { "enabled": true, "runs": 200 }, "metadata": { "useLiteralContent": false, "bytecodeHash": "ipfs", "appendCBOR": true }, "outputSelection": { "*": { "": [ "ast" ], "*": [ "abi", "evm.bytecode", "evm.deployedBytecode", "evm.methodIdentifiers", "metadata" ] } }, "evmVersion": "paris", "libraries": {} } } # `solc` cannot compile using the json $ solc --standard-json --no-import-callback A.json | jq .errors [ { "component": "general", "errorCode": "6275", "formattedMessage": "ParserError: Source \"node_modules/dependency/C.sol\" not found: No import callback.\n --> node_modules/dependency/B.sol:3:1:\n |\n3 | import \"./C.sol\";\n | ^^^^^^^^^^^^^^^^^\n\n", "message": "Source \"node_modules/dependency/C.sol\" not found: No import callback.", "severity": "error", "sourceLocation": { "end": 81, "file": "node_modules/dependency/B.sol", "start": 64 }, "type": "ParserError" } ] ``` Manually editing the host filesystem's absolute path as shown below allows the `solc` command to succeed. ```diff "node_modules/dependency/B.sol": { "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\nimport \"./C.sol\";\ncontract B is C {}\n" }, - "/Users/t/bench/repro/dependency/C.sol": { + "node_modules/dependency/C.sol": { "content": "// SPDX-License-Identifier: UNLICENSED\npragma solidity ^0.8.23;\ncontract C {}\n" } }, ``` ## Cause The issue arises because import paths in Solidity files are processed by the [`std::fs::canonicalize`](https://doc.rust-lang.org/std/fs/fn.canonicalize.html) function. This function resolves symlinks and normalizes paths. When symlinks are resolved, it results in `solc` being unable to locate the corresponding source paths in the json, as it relies on Solidity import statements. Therefore, symlinks should not be resolved here. The paths should be maintained as specified in the Solidity files, except for basic normalization. ## Solution To address this, I implemented an import path normalization function and replaced the canonicalization function where necessary. Based on [the Solidity documentation page](https://docs.soliditylang.org/en/v0.8.23/path-resolution.html), this function resolves `.` and `..` segments/components without resolving symlinks. The Standard JSON's source paths, for verification purposes, should be based on the project root. This allows the compiler to find sources within the json. The conversion of normalized paths to project root-based paths occurs after all sources are processed. That conversion is already implemented, so this PR doesn't need to address it. ([It seems like the path conversion needs improvement](https://github.com/foundry-rs/foundry/issues/5307), but it is a separate issue and should be handled in another PR.) https://github.com/foundry-rs/compilers/blob/b1561d807c246c066c7c4b0c72c8eb64c696a43d/src/lib.rs#L514-L525 With the changes proposed in this PR, I confirmed the fix of the issue by building `forge` with this patch and testing both the reproduction case and my project. The resulting json matches the manually edited json diff mentioned earlier. A potential downside of this approach is that the same file could be represented differently in the json if accessed via both symlinked and real paths. However, I see no issues with this, and it aligns with the behavior of the compiler's virtual filesystem. --- src/config.rs | 65 +++++++++++++++++- src/utils.rs | 172 ++++++++++++++++++++++++++++++++++++++++++++++- tests/project.rs | 68 +++++++++++++++++++ 3 files changed, 301 insertions(+), 4 deletions(-) diff --git a/src/config.rs b/src/config.rs index 5177246f..4c11336f 100644 --- a/src/config.rs +++ b/src/config.rs @@ -230,7 +230,7 @@ impl ProjectPathsConfig { if component == Component::CurDir || component == Component::ParentDir { // if the import is relative we assume it's already part of the processed input // file set - utils::canonicalize(cwd.join(import)).map_err(|err| { + utils::normalize_solidity_import_path(cwd, import).map_err(|err| { SolcError::msg(format!("failed to resolve relative import \"{err:?}\"")) }) } else { @@ -253,7 +253,7 @@ impl ProjectPathsConfig { // also try to resolve absolute imports from the project paths for path in [&self.root, &self.sources, &self.tests, &self.scripts] { if cwd.starts_with(path) { - if let Ok(import) = utils::canonicalize(path.join(import)) { + if let Ok(import) = utils::normalize_solidity_import_path(path, import) { return Ok(import); } } @@ -1026,4 +1026,65 @@ mod tests { Path::new("/root/test/") ); } + + #[test] + fn can_resolve_import() { + let dir = tempfile::tempdir().unwrap(); + let config = ProjectPathsConfig::builder().root(dir.path()).build().unwrap(); + config.create_all().unwrap(); + + fs::write(config.sources.join("A.sol"), r"pragma solidity ^0.8.0; contract A {}").unwrap(); + + // relative import + assert_eq!( + config + .resolve_import_and_include_paths( + &config.sources, + Path::new("./A.sol"), + &mut Default::default(), + ) + .unwrap(), + config.sources.join("A.sol") + ); + + // direct import + assert_eq!( + config + .resolve_import_and_include_paths( + &config.sources, + Path::new("src/A.sol"), + &mut Default::default(), + ) + .unwrap(), + config.sources.join("A.sol") + ); + } + + #[test] + fn can_resolve_remapped_import() { + let dir = tempfile::tempdir().unwrap(); + let mut config = ProjectPathsConfig::builder().root(dir.path()).build().unwrap(); + config.create_all().unwrap(); + + let dependency = config.root.join("dependency"); + fs::create_dir(&dependency).unwrap(); + fs::write(dependency.join("A.sol"), r"pragma solidity ^0.8.0; contract A {}").unwrap(); + + config.remappings.push(Remapping { + context: None, + name: "@dependency/".into(), + path: "dependency/".into(), + }); + + assert_eq!( + config + .resolve_import_and_include_paths( + &config.sources, + Path::new("@dependency/A.sol"), + &mut Default::default(), + ) + .unwrap(), + dependency.join("A.sol") + ); + } } diff --git a/src/utils.rs b/src/utils.rs index 5878e06d..7d15a49c 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -177,6 +177,81 @@ pub fn canonicalize(path: impl AsRef) -> Result { res.map_err(|err| SolcIoError::new(err, path)) } +/// Returns a normalized Solidity file path for the given import path based on the specified +/// directory. +/// +/// This function resolves `./` and `../`, but, unlike [`canonicalize`], it does not resolve +/// symbolic links. +/// +/// The function returns an error if the normalized path does not exist in the file system. +/// +/// See also: +pub fn normalize_solidity_import_path( + directory: impl AsRef, + import_path: impl AsRef, +) -> Result { + let original = directory.as_ref().join(import_path); + let cleaned = clean_solidity_path(&original); + + // this is to align the behavior with `canonicalize` + use path_slash::PathExt; + let normalized = PathBuf::from(dunce::simplified(&cleaned).to_slash_lossy().as_ref()); + + // checks if the path exists without reading its content and obtains an io error if it doesn't. + normalized.metadata().map(|_| normalized).map_err(|err| SolcIoError::new(err, original)) +} + +// This function lexically cleans the given path. +// +// It performs the following transformations for the path: +// +// * Resolves references (current directories (`.`) and parent (`..`) directories). +// * Reduces repeated separators to a single separator (e.g., from `//` to `/`). +// +// This transformation is lexical, not involving the file system, which means it does not account +// for symlinks. This approach has a caveat. For example, consider a filesystem-accessible path +// `a/b/../c.sol` passed to this function. It returns `a/c.sol`. However, if `b` is a symlink, +// `a/c.sol` might not be accessible in the filesystem in some environments. Despite this, it's +// unlikely that this will pose a problem for our intended use. +// +// # How it works +// +// The function splits the given path into components, where each component roughly corresponds to a +// string between separators. It then iterates over these components (starting from the leftmost +// part of the path) to reconstruct the path. The following steps are applied to each component: +// +// * If the component is a current directory, it's removed. +// * If the component is a parent directory, the following rules are applied: +// * If the preceding component is a normal, then both the preceding normal component and the +// parent directory component are removed. (Examples of normal components include `a` and `b` +// in `a/b`.) +// * Otherwise (if there is no preceding component, or if the preceding component is a parent, +// root, or prefix), it remains untouched. +// * Otherwise, the component remains untouched. +// +// Finally, the processed components are reassembled into a path. +fn clean_solidity_path(original_path: impl AsRef) -> PathBuf { + let mut new_path = Vec::new(); + + for component in original_path.as_ref().components() { + match component { + Component::Prefix(..) | Component::RootDir | Component::Normal(..) => { + new_path.push(component); + } + Component::CurDir => {} + Component::ParentDir => { + if let Some(Component::Normal(..)) = new_path.last() { + new_path.pop(); + } else { + new_path.push(component); + } + } + } + } + + new_path.iter().collect() +} + /// Returns the same path config but with canonicalized paths. /// /// This will take care of potential symbolic linked directories. @@ -228,7 +303,7 @@ pub fn resolve_library(libs: &[impl AsRef], source: impl AsRef) -> O /// until the `root` is reached. /// /// If an existing file under `root` is found, this returns the path up to the `import` path and the -/// canonicalized `import` path itself: +/// normalized `import` path itself: /// /// For example for following layout: /// @@ -247,7 +322,7 @@ pub fn resolve_absolute_library( ) -> Option<(PathBuf, PathBuf)> { let mut parent = cwd.parent()?; while parent != root { - if let Ok(import) = canonicalize(parent.join(import)) { + if let Ok(import) = normalize_solidity_import_path(parent, import) { return Some((parent.to_path_buf(), import)); } parent = parent.parent()?; @@ -654,6 +729,99 @@ pragma solidity ^0.8.0; assert_eq!(Some("^0.8.0"), find_version_pragma(s).map(|s| s.as_str())); } + #[test] + fn can_normalize_solidity_import_path() { + let dir = tempfile::tempdir().unwrap(); + let dir_path = dir.path(); + + // File structure: + // + // `dir_path` + // └── src (`cwd`) + // ├── Token.sol + // └── common + // └── Burnable.sol + + fs::create_dir_all(dir_path.join("src/common")).unwrap(); + fs::write(dir_path.join("src/Token.sol"), "").unwrap(); + fs::write(dir_path.join("src/common/Burnable.sol"), "").unwrap(); + + // assume that the import path is specified in Token.sol + let cwd = dir_path.join("src"); + + assert_eq!( + normalize_solidity_import_path(&cwd, "./common/Burnable.sol").unwrap(), + dir_path.join("src/common/Burnable.sol"), + ); + + assert!(normalize_solidity_import_path(&cwd, "./common/Pausable.sol").is_err()); + } + + // This test is exclusive to unix because creating a symlink is a privileged action on Windows. + // https://doc.rust-lang.org/std/os/windows/fs/fn.symlink_dir.html#limitations + #[test] + #[cfg(unix)] + fn can_normalize_solidity_import_path_symlink() { + let dir = tempfile::tempdir().unwrap(); + let dir_path = dir.path(); + + // File structure: + // + // `dir_path` + // ├── dependency + // │   └── Math.sol + // └── project + // ├── node_modules + // │   └── dependency -> symlink to actual 'dependency' directory + // └── src (`cwd`) + // └── Token.sol + + fs::create_dir_all(dir_path.join("project/src")).unwrap(); + fs::write(dir_path.join("project/src/Token.sol"), "").unwrap(); + fs::create_dir(dir_path.join("project/node_modules")).unwrap(); + + fs::create_dir(dir_path.join("dependency")).unwrap(); + fs::write(dir_path.join("dependency/Math.sol"), "").unwrap(); + + std::os::unix::fs::symlink( + dir_path.join("dependency"), + dir_path.join("project/node_modules/dependency"), + ) + .unwrap(); + + // assume that the import path is specified in Token.sol + let cwd = dir_path.join("project/src"); + + assert_eq!( + normalize_solidity_import_path(cwd, "../node_modules/dependency/Math.sol").unwrap(), + dir_path.join("project/node_modules/dependency/Math.sol"), + ); + } + + #[test] + fn can_clean_solidity_path() { + assert_eq!(clean_solidity_path("a"), PathBuf::from("a")); + assert_eq!(clean_solidity_path("./a"), PathBuf::from("a")); + assert_eq!(clean_solidity_path("../a"), PathBuf::from("../a")); + assert_eq!(clean_solidity_path("/a/"), PathBuf::from("/a")); + assert_eq!(clean_solidity_path("//a"), PathBuf::from("/a")); + assert_eq!(clean_solidity_path("a/b"), PathBuf::from("a/b")); + assert_eq!(clean_solidity_path("a//b"), PathBuf::from("a/b")); + assert_eq!(clean_solidity_path("/a/b"), PathBuf::from("/a/b")); + assert_eq!(clean_solidity_path("a/./b"), PathBuf::from("a/b")); + assert_eq!(clean_solidity_path("a/././b"), PathBuf::from("a/b")); + assert_eq!(clean_solidity_path("/a/../b"), PathBuf::from("/b")); + assert_eq!(clean_solidity_path("a/./../b/."), PathBuf::from("b")); + assert_eq!(clean_solidity_path("a/b/c"), PathBuf::from("a/b/c")); + assert_eq!(clean_solidity_path("a/b/../c"), PathBuf::from("a/c")); + assert_eq!(clean_solidity_path("a/b/../../c"), PathBuf::from("c")); + assert_eq!(clean_solidity_path("a/b/../../../c"), PathBuf::from("../c")); + assert_eq!( + clean_solidity_path("a/../b/../../c/./Token.sol"), + PathBuf::from("../c/Token.sol") + ); + } + #[test] fn can_find_ancestor() { let a = Path::new("/foo/bar/bar/test.txt"); diff --git a/tests/project.rs b/tests/project.rs index 5dda8442..84950940 100644 --- a/tests/project.rs +++ b/tests/project.rs @@ -1684,6 +1684,74 @@ fn can_compile_std_json_input() { } } +// This test is exclusive to unix because creating a symlink is a privileged action on windows. +// https://doc.rust-lang.org/std/os/windows/fs/fn.symlink_dir.html#limitations +#[test] +#[cfg(unix)] +fn can_create_standard_json_input_with_symlink() { + let mut project = TempProject::dapptools().unwrap(); + let dependency = TempProject::dapptools().unwrap(); + + // File structure: + // + // project + // ├── node_modules + // │   └── dependency -> symlink to actual 'dependency' directory + // └── src + // └── A.sol + // + // dependency + // └── src + // ├── B.sol + // └── C.sol + + fs::create_dir(project.root().join("node_modules")).unwrap(); + + std::os::unix::fs::symlink(dependency.root(), project.root().join("node_modules/dependency")) + .unwrap(); + project.project_mut().paths.remappings.push(Remapping { + context: None, + name: "@dependency/".into(), + path: "node_modules/dependency/".into(), + }); + + project + .add_source( + "A", + r"pragma solidity >=0.8.0; import '@dependency/src/B.sol'; contract A is B {}", + ) + .unwrap(); + dependency + .add_source("B", r"pragma solidity >=0.8.0; import './C.sol'; contract B is C {}") + .unwrap(); + dependency.add_source("C", r"pragma solidity >=0.8.0; contract C {}").unwrap(); + + // solc compiles using the host file system; therefore, this setup is considered valid + project.assert_no_errors(); + + // can create project root based paths + let std_json = + project.project().standard_json_input(project.sources_path().join("A.sol")).unwrap(); + assert_eq!( + std_json.sources.iter().map(|(path, _)| path.clone()).collect::>(), + vec![ + PathBuf::from("src/A.sol"), + PathBuf::from("node_modules/dependency/src/B.sol"), + PathBuf::from("node_modules/dependency/src/C.sol") + ] + ); + + // can compile using the created json + let compiler_errors = Solc::default() + .compile(&std_json) + .unwrap() + .errors + .into_iter() + .filter_map(|e| if e.severity.is_error() { Some(e.message) } else { None }) + .collect::>(); + assert!(compiler_errors.is_empty(), "{:?}", compiler_errors); +} + #[test] fn can_compile_model_checker_sample() { let root = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test-data/model-checker-sample");