Bug 182644 – The referer header is not set after redirect

• Component: Page Loading
• Status: RESOLVED FIXED

When making a xhr request that is redirected the Referer header is not being set. This is happening with Safari 11.0.3 running on High Sierra. This did not happen with Safari 11.0.3 running on Sierra.

example function
function getAccessToken(){
$.ajax({
url: "https://example.website.com/token",
xhrFields: {
withCredentials: true
},
dataType: 'json',
type: "GET",
crossDomain: true,
context: this,
success : function (data, textStatus) {
window.accesstoken = data.access_token;
}
})
}

[Alexey Proskuryakov]

Would it be feasible for you to provide a complete functioning example?

https://mw-uat.putnam.com/demo/example.jsp

example.jsp has a button named "redirect". When clicked it will make a xhr request to mw-stg.putnam.com which re-directs to oam-uat.putnam.com and then back to mw-stg.putnam.com if successful. 

Since we do not allow requests when the Origin header is null and the Referer header is not from a valid resource you will see this in the console for requests made with High Sierra.

Cross-origin redirection to https://mw-stg.putnam.com/demo/webkit.jsp denied by Cross-Origin Resource Sharing policy: Origin null is not allowed by Access-Control-Allow-Origin.

[Alexey Proskuryakov]

I can reproduce this with Safari 10.1.2 (12603.3.8) running on macOS Sierra 10.12.6.

Do you still have the setup where this didn't reproduce? Perhaps there is some difference in Safari preferences there (e.g. CORS checks being disabled via Developer menu).

[Alexey Proskuryakov]

Regression aspect notwithstanding, I was trying to find whether we do this intentionally on cross-origin HTTPS redirects, but couldn't.

For Apple employees, see also rdar://problem/11558962, rdar://problem/34904787.

[Alexey Proskuryakov]

Using Charles proxy, I see that the request to mw-stg has a referer, but the request to oam-uat does not. It does feel like a bug.

[Alexey Proskuryakov]

Created attachment 333645 [details]
Charles session

[Radar WebKit Bug Importer]

<rdar://problem/37479048>

I tried this on a different Sierra machine and it is not setting the Referer header with the redirect. There must have been another setting in Safari allowing it to be set.

Please let me know if you need any more information.

Thanks

[Sihui Liu]

Created attachment 336386 [details]
Patch

[EWS Watchlist]

Comment on attachment 336386 [details]
Patch

Attachment 336386 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7076244

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/referrer.any.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-worker.html
imported/w3c/web-platform-tests/fetch/api/basic/referrer.any.worker.html

[EWS Watchlist]

Created attachment 336393 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Chris Dumez]

Comment on attachment 336386 [details]
Patch

r- due to layout test failures. Those are not new PASS lines so this would indicate the patch is wrong in some way.

[Sihui Liu]

Created attachment 336400 [details]
Patch

[Chris Dumez]

Comment on attachment 336400 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-expected.txt:-20
> -PASS Cross origin redirection, empty init, same-origin redirect header  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-expected.txt:-23
> -PASS Cross origin redirection, empty init, no-referrer redirect header  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-expected.txt:-28
> -PASS Cross origin redirection, empty redirect header, same-origin init  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-expected.txt:-31
> -PASS Cross origin redirection, empty redirect header, no-referrer init  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-worker-expected.txt:-20
> -PASS Cross origin redirection, empty init, same-origin redirect header  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-worker-expected.txt:-23
> -PASS Cross origin redirection, empty init, no-referrer redirect header  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-worker-expected.txt:-28
> -PASS Cross origin redirection, empty redirect header, same-origin init  

This regressed.

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-worker-expected.txt:-31
> -PASS Cross origin redirection, empty redirect header, no-referrer init  

This regressed.

[Chris Dumez]

Comment on attachment 336400 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/fetch/api/redirect/redirect-referrer-expected.txt:-31
>> -PASS Cross origin redirection, empty redirect header, no-referrer init  
> 
> This regressed.

Sihui explained offline that we do not support the redirect header and we used to pass simply because we were never setting the referrer. I'll defer to Youenn on wether it is OK or not. My personal concern is that until we support this header, and because of this patch, we now sometimes expose information (a referrer URL) that we are not supposed to. For privacy reason, this seems bad and it may be better to never expose something rather than expose it always, even when we're not supposed to.

