Changeset 211248 in webkit


Ignore:
Timestamp:
Jan 26, 2017 6:00:17 PM (7 years ago)
Author:
aestes@apple.com
Message:

[QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id: URLs
​https://bugs.webkit.org/show_bug.cgi?id=167453

Reviewed by Brent Fulgham.

Source/WebCore:

Requests to x-apple-ql-id: URLs should be filtered by
-[QLPreviewConverter safeRequestForRequest:]. This method checks that the URL is one of the
ones generated for the current preview, and changes it to "about:" if it isn't.

WebCore::safeQLURLForDocumentURLAndResourceURL() was responsible for finding the
QLPreviewConverter instance to use by looking it up in an NSMutableDictionary using the
document URL as a key. In WebKit1, this dictionary was populated by the
QuickLookHandleClient when new QuickLookHandles were created, but the WebKit2
QuickLookHandleClient never did this. As a result, requests to invalid URLs were not being
rewritten.

An easy way to load a QuickLook document with invalid URLs is to create an HTML file with a
Microsoft Office extension (e.g. .xls); QuickLook, iWork, and Office support opening HTML
files with Office document extensions. In r207155 we applied a Content Security Policy to
QuickLook documents that only allows x-apple-ql-id: resources to load. This blocked
cross-origin requests from loading, but same-origin requests to URLs that weren't generated
by QLPreviewConverter were still allowed to load.

This change blocks these URLs by calling -[QLPreviewConverter safeRequestForRequest:] in a
way that works for both WebKit1 and WebKit2.

After implementing QuickLook for WebKit2, we found a bug when loading HTML-as-Office
documents (webkit.org/b/135651) that presented as a nil MIME type in the preview
NSURLResponse returned by QLPreviewConverter. Unfortunately r172159 papered over the actual
bug by failing to load previews with nil MIME types.

The real issue was that we were asking for the preview response before QuickLook had
received enough data to determine a MIME type, so this change also removes the bad fix from
r172159 and instead waits until QuickLook has converted the document to ask for its preview
response. This restores the ability to load HTML files with Office document extensions.
These two fixes are combined in a single patch because I don't know how to create an invalid
QuickLook URL for testing without loading an HTML-as-Office document.

Test: quicklook/invalid-ql-id-url.html

  • loader/ResourceLoader.cpp:

(WebCore::ResourceLoader::willSendRequestInternal): Called
QuickLookHandle::willSendRequest() if m_documentLoader has a QuickLookHandle.

  • loader/cache/CachedResource.cpp:

(WebCore::CachedResource::load): Removed the call to
WebCore::safeQLURLForDocumentURLAndResourceURL().

  • loader/ios/QuickLook.h: Removed safeQLURLForDocumentURLAndResourceURL() and declared

QuickLookHandle::willSendRequest().

  • loader/ios/QuickLook.mm: Removed _previewResponse and _hasFailed ivars from

WebPreviewConverter.
(-[WebPreviewConverter initWithResourceLoader:resourceResponse:quickLookHandle:]): Stopped
setting _previewResponse.
(-[WebPreviewConverter _sendDidReceiveResponseIfNecessary]): Only emptied _bufferedDataArray
if we haven't already called -_sendDidReceiveResponseIfNecessary; removed the check for a
nil _previewResponse MIME type; accessed -[QLPreviewConverter previewResponse] now that the
document has been converted and asserted its MIME type is non-nil.
(-[WebPreviewConverter connection:didReceiveData:lengthReceived:]): Removed _hasFailed check.
(-[WebPreviewConverter connectionDidFinishLoading:]): Ditto.
(isQuickLookPasswordError): Moved the check for password failure errors to here from
-connection:didFailWithError: for better readability.
(-[WebPreviewConverter connection:didFailWithError:]): Removed _hasFailed check and used
more early returns.
(WebCore::QuickLookHandle::willSendRequest): Filtered the request through
-[QLPreviewConverter safeRequestWithRequest:] if the request URL's scheme is x-apple-ql-id:.
(WebCore::safeQLURLForDocumentURLAndResourceURL): Deleted.

LayoutTests:

  • quicklook/invalid-ql-id-url-expected.txt: Added.
  • quicklook/invalid-ql-id-url.html: Added.
  • quicklook/nil-response-mime-type-expected.txt: Removed.
  • quicklook/nil-response-mime-type.html: Removed.
  • quicklook/resources/invalid-ql-id-url.xls: Added.
  • quicklook/resources/nil-response-mime-type.xls: Removed.
Location:
trunk
Files:
3 added
3 deleted
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r211235 r211248  
     12017-01-26  Andy Estes  <aestes@apple.com>
     2
     3        [QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id: URLs
     4        https://bugs.webkit.org/show_bug.cgi?id=167453
     5
     6        Reviewed by Brent Fulgham.
     7
     8        * quicklook/invalid-ql-id-url-expected.txt: Added.
     9        * quicklook/invalid-ql-id-url.html: Added.
     10        * quicklook/nil-response-mime-type-expected.txt: Removed.
     11        * quicklook/nil-response-mime-type.html: Removed.
     12        * quicklook/resources/invalid-ql-id-url.xls: Added.
     13        * quicklook/resources/nil-response-mime-type.xls: Removed.
     14
    1152017-01-26  Jeremy Jones  <jeremyj@apple.com>
    216

  • trunk/Source/WebCore/ChangeLog

    r211247 r211248  
     12017-01-26  Andy Estes  <aestes@apple.com>
     2
     3        [QuickLook] REGRESSION (WebKit2): Requests are made to invalid x-apple-ql-id: URLs
     4        https://bugs.webkit.org/show_bug.cgi?id=167453
     5
     6        Reviewed by Brent Fulgham.
     7
     8        Requests to x-apple-ql-id: URLs should be filtered by
     9        -[QLPreviewConverter safeRequestForRequest:]. This method checks that the URL is one of the
     10        ones generated for the current preview, and changes it to "about:" if it isn't.
     11
     12        WebCore::safeQLURLForDocumentURLAndResourceURL() was responsible for finding the
     13        QLPreviewConverter instance to use by looking it up in an NSMutableDictionary using the
     14        document URL as a key. In WebKit1, this dictionary was populated by the
     15        QuickLookHandleClient when new QuickLookHandles were created, but the WebKit2
     16        QuickLookHandleClient never did this. As a result, requests to invalid URLs were not being
     17        rewritten.
     18
     19        An easy way to load a QuickLook document with invalid URLs is to create an HTML file with a
     20        Microsoft Office extension (e.g. .xls); QuickLook, iWork, and Office support opening HTML
     21        files with Office document extensions. In r207155 we applied a Content Security Policy to
     22        QuickLook documents that only allows x-apple-ql-id: resources to load. This blocked
     23        cross-origin requests from loading, but same-origin requests to URLs that weren't generated
     24        by QLPreviewConverter were still allowed to load.
     25
     26        This change blocks these URLs by calling -[QLPreviewConverter safeRequestForRequest:] in a
     27        way that works for both WebKit1 and WebKit2.
     28
     29        After implementing QuickLook for WebKit2, we found a bug when loading HTML-as-Office
     30        documents (webkit.org/b/135651) that presented as a nil MIME type in the preview
     31        NSURLResponse returned by QLPreviewConverter. Unfortunately r172159 papered over the actual
     32        bug by failing to load previews with nil MIME types.
     33
     34        The real issue was that we were asking for the preview response before QuickLook had
     35        received enough data to determine a MIME type, so this change also removes the bad fix from
     36        r172159 and instead waits until QuickLook has converted the document to ask for its preview
     37        response. This restores the ability to load HTML files with Office document extensions.
     38        These two fixes are combined in a single patch because I don't know how to create an invalid
     39        QuickLook URL for testing without loading an HTML-as-Office document.
     40
     41        Test: quicklook/invalid-ql-id-url.html
     42
     43        * loader/ResourceLoader.cpp:
     44        (WebCore::ResourceLoader::willSendRequestInternal): Called
     45        QuickLookHandle::willSendRequest() if m_documentLoader has a QuickLookHandle.
     46        * loader/cache/CachedResource.cpp:
     47        (WebCore::CachedResource::load): Removed the call to
     48        WebCore::safeQLURLForDocumentURLAndResourceURL().
     49        * loader/ios/QuickLook.h: Removed safeQLURLForDocumentURLAndResourceURL() and declared
     50        QuickLookHandle::willSendRequest().
     51        * loader/ios/QuickLook.mm: Removed _previewResponse and _hasFailed ivars from
     52        WebPreviewConverter.
     53        (-[WebPreviewConverter initWithResourceLoader:resourceResponse:quickLookHandle:]): Stopped
     54        setting _previewResponse.
     55        (-[WebPreviewConverter _sendDidReceiveResponseIfNecessary]): Only emptied _bufferedDataArray
     56        if we haven't already called -_sendDidReceiveResponseIfNecessary; removed the check for a
     57        nil _previewResponse MIME type; accessed -[QLPreviewConverter previewResponse] now that the
     58        document has been converted and asserted its MIME type is non-nil.
     59        (-[WebPreviewConverter connection:didReceiveData:lengthReceived:]): Removed _hasFailed check.
     60        (-[WebPreviewConverter connectionDidFinishLoading:]): Ditto.
     61        (isQuickLookPasswordError): Moved the check for password failure errors to here from
     62        -connection:didFailWithError: for better readability.
     63        (-[WebPreviewConverter connection:didFailWithError:]): Removed _hasFailed check and used
     64        more early returns.
     65        (WebCore::QuickLookHandle::willSendRequest): Filtered the request through
     66        -[QLPreviewConverter safeRequestWithRequest:] if the request URL's scheme is x-apple-ql-id:.
     67        (WebCore::safeQLURLForDocumentURLAndResourceURL): Deleted.
     68
    1692017-01-26  Keith Miller  <keith_miller@apple.com>
    270

  • trunk/Source/WebCore/loader/ResourceLoader.cpp

    r210859 r211248  
    5656#endif
    5757
     58#if USE(QUICK_LOOK)
     59#include "QuickLook.h"
     60#endif
     61
    5862namespace WebCore {
    5963
    … …  
    371375    else
    372376        InspectorInstrumentation::willSendRequest(m_frame.get(), m_identifier, m_frame->loader().documentLoader(), request, redirectResponse);
     377
     378#if USE(QUICK_LOOK)
     379    if (auto quickLookHandle = m_documentLoader->quickLookHandle())
     380        quickLookHandle->willSendRequest(request);
     381#endif
    373382
    374383    bool isRedirect = !redirectResponse.isNull();

  • trunk/Source/WebCore/loader/cache/CachedResource.cpp

    r210697 r211248  
    215215    m_loading = true;
    216216
    217 #if USE(QUICK_LOOK)
    218     if (!m_resourceRequest.isNull() && m_resourceRequest.url().protocolIs(QLPreviewProtocol())) {
    219         // When QuickLook is invoked to convert a document, it returns a unique URL in the
    220         // NSURLReponse for the main document. To make safeQLURLForDocumentURLAndResourceURL()
    221         // work, we need to use the QL URL not the original URL.
    222         const URL& documentURL = frameLoader.documentLoader()->response().url();
    223         m_resourceRequest.setURL(safeQLURLForDocumentURLAndResourceURL(documentURL, url()));
    224     }
    225 #endif
    226 
    227217    if (isCacheValidator()) {
    228218        CachedResource* resourceToRevalidate = m_resourceToRevalidate;

  • trunk/Source/WebCore/loader/ios/QuickLook.h

    r211236 r211248  
    4747class QuickLookHandleClient;
    4848class ResourceLoader;
     49class ResourceRequest;
    4950class ResourceResponse;
    5051class SharedBuffer;
    … …  
    6061WEBCORE_EXPORT RetainPtr<NSURLRequest> registerQLPreviewConverterIfNeeded(NSURL *, NSString *mimeType, NSData *);
    6162
    62 const URL safeQLURLForDocumentURLAndResourceURL(const URL& documentURL, const String& resourceURL);
    63 
    6463WEBCORE_EXPORT const char* QLPreviewProtocol();
    6564
    … …  
    7372    ~QuickLookHandle();
    7473
     74    void willSendRequest(ResourceRequest&);
    7575    bool didReceiveData(const char* data, unsigned length);
    7676    bool didReceiveBuffer(const SharedBuffer&);

  • trunk/Source/WebCore/loader/ios/QuickLook.mm

    r211236 r211248  
    3535#import "ResourceError.h"
    3636#import "ResourceLoader.h"
     37#import "ResourceRequest.h"
    3738#import "SharedBuffer.h"
    3839#import <wtf/NeverDestroyed.h>
    … …  
    124125
    125126    return nil;
    126 }
    127 
    128 const URL WebCore::safeQLURLForDocumentURLAndResourceURL(const URL& documentURL, const String& resourceURL)
    129 {
    130     id converter = nil;
    131     NSURL *nsDocumentURL = documentURL;
    132     {
    133         LockHolder lock(qlPreviewConverterDictionaryMutex());
    134         converter = [QLPreviewConverterDictionary() objectForKey:nsDocumentURL];
    135     }
    136 
    137     if (!converter)
    138         return URL(ParsedURLString, resourceURL);
    139 
    140     RetainPtr<NSURLRequest> request = adoptNS([[NSURLRequest alloc] initWithURL:[NSURL URLWithString:resourceURL]]);
    141     NSURLRequest *safeRequest = [converter safeRequestForRequest:request.get()];
    142     return [safeRequest URL];
    143127}
    144128
    … …  
    182166    RetainPtr<NSURLResponse> _originalResponse;
    183167    RetainPtr<QLPreviewConverter> _platformConverter;
    184     RetainPtr<NSURLResponse> _previewResponse;
    185168    RetainPtr<NSMutableArray> _bufferedDataArray;
    186169    BOOL _hasSentDidReceiveResponse;
    187     BOOL _hasFailed;
    188170}
    189171
    … …  
    211193    _originalResponse = resourceResponse.nsURLResponse();
    212194    _platformConverter = adoptNS([allocQLPreviewConverterInstance() initWithConnection:nil delegate:self response:_originalResponse.get() options:nil]);
    213     _previewResponse = [_platformConverter previewResponse];
    214195    _bufferedDataArray = adoptNS([[NSMutableArray alloc] init]);
    215196
    … …  
    253234- (void)_sendDidReceiveResponseIfNecessary
    254235{
     236    if (_hasSentDidReceiveResponse)
     237        return;
     238
    255239    [_bufferedDataArray removeAllObjects];
    256240
    257     if (_hasSentDidReceiveResponse || _hasFailed)
    258         return;
    259 
    260     // QuickLook might fail to convert a document without calling connection:didFailWithError: (see <rdar://problem/17927972>).
    261     // A nil MIME type is an indication of such a failure, so stop loading the resource and ignore subsequent delegate messages.
    262     if (![_previewResponse MIMEType]) {
    263         _hasFailed = YES;
    264         _resourceLoader->didFail(_resourceLoader->cannotShowURLError());
    265         return;
    266     }
    267 
    268     ResourceResponse response { _previewResponse.get() };
     241    ResourceResponse response { [_platformConverter previewResponse] };
    269242    response.setIsQuickLook(true);
     243    ASSERT(response.mimeType().length());
    270244
    271245    _hasSentDidReceiveResponse = YES;
    … …  
    277251    ASSERT_UNUSED(connection, !connection);
    278252    [self _sendDidReceiveResponseIfNecessary];
    279     if (_hasFailed)
    280         return;
    281253
    282254    // QuickLook code sends us a nil data at times. The check below is the same as the one in
    … …  
    289261{
    290262    ASSERT_UNUSED(connection, !connection);
    291     if (_hasFailed)
    292         return;
    293 
    294263    ASSERT(_hasSentDidReceiveResponse);
    295264    _resourceLoader->didFinishLoading(0);
    296265}
    297266
     267static inline bool isQuickLookPasswordError(NSError *error)
     268{
     269    return error.code == kQLReturnPasswordProtected && [error.domain isEqualToString:@"QuickLookErrorDomain"];
     270}
     271
    298272- (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error
    299273{
    300274    ASSERT_UNUSED(connection, !connection);
    301275
    302     if (error.code == kQLReturnPasswordProtected && [error.domain isEqualToString:@"QuickLookErrorDomain"]) {
    303         if (!_client->supportsPasswordEntry()) {
    304             _resourceLoader->didFail(_resourceLoader->cannotShowURLError());
    305             return;
    306         }
    307 
    308         _client->didRequestPassword([retainedSelf = retainPtr(self)] (const String& password) {
    309             NSDictionary *passwordOption = @{ (NSString *)kQLPreviewOptionPasswordKey : password };
    310             auto converterWithPassword = adoptNS([allocQLPreviewConverterInstance() initWithConnection:nil delegate:retainedSelf.get() response:retainedSelf->_originalResponse.get() options:passwordOption]);
    311             [converterWithPassword appendDataArray:retainedSelf->_bufferedDataArray.get()];
    312             [converterWithPassword finishedAppendingData];
    313             retainedSelf->_previewResponse = [converterWithPassword previewResponse];
    314             retainedSelf->_platformConverter = WTFMove(converterWithPassword);
    315         });
     276    if (!isQuickLookPasswordError(error)) {
     277        [self _sendDidReceiveResponseIfNecessary];
     278        _resourceLoader->didFail(error);
    316279        return;
    317280    }
    318281
    319     [self _sendDidReceiveResponseIfNecessary];
    320     if (!_hasFailed)
    321         _resourceLoader->didFail(error);
     282    if (!_client->supportsPasswordEntry()) {
     283        _resourceLoader->didFail(_resourceLoader->cannotShowURLError());
     284        return;
     285    }
     286
     287    _client->didRequestPassword([retainedSelf = retainPtr(self)] (const String& password) {
     288        NSDictionary *passwordOption = @{ (NSString *)kQLPreviewOptionPasswordKey : password };
     289        auto converterWithPassword = adoptNS([allocQLPreviewConverterInstance() initWithConnection:nil delegate:retainedSelf.get() response:retainedSelf->_originalResponse.get() options:passwordOption]);
     290        [converterWithPassword appendDataArray:retainedSelf->_bufferedDataArray.get()];
     291        [converterWithPassword finishedAppendingData];
     292        retainedSelf->_platformConverter = WTFMove(converterWithPassword);
     293    });
    322294}
    323295
    … …  
    390362}
    391363
     364void QuickLookHandle::willSendRequest(ResourceRequest& request)
     365{
     366    if (request.url().protocolIs(QLPreviewProtocol()))
     367        request = [[m_converter platformConverter] safeRequestForRequest:request.nsURLRequest(DoNotUpdateHTTPBody)];
     368}
     369
    392370bool QuickLookHandle::didReceiveData(const char* data, unsigned length)
    393371{
Note: See TracChangeset for help on using the changeset viewer.