WebKit should dispatchDidFailProvisionalLoad while loading invalid URLs

• Component: WebKit2
• Status: RESOLVED FIXED

[Jiewen Tan]

* SUMMARY
Handling of invalid URLs should be investigated

* STEPS TO REPRODUCE
URLs that have certain invalid characters result in an about:blank page instead of a Safari error page. The following URLs can be used to demonstrate this. Try them out from href links.

file://www.apple.com<>/
https://www.apple.com<>/

[Jiewen Tan]

<rdar://problem/14967004>

[Jiewen Tan]

Created attachment 275148 [details]
Patch

[Brent Fulgham]

Comment on attachment 275148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275148&action=review

This looks great! I think the test cases need a little work. (1) I'd like to see a WK1 test case, since the 'cancelMainResourceLoad' is invoked for WK1 and WK2. (2) I'd like to see a "success" case for WK2 (and WK1) that confirms that normal loads work properly with this change. For those reasons, r- to add some additional test cases.

> Source/WebCore/ChangeLog:9
> +        Added API Tests.

It looks like you added a WK2 test, but this code change is in WebCore. It seems like you should also have a WK1 test, following something like the code in BackForwardList.mm as an example.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:77
> +}

Please add another test for the "success" case where you navigate to a valid URL just to make sure we didn't break anything.

[Sam Weinig]

