Bug 149873 – [Content Extensions] Content blocking rules are not consulted for pings

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Roopesh Chander]

Created attachment 262592 [details]
Testcase showing use of pings with content blockers.

Given a webpage that contains a link with a 'ping' attribute, like this:

    <a href="nav_url" ping="ping_url">link</a>

when the user clicks on the link, in addition to the page navigating to 'nav_url', a ping is sent to 'ping_url' as a POST request.

This POST request should be subject to the rules specified in content blocker extensions.

Specifically, when there exists a content blocker rule like this:

    {
        "trigger": { "url-filter": "ping_url" }
        "action": "block"
    }

then, when the user clicks on the link specified above, the ping POST request should not be sent.

Similarly, when there exists a content blocker rule like this:

    {
        "trigger": { "url-filter": "ping_url" }
        "action": "block-cookies"
    }

then, when the user clicks on the link specified above, the ping POST request should be sent, but with cookies stripped off the request.

Currently, in both the above scenarios, the ping is sent with cookies, without consulting the content blocker rules.

The same problem was filed in radar as rdar://problem/22673784

[Roopesh Chander]

I will also be submitting a patch to fix this issue.

[Roopesh Chander]

Created attachment 262692 [details]
Patch

[Darin Adler]

Comment on attachment 262692 [details]
Patch

Is blocking alone sufficient? What about content extension features other than blocking?

[Roopesh Chander]

