1196021 - Use channel->ascynOpen2 in netwerk/protocol/http/PackagedAppService.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1196021_packagedappservice.patch

Jonas, Bug 1187159 passes along a principal and flags from the requesting channel to GetResource(). I think it should rather pass the loadInfo along; there is only one callsite [1] and the tests [2].

What do you think?

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#5221
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_packaged_app_service.js#182

Comment 2

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

When we are fetching the package, we should definitely use all of the same loadinfo data as what was passed in when we initiated the load of the subresource in the package.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> When we are fetching the package, we should definitely use all of the same
> loadinfo data as what was passed in when we initiated the load of the
> subresource in the package.

Valentin, any chance you wanna implement that update and pass the loadInfo along instead of the principal?
Within GetResource you could then use NS_NewChannelInternal() wich takes a loadInfo as an argument.

Or do you rather want me to do the update?

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Hi Christoph,
I'm currently looking into that. I'm getting an assertion in nsContentSecurityManager.cpp:21 because apparently the loadInfo in nsHttpChannel doesn't have a security flag set.
Apart from that, do we have a way of creating nsILoadInfo in xpcshell tests? I'd hate to have different code paths for unit tests and real content.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Valentin Gosu [:valentin] from comment #4)
> Hi Christoph,
> I'm currently looking into that. I'm getting an assertion in
> nsContentSecurityManager.cpp:21 because apparently the loadInfo in
> nsHttpChannel doesn't have a security flag set.

So, what you would have to do is branch on the securityFlag within ::GetResource(), something like:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#158
call asyncOpen2 only if enforceSecurity is set, otherwise call AsyncOpen().

> Apart from that, do we have a way of creating nsILoadInfo in xpcshell tests?
> I'd hate to have different code paths for unit tests and real content.

Yeah, we don't want that. Worst case you could create a tmp-channel, passing separate LoadInfo arguments to newChannel() [1] and than get the LoadInfo from that tmpChannel and pass it to your tests.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#280

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Use ascynOpen2 in PackagedAppService

I kept both principal and loadInfo in the getResource signature, because the loadInfo passed from the xpcshell test passes the systemPrincipal with a null URI. I hope this is an ok compromise.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8650228 [details] [diff] [review]
Use ascynOpen2 in PackagedAppService

Review of attachment 8650228 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Valentin for fixing this up. Looks good in general, but no need to keep the principal as a separate argument. r=me with that fixed.