What security vulnerability is this bug in regards to? While it is not necessary to explain in detail in the bug the issue is (and you shouldn't) this bug should out line the concern.

[Jiewen Tan]

(In reply to comment #4)
> What security vulnerability is this bug in regards to? While it is not
> necessary to explain in detail in the bug the issue is (and you shouldn't)
> this bug should out line the concern.

The original concern is:
Window objects that are scriptable from other windows such as the opener could potentially use this bug to mislead users about what page they are at

However, I don't think it is valid now as the bug is not quite the same as it is originally reported.

[Jiewen Tan]

Comment on attachment 275148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275148&action=review

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:77
>> +}
> 
> Please add another test for the "success" case where you navigate to a valid URL just to make sure we didn't break anything.

I think existing layout tests/api tests should already cover the "success" case.

[Andy Estes]

Comment on attachment 275148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275148&action=review

> Source/WebCore/loader/DocumentLoader.cpp:1516
> +        if (!m_request.url().isValid())
> +            cancelMainResourceLoad(frameLoader()->client().cannotShowURLError(m_request));
> +        else {

You should just return early after calling cancelMainResourceLoad().

> Source/WebCore/loader/FrameLoader.cpp:2213
> +            if (!m_provisionalLoadErrorBeingHandledURL.isEmpty() && !error.isCancellation())
> +                return;
> +

You don't explain why you moved this or why added a check for cancellation errors. You also don't test this new behavior. I think you need to do both of these things.

[Andy Estes]

Comment on attachment 275148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275148&action=review

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:69
> +        [webView loadRequest:[NSURLRequest requestWithURL:contentURL]];

Is it actually necessary to load an HTML file that clicks a link to "https://www.apple.com<>/"? Would it not be sufficient to just call loadRequest: directly with that URL?

[Sam Weinig]

(In reply to comment #5)
> (In reply to comment #4)
> > What security vulnerability is this bug in regards to? While it is not
> > necessary to explain in detail in the bug the issue is (and you shouldn't)
> > this bug should out line the concern.
> 
> The original concern is:
> Window objects that are scriptable from other windows such as the opener
> could potentially use this bug to mislead users about what page they are at
> 
> However, I don't think it is valid now as the bug is not quite the same as
> it is originally reported.

Ok, then what is the issue now?

[Jiewen Tan]

Comment on attachment 275148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275148&action=review

>> Source/WebCore/loader/DocumentLoader.cpp:1516
>> +        else {
> 
> You should just return early after calling cancelMainResourceLoad().

I have a return statement after the if else control.

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:69
>> +        [webView loadRequest:[NSURLRequest requestWithURL:contentURL]];
> 
> Is it actually necessary to load an HTML file that clicks a link to "https://www.apple.com<>/"? Would it not be sufficient to just call loadRequest: directly with that URL?

I cannot create a NSURL object by passing a string containing an invalid URL. There is a complicated way we use in WebCoreNSURLExtras.mm to create a NSURL that accepts invalid URL strings. I choose the current way as it is easier and more understandable.

[Jiewen Tan]

(In reply to comment #9)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > What security vulnerability is this bug in regards to? While it is not
> > > necessary to explain in detail in the bug the issue is (and you shouldn't)
> > > this bug should out line the concern.
> > 
> > The original concern is:
> > Window objects that are scriptable from other windows such as the opener
> > could potentially use this bug to mislead users about what page they are at
> > 
> > However, I don't think it is valid now as the bug is not quite the same as
> > it is originally reported.
> 
> Ok, then what is the issue now?

I am not capable of reassessing the security vulnerability of this bug. From my understanding, it is that we should show a Safari error page by

[Jiewen Tan]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > What security vulnerability is this bug in regards to? While it is not
> > > > necessary to explain in detail in the bug the issue is (and you shouldn't)
> > > > this bug should out line the concern.
> > > 
> > > The original concern is:
> > > Window objects that are scriptable from other windows such as the opener
> > > could potentially use this bug to mislead users about what page they are at
> > > 
> > > However, I don't think it is valid now as the bug is not quite the same as
> > > it is originally reported.
> > 
> > Ok, then what is the issue now?
> 
> I am not capable of reassessing the security vulnerability of this bug. From
> my understanding, it is that we should show a Safari error page by

dispatchDidFailProvisionalLoad with proper error msg when users navigate to invalid URLs.

[Andy Estes]

(In reply to comment #10)
> Comment on attachment 275148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275148&action=review
> 
> >> Source/WebCore/loader/DocumentLoader.cpp:1516
> >> +        else {
> > 
> > You should just return early after calling cancelMainResourceLoad().
> 
> I have a return statement after the if else control.

Right. I'm suggesting you don't add the else block and instead just return after calling cancelMainResourceLoad().
> 
> >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:69
> >> +        [webView loadRequest:[NSURLRequest requestWithURL:contentURL]];
> > 
> > Is it actually necessary to load an HTML file that clicks a link to "https://www.apple.com<>/"? Would it not be sufficient to just call loadRequest: directly with that URL?
> 
> I cannot create a NSURL object by passing a string containing an invalid
> URL. There is a complicated way we use in WebCoreNSURLExtras.mm to create a
> NSURL that accepts invalid URL strings. I choose the current way as it is
> easier and more understandable.

Ok, thank makes sense.

[Jiewen Tan]

Created attachment 275254 [details]
Patch

[Brent Fulgham]

Comment on attachment 275254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275254&action=review

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1007
> +    // the page load state will not transit in a desired order.

// When the UIProcess is in the process of handling a failing provisional load, do not attempt to
// start a second alternative HTML load as this will prevent the page load state from being
// handled properly.

> Source/WebKit2/UIProcess/WebPageProxy.h:1514
> +    bool m_isDealingFailingProvisionalLoad;

I think this should be "bool m_isDealingFailingProvisionalLoad { false };"

[Andy Estes]

Comment on attachment 275254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275254&action=review

From your conversation with Sam, I'm somewhat unclear if this still needs to be a security bug. r=me.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
> +    if (m_isClosed || m_isDealingFailingProvisionalLoad)

Wouldn't any new load started in response to a provisional load failure cause this problem? For instance, if the client called loadRequest: instead of _loadAlternateHTMLString:baseURL:forUnreachableURL:, would you see the same assertion? If so, seems like we need a fix that won't involve adding checks like this to every loading entry point in the UI process. I'm ok with this for now, since you are handling what Safari does, but we might need to come up with a better long-term solution.

>> Source/WebKit2/UIProcess/WebPageProxy.h:1514
>> +    bool m_isDealingFailingProvisionalLoad;
> 
> I think this should be "bool m_isDealingFailingProvisionalLoad { false };"

Not a fan of this name. Let's be specific: m_isLoadingAlternateHTMLStringForFailingProvisionalLoad.

> Tools/ChangeLog:10
> +        * TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:

It doesn't make sense to have a test with WebKit1 code inside Tests/WebKit2Cocoa/. Either add two files, or just put this in Tests/Cocoa/.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:38
> +static int provisionalLoadCount = 0;

You don't need to initialize this to 0.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:48
> +static NSData *literalAsData(const char* literal)
> +{
> +    return [NSData dataWithBytes:literal length:strlen(literal)];
> +}
> +
> +static NSURL *literalURL(const char* literal)
> +{
> +    return WebCore::URLWithData(literalAsData(literal), nil);
> +}

Not sure you really need these functions.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:58
> +        [webView _loadAlternateHTMLString:@"error page" baseURL:nil forUnreachableURL:[NSURL URLWithString:@"data:"]];

The unreachable URL shouldn't be "data:", it should be the invalid URL you passed to loadRequest:.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:122
> +        [webView loadRequest:[NSURLRequest requestWithURL:literalURL("https://www.example.com<>/")]];
> +        [webView loadRequest:[NSURLRequest requestWithURL:literalURL("https://www.example.com<>/")]];

Why create two of the same URLs when you could just have one local NSURL?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:59
> +- (void)webView:(WKWebView *)webView didFailProvisionalNavigation:(WKNavigation *)navigation withError:(NSError *)error

Can you check that the NSError has the domain/code/userInfo you'd expect for a cannotShowURLError?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadInvalidURLRequest.mm:80
> +- (void)webView:(WebView *)sender didFailProvisionalLoadWithError:(NSError *)error forFrame:(WebFrame *)frame

Ditto.

[Jiewen Tan]

Comment on attachment 275254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275254&action=review

Thanks Andy and Brent for giving comments!

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
>> +    if (m_isClosed || m_isDealingFailingProvisionalLoad)
> 
> Wouldn't any new load started in response to a provisional load failure cause this problem? For instance, if the client called loadRequest: instead of _loadAlternateHTMLString:baseURL:forUnreachableURL:, would you see the same assertion? If so, seems like we need a fix that won't involve adding checks like this to every loading entry point in the UI process. I'm ok with this for now, since you are handling what Safari does, but we might need to come up with a better long-term solution.

I don't think other APIs have the same problem. It should be only specific to loadAlternateHTMLString as the way we tell if we are in the process of responding to a provisional load failure in WebProcess is to check whether FrameLoader ::m_provisionalLoadErrorBeingHandledURL is empty. This member is only set/clear at WebPage::loadAlternateHTMLString.

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:48
>> +}
> 
> Not sure you really need these functions.

I can't initiate a NSURL object easily by passing a string containing invalid URLs. However, with the help of WebCore::URLWithData, it can be done.

[Andy Estes]

(In reply to comment #17)
> Comment on attachment 275254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275254&action=review
> 
> Thanks Andy and Brent for giving comments!
> 
> >> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
> >> +    if (m_isClosed || m_isDealingFailingProvisionalLoad)
> > 
> > Wouldn't any new load started in response to a provisional load failure cause this problem? For instance, if the client called loadRequest: instead of _loadAlternateHTMLString:baseURL:forUnreachableURL:, would you see the same assertion? If so, seems like we need a fix that won't involve adding checks like this to every loading entry point in the UI process. I'm ok with this for now, since you are handling what Safari does, but we might need to come up with a better long-term solution.
> 
> I don't think other APIs have the same problem. It should be only specific
> to loadAlternateHTMLString as the way we tell if we are in the process of
> responding to a provisional load failure in WebProcess is to check whether
> FrameLoader ::m_provisionalLoadErrorBeingHandledURL is empty. This member is
> only set/clear at WebPage::loadAlternateHTMLString.

No, that's not true. m_provisionalLoadErrorBeingHandledURL is set in FrameLoader whenever there's a provisional load failure. It's also set in WebPage::loadAlternateHTMLString(), but that's just to account for the fact that WebKit2 clients do not load alternate HTML synchronously.

My question was whether the assertion failure you saw would also occur in the presence of two sequential provisionally failing loads when the client responded to the second one with something other than _loadAlternateHTMLString:...

> 
> >> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/LoadAlternateHTMLString.mm:48
> >> +}
> > 
> > Not sure you really need these functions.
> 
> I can't initiate a NSURL object easily by passing a string containing
> invalid URLs. However, with the help of WebCore::URLWithData, it can be done.

I get that part. I was just suggesting that you didn't need to create new functions to do this; just call URLWithData() directly.

[Jiewen Tan]

Comment on attachment 275254 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275254&action=review

>>>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
>>>> +    if (m_isClosed || m_isDealingFailingProvisionalLoad)
>>> 
>>> Wouldn't any new load started in response to a provisional load failure cause this problem? For instance, if the client called loadRequest: instead of _loadAlternateHTMLString:baseURL:forUnreachableURL:, would you see the same assertion? If so, seems like we need a fix that won't involve adding checks like this to every loading entry point in the UI process. I'm ok with this for now, since you are handling what Safari does, but we might need to come up with a better long-term solution.
>> 
>> I don't think other APIs have the same problem. It should be only specific to loadAlternateHTMLString as the way we tell if we are in the process of responding to a provisional load failure in WebProcess is to check whether FrameLoader ::m_provisionalLoadErrorBeingHandledURL is empty. This member is only set/clear at WebPage::loadAlternateHTMLString.
> 
> No, that's not true. m_provisionalLoadErrorBeingHandledURL is set in FrameLoader whenever there's a provisional load failure. It's also set in WebPage::loadAlternateHTMLString(), but that's just to account for the fact that WebKit2 clients do not load alternate HTML synchronously.
> 
> My question was whether the assertion failure you saw would also occur in the presence of two sequential provisionally failing loads when the client responded to the second one with something other than _loadAlternateHTMLString:...

I think the situation you mentioned is fine, but what will cause problems is replacing the first one with some other load APIs.

Once WebProcess enter the state of loadAlternateHTMLString, it will lose the ability to send DidFailProvisionalLoad error through checkLoadCompleteForThisFrame to the clients, and therefore any back to back load before loadAlternateHTMLString will cause the assertion to hit.

The way how loadAlternateHTMLString lock things down is too heavy. I don't have any idea to fix this for long term so far.

[Andy Estes]

(In reply to comment #19)
> Comment on attachment 275254 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275254&action=review
> 
> >>>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1008
> >>>> +    if (m_isClosed || m_isDealingFailingProvisionalLoad)
> >>> 
> >>> Wouldn't any new load started in response to a provisional load failure cause this problem? For instance, if the client called loadRequest: instead of _loadAlternateHTMLString:baseURL:forUnreachableURL:, would you see the same assertion? If so, seems like we need a fix that won't involve adding checks like this to every loading entry point in the UI process. I'm ok with this for now, since you are handling what Safari does, but we might need to come up with a better long-term solution.
> >> 
> >> I don't think other APIs have the same problem. It should be only specific to loadAlternateHTMLString as the way we tell if we are in the process of responding to a provisional load failure in WebProcess is to check whether FrameLoader ::m_provisionalLoadErrorBeingHandledURL is empty. This member is only set/clear at WebPage::loadAlternateHTMLString.
> > 
> > No, that's not true. m_provisionalLoadErrorBeingHandledURL is set in FrameLoader whenever there's a provisional load failure. It's also set in WebPage::loadAlternateHTMLString(), but that's just to account for the fact that WebKit2 clients do not load alternate HTML synchronously.
> > 
> > My question was whether the assertion failure you saw would also occur in the presence of two sequential provisionally failing loads when the client responded to the second one with something other than _loadAlternateHTMLString:...
> 
> I think the situation you mentioned is fine, but what will cause problems is
> replacing the first one with some other load APIs.
> 
> Once WebProcess enter the state of loadAlternateHTMLString, it will lose the
> ability to send DidFailProvisionalLoad error through
> checkLoadCompleteForThisFrame to the clients, and therefore any back to back
> load before loadAlternateHTMLString will cause the assertion to hit.
> 
> The way how loadAlternateHTMLString lock things down is too heavy. I don't
> have any idea to fix this for long term so far.

It isn't urgent since we don't know if any clients actually do this, but filing a bug and writing some test cases is a good place to start. Thanks!

[Jiewen Tan]

Created attachment 275426 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 275426 [details]
Patch for landing

Clearing flags on attachment: 275426

Committed r198956: <http://trac.webkit.org/changeset/198956>

[Jiewen Tan]

Bug 156117 is filed to address the issue of SPI: _LoadAlternateHTMLString.

[Jiewen Tan]

A build fix for adopting existing API tests to the new change has been landed:
Committed r198964: <http://trac.webkit.org/changeset/198964>

[Andy Estes]

(In reply to comment #22)
> Comment on attachment 275426 [details]
> Patch for landing
> 
> Clearing flags on attachment: 275426
> 
> Committed r198956: <http://trac.webkit.org/changeset/198956>
>
> +- (void)webView:(WKWebView *)webView didFailProvisionalNavigation:(WKNavigation *)navigation withError:(NSError *)error
> +{
> +    if ([error.domain isEqual:@"WebKitErrorDomain"]
> +        && error.code == WebKitErrorCannotShowURL
> +        && [error.userInfo[@"NSErrorFailingURLKey"] isEqual:literalURL(literal)])
> +        didFailProvisionalLoad = true;
> +    didFinishTest = true;
> +}

This wasn't what I meant when I suggested that you "check that the NSError has the domain/code/userInfo you'd expect". I was suggesting that you use EXPECT macros to verify the error is of the form we expect. Sorry if I wasn't clear.

[Jiewen Tan]

(In reply to comment #25)
> (In reply to comment #22)
> > Comment on attachment 275426 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 275426
> > 
> > Committed r198956: <http://trac.webkit.org/changeset/198956>
> >
> > +- (void)webView:(WKWebView *)webView didFailProvisionalNavigation:(WKNavigation *)navigation withError:(NSError *)error
> > +{
> > +    if ([error.domain isEqual:@"WebKitErrorDomain"]
> > +        && error.code == WebKitErrorCannotShowURL
> > +        && [error.userInfo[@"NSErrorFailingURLKey"] isEqual:literalURL(literal)])
> > +        didFailProvisionalLoad = true;
> > +    didFinishTest = true;
> > +}
> 
> This wasn't what I meant when I suggested that you "check that the NSError
> has the domain/code/userInfo you'd expect". I was suggesting that you use
> EXPECT macros to verify the error is of the form we expect. Sorry if I
> wasn't clear.

Committed r199018: <http://trac.webkit.org/changeset/199018> to address Andy's comments.