Skip to content

Commit d832b1a

Browse files
committed
fix: prevent csrf for cookie requests
Prevously, the token was passed as X-Gotify-Key by the UI, so there was no csrf because no cookie was added by the browser to the request. The cookie is saved by SameSite=strict, this provides some protection against csrf. But an subdomain takeover could still allow for csrf. E.g. evil.gotify.net could send authenticated requests to gotify.net. This uses the go builtin cross origin protection, listed on the owasp page: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#built-in-or-existing-csrf-implementations
1 parent 50b917a commit d832b1a

3 files changed

Lines changed: 83 additions & 2 deletions

File tree

auth/authentication.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package auth
22

33
import (
44
"errors"
5+
"net/http"
56
"strings"
67
"time"
78

@@ -40,6 +41,7 @@ type Database interface {
4041
type Auth struct {
4142
DB Database
4243
SecureCookie bool
44+
CrossOrigin *http.CrossOriginProtection
4345
}
4446

4547
// RequireAdmin requires an elevated client token or basic auth, the user must be an admin.
@@ -87,6 +89,9 @@ func (a *Auth) Optional(ctx *gin.Context) {
8789
}
8890

8991
func (a *Auth) evaluate(ctx *gin.Context, funcs ...func(ctx *gin.Context) (authState, error)) bool {
92+
if a.rejectForeignOrigin(ctx) {
93+
return true
94+
}
9095
for _, fn := range funcs {
9196
state, err := fn(ctx)
9297
if err != nil {
@@ -128,6 +133,17 @@ func (a *Auth) abort403(ctx *gin.Context) {
128133
ctx.AbortWithError(403, errors.New("you are not allowed to access this api"))
129134
}
130135

136+
func (a *Auth) rejectForeignOrigin(ctx *gin.Context) bool {
137+
if _, isCookie := a.readTokenFromRequest(ctx); !isCookie {
138+
return false
139+
}
140+
if err := a.CrossOrigin.Check(ctx.Request); err != nil {
141+
ctx.AbortWithError(403, err)
142+
return true
143+
}
144+
return false
145+
}
146+
131147
func (a *Auth) handleUser(checks ...func(*model.User) (authState, error)) func(ctx *gin.Context) (authState, error) {
132148
return func(ctx *gin.Context) (authState, error) {
133149
if name, pass, ok := ctx.Request.BasicAuth(); ok {

auth/authentication_test.go

Lines changed: 62 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ type AuthenticationSuite struct {
2929
func (s *AuthenticationSuite) SetupSuite() {
3030
mode.Set(mode.TestDev)
3131
s.DB = testdb.NewDB(s.T())
32-
s.auth = &Auth{DB: s.DB}
32+
s.auth = &Auth{DB: s.DB, CrossOrigin: http.NewCrossOriginProtection()}
3333

3434
now := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC)
3535
timeNow = func() time.Time { return now }
@@ -354,4 +354,65 @@ func (s *AuthenticationSuite) assertHeaderRequest(key, value string, f fMiddlewa
354354
return ctx
355355
}
356356

357+
func (s *AuthenticationSuite) TestCookieCrossOriginProtection() {
358+
// httptest sets the request host to example.com.
359+
s.assertCsrfRequest(map[string]string{"Origin": "http://example.com"}, "clienttoken", s.auth.RequireClient, 200)
360+
s.assertCsrfRequest(map[string]string{"Origin": "https://example.com"}, "clienttoken", s.auth.RequireClient, 200)
361+
362+
s.assertCsrfRequest(map[string]string{"Origin": "http://evil.com"}, "clienttoken", s.auth.RequireClient, 403)
363+
s.assertCsrfRequest(map[string]string{"Origin": "https://example.com.evil.com"}, "clienttoken", s.auth.RequireClient, 403)
364+
s.assertCsrfRequest(map[string]string{"Origin": "null"}, "clienttoken", s.auth.RequireClient, 403)
365+
366+
s.assertCsrfRequest(nil, "clienttoken", s.auth.RequireClient, 200)
367+
368+
s.assertCsrfRequest(map[string]string{"Sec-Fetch-Site": "same-origin"}, "clienttoken", s.auth.RequireClient, 200)
369+
s.assertCsrfRequest(map[string]string{"Sec-Fetch-Site": "none"}, "clienttoken", s.auth.RequireClient, 200)
370+
s.assertCsrfRequest(map[string]string{"Sec-Fetch-Site": "cross-site"}, "clienttoken", s.auth.RequireClient, 403)
371+
s.assertCsrfRequest(map[string]string{"Sec-Fetch-Site": "same-site"}, "clienttoken", s.auth.RequireClient, 403)
372+
373+
s.assertCsrfRequest(map[string]string{"Sec-Fetch-Site": "cross-site"}, "clienttoken_admin_elevated", s.auth.RequireElevatedClient, 403)
374+
s.assertCsrfRequest(map[string]string{"Sec-Fetch-Site": "same-origin"}, "clienttoken_admin_elevated", s.auth.RequireElevatedClient, 200)
375+
}
376+
377+
func (s *AuthenticationSuite) TestCrossOriginProtectionIgnoredForTokenAuth() {
378+
recorder := httptest.NewRecorder()
379+
ctx, _ := gin.CreateTestContext(recorder)
380+
ctx.Request = httptest.NewRequest("POST", "/", nil)
381+
ctx.Request.Header.Set("X-Gotify-Key", "clienttoken")
382+
ctx.Request.Header.Set("Sec-Fetch-Site", "cross-site")
383+
s.auth.RequireClient(ctx)
384+
assert.Equal(s.T(), 200, recorder.Code)
385+
386+
recorder = httptest.NewRecorder()
387+
ctx, _ = gin.CreateTestContext(recorder)
388+
ctx.Request = httptest.NewRequest("POST", "/?token=clienttoken", nil)
389+
ctx.Request.Header.Set("Sec-Fetch-Site", "cross-site")
390+
s.auth.RequireClient(ctx)
391+
assert.Equal(s.T(), 200, recorder.Code)
392+
}
393+
394+
func (s *AuthenticationSuite) TestCrossOriginProtectionAllowsSafeMethods() {
395+
recorder := httptest.NewRecorder()
396+
ctx, _ := gin.CreateTestContext(recorder)
397+
ctx.Request = httptest.NewRequest("GET", "/", nil)
398+
ctx.Request.AddCookie(&http.Cookie{Name: cookieName, Value: "clienttoken"})
399+
ctx.Request.Header.Set("Sec-Fetch-Site", "cross-site")
400+
s.auth.RequireClient(ctx)
401+
assert.Equal(s.T(), 200, recorder.Code)
402+
}
403+
404+
func (s *AuthenticationSuite) assertCsrfRequest(headers map[string]string, cookie string, f fMiddleware, code int) {
405+
recorder := httptest.NewRecorder()
406+
ctx, _ := gin.CreateTestContext(recorder)
407+
ctx.Request = httptest.NewRequest("POST", "/", nil)
408+
if cookie != "" {
409+
ctx.Request.AddCookie(&http.Cookie{Name: cookieName, Value: cookie})
410+
}
411+
for k, v := range headers {
412+
ctx.Request.Header.Set(k, v)
413+
}
414+
f(ctx)
415+
assert.Equal(s.T(), code, recorder.Code)
416+
}
417+
357418
type fMiddleware gin.HandlerFunc

router/router.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,11 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co
8484
}
8585
}
8686
}()
87-
authentication := auth.Auth{DB: db, SecureCookie: conf.Server.SecureCookie}
87+
authentication := auth.Auth{
88+
DB: db,
89+
SecureCookie: conf.Server.SecureCookie,
90+
CrossOrigin: http.NewCrossOriginProtection(),
91+
}
8892
messageHandler := api.MessageAPI{Notifier: streamHandler, DB: db}
8993
healthHandler := api.HealthAPI{DB: db}
9094
clientHandler := api.ClientAPI{

0 commit comments

Comments
 (0)