Bug 199232 - SubFrameSOAuthorizationSession should preserve the referrer header when falling back to web path
Summary: SubFrameSOAuthorizationSession should preserve the referrer header when falli...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-26 13:28 PDT by Jiewen Tan
Modified: 2019-06-26 23:33 PDT (History)
7 users (show)

See Also:


Attachments
Patch (14.27 KB, patch)
2019-06-26 13:48 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (12.36 KB, patch)
2019-06-26 16:14 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-06-26 13:28:36 PDT
SubFrameSOAuthorizationSession should preserve the referrer header when fall back to web path.
Comment 1 Jiewen Tan 2019-06-26 13:29:23 PDT
<rdar://problem/51718328>
Comment 2 Jiewen Tan 2019-06-26 13:48:50 PDT
Created attachment 372947 [details]
Patch
Comment 3 Chris Dumez 2019-06-26 13:51:51 PDT
Comment on attachment 372947 [details]
Patch

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

> Source/WebKit/UIProcess/WebFrameProxy.cpp:82
> +void WebFrameProxy::loadURL(const URL& url, const Optional<String>& referrer)

We normally do not use Optional<String> because the String class already has a null state.
Comment 4 Chris Dumez 2019-06-26 13:53:35 PDT
Comment on attachment 372947 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
> +                if (!navigationActionPtr->request().hasHTTPReferrer()) {

This if check would not be needed if you did not use Optional. navigationActionPtr->request().httpReferrer() would return a null String and the normal code path would do the right thing.
Comment 5 Chris Dumez 2019-06-26 15:59:40 PDT
Comment on attachment 372947 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1497
> +    if (!referrer) {

Ditto, this if check is likely not needed either if you stop using Optional<String> and use a simple String instead.
Comment 6 Jiewen Tan 2019-06-26 16:07:27 PDT
Comment on attachment 372947 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.h:123
> +    WEBCORE_EXPORT bool hasHTTPReferrer() const;

Removed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
>> +                if (!navigationActionPtr->request().hasHTTPReferrer()) {
> 
> This if check would not be needed if you did not use Optional. navigationActionPtr->request().httpReferrer() would return a null String and the normal code path would do the right thing.

It does!

>> Source/WebKit/UIProcess/WebFrameProxy.cpp:82
>> +void WebFrameProxy::loadURL(const URL& url, const Optional<String>& referrer)
> 
> We normally do not use Optional<String> because the String class already has a null state.

Fixed.
Comment 7 Jiewen Tan 2019-06-26 16:08:01 PDT
Comment on attachment 372947 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1497
>> +    if (!referrer) {
> 
> Ditto, this if check is likely not needed either if you stop using Optional<String> and use a simple String instead.

Removed.
Comment 8 Jiewen Tan 2019-06-26 16:14:51 PDT
Created attachment 372962 [details]
Patch
Comment 9 youenn fablet 2019-06-26 21:42:41 PDT
Comment on attachment 372962 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
> +                frame->loadURL(navigationActionPtr->request().url(), navigationActionPtr->request().httpReferrer());

I wonder whether other parameters might be needed in the future (cache policy, user agent...).
Maybe we should pass navigationActionPtr->request() directly.
Comment 10 Jiewen Tan 2019-06-26 23:02:52 PDT
Comment on attachment 372962 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SubFrameSOAuthorizationSession.mm:71
>> +                frame->loadURL(navigationActionPtr->request().url(), navigationActionPtr->request().httpReferrer());
> 
> I wonder whether other parameters might be needed in the future (cache policy, user agent...).
> Maybe we should pass navigationActionPtr->request() directly.

Thanks, Youenn. That’s in my mind as well. However, I’m not sure if there will be any side effects to load the same request again. We will find out later if it is needed.
Comment 11 WebKit Commit Bot 2019-06-26 23:33:56 PDT
Comment on attachment 372962 [details]
Patch

Clearing flags on attachment: 372962

Committed r246872: <https://trac.webkit.org/changeset/246872>
Comment 12 WebKit Commit Bot 2019-06-26 23:33:58 PDT
All reviewed patches have been landed.  Closing bug.