1509112 - Adding domains to permissions to bypass content blocking doesn't always work

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Mike Kaply [:mkaply]]

Per:

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/permissions#Host_permissions

adding a domain to permissions will

"bypass tracking protection if the host is a full domain without wildcards. Doesn't work with <all_urls>."

We are currently diagnosing problems with the Amazon assistant extension and adding their domains to permissions doesn't work.

To recreate, download Amazon assistant and unzip locally:

https://addons.mozilla.org/en-US/firefox/addon/amazon-browser-bar/

Load the amazon via about:debugging, click the dropdown and switch to the French version.

Turn on tracking protection for all pages and verify that the dropdown content no  longer loads. Going to the Javascript console shows a number of pages blocked.

"The resource at “https://www.amazon.fr/gp/ubp/json/config/liteBootstrap” was blocked because content blocking is enabled.[Learn More] extension.html"

Add the blocked domains to permissions:

    "https://www.amazon.fr/",
    "https://horizonte.browserapps.amazon.fr/",
    "https://reporter.browserapps.amazon.fr/",
    "https://dossier.browserapps.amazon.fr/",
    "https://identity.browserapps.amazon.fr/",
    "https://fls-eu.amazon.fr/",

Reload the extensions. You get less errors, but you still get a number of things blocked.

There should not be any blocked content once these domains are added to permissions.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Did anything change recently with TP that might have changed something in our handling of this?

Comment 2

[Mike Kaply [:mkaply]]

I think the issue is that these are domains loaded by the iframe in the popup.

So adding the domain in manifest.json allows the top level load of a domain into the iframe in the popup, but subsequent loads fail because those are from the website within the iframe in the popup.

I personally think that all loads within the popup should be allowed if those domains are listed in the manifest.

Comment 3

[Shane Caraveo (:mixedpuppy)]

IMO The ideal fix is that a top level iframe in an extension context is treated as first-party.  The host permissions was a workaround.

Comment 4

[Shane Caraveo (:mixedpuppy)]

ISTM that bottomDomain == moz-extension: should set aResult to false here in order to treat an iframe as first party:

https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/dom/base/ThirdPartyUtil.cpp#130

But there are a number of code paths that I don't have my head wrapped around.

Comment 5

[Mike Kaply [:mkaply]]

This seems to help a little bit, but all the sub content in the iframe is still blocked.

Comment 6

[Andrea Marchesini [:baku]]

>     "https://www.amazon.fr/",
>     "https://horizonte.browserapps.amazon.fr/",
>     "https://reporter.browserapps.amazon.fr/",
>     "https://dossier.browserapps.amazon.fr/",
>     "https://identity.browserapps.amazon.fr/",
>     "https://fls-eu.amazon.fr/",

Enabling TP, this list is not enough.  I tried to add:
                     
    "https://storage.browserapps.amazon.fr/",
    "https://fls-eu.amazon.com/",            

and something more happens. The real list could be found executing firefox with MOZ_LOG=nsChannelClassifier:5

The real fix for this issue, is to support "http://*/*" and "https://*/*".

Luca, do we have a bug opened for this issue?

Comment 7

[Mike Kaply [:mkaply]]

> The real fix for this issue, is to support "http://*/*" and "https://*/*".

The documentation seems to indicate that forcing full domains to override tracking protection was deliberate:

"bypass tracking protection if the host is a full domain without wildcards. Doesn't work with <all_urls>."

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

> The real fix for this issue, is to support "http://*/*" and "https://*/*".
> Luca, do we have a bug opened for this issue?

As mentioned by :mkaply in comment 7 this was a deliberate choice when this behavior has been implemented as part of Bug 1308640 (e.g. See Bug 1308640 comment 8).

The strategy that we would prefer (I also briefly discussed about this with Shane to confirm it) 
would be to consider as first party a regular webpage loaded into iframes directly embedded 
into a webextension page.

Most of the other existing bugzilla issues related to this have been marked as duplicate of Bug 1318532
for this reason (and not because of the other additional features that mozbrowser used to provide, because none
of those features is actually needed to fix this issue with TP).

