[Downloads] Clear suggested filename on cross origin redirect.

content/browser/download/download_browsertest.cc

Index: content/browser/download/download_browsertest.cc
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index 276f3c4587cd4588cec75db5199cfa1b60dba8e0..4045f33041bc788d892be7db85743d5bab6991b7 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -5,6 +5,8 @@
 // This file contains download browser tests that are known to be runnable
 // in a pure content context.  Over time tests should be migrated here.
 
+#include <queue>
+
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
@@ -36,6 +38,9 @@
 #include "content/shell/browser/shell_network_delegate.h"
 #include "content/test/net/url_request_mock_http_job.h"
 #include "content/test/net/url_request_slow_download_job.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -503,6 +508,176 @@ bool InitialSizeFilter(int* download_size, DownloadItem* download) {
   return true;
 }
 
+// RAII test server. Once instantiated, it initializes an embedded test server.
+// Expectations can be set via the OnURL method.
+//
+// Call HasFatalFailure() after constructing a TestServer instance to check if
+// the initialization was successful.
+//
+// Example:
+//
+// TEST_F(Foo, Bar) {
+//   TestServer test_server;
+//   if (HasFatalFailure())
+//     return;
+//
+//   test_server.OnURL("/foo").RedirectTo("/bar");
+//   test_server.OnURL("/bar").SendBody("baz");
+//   ...
+//   test_server.ServeFilesFromDirectory(test_file_path);
+//   ...
+//   // Do something that fetches "/foo".
+// }
+//
+// Note that the order of the OnURL() calls are significant. The server expects
+// the requests to arrive in the same order as the OnURL() calls.
+// TODO(asanka): Consider migrating more tests to use the embedded test server.

[Randy Smith (Not in Mondays), 2014/04/23 19:12:27]

I'd encourage this; this type of infrastructure looks useful to me.  You might
want to see if it makes sense to move it into the general test directory.

[asanka, 2014/04/23 22:59:27]

I actually removed this TestServer since in the end it didn't provide enough
readability over using EmbeddedTestServer directly to justify the additional
code.

However, I'm still very much in favor of moving as many tests as possible to
EmbeddedTestServer.

+class TestServer {
+ public:
+  // An expectation for a request. Created by TestServer::OnURL().
+  class ExpectedRequest {
+   public:
+    // Respond with a status code of 302 and a Location header set to
+    // |redirect_url|.
+    ExpectedRequest& RedirectTo(const GURL& redirect_url);
+
+    // Set the body of the response to |string|. Overwrites the current body if
+    // one had been previously set.
+    ExpectedRequest& SendBody(const std::string& string);

[Randy Smith (Not in Mondays), 2014/04/23 19:12:27]

Should you say what happens if you use both RedirectTo() and SendBody()
sequentially?

[asanka, 2014/04/23 22:59:27]

Removed.

+
+    // Set the Content-Type to |mime_type|.
+    ExpectedRequest& WithContentType(const char* mime_type);
+
+   private:
+    friend class TestServer;
+
+    // Construct a ExpectedRequest that will expect an HTTP GET for
+    // |relative_url|.
+    ExpectedRequest(const std::string& relative_url);
+
+    // Returns true if this expectation matches |url|.
+    bool MatchesUrl(const std::string& url) const;
+
+    // Returns the HttpResponse for this expectation. Once this is called, the
+    // ExpectedRequest object is no longer valid.
+    scoped_ptr<net::test_server::HttpResponse> GetResponse();
+
+    std::string relative_url_;
+    scoped_ptr<net::test_server::BasicHttpResponse> response_;
+    DISALLOW_COPY_AND_ASSIGN(ExpectedRequest);
+  };
+
+  TestServer();
+  ~TestServer();
+
+  // Create a new expectation for receiving an HTTP GET for |relative_url|. By
+  // default the response is an HTTP 200 with an empty body. The response can be
+  // modified via the returned |ExpectedRequest| reference. Note that the
+  // reference becomes invalidated once a request has been received by the
+  // server.
+  ExpectedRequest& OnURL(const std::string& relative_url);

[Randy Smith (Not in Mondays), 2014/04/23 19:12:27]

Huh.  I twitch somewhat hard at returning a non-const reference, but to my
surprise the style guide doesn't speak on the subject (just on non-const
references as out params).  And it does fit what you're trying to do. 
Alternatively, you could have ExpectedRequests not be associated with servers,
and require passing them into the server for them to be effective.  Another
alternative would be to have generators for creating HandleRequestCallbacks that
could be passed into an EmbeddedTestServer and get rid of TestServer entirely.  

Having said all that, I'm ok with this.  But I wanted to call out that I find
returning non-const references a bit dicy as a coding idiom.

