Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[De-AMP] If link tag is not within first chunk, AMP page does not get De-AMPed #22917

Closed
ShivanKaul opened this issue May 18, 2022 · 4 comments · Fixed by brave/brave-core#14271
Closed

[De-AMP] If link tag is not within first chunk, AMP page does not get De-AMPed #22917

ShivanKaul opened this issue May 18, 2022 · 4 comments · Fixed by brave/brave-core#14271
Assignees
@ShivanKaul
Labels
OS/Android Fixes related to Android browser functionality OS/Desktop privacy/de-amp QA Pass - Android ARM QA Pass - Android Tab QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/include
Milestone
1.44.x - Release

Comments

@ShivanKaul
Copy link
Collaborator

ShivanKaul commented May 18, 2022 •
edited
Loading

Description

By design, for performance and ease of implementation, we only check the first raw chunk of HTML for the AMP html tag and link canonical tag. If that is not the case, we don't De-AMP an AMP page.

We have two options:

  1. Inject JS to check if a page is an AMP page and navigate to canonical version. This is what iOS does. This is easy but loses the privacy advantage of using the throttle and URL loader.
  2. Continue looking past the first chunk for the canonical link. This maintains the privacy advantages of the current brave-core approach, but is slightly more involved to implement - have to make sure the body_sniffer state machine is correctly modified.

Note that with the second option we would still only be looking for the canonical link, and only if it's an AMP page.

Steps to Reproduce

  1. Make sure De-AMP is enabled in brave://settings/shields
  2. Go to a page like https://www-nytimes-com.cdn.ampproject.org/c/s/www.nytimes.com/wirecutter/blog/burner-browser-to-hide-internet-searches/amp/

Actual result:

image

Expected result:

Go to https://www.nytimes.com/wirecutter/blog/burner-browser-to-hide-internet-searches/

Other example URLs

  1. https://lifehacker-com.cdn.ampproject.org/c/s/lifehacker.com/do-this-android-app-privacy-audit-i-beg-you-1849151056/amp
  2. https://www-technologyreview-com.cdn.ampproject.org/c/s/www.technologyreview.com/2020/07/01/1004725/redesign-internet-apps-no-one-controls-data-privacy-innovation-cloud/amp/

Reproduces how often:

Always

Version/Channel/Platform

This happens for both Android and Desktop, Stable (1.38+)
@ShivanKaul ShivanKaul added OS/Android Fixes related to Android browser functionality OS/Desktop labels May 18, 2022
@ShivanKaul ShivanKaul self-assigned this May 18, 2022
@ShivanKaul ShivanKaul mentioned this issue Jul 21, 2022
Merged
25 tasks
@brave-builds brave-builds added this to the 1.44.x - Nightly milestone Aug 18, 2022
@stephendonner
Copy link

stephendonner commented Sep 8, 2022 •
edited
Loading

Verified PASSED using

