andygrunwald / andygrunwald/go-gerrit

Credentials in the URL are percent-decoded inconsistently depending on whether the URL has a port

Offen Anfängerfreundlich
#224 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
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

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.