Bug 227795

Summary: >4K Referer should have tailing /
Product: WebKit Reporter: Sam Sneddon [:gsnedders] <gsnedders>
Component: Page LoadingAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=215080
Attachments:
Description Flags
Patch none

Description Sam Sneddon [:gsnedders] 2021-07-08 08:46:48 PDT 
Per referrer policy we reduce long referrers to just the origin; however, this should be serialised as per URL which means that it should have a trailing /

This is a relatively big gain on WPT given the large number of referrer policy tests.
Comment 1 Chris Dumez 2021-07-08 08:48:37 PDT 
Example of failing WPT test(s)?
Comment 3 Sam Sneddon [:gsnedders] 2021-07-08 13:27:48 PDT 
Specifically something like https://wpt.fyi/results/referrer-policy/4K+1/gen/top.http-rp/no-referrer-when-downgrade/a-tag.http.html?label=experimental&label=master&aligned is the trailing-/ case
Comment 4 Alex Christensen 2021-07-13 11:29:50 PDT 
Created attachment 433431 [details]
Patch
Comment 5 Chris Dumez 2021-07-13 12:02:45 PDT 
Comment on attachment 433431 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Covered by existing tests and web platform tests we haven't imported yet.

It's too bad that we haven't imported those tests yet.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:79
> +    checkReferer([NSURL URLWithString:longPath], "http://webkit.org/");

It is a bit confusing because this referrer is not > 4K.

Also, it seems like this should be easily writable as a layout test?
Comment 6 Alex Christensen 2021-07-13 12:03:45 PDT 
Comment on attachment 433431 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:79
>> +    checkReferer([NSURL URLWithString:longPath], "http://webkit.org/");
> 
> It is a bit confusing because this referrer is not > 4K.
> 
> Also, it seems like this should be easily writable as a layout test?

This is the trimmed referrer.
Comment 7 Chris Dumez 2021-07-13 12:06:37 PDT 
Comment on attachment 433431 [details]
Patch

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

r=me assuming the bots are happy.

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:79
>>> +    checkReferer([NSURL URLWithString:longPath], "http://webkit.org/");
>> 
>> It is a bit confusing because this referrer is not > 4K.
>> 
>> Also, it seems like this should be easily writable as a layout test?
> 
> This is the trimmed referrer.

Oh, I get it now.

I would have liked if we had a layout test for this (as we can run in other browsers and other ports of WebKit). But technically, you have test coverage so r+.
Comment 8 Alex Christensen 2021-07-13 12:25:42 PDT 
Comment on attachment 433431 [details]
Patch

The web platform tests have done that for me!
Comment 9 EWS 2021-07-13 12:43:50 PDT 
Committed r279886 (239638@main): <https://commits.webkit.org/239638@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433431 [details].
Comment 10 Radar WebKit Bug Importer 2021-07-13 12:44:16 PDT 
<rdar://problem/80532911>