Bug 149873 - [Content Extensions] Content blocking rules are not consulted for pings
Summary: [Content Extensions] Content blocking rules are not consulted for pings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-10-07 04:09 PDT by Roopesh Chander
Modified: 2015-10-23 06:47 PDT (History)
12 users (show)

See Also:


Attachments
Testcase showing use of pings with content blockers. (2.74 KB, application/x-gzip)
2015-10-07 04:09 PDT, Roopesh Chander
no flags Details
Patch (14.77 KB, patch)
2015-10-08 06:38 PDT, Roopesh Chander
no flags Details | Formatted Diff | Diff
Patch (14.76 KB, patch)
2015-10-09 00:01 PDT, Roopesh Chander
benjamin: review-
Details | Formatted Diff | Diff
Patch (fails one test) (46.87 KB, patch)
2015-10-13 08:49 PDT, Roopesh Chander
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (862.61 KB, application/zip)
2015-10-13 13:28 PDT, Build Bot
no flags Details
Patch using Option (a) (43.58 KB, patch)
2015-10-14 03:28 PDT, Roopesh Chander
no flags Details | Formatted Diff | Diff
Patch using Option (b) (34.02 KB, patch)
2015-10-14 06:46 PDT, Roopesh Chander
no flags Details | Formatted Diff | Diff
Patch using Option (a) (44.12 KB, patch)
2015-10-14 07:11 PDT, Roopesh Chander
achristensen: review-
Details | Formatted Diff | Diff
Patch using Option (b) (33.57 KB, patch)
2015-10-15 06:54 PDT, Roopesh Chander
achristensen: review+
achristensen: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roopesh Chander 2015-10-07 04:09:55 PDT
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
Comment 1 Roopesh Chander 2015-10-07 06:05:03 PDT
I will also be submitting a patch to fix this issue.
Comment 2 Roopesh Chander 2015-10-08 06:38:49 PDT
Created attachment 262692 [details]
Patch
Comment 3 Darin Adler 2015-10-08 15:14:07 PDT
Comment on attachment 262692 [details]
Patch

Is blocking alone sufficient? What about content extension features other than blocking?
Comment 4 Roopesh Chander 2015-10-08 23:47:23 PDT
(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?
Comment 5 Roopesh Chander 2015-10-09 00:01:46 PDT
Created attachment 262754 [details]
Patch

Fixed quotes in ChangeLogs in the previous patch
Comment 6 Darin Adler 2015-10-09 09:36:43 PDT
So who’s going to review this? I’m not expert enough in our content blocking machinery to be the reviewer.
Comment 7 Alex Christensen 2015-10-09 11:14:42 PDT
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!
Comment 8 Alex Christensen 2015-10-09 11:19:45 PDT
(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.
Comment 9 Benjamin Poulain 2015-10-09 17:33:01 PDT
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
Comment 10 Roopesh Chander 2015-10-10 05:37:46 PDT
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']"?
Comment 11 Roopesh Chander 2015-10-11 00:34:19 PDT
(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.
Comment 12 Roopesh Chander 2015-10-13 08:49:41 PDT
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.
Comment 13 WebKit Commit Bot 2015-10-13 13:09:19 PDT
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.
Comment 14 Build Bot 2015-10-13 13:28:13 PDT
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
Comment 15 Build Bot 2015-10-13 13:28:16 PDT
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
Comment 16 Alex Christensen 2015-10-13 13:29:33 PDT
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.
Comment 17 Alex Christensen 2015-10-13 13:29:35 PDT
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.
Comment 18 Roopesh Chander 2015-10-14 03:23:34 PDT
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.
Comment 19 Roopesh Chander 2015-10-14 03:28:23 PDT
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.
Comment 20 Roopesh Chander 2015-10-14 06:46:24 PDT
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.
Comment 21 Roopesh Chander 2015-10-14 07:11:32 PDT
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.)
Comment 22 Alex Christensen 2015-10-14 12:06:29 PDT
I like option b a lot more.  I'll r+ it later today unless Ben has significant objection.  Option a is a lot messier.
Comment 23 Darin Adler 2015-10-14 17:33:07 PDT
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().
Comment 24 Roopesh Chander 2015-10-15 06:31:05 PDT
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?
Comment 25 Roopesh Chander 2015-10-15 06:54:53 PDT
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.
Comment 26 Darin Adler 2015-10-15 09:52:23 PDT
(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.
Comment 27 Darin Adler 2015-10-15 13:27:29 PDT
Alex, you should review!
Comment 28 Alex Christensen 2015-10-15 14:08:01 PDT
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.
Comment 29 Alex Christensen 2015-10-15 21:29:15 PDT
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!
Comment 30 Roopesh Chander 2015-10-15 22:47:41 PDT
Wow, it's landed :). Thanks for fixing that and landing it.
Comment 31 Roopesh Chander 2015-10-23 06:47:05 PDT
(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