464954 - cross-site XHR doesn't handle redirects correctly

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

There's various breakage here, as described in bug 235853 comment 147.  Need to fix this for final, imo.  Otherwise not only is cross-site XHR broken with proxies, but it also just doesn't work at all as soon as someone does a redirect.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Once this is fixed, the relevant part of content/base/test/Makefile.in needs to be changed (reenable the tests that got disabled in bug 235853).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Partial patch: fixes XHR itself, but not preflights.

Comment 3

[Johnny Stenback (:jst)]

Blocking, over to Jonas.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

This fixes redirects as well as a few spec compliance issues and other bugs.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch -w

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Given the number of spec compliance issues here I'd really like to get it into beta3

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 356866 [details] [diff] [review]
Patch -w

>+++ src.c1d7e0167221/content/base/public/nsContentUtils.h	2009-01-13 17:13:25.000000000 -0800
>+  static nsresult GetASCIIOrigin(nsIPrincipal* aPrincipal,
>+                                 nsCString& aOrigin);
>+  static nsresult GetASCIIOrigin(nsIURI* aURI, nsCString& aOrigin);
>+  static nsresult GetUTFOrigin(nsIPrincipal* aPrincipal,
>+                               nsString& aOrigin);
>+  static nsresult GetUTFOrigin(nsIURI* aURI, nsString& aOrigin);

Document, please.  Especially behavior if null is passed for principal or URI.  I assume that shouldn't be done, right?

>+++ src.c1d7e0167221/content/base/src/nsContentUtils.cpp	2009-01-13 17:13:25.000000000 -0800
>+nsContentUtils::GetASCIIOrigin(nsIPrincipal* aPrincipal, nsCString& aOrigin)

NS_PRECONDITION(aPrincipal), please.

>+nsContentUtils::GetASCIIOrigin(nsIURI* aURI, nsCString& aOrigin)

Similar here for aURI.

>+nsContentUtils::GetUTFOrigin(nsIPrincipal* aPrincipal, nsString& aOrigin)

And here for aPrincipal.

>+nsContentUtils::GetUTFOrigin(nsIURI* aURI, nsString& aOrigin)

and here for aURI.

> +    aOrigin = NS_ConvertASCIItoUTF16(scheme + NS_LITERAL_CSTRING("://") + host);

That's wrong.  You want to be converting from UTF-8, not ASCII, since GetHost can contain non-ascii UTF-8 in it.

This might also be better as:

  aOrigin = NS_ConvertUTF8toUTF16(scheme) + NS_LITERAL_STRING("://") + NS_ConvertUTF8toUTF16(host);

Hard to tell.

I wish we could share more code between the ASCII and UTF16 versions somehow, but I don't see how.  :(

>+++ src.c1d7e0167221/content/base/src/nsXMLHttpRequest.cpp	2009-01-13 17:13:25.000000000 -0800
> nsACProxyListener::OnChannelRedirect(nsIChannel *aOldChannel,
>   // No redirects allowed for now.

Fix the comment?

>+++ src.c1d7e0167221/content/base/test/file_CrossSiteXHR_inner.html	2009-01-13 17:13:25.000000000 -0800
>+  NOTE! The contents of this file is duplicated in file_CrossSiteXHR_inner.zip

"are duplicated"

r+sr=bzbarsky with thos changes, especially the fixed handling of non-ASCII hosts.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I keep getting pressure from shaver to land the PAC fix. Is this ready to land?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It is, as soon as I can get a green tinderbox :(

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in!

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

backing due to failing docshell(?!) tests.

Comment 12

[Boris Zbarsky [:bzbarsky]]

btw, when this goes back in again, mark in-testsuite+?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Had to back out due to failing tests. The problem was that I associated .zip with application/x-jar, and we have tests that rely on loading .zip files failing due to wrong mimetype. Renamed my file to .jar instead and checked back in.

So bug is still fixed :)

Comment 14

[(not currently active) Ted Mielczarek]

The windows unit test box has been failing test_CrossSiteXHR pretty frequently since this landed:
*** 2779 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CrossSiteXHR.html | wrong responseText in test for ({method:"HEAD", noAllowPreflight:1}) - got "<res>hello pass</res>\n", expected ""
*** 2780 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CrossSiteXHR.html | wrong responseText in test for ({method:"HEAD", noAllowPreflight:1}) - got "opening,rs1,sending,rs1,loadstart,rs2,rs3,rs4,load", expected "opening,rs1,sending,rs1,loadstart,rs2,rs4,load"
*** 2784 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CrossSiteXHR.html | wrong responseText in test for ({method:"HEAD", headers:{'Content-Type':"baz/bin", Accept:"foo/bar", 'Accept-Language':"sv-SE"}, noAllowPreflight:1}) - got "<res>hello pass</res>\n", expected ""
*** 2785 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CrossSiteXHR.html | wrong responseText in test for ({method:"HEAD", headers:{'Content-Type':"baz/bin", Accept:"foo/bar", 'Accept-Language':"sv-SE"}, noAllowPreflight:1}) - got "opening,rs1,sending,rs1,loadstart,rs2,rs3,rs4,load", expected "opening,rs1,sending,rs1,loadstart,rs2,rs4,load"

Looks like it's failed about 4/11 runs since this landed. (Not counting a few red builds in there.) Recently it has failed the last 2 completed runs.

Comment 15

[Peter Van der Beken [:peterv]]

I backed this out again to fix the orange mentioned in comment 14.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

When making a HEAD request and the server responds with an entity body, it seems like necko sometimes hands us back that body, but other times not.

I wasn't able to reproduce this locally, but I'll file a separate bug on that.

Checked this back in with the change to let file_CrossSiteXHR_server.sjs not respond with an entity body if it's a HEAD request.