From 62ecb121791f8120430b280775240830266fb45e Mon Sep 17 00:00:00 2001 From: Lukas Gruber Date: Tue, 12 Jul 2022 15:48:54 +0200 Subject: [PATCH] fix: do not use secure cookies if protocol is http --- pkg/api/auth.go | 52 ++++++++++---------------- pkg/api/auth_test.go | 87 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 99 insertions(+), 40 deletions(-) diff --git a/pkg/api/auth.go b/pkg/api/auth.go index ec0cc13..339e8fa 100644 --- a/pkg/api/auth.go +++ b/pkg/api/auth.go @@ -123,6 +123,17 @@ type AuthApiService struct { publicHttpPort int } +func (a AuthApiService) cookie(name, value, path string) http.Cookie { + return http.Cookie{ + Name: name, + Path: path, + Value: value, + Secure: a.publicHttpProtocol == "https", + HttpOnly: true, + SameSite: http.SameSiteNoneMode, + } +} + func (a AuthApiService) jwtTokenHeaders(t oauth2.Token, generateRefreshToken bool) (map[string][]string, error) { headers := make(map[string][]string) var cookies []string @@ -131,14 +142,7 @@ func (a AuthApiService) jwtTokenHeaders(t oauth2.Token, generateRefreshToken boo if err != nil { return nil, errors.New("could not sign access jwt") } - accessTokenCookie := http.Cookie{ - Name: "jwt", - Path: "/api", - Value: accessToken, - Secure: true, - HttpOnly: true, - SameSite: http.SameSiteNoneMode, - } + accessTokenCookie := a.cookie("jwt", accessToken, "/api") cookies = append(cookies, accessTokenCookie.String()) if generateRefreshToken { @@ -147,15 +151,8 @@ func (a AuthApiService) jwtTokenHeaders(t oauth2.Token, generateRefreshToken boo if err != nil { return nil, errors.New("could not sign refresh jwt") } - refreshTokenCookie := http.Cookie{ - Name: "jwt-refresh", - Path: "/api/auth", - Value: refreshToken, - Expires: time.Now().Add(time.Hour * 24), - Secure: true, - HttpOnly: true, - SameSite: http.SameSiteNoneMode, - } + refreshTokenCookie := a.cookie("jwt-refresh", refreshToken, "/api/auth") + refreshTokenCookie.Expires = time.Now().Add(refreshTokenExpiry) cookies = append(cookies, refreshTokenCookie.String()) } @@ -165,23 +162,12 @@ func (a AuthApiService) jwtTokenHeaders(t oauth2.Token, generateRefreshToken boo } func (a AuthApiService) AuthLogoutGet(ctx context.Context) (openapi.ImplResponse, error) { - accessTokenCookie := http.Cookie{ - Name: "jwt", - Path: "/api", - Expires: time.Unix(0, 0), - Secure: true, - HttpOnly: true, - SameSite: http.SameSiteNoneMode, - } + accessTokenCookie := a.cookie("jwt", "1", "/api") + accessTokenCookie.Expires = time.Unix(0, 0) + // todo: revoke refresh token => delete from database - refreshTokenCookie := http.Cookie{ - Name: "jwt-refresh", - Path: "/api/auth/refresh", - Expires: time.Unix(0, 0), - Secure: true, - HttpOnly: true, - SameSite: http.SameSiteNoneMode, - } + refreshTokenCookie := a.cookie("jwt-refresh", "1", "/api/auth") + refreshTokenCookie.Expires = time.Unix(0, 0) headers := make(map[string][]string) headers["Set-Cookie"] = []string{accessTokenCookie.String(), refreshTokenCookie.String()} diff --git a/pkg/api/auth_test.go b/pkg/api/auth_test.go index 9ed6ea5..9453045 100644 --- a/pkg/api/auth_test.go +++ b/pkg/api/auth_test.go @@ -140,6 +140,78 @@ func TestAuthenticationMiddleware(t *testing.T) { }) } +func TestAuthApiService_AuthLoginPost_secure_cookies(t *testing.T) { + t.Run("if protocol is http do not use secure cookies", func(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + httpmock.RegisterResponder("GET", "=~/me$", + func(req *http.Request) (*http.Response, error) { + return httpmock.NewJsonResponse(200, map[string]interface{}{ + "email": "user@test.com", + }) + }, + ) + + httpmock.RegisterResponder("POST", "=~/api/token", + func(req *http.Request) (*http.Response, error) { + return httpmock.NewJsonResponse(200, map[string]interface{}{ + "access_token": "access-token", + "token_type": "Bearer", + "refresh_token": "refresh_token", + }) + }, + ) + + ctx := context.Background() + loginRequest := openapi.AuthLoginPostRequest{Code: "oauth-code"} + + auth := AuthApiService{ + publicHttpProtocol: "http", + } + res, _ := auth.AuthLoginPost(ctx, loginRequest) + + cookies := parseCookies(res.Headers["Set-Cookie"]) + assert.False(t, cookies[0].Secure) + assert.False(t, cookies[1].Secure) + }) + + t.Run("if protocol is https define cookies as secure", func(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + httpmock.RegisterResponder("GET", "=~/me$", + func(req *http.Request) (*http.Response, error) { + return httpmock.NewJsonResponse(200, map[string]interface{}{ + "email": "user@test.com", + }) + }, + ) + + httpmock.RegisterResponder("POST", "=~/api/token", + func(req *http.Request) (*http.Response, error) { + return httpmock.NewJsonResponse(200, map[string]interface{}{ + "access_token": "access-token", + "token_type": "Bearer", + "refresh_token": "refresh_token", + }) + }, + ) + + ctx := context.Background() + loginRequest := openapi.AuthLoginPostRequest{Code: "oauth-code"} + + auth := AuthApiService{ + publicHttpProtocol: "https", + } + res, _ := auth.AuthLoginPost(ctx, loginRequest) + + cookies := parseCookies(res.Headers["Set-Cookie"]) + assert.True(t, cookies[0].Secure) + assert.True(t, cookies[1].Secure) + }) +} + func TestAuthApiService_AuthLoginPost(t *testing.T) { httpmock.Activate() defer httpmock.DeactivateAndReset() @@ -171,8 +243,12 @@ func TestAuthApiService_AuthLoginPost(t *testing.T) { assert.Nil(t, err) assert.Equal(t, http.StatusOK, res.Code) assert.Len(t, res.Headers["Set-Cookie"], 2, "should set two cookies ") - assert.Contains(t, res.Headers["Set-Cookie"][0], "jwt=", "should set the cookie `jwt` ") - assert.Contains(t, res.Headers["Set-Cookie"][1], "jwt-refresh=", "should set the cookie `jwt-refresh` ") + c := parseCookies(res.Headers["Set-Cookie"]) + + assert.Equal(t, "jwt", c[0].Name, "should set the cookie `jwt`") + assert.Equal(t, "/api", c[0].Path, "`jwt` should be valid for path /api") + assert.Equal(t, "jwt-refresh", c[1].Name, "should set the cookie `jwt-refresh`") + assert.Equal(t, "/api/auth", c[1].Path, "`jwt-refresh` should only be valid for path /api/auth") } func TestAuthApiService_AuthLogoutGet(t *testing.T) { @@ -182,9 +258,6 @@ func TestAuthApiService_AuthLogoutGet(t *testing.T) { assert.Nil(t, err) assert.Len(t, res.Headers["Set-Cookie"], 2, "should set two cookies") - assert.Contains(t, res.Headers["Set-Cookie"][0], "jwt=", "should set the cookie `jwt` ") - assert.Contains(t, res.Headers["Set-Cookie"][1], "jwt-refresh=", "should set the cookie `jwt-refresh` ") - c := parseCookies(res.Headers["Set-Cookie"]) assert.True(t, c[0].Expires.Before(time.Now()), "jwt cookie should be expired") assert.True(t, c[1].Expires.Before(time.Now()), "jwt-refresh cookie should be expired") @@ -193,8 +266,8 @@ func TestAuthApiService_AuthLogoutGet(t *testing.T) { func parseCookies(cookieHeaders []string) []*http.Cookie { header := http.Header{} for i := range cookieHeaders { - header.Add("Cookie", cookieHeaders[i]) + header.Add("Set-Cookie", cookieHeaders[i]) } - req := http.Request{Header: header} + req := http.Response{Header: header} return req.Cookies() }