[asanka, 2014/04/23 22:59:27]

Hehe. It's a builder pattern.  See for example
https://code.google.com/p/chromium/codesearch#chromium/src/gin/object_templat...

But the code is removed now.

+
+  // Get the absolue URL for |relative_url|.
+  GURL GetURL(const std::string& relative_url);
+
+  // Serve files contained within |directory|. See
+  // EmbeddedTestServer::ServeFilesFromDirectory().
+  void ServeFilesFromDirectory(const base::FilePath& directory);

[Randy Smith (Not in Mondays), 2014/04/23 19:12:27]

Probably worthwhile calling out that any ExpectedRequests for a URL override
serving from the directory (unless it's order dependent, but I don't think it
is).

[asanka, 2014/04/23 22:59:27]

Removed.

+
+ private:
+  scoped_ptr<net::test_server::HttpResponse> HandleRequest(
+      const net::test_server::HttpRequest& request);
+  void InitializeAndWaitUntilReady();
+  void ShutdownAndWaitUntilComplete();
+
+  net::test_server::EmbeddedTestServer embedded_test_server_;
+  std::queue<ExpectedRequest*> expected_requests_;
+  DISALLOW_COPY_AND_ASSIGN(TestServer);
+};
+
+TestServer::ExpectedRequest::ExpectedRequest(const std::string& relative_url)
+    : relative_url_(relative_url),
+      response_(new net::test_server::BasicHttpResponse) {
+}
+
+TestServer::ExpectedRequest& TestServer::ExpectedRequest::RedirectTo(
+    const GURL& redirect_url) {
+  response_->set_code(net::HTTP_FOUND);
+  response_->AddCustomHeader("Location", redirect_url.spec());
+  return *this;
+}
+
+TestServer::ExpectedRequest& TestServer::ExpectedRequest::SendBody(
+    const std::string& string) {
+  response_->set_content(string);
+  return *this;
+}
+
+TestServer::ExpectedRequest& TestServer::ExpectedRequest::WithContentType(
+    const char* mime_type) {
+  response_->set_content_type(mime_type);
+  return *this;
+}
+
+bool TestServer::ExpectedRequest::MatchesUrl(const std::string& url) const {
+  return relative_url_ == url;
+}
+
+scoped_ptr<net::test_server::HttpResponse>
+TestServer::ExpectedRequest::GetResponse() {
+  return response_.PassAs<net::test_server::HttpResponse>();
+}
+
+TestServer::TestServer() {
+  InitializeAndWaitUntilReady();
+}
+
+TestServer::~TestServer() {
+  EXPECT_TRUE(expected_requests_.empty());
+  while (!expected_requests_.empty()) {
+    delete expected_requests_.front();
+    expected_requests_.pop();
+  }
+  ShutdownAndWaitUntilComplete();
+}
+
+void TestServer::InitializeAndWaitUntilReady() {
+  ASSERT_TRUE(embedded_test_server_.InitializeAndWaitUntilReady());
+  embedded_test_server_.RegisterRequestHandler(
+      base::Bind(&TestServer::HandleRequest, base::Unretained(this)));
+}
+
+void TestServer::ShutdownAndWaitUntilComplete() {
+  ASSERT_TRUE(embedded_test_server_.ShutdownAndWaitUntilComplete());
+}
+
+TestServer::ExpectedRequest& TestServer::OnURL(
+    const std::string& relative_url) {
+  ExpectedRequest* request = new ExpectedRequest(relative_url);
+  expected_requests_.push(request);
+  return *request;
+}
+
+void TestServer::ServeFilesFromDirectory(const base::FilePath& directory) {
+  embedded_test_server_.ServeFilesFromDirectory(directory);
+}
+
+GURL TestServer::GetURL(const std::string& relative_url) {
+  return embedded_test_server_.GetURL(relative_url);
+}
+
+scoped_ptr<net::test_server::HttpResponse> TestServer::HandleRequest(
+    const net::test_server::HttpRequest& request) {
+  scoped_ptr<net::test_server::HttpResponse> response;
+  if (!expected_requests_.empty() &&
+      expected_requests_.front()->MatchesUrl(request.relative_url)) {
+    scoped_ptr<ExpectedRequest> expected_request(expected_requests_.front());
+    response = expected_request->GetResponse();
+    expected_requests_.pop();
+  }
+  return response.Pass();
+}
+
 }  // namespace
 
 class DownloadContentTest : public ContentBrowserTest {
@@ -1651,4 +1826,87 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CookiePolicy) {
                                 GURL(download)));
 }
 
