1944247 - imgLoader channels for user stylesheets don't match mRespectPrivacy

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

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

While working on a patch for bug 1899968 I added the following code to imgLoader:

diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -948,6 +948,12 @@ static nsresult NewImageChannel(
     rv = loadInfo->SetOriginAttributes(attrs);
   }
 
+  nsCOMPtr<nsILoadInfo> loadInfo = (*aResult)->LoadInfo();
+  OriginAttributes attrs = loadInfo->GetOriginAttributes();
+  attrs.mPrivateBrowsingId = aRespectPrivacy ? 1 : 0;
+  rv = loadInfo->SetOriginAttributes(
+      attrs);
+
   if (NS_FAILED(rv)) {
     return rv;
   }

This triggered an assertion in NS_CompareLoadInfoAndLoadContext.
As you'll notice, that code makes an exception for TYPE_INTERNAL_IMAGE_FAVICON.

This made me realize that this code in imagelib seems to also create channels when mRespectPrivacy=true that would have mPrivateBrowsingId = 0.

https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/image/imgLoader.cpp#894-910

// Note we are calling NS_NewChannelWithTriggeringPrincipal() here with a
// node and a principal. This is for things like background images that are
// specified by user stylesheets, where the document is being styled, but
// the principal is that of the user stylesheet.
if (aRequestingNode && aTriggeringPrincipal) {
  rv = NS_NewChannelWithTriggeringPrincipal(aResult, aURI, aRequestingNode,
                                            aTriggeringPrincipal,
                                            securityFlags, aPolicyType,
                                            nullptr,  // PerformanceStorage
                                            nullptr,  // loadGroup
                                            callbacks, aLoadFlags);

  if (NS_FAILED(rv)) {
    return rv;
  }

  if (aPolicyType == nsIContentPolicy::TYPE_INTERNAL_IMAGE_FAVICON) {

While the code block for TYPE_INTERNAL_IMAGE_FAVICON does set the originAttributes to match that of the triggering principal, which normally matches mRespectPrivacy, if the content policy is different, we'd end up loading non-pb resources loaded in a pb document.
Do we really need this exception, or should the originAttributes always say mPrivateBrowsingId=1 when mRespectPrivacy is true?

Comment 1

[Timothy Nikkel (:tnikkel)]

My knowledge is rusty on this, but do we use TYPE_INTERNAL_IMAGE_FAVICON? Is it the content policy for loading of favicons in the parent process? Which I believe we do not do in favour of loading the favicon in the content process and then passing it to the parent process.

Comment 2

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

I'm not 100% sure what that means.
Favicon loads are triggered by FaviconLoader.sys.mjs::getFaviconDataURLFromNetwork which will be later used by nsFaviconService::SetFaviconForPage. I think they might happen entirely in the parent process, but I'm not 100% sure.

However my question isn't about favicons but "things like background images that are specified by user stylesheets, where the document is being styled, but the principal is that of the user stylesheet."
Shouldn't these also have the pb attribute set, when loaded into a private browsing window?
I can't really tell if the answer is Yes, No, Doesn't matter

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:tnikkel, could you have a look please?

For more information, please visit BugBot documentation.