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

]add $PACKAGE_NAME#remotes/cache/pull/$NUM/head works; a feature or a security hole? #789

Closed
tkf opened this issue Oct 2, 2018 · 6 comments

Comments

@tkf
Copy link
Member

tkf commented Oct 2, 2018

I noticed that ]add $PACKAGE_NAME#remotes/cache/pull/$NUM/head installs a not-yet-merged PR from GitHub where $PACKAGE_NAME is a name of a registered package and $NUM is the PR ID. I used it sometimes when recommending people to use a PR. Although it's handy for letting users try things out in the PR, it gives a wrong impression that this installs code from the official repository $PACKAGE_NAME. But the truth is that it can be any code since anyone can send a PR. I think in the end users have to know what the command they use does, but I wonder if Pkg.jl wants to avoid random git ref to be specified.

@staticfloat
Copy link
Member

I think it's in general impossible to know what a git ref "means". The fact that a branch named remotes/cache/pull/<num>/head exists within the remote repository is just an implementation detail of GitHub; on GitLab this may not work or it may be available as pull_requests/by_issue_num/<num>, etc... I don't think Pkg can really do anything about this, because it is also completely valid for you as a user to have a git repository with a branch name that actually is remotes/cache/pull/99999/head. As an example, I just did that right here.

So it's impossible to know if this is "unsafe" code or an actually desirable branch name from Pkg's perspective, and even if we did try to "block" things matching this pattern, GitHub may change their internal representation eventually.

@tkf
Copy link
Member Author

tkf commented Oct 5, 2018

I was thinking to prepend refs/remotes/origin/ to the branch supplied by the user. Assuming that libgit2 has git-rev-parse-like behavior, wouldn't this (more or less) ensure that it is the branch name of the origin? But I just noticed that ]add claims to support specifying the commit hash. So Pkg would have to fallback to treat the given #name as the commit hash to support it.

@staticfloat
Copy link
Member

I just tried to reproduce this and I couldn't. How are you seeing this behavior?

@tkf
Copy link
Member Author

tkf commented Oct 6, 2018

Here is a quick oneliner to see it:

HOME="$(mktemp -d --suffix "-julia")" julia -i --color=yes --startup-file=no -e 'using Pkg; pkg"add PyCall#remotes/cache/pull/556/head"'

It seems that it works outside my laptop: JuliaPy/PyCall.jl#565 (comment)

@KristofferC
Copy link
Member

I would say this is a feature.

@StefanKarpinski
Copy link
Member

There's no such thing as client-side security.

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

No branches or pull requests

4 participants