Bug 227795 - >4K Referer should have tailing /
Summary: >4K Referer should have tailing /
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-08 08:46 PDT by Sam Sneddon [:gsnedders]
Modified: 2021-07-27 09:06 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.29 KB, patch)
2021-07-13 11:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>