Summary: | Limit HTTP referer to 4kb | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
Component: | New Bugs | Assignee: | 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
Alex Christensen
2020-05-07 16:31:59 PDT
Created attachment 398811 [details]
Patch
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 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. 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. 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? > 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 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. Created attachment 398874 [details]
Patch
Committed r261402: <https://trac.webkit.org/changeset/261402> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398874 [details]. 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 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? |