718486 - Make Account Provisioner XML handler only request the XML once.

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

Attachment: 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) (:⚙️)]

Attachment: Patch v2

Fixing a typo, putting back a log message that I accidentally chopped out.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

I should also point out that with this patch, I can successfully get an account from Hover.

Comment 4

[Ludovic Hirlimann [:Usul]]

Will this need security review ?

Comment 5

[Mike Conley (:mconley) (:⚙️)]

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) (:⚙️)]

Ludo:

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

-Mike

Comment 7

[David :Bienvenu]

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) (:⚙️)]

(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]]

(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) (:⚙️)]

Is there anything else I need to do in order to receive a security review on this?

Comment 11

[David Chan [:dchan]]

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]]

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) (:⚙️)]

Attachment: 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) (:⚙️)]

Attachment: 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) (:⚙️)]

Committed to comm-central as http://hg.mozilla.org/comm-central/rev/5608d36000c9

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/070572a7da3d