Closed Bug 1379345 Opened 2 years ago Closed 2 years ago

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


(Core :: Permission Manager, defect)

Not set



Tracking Status
firefox57 --- fixed


(Reporter: wisniewskit, Assigned: Nika)




(1 file)

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 yet: file /home/worker/workspace/build/src/extensions/cookie/nsPermissionManager.cpp, line 3330

(For instance, open a fresh tab and visit view-source: )

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.
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.
It looks like we clear the LOAD_DOCUMENT_URI flag when opening the async channel for nsViewSourceChannel. The reason why is described here:

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.
Flags: needinfo?(bzbarsky)
Summary: Bug 1362791 broke being able to visit view-source links in fresh tabs in debug builds. → Loading view-source URLs does not cause permissions to be transmitted
Depends on: 1337056
No longer depends on: 1362791
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.
Flags: needinfo?(bzbarsky)
Valentin, do you have any opinions as to how we should handle this edge case?
Flags: needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
Duplicate of this bug: 1377109
Assignee: nobody → michael
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
<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
Attachment #8897924 - Flags: review?(honzab.moz)
Attachment #8897924 - Flags: review?(ehsan)
Attachment #8897924 - Flags: review+
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
Attachment #8897924 - Flags: review?(honzab.moz) → review+
Pushed by
Transmit permissions for view-source URIs, r=ehsan
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.