14716: Fix accidentally accepting credentials at public download URL.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index b5c11e553c115c872d5ded605e0ad3b54956d406..837579fe25acfbff5283b28bbb7f4375a3322280 100644 (file)
--- 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 93259f74cd9c2f2692555bc4a995cf3ce261241a..dd91df354900175592501ce794a6d9dc46cf8f41 100644 (file)
--- 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 ab50641be19c780a7d0b6145353b2611d0b02578..0263dcf08f92c906032664c8b0d3b6de8726d9b7 100644 (file)
--- 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)