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

git: ensure file-looking git refs aren't parsed as URLs #4776

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Mar 18, 2024

Fixes #4326 (comment) (thanks @stephanos! 🎉)

URLs that look like ./path/to/file and ../path/to/file definitely aren't git URLs - so we should bail out early.

This was causing a weird issue where if you copied ./.git this would be detected as a valid url parsing with host = "." and path = "/git". The fix for this is to make sure that for these explicit file-like paths, we never parse them as url-refs.

Also some tests to make sure this doesn't break again!

URLs that look like `./path/to/file` and `../path/to/file` definitely
aren't git URLs - so we should bail out early.

This was causing a weird issue where if you copied `./.git` this would
be detected as a valid url parsing with `host = "."` and `path = "/git"`.
The fix for this is to make sure that for these explicit file-like
paths, we *never* parse them as url-refs.

Also some tests to make sure this doesn't break again!

Signed-off-by: Justin Chadwell <[email protected]>
@jedevc jedevc requested a review from tonistiigi March 18, 2024 14:28
@jedevc
Copy link
Member Author

jedevc commented Mar 18, 2024

I don't think this is a regression (from what I can tell), but potentially this is also a good backport candidate?

@thaJeztah
Copy link
Member

ISTR I once added a test-case for the classic builder to make sure that it was able to build from a local directory named github.com/foo/bar - I think that case had to check if a directory with the name existed, and otherwise had to fall back to "consider it a github URL", but not 100% sure on that part. Should we have a test for that as well? (Does BuildKit still have special cases for github.com/xxxxxxx ?

@tonistiigi tonistiigi added this to the v0.13.1 milestone Mar 18, 2024
@jedevc
Copy link
Member Author

jedevc commented Mar 18, 2024

Yup, we still have those github.com/... special cases - those test cases are also in the git_ref_test.go file.

@tonistiigi tonistiigi merged commit eb41916 into moby:master Mar 18, 2024
72 checks passed
@tonistiigi
Copy link
Member

@jedevc Looks like this was regression after all? #4777

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.

3 participants