1291652 - return origin attributes from nsILoadInfo in NS_GetOriginAttributes

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Jonas' proposal is to return the origin attributes from LoadInfo instead of from LoadContext in NS_GetOriginAttributes.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=20de40c1f9e9

Comment 2

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

Comment on attachment 8777290 [details] [diff] [review]
Patch.

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

This looks good to me. But there is significant risk that this will cause regressions. So we should make sure not to land late in a release cycle.

And any tests that we can do to make sure that we don't break private browsing would be great.

Comment 3

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

Adding bz here as well. This is a risky one so I want to make sure he knows this change is being made.

Adding the assertion in bug 1264231 comment 19 would make this a lot safer.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #2)
> And any tests that we can do to make sure that we don't break private
> browsing would be great.
Thanks for reminding, I'd also check the tests.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP - Part 2: update tests.

Comment 6

[Tim Huang[:timhuang]]

I will help on this bug.

Comment 7

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

Comment 8

[Tim Huang[:timhuang]]

Attachment: Part 3 : Update tests for fetching originAttributes from the nsILoadInfo, but not from the nsILoadContext.

Comment 9

[Tim Huang[:timhuang]]

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

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8780547 [details] [diff] [review]
Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

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

I'll quickly check the next version.

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +136,5 @@
>    LOG(("FTPChannelParent DoAsyncOpen [this=%p uri=%s]\n",
>         this, uriSpec.get()));
>  #endif
>  
> +  nsresult rv;

when you are here, please move this rv declaration to the top of the method

@@ +139,5 @@
>  
> +  nsresult rv;
> +  nsCOMPtr<nsIIOService> ios(do_GetIOService(&rv));
> +  if (NS_FAILED(rv))
> +    return SendFailedAsyncOpen(rv);

in braces plase

@@ -140,2 @@
>    bool app_offline = false;
> -  uint32_t appId = GetAppId();

do we need the GetAppId method since then?  If not, please remove it (maybe along with any in-browser complement)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ -292,5 @@
>    if (NS_FAILED(rv))
>      return SendFailedAsyncOpen(rv);
>  
> -  bool appOffline = false;
> -  uint32_t appId = GetAppId();

