Bug 186202

Created attachment 341776 [details]
For EWS

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

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

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

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

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

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

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

> 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 on attachment 341776 [details]
For EWS

r=me