[Webkit-unassigned] [Bug 165521] Add Link header support for preload.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 13 13:43:08 PST 2017


Alex Christensen <achristensen at apple.com> changed:

           What    |Removed                     |Added
 Attachment #297011|review?                     |review-
              Flags|                            |

--- Comment #16 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 297011
  --> https://bugs.webkit.org/attachment.cgi?id=297011

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

> Source/WebCore/loader/LinkHeader.cpp:42
> +static bool isValidURLChar(CharType chr)

This seems very strange.  Determining if a character is valid for a URL without context isn't something you should be able to do.  For example, if you have an invalid surrogate pair with a valid begin and an invalid end that is one of the characters that is determined here to be "invalid" then this would naively think the second half of an invalid surrogate pair is an invalid character when in fact the whole thing is something that will be URL encoded to be the replacement character.  It's bad enough that CSP has its own definition of what parsing part of a URL from a header should be (and maybe we should align with that "parsing") that is separate from the URL spec.  Let's not add a third definition of what a URL is supposed to look like.

> Source/WebCore/loader/LinkHeader.cpp:95
> +static bool parseURL(CharType*& position, CharType* end, String& url)

end should definitely be const
I don't like the name "parseURL".  This function doesn't parse the URL, it just finds the end of the URL.
This should return a std::optional<String>

> Source/WebCore/loader/LinkHeader.cpp:122
> +    ASSERT(position <= end);

This assertion should be at every function that has a position and an end.
It seems like an iterator type would be better than raw pointers in this whole patch.  StringView.h has code point and code unit iterators.

> Source/WebCore/loader/LinkHeader.cpp:138
> +static bool parseParameterDelimiter(CharType*& position, CharType* end, bool& isValid)

This should return std::optional<bool> instead of a bool with a bool& parameter.

> Source/WebCore/loader/LinkHeader.cpp:274
> +    else if (name == LinkParameterAnchor)

It looks like a switch would be better here.

> Source/WebCore/loader/LinkHeader.h:78
> +    LinkHeader& operator[](size_t i) { return m_headerSet[i]; }

This doesn't have bounds checks.

> Source/WebCore/loader/LinkHeader.h:82
> +    template <typename CharType>

typename CharacterType

> Source/WebCore/loader/LinkHeader.h:83
> +    void init(CharType* headerValue, unsigned len);

unsigned len -> size_t length
Or even better, use an iterator type.  See below.

> Source/WebCore/loader/LinkLoader.cpp:99
> +        if (url == baseURL)

What if url and baseURL only differ in the fragment?

> LayoutTests/http/tests/preload/resources/download_resources_from_header.php:2
> +header("Link: <../resources/dummy.js>; rel=preload; as=script", false);

Can we put a complete absolute URL here?  We should add a test with a URL that looks like http://localhost:8000/preload/resources/something.html on a page with CSP that doesn't allow loading from localhost.  We should also add a test with an invalid URL like http://host:invalidport/ just to see what happens.  It's hard to make a URL fail to parse when only giving the path.  A fragment-only URL might also be interesting. Also a lowercase "link", a missing colon, no <> or URL, <> but no characters inside it, and a missing space after the colon.  Also what about emojis, invalid surrogate pairs, and null characters in the middle of the URL?

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170113/b676bdb3/attachment.html>

More information about the webkit-unassigned mailing list