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.

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

[De-AMP] Regex does not check for empty value of ⚡ attribute #22573

Closed
ShivanKaul opened this issue Apr 27, 2022 · 3 comments · Fixed by brave/brave-core#13182
Closed

[De-AMP] Regex does not check for empty value of ⚡ attribute #22573

ShivanKaul opened this issue Apr 27, 2022 · 3 comments · Fixed by brave/brave-core#13182

Comments

@ShivanKaul
Copy link
Collaborator

From: https://bravesoftware.slack.com/archives/C03BE3VNL06/p1651027881491089?thread_ts=1650911256.912569&cid=C03BE3VNL06

"Regarding the buggy behavior: Our amp-detection (in part) looks for the ⚡ attribute without an explicit value (i.e. [⚡] vs [⚡=''] ). When the attribute is followed by ='', it no longer matches our current regular expression.
There is no reason why a web-author would provide the empty value, so this pattern is not likely to be encountered often, if ever, in the wild. That said, we found a scenario today which could yield this pattern.
When setting out to test an NBCNews AMP page locally, I downloaded the page via right-click, Save As on Windows. This process modifies the markup in a couple ways, one of which includes adding ='' after the ⚡ attribute.
So if an individual were to download an AMP page (via Save As), and re-upload it elsewhere, we would not properly bounce/redirect visitors to the second version."

We have this check for amp="", but not ⚡=""

@stephendonner
Copy link

Verified PASSED using

Brave 1.40.75 Chromium: 102.0.5005.61 (Official Build) beta (x86_64)
Revision 0e59bcc00cc4985ce39ad31c150065f159d95ad3-refs/branch-heads/5005@{#819}
OS macOS Version 12.5 (Build 21G5027d)

Followed the testplan/steps from brave/brave-core#13182 (comment)

Steps:

  1. installed 1.40.75
  2. launched Brave
  3. loaded https://shivankaul.com/brave/de-amp/test-amp-lightning-empty-attribute.html and confirmed it redirected to brave.com, with Auto-redirect AMP pages toggled to ON/Enabled in brave://settings/shields
  4. new profile
  5. toggled Auto-redirect AMP pages to OFF/Disabled via brave://settings/shields
  6. loaded https://shivankaul.com/brave/de-amp/test-amp-lightning-empty-attribute.html and confirmed it did NOT redirect
Auto-redirect ON Auto-redirect OFF
Screen Shot 2022-05-27 at 4 46 52 PM Screen Shot 2022-05-27 at 4 48 22 PM

@MadhaviSeelam
Copy link

Verification PASSED using

Brave | 1.40.80 Chromium: 102.0.5005.78 (Official Build) beta (64-bit)
-- | --
Revision | df6dbb5a9fd82af3f567198af2eb5fb4876ef99c-refs/branch-heads/5005_59@{#3}
OS | Windows 11 Version 21H2 (Build 22000.675)

Followed the testplan/steps from brave/brave-core#13182 (comment)

Steps:

  • Installed 1.40.80
  • Launched Brave
  • Loaded https://shivankaul.com/brave/de-amp/test-amp-lightning-empty-attribute.html and confirmed it redirected to brave.com, with Auto-redirect AMP pages toggled to ON/Enabled in brave://settings/shields
  • New profile
  • Toggled Auto-redirect AMP pages to OFF/Disabled via brave://settings/shields
  • Loaded https://shivankaul.com/brave/de-amp/test-amp-lightning-empty-attribute.html and confirmed it did NOT redirect
Auto-redirect ON Auto-redirect OFF
image1 image2

@kjozwiak
Copy link
Member

Verification PASSED on Samsung S10+ running Android 12 using the following build(s):

Brave | 1.40.101 Chromium: 103.0.5060.42 (Official Build) (64-bit)
--- | ---
Revision | de0d840bf9439c31bd86bf74f065c31fdf9b208d-refs/branch-heads/5060@{#667}
OS | Android 12; Build/SP1A.210812.016

Went through the STR/Cases outlined via brave/brave-core#13182 (comment) as per the following:

  • installed/launched 1.40.101 Chromium: 103.0.5060.42
  • loaded https://shivankaul.com/brave/de-amp/test-amp-lightning-empty-attribute.html and ensured that it redirected to brave.com, with Auto-redirect AMP pages toggled to ON/Enabled via Settings -> Brave Shields & privacy
  • removed/re-installed to launch 1.40.101 Chromium: 103.0.5060.42 via a new profile
  • toggled Auto-redirect AMP pages to OFF/Disabled via Settings -> Brave Shields & privacy
  • loaded https://shivankaul.com/brave/de-amp/test-amp-lightning-empty-attribute.html and ensured it did NOT redirect
Auto-redirect ON Auto-redirect OFF
image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment