cross-site XHR doesn't handle redirects correctly

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: bz, Assigned: sicking)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
x86
Mac OS X
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Flags: blocking1.9.1?
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).
Created attachment 348224 [details] [diff] [review]
Partial patch: fixes XHR itself, but not preflights.
Depends on: 464956
Blocking, over to Jonas.
Assignee: nobody → jonas
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b3

Updated

9 years ago
Priority: P1 → P2
Target Milestone: mozilla1.9.1b3 → mozilla1.9.1
Created attachment 356864 [details] [diff] [review]
Patch to fix

This fixes redirects as well as a few spec compliance issues and other bugs.
Attachment #348224 - Attachment is obsolete: true
Created attachment 356866 [details] [diff] [review]
Patch -w
Attachment #356866 - Flags: superreview?(bzbarsky)
Attachment #356866 - Flags: review?(bzbarsky)
Given the number of spec compliance issues here I'd really like to get it into beta3
Priority: P2 → P1
Attachment #356866 - Attachment is patch: true
Attachment #356866 - Attachment mime type: application/octet-stream → text/plain
Blocks: 462802
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.
Attachment #356866 - Flags: superreview?(bzbarsky)
Attachment #356866 - Flags: superreview+
Attachment #356866 - Flags: review?(bzbarsky)
Attachment #356866 - Flags: review+
I keep getting pressure from shaver to land the PAC fix. Is this ready to land?
It is, as soon as I can get a green tinderbox :(
Checked in!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
backing due to failing docshell(?!) tests.
btw, when this goes back in again, mark in-testsuite+?
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 :)
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.
I backed this out again to fix the orange mentioned in comment 14.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: in-testsuite? → in-testsuite+
Keywords: fixed1.9.1
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.