Bug 185807

Summary: REGRESSION (r231107): Test http/tests/quicklook/same-origin-xmlhttprequest-allowed.html logs CSP failure
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: youenn fablet <youennf>
Status: REOPENED ---    
Severity: Normal CC: aestes, cdumez, ews-watchlist, japhet, lforschler, webkit-bug-importer, youennf
Priority: P1 Keywords: InRadar, Regression
Version: WebKit Local Build   
Hardware: iPhone / iPad   
OS: iOS 11   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186165
https://bugs.webkit.org/show_bug.cgi?id=186202
Bug Depends on: 184741    
Bug Blocks:    
Attachments:
Description Flags
Special casing quick look none

Description Daniel Bates 2018-05-20 13:49:20 PDT
Before changeset <https://trac.webkit.org/changeset/231107/> (bug #184741) the expected result for test http/tests/quicklook/same-origin-xmlhttprequest-allowed.html was:

[[
CONSOLE MESSAGE: line 1: PASS: XMLHttpRequest allowed


--------
Frame: '<!--framePath //<!--frame0-->-->'
--------
Run test
]]
<https://trac.webkit.org/browser/webkit/trunk/LayoutTests/http/tests/quicklook/same-origin-xmlhttprequest-allowed-expected.txt?rev=214189&format=txt>

After changeset r231107 the result of this test changed (and unfortunately these new results were committed):

[[
CONSOLE MESSAGE: Blocked by Content Security Policy
CONSOLE MESSAGE: XMLHttpRequest cannot load about: due to access control checks.
CONSOLE MESSAGE: line 1: PASS: XMLHttpRequest allowed


--------
Frame: '<!--framePath //<!--frame0-->-->'
--------
Run test
]]
<https://trac.webkit.org/browser/webkit/trunk/LayoutTests/http/tests/quicklook/same-origin-xmlhttprequest-allowed-expected.txt?rev=231107&format=txt>
Comment 1 Radar WebKit Bug Importer 2018-05-20 13:54:20 PDT
<rdar://problem/40402483>
Comment 2 youenn fablet 2018-05-30 08:23:15 PDT
The test is trying to load a quick look URL using XHR.
In that particular case, ResourceLoader::willSendRequestInternal will call previewConverter->safeRequest. The request URL is then updated and becomes "about:".

Before moving the checks to NetworkProcess, DocumentThreadableLoader was not doing any further check since the request URL was the quick look URL and not "about:", thus the request is considered same-origin.

Now that checks are done in NetworkProcess, "about:" is used for the checks.
For this particular quick look + "about:" URL, it is easy to fix it in WebLoaderStrategy by disabling network checks in that particular case.

The same issue might arise if an injected bundle decides to change the request URL on the fly.
Comment 3 youenn fablet 2018-05-30 10:25:03 PDT
Created attachment 341580 [details]
Special casing quick look
Comment 4 youenn fablet 2018-05-30 11:56:15 PDT
Uploaded a patch that is fixing iOS-sim to the previous behavior for the given test.
Except if we end up in big compatibility issues, I am not sure we should actually try to solve the issue where some layer (quick look, injected bundles) below our previous security checks are changing the request (URL or headers).
Comment 5 youenn fablet 2018-05-31 15:38:46 PDT
I talked with andy about it.
The URL is converted from a quicklook URL to about: because it is not pointing to a valid resource. It is fine to keep the current behavior for now and make the load fails.

That said, we are no longer checking that CORS checks are passing in Network Process in case of a valid quick look URL.
We should add a test for that case.
I will file a separate bug entry for this particular issue and mark this one as WontFix.
Comment 6 Daniel Bates 2018-06-01 12:08:58 PDT
Re-opening this bug to update the test to actually check if the load was allowed or blocked and add a FIXME comment to determine a valid QuickLook attachment URL to use when making the XHR request. Currently the test considers XHR completion regardless of error as success.
Comment 7 Daniel Bates 2018-06-01 12:22:17 PDT
The test <https://trac.webkit.org/browser/webkit/trunk/LayoutTests/http/tests/quicklook/same-origin-xmlhttprequest-allowed.html> does not seem materially consistent with example given in <rdar://problem/29898214>. We need a document Q_i that when QuickLook converted has a non-empty sub-QuickLook attachment Q_i_0. Then we should modify same-origin-xmlhttprequest-allowed.html to fetch Q_i_o via XHR and ensure it loaded the contents of Q_i_0.