598304 - XHR rewrites non-POST methods upon 301/302 redirects

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Julian Reschke]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 (.NET CLR 3.5.30729)
Build Identifier: 3.6.10

The test cases at http://www.mnot.net/javascript/xmlhttprequest/ show that XHR, when following a 301/302 redirect, changes the method to GET.

According to RFC 2616, Sections 10.3.2 and 10.3.3 (http://greenbytes.de/tech/webdav/rfc2616.html#rfc.section.10.3), this is a bug.

That being said, it's understood that rewriting *POST* is required for compatibility with existing content. However, this does not seem to be the case for other methods, as IE restricts the conformance violation to POST and apparently gets away with it.

Please consider tightening the implementation.

Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

Necko does this, not XHR itself.  This is impossible to fix on the XHR level without necko changes.

Comment 2

[Julian Reschke]

(In reply to comment #1)
> Necko does this, not XHR itself.  This is impossible to fix on the XHR level
> without necko changes.

That sounds like a good thing. Optimally, the behavior would be the same everywhere.

Comment 3

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

Isn't 307 the redirect code that should be used if you want to preserve the method?

I don't feel strongly. What is the status of httpbis? Is that compatible enough with the web that we can use that as a reference?

Comment 4

[Julian Reschke]

(In reply to comment #3)
> Isn't 307 the redirect code that should be used if you want to preserve the
> method?

It's required for all but 303.
 
> I don't feel strongly. What is the status of httpbis? Is that compatible enough
> with the web that we can use that as a reference?

We're still discussing it. Fact is, IE doesn't rewrite anything *except* POST. Isn't that indication enough what that's good enough for compat with broken content?

Comment 5

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

Like I said, I don't feel strongly (though others that are involved more in networking might).

But it's not just a matter of what is compatible with existing content, but also about what the right thing to do is. That's why I'd like to be able to follow some sort of spec. That's why we have the http(bis) spec after all.

Comment 6

[Julian Reschke]

(In reply to comment #5)
> Like I said, I don't feel strongly (though others that are involved more in
> networking might).
> 
> But it's not just a matter of what is compatible with existing content, but
> also about what the right thing to do is. That's why I'd like to be able to
> follow some sort of spec. That's why we have the http(bis) spec after all.

The right thing to do, according to RFC 2616, is never to rewrite (except for 303 which has been added in 1999 for exactly that use case).

I think the WG is ready to relax the rules for 301 and 302, or rephrase the note that's currently in. The question is: how far do we *need* to relax it.

Please come over to the HTTPbis WG mailing list and provide feedback; see thrad around http://lists.w3.org/Archives/Public/ietf-http-wg/2010JulSep/0323.html

Comment 7

[Tyson Clugg]

Severity: critical (loss of data)

The POST request is lost, along with it's request body.  If the request body is included in a subsequent GET request, it would be rejected (correctly) as no content is allowed in GET requests.

Because there is DATA LOSS (the POST request body), this bug should be raised to CRITICAL severity.

This bug causes credit card payment failures for the payment system we use at my work.  The system relies on signalling via a POST after successful payments to prevent duplicate payment processing (as would happen if the user clicked the back button after making a payment then hit the submit button again).  As the POST body is LOST, the payment system refuses to process any more payments until the browser cache is cleared.  Clearing the cache works because the POST is changed to a GET which can be fulfilled with a cached request (see comment 17 of #659569 for why the cache is involved at all).

Comment 8

[Julian Reschke]

Potential place to fix:

HttpBaseChannel::SetupReplacementChannel

...change the code from

if (preserveMethod) 

to

if (preserveMethod || method-is-not-POST)

(then maybe rename "preserveMethod" to "preservePOSTMethod")

Comment 9

[Julian Reschke]

Seems the cause is in nsHttpChannel::ContinueProcessRedirectionAfterFallback nsHttpChannel.cpp and can be fixed by changing the computation of preserveMethod over here.

Comment 10

[Julian Reschke]

Attachment: (work in progress)

Comment 11

[Julian Reschke]

Here's a real world problem caused by this.

Consider a web page using XHR to use PROPFIND. Note that Apache/mod_dav redirects requests on folders when the trailing / is missing.

Client issues PROPFIND to URI not ending with /, server redirects to URI + "/" (301), client issues GET request to that URI. Request method and body are lost. Response doesn't make any sense to client.

Comment 12

[Julian Reschke]

To do this properly, we should have a single place for functions that can be used to check for various HTTP properties.

1) given a status code and a method name, should the redirected request use GET?

shouldUseGETinRedirect(statusCode, methodName) -> boolean

2) is the method safe (needed for prompting)

isMethodSafe(methodName) -> boolean

3) not needed here, but probably used elsewhere:

isMethodIdemPotent(methodName) -> boolean


...what would be the right place to add these?

Comment 13

[Julian Reschke]

Attachment: rough patch, work in progress

funcionality-wise this seems to be right; now looking for test cases

Comment 15

[Julian Reschke]

Attachment: proposed patch, incl test case

This patch:

- adds convenience functions for finding out what to do with a redirect, and for finding out about safe methods

- adds a few WebDAV methods as atoms, so they can be detected as safe

- changes redirect following for 301/302 to rewrite POST to GET (aligning with IE, and implementing what HTTPbis wants to do, see <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160>)

- changes the code that decided on prompting the user to do this based on the safeness of the method

This change probably should be exposed in nightly builds for a full six week period, thus it would be great if it could get reviewed and tuned so that it can be applied right after the next branch date.

Comment 16

[Julian Reschke]

(In reply to comment #15)
> - changes redirect following for 301/302 to rewrite POST to GET (aligning
> with IE, and implementing what HTTPbis wants to do, see
> <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160>)

...and only POST (that's the main change). Other methods like PUT, DELETE or PROPFIND are left alone.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 550098 [details] [diff] [review]
proposed patch, incl test case

> +HttpBaseChannel::IsSafeMethod(nsHttpAtom method)

All the extra parens in this method can go away, I think.

Likewise for the ones around the compare to 303 and to nsHttp::Post in RewriteRedirectMethodToGET.

I'd rename RewriteRedirectMethodToGET to ShouldRewriteMethodToGETForRedirect, perhaps.

> +  PRBool rewriteToGET = RewriteRedirectMethodToGET(mResponseHead->Status(),
> mRequestHead.Method());

Please wrap to 80 cols.

I'm not sure about the IsSafeMethod change.  That check is really not supposed to be about safety as much as about resending a request body.  I'd prefer we not change that in this bug and use a separate bug for it.  r- to make sure that happens...

The rest looks fine.

Comment 18

[Julian Reschke]

(In reply to comment #17)
> Comment on attachment 550098 [details] [diff] [review] [diff] [details] [review]
> proposed patch, incl test case
> 
> > +HttpBaseChannel::IsSafeMethod(nsHttpAtom method)
> 
> All the extra parens in this method can go away, I think.

Ack.

> Likewise for the ones around the compare to 303 and to nsHttp::Post in
> RewriteRedirectMethodToGET.

Ack.

> I'd rename RewriteRedirectMethodToGET to
> ShouldRewriteMethodToGETForRedirect, perhaps.

Ack.

> > +  PRBool rewriteToGET = RewriteRedirectMethodToGET(mResponseHead->Status(),
> > mRequestHead.Method());
> 
> Please wrap to 80 cols.

Ack.

> I'm not sure about the IsSafeMethod change.  That check is really not
> supposed to be about safety as much as about resending a request body.  I'd
> prefer we not change that in this bug and use a separate bug for it.  r- to
> make sure that happens...

The reason why I added this is because previously 301/302 redirected DELETE and PUT to GET, a safe method. Now they do not anymore. Not prompting would be dangerous and against the spec.

Maybe a compromise would be (requestBody || !safe), although that would still prompt for PROPFIND/REPORT/SEARCH, which imho doesn't make any sense at all.

> The rest looks fine.

Thx.

I'll prepare the requested changes except for the last one -- what's the best venue to discuss that one?

Comment 19

[Julian Reschke]

Attachment: proposed patch, incl test case

This patch:

- adds a convenience function for finding out what to do with a redirect

- changes redirect following for 301/302 to rewrite *only* POST to GET (aligning with IE, and implementing what the IETF HTTPbis Working Group is planning to specify, see <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160>)

- adds test cases

NOTE: the tests have been written assuming we'd fix 676059 at the same time; as we do not, I have modified the expected results accordingly and moved the diverging ("better") results into comments

Comment 20

[Jason Duell]

> I'd rename RewriteRedirectMethodToGET to
> ShouldRewriteMethodToGETForRedirect, perhaps.

God that's long.  How about ShouldChangeRedirectToGET()? Or "ShouldRewriteRedirectToGET"? ("Method" seems inferable from GET).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 550223 [details] [diff] [review]
proposed patch, incl test case

r=me with Jason's suggestion dealt with.

Comment 22

[Anne (:annevk)]

(In reply to comment #18)
> The reason why I added this is because previously 301/302 redirected DELETE
> and PUT to GET, a safe method. Now they do not anymore. Not prompting would
> be dangerous and against the spec.

How is it more dangerous than using PUT or DELETE directly for which the specification requires nothing? (Prompting here is something end users will never understand.)

Comment 23

[Julian Reschke]

(In reply to comment #22)
> (In reply to comment #18)
> > The reason why I added this is because previously 301/302 redirected DELETE
> > and PUT to GET, a safe method. Now they do not anymore. Not prompting would
> > be dangerous and against the spec.
> 
> How is it more dangerous than using PUT or DELETE directly for which the
> specification requires nothing? (Prompting here is something end users will
> never understand.)

That's a separate question; if you want to discuss whether it should prompt at all then you should open a new bug.

Comment 24

[Julian Reschke]

Attachment: proposed patch, incl test case

This patch:

- adds a convenience function for finding out what to do with a redirect

- changes redirect following for 301/302 to rewrite *only* POST to GET (aligning with IE, and implementing what the IETF HTTPbis Working Group is planning to specify, see <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160>)

- adds test cases

NOTE: the tests have been written assuming we'd fix 676059 at the same time; as we do not, I have modified the expected results accordingly and moved the diverging ("better") results into comments

(change from previous patch: shorter function name as suggested by Jason)

Comment 25

[Anne (:annevk)]

(In reply to comment #23)
> That's a separate question; if you want to discuss whether it should prompt
> at all then you should open a new bug.

I think starting to prompt for these 301/302 redirects might be more incompatible than leaving it at not changing the request method.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Can we please move the prompting discussion to bug 676059?  It doesn't seem directly relevant to this bug.

Julian, once you post a final patch, don't forget to add the checkin-needed keyword!  Adding it here for now.

Comment 27

[Julian Reschke]

(In reply to comment #26)
> Julian, once you post a final patch, don't forget to add the checkin-needed
> keyword!  Adding it here for now.

Yep, will keep this in mind. Still learning.

Comment 28

[Jason Duell]

clearing checkin-needed until issue mentioned in bug 676059 comment 9 is resolved (safety of DELETES being followed across redirects w/o prompt)

Comment 29

[Jason Duell]

Comment on attachment 550321 [details] [diff] [review]
proposed patch, incl test case

I'm surprised that you've gotten XHR to work under xpcshell.  Last time we tried that didn't work.  Usually we use mochitests for XHR.  bz: any opinion on whether we're happy with xpcshell if it works? 

Could you please rename test_bug598304.js to test_XHR_redirect_safety.js? (stick the bug # at the top of the bug as a comment). I'm currently encouraging a policy of having human-parsable test names for test files.

Also, please add an e10s version of the test.  This is quite trivial--just look in the netwerk/test/unit_ipc directory and copy and existing wrapper script (and add it to the unit_ipc/xpcshell.ini file).  Make sure it works--your luck with xpcshell might possibly run out there (though I'm guessing it'll work).

Finally, how about a comment like this (assuming I'm correct):

"This file tests whether XmlHttpRequests correctly handle redirects, including rewriting POSTs to GETs, and prompting for redirects of other unsafe methods (such as PUTs, DELETEs, etc--see HttpBaseChannel::IsSafeMethod).  Since no prompting is possible in xpcshell, we get an error for prompts, and the request fails."

+r with those changes. (i.e. you can mark the patch as +r yourself when you attach it).

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #24)
> - changes redirect following for 301/302 to rewrite *only* POST to GET
> (aligning with IE, and implementing what the IETF HTTPbis Working Group is
> planning to specify, see
> <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160>)

I agree that the current behavior is wrong. But, would the new behavior safe? I can see that it would be OK for same-origin (and same-domain http -> https) redirects, which is what mnot's testcases test. But, should we really be issuing a DELETE to a resource hosted on foo.com based on a redirect sent from bar.com? It might be the case that we shouldn't treat redirects like this specially, instead restricting cross-origin requests with non-SAFE methods other than POST more generally (if any restrictions need to be made). The security considerations seem to be very similar to the security considerations for following redirects for WebSockets and I think both cases need similar implementations (and APIs).

Comment 31

[Boris Zbarsky [:bzbarsky]]

> bz: any opinion on whether we're happy with xpcshell if it works? 

I am.  XHR created via createInstance should generally work in random XPCOM components (and hence in xpcshell)...

Comment 32

[Julian Reschke]

(In reply to comment #30)
> I agree that the current behavior is wrong. But, would the new behavior
> safe? I can see that it would be OK for same-origin (and same-domain http ->
> https) redirects, which is what mnot's testcases test. But, should we really
> be issuing a DELETE to a resource hosted on foo.com based on a redirect sent
> from bar.com? It might be the case that we shouldn't treat redirects like

No, we should not. If it's the case then it's already broken (for 307 redirects).

Comment 33

[Julian Reschke]

Attachment: proposed patch, incl test case

This patch:

- adds a convenience function for finding out what to do with a redirect

- changes redirect following for 301/302 to rewrite *only* POST to GET (aligning with IE, and implementing what the IETF HTTPbis Working Group is planning to specify, see <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160>)

- adds test cases (both in unit and unit_ipc)

NOTE: the tests have been written assuming we'd fix 676059 at the same time; as we do not, I have modified the expected results accordingly and moved the diverging ("better") results into comments

Comment 34

[Jason Duell]

I think I agree with Brian that we need to implement restricting cross-origin requests with non-SAFE methods as part of landing this.  Does that seem doable within this bug?

Comment 35

[Julian Reschke]

(In reply to comment #34)
> I think I agree with Brian that we need to implement restricting
> cross-origin requests with non-SAFE methods as part of landing this.  Does
> that seem doable within this bug?

My understanding is that that restriction is already in place (I tried this earlier on).

Comment 36

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also, what about the case where the XHR starts off cross-origin and then gets redirected to same-origin? It seems like this must also be blocked by default, and/or that the script must be able to control whether it is blocked. Also, I am not sure what to do about redirection in the case where the target of the redirection would allow the request with CORS. Again, it seems like the script should be notified of the redirection and given the opportunity to allow or deny it. In other words, for script-originated requests like XHR, the prompting should be an interaction between Gecko and the script, not between Gecko and the user.

The current behavior is relatively safe *because* we rewrite the request to a GET.

Brandon, have these issues regarding security and handling of redirects been discussed elsewhere?

Comment 37

[Julian Reschke]

(In reply to comment #36)
> Also, what about the case where the XHR starts off cross-origin and then
> gets redirected to same-origin? It seems like this must also be blocked by
> default, and/or that the script must be able to control whether it is
> blocked. Also, I am not sure what to do about redirection in the case where
> the target of the redirection would allow the request with CORS. Again, it
> seems like the script should be notified of the redirection and given the
> opportunity to allow or deny it. In other words, for script-originated
> requests like XHR, the prompting should be an interaction between Gecko and
> the script, not between Gecko and the user.
> 
> The current behavior is relatively safe *because* we rewrite the request to
> a GET.
> 
> Brandon, have these issues regarding security and handling of redirects been
> discussed elsewhere?

All of these are good questions, but I don't believe this patch changes anything related to this. If there's a problem, it's already present for 307 redirects.

Comment 38

[Julian Reschke]

Can we discuss how to proceed with this after the next branching event? As far as I can tell, there are no outstanding issues with this.

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The comment "Note that unsafe methods should not follow the redirect automatically of the methods below, DELETE, POST and PUT are unsafe" is misleading. The real rule is at [1]. We need to make sure we are implementing the rule at [1]. In particular, we need cross-domain test cases.

[1] http://www.w3.org/TR/XMLHttpRequest/#infrastructure-for-the-send-method

Comment 40

[Julian Reschke]

(In reply to Brian Smith (:bsmith) from comment #39)
> The comment "Note that unsafe methods should not follow the redirect
> automatically of the methods below, DELETE, POST and PUT are unsafe" is
> misleading. The real rule is at [1]. We need to make sure we are
> implementing the rule at [1]. In particular, we need cross-domain test cases.
> 
> [1] http://www.w3.org/TR/XMLHttpRequest/#infrastructure-for-the-send-method

Well, the HTTP spec defines the rules for the protocol. The XHR spec defines the rules for the API. Hopefully, they are not in conflict. Note that we have yet another bug about the to-prompt-or-not-to-prompt issue (676434).

And yes, we need cross-domain tests; I started some for bug 676829, but I'm not entirely sure they test the right thing.

The changes I've made for this bug and for 676059 should make Firefox less buggy (with respect to HTTP), not change the security implications, and shouldn't change the conformance to XHR.

Comment 41

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

note to self: invite jonas/lucas/julian/bsmith/dveditz/imeleven

Comment 42

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

Note to Curtis: Would *love* to get Ian's input on how this affects plugins.

Comment 43

[Julian Reschke]

Plugins using NSAPI shouldn't be affected at all, because they can only do GET and PUT, right?

Comment 44

[Julian Reschke]

(In reply to Julian Reschke from comment #43)
> Plugins using NSAPI shouldn't be affected at all, because they can only do
> GET and PUT, right?

s/PUT/POST/

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Julian Reschke from comment #40)
> The changes I've made for this bug and for 676059 should make Firefox less
> buggy (with respect to HTTP), not change the security implications, and
> shouldn't change the conformance to XHR.

This may be true, but we need to verify that it doesn't make any non-conformance to XHR or other unsafe redirect handling worse. That is, before we make this change, we must verify that there aren't any (other) XHR conformance bugs regarding redirects.

Comment 46

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here is a discussion of the reaction to IE9's change in this area:
http://news.ycombinator.com/item?id=2903493
http://techno-weenie.net/2011/8/19/ie9-deletes-stuff/

Comment 47

[Eric Lawrence (@ericlaw)]

@Brian: I think you're confused. IE9 didn't "change" anything here. IE has followed the spec'd behavior for non-POST requests since the mid-1990s.

http://blogs.msdn.com/b/ieinternals/archive/2011/08/19/understanding-the-impact-of-redirect-response-status-codes-on-http-methods-like-head-get-post-and-delete.aspx

Comment 48

[Julian Reschke]

OK, what would be the next step to move this forward?

Comment 49

[Julian Reschke]

http://trac.tools.ietf.org/wg/httpbis/trac/ticket/160 -- the HTTPbis WG has now changed the definitions of 301/302 to allow rewriting of POST to GET -- note, only POST.

Comment 50

[Julian Reschke]

Attachment: patch updated for trunk

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 560038 [details] [diff] [review]
patch updated for trunk

r=me.  Thanks!

Comment 52

[Ed Morley [:emorley]]

Can this land with the pending sec-review-needed? Not so familiar with the process flow...

Comment 53

[Ed Morley [:emorley]]

(Sorry spam)
Also Julian, has this been sent to try?

Comment 54

[Julian Reschke]

(In reply to Ed Morley [:edmorley] from comment #53)
> (Sorry spam)
> Also Julian, has this been sent to try?

No, it hasn't.

I believe this one and 676059 should land back-to-back, and we may want to wait for that after Sep 27.

Comment 55

[Ed Morley [:emorley]]

Ah ok, thanks for the clarification :-)

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Can this land with the pending sec-review-needed?

Good question.  Julian, can you mail Curtis about that sec review, please?

Comment 57

[Julian Reschke]

Attachment: updated patch

Updates the previous patch which was missing a JS test file.

Also, adds more comments so the rules for what is being rewritten get more clear:

+// Return whether upon a redirect code of httpStatus for method, the
+// request method should be rewritten to GET. See RFC 2616, Section 8.3.
+PRBool
+HttpBaseChannel::ShouldRewriteRedirectToGET(PRUint32 httpStatus,
+                                            nsCOMPtr<nsIAtom> method)
+{
+  if (httpStatus == 301 || httpStatus == 302) {
+    // for 301 and 302, only rewrite POST
+    return method == nsHttp::Post;
+  }
+  else if (httpStatus == 303) {
+    // always rewrite for 303
+    return PR_TRUE;
+  }
+  else {
+    // otherwise, such as for 307, do not rewrite
+    return PR_FALSE;
+  }
+}

(this should not require a new review)

Comment 58

[Julian Reschke]

Attachment: proposed patch that applies with 477578 being backed out

Comment 59

[Julian Reschke]

For the record: a solution between the extremes "rewrite correctly" and "always rewrite to POST" would disable following redirects where right now the method would be rewritten incorrectly. That may not be compliant with XHR, but it would at least give script authors the option to do the right thing, where, right now, the browser silently breaks the request without the script even being able to find out (PUT->302->GET->200, script sees only final status code and assumes PUT was executed).

Comment 60

[Julian Reschke]

Attachment: proposed patch that applies with 477578 being backed out

Comment 61

[Julian Reschke]

Switching dependencies so this bug can be resolved after fixing 676059.

Comment 62

[Julian Reschke]

Attachment: Proposed patch, incl updated test case results

Applies on top of the patch for 676059.

Comment 63

[Julian Reschke]

Attachment: Proposed patch, incl updated test case results

Applies on top of the patch for 676059.

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 566051 [details] [diff] [review]
Proposed patch, incl updated test case results

r=me

Comment 65

[Julian Reschke]

Attachment: Proposed patch, incl updated test case results

Updated to apply on top of patch for bug 676059

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 566661 [details] [diff] [review]
Proposed patch, incl updated test case results

r=me

Comment 67

[Julian Reschke]

removed "[land along with bug 676059 after sept 27th merge]" from whiteboard; 676059 has landed in the meantime; this one is still pending security review

Comment 68

[Julian Reschke]

The equivalent Chrome bug - https://code.google.com/p/chromium/issues/detail?id=56373 - was fixed for Chrome 17.

Comment 69

[Julian Reschke]

Attachment: Proposed patch, incl updated test case results

This patch is identical to the previous one, except that it also fixes the 303 behavior for HEAD (which really should not be rewritten to GET).

Comment 70

[Julian Reschke]

Comment on attachment 573553 [details] [diff] [review]
Proposed patch, incl updated test case results

Try results: https://tbpl.mozilla.org/?tree=Try&rev=2812c40d3748

Comment 71

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

If you all are at a stage where this is nearing completion or a design has been chosen to fix the problem we really should schedule a security review.  Please take a look at the calendar (https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and let me know the date/time slot you would like.

Comment 72

[Julian Reschke]

(In reply to Curtis Koenig [:curtisk] from comment #71)
> If you all are at a stage where this is nearing completion or a design has
> been chosen to fix the problem we really should schedule a security review. 

I haven't seen any progress on this since we last talked. Maybe having this on the agenda for the purpose of finding out what's left to be done and who's going to do it would be good. The view on site compatibility impact should be different now from two months ago, given the fact that Chrome already has made the change (in beta as of next week AFAIU).

Comment 73

[Julian Reschke]

OK, the corresponding Chrome change is in Chrome 17, in beta as of today.

Should we re-schedule the security review?

Comment 74

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Yes please, just let me know when you want to do and whom to invite.

Comment 75

[Julian Reschke]

(In reply to Curtis Koenig [:curtisk] from comment #74)
> Yes please, just let me know when you want to do and whom to invite.

a) any of the next free morning PST slots would be good (-> evening over here)

b) I think the same people as mentioned in comment 41.

I've made a tiny update to 
<https://wiki.mozilla.org/Security/Reviews/XHRnonpost>, integrating information to the changes in HTTPbis and Chrome 17. Should I re-arrange stuff so that it's clear what's done (-> historic) and what's left to do?

Comment 76

[Julian Reschke]

(In reply to Julian Reschke from comment #75)
> (In reply to Curtis Koenig [:curtisk] from comment #74)
> > Yes please, just let me know when you want to do and whom to invite.
> 
> a) any of the next free morning PST slots would be good (-> evening over
> here)
> 
> b) I think the same people as mentioned in comment 41.
> 
> I've made a tiny update to 
> <https://wiki.mozilla.org/Security/Reviews/XHRnonpost>, integrating
> information to the changes in HTTPbis and Chrome 17. Should I re-arrange
> stuff so that it's clear what's done (-> historic) and what's left to do?

Can we add bug 714302 to that discussion as well?

Comment 77

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

(In reply to Julian Reschke from comment #75)
> I've made a tiny update to 
> <https://wiki.mozilla.org/Security/Reviews/XHRnonpost>, integrating
> information to the changes in HTTPbis and Chrome 17. Should I re-arrange
> stuff so that it's clear what's done (-> historic) and what's left to do?

Yes, Please, or we can start a new review page if you think that is easier.
bug 714302 seems related to me so I don't see why we can't cover it at the same review if we have time.

How about 12-Jan 10:00 AM PST?

Comment 78

[Julian Reschke]

(In reply to Curtis Koenig [:curtisk] from comment #77)
> (In reply to Julian Reschke from comment #75)
> > I've made a tiny update to 
> > <https://wiki.mozilla.org/Security/Reviews/XHRnonpost>, integrating
> > information to the changes in HTTPbis and Chrome 17. Should I re-arrange
> > stuff so that it's clear what's done (-> historic) and what's left to do?
> 
> Yes, Please, or we can start a new review page if you think that is easier.

I'll reuse the existing one.

> bug 714302 seems related to me so I don't see why we can't cover it at the
> same review if we have time.

Ack.

> How about 12-Jan 10:00 AM PST?

Sounds good to me.

Comment 79

[Ian Melven :imelven]

(In reply to Julian Reschke from comment #44)
> (In reply to Julian Reschke from comment #43)
> > Plugins using NSAPI shouldn't be affected at all, because they can only do
> > GET and PUT, right?
> 
> s/PUT/POST/

That sounds right to me on first thought. Additionally AIUI we were already doing this for POSTs so this shouldn't be different.

Note that the fix in bug 573873 should be re-verified with this change - but i don't see much risk for breakage since the behaviour shouldn't be different in this case.

The redirect notification API in NPAPI doesn't notify of the method, so plugins won't be able to know there's a change in HTTP method via that mean, this is already the case today of course.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 573553 [details] [diff] [review]
Proposed patch, incl updated test case results

r=me

Comment 81

[Julian Reschke]

(In reply to Boris Zbarsky (:bz) from comment #80)
> Comment on attachment 573553 [details] [diff] [review]
> Proposed patch, incl updated test case results
> 
> r=me

Thanks Boris.

I wonder what's left to be done with respect to security review?

With respect to web compat: Chrome 17, which has this fix, should be released soonish.

Comment 82

[Boris Zbarsky [:bzbarsky]]

No idea on the sec review.  Curtis?

Comment 83

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

I think only dveditz might have still had any concerns and if he gives this the "green flag" I think we are done.

Comment 84

[Julian Reschke]

(In reply to Curtis Koenig [:curtisk] from comment #83)
> I think only dveditz might have still had any concerns and if he gives this
> the "green flag" I think we are done.

Dan doesn't seem to be on the cc list for this bug...

Comment 85

[Boris Zbarsky [:bzbarsky]]

Well, just cc him, then!

Dan, any issues with this change?

Comment 86

[Daniel Veditz [:dveditz]]

I don't have any concerns that weren't already considered when this was brought up in IETF HTTP WG discussions. As long as browsers do different things here some authors are going to create vulnerable web apps. I don't feel strongly whether IE changes or if we and everyone else does.

Comment 87

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

[kw:sec-review-complete]
since there is no further concern from members of the security team I think we can flip this bit and move on.

Comment 88

[Julian Reschke]

(In reply to Daniel Veditz from comment #86)
> I don't have any concerns that weren't already considered when this was
> brought up in IETF HTTP WG discussions. As long as browsers do different
> things here some authors are going to create vulnerable web apps. I don't
> feel strongly whether IE changes or if we and everyone else does.

Thanks. Chrome 17 (currently in beta) behaves like IE. How about waiting for Chrome 17 to ship, wait 2..3 weeks for complaints, and then check this in?

Comment 89

[Patrick McManus [:mcmanus]]

(In reply to Julian Reschke from comment #88)
> (In reply to Daniel Veditz from comment #86)
> > I don't have any concerns that weren't already considered when this was
> > brought up in IETF HTTP WG discussions. As long as browsers do different
> > things here some authors are going to create vulnerable web apps. I don't
> > feel strongly whether IE changes or if we and everyone else does.
> 
> Thanks. Chrome 17 (currently in beta) behaves like IE. How about waiting for
> Chrome 17 to ship, wait 2..3 weeks for complaints, and then check this in?

given that info and the fact that you have sec-review and r+ I think you should check in now.. given the pipeline delay we don't need to worry about being ahead of Chrome 17 feedback. Just make sure to reply to any feedback we get on the non-release channels.

Comment 90

[Julian Reschke]

(In reply to Patrick McManus [:mcmanus] from comment #89)
> given that info and the fact that you have sec-review and r+ I think you
> should check in now.. given the pipeline delay we don't need to worry about
> being ahead of Chrome 17 feedback. Just make sure to reply to any feedback
> we get on the non-release channels.

OK. Do I need to check for feedback in specific places (is there a forum or a mailing list I should be watching?)

Comment 91

[Patrick McManus [:mcmanus]]

(In reply to Julian Reschke from comment #90)
> 
> OK. Do I need to check for feedback in specific places (is there a forum or
> a mailing list I should be watching?)

I just meant bugzilla. I will send you anything that looks relevant, as I'm sure others will too. (I'll just add it to my mental "Content-Disposition mumble mumble mumble - send to julian!" filter.)

Comment 92

[Julian Reschke]

(In reply to Patrick McManus [:mcmanus] from comment #89)
> (In reply to Julian Reschke from comment #88)
> > (In reply to Daniel Veditz from comment #86)
> > > I don't have any concerns that weren't already considered when this was
> > > brought up in IETF HTTP WG discussions. As long as browsers do different
> > > things here some authors are going to create vulnerable web apps. I don't
> > > feel strongly whether IE changes or if we and everyone else does.
> > 
> > Thanks. Chrome 17 (currently in beta) behaves like IE. How about waiting for
> > Chrome 17 to ship, wait 2..3 weeks for complaints, and then check this in?
> 
> given that info and the fact that you have sec-review and r+ I think you
> should check in now.. given the pipeline delay we don't need to worry about
> ...

...and Chrome 17 is released :-)

Comment 93

[Ed Morley [:emorley]]

(re: checkin-needed, I'm getting autoland to send this to try again, since the comment 70 try run was prior to the last try repo reset so the results are unavailable / likely too old to be of relevance anyway)

Comment 94

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 573553
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/2bbc9d91c6ed
Try run started, revision 2bbc9d91c6ed. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=2bbc9d91c6ed

Comment 95

[Mozilla RelEng Bot]

Try run for 2bbc9d91c6ed is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2bbc9d91c6ed
Results (out of 210 total builds):
    success: 178
    warnings: 17
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2bbc9d91c6ed

Comment 96

[Julian Reschke]

Alternate try results:

https://tbpl.mozilla.org/?tree=Try&rev=3b5609613b9b

Comment 97

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9525d7e2d20d

Comment 98

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9525d7e2d20d