Closed Bug 255119 Opened 17 years ago Closed 10 years ago
What other redirects do we block?
interesting, I stumbled on this piece of code recently. it seems to be this place here: http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#729 (the else-branch) Contrarily, it seems that for cached redirects, the page isn't displayed: http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpChannel.cpp#400 Any reason for this discrepancy? > What other redirects do we block? "any for which CheckLoadURI fails". (or the http event sink)
Fixing this for data: URLs without also fixing bug 211999 would break the current version of Hixie's data URI kitchen <http://software.hixie.ch/utilities/cgi/data/data>.
Whiteboard: security xss parity-opera parity-ie → [sg:moderate] xss parity-opera parity-ie
Assignee: darin → cbiesinger
Status: NEW → ASSIGNED
Attachment #210969 - Flags: review?(darin)
16 years ago
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8.1
Hmm... doesn't this cause problems for 300 responses that lack a Location header? Also seems like a problem for 307 POSTs that get redirected and result in a prompt. The page sent with a redirect response may be intended to be shown to the user in some cases. At least, that seems to be the intent of HTTP/1.1.
Comment on attachment 210969 [details] [diff] [review] so is this the desired behaviour? marking review- based on previous comment.
Attachment #210969 - Flags: review?(darin) → review-
Christian - since you haven't touched this in a while I'm reassigning to nobody so that we can easily tell we need a new owner. Let me know if you still want to work on it.
Status: ASSIGNED → NEW
detects DOM_BAD_URI error, which currently just aborts the redirection and not the original transaction, and replaces it with CORRUPTED_CONTENT - which is the error page we use for header response smuggling and other cases where the HTTP response isn't acceptable to process. It could be made fancier, but this is a real edge case certainly aligned with ill intent, so I think this is fine. Previous patch to this bug was rejected because it cleared the page for all redirections where we could not follow the location header (perhaps there was no such header, or perhaps it couldn't be reached) and that was too broad. This restricts itself to cases where we _choose_ not to chase the location header because of policy.
Try run for f56633230ec2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=f56633230ec2 Results (out of 39 total builds): success: 34 warnings: 5 Builds available at http://email@example.com
Comment on attachment 560670 [details] [diff] [review] patch 1 phhhhbbbbt. (try fail/leak).
Attachment #560670 - Flags: review?(jduell.mcbugs) → review-
Try run for 69f7a4ca99a7 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=69f7a4ca99a7 Results (out of 3 total builds): exception: 1 failure: 2 Builds available at http://firstname.lastname@example.org
Try run for aee95dffd411 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=aee95dffd411 Results (out of 41 total builds): success: 39 warnings: 2 Builds available at http://email@example.com
comment 14 is a valid try run (modulo unrelated random orange). This version restricts the body suppression to redirects rejected for bad-uri reasons that are trying to change the protocol to something non-http.
Attachment #561183 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Attachment #561183 - Flags: review?(honzab.moz) → review+
> > see bug 255119. > bz doesn't like referring bugs. I didn't ask him why IIRC the idea is that generally one can get the bug number from a piece of logic by looking at hg blame.
Thanks Honza! (In reply to Honza Bambas (:mayhemer) from comment #16) > BTW, this also affects loading file:/// and actually any local or resource > URI, probably not an issue, actually a nice benefit? yes, done by design. > > Some |bool NS_SchemeIs(nsIChannel* channel, const char* scheme)| function I didn't do that here, but we should add something along those lines. (In reply to Jason Duell (:jduell) from comment #17) > IIRC the idea is that generally one can get the bug number from a piece of > logic by looking at hg blame. IMO, that's really the wrong trade off when trying to explain a piece of logic. I'm not saying every change needs a bug # in the code, I'm just saying if it helps explain the logic beyond what is reasonable to comment then a bug number is a useful footnote. hg blame is useful, but it is not equivalent - it can often be quite a bit of pain due to reorgs, history purges (already happened once, I heard discussion of doing it again during work week as part of a hypothetical transition to git) and similar things.
Status: NEW → ASSIGNED
Whiteboard: [sg:moderate] xss parity-opera parity-ie → [sg:moderate] xss parity-opera parity-ie [inbound]
bz: Patrick: I agree that there are times when citing a bug # in a comment is the tersest way to explain what some code is up to, and that hg blame is a potentially transient way to be able to look it up. So I'm not opposed to it always (just only when needed). Yes, this is a terrible forum for this. I suppose this could go all the way to dev.platform or some other large forum. I'm trying to get work done! :)
The usual problem is that if we had a bug# with every checkin or even every comment, we'd have bug numbers all over the code. Sometimes ones which are no longer relevant (in that the code has changed but the comment has not). We in fact have that problem in some modules. So the question is where to draw the line. That's usually up to whoever gets to maintain the code. ;)
IMO, instead of bug# there should be a comment how you came up to the solution, where the error takes from and why use this error only, why not to convert it to content load denial when redirecting to http(s), but do that for any other protocol, etc. A good reasonably long comment explaining it in a simple english is good think to have here, because the change is not obvious. More comments is always good..
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [sg:moderate] xss parity-opera parity-ie [inbound] → [sg:moderate][xss] [parity-opera][parity-ie]
Target Milestone: --- → mozilla9
Documented: https://developer.mozilla.org/en/HTTP#More_on_redirection_responses And mentioned on Firefox 9 for developers.
You need to log in before you can comment on or make changes to this bug.