Last Comment Bug 718486 - Make Account Provisioner XML handler only request the XML once.
: Make Account Provisioner XML handler only request the XML once.
Status: RESOLVED FIXED
[secr:dchan]
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: 11 Branch
: x86 All
: -- major (vote)
: Thunderbird 12.0
Assigned To: Mike Conley (:mconley) - (Needinfo me!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-16 11:48 PST by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-08-15 08:32 PDT (History)
5 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch v1 (18.52 KB, patch)
2012-01-16 11:54 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v2 (18.64 KB, patch)
2012-01-16 11:56 PST, Mike Conley (:mconley) - (Needinfo me!)
mozilla: review+
Details | Diff | Splinter Review
Patch v3 (r+'d by bienvenu) (18.65 KB, patch)
2012-01-25 15:23 PST, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Patch v4 (r+'d by bienvenu) (19.44 KB, patch)
2012-01-27 14:01 PST, Mike Conley (:mconley) - (Needinfo me!)
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 11:48:38 PST
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.
Comment 1 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 11:54:00 PST
Created attachment 588950 [details] [diff] [review]
Patch v1

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
Comment 2 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 11:56:52 PST
Created attachment 588953 [details] [diff] [review]
Patch v2

Fixing a typo, putting back a log message that I accidentally chopped out.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 11:57:25 PST
I should also point out that with this patch, I can successfully get an account from Hover.
Comment 4 Ludovic Hirlimann [:Usul] 2012-01-16 11:58:26 PST
Will this need security review ?
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 11:59:57 PST
Comment on attachment 588953 [details] [diff] [review]
Patch v2

Hey Blake - let me know if I should redirect this review request elsewhere.
Comment 6 Mike Conley (:mconley) - (Needinfo me!) 2012-01-16 12:03:20 PST
Ludo:

I'm unsure, but I think I'm leaning towards "yes".

-Mike
Comment 7 David :Bienvenu 2012-01-17 07:55:58 PST
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?
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-01-17 08:02:07 PST
(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?
Comment 9 Ludovic Hirlimann [:Usul] 2012-01-17 08:11:36 PST
(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.
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-01-23 07:46:31 PST
Is there anything else I need to do in order to receive a security review on this?
Comment 11 David Chan [:dchan] 2012-01-24 16:02:39 PST
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.
Comment 12 David Chan [:dchan] 2012-01-25 14:44:08 PST
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.
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-01-25 15:23:03 PST
Created attachment 591631 [details] [diff] [review]
Patch v3 (r+'d by bienvenu)

Thanks for pointing that out, dchan.  I'm running the content type through toLowerCase() first now, before checking for text/xml at index 0.
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-01-27 14:01:15 PST
Created attachment 592263 [details] [diff] [review]
Patch v4 (r+'d by bienvenu)

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.
Comment 15 Mike Conley (:mconley) - (Needinfo me!) 2012-01-27 14:04:04 PST
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/5608d36000c9
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-01-27 15:15:34 PST
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/070572a7da3d

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