1317067 - AMP pages broken when loading from Google

Status: RESOLVED FIXED

(Focus-iOS :: General, defect)

Description

[Malte]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2912.0 Safari/537.36

Steps to reproduce:

This was observed with focus ad blocker, but may impact other disconnect based blockers.

Filed https://github.com/ampproject/amphtml/issues/6163 for our investigation

To reproduce:

- Activate ad blocker
- Search https://www.google.com for "obama" or "trump"
- Find results marked as "AMP"
- Try to load them
- Loading Spinner is never removed.

In general, AMP does not do any tracking itself (documents and most resources are served from a cookieless domain). It can instrument traditional tracking services, so things should just work if you block those.



Expected results:

Page should load.

Comment 1

[Malte]

Here is a gist from the original reporter showing the blocked content
https://gist.github.com/dmccown/2a300939844c87a5c2d787ae2a9a607b

Apparently it also affects Forbes
[Info] Content blocker prevented frame displaying https://www.google.com/amp/www.forbes.com/sites/susandudley/2016/11/14/election-could-wake-the-sleeping-cra-giant/?client=safari from loading a resource from https://cdn.ampproject.org/v/www.forbes.com/sites/susandudley/2016/11/14/election-could-wake-the-sleeping-cra-giant/?amp_js_v=6#origin=https%3A%2F%2Fwww.google.com&exp=a4a%3A0&channelid=0&cid=1&dialog=0&prerenderSize=1&visibilityState=prerender&paddingTop=54&history=1&p2r=0&horizontalScrolling=0&csi=0&referrer&storage=1&viewerUrl=https%3A%2F%2Fwww.google.com%2Famp%2Fwww.forbes.com%2Fsites%2Fsusandudley%2F2016%2F11%2F14%2Felection-could-wake-the-sleeping-cra-giant%2F (rs=ACT90oEnPrRtjE0AFhGuu39NRlNGIvTM5A, line 1270)

Comment 2

[willchou]

I believe the problem is the way Focus builds the URL filtering regex from the disconnect's blacklist JSON, specifically this line: https://github.com/mozilla-mobile/focus/blob/67b4a95ef7da2dfd0bf259f8f1e33b50f1d05b09/build-disconnect.py#L14

This creates Swift regexes for blacklisted domains that incorrectly match AMP CDN URLs. For example, reddit's domain blacklist is:

  ^https?://(.+\\.)?reddit\\.com

which will match "https://www.google.com/amp/s/amp.reddit.com/...". A fix could be to change the capturing group to exclude the '/' character, i.e.

  ^https?://([^/]+\\.)?reddit\\.com

This ensures that the capturing group will only match subdomains of reddit.com (which I think is intended). I tested this change on a Swift sandbox and it appears to work -- I'll create a PR with this change. Please let me know if I've made a mistake somewhere.

Comment 3

[Brian Nicholson (:bnicholson)]

This works well. Thanks for the fix!

Merged: https://github.com/mozilla-mobile/focus/commit/9b4d706411cf8ee51015773d7fb8c0eb90e92617

Comment 4

[Malte]

When can we expect this to roll out to users?

Comment 5

[Brian Nicholson (:bnicholson)]

Right now :)