<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add Link header support for preload."
href="https://bugs.webkit.org/show_bug.cgi?id=165521#c17">Comment # 17</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add Link header support for preload."
href="https://bugs.webkit.org/show_bug.cgi?id=165521">bug 165521</a>
from <span class="vcard"><a class="email" href="mailto:yoav@yoav.ws" title="Yoav Weiss <yoav@yoav.ws>"> <span class="fn">Yoav Weiss</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=297011&action=diff" name="attach_297011" title="Patch">attachment 297011</a> <a href="attachment.cgi?id=297011&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=297011&action=review">https://bugs.webkit.org/attachment.cgi?id=297011&action=review</a>
<span class="quote">>> 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.</span >
OK. Since URL validation is not critical here (and will happen further down the pipe), I've changed this to just detect ">" as the URL boundary.
<span class="quote">>> 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></span >
Good point regarding end. Changed parseURL into FindURLBoundaries with an optional return value.
<span class="quote">>> 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.</span >
Good point regarding the assertion. Added everywhere else.
Regarding switching to StringView's iterators I tried to convert the logic to that, but it seems to be missing some concepts needed here (skipWhile, skipExactly and/or getting an iterator for a position in the middle of a StringView).
For now I kept it using pointers, but if we want to go the iterator route, it'd require adding a new iterator type (e.g. ParserStringView) which will include the required concepts. Let me know if you think this is the route we should take and if so, where should such a class be located.
<span class="quote">>> 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.</span >
OK
<span class="quote">>> Source/WebCore/loader/LinkHeader.cpp:274
>> + else if (name == LinkParameterAnchor)
>
> It looks like a switch would be better here.</span >
switched
<span class="quote">>> Source/WebCore/loader/LinkHeader.h:78
>> + LinkHeader& operator[](size_t i) { return m_headerSet[i]; }
>
> This doesn't have bounds checks.</span >
This as well as size() don't seem to be required. Removed both
<span class="quote">>> Source/WebCore/loader/LinkHeader.h:82
>> + template <typename CharType>
>
> typename CharacterType</span >
OK
<span class="quote">>> 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.</span >
switched to size_t for now
<span class="quote">>> Source/WebCore/loader/LinkLoader.cpp:99
>> + if (url == baseURL)
>
> What if url and baseURL only differ in the fragment?</span >
Good point. Switched to use equalIgnoringFragmentIdentifier
<span class="quote">>> 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 <a href="http://localhost:8000/preload/resources/something.html">http://localhost:8000/preload/resources/something.html</a> on a page with CSP that doesn't allow loading from localhost. We should also add a test with an invalid URL like <a href="http://host:invalidport/">http://host:invalidport/</a> 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?</span >
Added tests for CSP blocked, invalid port, fragment-only, lower-case link, missing colon, missing spaces, missing URL, etc.
(Invalid port passes the isPreloaded() test, but I'm guessing that it's being blocked further down the stack)
I'm not sure how to add emojis, invalid surrogate pairs and null chars in PHP... Any examples you can point me to?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>