[webkit-reviews] review granted: [Bug 201077] [GTK][WPE] webkit_settings_set_user_agent() allows content forbidden in HTTP headers : [Attachment 377718] Patch v3

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has granted Adrian Perez
<aperez at igalia.com>'s request for review:
Bug 201077: [GTK][WPE] webkit_settings_set_user_agent() allows content
forbidden in HTTP headers
https://bugs.webkit.org/show_bug.cgi?id=201077

Attachment 377718: Patch v3

https://bugs.webkit.org/attachment.cgi?id=377718&action=review




--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 377718
  --> https://bugs.webkit.org/attachment.cgi?id=377718
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=377718&action=review

LGTM

> Source/WebCore/platform/network/HTTPParsers.cpp:177
> +// See RFC 7230, Section 3.2.6.
> +static inline bool isOctectInFieldContentCharacter(const UChar c)
> +{
> +    // obs-text = %x80-FF
> +    return (c >= 0x80 && c <= 0xFF);
> +}
> +
> +// See RFC 7230, Section 3.2.6.
> +static bool isCommentTextCharacter(const UChar c)
> +{
> +    // ctext = HTAB / SP
> +    //	/ %x21-27 ; '!'-'''
> +    //	/ %x2A-5B ; '*'-'['
> +    //	/ %x5D-7E ; ']'-'~'
> +    //	/ obs-text
> +    return (c == '\t' || c == ' '
> +	   || (c >= 0x21 && c <= 0x27)
> +	   || (c >= 0x2A && c <= 0x5B)
> +	   || (c >= 0x5D && c <= 0x7E)
> +	   || isOctectInFieldContentCharacter(c));
> +}

We have these already in HTTPHeaderField.cpp maybe we can move them to a common
place?

> Source/WebCore/platform/network/HTTPParsers.cpp:246
> +static inline bool skipCharacter(const String& value, unsigned& pos, const
UChar expected)
> +{
> +    if (pos < value.length() && value[pos] == expected) {
> +	   ++pos;
> +	   return true;
> +    }
> +    return false;
> +}

Can't this be used by passing a predicate doing the value[pos] == expected
check?

> Source/WebCore/platform/network/HTTPParsers.cpp:275
> +// True is a comment is present, incrementing "pos" to the position after
the comment.

True if

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3048
> +    g_return_if_fail(WebCore::isValidUserAgentHeaderValue(userAgentString));

I think we should only validate the given user agent. The standard user agent
is generated by WebKit, it's expected to be valid. We could add an assert in
WebCore::standardUserAgent() before returnig the generated value, though.