966216 - Restore non-anonymous requests made via XMLHttpRequest(mozSystem) for trusted apps

Status: RESOLVED INCOMPLETE

(Core :: DOM: Device Interfaces, defect)

Description

[Jose Manuel Cantera]

The recent landing of anonmymous mode for SystemXHR requests made from B2G has caused significant regressions in terms of functionality and UX, as we cannot logout easily and conveniently from external services, (Facebook, Gmail, Hotmail). This has been tracked for instance in Bug 962377.

Unfortunately window.open is not always a solution in terms of UX or performance, as reported in Bug 962377. Thus, we are requesting to restore the non-anon mode as it were in version 1.2. We can understand the sec reasons behind it, so here we have two options:

add a modifier to the systemXHR permission to request additionally non-anon mode or only allow certified apps to go through non-anon mode.

Comment 1

[Jason Smith [:jsmith]]

Paul - Are you okay with doing this?

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Since when were System XHR requests allowed to contain credentials? The original design was that these requests were forced to be anonymous (https://bugzilla.mozilla.org/show_bug.cgi?id=692677#c68)
Did this change? It really shouldn't have, as it changes the risk profile of this permissions dramatically.

On one hand, I really don't like have to allow systemxhr with credentials at all - my understanding was that this API was for use in retrieving public resources, not for interacting with APIs on other domains. But on the flipside, on other APIs my understanding is this level of access is more or less part and parcel with the ability to make a WebView. 

Jonas - since you proposed the initial API, what are your thoughts on this? Is non-mode a necessary risk?

Comment 3

[Paul Theriault [:pauljt] (no longer reading bugmail)]

To clarify: "but on the flipside, on other APIs my understanding is this level of access is more or less part and parcel with the ability to make a WebView. " I mean on other mobile platforms not other APis, ie IoS, Android etc. Ie do we need this for feature parity?

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Paul Theriault [:pauljt] from comment #2)
> Since when were System XHR requests allowed to contain credentials? The
> original design was that these requests were forced to be anonymous
> (https://bugzilla.mozilla.org/show_bug.cgi?id=692677#c68)
> Did this change? It really shouldn't have, as it changes the risk profile of
> this permissions dramatically.

The documentation said so, but the mozAnon flag wasn't being set on mozSystem. So since most people didn't read the doc they just assumed that mozSystem === no CORS headers but otherwise same behavior than normal XHR. 

BTW, I did saw that it wasn't working as documented, sometime in 2012 while debugging an unrelated problem but I assumed that the *documentation* was incorrect since considering that back at that time mozSystem was a certified only permission, and then it was raised only to privileged, that seemed like a sensible behavior to me. After all, why would we even  need a permission to access public web resources?

We have another privileged permission, which is TCPSocket, which allows to mimic the old behavior of SystemXHR. It will take more work (for the first guy that actually makes it, the rest will just reuse his code I guess) since it would have to implement HTTP on top of the Socket and take care of reading/resending the cookies, but can be done (which means that someone at someplace will undoubtedly do it).

I know that what we were allowing was akin to XSRF. No, bar that, it *was* XSRF all the way. Still, that was why it was behind a permission, and why we have reviews for privileged apps. In any case, and if you still think this permission is too risky for privileged apps, can we get a certified only, ReallySystemXHR permission, that works like the old one?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

As I read the docs it sounds like you should still be able to add request headers manually to anonymous XHR requests. Is that an option here?

Comment 6

[Jose Manuel Cantera]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #5)
> As I read the docs it sounds like you should still be able to add request
> headers manually to anonymous XHR requests. Is that an option here?

I don't think so. We need to send back the same cookies we were set when the user was authenticated. Otherwise the logout will not proceed

Comment 7

[Jason Smith [:jsmith]]

Given that the blocking bug was able to land without this fix, I don't think we need to block on this anymore.

Paul & Jonas - If you guys feel otherwise, then feel free to renominate.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm ok with allowing certified apps being able to include credentials with system-xhr requests.

So we could add yet another manifest permission for 'systemXHR with credentials' and only if that permission has been granted do we not automatically set anon to true when system is true.

It's definitely a pretty hacky solution though. It's a bad idea to have the same piece of code (i.e. "new XHR({mozSystem: true})") do different things in different apps depending on permissions.

But I think a proper fix would be pretty complex. Ideally we'd be able to create some sort of "cookie jar" object which could be used in both the mozbrowser API and in the systemXHR API. That way cookies can be shared between a mozbrowser call and a separate systemXHR call.

Comment 9

[Jose Manuel Cantera]

(In reply to Jonas Sicking (:sicking) from comment #8)
> I'm ok with allowing certified apps being able to include credentials with
> system-xhr requests.
> 
> So we could add yet another manifest permission for 'systemXHR with
> credentials' and only if that permission has been granted do we not
> automatically set anon to true when system is true.
> 

I'm happy to hear this. Can we implement this for v1.4? if so we can restore the nice UX with logouts in importers. 

> It's definitely a pretty hacky solution though. It's a bad idea to have the
> same piece of code (i.e. "new XHR({mozSystem: true})") do different things
> in different apps depending on permissions.
> 

can we have something like new XHR({mozSystem: true, anon: false}), using that syntax we can be coherent between the runtime request and the granted permissions

> But I think a proper fix would be pretty complex. Ideally we'd be able to
> create some sort of "cookie jar" object which could be used in both the
> mozbrowser API and in the systemXHR API. That way cookies can be shared
> between a mozbrowser call and a separate systemXHR call.

Could that fix be ready in 1.4 timeframe?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Unfortunately for backwards compatibility we most likely need to make 
"new XHR({mozSystem: true, anon: false})" not throw an exception, even when called from normal privileged apps. So we are more or less forced to treat that as "new XHR({mozSystem: true, anon: true})".

We could add something like "new XHR({mozCertifiedSystem: true})" which would create a non-anonymous system-xhr, and only permit that from certified apps that has a certifiedSystemXHR permission in the manifest. That's slightly less hacky.

Something like that could probably be done in time for v1.4. So that would be enough to bring back whatever features we had to drop.

The full cookiejar API solution is most likely not something we could do for v1.4 though.

Comment 11

[Preeti Raghunath(:Preeti)]

Based on Comment # 10 moving to 1.5

Comment 12

[Jason Smith [:jsmith]]

Not a blocker for the release, as we've shipped with this issue previously.

Comment 13

[(not reading bugmail) Nick Desaulniers [:\n]]

Should setting the withCredentials attribute of a xhr instance [0] with mozSystem set to true allow for this? 

[0] https://dvcs.w3.org/hg/xhr/raw-file/default/xhr-1/Overview.html#the-withcredentials-attribute

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

.withCredentials is specifically about CORS behavior. So it doesn't just affect which cookies are sent, but also what CORS headers to expect in the response.

So I'd rather not overload that property here.

I'd prefer the solution in comment 10. Or create some sort of cookiejar support.

Comment 15

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Cleaning up Device Interfaces component, and mass-marking old FxOS bugs as incomplete.

If any of these bugs are still valid, please let me know.