Brave 1.44.79 Chromium: 105.0.5195.102 (Official Build) beta (x86_64)
Revision 4c16f5ffcc2da70ee2600d5db77bed423ac03a5a-refs/branch-heads/5195_55@{#4}
OS macOS Version 13.0 (Build 22A5331f)

Steps:

  1. installed 1.44.79
  2. launched Brave
  3. opened brave://settings/shields
  4. confirmed Auto-redirect AMP pages was toggled to On / Enabled
  5. loaded the URLs mentioned in the original test plan

Sites:

  1. https://www.nytimes.com/wirecutter/blog/burner-browser-to-hide-internet-searches/
  2. https://lifehacker-com.cdn.ampproject.org/c/s/lifehacker.com/do-this-android-app-privacy-audit-i-beg-you-1849151056/amp
  3. https://www-technologyreview-com.cdn.ampproject.org/c/s/www.technologyreview.com/2020/07/01/1004725/redesign-internet-apps-no-one-controls-data-privacy-innovation-cloud/amp/
step 4 site 1 site 2 site 3
Screenshot 2022-09-08 at 12 57 19 PM Screenshot 2022-09-08 at 12 57 52 PM Screenshot 2022-09-08 at 12 58 06 PM Screenshot 2022-09-08 at 12 58 20 PM

Confirmed all sites loaded full-desktop views.

@MadhaviSeelam
Copy link

Verification PASSED using

Brave | 1.44.85 Chromium: 105.0.5195.127 (Official Build) beta (64-bit)
-- | --
Revision | 912488396852bf658ab32465980c0b93a3c27a83-refs/branch-heads/5195@{#1109}
OS | Windows 11 Version 21H2 (Build 22000.978)
  1. Installed 1.44.85
  2. launched Brave
  3. opened brave://settings/shields
  4. confirmed Auto-redirect AMP pages setting is Enabled/ON
  5. verified below URLs loaded as expected

URL 1:

URL 2:

URL 3:

URL 4:

URL 5:

step 4 URL1 URL 2 URL 3 URL 4 URL 5
image image image image image image

@stephendonner stephendonner added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Sep 16, 2022
@stephendonner
Copy link

stephendonner commented Sep 16, 2022 •
edited
Loading

Verification PASS using

Brave 1.44.88 Chromium: 105.0.5195.136 (Official Build) beta (64-bit)
Revision 872774b783d0e674186a3adcd2f92e7aa22a219c-refs/branch-heads/5195_124@{#4}
OS Linux

Steps:

  1. Installed 1.44.88
  2. launched Brave
  3. opened brave://settings/shields
  4. confirmed Auto-redirect AMP pages setting is Enabled/ON
  5. verified below URLs loaded as expected

URL 1:

URL 2:

URL 3:

URL 4:

URL 5:

step 4 URL1 URL 2 URL 3 URL 4 URL 5
Screen Shot 2022-09-16 at 2 07 45 PM Screen Shot 2022-09-16 at 2 14 08 PM Screen Shot 2022-09-16 at 2 14 32 PM Screen Shot 2022-09-16 at 2 15 53 PM Screen Shot 2022-09-16 at 2 16 17 PM Screen Shot 2022-09-16 at 2 16 31 PM

@stephendonner stephendonner added QA Pass-Linux and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Sep 16, 2022
@kjozwiak
Copy link
Member

kjozwiak commented Sep 22, 2022 •
edited
Loading

Verification PASSED on Pixel 6 running Android 13 using the following build(s):

Brave | 1.44.95 Chromium: 106.0.5249.40 (Official Build) (32-bit)
--- | ---
Revision | fab1d91915d2722d6339aaa7f4e9ce44f1e9b103-refs/branch-heads/5249@{#442}
OS | Android 13; Build/T1B1.220819.006

STR/Cases:

  • launched 1.44.95 Chromium: 106.0.5249.40
  • ensured that Hamburger Menu -> Settings -> Brave Shields & privacy -> Auto-redirect AMP pages is enabled by default

Ensured that the following URL are correctly being redirected:

Verification PASSED on Samsung Tab S8 Ultra running Android 12 using the following build(s):

Brave | 1.44.95 Chromium: 106.0.5249.40 (Official Build) (32-bit)
--- | ---
Revision | fab1d91915d2722d6339aaa7f4e9ce44f1e9b103-refs/branch-heads/5249@{#442}
OS | Android 12; Build/SP2A.220305.013

STR/Cases:

  • launched 1.44.95 Chromium: 106.0.5249.40
  • ensured that Hamburger Menu -> Settings -> Brave Shields & privacy -> Auto-redirect AMP pages is enabled by default

Ensured that the following URL are correctly being redirected:

This was referenced Sep 27, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ShivanKaul ShivanKaul

Labels
OS/Android Fixes related to Android browser functionality OS/Desktop privacy/de-amp QA Pass - Android ARM QA Pass - Android Tab QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/include
Projects
None yet
Milestone
1.44.x - Release
Development

Successfully merging a pull request may close this issue.

[De-AMP] Check for canonical link tag in subsequent chunks as well brave/brave-core
6 participants
@stephendonner @kjozwiak @ShivanKaul @LaurenWags @brave-builds @MadhaviSeelam