Bug 211603 – Limit HTTP referer to 4kb

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Limit HTTP referer to 4kb

[Alex Christensen]

Created attachment 398811 [details]
Patch

[Alexey Proskuryakov]

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.

[Alex Christensen]

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.

[Alexey Proskuryakov]

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.

[Alex Christensen]

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?

[Alexey Proskuryakov]

> 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.

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 398874 [details]
Patch

[EWS]

Committed r261402: <https://trac.webkit.org/changeset/261402>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398874 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63032015>

[Alex Christensen]

https://trac.webkit.org/changeset/261414/webkit

[Darin Adler]

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.

[youenn fablet]

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?