Matthew John Cheetham via GitGitGadget wrote:
> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
> +{
> + size_t size = eltsize * nmemb;
> + struct strvec *values = &http_auth.wwwauth_headers;
> + struct strbuf buf = STRBUF_INIT;
> + const char *val;
> + const char *z = NULL;
> +
> + /*
> + * Header lines may not come NULL-terminated from libcurl so we must
> + * limit all scans to the maximum length of the header line, or leverage
> + * strbufs for all operations.
> + *
> + * In addition, it is possible that header values can be split over
> + * multiple lines as per RFC 2616 (even though this has since been
> + * deprecated in RFC 7230). A continuation header field value is
> + * identified as starting with a space or horizontal tab.
> + *
> + * The formal definition of a header field as given in RFC 2616 is:
> + *
> + * message-header = field-name ":" [ field-value ]
> + * field-name = token
> + * field-value = *( field-content | LWS )
> + * field-content = <the OCTETs making up the field-value
> + * and consisting of either *TEXT or combinations
> + * of token, separators, and quoted-string>
> + */
> +
> + strbuf_add(&buf, ptr, size);
> +
> + /* Strip the CRLF that should be present at the end of each field */
> + strbuf_trim_trailing_newline(&buf);
> +
> + /* Start of a new WWW-Authenticate header */
> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
> + while (isspace(*val))
> + val++;
Per the RFC [1]:
> The field value MAY be preceded by any amount of LWS, though a single SP
> is preferred.
And LWS (linear whitespace) is defined as:
> CRLF = CR LF
> LWS = [CRLF] 1*( SP | HT )
and 'isspace()' includes CR, LF, SP, and HT [2].
Looks good!
[1] https://datatracker.ietf.org/doc/html/rfc2616#section-4-2
[2] https://linux.die.net/man/3/isspace
> +
> + strvec_push(values, val);
I had the same question about "what happens with an empty 'val' here?" as
Stolee did earlier [3], but I *think* the "zero length" (i.e., single null
terminator) will be copied successfully. It's probably worth testing that
explicitly, though (I see you set up tests in later patches - ideally a
"www-authenticate:<mix of whitespace>" line could be tested there).
[3] https://lore.kernel.org/git/9fded44b-c503-f8e5-c6a6-93e882d50e27@xxxxxxxxxx/
> + http_auth.header_is_last_match = 1;
> + goto exit;
> + }
> +
> + /*
> + * This line could be a continuation of the previously matched header
> + * field. If this is the case then we should append this value to the
> + * end of the previously consumed value.
> + */
> + if (http_auth.header_is_last_match && isspace(*buf.buf)) {
> + const char **v = values->v + values->nr - 1;
> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
In this case (where the line is a continuation of a 'www-authenticate'
header), it looks like the code here expects *exactly* one LWS at the start
of the line ('isspace(*buf.buf)' requiring at least one space to append the
header, 'ptr + 1' skipping no more than one). But, according to the RFC, it
could be more than one:
> Header fields can be extended over multiple lines by preceding each extra
> line with at least one SP or HT.
So I think 'buf.buf' might need to have all preceding spaces removed, like
you did in the "Start of a new WWW-Authenticate header" block.
Also, if you're copying 'ptr' into 'buf' to avoid issues from a missing null
terminator, wouldn't you want to use 'buf.buf' (instead of 'ptr') in
'xstrfmt()'?
> +
> + free((void*)*v);
> + *v = append;
I was about to suggest (optionally) rewriting this to use 'strvec_pop()' and
'strvec_push_nodup()':
strvec_pop(values);
strvec_push_nodup(values, append);
to maybe make this a bit easier to follow, but unfortunately
'strvec_push_nodup()' isn't available outside of 'strvec.c'. If you did want
to use 'strvec' functions, you could remove the 'static' from
'strvec_push_nodup()' and add it to 'strvec.h' it in a later reroll, but I
don't consider that change "blocking" or even important enough to warrant
its own reroll.
> +
> + goto exit;
> + }
> +
> + /* This is the start of a new header we don't care about */
> + http_auth.header_is_last_match = 0;
> +
> + /*
> + * If this is a HTTP status line and not a header field, this signals
> + * a different HTTP response. libcurl writes all the output of all
> + * response headers of all responses, including redirects.
> + * We only care about the last HTTP request response's headers so clear
> + * the existing array.
> + */
> + if (skip_iprefix(buf.buf, "http/", &z))
> + strvec_clear(values);
The comments describing the intended behavior (as well as the commit
message) are clear and explain the somewhat esoteric (at least to my
untrained eye ;) ) code. Thanks!
> +
> +exit:
> + strbuf_release(&buf);
> + return size;
> +}
> +
> size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
> {
> return nmemb;
> @@ -1864,6 +1940,8 @@ static int http_request(const char *url,
> fwrite_buffer);
> }
>
> + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
> +
> accept_language = http_get_accept_language_header();
>
> if (accept_language)
