[WK2] Authentication dialog is displayed for cross-origin XHR

[Alexey Proskuryakov]

Authentication dialog is displayed for cross-origin XHR, which breaks three regression tests, and is just the wrong behavior.

http/tests/xmlhttprequest/access-control-preflight-credential-async.html
http/tests/xmlhttprequest/cross-origin-no-authorization.html
http/tests/xmlhttprequest/cross-origin-no-credential-prompt.html

[Alexey Proskuryakov]

Created attachment 228814 [details]
proposed fix

[Alexey Proskuryakov]

Comment on attachment 228814 [details]
proposed fix

Sorry, wrong bug.

[youenn fablet]

Created attachment 232423 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 232423 [details]
Patch

This is a wrong level for cross origin logic. ResourceRequest is a low level networking class that may not know about web concepts.

[youenn fablet]

Created attachment 237689 [details]
Patch based on passing origin from ResourceLoader to NetworkResourceLoader

[Alexey Proskuryakov]

Comment on attachment 237689 [details]
Patch based on passing origin from ResourceLoader to NetworkResourceLoader

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

> Source/WebCore/ChangeLog:2
> +        [WK2] Authentication dialog is displayed for cross-origin XHR

Do you know why this is a problem for WK2, but not for WK1?

> Source/WebCore/loader/ResourceLoader.cpp:543
> +    return m_frame->document() ? m_frame->document()->securityOrigin() : nullptr;

When can this check fail? We'd need to do loading without a document.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:108
> +    m_origin = SecurityOrigin::createFromString(parameters.origin);

SecurityOrigin's behavior depends on static data (e.g. SecurityPolicy::isAccessWhiteListed). This doesn't work in NetworkProcess, so we can't achieve 100% correctness with any code that does this.

[youenn fablet]

(In reply to comment #6)
> (From update of attachment 237689 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237689&action=review
> 
> > Source/WebCore/ChangeLog:2
> > +        [WK2] Authentication dialog is displayed for cross-origin XHR
> 
> Do you know why this is a problem for WK2, but not for WK1?

In WK1, the auth challenge is going from the network layer to webcore and then to the UI. WebCore::ResourceLoader is responsible of requesting the auth dialog or not.
In WK2, the NetworkProcess is directly asking the UI process for the auth dialog, without going through WebProcess/ResourceLoader.

> > Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:108
> > +    m_origin = SecurityOrigin::createFromString(parameters.origin);
> 
> SecurityOrigin's behavior depends on static data (e.g. SecurityPolicy::isAccessWhiteListed). This doesn't work in NetworkProcess, so we can't achieve 100% correctness with any code that does this.

I see...
I was thinking that having origin may have helped NetworkResourceLoader, for instance for sync request handling. This will obviously not work...

One option is to make a IPC NetworkProcess -> WebProcess sync call to retrieve ResourceLoader decision.
This only works in async case which seems ok for now as requesting credentials to the user for sync requests is not supported IIUC.

A second alternative is to set (creation time) and update (in case of redirection) NetworkResourceLoader::m_clientCredentialPolicy to be either DoNotAskClientForAnyCredentials or AskClientForAllCredentials.

Any thoughts?

[Brady Eidson]

(In reply to comment #7)
> One option is to make a IPC NetworkProcess -> WebProcess sync call to retrieve ResourceLoader decision.

We're not going to do this.

The bar for adding a sync message call is very high, and this doesn't qualify.

[youenn fablet]

Created attachment 237847 [details]
Precomputing credential request policy in WebProcess

[Alexey Proskuryakov]

Comment on attachment 237847 [details]
Precomputing credential request policy in WebProcess

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

Looks good to me. Please address the comments before landing.

> Source/WebCore/loader/ResourceLoader.cpp:541
> +bool ResourceLoader::canAskUserForCredentials() const

isAllowedToAskClientForCredentials would be a more accurate name:

1. "can" is ambiguous, one could misunderstand it and decide that we are talking about ability, not about permission.
2. It's about asking the client, which may or may not present UI to the user (e.g. WebKitTestRunner does not).

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:388
> +    // The Web Process is precomputing client credential policy, hence below ASSERT.

I'd say:

// NetworkResourceLoader does not know whether the request is cross origin, so Web process computes an applicable credential policy for it.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:92
> +    // TODO: check whether we need to update NetworkResourceLoader clientCredentialPolicy in case loader policy is DoNotAskClientForCrossOriginCredentials.

s/TODO/FIXME/

I'd make an actual question out of it:

// FIXME: Do we need to update NetworkResourceLoader clientCredentialPolicy in case loader policy is DoNotAskClientForCrossOriginCredentials?

[youenn fablet]

Created attachment 237938 [details]
Updated according review

[youenn fablet]

Created attachment 237949 [details]
Updated according review

[WebKit Commit Bot]

Comment on attachment 237949 [details]
Updated according review

Clearing flags on attachment: 237949

Committed r173516: <http://trac.webkit.org/changeset/173516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.