Bug 186202 – [QuickLook] Add a test to ensure that a same-origin XHR for a non-existent QuickLook attachment is allowed

• Status: ASSIGNED
• Component: Tools / Tests

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 341776 [details]
For EWS

[Daniel Bates]

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();
})();

[youenn fablet]

*** Bug 186165 has been marked as a duplicate of this bug. ***

[youenn fablet]

(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.

[youenn fablet]

(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?

[Daniel Bates]

(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.

[Andy Estes]

(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.

[youenn fablet]

> 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?

[youenn fablet]

> 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:".

[Brent Fulgham]

Comment on attachment 341776 [details]
For EWS

r=me