andygrunwald / andygrunwald/go-gerrit
Credentials in the URL are percent-decoded inconsistently depending on whether the URL has a port
- Ngôn ngữ chính
- Go
- Star
- 106
- Fork
- 54
- Merge trung bình
- 7 giờ 21 phút
- Pull request đã merge (30 ngày)
- 3
Mô tả
## Summary
`NewClient` extracts credentials from the URL through two different code paths, and only one of them percent-decodes them. Which path is taken depends on whether the URL contains an explicit port, so the same credentials are interpreted differently for `http://user:pass@host:8080/` and `http://user:pass@host/`.
## Details
`NewClient` first tries the `ReParseURL` regular expression (gerrit.go), and falls back to `url.Parse` if it does not match:
* Regex path (gerrit.go:110-118): `username` and `password` are taken as raw substrings of the URL, so percent-escapes are left untouched.
* `url.Parse` path (gerrit.go:128-139): `url.Userinfo.Username()` and `url.Userinfo.Password()` return percent-decoded values.
Because `ReParseURL` requires an explicit port (`:(\d+)`), a URL without a port never reaches the regex path. So the two paths disagree for any credential that contains a percent-escape.
## Reproduction
```go
func TestCredentialDecoding(t *testing.T) {
for _, rawurl := range []string{
"http://admin:se%2Fcret@localhost:8080/", // has a port -> regex path
"http://admin:se%2Fcret@gerrit.example.com/", // no port -> url.Parse path
} {
if m := gerrit.ReParseURL.FindAllStringSubmatch(rawurl, -1); len(m) > 0 && len(m[0]) > 5 {
t.Logf("%-45s regex user=%q pass=%q", rawurl, m[0][2], m[0][3])
continue
}
u, err := url.Parse(rawurl)
if err != nil {
t.Fatal(err)
}
pass, _ := u.User.Password()
t.Logf("%-45s url.Parse user=%q pass=%q", rawurl, u.User.Username(), pass)
}
}
```
Output:
```
http://admin:se%2Fcret@localhost:8080/ regex user="admin" pass="se%2Fcret"
http://admin:se%2Fcret@gerrit.example.com/ url.Parse user="admin" pass="se/cret"
```
## Expected behaviour
Both paths should produce the same credentials for the same URL. Since `url.Parse` decodes them, the regex path should decode them too, so that a user who correctly percent-encodes a credential gets the decoded value in both cases.
## Actual behaviour
With a port in the URL, the still-encoded string is stored as the secret and sent to the server verbatim (`se%2Fcret` instead of `se/cret`), so authentication fails.
## Suggested fix
Decode the two regex submatches before using them:
```go
username = submatch[2]
password = submatch[3]
if decoded, err := url.PathUnescape(username); err == nil {
username = decoded
}
if decoded, err := url.PathUnescape(password); err == nil {
password = decoded
}
```
Two notes on this:
* `url.PathUnescape` rather than `url.QueryUnescape`: `QueryUnescape` also turns `+` into a space, which would corrupt the very example password quoted in the comment above `ReParseURL` (`ZOSOKjgV/kgEkN0bzPJp+oGeJLqpXykqWFJpon/Ckg`). `PathUnescape` leaves `+` alone.
* Falling back to the raw value on error matters: `PathUnescape` fails on an unencoded `%` (for example `p%ss`), and the regex path exists precisely to accept raw credentials that `url.Parse` rejects. Ignoring the error keeps those working.
I am happy to send a PR with this change plus a table test covering both paths, if this looks like the right direction.
## Environment
* go-gerrit: master (42c7af0)
* Go: 1.16 (go.mod)
Hướng dẫn đóng góp
Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này
Hướng nghiên cứu
Start in gerrit.go around ReParseURL and the NewClient URL-parsing paths. Add a table test covering percent-encoded credentials with and without an explicit port, including plus signs and malformed percent escapes, then run the relevant Go tests to verify both paths produce consistent credentials without breaking raw values.
Do mô hình lập chỉ mục viết ra từ nội dung của issue.
Đánh giá
- Công nghệ
- go
- Lĩnh vực
- api
- Loại issue
- Lỗi
- Độ khó
- 2/5
- Thời gian dự kiến
- 1-3 giờ
- Mức độ hoạt động
- Sôi nổi
- Độ rõ ràng
- Đặc tả rõ ràng
- Mức phù hợp với người mới
- 90/100