Fixing 598304 will cause certain HTTP methods not being rewritten anymore to GET upon a 301/302. For instance, DELETE will be followed automatically, although being unsafe and quite destructive. On the other hand, PROPFIND will be prompted for (when it has a request body), although it is safe. Prompting really should depend on the safeness of the method; see HTTPbis, Part 2, Introduction to 3xx codes: "The action required MAY be carried out by the user agent without interaction with the user if and only if the method used in the second request is known to be "safe", as defined in Section 7.1.1."

This patch: - adds a convenience function for finding out about safe methods - adds a few WebDAV methods as atoms, so they can be detected as safe - changes the code that decided on prompting the user to do this based on the safeness of the method - updates the tests for 598304 with the now expected results

We should just remove all prompting, period. If following a redirect isn't something we can do without prompting, then we shouldn't follow the redirect. However, the http definition of "safe" is, or at least has traditionally been, useless as it has said for example that a same-origin DELETE isn't "safe".

(In reply to comment #2) > We should just remove all prompting, period. > > If following a redirect isn't something we can do without prompting, then we > shouldn't follow the redirect. > > However, the http definition of "safe" is, or at least has traditionally > been, useless as it has said for example that a same-origin DELETE isn't > "safe". I think a better approach would be a change/extension to XHR that gives the client code actual control over following redirects, in which case the client code could make a more informed decision about what the right thing do is. I realize that Boris has sent people over here to discuss prompting in general :-). However the intent of this bug was to fix how the code decides on prompting, not to remove it entirely. The current code is problematic in that it prompts when it doesn't need to, but does not when it should according to the spec.

Comment on attachment 550351 [details] [diff] [review] proposed patch (incl updates to previously existing tests) I'd rather someone more familiar with HTTP review this...

Well, I'd rather that we spend time nuking the prompting code entirely, rather than waste futzing around with details of it. I think as things stand we can simply remove any code from necko that prompts. We already have code in place elsewhere which makes us not follow harmful redirects for example from XHR requests. I don't think we've relied on the prompts in necko for any real security for a long time anyway, since we've always known that there's a good chance the user will simply click "yes" without understanding the implications of (especially since we don't provide nearly enough data to make a informed decision).

(In reply to comment #5) > Well, I'd rather that we spend time nuking the prompting code entirely, > rather than waste futzing around with details of it. Well, the patch removes prompts where they aren't needed and adds some in cases that didn't work prior to 598304 anyway (that's why I would have preferred to do this in one step). > I think as things stand we can simply remove any code from necko that > prompts. We already have code in place elsewhere which makes us not follow > harmful redirects for example from XHR requests. I don't believe this is the case. As far as I can tell, DELETEs are followed without prompting (in XHR). > I don't think we've relied on the prompts in necko for any real security for > a long time anyway, since we've always known that there's a good chance the > user will simply click "yes" without understanding the implications of > (especially since we don't provide nearly enough data to make a informed > decision). ...and of course, we might also look at making the prompts more informative. But anyway; please consider that this patch only adds prompting for scenarios that didn't work before at all, plus, as far as I can tell, for POST with empty bodies (where you really want to prompt *if* you want to prompt at all).

Comment on attachment 550351 [details] [diff] [review] proposed patch (incl updates to previously existing tests) I agree with julian - this makes http consistent in its prompting. I also agree we should probably get rid of all the prompting - but that can be considered separately.

Ok, as long as someone files a separate bug on all prompting I won't waste my time here. (In reply to comment #6) > > I think as things stand we can simply remove any code from necko that > > prompts. We already have code in place elsewhere which makes us not follow > > harmful redirects for example from XHR requests. > > I don't believe this is the case. As far as I can tell, DELETEs are followed > without prompting (in XHR). What are you basing this on? If we are following redirects DELETEs cross site that would be a horrible bug which would likely let you wreck havoc with several websites. If we in fact are, please file a bug ASAP as that's something that we need to fix on FF3.6.20 and FF6.

> > As far as I can tell, DELETEs are followed without prompting (in XHR). > > What are you basing this on? If we are following redirects DELETEs cross site > that would be a horrible bug which would likely let you wreck havoc with > several websites. Am I understanding that we're not currently following DELETES (instead turning them into GETs), but that your patch (bug 598304 comment 16) is going to change them to get followed (as per your reading of the spec), but that with *this* bug, we'll at least prompt the user? Are we happy with that? If so I think we should make sure we land both of these together :) Filed bug 676434 to cover removing prompts from necko.

