The following call will succeed, and result in invalid characters being sent over the wire: webkit_settings_set_user_agent(settings, "\x1B"); /* Escape character */ As a reminder: the HTTP specification indicates that headers must contain only visible, printable characters (so control characters like 0x1B are forbidden). If an user-agent string with an embeeded newline character is set, libsoup will actually warn: soup_message_headers_append: assertion 'strpbrk (value, "\r\n") == NULL' failed I am not sure what libsoup does (maybe it ignores setting the header?), but at any rathe an HTTP header must *not* contain newlines embedded in their values. It would be good to have some validation to ensure that WebKitSettings never ends up with an user-agent string configured which would result in incorrect requests being sent to servers. The particular case of control characters is something I have observed after an use-after-free ended up setting some garbage string as user-agent, and some servers would start mysteriously returning HTTP 400 (that is: “Bad Request”).
In a chat with Claudio Saavedra about this we agreed that we could consider that passing a string as user agent which would result in an incorrect HTTP header can be considered a programming error. That way, we can validate the string and return early using g_return_if_fail() —which will warn— thus avoiding the need to change the API/ABI.
Created attachment 377168 [details] Patch
Attachment 377168 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3076: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3078: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Maybe I should also add some new API function to check whether a string can be used as User-Agent string. Something like: gboolean webkit_is_valid_user_agent (const gchar *user_agent); WDYT? It would be useful to validate them before trying to call webkit_settings_set_user_agent().
(In reply to Build Bot from comment #3) > Attachment 377168 [details] did not pass style-queue: > > > ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3076: One line > control clauses should not use braces. [whitespace/braces] [4] > ERROR: Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3078: One line > control clauses should not use braces. [whitespace/braces] [4] > Total errors found: 2 in 2 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. These are false positives: notice the CHECK_OK macro, which expands parseFoo(CHECK_OK); into: parseFoo(ok); if (!ok) return; ((void) 0); Therefore the braces are needed because those are not really one-line clauses. This preprocessor trick is very handy to make sure that the “ok” status flag is always passed to the parsing functions and checked afterwards.
Comment on attachment 377168 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377168&action=review > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3037 > +// Checks User-Agent strings as per https://tools.ietf.org/html/rfc7231#section-5.5.3 > +// The comments with the BNF productions before the functions below have been copied > +// from the RFC verbatim as reference. > +struct UserAgentChecker { I think this belongs to Source/WebCore/platform/network/HTTPParsers.cpp You can probably reuse code from there.
(In reply to Carlos Garcia Campos from comment #6) > Comment on attachment 377168 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377168&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3037 > > +// Checks User-Agent strings as per https://tools.ietf.org/html/rfc7231#section-5.5.3 > > +// The comments with the BNF productions before the functions below have been copied > > +// from the RFC verbatim as reference. > > +struct UserAgentChecker { > > I think this belongs to Source/WebCore/platform/network/HTTPParsers.cpp You > can probably reuse code from there. Oh, cool, somehow my grepping didn't unearth this file. I see there are other utility functions already there like isValidLanguageHeaderValue() already, so I will move the code to this file, make the entry point be isValidUserAgentHeaverValue() and make the code style similar to the rest of the file. Thanks for the pointer!
Created attachment 377247 [details] WIP Patch Work in progress moving things to HTTPParsers.cpp; I will be adding some unit tests in Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp for the new code.
Attachment 377247 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:10: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 377707 [details] Patch
Created attachment 377713 [details] Patch v2 This adds the symbol to the linker script, which should make the mac and iOS EWS bots happy.
Created attachment 377718 [details] Patch v3 Well, I was wrong: it seems that the right thing to do is marking the symbol using WEBCORE_EXPORT, rather than adding it to WebCore.order. Third time is the charm. Maybe.
Ping reviewers :-)
Comment on attachment 377718 [details] 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.
(In reply to Carlos Garcia Campos from comment #14) > Comment on attachment 377718 [details] > Patch v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377718&action=review > > LGTM \o/ > > 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? There is already a couple of utility function prototypes declared in “HTTPHeaderField.h” inside the WebCore::RFC7230 namespace. We could add the prototypes for the other functions used by “HTTPParsers.cpp” and use them from there. How about doing that in a follow-up patch? > > 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? True, let's change it to that! (Probably I wrote have written the version that takes just the character before writing the overload that takes a predicate, then forgot or didn't notice that one could be written in terms of the other.) > > 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. Done.
Created attachment 378579 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 378579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378579&action=review Clearing r flag since this is already reviewed > Source/WebCore/platform/network/HTTPParsers.h:75 > +WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&); We don't need the export since this is only used by linux ports > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3053 > + userAgentString = WebCore::standardUserAgent(""); I would add an assert in WebCore::standardUserAgent() to ensure the generated user agent is valid.
(In reply to Carlos Garcia Campos from comment #18) > Comment on attachment 378579 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=378579&action=review > > Clearing r flag since this is already reviewed > > > Source/WebCore/platform/network/HTTPParsers.h:75 > > +WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&); > > We don't need the export since this is only used by linux ports ...but we need the export, otherwise TestWebCore does not link. Or do you mean that the unit tests added by the patch should be built only for the GTK and WPE ports, too? > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3053 > > + userAgentString = WebCore::standardUserAgent(""); > > I would add an assert in WebCore::standardUserAgent() to ensure the > generated user agent is valid. Ah, I missed this, so I'll add it before landing.
(In reply to Adrian Perez from comment #19) > (In reply to Carlos Garcia Campos from comment #18) > > Comment on attachment 378579 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=378579&action=review > > > > Clearing r flag since this is already reviewed > > > > > Source/WebCore/platform/network/HTTPParsers.h:75 > > > +WEBCORE_EXPORT bool isValidUserAgentHeaderValue(const String&); > > > > We don't need the export since this is only used by linux ports > > ...but we need the export, otherwise TestWebCore does not link. > Or do you mean that the unit tests added by the patch should be > built only for the GTK and WPE ports, too? No no, you are right, I forgot the unit test, sorry. > > > Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:3053 > > > + userAgentString = WebCore::standardUserAgent(""); > > > > I would add an assert in WebCore::standardUserAgent() to ensure the > > generated user agent is valid. > > Ah, I missed this, so I'll add it before landing. :-)
Created attachment 378644 [details] Patch for landing
Comment on attachment 378644 [details] Patch for landing Clearing flags on attachment: 378644 Committed r249810: <https://trac.webkit.org/changeset/249810>
All reviewed patches have been landed. Closing bug.
(In reply to Adrian Perez from comment #15) > (In reply to Carlos Garcia Campos from comment #14) > > Comment on attachment 377718 [details] > > > > > 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? > > There is already a couple of utility function prototypes declared > in “HTTPHeaderField.h” inside the WebCore::RFC7230 namespace. We could > add the prototypes for the other functions used by “HTTPParsers.cpp” > and use them from there. > > How about doing that in a follow-up patch? Filed bug #201721 for the follow-up refactoring, I'll give it a go in the next days.