Closed
Bug 677754
Opened 13 years ago
Closed 11 years ago
Remove all redirects prompts from Necko
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: sicking, Assigned: valentin)
References
Details
Attachments
(2 files, 1 obsolete file)
1.41 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
6.02 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•13 years ago
|
||
I thought that some redirects/method pairs were inherently unsafe and we either had to prompt or automatically reject them...
Comment 2•13 years ago
|
||
Some are unsafe, so if we do not prompt FF should return the 3xx code the the caller.
Reporter | ||
Comment 3•13 years ago
|
||
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?
Comment 4•13 years ago
|
||
"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.
Comment 5•13 years ago
|
||
(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.
Reporter | ||
Comment 6•13 years ago
|
||
(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?
Comment 7•13 years ago
|
||
(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).
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 8•13 years ago
|
||
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>.
Comment 9•11 years ago
|
||
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.
Comment 10•11 years ago
|
||
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...
Comment 11•11 years ago
|
||
It's a nuisance. I suspect it might be a reason for people to avoid using 307 and 308 status codes.
Comment 12•11 years ago
|
||
(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>.
Updated•11 years ago
|
Flags: firefox-backlog?
Comment 13•11 years ago
|
||
(the Firefox backlog flag is typically not relevant to "Core" bugs - other teams use different processes for tracking work)
Flags: firefox-backlog? → firefox-backlog-
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8430392 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•11 years ago
|
Attachment #8430392 -
Flags: review?(jduell.mcbugs) → review?(mcmanus)
Updated•11 years ago
|
Attachment #8430392 -
Flags: review?(mcmanus) → review?(jduell.mcbugs)
Comment 15•11 years ago
|
||
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.
Attachment #8430392 -
Flags: review?(jduell.mcbugs) → review+
Comment 16•11 years ago
|
||
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
Assignee | ||
Comment 17•11 years ago
|
||
(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.
Assignee | ||
Comment 18•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8430392 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Comment 19•11 years ago
|
||
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.
Attachment #8443633 -
Flags: review+
Assignee | ||
Comment 20•11 years ago
|
||
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
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Comment 23•11 years ago
|
||
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.
Attachment #8443834 -
Flags: review?(jduell.mcbugs)
Flags: needinfo?(valentin.gosu)
Assignee | ||
Comment 24•11 years ago
|
||
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=9eae470ecbfb
Comment 25•11 years ago
|
||
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/
Attachment #8443834 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 26•11 years ago
|
||
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e4d09c5643af
https://hg.mozilla.org/mozilla-central/rev/691e52f72eca
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•