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

Allows to use files outside of the main directory in setup.cfg file #2701

Closed
wants to merge 3 commits into from
Closed

Allows to use files outside of the main directory in setup.cfg file #2701

wants to merge 3 commits into from

Conversation

airvzxf
Copy link

@airvzxf airvzxf commented Jun 18, 2021

Summary of changes

This change allows to use the file outside the main directory. For example, in the file setup.cfg, section long_description the user can use an outside file file: ../README.md.

Closes #2699

Pull Request Checklist

@airvzxf airvzxf changed the title Removed the sand box when the file is parsed. Allows to use files outside of the main directory in setup.cfg file Jun 18, 2021
@airvzxf airvzxf marked this pull request as ready for review June 18, 2021 04:42
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I'm pretty sure that it's not a bug but a designed behavior. Even the test name says "sandbox". Referencing files outside the project is prone to errors and security bugs. I think, as a workaround, you could symlink it. Also, how do you include it in an sdist if it's outside?

@airvzxf
Copy link
Author

airvzxf commented Jun 23, 2021

All the information and my arguments are in the Issue #2699, but I'll try to resume in this comment.

Even the test name says "sandbox". Referencing files outside the project is prone to errors and security bugs.

In my point of view, say that take file outside the project is a security issue is relative. Because if I have the permissions to access to a root file via setuptools, means I can copy manually this file in the project directory. What is the preventing this security code changes? Is the person who build the application the only one who can modify anything before upload to the PyPi?

  • If you think that my PR is not relevant, and you didn't merge my code. At least add in these pieces of code a warning message saying: For security reasons, you could't use file outside of this directory: /home/my-user/projects/hulala/src/

Also, how do you include it in a sdist if it's outside?

This is the interesting part, if you review my code changes and description in the issue. It isn't copying the file into the project directory, it is copy the content and put in some place, in this case in the PKG-INFO file. Means I am able to have a README.md file in /root/hack-me/README.md and put this content in my project /home/my-user/projects/hulala/src/hulala.egg/PKG-INFO. Do I expose that is essential in this PR?

I think, as a workaround, you could symlink it.

As I mentioned in the created issue. I am able to fix using the setup.py without problems with open('../README.md', 'r', encoding="utf-8") as fh:, also create a symlink looks ugly and not make sense to have a file. Let me put an example: Imagine that my documentation in ./hulala/src/README.md is referring to the application, this documentation not necessary needs to have the documentation for the PyPi. In fact, it shouldn't have the PyPi documentation. Let say that this documentation is in ./hulala/docs/pypi/README.md. Then, when I create the package, I want to specify in the file setup.cfg:

  • long_description = file: ../docs/pypi/README.md
  • long_description = file: /home/my-user/hulala/docs/pypi/README.md

Do you get my point?

Copy link
Member

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

Also, how do you include it in a sdist if it's outside?

This is the interesting part, if you review my code changes and description in the issue. It isn't copying the file into the project directory, it is copy the content and put in some place, in this case in the PKG-INFO file.

I think you may have missed the point. If your project loads its long_description from ../README, then you build an sdist and ship that off to PyPI, and then I download the sdist and try to build it, it will fail to build because ../README doesn't exist.

What's worse, though, is if you use long_description = file:/etc/passwd, and then you build an sdist and ship that off to your colleague who builds a wheel on their build server and installs that to their applications, now importlib.metadata.metadata('your-package')['Description'] will have the contents of /etc/passwd for the build server.

I'm sorry, but it's not about security, but it's about having hermetic packages whose sources are transportable.

I think you may wish to update the error message for clarity, but I recommend to do that in a separate PR.

Comment on lines -311 to -312
Directive is sandboxed and won't reach anything outside
directory with setup.py.
Copy link
Member

Choose a reason for hiding this comment

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

This change removes the intentional constraint to disallow loading from outside the project directory. This change will affect any config option that uses _parse_file. If we wish to change the behavior here to be more lenient, we should at least enumerate the directives that might use this behavior. Is it only long_description or are there others?

@jaraco jaraco closed this Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Allow paths outiside of the source code for long_description
3 participants