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

Don't allow custom build script path to escape package root #12286

Conversation

yerke
Copy link
Contributor

@yerke yerke commented Jun 18, 2023

What does this PR try to resolve?

Don't allow custom build script path to escape package root.
Specifically the PR:

  • shows an error for absolute paths in build field of Cargo.toml, since it will affect reproducibility.
  • checks if the relative path in build field of Cargo.toml escapes the package root and if so, shows an error.

Fixes 11383

How should we test and review this PR?

This PR is still in the exploration stage and I will add tests after we settle on the general approach.

Additional information

This PR is still in the exploration stage and I want to get feedback from the team on the general approach.

@rustbot
Copy link
Collaborator

rustbot commented Jun 18, 2023

r? @epage

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 18, 2023
@yerke
Copy link
Contributor Author

yerke commented Jun 18, 2023

@weihanglo I tried to follow your suggestions, but it didn't work. Specifically if the build script is outside of package directory, then build_ar_list doesn't even know about it. So I couldn't add the check there.

Eventually I found where build field parsing happens, and decided to add the check there.

Please let me know if this is an acceptable approach or not. I realize that there could be breakages for existing users who use build = "../path_to_build_script/build.rs" pattern and don't do cargo package or cargo publish. I am not sure how prevalent that pattern is.

EDIT: Failure of jobserver::makes_jobserver_used test is expected, since it uses build = "../dbuild.rs" pattern.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Yup your concern is totally valid. It seems to prevent from having any build script outside the package root even for a local build. From my understanding we want the check only when packaging .crate file.

Specifically if the build script is outside of package directory, then build_ar_list doesn't even know about it. So I couldn't add the check there.

Have you tried this? You should be able to access build scripts and other targets from Target::src_path().

pkg.manifest().targets().filter(|t| t.src_path() /* and some logic to check */)

@weihanglo
Copy link
Member

Ping @yerke. Just checking in to see if you are still interested in working on this, or if you had any questions.

r? @weihanglo
@rustbot author

@rustbot rustbot assigned weihanglo and unassigned epage Aug 10, 2023
@rustbot rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 10, 2023
@bors
Copy link
Contributor

bors commented Oct 4, 2023

☔ The latest upstream changes (presumably #12768) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
Copy link
Member

Close this in favor of another new active PR #12995

As always, thanks for the contribution, and looking forward to you coming back :)

@weihanglo weihanglo closed this Nov 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-manifest Area: Cargo.toml issues S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cargo package doesn't include build script if this has a custom path
5 participants