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

Do not whitelist remapping targets #12014

Closed
cameel opened this issue Sep 23, 2021 · 6 comments
Closed

Do not whitelist remapping targets #12014

cameel opened this issue Sep 23, 2021 · 6 comments

Comments

@cameel
Copy link
Member

cameel commented Sep 23, 2021

Originally requested in #11688 (comment).

Abstract

Normally, importing files from arbitrary directories requires whitelisting those directories using --allow-paths. That's not the case when the import contains a remapping. This automatic whitelisting should be removed.

Example

This causes an error:

mkdir /tmp/{dir,anotherdir}
touch /tmp/anotherdir/contract.sol
cd /tmp/dir/
echo "import '/tmp/anotherdir/contract.sol';" | solc -
Error: Source "/tmp/anotherdir/contract.sol" not found: File outside of allowed directories.
 --> <stdin>:1:1:
  |
1 | import '/tmp/anotherdir/contract.sol';
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

While this does not:

echo "import '/tmp/dir/contract.sol';" | solc - /tmp/dir/=/tmp/anotherdir/

This is especially weird when the remapping does not really do anything:

echo "import '/tmp/anotherdir/contract.sol';" | solc - /tmp/anotherdir/=/tmp/anotherdir/

Specification

Remapping prefixes, targets and contexts should not be added to allowed paths.

Backwards Compatibility

The change is not backwards-compatible. Users relying on this behavior will have to start adding --allowed-paths to their commands.

@soroosh-sdi
Copy link
Contributor

I like to try this issue, if it's not urgent

@soroosh-sdi
Copy link
Contributor

soroosh-sdi commented Sep 24, 2021

I think we also need a new test case; I don't know you add tests for this kind of issues or not. a downside comes to my mind is that adding tests for such small issues could make running our tests so time consuming after couple of years.
EDIT: we have lots of tests in CommandLineParser.cpp which tests this feature implicitly.

@cameel
Copy link
Member Author

cameel commented Sep 24, 2021

It's not urgent but this is not the best moment to start working on this because #11688 and #12007 have not been merged yet and they change a lot regarding --allow-paths. In particular #11688 adds a complete suite of tests so testing this should no longer be a problem.

It's also a breaking change so it should be based on breaking rather than develop branch and will only be released in 0.9.0, some time in the future.

@soroosh-sdi
Copy link
Contributor

please move this issue to Done, after merging #12021.

@cameel
Copy link
Member Author

cameel commented Oct 15, 2021

Sure. Move to "review in progress" now. It will get moved to "done" automatically when the issue is closed.

Merging needs to wait for #12150 unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants