http/tests/security/xss-DENIED-xsl-document-redirect.xml fails with NetworkProcess

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[WK2] NetworkProcess doesn't set response URL on error

[Myles C. Maxfield]

Created attachment 230751 [details]
Patch

[Brady Eidson]

Comment on attachment 230751 [details]
Patch

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

r- because the ambiguity in the code change concerns me

> Source/WebKit2/ChangeLog:4
> +        [WK2] NetworkProcess doesn't set response URL on error
> +        https://bugs.webkit.org/show_bug.cgi?id=132523

Would like a more accurate  ChangeLog message here, as this is specifically about synchronous XHRs

> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:105
> +    if (m_response.isNull() && m_response.url().isNull())
> +        m_response.setURL(m_originalRequest.url());

The bug title suggests that response urls are never set for these loads, yet this code suggests that they are sometimes set...?

In which cases is it already set?

> LayoutTests/ChangeLog:8
> +        Un-marking test as fail

I find it kind of hard to parse this.

"Re-enable test that now passes"?

[Myles C. Maxfield]

Comment on attachment 230751 [details]
Patch

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

>> Source/WebKit2/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=132523
> 
> Would like a more accurate  ChangeLog message here, as this is specifically about synchronous XHRs

Done.

>> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:105
>> +        m_response.setURL(m_originalRequest.url());
> 
> The bug title suggests that response urls are never set for these loads, yet this code suggests that they are sometimes set...?
> 
> In which cases is it already set?

Originally I did this to be conservative. However, I can't find a place where this gets set at all.

Done.

>> LayoutTests/ChangeLog:8
>> +        Un-marking test as fail
> 
> I find it kind of hard to parse this.
> 
> "Re-enable test that now passes"?

Done.

[Myles C. Maxfield]

Created attachment 230763 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 230763 [details]
Patch

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

Thank you for tackling this, very nice!

I think that this approach is OK, although not the right long-term fix.

> Source/WebKit2/ChangeLog:9
> +        On network failure, WK1 sets the response URL to the original request
> +        URL (at the end of ResourceHandle::platformLoadResourceSynchronously).

This code has a FIXME, which I think makes sense. Manufacturing a response when there is no response is bad, we should fix the code that needs it.

        // FIXME: We might not ever need to manufacture a response: This might all be dead code.
        // When exploring removal of this code, we should substitute appropriate ASSERTs.

I'd add a FIXME in NetworkProcess code too, and amend the one in platformLoadResourceSynchronously() saying that at least this test verifies that we still need the URL.

> Source/WebKit2/NetworkProcess/SynchronousNetworkLoaderClient.cpp:104
> +    m_response.setURL(m_originalRequest.url());

m_response can be non-null if the failure occurred after didReceiveResponse. But ResourceHandle code overwrites the response too.

It seems wrong to use the original URL and not the latest one. It's not really clear what the response should contain, which is why I think that we shouldn't rely on it when loading fails.

[Myles C. Maxfield]

I agree with you, ap. Marking as r-.

[Myles C. Maxfield]

The problematic code is in docLoaderFunc() in XSLTProcessorLibxslt.cpp

[Myles C. Maxfield]

Created attachment 230960 [details]
Not for review

[Myles C. Maxfield]

This latest patch is not quite right. Rebaselining the expected result for xss-DENIED-xsl-document-redirect.html should not have to be done.

[Alexey Proskuryakov]

Removing [WK2] from the title, as the code change is now expected to touch non-WebKit2 files.

[Myles C. Maxfield]

Created attachment 231036 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231036 [details]
Patch

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

Nice!

> Source/WebCore/xml/XSLTProcessorLibxslt.cpp:132
> -            requestAllowed = globalCachedResourceLoader->document()->securityOrigin()->canRequest(response.url());
> +            if (error.isNull())
> +                requestAllowed = globalCachedResourceLoader->document()->securityOrigin()->canRequest(response.url());

I think that this introduces an incorrect behavior in the following scenario:
1. A cross-origin redirect occurs.
2. The resource starts to load, and then connection is dropped. If this happens at an appropriate time, the data could still be a well-formed XML document.

As error is not null, we don't do the canRequest check, and proceed using the data.

The function is structured strangely, so I'm not sure if we can do good error handling here (and we don't report more common errors anyway already). Please add "else data.clear();", and post a patch for EWS to run tests with that.

[Myles C. Maxfield]

Created attachment 231093 [details]
Patch

[Myles C. Maxfield]

http://trac.webkit.org/changeset/168498

[WebKit Commit Bot]

Re-opened since this is blocked by bug 132714

