1379345 - Loading view-source URLs does not cause permissions to be transmitted

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Thomas Wisniewski]

I'm not sure if this is intentional or desirable, but new tabs now crash in debug builds when visiting a view-source link directly, with the following message logged to console:

>2:57.33 INFO: [Child 21361] WARNING: This content process hasn't received the permissions for https://www.google.com yet: file /home/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 3330

(For instance, open a fresh tab and visit view-source:https://www.google.com/ )

The code block that's causing the crash is in nsPermissionManager::CommonTestPermissionInternal:

>  #ifdef DEBUG
>    {
>      nsCOMPtr<nsIPrincipal> prin = aPrincipal;
>      if (!prin) {
>        prin = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, OriginAttributes());
>      }
>      MOZ_ASSERT(PermissionAvaliable(prin, aType));
>    }
>  #endif

hg blame indicates this code was added in bug 1362791.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

This is probably actually caused by bug 1337056. 

It seems like view-source URLs aren't updating permissions correctly for some reason. 

How are view-source URLs loaded in the content process right now? I don't think I know how that particular protocol is proxied across process boundaries. I assume it has to call into the parent process at some point, and we will just need to make sure to call AboutToLoadHttpFtpWyciwygDocumentForChild on that call site.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

It looks like we clear the LOAD_DOCUMENT_URI flag when opening the async channel for nsViewSourceChannel. The reason why is described here: http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/netwerk/protocol/viewsource/nsViewSourceChannel.cpp#343-354

Not sure how we should handle this in the parent process.

bz, as you wrote the comment in question, do you have any ideas :D?

Summary of the situation:
1. We send down permissions for webpages lazily as they are needed
2. We need to send down permissions for view-source URIs as they are loaded with the given principal.
3. Currently we do this for HTTP URIs in AboutToLoadHttpFtbWyciwygDocumentForChild, which is fired before we send the beginning of the reply to the content.
4. Unfortunately, we don't have the loads for nsViewSourceChannel marked as document loads, so they don't get permissions transmitted for them.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, one simple option is to have an explicit way to flag a channel (just an HTTP channel?  Seems like it could be an issue for others too, right?) to return true from isDocument.  The viewsource channel would then thus flag its underlying channel when its mIsDocument is true.

Probably best to check with the necko folks how they would prefer this flagging to happen.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Valentin, do you have any opinions as to how we should handle this edge case?

Comment 5

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

We generally discourage adding any more loadFlags as we are approaching its size limits :)
A boolean attribute on nsIHttpChannel should do it, if we want this just for HTTP channels, or on nsIChannel if we want to extend it to other channel types.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Transmit permissions for view-source URIs

Comment 8

[(no longer active)]

Comment on attachment 8897924 [details] [diff] [review]
Transmit permissions for view-source URIs

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

Honza, any chance you could look over the Necko bits please?  It sort of made sense to me after the following short IRC conversation with Michael but I'd like a Necko peer to sign off on the changes as well.  Thanks!

<ehsan_> mystor, I don't understand the HttpBaseChannel.cpp hunks in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1379345&attachment=8897924
<mystor> ehsan_: Basically in the original patch which has your name on it you added the mForceMainDocumentChannel flag, and in some places you would return mForceMainDocumentChannel || (mLoadFlags & LOAD_DOCUMENT_URI), and in other placces you would just return mForceMainDocumentChannel. You also, whenever mLoadFlags was set would set mForceMainDOcumentChannel =
<mystor> mLoadFlags & LOAD_DOCUMENT_URI.
<mystor> ehsan_: I wanted to simplify this and so cleaned it up so mForceMainDocumentChannel is only set when forcing a main document channel, and LOAD_DOCUMENT_URI is checked on each of the places where you return the actual value
<ehsan_> mystor, ok I think this sort of makes sense but I think I'd like a necko peer to look at those bits also

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8897924 [details] [diff] [review]
Transmit permissions for view-source URIs

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

sorry for the delay, this took some code checking.  seems alright, just some code could be shared now.

::: dom/ipc/ContentParent.cpp
@@ -5142,5 @@
>    nsLoadFlags newLoadFlags;
>    aChannel->GetLoadFlags(&newLoadFlags);
> -  bool isDocument = false;
> -  aChannel->GetIsDocument(&isDocument);
> -  if (newLoadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE && isDocument) {

ah, removed because isDocument will always be true here (a duplicate check), nothing happening in this method seems to possibly change the IsDocument state on the channel.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2944,5 @@
>  
>  bool
>  HttpBaseChannel::IsNavigation()
>  {
> +  return mForceMainDocumentChannel || (mLoadFlags & LOAD_DOCUMENT_URI);

this is now exactly the same as HttpBaseChannel::GetIsMainDocumentChannel

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5d8c668b651
Transmit permissions for view-source URIs, r=ehsan

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b5d8c668b651