::: netwerk/base/nsIPackagedAppService.idl
@@ +43,5 @@
>     * resource URI, store it in the cache and pass the cache entry to aCallback,
>     * or if that resource has already been downloaded it will be served from
>     * the cache.
>     */
>    void getResource(in nsIPrincipal aPrincipal,

Please drop the principal from here, it's just duplicate information.

::: netwerk/test/unit/test_packaged_app_service_paths.js
@@ +21,5 @@
>  
> +function getLoadInfo(url) {
> +  let tmpChannel = NetUtil.newChannel({uri: url, loadUsingSystemPrincipal: true});
> +  return tmpChannel.loadInfo;
> +}

You can combine getPrincipal and getLoadInfo to look something like this:

function getLoadInfo(aURL, aPrincipalURL) {
  let uri = createURI(aPrincipalURL);
  var principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
                  .getService(Ci.nsIScriptSecurityManager)
                  .getNoAppCodebasePrincipal(uri);

  let tmpChannel = NetUtil.newChannel({
                     uri: aURL,
                     loadingPrincipal: principal,
                     contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER});
  return tmpChannel.loadInfo;
}

An in case you need to use the systemPrincipal you can just create it at the callsite, that should work, right?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8649548 [details]
bug_1196021_packagedappservice.patch

This patch becomes obsolete with the changes from Valentin.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8650228 [details] [diff] [review]
Use ascynOpen2 in PackagedAppService

Review of attachment 8650228 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +784,5 @@
>  
>    nsRefPtr<PackagedAppChannelListener> listener =
>      new PackagedAppChannelListener(downloader, mimeConverter);
>  
> +  if (aLoadInfo->GetEnforceSecurity()) {

arrh, I just realized that addon created channels might not have a LoadInfo, so please do
if (aLoadInfo && aLoadInfo->GetEnforceSecurity()) {

Comment 10

[Valentin Gosu [:valentin] (he/him)]

> Please drop the principal from here, it's just duplicate information.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> 
> arrh, I just realized that addon created channels might not have a LoadInfo,
> so please do
> if (aLoadInfo && aLoadInfo->GetEnforceSecurity()) {

In that case shouldn't I keep using the principal? Or else addons wouldn't be able to use packaged apps channels?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Valentin Gosu [:valentin] from comment #10)
> > Please drop the principal from here, it's just duplicate information.
> 
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> > 
> > arrh, I just realized that addon created channels might not have a LoadInfo,
> > so please do
> > if (aLoadInfo && aLoadInfo->GetEnforceSecurity()) {
> 
> In that case shouldn't I keep using the principal? Or else addons wouldn't
> be able to use packaged apps channels?

Sorry for not being explicit, addons might still use the deprecated API to create a channel, in which case the channel ends up not having a loadInfo attached to the channels, we still allow those channels, but in such a case we would deref a nullptr to access the enforcesecurity check. Basically we only have to ensure that aLoadInfo is not null.

In fact you could add a test for that, since you already have such a nice test setup where you call doGetResource() without a LoadInfo.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to Valentin Gosu [:valentin] from comment #10)
> > > Please drop the principal from here, it's just duplicate information.

Might I propose that we change the signature to ::GetResource(nsIChannel *aChannel) ?
This way we have easy access to the principal, loadInfo, loadContextInfo, loadContext, loadFlags

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Valentin Gosu [:valentin] from comment #12)
> Might I propose that we change the signature to ::GetResource(nsIChannel
> *aChannel) ?

That sounds very reasonable to me.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Pass requesting channel to PackagedAppService::GetResource

GetResource now takes the requesting channel and a callback. I also added a test that it rejects channels with a null loadInfo.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8651556 [details] [diff] [review]
Pass requesting channel to PackagedAppService::GetResource

Review of attachment 8651556 [details] [diff] [review]:
-----------------------------------------------------------------

Valentin, thanks for fixing all of that up. I think using the channel as an argument is definitely the way to go here. Please address my nits and have me see it one more time and then we have that patch landed in no time. Thanks!

::: netwerk/base/nsIPackagedAppService.idl
@@ +31,5 @@
>     *    aCallback->OnCacheEntryCheck() is called to verify the entry is valid
>     *    aCallback->OnCacheEntryAvailable() is called with a pointer to the
>     *    the cached entry, if one exists, or an error code otherwise
>     *    aCallback is kept alive using an nsCOMPtr until OnCacheEntryAvailable
>     *    is called

It seems that ::GetResource is only called from within nshttpchannel.cpp and it feels wrong to call ::GetResource(this, this);

Since nshttpChannel implements nsICacheEntryOpenCallback [1] please also drop the second argument here and query the callback within ::GetResource from the channel argument.

nsCOMPtr<nsICacheEntryOpenCallback> cb = do_QueryInterface(aChannel);

Maybe you should keep some of the documentation about the callback in the *.idl though.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.h#58

::: netwerk/protocol/http/PackagedAppService.cpp
@@ +701,5 @@
>      return NS_ERROR_INVALID_ARG;
>    }
>  
>    nsresult rv;
> +  nsCOMPtr<nsIPrincipal> principal;

nit, move the principal after the securityManager check right before you call GetChannelURIPrincipal().

@@ +706,5 @@
> +  nsIScriptSecurityManager *securityManager =
> +    nsContentUtils::GetSecurityManager();
> +  if (!securityManager) {
> +    LOG(("[%p]    > No securityManager\n", this));
> +    return NS_ERROR_NULL_POINTER;

nit: I think NS_ERROR_UNEXPECTED is more appropriate here since this should really never fail.

@@ +708,5 @@
> +  if (!securityManager) {
> +    LOG(("[%p]    > No securityManager\n", this));
> +    return NS_ERROR_NULL_POINTER;
> +  }
> +  securityManager->GetChannelURIPrincipal(aChannel, getter_AddRefs(principal));

rv = securityManager->GetChannelURIPrincipal()

@@ +709,5 @@
> +    LOG(("[%p]    > No securityManager\n", this));
> +    return NS_ERROR_NULL_POINTER;
> +  }
> +  securityManager->GetChannelURIPrincipal(aChannel, getter_AddRefs(principal));
> +  if (!principal) {

if (NS_FAILED(rv) || !principal) { ...

@@ +736,5 @@
> +      // Channels with a null loadInfo are not allowed.
> +      rv = NS_ERROR_NULL_POINTER;
> +    }
> +    return rv;
> +  }

We should *not* return if the channel does not have a loadInfo, because addon created channels might not have a loadInfo. So all you need is:
> nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();

Later, NewChannelInternal knows how to deal with a null loadInfo internally.

@@ +818,5 @@
>  
>    nsRefPtr<PackagedAppChannelListener> listener =
>      new PackagedAppChannelListener(downloader, mimeConverter);
>  
> +  if (loadInfo && loadInfo->GetEnforceSecurity()) {

Thanks, this is the only place where we have to make sure that loadInfo is non-null.

::: netwerk/test/unit/test_packaged_app_service.js
@@ +66,2 @@
>           .getService(Ci.nsIScriptSecurityManager)
>           .getNoAppCodebasePrincipal(uri);

nit: indentation

@@ +70,5 @@
> +                     uri: url,
> +                     loadingPrincipal: principal,
> +                     contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER});
> +
> +  tmpChannel.notificationCallbacks = new LoadContextCallback(principal.appId, principal.isInBrowserElement, false, false);

nit: 80 columns per line

::: netwerk/test/unit/test_packaged_app_service_paths.js
@@ +18,2 @@
>           .getService(Ci.nsIScriptSecurityManager)
>           .getNoAppCodebasePrincipal(uri);

nit: indentation

@@ +22,5 @@
> +                     uri: url,
> +                     loadingPrincipal: principal,
> +                     contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER});
> +
> +  tmpChannel.notificationCallbacks = new LoadContextCallback(principal.appId, principal.isInBrowserElement, false, false);

nit: 80 columns per line.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> It seems that ::GetResource is only called from within nshttpchannel.cpp and
> it feels wrong to call ::GetResource(this, this);
> 
> Since nshttpChannel implements nsICacheEntryOpenCallback [1] please also
> drop the second argument here and query the callback within ::GetResource
> from the channel argument.

I have thought about this, however, it makes the API very difficult to use from javascript and our unit tests hard to read. I'd rather not change this just to make one call site look a bit better.
And there are quite a few other instances of cpp code calling Method(this, this)

> @@ +736,5 @@
> > +      // Channels with a null loadInfo are not allowed.
> > +      rv = NS_ERROR_NULL_POINTER;
> > +    }
> > +    return rv;
> > +  }
> 
> We should *not* return if the channel does not have a loadInfo, because
> addon created channels might not have a loadInfo. So all you need is:
> > nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
> 
> Later, NewChannelInternal knows how to deal with a null loadInfo internally.
> 

