Merge pull request #143 from buzzfeed/jhines-remove-empty-cookie

jphines · web-flow · commit 26fbd098e490 · 2019-01-18T17:25:41.000-05:00
proxy: remove empty cookie from proxy and ignore in signer
diff --git a/internal/proxy/oauthproxy.go b/internal/proxy/oauthproxy.go
@@ -112,6 +112,14 @@ func deleteSSOCookieHeader(req *http.Request, cookieName string) {
 			headers = append(headers, cookie.String())
 		}
 	}
+
+	if len(headers) == 0 {
+		// there are no cookies other then session cookie so we delete the header entirely
+		req.Header.Del("Cookie")
+		return
+	}
+
+	// if there are other headers to keep, we set them minus the session cookie
 	req.Header.Set("Cookie", strings.Join(headers, ";"))
 }
 
diff --git a/internal/proxy/request_signer.go b/internal/proxy/request_signer.go
@@ -96,8 +96,8 @@ func NewRequestSigner(signingKeyPemStr string) (*RequestSigner, error) {
 //   <URL>
 //   <BODY>
 //  where:
-//    <HEADER.k> is the ','-joined concatenation of all header values of `signedHeaders[k]`; all
-//      other headers in the request are ignored,
+//    <HEADER.k> is the ','-joined concatenation of all header values of `signedHeaders[k]`; empty
+//      values such as '' and all other headers in the request are ignored,
 //    <URL> is the string "<PATH>(?<QUERY>)(#FRAGMENT)", where "?<QUERY>" and "#<FRAGMENT>" are
 //      ommitted if the associated components are absent from the request URL,
 //    <BODY> is the body of the Request (may be `nil`; e.g. for GET requests).
@@ -109,7 +109,8 @@ func mapRequestToHashInput(req *http.Request) (string, error) {
 
 	// Add signed headers.
 	for _, hdr := range signedHeaders {
-		if hdrValues := req.Header[hdr]; len(hdrValues) > 0 {
+		hdrValues := removeEmpty(req.Header[hdr])
+		if len(hdrValues) > 0 {
 			entries = append(entries, strings.Join(hdrValues, ","))
 		}
 	}
@@ -189,3 +190,13 @@ func (signer RequestSigner) Sign(req *http.Request) error {
 func (signer RequestSigner) PublicKey() (string, string) {
 	return signer.publicKeyID, signer.publicKeyStr
 }
+
+func removeEmpty(s []string) []string {
+	r := []string{}
+	for _, str := range s {
+		if len(str) > 0 {
+			r = append(r, str)
+		}
+	}
+	return r
+}
diff --git a/internal/proxy/request_signer_test.go b/internal/proxy/request_signer_test.go
@@ -27,6 +27,7 @@ func addHeaders(req *http.Request, examples []string, extras map[string][]string
 		"Content-Type":       {"application/json"},
 		"Date":               {"2018-11-08"},
 		"Authorization":      {"Bearer ab12cd34"},
+		"Cookie":             {""},
 		"X-Forwarded-User":   {"octoboi"},
 		"X-Forwarded-Email":  {"octoboi@example.com"},
 		"X-Forwarded-Groups": {"molluscs", "security_applications"},

Original file line number	Diff line number	Diff line change
`@@ -112,6 +112,14 @@ func deleteSSOCookieHeader(req *http.Request, cookieName string) {`
`112`	`112`	`headers = append(headers, cookie.String())`
`113`	`113`	`}`
`114`	`114`	`}`
	`115`	`+`
	`116`	`+ if len(headers) == 0 {`
	`117`	`+ // there are no cookies other then session cookie so we delete the header entirely`
	`118`	`+ req.Header.Del("Cookie")`
	`119`	`+ return`
	`120`	`+ }`
	`121`	`+`
	`122`	`+ // if there are other headers to keep, we set them minus the session cookie`
`115`	`123`	`req.Header.Set("Cookie", strings.Join(headers, ";"))`
`116`	`124`	`}`
`117`	`125`