Last Comment Bug 676059 - Make redirect prompting depend on HTTP-safeness of method, not presence of request body
: Make redirect prompting depend on HTTP-safeness of method, not presence of re...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Julian Reschke
:
Mentors:
Depends on:
Blocks: 598304 676829
  Show dependency treegraph
 
Reported: 2011-08-02 12:52 PDT by Julian Reschke
Modified: 2012-08-15 08:29 PDT (History)
6 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (incl updates to previously existing tests) (4.62 KB, patch)
2011-08-03 05:06 PDT, Julian Reschke
jduell.mcbugs: review+
mcmanus: review+
Details | Diff | Review
proposed patch (incl updates to previously existing tests) (4.63 KB, patch)
2011-08-04 06:30 PDT, Julian Reschke
no flags Details | Diff | Review
proposed patch (incl updates to previously existing tests), brought up-to-date with trunk (4.80 KB, patch)
2011-09-14 02:28 PDT, Julian Reschke
no flags Details | Diff | Review
proposed patch with code comments clarified (4.71 KB, text/plain)
2011-09-22 07:07 PDT, Julian Reschke
no flags Details
variant of the patch that applies after backout of 477578 (4.66 KB, patch)
2011-09-23 04:34 PDT, Julian Reschke
no flags Details | Diff | Review
variant of the patch that applies after backout of 477578 (4.65 KB, patch)
2011-10-05 09:18 PDT, Julian Reschke
no flags Details | Diff | Review
Updated patch, incl test cases (10.07 KB, patch)
2011-10-09 09:39 PDT, Julian Reschke
no flags Details | Diff | Review
Proposed patch, incl test case (10.07 KB, patch)
2011-10-10 15:07 PDT, Julian Reschke
bzbarsky: review+
Details | Diff | Review
Proposed patch, incl test case (10.34 KB, patch)
2011-10-12 15:05 PDT, Julian Reschke
bzbarsky: review+
Details | Diff | Review

Description Julian Reschke 2011-08-02 12:52:51 PDT
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."
Comment 1 Julian Reschke 2011-08-03 05:06:25 PDT
Created attachment 550351 [details] [diff] [review]
proposed patch (incl updates to previously existing tests)

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
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-03 08:51:31 PDT
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".
Comment 3 Julian Reschke 2011-08-03 08:56:38 PDT
(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 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-08-03 10:20:15 PDT
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...
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-03 10:40:50 PDT
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).
Comment 6 Julian Reschke 2011-08-03 10:54:26 PDT
(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 7 Patrick McManus [:mcmanus] 2011-08-03 12:50:53 PDT
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.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-03 15:39:58 PDT
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.
Comment 9 Jason Duell [:jduell] (needinfo? me) 2011-08-03 17:44:34 PDT
> > 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 10 Jason Duell [:jduell] (needinfo? me) 2011-08-03 18:07:03 PDT
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?)
Comment 11 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-08-03 19:09:39 PDT
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.
Comment 12 Julian Reschke 2011-08-03 23:53:27 PDT
(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.
Comment 13 Julian Reschke 2011-08-04 06:30:52 PDT
Created attachment 550673 [details] [diff] [review]
proposed patch (incl updates to previously existing tests)

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)
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-10 18:13:25 PDT
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.
Comment 15 Julian Reschke 2011-09-14 02:28:58 PDT
Created attachment 560118 [details] [diff] [review]
proposed patch (incl updates to previously existing tests), brought up-to-date with trunk
Comment 16 Anne (:annevk) 2011-09-14 03:23:01 PDT
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.
Comment 17 Julian Reschke 2011-09-14 04:27:07 PDT
(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 18 Jason Duell [:jduell] (needinfo? me) 2011-09-21 22:44:08 PDT
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, ...)"?
Comment 19 Julian Reschke 2011-09-21 23:42:47 PDT
(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.
Comment 20 Julian Reschke 2011-09-22 07:07:20 PDT
Created attachment 561725 [details]
proposed patch with code comments clarified

(this version of the patch fixes/clarifies the comments in the code)
Comment 21 Jason Duell [:jduell] (needinfo? me) 2011-09-22 11:59:02 PDT
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.
Comment 22 Julian Reschke 2011-09-22 12:07:18 PDT
(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.
Comment 23 Julian Reschke 2011-09-23 04:34:00 PDT
Created attachment 562017 [details] [diff] [review]
variant of the patch that applies after backout of 477578
Comment 24 Julian Reschke 2011-10-04 12:24:14 PDT
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).
Comment 25 Julian Reschke 2011-10-05 09:18:01 PDT
Created attachment 564877 [details] [diff] [review]
variant of the patch that applies after backout of 477578
Comment 26 Julian Reschke 2011-10-09 06:30:46 PDT
Switching bug dependency so that this bug can be resolved first.
Comment 27 Julian Reschke 2011-10-09 09:39:32 PDT
Created attachment 565811 [details] [diff] [review]
Updated patch, incl test cases

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 28 Honza Bambas (:mayhemer) 2011-10-10 10:37:54 PDT
Comment on attachment 565811 [details] [diff] [review]
Updated patch, incl test cases

>+    [301, "DELETE", "GET", 200], // but see bug 598394

Rather bug 598304 ?
Comment 29 Julian Reschke 2011-10-10 10:42:55 PDT
(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.
Comment 30 Julian Reschke 2011-10-10 15:07:26 PDT
Created attachment 566047 [details] [diff] [review]
Proposed patch, incl test case

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 31 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-10 18:13:06 PDT
I propose we WONTFIX this in favor of fixing bug 677754, which will make this a non-issue.
Comment 32 Julian Reschke 2011-10-11 07:21:21 PDT
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 33 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-12 11:49:45 PDT
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.
Comment 34 Julian Reschke 2011-10-12 15:04:09 PDT
(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?
Comment 35 Julian Reschke 2011-10-12 15:05:00 PDT
Created attachment 566644 [details] [diff] [review]
Proposed patch, incl test case

Patch adjusted as requested.
Comment 36 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-12 15:09:19 PDT
Sec rev triage, calling this one completed
Comment 37 Julian Reschke 2011-10-12 15:14:04 PDT
(In reply to Curtis Koenig [:curtisk] from comment #36)
> Sec rev triage, calling this one completed

So can this go to checkin-needed?
Comment 38 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-10-12 15:24:36 PDT
I don't see why not.
Comment 40 Ed Morley [:emorley] 2011-10-15 05:36:52 PDT
https://hg.mozilla.org/mozilla-central/rev/16d785492262

Note You need to log in before you can comment on or make changes to this bug.