Merge pull request #13682 from brave/pr13674_bugfix/de-amp/js-redirec…

…t-check_1.40.x Check for De-AMP loops using last committed URL (uplift to 1.40.x)
brave · Jun 14, 2022 · 347dafe · 347dafe
2 parents 04e65ff + d3d4bf9
commit 347dafe
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 16 deletions.
diff --git a/components/de_amp/browser/de_amp_throttle.cc b/components/de_amp/browser/de_amp_throttle.cc
@@ -100,6 +100,10 @@ bool DeAmpThrottle::OpenCanonicalURL(const GURL& new_url,
   if (!contents)
     return false;
 
+  // The pending entry is the one in progress i.e. the AMP link
+  // The visible entry is the one visible in the address bar. If the AMP link
+  // was clicked on a page, then this will be that page. If it's a direct
+  // navigation, visible entry will be the same as pending entry.
   auto* entry = contents->GetController().GetPendingEntry();
   if (!entry) {
     if (contents->GetController().GetVisibleEntry()) {
@@ -109,14 +113,16 @@ bool DeAmpThrottle::OpenCanonicalURL(const GURL& new_url,
     }
   }
 
-  // If the canonical/target URL is the same as the current pending URL being
-  // navigated to, we should stop De-AMPing. This is done to prevent redirect
-  // loops. https://github.com/brave/brave-browser/issues/22610
-  if (new_url == entry->GetURL()) {
+  // If we've already navigated to the canonical URL last time, we
+  // should stop De-AMPing. This is done to prevent redirect loops.
+  // https://github.com/brave/brave-browser/issues/22610
+  bool new_url_same_as_last_committed =
+      contents->GetController().GetLastCommittedEntry()
+          ? contents->GetController().GetLastCommittedEntry()->GetURL() ==
+                new_url
+          : false;
+  if (new_url_same_as_last_committed)
     return false;
-  }
-
-  DCHECK(entry->GetURL() == response_url);
 
   delegate_->CancelWithError(net::ERR_ABORTED);
 
@@ -132,6 +138,7 @@ bool DeAmpThrottle::OpenCanonicalURL(const GURL& new_url,
   auto redirect_chain = request_.navigation_redirect_chain;
   redirect_chain.pop_back();
   params.redirect_chain = std::move(redirect_chain);
+  // This is added to check for server redirect loops
   params.extra_headers += base::StringPrintf("%s: true\r\n", kDeAmpHeaderName);
 
   base::SequencedTaskRunnerHandle::Get()->PostTask(

diff --git a/components/de_amp/browser/test/de_amp_browsertest.cc b/components/de_amp/browser/test/de_amp_browsertest.cc
@@ -156,7 +156,8 @@ std::unique_ptr<net::test_server::HttpResponse> BuildHttpResponseForAmpPage(
   return http_response;
 }
 
-std::unique_ptr<net::test_server::HttpResponse> HandleRequestForRedirectTest(
+std::unique_ptr<net::test_server::HttpResponse>
+HandleRequestForCyclicAmpPagesTest(
     const std::string& body,
     const net::test_server::HttpRequest& request) {
   if (request.relative_url == kTestRedirectingAmpPage1) {
@@ -167,7 +168,7 @@ std::unique_ptr<net::test_server::HttpResponse> HandleRequestForRedirectTest(
   return nullptr;
 }
 
-std::unique_ptr<net::test_server::HttpResponse> HandleServerRedirect(
+std::unique_ptr<net::test_server::HttpResponse> HandleRedirect(
     net::HttpStatusCode code,
     const std::string& source,
     const std::string& dest,
@@ -184,11 +185,26 @@ std::unique_ptr<net::test_server::HttpResponse> HandleServerRedirect(
     auto http_response =
         std::make_unique<net::test_server::BasicHttpResponse>();
     http_response->set_code(code);
-    http_response->AddCustomHeader("Location", dest);
+    if (code != net::HttpStatusCode::HTTP_OK) {
+      http_response->AddCustomHeader("Location", dest);
+      http_response->set_content(base::StringPrintf(
+          "<html><head></head><body>Redirecting to %s</body></html>",
+          dest.c_str()));
+    } else {
+      auto js_load_body = base::StringPrintf(
+          R"(
+        <html>
+          <head>
+            <script type="text/javascript">
+              window.location.replace("https://%s:%s%s");
+            </script>
+          </head>
+        </html>
+      )",
+          kTestHost, request.GetURL().port().c_str(), dest.c_str());
+      http_response->set_content(js_load_body);
+    }
     http_response->set_content_type("text/html");
-    http_response->set_content(base::StringPrintf(
-        "<html><head></head><body>Redirecting to %s</body></html>",
-        dest.c_str()));
     return http_response;
   } else {
     return BuildHttpResponseForAmpPage(body, source, request);
@@ -228,7 +244,7 @@ IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, SimpleDeAmp) {
 IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, AmpPagesPointingAtEachOther) {
   TogglePref(true);
   https_server_->RegisterRequestHandler(
-      base::BindRepeating(HandleRequestForRedirectTest, kTestAmpBody));
+      base::BindRepeating(HandleRequestForCyclicAmpPagesTest, kTestAmpBody));
   ASSERT_TRUE(https_server_->Start());
   const GURL original_url =
       https_server_->GetURL(kTestHost, kTestRedirectingAmpPage1);
@@ -237,10 +253,10 @@ IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, AmpPagesPointingAtEachOther) {
   NavigateToURLAndWaitForRedirects(original_url, landing_url);
 }
 
-IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, CanonicalRedirectsToAmp) {
+IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, CanonicalRedirectsToAmp301) {
   TogglePref(true);
   https_server_->RegisterRequestHandler(base::BindRepeating(
-      HandleServerRedirect, net::HttpStatusCode::HTTP_PERMANENT_REDIRECT,
+      HandleRedirect, net::HttpStatusCode::HTTP_PERMANENT_REDIRECT,
       kTestCanonicalPage, kTestAmpPage, kTestAmpBody));
   ASSERT_TRUE(https_server_->Start());
 
@@ -249,6 +265,38 @@ IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, CanonicalRedirectsToAmp) {
       https_server_->GetURL(kTestHost, kTestCanonicalPage);
 
   NavigateToURLAndWaitForRedirects(amp_url, amp_url);
+  NavigateToURLAndWaitForRedirects(canonical_url, amp_url);
+}
+
+IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, CanonicalRedirectsToAmp302) {
+  TogglePref(true);
+  https_server_->RegisterRequestHandler(
+      base::BindRepeating(HandleRedirect, net::HttpStatusCode::HTTP_FOUND,
+                          kTestCanonicalPage, kTestAmpPage, kTestAmpBody));
+  ASSERT_TRUE(https_server_->Start());
+
+  const GURL amp_url = https_server_->GetURL(kTestHost, kTestAmpPage);
+  const GURL canonical_url =
+      https_server_->GetURL(kTestHost, kTestCanonicalPage);
+
+  NavigateToURLAndWaitForRedirects(amp_url, amp_url);
+  NavigateToURLAndWaitForRedirects(canonical_url, amp_url);
+}
+
+IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, CanonicalJSRedirectsToAmp) {
+  TogglePref(true);
+  // Load canonical page normally and then navigate to AMP page
+  https_server_->RegisterRequestHandler(
+      base::BindRepeating(HandleRedirect, net::HttpStatusCode::HTTP_OK,
+                          kTestCanonicalPage, kTestAmpPage, kTestAmpBody));
+  ASSERT_TRUE(https_server_->Start());
+
+  const GURL amp_url = https_server_->GetURL(kTestHost, kTestAmpPage);
+  const GURL canonical_url =
+      https_server_->GetURL(kTestHost, kTestCanonicalPage);
+
+  NavigateToURLAndWaitForRedirects(amp_url, amp_url);
+  NavigateToURLAndWaitForRedirects(canonical_url, amp_url);
 }
 
 IN_PROC_BROWSER_TEST_F(DeAmpBrowserTest, NonHttpScheme) {