[Alexey Proskuryakov]

What was the breakage?

[Myles C. Maxfield]

Created attachment 231217 [details]
Patch

[Myles C. Maxfield]

Mavericks WK2 had the unmodified test output. This latest patch marks it as such.

[Alexey Proskuryakov]

I'm still a bit confused. The new results that you add appear to be correct, and cross-platform results in fast/xmlhttprequest/xmlhttprequest-nonexistent-file-expected.txt are incorrect.

Shouldn't we have correct results in the cross-platform file, and let platforms that fail land custom results? Spraying the same correct results over many port directories is inconvenient.

[Myles C. Maxfield]

Created attachment 231355 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231355 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        synchronous XHR. In addition, this test removes one place that is
> +        sensitive to such a manufactured response.

Is it still one place?

> Source/WebCore/loader/DocumentThreadableLoader.cpp:426
> -    if (requestURL != response.url() && !isAllowedRedirect(response.url())) {
> +    if (error.isNull() && requestURL != response.url() && !isAllowedRedirect(response.url())) {
>          m_client->didFailRedirectCheck();
>          return;
>      }

This change is new in this version of the patch. I agree that this place looks like it needs to be fixed too, but the fix looks very confusing.

Above, there is a comment about how "if we have an HTTP response, then it wasn't a network error in fact." Doesn't it become obsolete with your changes? Even considering that we have the quirk for local files, perhaps something like this would be less confusing?

if (!error.isNull() {
    if (requestURL.isLocalFile()) {
        // We don't want XMLHttpRequest to raise an exception for file:// resources, see <rdar://problem/4962298>.
        // FIXME: XMLHttpRequest quirks should be in XMLHttpRequest code, not in DocumentThreadableLoader.cpp.
        didReceiveResponse(identifier, ResourceResponse());
        didFinishLoading(identifier, 0.0);
        return;
    }
    m_client->didFail(error);
    return;
    }
}

// FIXME: FrameLoader::loadSynchronously() does not tell us whether a redirect happened or not, so <...>
if (requestURL != response.url() && !isAllowedRedirect(response.url())) {
    m_client->didFailRedirectCheck();
    return;
}

... normal success case

[Alexey Proskuryakov]

Not sure where our check for http:// resources not being allowed to redirect to file:// is, my proposed snippet might change the behavior in this case.

[Myles C. Maxfield]

(In reply to comment #22)
> Not sure where our check for http:// resources not being allowed to redirect to file:// is, my proposed snippet might change the behavior in this case.

Looks like it is done in SecurityOrigin::canRequest() which is called from isAllowedRedirect()

[Myles C. Maxfield]

Created attachment 231599 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 231599 [details]
Patch

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

r=me assuming there is a good response to the question below. Otherwise, please remove the httpStatusCode check.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:413
> +    // If we have an HTTP response, then it wasn't a network error (even if !error.isNull()).

Why? I don't see how this can happen.

[Myles C. Maxfield]

Comment on attachment 231599 [details]
Patch

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

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:413
>> +    // If we have an HTTP response, then it wasn't a network error (even if !error.isNull()).
> 
> Why? I don't see how this can happen.

Turns out you added it yourself in https://bugs.webkit.org/show_bug.cgi?id=14704

[Myles C. Maxfield]

Comment on attachment 231599 [details]
Patch

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

>>> Source/WebCore/loader/DocumentThreadableLoader.cpp:413
>>> +    // If we have an HTTP response, then it wasn't a network error (even if !error.isNull()).
>> 
>> Why? I don't see how this can happen.
> 
> Turns out you added it yourself in https://bugs.webkit.org/show_bug.cgi?id=14704

http/tests/appcache/fallback.html relies on it.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/169243

[Alexey Proskuryakov]

> Turns out you added it yourself in https://bugs.webkit.org/show_bug.cgi?id=14704

Yes, but doesn't this change make it impossible? I thought that it did.

> http/tests/appcache/fallback.html relies on it.

Even after this change? I'm curious about the mechanism of this dependence.

[Myles C. Maxfield]

(In reply to comment #29)
> > Turns out you added it yourself in https://bugs.webkit.org/show_bug.cgi?id=14704
> 
> Yes, but doesn't this change make it impossible? I thought that it did.
> 
> > http/tests/appcache/fallback.html relies on it.
> 
> Even after this change? I'm curious about the mechanism of this dependence.

Yes, even after this change. Appcache seems to have it's own logic about what to return.

I started reading the test but couldn't immediately find the cause (and I had other work to do), so I left it at that. At some point I'd like to remove this check.