From aaa257baff8c39b2349ca146f8b5601cccba3e49 Mon Sep 17 00:00:00 2001 From: Brett Smith Date: Mon, 30 Oct 2023 09:48:40 -0400 Subject: [PATCH] Merge branch '21025-keep-web-redirect-bypass' Closes #21025. Arvados-DCO-1.1-Signed-off-by: Brett Smith --- services/keep-web/handler.go | 38 ++++++++++++++------- services/keep-web/handler_test.go | 56 +++++++++++++++++++++++++++---- 2 files changed, 76 insertions(+), 18 deletions(-) diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go index 3af326a1ad..123c4fe34d 100644 --- a/services/keep-web/handler.go +++ b/services/keep-web/handler.go @@ -293,12 +293,18 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { reqTokens = auth.CredentialsFromRequest(r).Tokens } - formToken := r.FormValue("api_token") + r.ParseForm() origin := r.Header.Get("Origin") cors := origin != "" && !strings.HasSuffix(origin, "://"+r.Host) safeAjax := cors && (r.Method == http.MethodGet || r.Method == http.MethodHead) - safeAttachment := attachment && r.URL.Query().Get("api_token") == "" - if formToken == "" { + // Important distinction: safeAttachment checks whether api_token exists + // as a query parameter. haveFormTokens checks whether api_token exists + // as request form data *or* a query parameter. Different checks are + // necessary because both the request disposition and the location of + // the API token affect whether or not the request needs to be + // redirected. The different branch comments below explain further. + safeAttachment := attachment && !r.URL.Query().Has("api_token") + if formTokens, haveFormTokens := r.Form["api_token"]; !haveFormTokens { // No token to use or redact. } else if safeAjax || safeAttachment { // If this is a cross-origin request, the URL won't @@ -313,7 +319,9 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) { // form?" problem, so provided the token isn't // embedded in the URL, there's no reason to do // redirect-with-cookie in this case either. - reqTokens = append(reqTokens, formToken) + for _, tok := range formTokens { + reqTokens = append(reqTokens, tok) + } } else if browserMethod[r.Method] { // If this is a page view, and the client provided a // token via query string or POST body, we must put @@ -776,7 +784,7 @@ func applyContentDispositionHdr(w http.ResponseWriter, r *http.Request, filename } func (h *handler) seeOtherWithCookie(w http.ResponseWriter, r *http.Request, location string, credentialsOK bool) { - if formToken := r.FormValue("api_token"); formToken != "" { + if formTokens, haveFormTokens := r.Form["api_token"]; haveFormTokens { if !credentialsOK { // It is not safe to copy the provided token // into a cookie unless the current vhost @@ -797,13 +805,19 @@ func (h *handler) seeOtherWithCookie(w http.ResponseWriter, r *http.Request, loc // bar, and in the case of a POST request to avoid // raising warnings when the user refreshes the // resulting page. - http.SetCookie(w, &http.Cookie{ - Name: "arvados_api_token", - Value: auth.EncodeTokenCookie([]byte(formToken)), - Path: "/", - HttpOnly: true, - SameSite: http.SameSiteLaxMode, - }) + for _, tok := range formTokens { + if tok == "" { + continue + } + http.SetCookie(w, &http.Cookie{ + Name: "arvados_api_token", + Value: auth.EncodeTokenCookie([]byte(tok)), + Path: "/", + HttpOnly: true, + SameSite: http.SameSiteLaxMode, + }) + break + } } // Propagate query parameters (except api_token) from diff --git a/services/keep-web/handler_test.go b/services/keep-web/handler_test.go index 4a76276392..5e81f3a01e 100644 --- a/services/keep-web/handler_test.go +++ b/services/keep-web/handler_test.go @@ -797,6 +797,34 @@ func (s *IntegrationSuite) TestVhostRedirectQueryTokenAttachmentOnlyHost(c *chec c.Check(resp.Header().Get("Content-Disposition"), check.Equals, "attachment") } +func (s *IntegrationSuite) TestVhostRedirectMultipleTokens(c *check.C) { + baseUrl := arvadostest.FooCollection + ".example.com/foo" + query := url.Values{} + + // The intent of these tests is to check that requests are redirected + // correctly in the presence of multiple API tokens. The exact response + // codes and content are not closely considered: they're just how + // keep-web responded when we made the smallest possible fix. Changing + // those responses may be okay, but you should still test all these + // different cases and the associated redirect logic. + query["api_token"] = []string{arvadostest.ActiveToken, arvadostest.AnonymousToken} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusOK, "foo") + query["api_token"] = []string{arvadostest.ActiveToken, arvadostest.AnonymousToken, ""} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusOK, "foo") + query["api_token"] = []string{arvadostest.ActiveToken, "", arvadostest.AnonymousToken} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusOK, "foo") + query["api_token"] = []string{"", arvadostest.ActiveToken} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusOK, "foo") + + expectContent := regexp.QuoteMeta(unauthorizedMessage + "\n") + query["api_token"] = []string{arvadostest.AnonymousToken, "invalidtoo"} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusUnauthorized, expectContent) + query["api_token"] = []string{arvadostest.AnonymousToken, ""} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusUnauthorized, expectContent) + query["api_token"] = []string{"", arvadostest.AnonymousToken} + s.testVhostRedirectTokenToCookie(c, "GET", baseUrl, "?"+query.Encode(), nil, "", http.StatusUnauthorized, expectContent) +} + func (s *IntegrationSuite) TestVhostRedirectPOSTFormTokenToCookie(c *check.C) { s.testVhostRedirectTokenToCookie(c, "POST", arvadostest.FooCollection+".example.com/foo", @@ -999,20 +1027,36 @@ func (s *IntegrationSuite) testVhostRedirectTokenToCookie(c *check.C, method, ho s.handler.ServeHTTP(resp, req) if resp.Code != http.StatusSeeOther { + attachment, _ := regexp.MatchString(`^attachment(;|$)`, resp.Header().Get("Content-Disposition")) + // Since we're not redirecting, check that any api_token in the URL is + // handled safely. + // If there is no token in the URL, then we're good. + // Otherwise, if the response code is an error, the body is expected to + // be static content, and nothing that might maliciously introspect the + // URL. It's considered safe and allowed. + // Otherwise, if the response content has attachment disposition, + // that's considered safe for all the reasons explained in the + // safeAttachment comment in handler.go. + c.Check(!u.Query().Has("api_token") || resp.Code >= 400 || attachment, check.Equals, true) return resp } + + loc, err := url.Parse(resp.Header().Get("Location")) + c.Assert(err, check.IsNil) + c.Check(loc.Scheme, check.Equals, u.Scheme) + c.Check(loc.Host, check.Equals, u.Host) + c.Check(loc.RawPath, check.Equals, u.RawPath) + // If the response was a redirect, it should never include an API token. + c.Check(loc.Query().Has("api_token"), check.Equals, false) c.Check(resp.Body.String(), check.Matches, `.*href="http://`+regexp.QuoteMeta(html.EscapeString(hostPath))+`(\?[^"]*)?".*`) - c.Check(strings.Split(resp.Header().Get("Location"), "?")[0], check.Equals, "http://"+hostPath) cookies := (&http.Response{Header: resp.Header()}).Cookies() - u, err := u.Parse(resp.Header().Get("Location")) - c.Assert(err, check.IsNil) c.Logf("following redirect to %s", u) req = &http.Request{ Method: "GET", - Host: u.Host, - URL: u, - RequestURI: u.RequestURI(), + Host: loc.Host, + URL: loc, + RequestURI: loc.RequestURI(), Header: reqHeader, } for _, c := range cookies { -- 2.30.2