127172 2014-01-17 06:01:13 -0800 Never send a non-http(s) referrer header even with a referrer policy 2014-01-20 11:49:45 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jochen jochen ap commit-queue darin oldest_to_newest 969121 0 jochen 2014-01-17 06:01:13 -0800 Never send a non-http(s) referrer header even with a referrer policy 969123 1 221460 jochen 2014-01-17 06:02:56 -0800 Created attachment 221460 Patch 969124 2 jochen 2014-01-17 06:03:38 -0800 Alexey, can you have a look please? 969153 3 221460 ap 2014-01-17 09:37:28 -0800 Comment on attachment 221460 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221460&action=review It's not cool to have a function with such an ambitious name as shouldHideReferrer, and then have it explained in a header comment what it actually means (that being, do the same thing generateReferrerHeader(ReferrerPolicyDefault), but with a murky restriction of not using the result for Referrer header sending!) There is only one call site outside SecurityPolicy, and that looks incorrect, as it actually blocks Referer header regardless of policy if shouldHideReferrer said so. This patch is an improvement, r=me. > Source/WebCore/page/SecurityPolicy.cpp:75 > + if (!protocolIs(referrer, "http") && !protocolIs(referrer, "https")) > + return String(); This should use url.protocolIsInHTTPFamily(). 969747 4 jochen 2014-01-20 00:34:09 -0800 (In reply to comment #3) > (From update of attachment 221460 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221460&action=review > > It's not cool to have a function with such an ambitious name as shouldHideReferrer, and then have it explained in a header comment what it actually means (that being, do the same thing generateReferrerHeader(ReferrerPolicyDefault), but with a murky restriction of not using the result for Referrer header sending!) Agreed. The ping loader should probably just use generateReferrerHeader() > > There is only one call site outside SecurityPolicy, and that looks incorrect, as it actually blocks Referer header regardless of policy if shouldHideReferrer said so. Since it's not using the string as referrer header, i'm not sure what to do here. Even if the policy is default, the Ping-From field won't be cleared on a https to http transition in the network stack. The most reasonable thing to do would be to change the standard to use the referrer header instead of a Ping-From header. > > This patch is an improvement, r=me. > > > Source/WebCore/page/SecurityPolicy.cpp:75 > > + if (!protocolIs(referrer, "http") && !protocolIs(referrer, "https")) > > + return String(); > > This should use url.protocolIsInHTTPFamily(). That means converting referrer to a KURL, is that ok with you? 969846 5 221460 ap 2014-01-20 08:22:44 -0800 Comment on attachment 221460 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=221460&action=review >>> Source/WebCore/page/SecurityPolicy.cpp:75 >>> + return String(); >> >> This should use url.protocolIsInHTTPFamily(). > > That means converting referrer to a KURL, is that ok with you? You are right, let's keep it as is then. 969860 6 221460 commit-queue 2014-01-20 08:58:30 -0800 Comment on attachment 221460 Patch Clearing flags on attachment: 221460 Committed r162351: <http://trac.webkit.org/changeset/162351> 969861 7 commit-queue 2014-01-20 08:58:32 -0800 All reviewed patches have been landed. Closing bug. 969946 8 darin 2014-01-20 11:49:45 -0800 (In reply to comment #5) > (From update of attachment 221460 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221460&action=review > > >>> Source/WebCore/page/SecurityPolicy.cpp:75 > >>> + return String(); > >> > >> This should use url.protocolIsInHTTPFamily(). > > > > That means converting referrer to a KURL, is that ok with you? > > You are right, let's keep it as is then. We should create a string version of the protocolIsInHTTPFamily function, for the same reason we have a URL version. 221460 2014-01-17 06:02:56 -0800 2014-01-20 08:58:30 -0800 Patch bug-127172-20140117150254.patch text/plain 1548 jochen U3VidmVyc2lvbiBSZXZpc2lvbjogMTYyMTk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTlmY2MwMTJlMjYzM2Qz NDBkZmVkNTRjMDQ5OTgwNGUwNDQ2NWU3Mi4uOWE4MzdjYWQyNTBiMWVlOTZiMDNlNTY1NTk2Mzkw Y2RjNGU0ZWIxMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDE0LTAxLTE3ICBKb2No ZW4gRWlzaW5nZXIgIDxqb2NoZW5AY2hyb21pdW0ub3JnPgorCisgICAgICAgIE5ldmVyIHNlbmQg YSBub24taHR0cChzKSByZWZlcnJlciBoZWFkZXIgZXZlbiB3aXRoIGEgcmVmZXJyZXIgcG9saWN5 CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjcxNzIK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIG1p cnJvcnMgdGhlIGNvZGUgU2VjdXJpdHlQb2xpY3k6OnNob3VsZEhpZGVSZWZlcnJlciB3aGljaCBp cyB1c2VkCisgICAgICAgIGZvciBSZWZlcnJlclBvbGljeURlZmF1bHQuCisKKyAgICAgICAgTm8g bmV3IHRlc3RzLCBvbmx5IGFmZmVjdHMgYW4gZW1iZWRkZXIgdGhhdCByZWdpc3RlcnMgb3RoZXIg c2NoZW1lcywKKyAgICAgICAgZS5nLiBjaHJvbWU6Ly8KKworICAgICAgICAqIHBhZ2UvU2VjdXJp dHlQb2xpY3kuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2VjdXJpdHlQb2xpY3k6OmdlbmVyYXRl UmVmZXJyZXJIZWFkZXIpOgorCiAyMDE0LTAxLTE3ICBaYW4gRG9iZXJzZWsgIDx6ZG9iZXJzZWtA aWdhbGlhLmNvbT4KIAogICAgICAgICBbQVRLXSBNb2Rlcm5pemUgdGhlIGZvciBsb29wcyBpbiBB VEsgQVggY29kZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9TZWN1cml0eVBvbGlj eS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1NlY3VyaXR5UG9saWN5LmNwcAppbmRleCA5N2Uw NzA2ZDNkOWZiZmMxZTExMmQ5NTk1NzUzZDIwNzVlNmM2NjExLi4xN2IwOTMzYmMyN2VhMjExODU4 YjU0MDMwYTgzNDYwNDYwNzU0MTBlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL1Nl Y3VyaXR5UG9saWN5LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1NlY3VyaXR5UG9saWN5 LmNwcApAQCAtNzEsNiArNzEsOSBAQCBTdHJpbmcgU2VjdXJpdHlQb2xpY3k6OmdlbmVyYXRlUmVm ZXJyZXJIZWFkZXIoUmVmZXJyZXJQb2xpY3kgcmVmZXJyZXJQb2xpY3ksIGNvbgogICAgIGlmIChy ZWZlcnJlci5pc0VtcHR5KCkpCiAgICAgICAgIHJldHVybiBTdHJpbmcoKTsKIAorICAgIGlmICgh cHJvdG9jb2xJcyhyZWZlcnJlciwgImh0dHAiKSAmJiAhcHJvdG9jb2xJcyhyZWZlcnJlciwgImh0 dHBzIikpCisgICAgICAgIHJldHVybiBTdHJpbmcoKTsKKwogICAgIHN3aXRjaCAocmVmZXJyZXJQ b2xpY3kpIHsKICAgICBjYXNlIFJlZmVycmVyUG9saWN5TmV2ZXI6CiAgICAgICAgIHJldHVybiBT dHJpbmcoKTsK