From f89544af7f3d38bd61b4216527d66897eb08dcd0 Mon Sep 17 00:00:00 2001
From: Tom Clegg <tclegg@veritasgenetics.com>
Date: Tue, 6 Aug 2019 13:59:46 -0400
Subject: [PATCH 1/1] 14716: Fix accidentally accepting credentials at public
 download URL.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>
---
 services/keep-web/handler.go      | 19 ++++++++++---------
 services/keep-web/handler_test.go | 22 +++++++++++++++++-----
 services/keep-web/server_test.go  |  1 +
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index b5c11e553c..837579fe25 100644
--- a/services/keep-web/handler.go
+++ b/services/keep-web/handler.go
@@ -283,8 +283,11 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 		} else {
 			// /collections/ID/PATH...
 			collectionID = parseCollectionIDFromURL(pathParts[1])
-			tokens = h.Config.AnonymousTokens
 			stripParts = 2
+			// This path is only meant to work for public
+			// data. Tokens provided with the request are
+			// ignored.
+			credentialsOK = false
 		}
 	}
 
@@ -298,6 +301,10 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 		forceReload = true
 	}
 
+	if credentialsOK {
+		reqTokens = auth.CredentialsFromRequest(r).Tokens
+	}
+
 	formToken := r.FormValue("api_token")
 	if formToken != "" && r.Header.Get("Origin") != "" && attachment && r.URL.Query().Get("api_token") == "" {
 		// The client provided an explicit token in the POST
@@ -313,7 +320,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 		//
 		// * The token isn't embedded in the URL, so we don't
 		//   need to worry about bookmarks and copy/paste.
-		tokens = append(tokens, formToken)
+		reqTokens = append(reqTokens, formToken)
 	} else if formToken != "" && browserMethod[r.Method] {
 		// The client provided an explicit token in the query
 		// string, or a form in POST body. We must put the
@@ -325,10 +332,7 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 	}
 
 	if useSiteFS {
-		if tokens == nil {
-			tokens = auth.CredentialsFromRequest(r).Tokens
-		}
-		h.serveSiteFS(w, r, tokens, credentialsOK, attachment)
+		h.serveSiteFS(w, r, reqTokens, credentialsOK, attachment)
 		return
 	}
 
@@ -347,9 +351,6 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
 	}
 
 	if tokens == nil {
-		if credentialsOK {
-			reqTokens = auth.CredentialsFromRequest(r).Tokens
-		}
 		tokens = append(reqTokens, h.Config.AnonymousTokens...)
 	}
 
diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go
index 93259f74cd..dd91df3549 100644
--- a/services/keep-web/handler_test.go
+++ b/services/keep-web/handler_test.go
@@ -559,7 +559,17 @@ func (s *IntegrationSuite) testVhostRedirectTokenToCookie(c *check.C, method, ho
 	return resp
 }
 
-func (s *IntegrationSuite) TestDirectoryListing(c *check.C) {
+func (s *IntegrationSuite) TestDirectoryListingWithAnonymousToken(c *check.C) {
+	s.testServer.Config.AnonymousTokens = []string{arvadostest.AnonymousToken}
+	s.testDirectoryListing(c)
+}
+
+func (s *IntegrationSuite) TestDirectoryListingWithNoAnonymousToken(c *check.C) {
+	s.testServer.Config.AnonymousTokens = nil
+	s.testDirectoryListing(c)
+}
+
+func (s *IntegrationSuite) testDirectoryListing(c *check.C) {
 	s.testServer.Config.AttachmentOnlyHost = "download.example.com"
 	authHeader := http.Header{
 		"Authorization": {"OAuth2 " + arvadostest.ActiveToken},
@@ -584,10 +594,12 @@ func (s *IntegrationSuite) TestDirectoryListing(c *check.C) {
 			cutDirs: 1,
 		},
 		{
-			uri:     "download.example.com/collections/" + arvadostest.FooAndBarFilesInDirUUID + "/",
-			header:  authHeader,
-			expect:  []string{"dir1/foo", "dir1/bar"},
-			cutDirs: 2,
+			// URLs of this form ignore authHeader, and
+			// FooAndBarFilesInDirUUID isn't public, so
+			// this returns 404.
+			uri:    "download.example.com/collections/" + arvadostest.FooAndBarFilesInDirUUID + "/",
+			header: authHeader,
+			expect: nil,
 		},
 		{
 			uri:     "download.example.com/users/active/foo_file_in_dir/",
diff --git a/services/keep-web/server_test.go b/services/keep-web/server_test.go
index ab50641be1..0263dcf08f 100644
--- a/services/keep-web/server_test.go
+++ b/services/keep-web/server_test.go
@@ -298,6 +298,7 @@ func (s *IntegrationSuite) runCurl(c *check.C, token, host, uri string, args ...
 }
 
 func (s *IntegrationSuite) TestMetrics(c *check.C) {
+	s.testServer.Config.AttachmentOnlyHost = s.testServer.Addr
 	origin := "http://" + s.testServer.Addr
 	req, _ := http.NewRequest("GET", origin+"/notfound", nil)
 	_, err := http.DefaultClient.Do(req)
-- 
2.30.2