If we do not want to add full support for this referrer header, maybe we could at least omit the referrer entirely if the header is present, as a cheap way to avoid unnecessary information exposure?

Again, I'll defer to Youenn to decide what we want to do. I merely wanted to raise the issue.

[youenn fablet]

Comment on attachment 336400 [details]
Patch

Looks to go in the right direction.
I suggested to not implement Referrer-Policy header in redirect response but looking at it again, it should be small enough that we should do it in this patch.

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

> Source/WebCore/loader/CrossOriginAccessControl.cpp:63
> +void updateRequestReferrerForAccessControl(ResourceRequest& request, ReferrerPolicy referrerPolicy, String& outgoingOrigin, String& outgoingReferrer) 

I am not sure it is worth adding this method.
The implementation for first request and redirect requests are quite different.

> Source/WebCore/loader/SubresourceLoader.cpp:576
> +        updateRequestReferrerForAccessControl(newRequest, m_frame.get()->document()->referrerPolicy(), outgoingOrigin, outgoingReferrer);

We are trying to implement https://w3c.github.io/webappsec-referrer-policy/#set-requests-referrer-policy-on-redirect here.
Looking at it, and given we have redirectResponse, it would make sense to use redirectResponse Referrer-Policy header instead of using the document referrer policy here.
Hopefully, this should not be a big addition to the patch.

> LayoutTests/http/wpt/fetch/referrer-cors-redirect.js:4
> +    importScripts("/common/get-host-info.sub.js");