Comment on attachment 550351 [details] [diff] [review] proposed patch (incl updates to previously existing tests) Review of attachment 550351 [details] [diff] [review]: ----------------------------------------------------------------- +r assuming we're ok with the DELETE issue (followed, but only if user accepts prompt), which I'm not sure we are (it seems like many users might blindly click "OK" and let DELETES proceed that aren't happy ones?)

We cannot ask the user to approve a redirected non-safe request. Virtually no users know what the question means. It might be necessary to do so for only POST for backward compatibility, but otherwise, if we do not feel comfortable doing the redirected action without asking permission, then we must not do it at all.

(In reply to comment #8) > > I don't believe this is the case. As far as I can tell, DELETEs are followed > > without prompting (in XHR). > > What are you basing this on? If we are following redirects DELETEs cross > site that would be a horrible bug which would likely let you wreck havoc > with several websites. Test cases (http://www.mnot.net/javascript/xmlhttprequest/). And no, I didn't say "cross site". (In reply to comment #9) > Am I understanding that we're not currently following DELETES (instead > turning them into GETs), but that your patch (bug 598304 comment 16) is > going to change them to get followed (as per your reading of the spec), but > that with *this* bug, we'll at least prompt the user? Yes. > Are we happy with that? If so I think we should make sure we land both of > these together :) That would be my preference.

This patch: - adds a convenience function for finding out about safe methods - adds a few WebDAV methods as atoms, so they can be detected as safe - changes the code that decided on prompting the user to do this based on the safeness of the method - updates the tests for 598304 with the now expected results (patch was updated because of test case renaming in 598304 test case)

By the way, is this prompt happening before or after we call the redirect handlers? In the vast majority of dangerous cases the redirect handlers will cancel the redirect so bothering the user with a prompt seems unnecessary.

FWIW, redirect prompting makes no sense at all given that there is no prompting going on for the requests to begin with. If we add the ability to XMLHttpRequest for authors to handle redirects it will be even more ridiculous as author controlled redirects would not prompt, whereas browser controlled redirects would.

(In reply to Anne van Kesteren from comment #16) > FWIW, redirect prompting makes no sense at all given that there is no > prompting going on for the requests to begin with. If we add the ability to > XMLHttpRequest for authors to handle redirects it will be even more > ridiculous as author controlled redirects would not prompt, whereas browser > controlled redirects would. This bug is about how to decide to prompt; not whether or not to prompt.

Comment on attachment 560118 [details] [diff] [review] proposed patch (incl updates to previously existing tests), brought up-to-date with trunk Review of attachment 560118 [details] [diff] [review]: ----------------------------------------------------------------- So this patch does what it says it does: pop up a prompt if we're redirecting while retaining an unsafe method (rather than re-writing it to GET). I suppose I could +r it for that. Except that I'm totally confused about whether this is the right thing. We've scattered our discussion about this over several bugs, but looking over it all, it looks like the following is the gist of what will happen if we land this and the patch for bug 598304: Current logic: we convert all "unsafe" methods to GET, except for 307 redirects, in which case we prompt the user if it's OK. This is not spec-compliant, and XHR users are complaining. New logic: we only convert unsafe methods for 303 redirects (except POST which we convert to GET for 301/302, for legacy compat). But for a 301/302/307 PUT/DELETE/POST, we ask the user for permission. This seems to just add more prompts to necko. In particular, it will now allow (confusing, footgun?) prompts for XHR PUT requests AFAICT. Meanwhile in bug 676434 and bug 677754 we all seem to agree that we want to remove all prompts from necko and simply drop the redirect in such cases. Is there some reason then that we want to move to this intermediate, more prompt-y behavior? Why not just go to failing all prompt-able redirects? Perhaps I'm missing something, and we're already checking and dropping unsafe redirects: jonas suggests as much in bug 677754 comment 0. But in that case shouldn't we be removing all the prompt code? I'm confused. Hopefully bz (who +r'd bug 598304) and the security team can make more sense of this than I've been able to. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1515,4 @@ > (httpStatus == 301 || httpStatus == 302) && (method == nsHttp::Post); > } > > +// Return whether the specified method is safe as per RFC 2616, Section 7.1.1. Typo: you mean section 9.1.1 ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +3346,4 @@ > PRBool rewriteToGET = HttpBaseChannel::ShouldRewriteRedirectToGET( > mRedirectType, mRequestHead.Method()); > > + // prompt if the method is not safe (HEAD, GET, PROPFIND, ...) confusing: not clear if the examples count as "not safe" or "safe" (yes, I figured out that they're "safe": but perhaps you see what I mean). How about "if method is unsafe (ex: POST, PUT, DELETE, ...)"?

(In reply to Jason Duell (:jduell) from comment #18) > Comment on attachment 560118 [details] [diff] [review] > proposed patch (incl updates to previously existing tests), brought > up-to-date with trunk Thanks for the feedback... > Current logic: we convert all "unsafe" methods to GET, except for 307 > redirects, in which case we prompt the user if it's OK. This is not > spec-compliant, and XHR users are complaining. Even worse: even safe methods (PROPFIND etc) are converted for 301/302. > New logic: we only convert unsafe methods for 303 redirects (except POST > which we convert to GET for 301/302, for legacy compat). But for a Not true: 303 behavior does not change. > 301/302/307 PUT/DELETE/POST, we ask the user for permission. Not true: the prompting does not depend on the status code but the safeness of the subsequent request. (This is the intent; if the patch doesn't do that, there's a bug). > This seems to just add more prompts to necko. In particular, it will now > allow (confusing, footgun?) prompts for XHR PUT requests AFAICT. Meanwhile For PUTs that get redirected. Yes. > in bug 676434 and bug 677754 we all seem to agree that we want to remove all > prompts from necko and simply drop the redirect in such cases. Is there > some reason then that we want to move to this intermediate, more prompt-y > behavior? Why not just go to failing all prompt-able redirects? I'm ok with not prompting, but letting the caller of XHR decide. But that's orthogonal to *this* bug; right now Firefox *does* prompt in some cases, and it just fixes the cases in which this happens. > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +1515,4 @@ > > (httpStatus == 301 || httpStatus == 302) && (method == nsHttp::Post); > > } > > > > +// Return whether the specified method is safe as per RFC 2616, Section 7.1.1. > > Typo: you mean section 9.1.1 Will fix. > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +3346,4 @@ > > PRBool rewriteToGET = HttpBaseChannel::ShouldRewriteRedirectToGET( > > mRedirectType, mRequestHead.Method()); > > > > + // prompt if the method is not safe (HEAD, GET, PROPFIND, ...) > > confusing: not clear if the examples count as "not safe" or "safe" (yes, I > figured out that they're "safe": but perhaps you see what I mean). How > about "if method is unsafe (ex: POST, PUT, DELETE, ...)"? Will fix.

(this version of the patch fixes/clarifies the comments in the code)

Julian, Thanks for clarification. I think my main question mark is about starting to prompt for XHR PUT redirects. If we're counting on the user to determine if they're safe, that seems bad. If we've already done some check to make sure they're safe (my knowledge of our XHR code is thin) then seems like we could skip prompting.

(In reply to Jason Duell (:jduell) from comment #21) > Thanks for clarification. I think my main question mark is about starting > to prompt for XHR PUT redirects. If we're counting on the user to determine > if they're safe, that seems bad. If we've already done some check to make > sure they're safe (my knowledge of our XHR code is thin) then seems like we > could skip prompting. Oh, we don't start to prompt for PUT requests in general. We *would* prompt for them if a PUT was redirected. It really doesn't make sense to prompt for a redirected POST but not a redirected PUT. It also doesn't make sense not to prompt for a redirected DELETE just because it doesn't have request body.

In the security review telco I had the impression that there may be rough consensus for this change, but not (yet) for 598304. Maybe we should swap the bug dependency and get this one resolved first? (It needs to be done no matter what the outcome of 598304 is).

Switching bug dependency so that this bug can be resolved first.

This patch changes the code that decides when to prompt to do that based on the HTTP-safeness of the method; it does NOT change the method rewriting behavior (which is bug 598304). It *does* adopt some of the code from the patch for 598304 in order to make things easier to understand (and change later on).

Comment on attachment 565811 [details] [diff] [review] Updated patch, incl test cases >+ [301, "DELETE", "GET", 200], // but see bug 598394 Rather bug 598304 ?

(In reply to Honza Bambas (:mayhemer) from comment #28) > Comment on attachment 565811 [details] [diff] [review] [diff] [details] [review] > Updated patch, incl test cases > > >+ [301, "DELETE", "GET", 200], // but see bug 598394 > > Rather bug 598304 ? Indeed. Will fix.

This patch changes the code that decides when to prompt to do that based on the HTTP-safeness of the method; it does NOT change the method rewriting behavior (which is bug 598304). It *does* adopt some of the code from the patch for 598304 in order to make things easier to understand (and change later on).

I propose we WONTFIX this in favor of fixing bug 677754, which will make this a non-issue.

The proposed patch: - adds a set of tests we didn't have before - changes the network code to move the prompting-decision into a single place for better future maintainability - improves specification conformance - fixes bug 677754 at least as observed from xpc I believe it would be good to get it into FF 10, even if more work will be done in the future.

Comment on attachment 566047 [details] [diff] [review] Proposed patch, incl test case Don't do else-after-return. So instead of: if (X) return a; else if (Y) return b; else return z; just use: if (X) return a; if (Y) return b; return z; With that nit, I think this looks fine. I think we should do this no matter what we decide to do with the prompting in general; it makes removing the prompting pretty simple.

(In reply to Boris Zbarsky (:bz) from comment #33) > Comment on attachment 566047 [details] [diff] [review] [diff] [details] [review] > Proposed patch, incl test case > > Don't do else-after-return. So instead of: > ... Ouch. Ok, will have to adapt. > ... > With that nit, I think this looks fine. I think we should do this no matter > what we decide to do with the prompting in general; it makes removing the > prompting pretty simple. Do we need another sec-review?

Patch adjusted as requested.

Sec rev triage, calling this one completed

(In reply to Curtis Koenig [:curtisk] from comment #36) > Sec rev triage, calling this one completed So can this go to checkin-needed?

I don't see why not.