Bug 32398 – [Chromium] Expose SecurityOrigin::shouldHideReferrer() in chromium port api.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Nate Chapin]

Chromium is currently clearing document.referrer on redirect (http://crbug.com/7357).   To fix properly (i.e., ensure we don't set the referrer when it ought to be suppressed), we should have access to SecurityOrigin::shouldHideReferrer().

[Nate Chapin]

Created attachment 44646 [details]
patch

[WebKit Review Bot]

Attachment 44646 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebSecurityOrigin.cpp:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1

[Dmitry Titov]

Comment on attachment 44646 [details]
patch

A few style nits:

> Index: WebKit/chromium/src/WebSecurityOrigin.cpp
> +bool WebSecurityOrigin::shouldHideReferrer(const WebURL& url, const WebString& referrer) {

'}' should go on the next line as bot said ^^^

> +    return SecurityOrigin::shouldHideReferrer(WebCore::KURL(url.spec(), url.parsed(), url.isValid()), 

You could simply pass WebURL into SecurityOrigin::shouldHideReferer since WebURL has conversion operator to convert to KURL.

> +        WebCore::String(referrer));

WebKit code usually does not wrap a long line like this.

> Index: WebKit/chromium/public/WebSecurityOrigin.h

> +    // Returns whether the url should be allowed to see the referrer
> +    // based on their respective protocols.
> +    WEBKIT_API static bool shouldHideReferrer(const WebURL& url, 
> +        const WebString& referrer);

No need to wrap lines like this.

[Nate Chapin]

(In reply to comment #3)
> (From update of attachment 44646 [details])
> A few style nits:
> 
> > Index: WebKit/chromium/src/WebSecurityOrigin.cpp
> > +bool WebSecurityOrigin::shouldHideReferrer(const WebURL& url, const WebString& referrer) {
> 
> '}' should go on the next line as bot said ^^^
> 
> > +    return SecurityOrigin::shouldHideReferrer(WebCore::KURL(url.spec(), url.parsed(), url.isValid()), 
> 
> You could simply pass WebURL into SecurityOrigin::shouldHideReferer since
> WebURL has conversion operator to convert to KURL.

Are you sure about this one?  If I just try to pass the WebURL into SecurityOrigin::shouldHideReferrer(), I'm getting compile errors (claims it can't do the conversion).

> 
> > +        WebCore::String(referrer));
> 
> WebKit code usually does not wrap a long line like this.
> 
> > Index: WebKit/chromium/public/WebSecurityOrigin.h
> 
> > +    // Returns whether the url should be allowed to see the referrer
> > +    // based on their respective protocols.
> > +    WEBKIT_API static bool shouldHideReferrer(const WebURL& url, 
> > +        const WebString& referrer);
> 
> No need to wrap lines like this.

[Nate Chapin]

Created attachment 44695 [details]
patch2

Never mind, I got it.  All your comments should be resolved.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44695 [details] without any errors.

[Darin Fisher (:fishd, Google)]

Comment on attachment 44695 [details]
patch2

> Index: WebKit/chromium/public/WebSecurityOrigin.h
...
> +    // Returns whether the url should be allowed to see the referrer
> +    // based on their respective protocols.
> +    WEBKIT_API static bool shouldHideReferrer(const WebURL& url, const WebString& referrer);

Even though WebCore uses String for referrer, please use a WebURL here
because it is in fact supposed to be an URL.  The WebKit API tries to
use WebURL everywhere it is true that a parameter is an URL.

[Darin Fisher (:fishd, Google)]

Also, I think this method would be better on WebSecurityPolicy.  The WebKit API has policy related stuff on that interface instead of lumping them on WebSecurityOrigin.

[Nate Chapin]

Created attachment 44700 [details]
patch3

fishd and I discussed it off thread and concluded that it makes sense to leave the referrer as a WebString.

This patch moves the change to WebSecurityPolicy, as requested.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44700 [details] without any errors.

[Adam Barth]

> fishd and I discussed it off thread and concluded that it makes sense to leave
> the referrer as a WebString.

Just so I know for the future, what was the reasoning here?

[Nate Chapin]

(In reply to comment #11)
> > fishd and I discussed it off thread and concluded that it makes sense to leave
> > the referrer as a WebString.
> 
> Just so I know for the future, what was the reasoning here?

As I understood it, the rationale was that in WebHistoryItem and WebURLRequest (two of the places that store referrers), we are returning the referrer as a WebString, and it didn't make sense to do a WebString->WebUrl->WebString->WebCore::String translation, particularly since translating between WebString and WebURL is not well supported at this time.

fishd, feel free to correct me if I misundestood the logic. :-)

[Darin Fisher (:fishd, Google)]

> fishd, feel free to correct me if I misundestood the logic. :-)

yeah, that's basically it.  in general, i would prefer to pass around anything that is an url as a {Web,K,G}URL, but in this case WebString seems like the more practical choice.

[Nate Chapin]

(In reply to comment #13)
> > fishd, feel free to correct me if I misundestood the logic. :-)
> 
> yeah, that's basically it.  in general, i would prefer to pass around anything
> that is an url as a {Web,K,G}URL, but in this case WebString seems like the
> more practical choice.

http://trac.webkit.org/changeset/52027