if a channel doesn't have a load group, its origin attributes aren't honored

RESOLVED FIXED in Firefox 62

Status

()

P1
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: keeler, Assigned: mayhemer)

Tracking

({regression})

unspecified
mozilla62
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment)

This may just be a misunderstanding about the API, but it doesn't seem like the intended behavior. It appears that if a channel doesn't have its own load group set, any origin attributes that are set on the channel's load info aren't honored. See for example the OCSP request code:

https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/security/manager/ssl/nsNSSCallbacks.cpp#129

  if (mRequestSession->mOriginAttributes != OriginAttributes()) {
    OriginAttributes attrs;
    attrs.mFirstPartyDomain =
      mRequestSession->mOriginAttributes.mFirstPartyDomain;
    attrs.mPrivateBrowsingId =
      mRequestSession->mOriginAttributes.mPrivateBrowsingId;

    nsCOMPtr<nsILoadInfo> loadInfo = chan->GetLoadInfo();
    if (loadInfo) {
      rv = loadInfo->SetOriginAttributes(attrs);
      NS_ENSURE_SUCCESS(rv, rv);
    }
  }

  // Create a loadgroup for this new channel.  This way if the channel
  // is redirected, we'll have a way to cancel the resulting channel.
  nsCOMPtr<nsILoadGroup> lg = do_CreateInstance(NS_LOADGROUP_CONTRACTID);
  chan->SetLoadGroup(lg);

If the given origin attributes aren't the default, we set some custom origin attributes on the channel's load info. We then set a load group. If the load group isn't set (basically, comment out references to 'lg' or 'mLoadGroup' in that file), the test xpcshell test security/manager/ssl/tests/unit/test_ocsp_private_caching.js will fail because we make an OCSP request in a private context that gets cached as non-private.

Obviously the workaround is clear (just set a load group), but it's less clear that this is the intended behavior. It also seems like a pitfall for any new code that relies on separating requests by origin attributes.
Honza, can you have a look at this?
Flags: needinfo?(honzab.moz)
(Assignee)

Updated

11 months ago
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
(giving it a prio 1, feel free to change)
Priority: -- → P1
Whiteboard: [necko-triaged]
(Assignee)

Comment 3

10 months ago
Seems like a regressions (or a feature? :)) of bug 1199841.  

The problem is that UpdatePrivateBrowsing() is not called by the channel on itself.  We only call it when notification callbacks or a load group is set on the channel (as it affects the is-private state).  

The UpdatePrivateBrowsing() method is responsible for updating PrivateBrowsingChannel->mPrivateBrowsing member that is used to _sync_ PB state on the channel's origin attributes when we want to be used by external consumers.

So, I think one way to fix this is to call UpdatePrivateBrowsing() from inside AsyncOpen or some later point when the channel has no load group and also doesn't have notification callbacks.  That's a good fix.

I don't understand why there is this weird mechanism in the first place.  When we want to tell the cache which jar to use the privacy state is determined here:
	xul.dll!mozilla::net::PrivateBrowsingChannel<mozilla::net::HttpBaseChannel>::GetIsChannelPrivate(0x0000000a415f2760) Line 51	C++
 	xul.dll!NS_GetOriginAttributes(0x000001e3dc199050, {...}) Line 2038	C++
 	xul.dll!mozilla::net::GetLoadContextInfo(0x000001e3dc199050) Line 131	C++
 	xul.dll!mozilla::net::nsHttpChannel::OpenCacheEntryInternal(true, 0x0000000000000000, true) Line 3723	C++

instead of just directly returning the OAs of the channel.
Blocks: 1199841
Status: NEW → ASSIGNED
(Assignee)

Comment 4

10 months ago
Posted patch v1Splinter Review
I think the patch is enough self-explanatory.  Locally tested for an ocsp request made for a load in a PB window.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c1af49396dfa97f03ac57c4fb58f81b971e8162
Attachment #8974977 - Flags: review?(josh)
Keywords: regression

Updated

10 months ago
Attachment #8974977 - Flags: review?(josh) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 5

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d26c2c475f
Make sure to call UpdatePrivateBrowsing() in http channel to properly set private browsing state of the channel when no load group or callbacks have been set on it before AsyncOpen. r=jdm
Keywords: checkin-needed

Comment 6

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73d26c2c475f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
What's the severity of this issue? Given how old it is, can the fix just ride the trains or should we consider backport?
status-firefox60: --- → wontfix
status-firefox61: --- → affected
status-firefox-esr52: --- → wontfix
status-firefox-esr60: --- → affected
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 8

10 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> What's the severity of this issue? Given how old it is, can the fix just
> ride the trains or should we consider backport?

I think the only affected consumer is now PSM and they have a workaround.  Hence, I think this can (should) ride.

Thanks.
Flags: needinfo?(honzab.moz)
status-firefox61: affected → wontfix
status-firefox-esr60: affected → wontfix
You need to log in before you can comment on or make changes to this bug.