andygrunwald / andygrunwald/go-gerrit
Credentials in the URL are percent-decoded inconsistently depending on whether the URL has a port
- Vorherrschende Sprache
- Go
- Sterne
- 106
- Forks
- 54
- Ø Merge
- 7 Std. 21 Min.
- Gemergte PRs (30 T.)
- 3
Beschreibung
## 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)
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Rechercherichtung
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.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- go
- Bereich
- api
- Issue-Typ
- Bug
- Schwierigkeit
- 2/5
- Geschätzter Aufwand
- 1-3 Stunden
- Aktivitätsstatus
- Aktiv
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 90/100