-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
Simpler fix for credentials with characters such as '/'. #58
Comments
Agreed with @andygrunwald, nice catch! I'm all for simplifying the code and would support a PR to fix this too. Last I remember there are some specific tests that will need to be updated or removed to reflect the change you're making. You probably should also consider adding some tests specifically to make sure that http basic auth works properly with your change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Thanks for describing the problem in great detail in PR #39 @opalmer.
I suspect there may be a much simpler fix, which I'd like to discuss. Consider this comment:
go-gerrit/gerrit.go
Lines 101 to 104 in 70bbb05
If I'm not mistaken, that URL gets parsed correctly by
url.Parse
. It's just that the URL itself is not correctly escaped, so it doesn't produce the results you want.Let's use
url.URL.String
method to construct a URL with "http" schema, "admin" username, "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg" password, and "localhost:38607" host:(See on playground: https://play.golang.org/p/M3cq7xWI2eE.)
Note that the
/
character in the password gets escaped to%2F
.When we parse that URL with
url.Parse
, it produces the expected results:(See on playground: https://play.golang.org/p/01GMpYMMzsw.)
Notably, the original "ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg" password is preserved.
So, I believe as long as the URL is correctly escaped, the logic added in #39 isn't needed and can be reverted. That would simplify the code. What do you think @opalmer?
The text was updated successfully, but these errors were encountered: