Bug 211603

Summary: Limit HTTP referer to 4kb
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, cdumez, darin, ggaren, webkit-bug-importer, wilander, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=211731
https://bugs.webkit.org/show_bug.cgi?id=212180
Attachments:
Description Flags
Patch
none
Patch none

Description Alex Christensen 2020-05-07 16:31:59 PDT
Limit HTTP referer to 4kb
Comment 1 Alex Christensen 2020-05-07 16:38:25 PDT
Created attachment 398811 [details]
Patch
Comment 2 Alexey Proskuryakov 2020-05-07 17:29:09 PDT
Comment on attachment 398811 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Use the origin if it's longer, unless the origin is too long.

What do other browsers do?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:49
> +TEST(WebKit, HTTPReferer)

Can this be a layout test? API tests are bad news each time.
Comment 3 Alex Christensen 2020-05-07 19:26:24 PDT
Comment on attachment 398811 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        Use the origin if it's longer, unless the origin is too long.
> 
> What do other browsers do?

They do this.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1557346

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:49
>> +TEST(WebKit, HTTPReferer)
> 
> Can this be a layout test? API tests are bad news each time.

We could test long referrers in layout tests with queries instead of paths, but we cannot test the behavior of long hosts in layout tests because we are limited to 127.0.0.1 and localhost.  If you feel strongly enough that this should sacrifice test coverage to be a layout test I'll do it, but I find this superior.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:58
> +                WTFLogAlways("REQUEST %s", request.data());

I will remove this before committing.
Comment 4 Alexey Proskuryakov 2020-05-08 09:34:46 PDT
I don't feel very strongly about this particular case, but API tests are slow, and we have no path towards making them fast.

So if we keep adding them, eventually we'll have to stop running them or something.
Comment 5 Alex Christensen 2020-05-08 09:36:21 PDT
We should make a path towards making them fast.  Most of them can be run in parallel, and those that can't should be marked as such.

AP, how would you feel about reviewing this patch?
Comment 6 Alexey Proskuryakov 2020-05-08 09:54:04 PDT
> We should make a path towards making them fast.

I doubt that this "should" will turn into "have done". I can r+ with a layout test, but won't stop others from reviewing.
Comment 7 Chris Dumez 2020-05-08 10:21:21 PDT
Comment on attachment 398811 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        Use the origin if it's longer, unless the origin is too long.
>> 
>> What do other browsers do?
> 
> They do this.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1557346

Please update the change log to indicate that.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:382
> +        String origin = SecurityOrigin::create(URL(URL(), httpReferrer))->toString();

Please add release logging (maybe even release error logging) to indicate that this is happening, in case we have to investigate regressions caused by this.
Comment 8 Alex Christensen 2020-05-08 10:35:42 PDT
Created attachment 398874 [details]
Patch
Comment 9 EWS 2020-05-08 12:39:47 PDT
Committed r261402: <https://trac.webkit.org/changeset/261402>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398874 [details].
Comment 10 Radar WebKit Bug Importer 2020-05-08 12:40:14 PDT
<rdar://problem/63032015>
Comment 11 Alex Christensen 2020-05-08 13:58:30 PDT
https://trac.webkit.org/changeset/261414/webkit
Comment 12 Darin Adler 2020-05-08 14:21:21 PDT
Comment on attachment 398874 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.cpp:381
> +    const size_t maxLength = 4096;

New code should use constexpr.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:383
> +        RELEASE_LOG(Loading, "Truncating HTTP referer");

Could be better if we logged different messages if we truncate to origin, or if we don’t set Referer at all.
Comment 13 youenn fablet 2020-05-09 02:51:13 PDT
Comment on attachment 398874 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        See https://bugzilla.mozilla.org/show_bug.cgi?id=1557346

Would have been good to refer to https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer which describes this behavior.

I believe this impacts service workers.
Is there a way we can add a test checking the fetch event request referrer as well (and verify other browsers behave the same)?
It seems a bit sad that this particular restriction is visible from service worker, especially since there may be some user agent specific restrictions, but that is what the spec mandates currently.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:386
> +            setHTTPHeaderField(HTTPHeaderName::Referer, origin);

This does not seem to be described by the spec although we can do it as per step 7.
Also if the origin value is too long, what can we do about the Origin header?