same here

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +107,5 @@
> +  if (appId != NECKO_UNKNOWN_APP_ID &&
> +      appId != NECKO_NO_APP_ID) {
> +    gIOService->IsAppOffline(appId, &appOffline);
> +    if (appOffline) {
> +      goto fail;

something tells me to rather do this BEFORE we create mChannel

Comment 11

[Tim Huang[:timhuang]]

(In reply to Honza Bambas (:mayhemer) from comment #10)

> @@ -140,2 @@
> >    bool app_offline = false;
> > -  uint32_t appId = GetAppId();
> 
> do we need the GetAppId method since then?  If not, please remove it (maybe
> along with any in-browser complement)

I think we still need the GetAppId() because the OfflineObserver [1] still depends on it. But, we should get the app id from the nsILoadInfo instead of the nsILoadContext inside the GetAppId().

[1] http://searchfox.org/mozilla-central/source/netwerk/base/OfflineObserver.cpp#110

Comment 12

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

Addressing review comments of comment 10.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8782312 [details] [diff] [review]
Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

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

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +125,5 @@
>                                const nsCString& aEntityID,
>                                const OptionalInputStreamParams& aUploadStream,
>                                const OptionalLoadInfoArgs& aLoadInfoArgs)
>  {
> +  nsresult rv;

nit: blank line after this line

@@ +891,5 @@
>  {
>    uint32_t appId = NECKO_UNKNOWN_APP_ID;
> +  if (mChannel) {
> +    NeckoOriginAttributes attrs;
> +    if(NS_GetOriginAttributes(mChannel, attrs)) {

if (

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1652,5 @@
>  {
>    uint32_t appId = NECKO_UNKNOWN_APP_ID;
> +  if (mChannel) {
> +    NeckoOriginAttributes attrs;
> +    if(NS_GetOriginAttributes(mChannel, attrs)) {

if (

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +343,5 @@
> +    nsresult rv = mChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +
> +    if (NS_SUCCEEDED(rv)) {
> +      appId = attrs.mAppId;
> +    }

rather incomplete (attrs are uninitialized)!  maybe use NS_GetOriginAttributes on the channel?

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8780549 [details] [diff] [review]
Part 3 : Update tests for fetching originAttributes from the nsILoadInfo, but not from the nsILoadContext.

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

please push to try before landing

::: extensions/cookie/test/channel_utils.js
@@ +167,2 @@
>   */
> +function OriginAttributes(appId, inIsolatedMozBrowser, isPrivate) {

s/isPrivate/privateId/ (or something like that)

::: netwerk/test/unit/head_channels.js
@@ +208,2 @@
>   */
> +function OriginAttributes(appId, inIsolatedMozBrowser, isPrivate) {

here as well

::: netwerk/test/unit/test_packaged_app_service.js
@@ +90,5 @@
>    } else {
>      tmpChannel.loadInfo.originAttributes = { appId: principal.appId,
>                                               inIsolatedMozBrowser: principal.isInIsolatedMozBrowserElement
>                                             };
> +    tmpChannel.notificationCallbacks = null;

please add a comment why you null this out

Comment 15

[Tim Huang[:timhuang]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8782312 [details] [diff] [review]
> Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but
> not from the nsIloadContext.
> 
> Review of attachment 8782312 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
> @@ +343,5 @@
> > +    nsresult rv = mChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> > +
> > +    if (NS_SUCCEEDED(rv)) {
> > +      appId = attrs.mAppId;
> > +    }
> 
> rather incomplete (attrs are uninitialized)!  maybe use
> NS_GetOriginAttributes on the channel?

We cannot use the NS_GetOriginAttributes here because the mChannel here is a nsIWebSocketChannel which doesn't inherit nsIChannel. And I did miss something here for initializing attrs.

Comment 16

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

Comment 17

[Tim Huang[:timhuang]]

Attachment: Part 3 : Update tests for fetching originAttributes from the nsILoadInfo, but not from the nsILoadContext.

Comment 18

[Tim Huang[:timhuang]]

Attachment: Part 1 : Return origin attributes from nsILoadInfo in NS_GetOriginAttributes.

Update the commit message.

Comment 19

[Tim Huang[:timhuang]]

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

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 8782670 [details] [diff] [review]
Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

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

::: netwerk/base/nsNetUtil.cpp
@@ -1273,3 @@
>      }
>  
> -    nsresult rv = loadContext->GetAppId(aAppID);

again, rv as the first declaration in the function

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +338,5 @@
>  {
>    uint32_t appId = NECKO_UNKNOWN_APP_ID;
> +  if (mChannel) {
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    NeckoOriginAttributes attrs;

declare before first use

@@ +339,5 @@
>    uint32_t appId = NECKO_UNKNOWN_APP_ID;
> +  if (mChannel) {
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    NeckoOriginAttributes attrs;
> +    nsresult rv = mChannel->GetLoadInfo(getter_AddRefs(loadInfo));

declare rv at the top of the method.

Comment 21

[Tim Huang[:timhuang]]

Attachment: Part 2 : Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext.

Thanks, Honza. Addressing your review comments.

Comment 22

[Tim Huang[:timhuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=088f010cef2c

Comment 23

[Tim Huang[:timhuang]]

Try looks good.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e78bdf4f022
Part 1: Return origin attributes from nsILoadInfo in NS_GetOriginAttributes. r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/8001a9bfcf42
Part 2: Make sure that we fetch originAttributes from the nsIloadInfo, but not from the nsIloadContext. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/4189c538b01e
Part 3: Update tests for fetching originAttributes from the nsILoadInfo, but not from the nsILoadContext. r=mayhemer

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e78bdf4f022
https://hg.mozilla.org/mozilla-central/rev/8001a9bfcf42
https://hg.mozilla.org/mozilla-central/rev/4189c538b01e