okhttp3.internal.http.HttpHeaders#parseChallenges does not handle authentication header with only scheme

[Vampire]

~If you e. g. have Proxy-Authenticate: Basic which is a valid value, the method will not work properly, because of int index = header.indexOf(' '); if (index == -1) continue;, which means the method only works properly if there is a space in the header value.~ (For Basic auth realm indeed is required)

This also breaks okhttp3.Response#challenges and okhttp3.JavaNetAuthenticator#authenticate.

[Vampire]

~Oh, it's even worse, even with a space in the header it does not work, as the code depends on a realm being specified, while https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authenticate show that it is optional.~ (For Basic auth realm indeed is required)

And https://tools.ietf.org/html/rfc7235#section-4.1 defines that User agents are advised to take special care in parsing the field value, as it might contain more than one challenge, and each challenge can contain a comma-separated list of authentication parameters. Furthermore, the header field itself can occur multiple times. and the mentioned method cares about multiple headers, but not about mutliple challenges in one header.

[Vampire]

It also does not work if the realm is not quoted. The used regex matches it, but the value is in group 4 if there are no quotes and it is always group 3 used, both should be considered.

Besides that it should probably be group 2 and 3, not 3 and 4.
In the regex there is :? used twice which is an optional colon, while I think this should be ?: which would then be a non-capturing group which makes more sense here.

You chould also use named groups, so that the numbering cannot get confused by newly added parens. The current regex is
+([^ "=]*)=(:?"([^"]*)"|([^ "=]*)) *(:?,|$)
or with the optional colons fixed
+([^ "=]*)=(?:"([^"]*)"|([^ "=]*)) *(?:,|$)
but instead you could use
+(?<key>[^ "=]*)=(?:"(?<value1>[^"]*)"|(?<value2>[^ "=]*)) *(?:,|$)
snd then you can use matcher.group("key"), matcher.group("value1") and matcher.group("value2").

[Vampire]

And also the list of authentication parameters is defined to be comma-separated, not white-space separated or comma-whitespace-separated, but your current regex only matches parameters that follow spaces, that is also not correct.

[Vampire]

Also spaces around the "=" in the authentication parameter are not considered correctly.
Here you can find ABNF description of the format of the headers: https://tools.ietf.org/html/rfc7235#appendix-C

Here a small selection of challenges that should all work, but only the first one is working as exected:

 ,  , Basic realm=foo
Basic realm=foo
Basic realm="foo"
Basic realm = "foo"
Digest,Basic realm="foo"
Digest,Basic ,,realm="foo"
Digest, Basic realm="foo"
Digest, Basic ,,realm="foo"
Digest,,,, Basic ,,realm="foo"
Digest,,,, Basic ,,,realm="f\"o\o"

[swankjesse]

Yikes. Wanna send a pull request?

[Vampire]

I'm currently trying to write a parser that handles the spec as work around, maybe. If I'm successful I maybe also make a PR out of it.

[swankjesse]

Please do! Ideally you don’t need hundreds of lines of code to get something satisfactory.

[Vampire]

76 lines of code and 18 private static final fields, so actually not even one hundred. :-D
And it works fully compliant to RFC 7235 for any auth scheme.
Well, for the PR I probably have to rewrite it a tiny bit to become 1.7-compatible as it is currently written in 1.8 and throws away anything non-Basic.
But as hundreds would imply at least 200, I think I'll stay below that. :-D

[swankjesse]

Great!