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

Prevent test failures for invalid SCP-style URLs in go 1.12 #205

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 6, 2019

Fixes #203

This (invalid) format was introduced in f9ec369 but it was introduced in such a way that getter always treated it as invalid anyway (as mentioned in the commit message).

Detector would keep it as is, which I assume was just attempt to retain some backward compatibility, rather than intention? @apparentlymart

That said (because go-getter is used as a library) if anyone already compiled this under Go 1.12.8+ then it's already "broken" for them anyway and I so I feel there's very little value in attempting to retain backward compatibility for <1.12.8 when we already broke it for many people and depending on the way folks use this library it may (probably?) even pose a risk - see https://nvd.nist.gov/vuln/detail/CVE-2019-14809 for more details about the motivation behind the patch in Go.

@radeksimko radeksimko force-pushed the fix-go112-failures branch 6 times, most recently from 8fa6eab to b903dc0 Compare September 6, 2019 15:24
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

My memory here is a bit hazy since I remember having to try a bunch of different permutations to get something that worked as I wanted, but my original goal was to report that specialized error message that includes the prompt to remove the ssh: prefix if you are trying to be scp-like because we'd seen a bunch of questions from users who had tried to use scp-like URLs with ssh:// in front and were previously getting very strange error messages due to go-getter just not understanding what they meant at all.

I think I'd made the detector just ignore the invalid form so that I could centralize the error handling in the getter, but the new more restrictive URL parsing in net/url defeats that by making the detector parse fail.

In an ideal world I'd like to have the detector notice that it's specifically got this "invalid port number" error from net/url and return a variant of our custom error message, but I'm not sure that url.Error includes enough detail to distinguish that from other errors, in which case I guess we'll have to live with the more generic error message.

I'm approving this because it does seem like it should solve the problem at hand. I hope there's also a way to preserve the helpful error message, but I don't intend to block merging this on that account.

@radeksimko
Copy link
Member Author

I hope there's also a way to preserve the helpful error message

I assume there is, but it's ... non trivial.

The trickiest part is that we used to "catch" this invalid URL under GitGetter, but the overall logic there assumes positive outcome of url.Parse(), or rather a url.URL from the detector.

I like the idea of having a more helpful error message, but I'm personally ok with worse message if the implementation/maintenance cost is too high or if it's too fragile.

@radeksimko radeksimko merged commit 7009632 into master Sep 9, 2019
@radeksimko radeksimko deleted the fix-go112-failures branch September 9, 2019 11:40
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.

The scp-like URL test fails after Go 1.12.8.
3 participants