|Summary:||[WK2] Authentication dialog is displayed for cross-origin XHR|
|Product:||WebKit||Reporter:||Alexey Proskuryakov <ap>|
|Component:||Page Loading||Assignee:||youenn fablet <youennf>|
|Severity:||Normal||CC:||beidson, commit-queue, japhet, youennf|
|Version:||528+ (Nightly build)|
Description Alexey Proskuryakov 2014-04-08 00:17:25 PDT
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
Comment 1 Alexey Proskuryakov 2014-04-08 00:21:29 PDT
Created attachment 228814 [details] proposed fix
Comment 2 Alexey Proskuryakov 2014-04-08 00:21:45 PDT
Comment on attachment 228814 [details] proposed fix Sorry, wrong bug.
Comment 4 Alexey Proskuryakov 2014-09-04 22:57:39 PDT
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.
Comment 5 youenn fablet 2014-09-05 07:27:11 PDT
Created attachment 237689 [details] Patch based on passing origin from ResourceLoader to NetworkResourceLoader
Comment 6 Alexey Proskuryakov 2014-09-05 18:50:02 PDT
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.
Comment 7 youenn fablet 2014-09-08 09:52:32 PDT
(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?
Comment 8 Brady Eidson 2014-09-08 10:08:06 PDT
(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.
Comment 9 youenn fablet 2014-09-09 05:44:12 PDT
Created attachment 237847 [details] Precomputing credential request policy in WebProcess
Comment 10 Alexey Proskuryakov 2014-09-09 12:54:41 PDT
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?
Comment 11 youenn fablet 2014-09-11 00:03:33 PDT
Created attachment 237938 [details] Updated according review
Comment 12 youenn fablet 2014-09-11 03:03:16 PDT
Created attachment 237949 [details] Updated according review
Comment 13 WebKit Commit Bot 2014-09-11 10:20:26 PDT
Comment on attachment 237949 [details] Updated according review Clearing flags on attachment: 237949 Committed r173516: <http://trac.webkit.org/changeset/173516>
Comment 14 WebKit Commit Bot 2014-09-11 10:20:29 PDT
All reviewed patches have been landed. Closing bug.