Fix for #1205

[AlexVallat]

This fixes #1205. In PageStore.filterRequest, it took care to use context for all filtering checks, except the master getNetFilteringSwitch. I've fixed that to be consistent. See also traffic.js line 318.

@chrisaljoudi: Thank you for giving me write permissions to the project, but as this is global code, I'd appreciate it if you could give a second opinion on this fix before I merge it.

[chrisaljoudi]

@AlexVallat looks good (kudos on catching that!).

Shouldn't the same go for filterRequestNoCache? Like:

From f7f42d9fc0cc39e810fa34a699299254c6b2d5e8 Mon Sep 17 00:00:00 2001
From: Chris <chris@chrismatic.io>
Date: Tue, 7 Apr 2015 14:48:09 -0600
Subject: [PATCH] Same should go for filterRequestNoCache

---
 src/js/pagestore.js | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/js/pagestore.js b/src/js/pagestore.js
index 2d174b9..e50a191 100644
--- a/src/js/pagestore.js
+++ b/src/js/pagestore.js
@@ -693,8 +693,14 @@ var collapsibleRequestTypes = 'image sub_frame object';
 /******************************************************************************/

 PageStore.prototype.filterRequestNoCache = function(context) {
-
-    if ( this.getNetFilteringSwitch() === false ) {
+    // filterRequest coming through for a different URL,
+    // don't check the filtering switch for this pageStore, 
+    // but rather for the context URL.
+    if (context !== this &&
+        µb.getNetFilteringSwitch(context.requestURL) === false) {
+        return '';
+    }
+    else if (this.getNetFilteringSwitch() === false) {
         return '';
     }

-- 
2.3.5

[chrisaljoudi]

@AlexVallat Never mind. Should've noted that filterRequestNoCache is only being used for behind-the-scene requests.

[AlexVallat]

As you say, currently filterRequestNoCache will always be passed pageStore as context. However, it's been designed so that, presumably, it could potentially be called with a different context in the same way as filterRequest is in the future, so it's probably a good idea if the same change is made - I've done that now.

[gorhill]

That code change is problematic. The context could that of an embedded frame, and when it is the case, the master switch of the page still need to be used to find out whether filtering must take place in that embedded frame.

[chrisaljoudi]

@gorhill isn't that a policy, at some level?

I mean, is there an objective reason why that should be the behavior? Intuitively, to me, it seems like it should be the exact opposite. Just wondering what the concern is.

[gorhill]

@chrisaljoudi Whitelisting a page usually means to allow everything on that page as if uBlock wasn't installed. With the above code change, this means that embedded frames of a whitelisted page, unless their URLs match a whitelist directive, will still be filtered as if the page was not whitelisted.

Embedded frames must be evaluated within their own context -- hence FrameStore, but the master switch should affects the page and embedded frames.

[chrisaljoudi]

@gorhill oh, got it — I misunderstood to start.

Anyway, it appears that the real issue is in this line, (cc @AlexVallat):

var df = µb.sessionFirewall.evaluateCellZY(context.rootHostname, context.requestHostname, context.requestType);

That should be context.pageHostname, it looks like.

[chrisaljoudi]

Yep. In static net filtering, it's correct:

pageHostnameRegister = context.pageHostname || '';

That explains why it's occurring with the dynamic filtering test cases. :) @AlexVallat

[chrisaljoudi]

Well...

That doesn't explain the whitelisting issue, however — I don't think.

[gorhill]

That should be context.pageHostname, it looks like.

No, the first argument is the dynamic filtering scope, which is always the hostname of the page in dynamic filtering. rootHostname is always the hostname of the page, and this value propagate down to embedded frames through FrameStore.rootHostname.

[chrisaljoudi]

which is always the hostname of the page in dynamic filtering

@gorhill right, exactly. rootHostname isn't always that:

currently, if a request originates in a sub-frame, the dynamic filtering scope will be context.rootHostname — the parent frame hostname, not the frame's hostname.

[gorhill]

the parent frame hostname, not the frame's hostname

rootHostname is the hostname one can see in the URL in the address bar of the browser. The bug here occurs because uBlock has not been told that there is a new URL in the address bar -- hence it ends up using whatever URL there was in there before. My understanding is that it's a Firefox-specific issue which started with recent changes -- using webProgressListener instead of tabProgressListener.

[chrisaljoudi]

@gorhill well, okay.

Let's forget about the particular issue for a moment. This part doesn't have to do with whitelisting.

Say I have this dynamic filtering rule — I have nothing else, just this rule:

untrustworthy.com * * block

And let's say we navigate to a page http://news.com/. All the page has is the following:

<iframe src="http://untrustworthy.com/"></iframe>

And that starts trying to load images/etc. from untrustworthy.com.

Now in onBeforeRequest, we get the appropriate FrameStore corresponding to the iframe:

 if ( frameStore = pageStore.getFrame(frameId) ) {
    requestContext = frameStore;

That FrameStore has a rootHostname of news.com and a pageHostname of untrustworthy.com.

Right now, when filterRequest is evaluating dynamic filtering, it uses rootHostname. This means that requests in the iframe won't be blocked in our example — bad.

Note that static net filtering does, in fact, use pageHostname to, for example, qualify $domain= matches.

I hope I'm not communicating poorly :(.

[gorhill]

My understanding is that it's a Firefox-specific issue which started with recent changes

Actually, I take that back, my mistake. The issue also occurs with Chromium.

[gorhill]

I have a fix here: gorhill/uBlock@a2caf3c

But I don't know how to make a pull request with my commit.

[gorhill]

@chrisaljoudi

And that starts trying to load images/etc. from untrustworthy.com.

If I do not trust untrustworthy.com, my dynamic rule will be * untrustworthy.com * block, not untrustworthy.com * * block.

[chrisaljoudi]

@AlexVallat Just pushed 6de836b, and that fixed it in my testing.

Please let me know how that works for you.

[AlexVallat]

@gorhill Thank you for taking a look at this, and providing a better fix.

@chrisaljoudi I've just tested with the pushed changes and it works fine for the 12bytes.org case