It doesn't seem to deal with it very well as it triggers this assertion:

 ###!!! ASSERTION: Please pass security info when creating a channel: 'loadInfo', file /home/icecold/mozilla-central/netwerk/base/nsIOService.cpp, line 842"
 "#01: nsIOService::NewChannelFromURIWithProxyFlags2(nsIURI*, nsIURI*, unsigned int, nsIDOMNode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsIChannel**) (/home/icecold/mozilla-central/netwerk/base/nsIOService.cpp:842 (discriminator 3))"
 "#02: nsIOService::NewChannelFromURI2(nsIURI*, nsIDOMNode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsIChannel**) (/home/icecold/mozilla-central/netwerk/base/nsIOService.cpp:639)"
 "#03: NS_NewChannelInternal(nsIChannel**, nsIURI*, nsINode*, nsIPrincipal*, nsIPrincipal*, unsigned int, unsigned int, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*) (/home/icecold/mozilla-central/netwerk/base/nsNetUtil.inl:158)"
 "#04: NS_NewChannelInternal(nsIChannel**, nsIURI*, nsILoadInfo*, nsILoadGroup*, nsIInterfaceRequestor*, unsigned int, nsIIOService*) (/home/icecold/mozilla-central/netwerk/base/nsNetUtil.inl:204)"
 "#05: mozilla::net::PackagedAppService::GetResource(nsIChannel*, nsICacheEntryOpenCallback*) (/home/icecold/mozilla-central/netwerk/protocol/http/PackagedAppService.cpp:771)"

I could either create a new LoadInfo using the principal we do have, or call the NS_NewChannelInternal that does not take a loadInfo as a parameter. Both of these avoid the assertion.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Valentin Gosu [:valentin] from comment #16)
> I have thought about this, however, it makes the API very difficult to use
> from javascript and our unit tests hard to read. I'd rather not change this
> just to make one call site look a bit better.

Making unit tests easier to read should not be the argument here in my opinion, but OK, I leave this up to you. The most important part is that we pass the channel as an argument.

> > Later, NewChannelInternal knows how to deal with a null loadInfo internally.
> It doesn't seem to deal with it very well as it triggers this assertion:

