Bug 186202 - [QuickLook] Add a test to ensure that a same-origin XHR for a non-existent QuickLook attachment is allowed
Summary: [QuickLook] Add a test to ensure that a same-origin XHR for a non-existent Qu...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-01 11:59 PDT by Daniel Bates
Modified: 2018-06-29 16:38 PDT (History)
4 users (show)

See Also:


Attachments
For EWS (5.30 KB, patch)
2018-06-01 12:02 PDT, Daniel Bates
bfulgham: review+
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-06-01 11:59:36 PDT
Add a test to ensure that a same-origin XHR for a non-existent QuickLook attachment is allowed. That is, it is not blocked by the CSP policy of the QuickLook sandbox.
Comment 1 Daniel Bates 2018-06-01 12:02:40 PDT
Created attachment 341776 [details]
For EWS
Comment 2 Daniel Bates 2018-06-01 12:03:59 PDT
For convenience, here is the pretty-printed JavaScript URL embedded in the RTF included in the proposed patch (attachment #341776 [details]):

javascript: (function() {
    function logMessageAndDone(message) {
        console.log(message);
        window.testRunner && window.testRunner.notifyDone();
    };
    var xhr = new XMLHttpRequest;
    xhr.onload = () => logMessageAndDone("PASS: XMLHttpRequest allowed.");
    xhr.onerror = () => logMessageAndDone("FAIL: XMLHttpRequest blocked.");
    xhr.open("GET", document.origin + "/x-apple-ql-magic/non-existent-quicklook-attachment.rtf");
    xhr.send();
})();
Comment 3 youenn fablet 2018-06-04 08:42:11 PDT
*** Bug 186165 has been marked as a duplicate of this bug. ***
Comment 4 youenn fablet 2018-06-04 08:50:20 PDT
(In reply to youenn fablet from comment #3)
> *** Bug 186165 has been marked as a duplicate of this bug. ***
Made a mistake there in marking as duplicate.
Comment 5 youenn fablet 2018-06-04 08:50:44 PDT
(In reply to Daniel Bates from comment #0)
> Add a test to ensure that a same-origin XHR for a non-existent QuickLook
> attachment is allowed. That is, it is not blocked by the CSP policy of the
> QuickLook sandbox.

I am not sure to understand.
After https://trac.webkit.org/changeset/231107, we make this load fails.
After discussion, it seems to me that we are ok with this change of behavior.
If so, shouldn't we make the test to expect failure and rename it accordingly?
Comment 6 Daniel Bates 2018-06-04 09:41:21 PDT
(In reply to youenn fablet from comment #5)
> (In reply to Daniel Bates from comment #0)
> > Add a test to ensure that a same-origin XHR for a non-existent QuickLook
> > attachment is allowed. That is, it is not blocked by the CSP policy of the
> > QuickLook sandbox.
> 
> I am not sure to understand.
> After https://trac.webkit.org/changeset/231107, we make this load fails.
> After discussion, it seems to me that we are ok with this change of behavior.
> If so, shouldn't we make the test to expect failure and rename it
> accordingly?

I did not have the impression that blocking the load is an acceptable change of behavior.
Comment 7 Andy Estes 2018-06-04 16:42:45 PDT
(In reply to Daniel Bates from comment #6)
> (In reply to youenn fablet from comment #5)
> > (In reply to Daniel Bates from comment #0)
> > > Add a test to ensure that a same-origin XHR for a non-existent QuickLook
> > > attachment is allowed. That is, it is not blocked by the CSP policy of the
> > > QuickLook sandbox.
> > 
> > I am not sure to understand.
> > After https://trac.webkit.org/changeset/231107, we make this load fails.
> > After discussion, it seems to me that we are ok with this change of behavior.
> > If so, shouldn't we make the test to expect failure and rename it
> > accordingly?
> 
> I did not have the impression that blocking the load is an acceptable change
> of behavior.

The intent of Quick Look converting invalid x-apple-ql-id: URLs to about: is to block the load, so I think this behavior change is acceptable, although ideally we'd retain the original behavior of successfully loading about:.

It would be unacceptable if *valid* x-apple-ql-id: URLs were blocked, of course.
Comment 8 youenn fablet 2018-06-04 19:07:58 PDT
> The intent of Quick Look converting invalid x-apple-ql-id: URLs to about: is
> to block the load, so I think this behavior change is acceptable, although
> ideally we'd retain the original behavior of successfully loading about:.

I am not sure to see the benefit of keeping the original behavior.
Isn't it better to stop failing silently if invalid x-apple-ql-id means a bug in the generation of these URLs?
Comment 9 youenn fablet 2018-06-06 08:55:21 PDT
> I am not sure to see the benefit of keeping the original behavior.
> Isn't it better to stop failing silently if invalid x-apple-ql-id means a
> bug in the generation of these URLs?

Instead of relying on CSP/CORS to fail the load, we could be cancelling the load in ResourceLoader with a comprehensive message when the request URL is converted to "about:".
Comment 10 Brent Fulgham 2018-06-29 16:38:01 PDT
Comment on attachment 341776 [details]
For EWS

r=me