Closed Bug 1291652 Opened 3 years ago Closed 3 years ago

return origin attributes from nsILoadInfo in NS_GetOriginAttributes

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: allstars.chh, Assigned: timhuang)

References

Details

(Whiteboard: [domsecurity-active],[OA])

Attachments

(3 files, 6 obsolete files)

27.34 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
1.66 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
10.22 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
Jonas' proposal is to return the origin attributes from LoadInfo instead of from LoadContext in NS_GetOriginAttributes.
Summary: return origin attributes from nsILoadInfo from NS_GetOriginAttributes → return origin attributes from nsILoadInfo in NS_GetOriginAttributes
Whiteboard: [domsecurity-active],[OA]
Priority: -- → P1
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.
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.
(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.
I will help on this bug.
Assignee: allstars.chh → tihuang
Status: NEW → ASSIGNED
Attachment #8778116 - Attachment is obsolete: true
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
Attachment #8780547 - Flags: review?(honzab.moz) → feedback+
(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
Attachment #8780547 - Attachment is obsolete: true
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?
Attachment #8782312 - Flags: review?(honzab.moz) → review-
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
Attachment #8780549 - Flags: review?(honzab.moz) → review+
(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.
Attachment #8782312 - Attachment is obsolete: true
Attachment #8780549 - Attachment is obsolete: true
Attachment #8777290 - Attachment is obsolete: true
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.
Attachment #8782670 - Flags: review?(honzab.moz) → review+
Attachment #8782670 - Attachment is obsolete: true
Try looks good.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2e78bdf4f022
https://hg.mozilla.org/mozilla-central/rev/8001a9bfcf42
https://hg.mozilla.org/mozilla-central/rev/4189c538b01e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
See Also: → 1301091
Depends on: 1301091
See Also: 1301091
You need to log in before you can comment on or make changes to this bug.