You only need these if you are in a worker, so this is probably not needed here.
It might be nice though to have fetch test in both worker and document environment.
In that case, you would need to rename referrer-cors-redirect.js to referrer-cors-redirect.any.js (see *.any.js/*.any.html tests elsewhere in web-platform-tests).

> LayoutTests/http/wpt/fetch/referrer-cors-redirect.js:15
> +testImageReferrer("Default Referrer Policy, CORS Image, source-url: " + sourceUrl + ", redirect-url: " + resultUrl, sourceUrl, resultUrl, localResultUrl, locationUrl);

We usually define the functions before using them.

> LayoutTests/http/wpt/fetch/referrer-cors-redirect.js:46
> +    promise_test(test=>{

By doing async (test) => {
you can write something like:
await loadImage(...).
let response = await fetch(...).

> LayoutTests/http/wpt/fetch/referrer-cors-redirect.js:56
> +done();

Probably not needed.

> LayoutTests/http/wpt/fetch/resources/referrer-cors-result.py:7
> +    if request.headers.get("Origin"):

This is quite subtle. How about adding an URL parameter that says whether to write the referrer or to read it?

[EWS Watchlist]

Comment on attachment 336400 [details]
Patch

Attachment 336400 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7081850

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 336451 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Sihui Liu]

Created attachment 336638 [details]
Patch

[Chris Dumez]

Comment on attachment 336638 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:9
> +        Rebaseline some tests for fetch api as they are passing now.

nice.

[Chris Dumez]

Comment on attachment 336638 [details]
Patch

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

> Source/WebCore/loader/SubresourceLoader.cpp:592
> +void SubresourceLoader::updateReferrerPolicy(const String& referrerPolicyStr)

Can we try and avoid the code duplication with Document::processReferrerPolicy() ?

> Source/WebCore/loader/SubresourceLoader.cpp:622
> +    return mutableOptions().referrerPolicy;

Why do you need the mutableOptions() here and not options()?

[youenn fablet]

Comment on attachment 336638 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Add support for ReferrerPolicy header.

Great patch, it looks almost good to go to me.
Some suggestions below.

Could you add some more details about your changes in the change log?
For instance say that we are now updating the referrer policy on redirections based on ReferrerPolicy header value and computing the referrer value of the new request based on it.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:63
> +void updateRequestReferrer(ResourceRequest& request, ReferrerPolicy referrerPolicy, String& outgoingReferrer)

Can we make outgoingReferrer (last parameter) be an in parameter only?
That would mean making it a const String&.

> Source/WebCore/loader/ResourceLoader.h:152
> +    ResourceLoaderOptions& mutableOptions() { return m_options; }

How about adding just a protected setReferrerPolicy()?

> Source/WebCore/loader/SubresourceLoader.cpp:573
> +        updateReferrerPolicy(redirectResponse.httpHeaderField(HTTPHeaderName::ReferrerPolicy));

We could probably call updateReferrerPolicy unconditionally and return early if the header value is empty.
That would allow us to make only one lookup in the header table.

> Source/WebCore/loader/SubresourceLoader.cpp:575
> +    String outgoingReferrer = m_frame.get()->loader().outgoingReferrer();

We probably do not need this referrer, previousRequest.httpReferrer() should be sufficient.

> Source/WebCore/loader/SubresourceLoader.cpp:587
> +        updateRequestReferrer(newRequest, m_frame.get()->document()->referrerPolicy(), outgoingReferrer);

Hopefully we cannot have referrerPolicy == ReferrerPolicy::EmptyString?
Maybe we can just do ASSERT(referrerPolicy != ReferrerPolicy::EmptyString)

Ideally we could write something like:
updateReferrerPolicy(redirectResponse.httpHeaderField(HTTPHeaderName::ReferrerPolicy));
if (redirectingToNoewOrigin) {
...
}
updateRequestReferrer(newRequest, options().referrerPolicy, previoustRequest.httpReferrer());

That would mean that we would add ASSERT(options().referrerPolicy != ReferrerPolicy::EmptyString); in updateRequestReferrer

> Source/WebCore/loader/SubresourceLoader.cpp:591
>  

Can you add a https://w3c.github.io/webappsec-referrer-policy/#parse-referrer-policy-from-header as a comment?

> Source/WebCore/loader/SubresourceLoader.cpp:592
> +void SubresourceLoader::updateReferrerPolicy(const String& referrerPolicyStr)

s/referrerPolicyStr/referrerPolicyValue

> Source/WebCore/loader/SubresourceLoader.cpp:597
> +    for (auto& token : referrerTokens) {

We usually try to not allocate a Vector of Strings that we do not keep.
Maybe we can just write it as for (auto token : StringView { referrerPolicyValue.split(',') })
You can then use stripLeadingAndTrailingHTTPSpace on token.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:238
>          outgoingOrigin = SecurityOrigin::createFromString(outgoingReferrer)->toString();

We can probably try to no longer allocate a SecurityOrigin and instead use a SecurityOriginData to get the outgoingOrigin header value.
Something like SecurityOriginData::fromURL(URL { URL { }, outgoingReferrer }).toString();

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:240
> +    WebCore::updateRequestReferrer(m_resourceRequest, m_options.referrerPolicy, outgoingReferrer);

Is WebCore prefix needed?

[Daniel Bates]

Comment on attachment 336638 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:238
>>          outgoingOrigin = SecurityOrigin::createFromString(outgoingReferrer)->toString();
> 
> We can probably try to no longer allocate a SecurityOrigin and instead use a SecurityOriginData to get the outgoingOrigin header value.
> Something like SecurityOriginData::fromURL(URL { URL { }, outgoingReferrer }).toString();

I am tired and haven’t thought about the correctness of this patch or whether this suggestion will be sufficient given the context. SecurityOriginData::fromURL(URL { URL { }, X }).toString() is not equivalent to SecurityOrigin::createFromString(X)->toString() for all values X.

[youenn fablet]

(In reply to Daniel Bates from comment #23)
> Comment on attachment 336638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336638&action=review
> 
> >> Source/WebCore/loader/cache/CachedResourceRequest.cpp:238
> >>          outgoingOrigin = SecurityOrigin::createFromString(outgoingReferrer)->toString();
> > 
> > We can probably try to no longer allocate a SecurityOrigin and instead use a SecurityOriginData to get the outgoingOrigin header value.
> > Something like SecurityOriginData::fromURL(URL { URL { }, outgoingReferrer }).toString();
> 
> I am tired and haven’t thought about the correctness of this patch or
> whether this suggestion will be sufficient given the context.
> SecurityOriginData::fromURL(URL { URL { }, X }).toString() is not equivalent
> to SecurityOrigin::createFromString(X)->toString() for all values X.

Right, I take it back for now.

[youenn fablet]

I forgot to mention that, probably as follow-up patches, we might also want to handle referrer in case of redirections for:
- PingLoad
- DocumentThreadableLoader in case of same-origin credentials or preflight

[Sihui Liu]

Created attachment 336732 [details]
Patch

[Sihui Liu]

Comment on attachment 336638 [details]
Patch

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

>>> Source/WebCore/loader/SubresourceLoader.cpp:592
>>> +void SubresourceLoader::updateReferrerPolicy(const String& referrerPolicyStr)
>> 
>> Can we try and avoid the code duplication with Document::processReferrerPolicy() ?
> 
> s/referrerPolicyStr/referrerPolicyValue

Document::processReferrerPolicy() is similar except that updateReferrerPolicy can take in a list of values. I can try writing a util function but I am not sure where(which file) should it be. Do you have any suggestion?

[Chris Dumez]

(In reply to Sihui Liu from comment #27)
> Comment on attachment 336638 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336638&action=review
> 
> >>> Source/WebCore/loader/SubresourceLoader.cpp:592
> >>> +void SubresourceLoader::updateReferrerPolicy(const String& referrerPolicyStr)
> >> 
> >> Can we try and avoid the code duplication with Document::processReferrerPolicy() ?
> > 
> > s/referrerPolicyStr/referrerPolicyValue
> 
> Document::processReferrerPolicy() is similar except that
> updateReferrerPolicy can take in a list of values. I can try writing a util
> function but I am not sure where(which file) should it be. Do you have any
> suggestion?

Assuming it is a utility function that converts a String into a ReferrerPolicy, I would add this utility function to ReferrerPolicy.h. You'd need to create a new ReferrerPolicy.cpp for the implementation.

[Chris Dumez]

Comment on attachment 336732 [details]
Patch

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

> Source/WebCore/loader/SubresourceLoader.cpp:585
> +void SubresourceLoader::updateReferrerPolicy(const String& referrerPolicyValue)

Please avoid code duplication with Document.cpp. We likely want a utility function in ReferrerPolicy.h which converts a StringView into a ReferrerPolicy enum value.

[Sihui Liu]

Created attachment 336743 [details]
Patch

[Chris Dumez]

Comment on attachment 336743 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Add Referer head back in redirection check.

I do not understand this sentence.

> Source/WebCore/dom/Document.cpp:3464
> +    ReferrerPolicy referrerPolicy= toReferrerPolicy(policy, ReferrerPolicyDeliveryMethod::Meta);

Missing space before =.

Also, we could use:
auto referrerPolicy = ..

> Source/WebCore/loader/CrossOriginAccessControl.cpp:37
> +#include "SecurityPolicy.h"

Unnecessary include?

> Source/WebCore/loader/ResourceLoader.h:164
> +    ReferrerPolicy getReferrerPolicy() { return m_options.referrerPolicy; }

We do not use "get" prefix for getters in WebKit. This should be named referrerPolicy(). Also, this method should be const.

> Source/WebCore/loader/SubresourceLoader.cpp:579
> +    String outgoingReferrer = previousRequest.hasHTTPReferrer() ? previousRequest.httpReferrer() : emptyString();

Why emptyString() and not String()?

> Source/WebCore/loader/SubresourceLoader.cpp:592
> +    Vector<String> referrerTokens;

This is unused.

> Source/WebCore/loader/SubresourceLoader.cpp:593
> +    referrerPolicyValue.split(",", false, referrerTokens);

Seems unnecessary since referrerTokens is unused.

> Source/WebCore/loader/SubresourceLoader.cpp:595
> +        const String token = stripLeadingAndTrailingHTTPSpaces(tokenView).toString();

We do not need toString() here, this is inefficient. a StringView is sufficient to convert to a ReferrerPolicy.
auto token = stripLeadingAndTrailingHTTPSpaces(tokenView);

> Source/WebCore/loader/SubresourceLoader.h:32
> +#include "ReferrerPolicy.h"

Unnecessary include?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:236
> +    if (!m_resourceRequest.httpReferrer().isNull()) {

if (m_resourceRequest.hasHTTPReferrer())

> Source/WebCore/platform/ReferrerPolicy.cpp:34
> +

This is not building on the bots. Looks like you're missing some includes here.

> Source/WebCore/platform/ReferrerPolicy.cpp:37
> +ReferrerPolicy toReferrerPolicy(const String& policy, ReferrerPolicyDeliveryMethod method)

policy should be a StringView here for performance. This way, calls site that have a StringView (by value) do not need to construct a String:
ReferrerPolicy toReferrerPolicy(StringView policy, ReferrerPolicyDeliveryMethod method)

Although I would have preferred parseReferrerPolicy as naming.

> Source/WebCore/platform/ReferrerPolicy.cpp:39
> +    // "never" / "default" / "always" are legacy keywords that we will support. They were defined in:

s/we will support/we support/

> Source/WebCore/platform/ReferrerPolicy.cpp:41
> +    ReferrerPolicy referrerPolicy = ReferrerPolicy::EmptyString;

I do not think we need this local variable. We can just use early returns.

> Source/WebCore/platform/ReferrerPolicy.cpp:45
> +            referrerPolicy = ReferrerPolicy::NoReferrer;

return ReferrerPolicy::NoReferrer;

> Source/WebCore/platform/ReferrerPolicy.cpp:53
> +        referrerPolicy = ReferrerPolicy::NoReferrer;

ReferrerPolicy::NoReferrer;

> Source/WebCore/platform/ReferrerPolicy.cpp:54
> +    else if (equalLettersIgnoringASCIICase(policy, "unsafe-url"))

then "if" instead of "else if" here.

> Source/WebCore/platform/ReferrerPolicy.cpp:68
> +    

if (policy == "")
    return ReferrerPolicy::EmptyString;

> Source/WebCore/platform/ReferrerPolicy.cpp:69
> +    return referrerPolicy;

return std::nullopt;

> Source/WebCore/platform/ReferrerPolicy.h:51
> +enum class ReferrerPolicyDeliveryMethod {

How about ShouldParseLegacyKeywords { No, Yes }; ?

> Source/WebCore/platform/ReferrerPolicy.h:56
> +ReferrerPolicy toReferrerPolicy(const String&, ReferrerPolicyDeliveryMethod);

I think this should return a std::optional<ReferrerPolicy> and return std::nullopt when the keyword cannot be parsed. Note that the empty string is valid referrerPolicy and the specification distinguishes the empty string and no valid token.

[Sihui Liu]

Created attachment 336813 [details]
Patch

[Chris Dumez]

Comment on attachment 336813 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:3464
> +    auto referrerPolicy = toReferrerPolicy(StringView(policy), ShouldParseLegacyKeywords::Yes);

StringView(const String&); is not explicit so you don't need StringView() here.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:37
> +#include "SecurityPolicy.h"

Why is this needed?

> Source/WebCore/loader/SubresourceLoader.cpp:594
> +        auto referrerPolicyToken = toReferrerPolicy(token, ShouldParseLegacyKeywords::No);

This could go in the if condition:
if (auto referrerPolicyToken = toReferrerPolicy(token, ShouldParseLegacyKeywords::No)

> Source/WebCore/loader/SubresourceLoader.cpp:595
> +        if (referrerPolicyToken)

Step 3: For each token in policy-tokens, if token is a referrer policy and token is not the empty string, then set policy to token.

So I believe this should be:
if (referrerPolicyToken && *referrerPolicy != ReferrerPolicy::EmptyString)

otherwise, emptyString may overwrite a valid token that preceded it.

> Source/WebCore/platform/ReferrerPolicy.cpp:37
> +std::optional<ReferrerPolicy> toReferrerPolicy(StringView policy, ShouldParseLegacyKeywords shouldParseLegacy)

s/shouldParseLegacy/shouldParseLegacyKeywords/

> Source/WebCore/platform/ReferrerPolicy.cpp:66
> +    if (policy.isEmpty())

I do not believe this is correct. isEmpty() return true for both a null StringView and a StringView containing the empty string. We only want to match the latter. a null StringView() should return std::nullopt, not ReferrerPolicy::EmptyString IMO.

[youenn fablet]

Comment on attachment 336813 [details]
Patch

Patch looks pretty great already.
Some additional nits below.
Most important thing might be to fix other port builds.

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

> Source/WebCore/loader/ResourceLoader.h:164
> +    ReferrerPolicy referrerPolicy() const { return m_options.referrerPolicy; }

This one is probably not strictly needed but does not really hurt either.

> Source/WebCore/loader/SubresourceLoader.cpp:579
> +    String outgoingReferrer = previousRequest.hasHTTPReferrer() ? previousRequest.httpReferrer() : String();

Why do we need to check for previousRequest.hasHTTPReferrer?
Can we just write it as:
updateRequestReferrer(newRequest, referrerPolicy(), previousRequest.httpReferrer());

> Source/WebCore/platform/ReferrerPolicy.cpp:1
> +/*

Since it is a new file, you will need to add it to Source/WebCore/Sources.txt and do not add it as a target in Xcode project.

> Source/WebCore/platform/ReferrerPolicy.h:54
> +std::optional<ReferrerPolicy> toReferrerPolicy(StringView, ShouldParseLegacyKeywords);

Could probably be a const StringView&

[Sihui Liu]

Created attachment 336824 [details]
Patch

[Chris Dumez]

(In reply to youenn fablet from comment #34)
> Comment on attachment 336813 [details]
> Patch
> 
> Patch looks pretty great already.
> Some additional nits below.
> Most important thing might be to fix other port builds.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336813&action=review
> 
> > Source/WebCore/loader/ResourceLoader.h:164
> > +    ReferrerPolicy referrerPolicy() const { return m_options.referrerPolicy; }
> 
> This one is probably not strictly needed but does not really hurt either.
> 
> > Source/WebCore/loader/SubresourceLoader.cpp:579
> > +    String outgoingReferrer = previousRequest.hasHTTPReferrer() ? previousRequest.httpReferrer() : String();
> 
> Why do we need to check for previousRequest.hasHTTPReferrer?
> Can we just write it as:
> updateRequestReferrer(newRequest, referrerPolicy(),
> previousRequest.httpReferrer());
> 
> > Source/WebCore/platform/ReferrerPolicy.cpp:1
> > +/*
> 
> Since it is a new file, you will need to add it to
> Source/WebCore/Sources.txt and do not add it as a target in Xcode project.
> 
> > Source/WebCore/platform/ReferrerPolicy.h:54
> > +std::optional<ReferrerPolicy> toReferrerPolicy(StringView, ShouldParseLegacyKeywords);
> 
> Could probably be a const StringView&

No, the policy is to pass StringView by value. Sihui ‘s code is correct.

[youenn fablet]

Comment on attachment 336824 [details]
Patch

LGTM

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

> Source/WebCore/loader/SubresourceLoader.cpp:575
>          cleanHTTPRequestHeadersForAccessControl(newRequest);

As a follow-up, it might be good to update cleanHTTPRequestHeadersForAccessControl to no longer strip out Referrer.
There is another call site in ServiceWorker where we can explicitly clear the referrer since it is passed as an independent parameter.
Then we may be able to call updateReferrerPolicy and just after call updateRequestReferrer.

> Source/WebCore/platform/ReferrerPolicy.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

Should be updated probably.

[Sihui Liu]

Created attachment 337004 [details]
Patch

[Sihui Liu]

Created attachment 337026 [details]
Patch

[Daniel Bates]

Comment on attachment 337026 [details]
Patch

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

> Source/WebCore/loader/CrossOriginAccessControl.h:30
> +#include "ReferrerPolicy.h"

It is unnecessary to include this header. It is sufficient to forward declare the enum class ReferrerPolicy in this file.

> Source/WebCore/loader/CrossOriginAccessControl.h:46
> +WEBCORE_EXPORT void updateRequestReferrer(ResourceRequest&, ReferrerPolicy, const String&);

It is unnecessary to export this function by prefixing the declaration with WEBCORE_EXPORT. This function is only used in WebCore. That is, it is not used in WebKit.

> Source/WebCore/loader/SubresourceLoader.cpp:589
> +    // Implementing https://w3c.github.io/webappsec-referrer-policy/#parse-referrer-policy-from-header.

The spec. can change from the time this code was added and the date this code is landed may not unambiguously refer to a particular draft revision. I suggest that we either get a permalink URL that includes the draft revision (like the URL in toReferrerPolicy()) or reference the date of this draft in the comment for historical preservation.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:233
> +    // Implementing step 9 to 11 of https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

Ditto.

> Source/WebCore/platform/ReferrerPolicy.cpp:25
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

This is not the correct license block for an Apple contribution. Assuming this code did not originate from some other organization (please check) then the Apple license block template is in <https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/LICENSE-APPLE>. If this code did originate from some other organization then please use the license block from Document.cpp adjusted as appropriate to reflect the original copyright holder(s) and Apple.

[Sihui Liu]

Created attachment 337040 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 337040 [details]
Patch

Clearing flags on attachment: 337040

Committed r230208: <https://trac.webkit.org/changeset/230208>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

This is still an issue for AppleWebKit/605.1.15. Is this fix a part of that release?

[Alexey Proskuryakov]

It is not part of any shipping release yet.

Do you have an estimate on when it will be added?

[youenn fablet]

Samuel, would you be able to check Safari Tech Preview and/or recent Safari 12 betas?

Safari 12 beta is setting the Referer header.