(In reply to comment #3)
> Is blocking alone sufficient? What about content extension features other
> than blocking?

Per my understanding, there are only three actions possible: blocking a request ("block"), blocking cookies in a request ("block-cookies"), and hiding an element ("css-display-none"). This bug (and patch) addresses "block" and "block-cookies" for pings. I don't see how "css-display-none" can apply to pings.

That said, my understanding of this code is limited, so I could be missing your point. Can you elaborate on what other content extension features need to be considered here?

[Roopesh Chander]

Created attachment 262754 [details]
Patch

Fixed quotes in ChangeLogs in the previous patch

[Darin Adler]

So who’s going to review this? I’m not expert enough in our content blocking machinery to be the reviewer.

[Alex Christensen]

Comment on attachment 262754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262754&action=review

Thanks!  This is great! I think Brady should look at this.

> Source/WebCore/loader/PingLoader.cpp:110
>      startPingLoad(frame, request);

I'm not sure that blocking right before this call to startPingLoad is the right place.  There are two other calls to startPingLoad that are not covered by this.

> LayoutTests/ChangeLog:3
> +        Tests to ensure content blocking rules are respected for "<a ping>" pings

Hooray!  Tests!

[Alex Christensen]

(In reply to comment #4)
> I don't see how "css-display-none" can apply to pings.
Making a request to a url (or trying to make a blocked request to a url) that matches a css-display-none rule should hide content on the page.  This probably already works, but it would be a good test case to add.

[Benjamin Poulain]

Comment on attachment 262754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=262754&action=review

> Source/WebCore/loader/PingLoader.cpp:98
> +#if ENABLE(CONTENT_EXTENSIONS)

We should go further and handle the 3 types of requests from PingLoader. Ping can be used to track users, this is an oversight.

Can you please add an utility function with the Content Blocker code and call it from the 3 source of requests?
That should be done on entry, not after creating the request.

>> LayoutTests/ChangeLog:3
>> +        Tests to ensure content blocking rules are respected for "<a ping>" pings
> 
> Hooray!  Tests!

+1

[Roopesh Chander]

Thanks everyone for the encouraging feedback.

I didn't quite know earlier what the other 2 methods in PingLoader were doing. This is what I understand to be the purpose of the 3 methods:

 1. loadImage(): Loading images in unload / beforeunload / pagehide handlers
 2. sendPing(): Sending <a ping> pings
 3. sendViolationReport(): Sending CSP violation / X-XSS-Protection reports

They all do different things, but they are all in PingLoader because all are fire-and-forget scenarios - we need not bother to wait for the response.

I can see that content blocking rules should be consulted for 1 & 2 definitely. I'm not sure about 3, but I'll submit a new patch that covers all 3 based on Alex's and Benjamin's comments.

(In reply to comment #8)
> (In reply to comment #4)
> > I don't see how "css-display-none" can apply to pings.
> Making a request to a url (or trying to make a blocked request to a url)
> that matches a css-display-none rule should hide content on the page.  This
> probably already works, but it would be a good test case to add.

I don't understand this part.

Let's say we have a rule like this:

    {
        "trigger": { "url-filter": "ping_url" },
        "action": { "type": "css-display-none", "selector": ".foo" }
    }

Per my understanding, this rule says: When the frame URL matches "ping_url", add ".foo { display: none }" style to the stylesheet engine.

How would this rule affect any of the three fire-and-forget scenarios mentioned above? There wouldn't be any loaded webpage to apply the ".foo { display: none }" style, right?

Or are we talking about the case where the selector for a "css-display-none" rule is "a[ping='ping_url']"?

[Roopesh Chander]

(In reply to comment #10)
> > Making a request to a url (or trying to make a blocked request to a url)
> > that matches a css-display-none rule should hide content on the page.  This
> > probably already works, but it would be a good test case to add.
> 
> I don't understand this part.

After looking at the code, I understand what "css-display-none" additionally means. Sorry about my uninformed earlier comment on this.

[Roopesh Chander]

Created attachment 262988 [details]
Patch (fails one test)

On the code changes:

 1. I created a new function in ContentExtensionsBackend.cpp called processContentExtensionRulesForPing() for handling pings, but it turned out to have a lot in common with processContentExtensionRulesForLoad(), so I've refactored out the common parts into a private method called processContentExtensionRules().

 2. In PingLoader::sendViolationReport(), I changed the existing code that roughly translates to (pseudocode):

        request.setAllowCookies(isSameSchemeHostPort(securityOriginOfURL))

    to something like (pseudocode):

        if (!isSameSchemeHostPort(securityOriginOfURL) || isCookiesBlocked) {
            request.setAllowCookies(false)
        }

    because it appears that the default allowCookies value could be false in iOS, which the earlier version could inadvertently turn on.

 3. Hiding content ("css-display-none") on an <a ping> is not working. I don't know why at present. I'm submitting this patch anyway so I can get an early feedback, and maybe some insight into why it might not be working.

On the tests:

 1. There are 3 types of pings (loadImage(), sendPing(), sendViolationReport()), and 3 actions ("block", "block-cookies", "css-display-none"), resulting in 9 scenarios to test. However, I can't think of a good way to test the combination of loadImage() + css-display-none (the ping is sent when the page is unloaded, so is it possible / useful to test hiding of an element in an unloaded page?), so that is excluded.

 2. The hide-on-ping.html test fails because of #3 above. So, I've not marked this for review.

[WebKit Commit Bot]

Attachment 262988 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/PingLoader.cpp:58:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:63:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:76:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:80:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:97:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:109:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:113:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:138:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:149:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:153:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/loader/PingLoader.cpp:163:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:173:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 12 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 262988 [details]
Patch (fails one test)

Attachment 262988 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/280328

New failing tests:
http/tests/contentextensions/hide-on-ping.html

[Build Bot]

Created attachment 263015 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Alex Christensen]

Comment on attachment 262988 [details]
Patch (fails one test)

View in context: https://bugs.webkit.org/attachment.cgi?id=262988&action=review

This is great! We could probably land this without the one failing test, then address that in a different bug.  Please upload another patch, which will probably be pretty close to landing

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:173
> +    } else if (blockedStatus == BlockedStatus::BlockedCookies) {
> +        request.setAllowCookies(false);
> +    }

There are lots of places you use {} around single lines, which WebKit style doesn't like.  https://www.webkit.org/coding/coding-style.html Use Tools/Scripts/check-webkit-style to check this.

> Source/WebCore/loader/PingLoader.cpp:96
> +        request.setAllowCookies(false);

Why not call processContentExtensionRulesForLoad with this request instead of making a new function processContentExtensionRulesForPing?  I think that would be even cleaner.

[Alex Christensen]

Comment on attachment 262988 [details]
Patch (fails one test)

View in context: https://bugs.webkit.org/attachment.cgi?id=262988&action=review

This is great! We could probably land this without the one failing test, then address that in a different bug.  Please upload another patch, which will probably be pretty close to landing

> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:173
> +    } else if (blockedStatus == BlockedStatus::BlockedCookies) {
> +        request.setAllowCookies(false);
> +    }

There are lots of places you use {} around single lines, which WebKit style doesn't like.  https://www.webkit.org/coding/coding-style.html Use Tools/Scripts/check-webkit-style to check this.

> Source/WebCore/loader/PingLoader.cpp:96
> +        request.setAllowCookies(false);

Why not call processContentExtensionRulesForLoad with this request instead of making a new function processContentExtensionRulesForPing?  I think that would be even cleaner.

[Roopesh Chander]

Comment on attachment 262988 [details]
Patch (fails one test)

View in context: https://bugs.webkit.org/attachment.cgi?id=262988&action=review

>>> Source/WebCore/contentextensions/ContentExtensionsBackend.cpp:173
>>> +    }
>> 
>> There are lots of places you use {} around single lines, which WebKit style doesn't like.  https://www.webkit.org/coding/coding-style.html Use Tools/Scripts/check-webkit-style to check this.

Sorry, I forgot to run check-webkit-style this time.

>>> Source/WebCore/loader/PingLoader.cpp:96
>>> +        request.setAllowCookies(false);
>> 
>> Why not call processContentExtensionRulesForLoad with this request instead of making a new function processContentExtensionRulesForPing?  I think that would be even cleaner.

Our options are:

 (a) Process content blocking rules on the ping URL before creating the request object.
     Then, if a request should be sent, call request.setAllowCookies(false) if applicable.

     (or)

 (b) Create a request object always, then call processContentExtensionRulesForLoad(request).
     If this results in BlockedStatus::Blocked, discard the request object.

Benjamin's earlier comment (https://bugs.webkit.org/show_bug.cgi?id=149873#c9) suggested that (a) is the preferred option, so that's what this patch implements. Implementing (b) would be a simpler patch because I wouldn't have to touch the ContentExtensionsBackend at all.

[Roopesh Chander]

Created attachment 263070 [details]
Patch using Option (a)

This is a patch using option (a) mentioned earlier. It's just a cleaned-up version of the previous patch submission - coding style is corrected, and the failing testcase (hide-on-ping) is removed.

[Roopesh Chander]

Created attachment 263075 [details]
Patch using Option (b)

This is a patch using option (b) mentioned earlier. This looks simpler, but here, we create a ResourceRequest object even when that request is about to get blocked. The failing hide-on-ping test is not included in this too.

[Roopesh Chander]

Created attachment 263076 [details]
Patch using Option (a)

This is a patch using option (a) mentioned earlier. (Resubmitting after adding my copyright for files with >10 lines of changes.)

[Alex Christensen]

I like option b a lot more.  I'll r+ it later today unless Ben has significant objection.  Option a is a lot messier.

[Darin Adler]

Comment on attachment 263075 [details]
Patch using Option (b)

View in context: https://bugs.webkit.org/attachment.cgi?id=263075&action=review

> Source/WebCore/loader/PingLoader.cpp:140
> +    bool isSameSchemePort = frame.document()->securityOrigin()->isSameSchemeHostPort(SecurityOrigin::create(reportURL).ptr());
> +    if (!isSameSchemePort)
> +        request.setAllowCookies(false);

I think this would read better without the local variable.

Not new to this patch, but I don’t understand why this doesn’t have to null check document() or securityOrigin().

[Roopesh Chander]

Comment on attachment 263075 [details]
Patch using Option (b)

View in context: https://bugs.webkit.org/attachment.cgi?id=263075&action=review

>> Source/WebCore/loader/PingLoader.cpp:140
>> +    bool isSameSchemePort = frame.document()->securityOrigin()->isSameSchemeHostPort(SecurityOrigin::create(reportURL).ptr());
>> +    if (!isSameSchemePort)
>> +        request.setAllowCookies(false);
> 
> I think this would read better without the local variable.
> 
> Not new to this patch, but I don’t understand why this doesn’t have to null check document() or securityOrigin().

I'll resubmit with null checks and without the local variable.

There are quite a few other places in PingLoader.cpp that dereference document() and securityOrigin() without checking for null. Maybe they should be addressed in another bug? Can I file a new bug report and fix them there?

[Roopesh Chander]

Created attachment 263155 [details]
Patch using Option (b)

Resubmitting patch for Option (b) with null checks and without using a local variable.

Also removed a stray file in the tests that I'd inadvertently added to the patch.

[Darin Adler]

(In reply to comment #24)
> > Not new to this patch, but I don’t understand why this doesn’t have to null check document() or securityOrigin().
> 
> I'll resubmit with null checks and without the local variable.
> 
> There are quite a few other places in PingLoader.cpp that dereference
> document() and securityOrigin() without checking for null. Maybe they should
> be addressed in another bug? Can I file a new bug report and fix them there?

Perhaps you misunderstood my comment.

It’s entirely possible that there is some guarantee that document() and securityOrigin() are non-null. If so, there is no need to add checks. But somehow the code needs to make clear where the guarantee that they are non-null comes from. Maybe one of the other contributors can help with this.

One reason for a guarantee could be that earlier code checks for the null case and this would never be called in that case. Another would be some more high level semantic guarantee that would prevent this code from being called when one or both of those would be null.

[Darin Adler]

Alex, you should review!

[Alex Christensen]

Comment on attachment 263155 [details]
Patch using Option (b)

View in context: https://bugs.webkit.org/attachment.cgi?id=263155&action=review

r=me 
I don't think we should change the null checks in this patch.  That's a separate bug

> Source/WebCore/loader/PingLoader.cpp:143
> -    request.setAllowCookies(frame.document()->securityOrigin()->isSameSchemeHostPort(SecurityOrigin::create(reportURL).ptr()));
> +    if (Document* document = frame.document()) {
> +        if (SecurityOrigin* securityOrigin = document->securityOrigin()) {
> +            if (!securityOrigin->isSameSchemeHostPort(SecurityOrigin::create(reportURL).ptr()))
> +                request.setAllowCookies(false);
> +        }
> +    }

I like that this adds null checks, but not that this would fail to remove cookies if one of the null checks fails.  I think if the null checks fail, then we should setAllowCookies(false) to avoid sending cookies to somewhere we're not supposed to, which could be a privacy problem.

[Alex Christensen]

I modified the call to setAllowCookies to remove cookies if it cannot verify the scheme, host, and port, while keeping the null checks before dereferencing pointers.
Committed to http://trac.webkit.org/changeset/191167

Thanks!

[Roopesh Chander]

Wow, it's landed :). Thanks for fixing that and landing it.

[Roopesh Chander]

(In reply to comment #17)
> We could probably land this without the one failing test,
> then address that in a different bug.

The testcase submitted earlier here on this bug was failing because it was a bad testcase: It had a <a ping> link inside an iframe and expected a parent frame's div to be hidden when the ping is sent, which is incorrect.

Created a new bug + patch for testing this scenario: 
https://bugs.webkit.org/show_bug.cgi?id=150499