On 2022-09-19 09:21, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>
>> + /**
>> + * A `strvec` of WWW-Authenticate header values. Each string
>> + * is the value of a WWW-Authenticate header in an HTTP response,
>> + * in the order they were received in the response.
>> + */
>> + struct strvec wwwauth_headers;
>
> I like this careful documentation.
>
>> + unsigned header_is_last_match:1;
>
> But then this member is unclear how it is attached. It could use its
> own "for internal use" comment if we don't want to describe it in full
> detail here.
A fair point. I will update in a future iteration.
>> +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 */
>
> Is it really a CRLF? Or just an LF?
It is indeed an CRLF, agnostic of platform. HTTP defines CRLF as the
end-of-line marker for all entities other than the body.
See RFC 2616 section 2.2: https://www.rfc-editor.org/rfc/rfc2616#section-2.2
>> + strbuf_trim_trailing_newline(&buf);
>
> Thankfully, this will trim an LF _or_ CR/LF pair, so either way would be fine.
>
>> + /* Start of a new WWW-Authenticate header */
>> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
>> + while (isspace(*val)) val++;
>
> Break the "val++;" to its own line:
>
> while (isspace(*val))
> val++;
Sure! Sorry I missed this one.
> While we are here, do we need to be careful about the end of the string at
> this point? Is it possible that the server will send all spaces up until the
> maximum header size (as mentioned in the message)?
>
>> +
>> + strvec_push(values, val);
>> + 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;
>
> I suppose we expect leading spaces as critical to this header, right?
Leading (and trailing) spaces are not part of the header value.
>From RFC 2616 section 2.2 regarding header field values:
"All linear white space, including folding, has the same semantics as SP.
A recipient MAY replace any linear white space with a single SP before
interpreting the field value or forwarding the message downstream."
>> + char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
>
> We might have better luck using a strbuf, initializing it with the expected
> size and using strbuf_add() to append the strings. Maybe I'm just prematurely
> optimizing, though.
This code path is used to re-join/fold a header value continuation, which is
pretty rare in the wild (if at all with modern web servers).
>> +
>> + free((void*)*v);
>> + *v = append;
>> +
>> + 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);
>> +
>> +exit:
>> + strbuf_release(&buf);
>> + return size;
>> +}
>> +
>> size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
>> {
>> return nmemb;
>> @@ -1829,6 +1904,8 @@ static int http_request(const char *url,
>> fwrite_buffer);
>> }
>>
>> + curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
>
> Nice integration point!
>
> Thanks,
> -Stolee
Thanks,
Matthew
