solidity icon indicating copy to clipboard operation
solidity copied to clipboard

Disallow absolute import paths (path spec v3)

Open cameel opened this issue 3 years ago • 1 comments

Closes #9346. Depends on #11409. Part of a set of issues that replaces #11138.

Abstract

Disallow absolute paths in the virtual filesystem to make it harder to create non-portable metadata containg system-dependent paths.

Motivation

Absolute paths are in most cases different on every machine (especially when they include the path to user's home directory) and using them results in non-reproducible bytecode (because they end up in metadata and metadata hash is included in the bytecode). This leads to verification tools giving up and just stripping metadata completely. Even if such paths are not directly used in imports, frameworks often use them in Standard JSON input or remappings. While it's entirely possible to structure a project in such a way that it uses only relative paths, it's not always convenient or even clear how to do it when using external libraries.

#11409 adds a mechanism that allows conveniently importing files from libraries stored in arbitrary locations using relative paths. Once it's implemented we could deprecate and later completely disallow absolute paths in the VFS without taking away any useful functionality.

Specification

  1. In the VFS disallow any source unit names that start with /. This means that they would be also disallowed in import statements and on the urls list in source dict in Standard JSON.
  2. Disallow using empty path for --base-path.
    • An empty path is currently the default value. Make the path to the current working directory the default.
  3. On the command line only allow specifying paths to files that are located inside --base-path or --include-paths.
    • The paths on the command line can of course still be both relative and absolute because due to #11408 and #11409 they will always be normalized and made relative before they end up in the VFS.
    • Also adjust solcjs to behave in the same way.
  4. (Optional) In Standard JSON provide a new key called path inside sources, in addition to the currently available content and urls.
    • This would be a way to preserve the ability to load files from absolute paths, which some users find useful (#9346). These are not included in metadata so allowing absolute paths here would not be harmful.
    • The value is not passed to the file loader callback. Instead the file is loaded directly from the specified path.
    • Path can be absolute or relative to the current working directory. It's not affected by --base-path.
    • Path must be within --allowed-paths.
    • The feature is only available when using the native compiler which has access to the underlying filesystem.
    • It cannot be used together with with content or urls.

NOTE: --allowed-directories becomes almost redundant after these changes. There are now only two cases where it's needed:

  • When a file is a symlink that leads to a file outside of base path or include directories. The directory containing the symlink target must be in --allowed-directories for this to be allowed.
  • The optional sources.path key mentioned above.

Backwards Compatibility

The change is not backwards-compatible. It will require adjusting code or compiler options in several use cases:

  • Contracts importing directly from absolute paths.
  • Absolute paths used for source unit names in Standard JSON.
  • Import remappings with absolute path as a target.
  • Absolute paths in sources.urls in Standard JSON (they will now work as if they were relative).

All of these can easily be solved by using --base-path and --include-paths.

Path resolution in frameworks

This change will require changes in popular frameworks. Of the four I investigated, two (Truffle and Brownie) are using absolute paths in some cases and will require adjustments. The other two (Hardhat and dapp.tools) should be unaffected.

Truffle

Truffle has a resolver package which is responsible for finding a library on disk based on the path used in the import and the path of the importing source unit. The whole system is pretty modular and can find files in npm's node_modules/ (global and local), EthPM registry or even files auto-generated from ABI JSON using abi-to-sol. Located source is inserted into Standard JSON input as content and it does not rely on the file loader callback. In case of npm packages the source unit name used is a relative path.

It allows importing from arbitrary locations in the filesystem too, in which case the path ends up being absolute.

One thing that will no longer be possible in Truffle will be imports like import "../node_modules/@openzeppelin/contracts/utils.sol" in a file located in contracts/ that exists at the same level as node_modules/. I've seen some people using them as workarounds for other problems with imports (https://github.com/trufflesuite/truffle/issues/593#issuecomment-635143731). It works if the source unit name of the importing file in the virtual filesystem is an absolute path because then going one level above node_modules/ in the directory tree is allowed. It the source unit name was just the name of the file, the import would break.

The use of absolute paths has been already recognized as a problem in https://github.com/trufflesuite/truffle/issues/1621.

Hardhat

Hardhat, just like Truffle, uses Standard JSON and does not rely on a file loader callback but its path resolution is much simpler. Its localPathToSourceName() only has special cases for relative imports (starting with ../) and for node_modules/.

It does not allow using absolute paths in imports at all (reports error HH407).

Brownie

Brownie uses Standard JSON but only includes project files as content in it. For loading libraries it relies on the default file loader. It has its own package manager, which by default installs libraries in ~/.brownie/packages/. For example Open Zeppelin 3.0.0 is installed in ~/.brownie/packages/OpenZeppelin/[email protected]. By default it adds remappings of the form OpenZeppelin=/home/user/.brownie/packages/OpenZeppelin so that files can be imported with import "OpenZeppelin/[email protected]/contracts/math/SafeMath.sol". It also encourages users to use additional remappings to make imports of the form import "@openzeppelin/contracts/math/SafeMath.sol" work as well.

dapp.tools

dapp.tools, like Brownie, uses Standard JSON and relies on the default file loader and import remappings. Unlike Brownie, it requires a rigid project structure where all project and library files are stored (or symlinked) inside the same root directory (project files under src/, libraries under lib/) so absolute paths are not necessary and not used.

cameel avatar May 19 '21 02:05 cameel

We talked about this on today's call but did not make a decision yet. It's at least clear that we do not want this to be a sudden change that catches anyone by surprise. If we want to get it through, we need to reach out to frameworks first and help them get rid of absolute paths from metadata. Only then it would be feasible to phase out paths starting with /.

Reporting warnings instead of errors when absolute paths are used is also not necessarily something we want to do.

Our primary motivation here is actually reducing confusion among users about what absolute paths used in imports mean.

cameel avatar May 27 '21 15:05 cameel

This issue has been marked as stale due to inactivity for the last 90 days. It will be automatically closed in 7 days.

github-actions[bot] avatar Mar 18 '23 12:03 github-actions[bot]

Hi everyone! This issue has been automatically closed due to inactivity. If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen. However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

github-actions[bot] avatar Mar 26 '23 12:03 github-actions[bot]