+// A filename suggestion specified via a @download attribute should be effective
+// if the final download URL is in the same origin as the initial download URL.
+// Test that this holds even if there are cross origin redirects in the middle
+// of the redirect chain.
+IN_PROC_BROWSER_TEST_F(DownloadContentTest,
+                       DownloadAttributeSameOriginRedirect) {
+  TestServer origin_one;
+  TestServer origin_two;
+  if (HasFatalFailure())
+    return;
+
+  // The download-attribute.html page contains an anchor element whose href is
+  // set to the value of the query parameter (specified as |target| in the URL
+  // below). The suggested filename for the anchor is 'suggested-filename'. When
+  // the page is loaded, a script simulates a click on the anchor, triggering a
+  // download of the target URL.
+  //
+  // We construct two test servers; origin_one and origin_two. Once started, the
+  // server URLs will differ by the port number. Therefore they will be in
+  // different origins.
+  GURL download_url = origin_one.GetURL("/ping");
+  GURL referrer_url = origin_one.GetURL(
+      std::string("/download-attribute.html?target=") + download_url.spec());
+  origin_one.ServeFilesFromDirectory(GetTestFilePath("download", ""));
+
+  // <origin_one>/download-attribute.html initiates a download of
+  // <origin_one>/ping, which redirects to <origin_two>/pong, and then finally
+  // to <origin_one>/download.
+  origin_one.OnURL("/ping").RedirectTo(origin_two.GetURL("/pong"));
+  origin_two.OnURL("/pong").RedirectTo(origin_one.GetURL("/download"));
+  origin_one.OnURL("/download").SendBody("Hello").WithContentType(
+      "application/octet-stream");
+
+  DownloadAndWait(shell(), referrer_url, DownloadItem::COMPLETE);

[Randy Smith (Not in Mondays), 2014/04/23 19:12:27]

nit: Chuckle.  This relies on "DownloadAndWait()" not actually downloading, but
navigating in a way that results in a download.  We've historically done that by
navigating to something that's not an HTML page; now we're navigating to an HTML
page that indirectly results in a download.  I'm good with that, but I trip a
little bit over the name of the function in this context.  Include a comment? 
Or maybe refer to this function in the comment above?  Or maybe change the name
of the function?

[asanka, 2014/04/23 22:59:27]

I renamed the function to NavigateToURLAndWaitForDownload() which I think makes
is clearer what it's really doing.

+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+
+  EXPECT_EQ(FILE_PATH_LITERAL("suggested-filename"),
+            downloads[0]->GetTargetFilePath().BaseName().value());
+}
+
+// A filename suggestion specified via a @download attribute should not be
+// effective if the final download URL is in another origin from the original
+// download URL.

[Randy Smith (Not in Mondays), 2014/04/23 19:12:27]

nit: I'd interchange the order of these two tests; I think "shouldn't work cross
origin", followed by "should work if back to same origin" is easier to
understand while reading.

[asanka, 2014/04/23 22:59:27]

Done.

+IN_PROC_BROWSER_TEST_F(DownloadContentTest,
+                       DownloadAttributeCrossOriginRedirect) {
+  TestServer origin_one;
+  TestServer origin_two;
+  if (HasFatalFailure())
+    return;
+
+  // The download-attribute.html page contains an anchor element whose href is
+  // set to the value of the query parameter (specified as |target| in the URL
+  // below). The suggested filename for the anchor is 'suggested-filename'. When
+  // the page is loaded, a script simulates a click on the anchor, triggering a
+  // download of the target URL.
+  //
+  // We construct two test servers; origin_one and origin_two. Once started, the
+  // server URLs will differ by the port number. Therefore they will be in
+  // different origins.
+  GURL download_url = origin_one.GetURL("/ping");
+  GURL referrer_url = origin_one.GetURL(
+      std::string("/download-attribute.html?target=") + download_url.spec());
+
+  // <origin_one>/download-attribute.html initiates a download of
+  // <origin_one>/ping, which redirects to <origin_two>/download.
+  origin_one.ServeFilesFromDirectory(GetTestFilePath("download", ""));
+  origin_one.OnURL("/ping").RedirectTo(origin_two.GetURL("/download"));
+  origin_two.OnURL("/download").SendBody("Hello").WithContentType(
+      "application/octet-stream");
+
+  DownloadAndWait(shell(), referrer_url, DownloadItem::COMPLETE);
+
+  std::vector<DownloadItem*> downloads;
+  DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
+  ASSERT_EQ(1u, downloads.size());
+
+  EXPECT_EQ(FILE_PATH_LITERAL("download"),
+            downloads[0]->GetTargetFilePath().BaseName().value());
+}
+
 }  // namespace content