<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:achristensen@apple.com" title="Alex Christensen <achristensen@apple.com>"> <span class="fn">Alex Christensen</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #297011 Flags</td>
<td>review?
</td>
<td>review-
</td>
</tr></table>
<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#c16">Comment # 16</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:achristensen@apple.com" title="Alex Christensen <achristensen@apple.com>"> <span class="fn">Alex Christensen</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)</span >
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 class="quote">> Source/WebCore/loader/LinkHeader.cpp:95
> +static bool parseURL(CharType*& position, CharType* end, String& url)</span >
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 class="quote">> Source/WebCore/loader/LinkHeader.cpp:122
> + ASSERT(position <= end);</span >
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 class="quote">> Source/WebCore/loader/LinkHeader.cpp:138
> +static bool parseParameterDelimiter(CharType*& position, CharType* end, bool& isValid)</span >
This should return std::optional<bool> instead of a bool with a bool& parameter.
<span class="quote">> Source/WebCore/loader/LinkHeader.cpp:274
> + else if (name == LinkParameterAnchor)</span >
It looks like a switch would be better here.
<span class="quote">> Source/WebCore/loader/LinkHeader.h:78
> + LinkHeader& operator[](size_t i) { return m_headerSet[i]; }</span >
This doesn't have bounds checks.
<span class="quote">> Source/WebCore/loader/LinkHeader.h:82
> + template <typename CharType></span >
typename CharacterType
<span class="quote">> Source/WebCore/loader/LinkHeader.h:83
> + void init(CharType* headerValue, unsigned len);</span >
unsigned len -> size_t length
Or even better, use an iterator type. See below.
<span class="quote">> Source/WebCore/loader/LinkLoader.cpp:99
> + if (url == baseURL)</span >
What if url and baseURL only differ in the fragment?
<span class="quote">> LayoutTests/http/tests/preload/resources/download_resources_from_header.php:2
> +header("Link: <../resources/dummy.js>; rel=preload; as=script", false);</span >
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?</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>