We should remove all prompts about "unsafe" redirects from necko. Rather than prompting we should simply follow the redirect. We already have code elsewhere that checks outside of necko that checks redirects and blocks them if they risk causing websites to be hacked. For example the XHR code that issues a DELETE command checks if a same-origin DELETE is turned into a cross-origin DELETE. Or if a "CORS-checked" cross-origin DELETE is redirected to a server which we haven't "CORS-checked". So lets just remove the necko prompts for redirects and follow the redirect.

I thought that some redirects/method pairs were inherently unsafe and we either had to prompt or automatically reject them...

Some are unsafe, so if we do not prompt FF should return the 3xx code the the caller.

Can someone explain to me what redirects are inherently unsafe? How long have we been putting up prompts rather than rejecting/changing them to something that's not unsafe?

"If the 307 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued." 307 redirects must not change a POST to a GET (as 302/303 do), but it's not safe to redirect a POST without prompting. Although I think we should probably just reject the redirect and not prompt in that case, given that a prompt is just a footgun.

(In reply to Jonas Sicking (:sicking) from comment #3) > Can someone explain to me what redirects are inherently unsafe? > ... I would consider a redirect unsafe if and only if the redirected request is unsafe (as per HTTP spec). For 307, this is the case for all unsafe methods. For 303, this is never the case (it's always rewritten to HEAD or GET). For 301/302, this is *currently* never the case (because of the non-compliant rewriting to GET). With the patch for bug 598304, it will be almost the same as for 307 (except for POST which will continue to be rewritten to GET). Note that that change 598304 only affects XHR; forms do not cause requests other than GET or POST for now.

(In reply to Julian Reschke from comment #5) > I would consider a redirect unsafe if and only if the redirected request is > unsafe (as per HTTP spec). So would you also consider a unredirected DELETE request unsafe? Doesn't the http spec say that any such request is unsafe? Or to put it another way, is the redirect any more unsafe than the original request?

(In reply to Jonas Sicking (:sicking) from comment #6) > (In reply to Julian Reschke from comment #5) > > I would consider a redirect unsafe if and only if the redirected request is > > unsafe (as per HTTP spec). > > So would you also consider a unredirected DELETE request unsafe? Doesn't the > http spec say that any such request is unsafe? > > Or to put it another way, is the redirect any more unsafe than the original > request? From the protocol's point of view, both are unsafe. What's important here is that automatically applying the redirect without conformation means applying an unsafe methods to a URI different from the requested one (and, in XHR, without any way for the call to find out).

See also <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/238>, which is resolved in <http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-19.html#rfc.section.7.3>.

We should fix this. Chrome does not prompt for http://dump.testsuite.org/xhr/upload-redirect.html either. We don't ask the user whether we can make the initial unsafe request, we should not ask for redirected ones either.

OK, we'll fix it. Just to get an idea of the priority here, is there a reason I can't ever recall personally seeing such a prompt? I suspect this is rarely used, so I've added it to our necko backlog, but not near the top...

It's a nuisance. I suspect it might be a reason for people to avoid using 307 and 308 status codes.

(In reply to Anne (:annevk) from comment #11) > It's a nuisance. I suspect it might be a reason for people to avoid using > 307 and 308 status codes. It happens for 301 and 302 as well (except for POST which is rewritten to GET). See <http://greenbytes.de/tech/tc/httpredirects/#t302methods>.

(the Firefox backlog flag is typically not relevant to "Core" bugs - other teams use different processes for tracking work)

Comment on attachment 8430392 [details] [diff] [review] no_redirect_prompts.patch Review of attachment 8430392 [details] [diff] [review]: ----------------------------------------------------------------- Wow--it's that easy? I assume you've manually tested it (including for 301/302, as comment 12 mentions)? If so, then +r.

Actually, the comment on the preference should be tweaked as well, such as: // Prompt for 307/308 redirects or // Prompt for redirects resulting in unsafe HTTP requests

(In reply to Jason Duell (:jduell) from comment #15) > Wow--it's that easy? I assume you've manually tested it (including for > 301/302, as comment 12 mentions)? If so, then +r. I ran the method tests at http://greenbytes.de/tech/tc/httpredirects/ and confirmed that the prompts don't happen anymore with the pref turned off.

Comment on attachment 8443633 [details] [diff] [review] Remove all redirects prompts from Necko Review of attachment 8443633 [details] [diff] [review]: ----------------------------------------------------------------- OK, then it looks like we're good.

I had to back this out for xpcshell bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=42163656&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/367d902bd3b0

Comment on attachment 8443834 [details] [diff] [review] Fix test expecting a prompt for an unsafe redirect Forgot to fix the test checking for a prompt. Now the test checks the pref and does the validation accordingly.

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=9eae470ecbfb

Comment on attachment 8443834 [details] [diff] [review] Fix test expecting a prompt for an unsafe redirect Review of attachment 8443834 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/test/unit/test_XHR_redirects.js @@ +35,5 @@ > + if (sRedirectPromptPref) { > + // The method is null if we prompt for unsafe redirects > + method = null; > + } else { > + // The status code is 200 when we don't propt for unsafe redirects s/propt/prompt/

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d09c5643af https://hg.mozilla.org/integration/mozilla-inbound/rev/691e52f72eca

https://hg.mozilla.org/mozilla-central/rev/e4d09c5643af https://hg.mozilla.org/mozilla-central/rev/691e52f72eca