brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.76k stars 2.32k forks source link

AMP auto redirect breaks website #41665

Open 0mMdy8R6uS8xpP opened 1 week ago

0mMdy8R6uS8xpP commented 1 week ago

Description

Brave browsers AMP auto redirect feature broke a website

Steps to reproduce

  1. Turn on brave AMP auto redirect
  2. Go to https://m.nocutnews.co.kr/news/amp/6227673
  3. The url becomes https://m.nocutnews.co.kr/news/<%= resulting in broken site

Actual result

I went to a google AMP page and brave browser broke the website, turning off AMP auto redirect feature fixes this

Expected result

Brave browser should redirect to https://m.nocutnews.co.kr/news/6227673 instead of https://m.nocutnews.co.kr/news/<%= when user tries to access https://m.nocutnews.co.kr/news/amp/6227673

Reproduces how often

Easily reproduced

Desktop Brave version (brave://version info)

v1.70.126 on Windows 11

Android device

Channel information

Reproducibility

Miscellaneous information

No response

ShivanKaul commented 1 week ago

The AMP canonical link element is malformed for the website: <link rel="canonical" href="https://m.nocutnews.co.kr/news/<%= Index %>"> Given that the website does not follow the AMP spec, I'm inclined to call this a wontfix. Thoughts @boocmp?

boocmp commented 1 week ago

Yes it is. It references to <link rel="canonical" href="https://m.nocutnews.co.kr/news/&lt;%= Index %">.

ShivanKaul commented 1 week ago

I wonder if we could do something clever here: if the target URL after a de-amp results in an error status code (4xx or 5xx), then we fallback to the original URL. WDYT?

ShivanKaul commented 1 week ago

@boocmp pointed out that in this case we should just have a check that the regex matches the href, and that would've resulted in the existing fallback.

ShivanKaul commented 1 week ago

additionally we should also do https://github.com/brave/brave-browser/issues/41665#issuecomment-2418275201