Comment 9

[Mike Kaply [:mkaply]]

I'm marking this P1. This is a major issue for Amazon Assistant and we need to figure it out.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Kaply [:mkaply] from comment #2)
> I think the issue is that these are domains loaded by the iframe in the
> popup.
> 
> So adding the domain in manifest.json allows the top level load of a domain
> into the iframe in the popup, but subsequent loads fail because those are
> from the website within the iframe in the popup.
> 
> I personally think that all loads within the popup should be allowed if
> those domains are listed in the manifest.

To be clear, IIUC the extension browserAction (or pageAction) has an extension page, which contains an iframe.  That iframe is where they are loading an external site.  If this is incorrect, can you outline exactly how the extension is working?

Comment 11

[Mike Kaply [:mkaply]]

> To be clear, IIUC the extension browserAction (or pageAction) has an extension page, which contains an iframe.  That iframe is where they are loading an external site.  If this is incorrect, can you outline exactly how the extension is working?

That is correct. <iframe> in the webextension popup that loads an amazon URL.

So the iframe is considered third party, so it can't load certain amazon domains when tracking protection is turned on.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Blake, Ehsan,
ISTM that TP (specifically in nsChannelClassifier) ends up using ThirdPartyUtil to determine third-partiness.  Would it be reasonable that if an iframes direct parent is a moz-extension principal that ThirdPartyUtil would not claim it is third party?

See comment 11 above for concise description of where content is getting blocked.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> ISTM that TP (specifically in nsChannelClassifier) ends up using
> ThirdPartyUtil to determine third-partiness.  Would it be reasonable that if
> an iframes direct parent is a moz-extension principal that ThirdPartyUtil
> would not claim it is third party?

This seems reasonable to me, but I'd like to hear Ehsan's views on this too.

Comment 14

[(no longer active)]

(In reply to Luca Greco [:rpl] from comment #8)
> Most of the other existing bugzilla issues related to this have been marked
> as duplicate of Bug 1318532
> for this reason (and not because of the other additional features that
> mozbrowser used to provide, because none
> of those features is actually needed to fix this issue with TP).

I see...  Before I say anything, I would like to understand the problem here better.  Would you all have a bit of time to get together over Vidyo so that you can walk me through the problem and the implications of treating such pages as first-party to ensure I understand all of the important aspects here correctly?

My calendar should be up to date, please feel free to suggest a time that works best for you.  I think it'd be nice to have baku as well.  Thanks a lot!

Comment 15

[(no longer active)]

Also, is this particular bug a regression?  I'm asking because the code related to this has been changing significantly over the past few months so it's possible that in that process something somewhere has been broken...

Comment 16

[Shane Caraveo (:mixedpuppy)]

I'm unsure if it's a regression, we've had this for some time.  You can see a fairly good description of what we want in bug 1458374 comment 4.  We put this off thinking iframe mozbrowser (bug 1318532) would be a good way to go, but at this point it seems probably better to just address TP.

I'll look at the calendar later after dinner.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Just to also capture this, one suggestion I think Kris made was to have a top-level domain whitelist in the extension manifest specifically for this purpose.  Extensions would have to list any domain they want treated as a top-level in an iframe in their content.  I don't think Kris liked the idea of modifying the ThirdPartyUtils this way.

Comment 18

[(no longer active)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> I'm unsure if it's a regression, we've had this for some time.  You can see
> a fairly good description of what we want in bug 1458374 comment 4.  We put
> this off thinking iframe mozbrowser (bug 1318532) would be a good way to go,
> but at this point it seems probably better to just address TP.

Thanks, that's helpful.  In that discussion Francois has pointed out how treating such content-in-extension-frame cases as first-parties may not be exactly what we want, it would be nice to discuss that in more detail.

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> Just to also capture this, one suggestion I think Kris made was to have a
> top-level domain whitelist in the extension manifest specifically for this
> purpose.  Extensions would have to list any domain they want treated as a
> top-level in an iframe in their content.  I don't think Kris liked the idea
> of modifying the ThirdPartyUtils this way.

Yeah, I share his concerns too.  One problem with modifying ThirdPartyUtils is that code is quite generic as you know and is used for many different purposes, not just tracking protection.  The chances of such a modification breaking tracking protection in completely unexpected ways would be too high, I think.

(That being said, I have no idea how acceptable an alternative solution based on modifying the extension manifest would be either.  For example, for bug 1514054 it seems like that won't be a good solution.)

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to :Ehsan Akhgari from comment #18)
ls this way.
> 
> Yeah, I share his concerns too.  One problem with modifying ThirdPartyUtils
> is that code is quite generic as you know and is used for many different
> purposes, not just tracking protection.  The chances of such a modification
> breaking tracking protection in completely unexpected ways would be too
> high, I think.

Yeah, this is why I ni? you, I'm not familiar with everything going on there, it just seemed central to the issue.

> (That being said, I have no idea how acceptable an alternative solution
> based on modifying the extension manifest would be either.  For example, for
> bug 1514054 it seems like that won't be a good solution.)

That is the real crux of the problem here, we either say "to bad, doesn't work" (rss example) and use a whitelist in the manifest, or do we treat the iframe as first-party, at least from TP.

Overall, I'd be happy with figuring out a solution just for TP right now, but my preference is to treating moz-ext and mozext->iframe as first-party rather than the whitelist.  I'd also be happy with somehow strictly treating hidden iframes in a way that TP would still work as expected on those.

Comment 20

[(no longer active)]

OK, thanks for explaining things further Shane!  It still be great if we can get together for an hour or so in the next few days to discuss what options exist to solve this in a way that would be easiest for extension developers, I'd appreciate that.  :-)

Comment 21

[(no longer active)]

So I investigated this under the debugger.  The problem here isn't the third-party checks at all, it is the principal used to do the add-on loading checks with.  Currently we are using the loading principal, but this doesn't work well with loads triggered by the tracker itself when embedded under the moz-extension:// iframe, since the tracker would be the loading principal in that situation.

I think the right principal to use is the top-level principal, and when that is not available, fall back to the loading principal.

Comment 22

[(no longer active)]

Attachment: Bug 1509112 - Consider the content frame sitting directly beneath a moz-extension frame that has a host permission granting access to that frame as a top-level frame;

Comment 23

[(no longer active)]

The attached patch fixes the STR in comment 0.  Of course, the full set of URLs that need to receive host permissions include:

    "https://www.amazon.fr/",
    "https://horizonte.browserapps.amazon.fr/",
    "https://reporter.browserapps.amazon.fr/",
    "https://dossier.browserapps.amazon.fr/",
    "https://identity.browserapps.amazon.fr/",
    "https://fls-eu.amazon.fr/",
    "https://storage.browserapps.amazon.fr/",
    "https://feed.browserapps.amazon.fr/",

Comment 24

[Shane Caraveo (:mixedpuppy)]

Nice.  What is your opinion on uplifting that into 65?

Comment 25

[(no longer active)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> Nice.  What is your opinion on uplifting that into 65?

The code changes are pretty safe to uplift (in terms of the risk of introducing stability issues and whatnot.)

But that being said, I'm changing the principal which we're using to do this check based on, which is far from a small change.  If bug 1458374 comment 4 is true, then the original code just never did the right thing in the first place, which makes this change kind of safe to uplift, but I think I'd really like to have the opinion of someone more familiar with the add-ons side of things like yourself or Kris on whether that's the case, and how much value we'll get from the uplift.  If we feel safe from the add-ons side of things, I feel OK to uplift this.

(At least now we have a test which ensures we're doing the right thing both in the case where we have to use the top-level and the loading principal... so we *know* the code is correct now!)

Comment 26

[Shane Caraveo (:mixedpuppy)]

This relies on getting the WebExtensionPolicy instance for the principal, if it does not (ie. it is not an extension principal) then it will return false.  If it does, then it checks against the host permissions of the extension.  From that perspective, it looks safe to me from the addon side of things.  I'd still like Kris to review.

Comment 27

[David Durst [:ddurst]]

When this lands in 66, mkaply, do you want to recheck with your same tests? I'd like to get this uplifted to 65, but QA needs a pass through it as well.

Comment 28

[(no longer active)]

Hmm, the new approach doesn't in fact seem to be working.  I'm not sure why I didn't encounter this problem when I first rewrote the patch...  Maybe I ended up running the tests without properly redoing the build, or something.  Anyway, I must have somehow been confused, my apologies.

The problem with the new approach is that we will try to walk up the parent hierarchy before the initial navigation has occurred, with a call stack looking like this:

#0  0x00007f36692c5260 in nsPIDOMWindowOuter::GetDocumentURI() const (this=0x7f365da03c20) at /home/ehsan/moz/src/dom/base/nsGlobalWindowOuter.cpp:7323
#1  0x00007f36692affd6 in GetTopImpl(nsGlobalWindowOuter*, nsPIDOMWindowOuter**, bool, bool) (aWin=0x7f365da03c00, aTop=0x7ffcab1ef9b0, aScriptable=false, aExcludingExtensionAccessibleContentFrames=true)
    at /home/ehsan/moz/src/dom/base/nsGlobalWindowOuter.cpp:2716
#2  0x00007f36692b02ac in nsGlobalWindowOuter::GetTopExcludingExtensionAccessibleContentFrames() (this=0x7f365da03c00) at /home/ehsan/moz/src/dom/base/nsGlobalWindowOuter.cpp:2771
#3  0x00007f366944517b in ThirdPartyUtil::GetTopWindowForChannel(nsIChannel*, mozIDOMWindowProxy**) (this=0x7f365da85420, aChannel=0x7f365af98078, aWin=0x7ffcab1efb10)
    at /home/ehsan/moz/src/dom/base/ThirdPartyUtil.cpp:270
#4  0x00007f36677b01ff in mozilla::net::HttpBaseChannel::GetTopWindowURI(nsIURI**) (this=0x7f365af98028, aTopWindowURI=0x7ffcab1efea0) at /home/ehsan/moz/src/netwerk/protocol/http/HttpBaseChannel.cpp:2317
#5  0x00007f36677f76ba in mozilla::net::HttpChannelChild::ContinueAsyncOpen() (this=0x7f365af98000) at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:2663
#6  0x00007f36677f1680 in mozilla::net::HttpChannelChild::AsyncOpen(nsIStreamListener*, nsISupports*) (this=0x7f365af98000, listener=0x7f3658138400, aContext=0x0)
    at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:2510
#7  0x00007f36677f0b35 in mozilla::net::HttpChannelChild::AsyncOpen2(nsIStreamListener*) (this=0x7f365af98000, aListener=0x7f3658138400) at /home/ehsan/moz/src/netwerk/protocol/http/HttpChannelChild.cpp:2522
#8  0x00007f36687a3dda in nsURILoader::OpenURI(nsIChannel*, unsigned int, nsIInterfaceRequestor*) (this=0x7f365d91f420, channel=0x7f365af98078, aFlags=0, aWindowContext=0x7f365af97830)
    at /home/ehsan/moz/src/uriloader/base/nsURILoader.cpp:837
#9  0x00007f366e9e768b in nsDocShell::DoChannelLoad(nsIChannel*, nsIURILoader*, bool) (this=0x7f365af97800, aChannel=0x7f365af98078, aURILoader=0x7f365d91f420, aBypassClassifier=false)
    at /home/ehsan/moz/src/docshell/base/nsDocShell.cpp:10496
#10 0x00007f366e9e62b5 in nsDocShell::DoURILoad(nsIURI*, nsIURI*, mozilla::Maybe<nsCOMPtr<nsIURI> > const&, bool, bool, bool, bool, bool, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, nsTSubstring<char> const&, nsTSubstring<char16_t> const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsTSubstring<char16_t> const&, nsIURI*, unsigned int) (this=0x7f365af97800, aURI=0x7f3658108200, aOriginalURI=0x0, aResultPrincipalURI=..., aKeepResultPrincipalURIIfSet=false, aLoadReplace=false, aIsFromProcessingFrameAttributes=true, aLoadFromExternal=false, aForceAllowDataURI=false, aOriginalFrameSrc=true, aReferrerURI=0x7f3658106300, aSendReferrer=true, aReferrerPolicy=0, aTriggeringPrincipal=0x7f365af80ee0, aPrincipalToInherit=0x7f365af80ee0, aTypeHint=..., aFileName=..., aPostData=0x0, aHeadersData=0x0, aFirstParty=false, aDocShell=0x0, aRequest=0x7ffcab1f2480, aIsNewWindowTarget=false, aBypassClassifier=false, aForceAllowCookies=false, aSrcdoc=..., aBaseURI=0x0, aContentPolicyType=29)
    at /home/ehsan/moz/src/docshell/base/nsDocShell.cpp:10288
#11 0x00007f366e9b76d1 in nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) (this=0x7f365af97800, aLoadState=0x7f365811d3c0, aDocShell=0x0, aRequest=0x0)
    at /home/ehsan/moz/src/docshell/base/nsDocShell.cpp:9610
#12 0x00007f366e9b140d in nsDocShell::LoadURI(nsDocShellLoadState*) (this=0x7f365af97800, aLoadState=0x7f365811d3c0) at /home/ehsan/moz/src/docshell/base/nsDocShell.cpp:749
#13 0x00007f3669517ab1 in nsFrameLoader::ReallyStartLoadingInternal() (this=0x7f365da26580) at /home/ehsan/moz/src/dom/base/nsFrameLoader.cpp:462
#14 0x00007f36694de7d9 in nsFrameLoader::ReallyStartLoading() (this=0x7f365da26580) at /home/ehsan/moz/src/dom/base/nsFrameLoader.cpp:337
#15 0x00007f36694d7236 in nsIDocument::MaybeInitializeFinalizeFrameLoaders() (this=0x7f365812a000) at /home/ehsan/moz/src/dom/base/nsDocument.cpp:6156
#16 0x00007f36694d6f9c in nsDocument::EndUpdate() (this=0x7f365812a000) at /home/ehsan/moz/src/dom/base/nsDocument.cpp:4716
#17 0x00007f366b08de13 in nsHTMLDocument::EndUpdate() (this=0x7f365812a000) at /home/ehsan/moz/src/dom/html/nsHTMLDocument.cpp:2025
#18 0x00007f3668868f6a in nsHtml5DocumentBuilder::EndDocUpdate() (this=0x7f365d93d800) at /home/ehsan/moz/src/parser/html/nsHtml5DocumentBuilder.h:73
#19 0x00007f36688bccef in nsHtml5AutoFlush::~nsHtml5AutoFlush() (this=0x7ffcab1f3030) at /home/ehsan/moz/src/parser/html/nsHtml5TreeOpExecutor.cpp:72
#20 0x00007f366889f659 in nsHtml5TreeOpExecutor::RunFlushLoop() (this=0x7f365d93d800) at /home/ehsan/moz/src/parser/html/nsHtml5TreeOpExecutor.cpp:487
#21 0x00007f36688bfc11 in nsHtml5ExecutorFlusher::Run() (this=0x7f365da1be80) at /home/ehsan/moz/src/parser/html/nsHtml5StreamParser.cpp:118
#22 0x00007f3666fe83d9 in mozilla::SchedulerGroup::Runnable::Run() (this=0x7f36581263d0) at /home/ehsan/moz/src/xpcom/threads/SchedulerGroup.cpp:299
#23 0x00007f366701a350 in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f365ee19340, aMayWait=true, aResult=0x7ffcab1f3857) at /home/ehsan/moz/src/xpcom/threads/nsThread.cpp:1157
#24 0x00007f366701d773 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=0x7f365ee19340, aMayWait=true) at /home/ehsan/moz/src/xpcom/threads/nsThreadUtils.cpp:468
#25 0x00007f3667b6d4b3 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f367c69a9c0, aDelegate=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/glue/MessagePump.cpp:110
#26 0x00007f3667b6e0a9 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f367c69a9c0, aDelegate=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/glue/MessagePump.cpp:271
#27 0x00007f3667a9221f in MessageLoop::RunInternal() (this=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:314
#28 0x00007f3667a92195 in MessageLoop::RunHandler() (this=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:307
#29 0x00007f3667a9214a in MessageLoop::Run() (this=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:289
#30 0x00007f366c297b03 in nsBaseAppShell::Run() (this=0x7f365ee46dd0) at /home/ehsan/moz/src/widget/nsBaseAppShell.cpp:137
#31 0x00007f366f15a1e4 in XRE_RunAppShell() () at /home/ehsan/moz/src/toolkit/xre/nsEmbedFunctions.cpp:915
#32 0x00007f3667b6df03 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (this=0x7f367c69a9c0, aDelegate=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/glue/MessagePump.cpp:238
#33 0x00007f3667a9221f in MessageLoop::RunInternal() (this=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:314
#34 0x00007f3667a92195 in MessageLoop::RunHandler() (this=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:307
#35 0x00007f3667a9214a in MessageLoop::Run() (this=0x7ffcab1f3d98) at /home/ehsan/moz/src/ipc/chromium/src/base/message_loop.cc:289
#36 0x00007f366f1599bb in XRE_InitChildProcess(int, char**, XREChildData const*) (aArgc=15, aArgv=0x7ffcab1f4138, aChildData=0x7ffcab1f3fe0) at /home/ehsan/moz/src/toolkit/xre/nsEmbedFunctions.cpp:753
#37 0x00007f366f16c377 in mozilla::BootstrapImpl::XRE_InitChildProcess(int, char**, XREChildData const*) (this=0x7f367c6296b0, argc=17, argv=0x7ffcab1f4138, aChildData=0x7ffcab1f3fe0)
    at /home/ehsan/moz/src/toolkit/xre/Bootstrap.cpp:61
#38 0x000055706522318a in content_process_main(mozilla::Bootstrap*, int, char**) (bootstrap=0x7f367c6296b0, argc=17, argv=0x7ffcab1f4138)
    at /home/ehsan/moz/src/browser/app/../../ipc/contentproc/plugin-container.cpp:49

At this point, the outer window doesn't yet have any inner window at all.  So it is impossible to grab a URI for the inner window, even though this URI is present in the nsDocShell::DoURILoad() frame up the call stack...  :-(

Piping this information somewhere that GetTop() can read is going to be some work, possibly involving some serious surgery.  Before I do that, I wanted to double check how firmly are you against the original approach I took here?  Knowing what I know now, I think that was a much more elegant way to fix this bug...

Comment 29

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #28)
> Piping this information somewhere that GetTop() can read is going to be some
> work, possibly involving some serious surgery.  Before I do that, I wanted
> to double check how firmly are you against the original approach I took
> here?  Knowing what I know now, I think that was a much more elegant way to
> fix this bug...

Giving web content the same set of tracking protection exemptions as the extension that loads it still makes me super nervous. I'm worried that we're going to wind up allowing a load for some tracker that an extension happens to have permission for that we really don't want, say, Amazon to load directly.

You know as well as I do how quickly that sort of thing can lead to related ads following you around the rest of the web.

Comment 30

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #29)
> (In reply to :Ehsan Akhgari from comment #28)
> > Piping this information somewhere that GetTop() can read is going to be some
> > work, possibly involving some serious surgery.  Before I do that, I wanted
> > to double check how firmly are you against the original approach I took
> > here?  Knowing what I know now, I think that was a much more elegant way to
> > fix this bug...
> 
> Giving web content the same set of tracking protection exemptions as the
> extension that loads it still makes me super nervous. I'm worried that we're
> going to wind up allowing a load for some tracker that an extension happens
> to have permission for that we really don't want, say, Amazon to load
> directly.
> 
> You know as well as I do how quickly that sort of thing can lead to related
> ads following you around the rest of the web.

Hmm, do you mind laying out a set of steps which would allow the first approach to be abused?  The effective outcome of both patches (I think) should be exactly the same, in that inside a moz-extension:// frame from the extension in comment 0, any amazon content with any level of nesting will be able to load successfully and set tracking cookies in the user's profile...

Comment 31

[Mike Kaply [:mkaply]]

> When this lands in 66, mkaply, do you want to recheck with your same tests? I'd like to get this uplifted to 65, but QA needs a pass through it as well.

I absolutely will run through this when it lands.

Comment 32

[Mike Kaply [:mkaply]]

This seems to have stalled and 65 is approaching fast...

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Kris, we're about a week away from the first 65 RC builds. What can we do to get this bug moving again?

Comment 34

[(no longer active)]

Shane, any ideas how we can make progress here? I've been waiting on comment 30 for about three weeks now. :-) We're almost out of time for doing anything for 65 here.

Comment 35

[Shane Caraveo (:mixedpuppy)]

ISTM that the patch fixes what we were trying to do before and we should just get it in, the followup with something better if necessary. But if Kris has some concerns here, he should be giving the feedback.

Comment 36

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #35)

ISTM that the patch fixes what we were trying

By that I mean the original approach.

Comment 37

[(no longer active)]

I'd really like Kris to weigh in I think...

Comment 38

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #30)

Hmm, do you mind laying out a set of steps which would allow the first
approach to be abused? The effective outcome of both patches (I think)
should be exactly the same, in that inside a moz-extension:// frame from the
extension in comment 0, any amazon content with any level of nesting will be
able to load successfully and set tracking cookies in the user's profile...

So, essentially, I'm imagining something like this:

Extension has host permissions ["*://*.amazon.com/", "*://*.facebook.com/"].

It loads Amazon.com into a frame.

With your suggested approach of using the extension's privileges to determine what Amazon is allowed to load, a Facebook resource loaded into an Amazon product page would be allowed to load without tracking protection, and link the product page view to the user's Facebook identity.

With my suggested approach of treating the Amazon frame as if it were the top-level document, the Facebook frame would load the same way as it would in an Amazon tab, as an isolated third-party resource, with at least some protection against linking the load to the user's identity.

Comment 39

[(no longer active)]

OK, I'll see what I can do to work around the issue in comment 28.

I doubt the patch is gonna be possible to backport to 65 at this point unfortunately, it'll be too big and too risky.

Comment 40

[(no longer active)]

New version of the patch is on phabricator for review.

Comment 41

[David Durst [:ddurst]]

(Trying to keep track of what to communicate to strategic partnerships about this) Ehsan -- I assume that the workaround from the extension point of view (regardless of the patch particulars) would be the same: they'd indicate the discrete domains for host permissions?

Comment 42

[(no longer active)]

That's right. Note that they need to indicate all such domains. For example, the set of domains mentioned in comment 0 were insufficient to make the use case in that comment work correctly, rather the set of domains in comment 23 seems to do the job. Presumably this extension would need to declare all domains that various regional Amazon websites (the site that gets embedded in the panel) can use.

Comment 43

[David Durst [:ddurst]]

Thanks. Wanted to make sure we weren't introducing potential use of wildcards for this.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Ehsan and I both agree that it's too late to fix this in 65.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/afa7959985cd
Consider the content frame sitting directly beneath a moz-extension frame that has a host permission granting access to that frame as a top-level frame; r=kmag,baku

Comment 46

[(no longer active)]

I'd appreciate if someone (other than myself) could test whether the fix works for the original use case once the patch hits Nightly. I did test this locally but this was a rather complex change so another round of manual testing to ensure the Amazon Assistant extension now works as expected would be much appreciated.

Comment 47

[Shane Caraveo (:mixedpuppy)]

@mkaply comment 46

Comment 48

[Andrei Ciure[:aciure]]

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

Comment 49

[Mike Kaply [:mkaply]]

This does appear to fix it. I only had to add "https://horizonte.browserapps.amazon.fr/"
to permissions and connecting to the French site worked.

Comment 50

[(no longer active)]

Great, thanks for testing!

Comment 51

[Vlad Jiman]

Verified using windows 10 x64 bit using both the latest Nightly and 66. The issue was no longer reproducible on these versions, console was not showing any blocked items after adding the domains from the description.