Bug 185807 - REGRESSION (r231107): Test http/tests/quicklook/same-origin-xmlhttprequest-allowed.html logs CSP failure
Summary: REGRESSION (r231107): Test http/tests/quicklook/same-origin-xmlhttprequest-al...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: iPhone / iPad iOS 11
: P1 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar, Regression
Depends on: 184741
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-20 13:49 PDT by Daniel Bates
Modified: 2018-06-01 12:22 PDT (History)
7 users (show)

See Also:


Attachments
Special casing quick look (5.06 KB, patch)
2018-05-30 10:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.