Oh yes, I forgot about that assertion, so please remove the null loadInfo test again. Sorry about that. We have that assertion so we know that in DEBUG builds we should *never* try to create a channel without a loadInfo, but we allow it in release builds, so addons not providing a loadInfo still work. Please remove the null loadInfo test again, unless you know of a way to only trigger that specific tests in release builds, but I don't think there is.


One other thing I just realized; the default security flag in Netutil.jsm is SEC_NORMAL [1]. I will update that ASAP. What you would have to do is update you NetUtil.newChannel to include that securityFlag:

> NetUtil.newChannel({uri: url,
>                    ...
>                    securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,

If that update causes tests to fail, please let me know. And If so, you can land your patch without any securitFlags (basically falling back to the default security flags) and I will take care of passing the right securityFlags once I get to it. Shouldn't block you from landing your patch.

Other than that, patch looks great, I just would like to see it one more time but it's basically an r+ by now. Thanks!

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#371

Comment 18

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e47fc893b2c

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Attachment: Pass requesting channel to PackagedAppService::GetResource

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14c5fee50482

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8652060 [details] [diff] [review]
Pass requesting channel to PackagedAppService::GetResource

Review of attachment 8652060 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great - thanks!

::: netwerk/test/unit/test_packaged_app_service.js
@@ +149,5 @@
> +  // it works in optimized mode. See bug 1196021 comment 17
> +  if (Components.classes["@mozilla.org/xpcom/debug;1"]
> +                .getService(Components.interfaces.nsIDebug2)
> +                .isDebugBuild == false) {
> +    add_test(test_channel_no_loadinfo);

sweet - thanks a lot!

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> >                    securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
> 
> If that update causes tests to fail, please let me know. And If so, you can
> land your patch without any securitFlags (basically falling back to the
> default security flags) and I will take care of passing the right
> securityFlags once I get to it. Shouldn't block you from landing your patch.

I got an assertion with those securityFlags:

"Assertion failure: !mLoadInfo || mLoadInfo->GetSecurityMode() == 0 || mLoadInfo->GetInitialSecurityCheckDone() (security flags in loadInfo but asyncOpen2() not called), at /home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4958"
"#01: mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) (/home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4956 (discriminator 7))"
"#02: non-virtual thunk to mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) (/home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4954)"
"#03: mozilla::net::PackagedAppService::GetResource(nsIChannel*, nsICacheEntryOpenCallback*) (/home/icecold/mozilla-central/netwerk/protocol/http/PackagedAppService.cpp:819)"

It seems GetEnforceSecurity() returns 0 when the flag is set, but maybe it shouldn't?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Valentin Gosu [:valentin] from comment #21)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> > >                    securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
> > 
> > If that update causes tests to fail, please let me know. And If so, you can
> > land your patch without any securitFlags (basically falling back to the
> > default security flags) and I will take care of passing the right
> > securityFlags once I get to it. Shouldn't block you from landing your patch.
> 
> I got an assertion with those securityFlags:
> 
> "Assertion failure: !mLoadInfo || mLoadInfo->GetSecurityMode() == 0 ||
> mLoadInfo->GetInitialSecurityCheckDone() (security flags in loadInfo but
> asyncOpen2() not called), at
> /home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4958"
> "#01: mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*,
> nsISupports*)
> (/home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4956
> (discriminator 7))"
> "#02: non-virtual thunk to
> mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*)
> (/home/icecold/mozilla-central/netwerk/protocol/http/nsHttpChannel.cpp:4954)"
> "#03: mozilla::net::PackagedAppService::GetResource(nsIChannel*,
> nsICacheEntryOpenCallback*)
> (/home/icecold/mozilla-central/netwerk/protocol/http/PackagedAppService.cpp:
> 819)"
> 
> It seems GetEnforceSecurity() returns 0 when the flag is set, but maybe it
> shouldn't?

What test were you running to encounter that? The tests for this patch? I will take a look once you have landed this patch. Either way, the problem is not related to this bug. Thanks!

Comment 23

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19743fe8f522ed41d460f5c29e015f837f159638
Bug 1196021 - Use asyncOpen2 in PackagedAppService r=ckerschb

https://hg.mozilla.org/integration/mozilla-inbound/rev/0880613d3a905b138a33654b1420a996ee26c1ea
Bug 1196021 - Pass requesting channel to PackagedAppService::GetResource r=ckerschb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/19743fe8f522
https://hg.mozilla.org/mozilla-central/rev/0880613d3a90