Closed Bug 718486 Opened 8 years ago Closed 8 years ago
Make Account Provisioner XML handler only request the XML once
This is a pretty serious bug that made itself known once we started live-testing against some of our account providers. For one thing, the old code only considered XML where the contentType exactly matched "text/xml". Next, it looks like we requested the XML twice - once to determine the content type, and a second time in order to invisibly parse / deal with the XML content. That doesn't work if our providers dynamically generate the XML, and don't allow us to re-request it.
Assignee: nobody → mconley
Here's my first pass at a solution. This is quite extensive for something we'd likely want to land on Aurora, but unfortunately, I don't think this can be helped. Instead of stopping the request as soon as we detect the contentType, and re-requesting, I use the following approach: 1. Once the order form tab opens, wire up an observer to watch for any and all HTTP requests going in and out of Thunderbird (this observer is automatically unregistered once the order form tab is closed) 2. For each HTTP request, watch for one where the contentType *starts* with the string "text/xml". When we see one, ensure that the request is associated with the order form tab, and then attach an nsITraceableChannel listener to the request 3. The nsITraceableChannel listener does the job of assembling the XML and trying to parse it. I have to stress that this is the first time I've ever had to work with nsITraceableChannel, and the input/output streams/buffers in this code. Most of this work is based upon examples found in http://www.softwareishard.com/blog/firebug/nsitraceablechannel-intercept-http-traffic/, Firebug, and this file: http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/test/unit/test_traceable_channel.js
Fixing a typo, putting back a log message that I accidentally chopped out.
Attachment #588950 - Attachment is obsolete: true
I should also point out that with this patch, I can successfully get an account from Hover.
Will this need security review ?
Comment on attachment 588953 [details] [diff] [review] Patch v2 Hey Blake - let me know if I should redirect this review request elsewhere.
Attachment #588953 - Flags: review?(bwinton)
Ludo: I'm unsure, but I think I'm leaning towards "yes". -Mike
Attachment #588953 - Flags: review?(bwinton) → review?(dbienvenu)
Comment on attachment 588953 [details] [diff] [review] Patch v2 I can't test this, but it looks OK in general - but the scoping issues with the services includes are new and surprising to me. Are there other examples in the code of that happening?
Attachment #588953 - Flags: review?(dbienvenu) → review+
(In reply to David :Bienvenu from comment #7) > Comment on attachment 588953 [details] [diff] [review] > Patch v2 > > I can't test this, but it looks OK in general - but the scoping issues with > the services includes are new and surprising to me. Are there other examples > in the code of that happening? They were a surprise to me too. I'm not sure why they happen, I just know that they do. :/ It might have to do with the Account Provisioner running within a modal dialog, and therefore possibly having a scope / event loop completely separated from the main one. Ludo marked this bug as sec-review-needed...should I just hold on committing this until we receive a security review?
(In reply to Mike Conley (:mconley) from comment #8) > Ludo marked this bug as sec-review-needed...should I just hold on committing > this until we receive a security review? We should probably talk to some of the security guys to get a faster review adding Curtis to the bug list.
Is there anything else I need to do in order to receive a security review on this?
Comment on attachment 588953 [details] [diff] [review] Patch v2 Review of attachment 588953 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/newmailaccount/content/uriListener.js @@ +105,2 @@ > > + if (contentType.indexOf("text/xml") != 0) nsIHttpChannel doesn't do case-normalization of header values from what I know. This may not be an issue in practice if the current service providers return a lowercase Content-Type.
The security review will be for bug #686347 . There isn't any further security work that needs to be done for this particular bug. Mike: Can you address comment 11 ? It may be a potential functionality bug but shouldn't have security implications.
Thanks for pointing that out, dchan. I'm running the content type through toLowerCase() first now, before checking for text/xml at index 0.
I've taken the security review comments from bug 686347, and made the corrections in this patch. I think this is ready to land on Aurora now.
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/5608d36000c9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #592263 - Flags: approval-comm-aurora? → approval-comm-aurora+
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/070572a7da3d
You need to log in before you can comment on or make changes to this bug.