Use proper oauth state (#3847)

.vscode/settings.json

@@ -8,6 +8,7 @@
   },
   "go.lintTool": "golangci-lint",
   "go.lintFlags": ["--fast"],
+  "go.buildTags": "test",
   "eslint.workingDirectories": ["./web"],
   "prettier.ignorePath": "./web/.prettierignore",
   // Enable the ESlint flat config support

cmd/server/server.go

@@ -265,6 +265,13 @@ func run(c *cli.Context) error {
 }
 
 func setupEvilGlobals(c *cli.Context, s store.Store) error {
+	// secrets
+	var err error
+	server.Config.Server.JWTSecret, err = setupJWTSecret(s)
+	if err != nil {
+		return fmt.Errorf("could not setup jwt secret: %w", err)
+	}
+
 	// services
 	server.Config.Services.Queue = setupQueue(c, s)
 	server.Config.Services.Logs = logging.New()

cmd/server/setup.go

@@ -17,10 +17,13 @@ package main
 
 import (
 	"context"
+	"encoding/base32"
+	"errors"
 	"fmt"
 	"os"
 	"time"
 
+	"github.com/gorilla/securecookie"
 	"github.com/prometheus/client_golang/prometheus"
 	prometheus_auto "github.com/prometheus/client_golang/prometheus/promauto"
 	"github.com/rs/zerolog/log"
@@ -34,6 +37,7 @@ import (
 	"go.woodpecker-ci.org/woodpecker/v2/server/services/log/file"
 	"go.woodpecker-ci.org/woodpecker/v2/server/store"
 	"go.woodpecker-ci.org/woodpecker/v2/server/store/datastore"
+	"go.woodpecker-ci.org/woodpecker/v2/server/store/types"
 )
 
 func setupStore(c *cli.Context) (store.Store, error) {
@@ -165,3 +169,26 @@ func setupLogStore(c *cli.Context, s store.Store) (logService.Service, error) {
 		return s, nil
 	}
 }
+
+const jwtSecretID = "jwt-secret"
+
+func setupJWTSecret(_store store.Store) (string, error) {
+	jwtSecret, err := _store.ServerConfigGet(jwtSecretID)
+	if errors.Is(err, types.RecordNotExist) {
+		jwtSecret := base32.StdEncoding.EncodeToString(
+			securecookie.GenerateRandomKey(32),
+		)
+		err = _store.ServerConfigSet(jwtSecretID, jwtSecret)
+		if err != nil {
+			return "", err
+		}
+		log.Debug().Msg("created jwt secret")
+		return jwtSecret, nil
+	}
+
+	if err != nil {
+		return "", err
+	}
+
+	return jwtSecret, nil
+}

server/api/hook.go

@@ -104,7 +104,7 @@ func BlockTilQueueHasRunningItem(c *gin.Context) {
 func PostHook(c *gin.Context) {
 	_store := store.FromContext(c)
 
-	_forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get the forge for the specific repo somehow
+	_forge, err := server.Config.Services.Manager.ForgeByID(1) // TODO: get the forge for the specific repo somehow
 	if err != nil {
 		log.Error().Err(err).Msg("Cannot get main forge")
 		c.AbortWithStatus(http.StatusInternalServerError)

server/api/login.go

@@ -37,6 +37,8 @@ import (
 	"go.woodpecker-ci.org/woodpecker/v2/shared/token"
 )
 
+const stateTokenDuration = time.Minute * 5
+
 func HandleAuth(c *gin.Context) {
 	// TODO: check if this is really needed
 	c.Writer.Header().Del("Content-Type")
@@ -56,17 +58,45 @@ func HandleAuth(c *gin.Context) {
 	}
 
 	_store := store.FromContext(c)
+
+	code := c.Request.FormValue("code")
+	state := c.Request.FormValue("state")
+	isCallback := code != "" && state != ""
 	forgeID := int64(1) // TODO: replace with forge id when multiple forges are supported
-	_forge, err := server.Config.Services.Manager.ForgeMain()
+
+	if isCallback { // validate the state token
+		_, err := token.Parse([]token.Type{token.OAuthStateToken}, state, func(_ *token.Token) (string, error) {
+			return server.Config.Server.JWTSecret, nil
+		})
+		if err != nil {
+			log.Error().Err(err).Msg("cannot verify state token")
+			c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=invalid_state")
+			return
+		}
+	} else { // only generate a state token if not a callback
+		var err error
+		jwtSecret := server.Config.Server.JWTSecret
+		exp := time.Now().Add(stateTokenDuration).Unix()
+		stateToken := token.New(token.OAuthStateToken)
+		stateToken.Set("forge-id", strconv.FormatInt(forgeID, 10))
+		state, err = stateToken.SignExpires(jwtSecret, exp)
+		if err != nil {
+			log.Error().Err(err).Msg("cannot create state token")
+			c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
+			return
+		}
+	}
+
+	_forge, err := server.Config.Services.Manager.ForgeByID(forgeID)
 	if err != nil {
-		log.Error().Err(err).Msg("Cannot get main forge")
+		log.Error().Err(err).Msgf("Cannot get forge by id %d", forgeID)
 		c.Redirect(http.StatusSeeOther, server.Config.Server.RootPath+"/login?error=internal_error")
 		return
 	}
 
 	userFromForge, redirectURL, err := _forge.Login(c, &forge_types.OAuthRequest{
 		Code:  c.Request.FormValue("code"),
-		State: "woodpecker", // TODO: use proper state
+		State: state,
 	})
 	if err != nil {
 		log.Error().Err(err).Msg("cannot authenticate user")
@@ -250,7 +280,7 @@ func GetLogout(c *gin.Context) {
 func DeprecatedGetLoginToken(c *gin.Context) {
 	_store := store.FromContext(c)
 
-	_forge, err := server.Config.Services.Manager.ForgeMain() // TODO: get selected forge from auth request
+	_forge, err := server.Config.Services.Manager.ForgeByID(1) // TODO: get selected forge from auth request
 	if err != nil {
 		log.Error().Err(err).Msg("Cannot get main forge")
 		c.AbortWithStatus(http.StatusInternalServerError)

server/api/login_test.go

@@ -1,6 +1,7 @@
 package api_test
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -16,11 +17,13 @@ import (
 	"go.woodpecker-ci.org/woodpecker/v2/server"
 	"go.woodpecker-ci.org/woodpecker/v2/server/api"
 	mocks_forge "go.woodpecker-ci.org/woodpecker/v2/server/forge/mocks"
+	forge_types "go.woodpecker-ci.org/woodpecker/v2/server/forge/types"
 	"go.woodpecker-ci.org/woodpecker/v2/server/model"
 	mocks_services "go.woodpecker-ci.org/woodpecker/v2/server/services/mocks"
 	"go.woodpecker-ci.org/woodpecker/v2/server/services/permissions"
 	mocks_store "go.woodpecker-ci.org/woodpecker/v2/server/store/mocks"
 	"go.woodpecker-ci.org/woodpecker/v2/server/store/types"
+	"go.woodpecker-ci.org/woodpecker/v2/shared/token"
 )
 
 func TestHandleAuth(t *testing.T) {
@@ -69,12 +72,37 @@ func TestHandleAuth(t *testing.T) {
 			assert.Equal(g, fmt.Sprintf("/login?%s", query.Encode()), c.Writer.Header().Get("Location"))
 		})
 
-		g.It("should fail if a code was provided and no state", func() {
-			// TODO: implement
-		})
-
 		g.It("should fail if the state is wrong", func() {
-			// TODO: implement
+			_manager := mocks_services.NewManager(t)
+			_store := mocks_store.NewStore(t)
+			server.Config.Services.Manager = _manager
+			server.Config.Permissions.Open = true
+			server.Config.Permissions.Orgs = permissions.NewOrgs(nil)
+			server.Config.Permissions.Admins = permissions.NewAdmins(nil)
+			w := httptest.NewRecorder()
+			c, _ := gin.CreateTestContext(w)
+			c.Set("store", _store)
+
+			query := url.Values{}
+			query.Set("code", "assumed_to_be_valid_code")
+
+			wrongToken := token.New(token.OAuthStateToken)
+			wrongToken.Set("forge_id", "1")
+			signedWrongToken, _ := wrongToken.Sign("wrong_secret")
+			query.Set("state", signedWrongToken)
+
+			c.Request = &http.Request{
+				Header: make(http.Header),
+				URL: &url.URL{
+					Scheme:   "https",
+					RawQuery: query.Encode(),
+				},
+			}
+
+			api.HandleAuth(c)
+
+			assert.Equal(g, http.StatusSeeOther, c.Writer.Status())
+			assert.Equal(g, "/login?error=invalid_state", c.Writer.Header().Get("Location"))
 		})
 
 		g.It("should redirect to forge login page", func() {
@@ -95,10 +123,17 @@ func TestHandleAuth(t *testing.T) {
 				},
 			}
 
-			forgeRedirectURL := "https://my-awesome-forge.com/oauth/authorize?client_id=client-id"
+			_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 
-			_manager.On("ForgeMain").Return(_forge, nil)
+			forgeRedirectURL := ""
-			_forge.On("Login", mock.Anything, mock.Anything).Return(nil, forgeRedirectURL, nil)
+			_forge.On("Login", mock.Anything, mock.Anything).Run(func(args mock.Arguments) {
+				state, ok := args.Get(1).(*forge_types.OAuthRequest)
+				if ok {
+					forgeRedirectURL = fmt.Sprintf("https://my-awesome-forge.com/oauth/authorize?client_id=client-id&state=%s", state.State)
+				}
+			}).Return(nil, func(context.Context, *forge_types.OAuthRequest) string {
+				return forgeRedirectURL
+			}, nil)
 
 			api.HandleAuth(c)
 
@@ -124,7 +159,7 @@ func TestHandleAuth(t *testing.T) {
 				},
 			}
 
-			_manager.On("ForgeMain").Return(_forge, nil)
+			_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 			_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 			_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist)
 			_store.On("CreateUser", mock.Anything).Return(nil)
@@ -158,7 +193,7 @@ func TestHandleAuth(t *testing.T) {
 				},
 			}
 
-			_manager.On("ForgeMain").Return(_forge, nil)
+			_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 			_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 			_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
 			_store.On("OrgGet", org.ID).Return(org, nil)
@@ -190,7 +225,7 @@ func TestHandleAuth(t *testing.T) {
 				},
 			}
 
-			_manager.On("ForgeMain").Return(_forge, nil)
+			_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 			_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 			_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(nil, types.RecordNotExist)
 
@@ -218,7 +253,7 @@ func TestHandleAuth(t *testing.T) {
 				},
 			}
 
-			_manager.On("ForgeMain").Return(_forge, nil)
+			_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 			_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 			_forge.On("Teams", mock.Anything, user).Return([]*model.Team{
 				{
@@ -252,7 +287,7 @@ func TestHandleAuth(t *testing.T) {
 				}
 				user.OrgID = 0
 
-				_manager.On("ForgeMain").Return(_forge, nil)
+				_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 				_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 				_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
 				_store.On("OrgFindByName", user.Login).Return(nil, types.RecordNotExist)
@@ -286,7 +321,7 @@ func TestHandleAuth(t *testing.T) {
 				}
 				user.OrgID = 0
 
-				_manager.On("ForgeMain").Return(_forge, nil)
+				_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 				_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 				_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
 				_store.On("OrgFindByName", user.Login).Return(org, nil)
@@ -320,7 +355,7 @@ func TestHandleAuth(t *testing.T) {
 				}
 				org.Name = "not-the-user-name"
 
-				_manager.On("ForgeMain").Return(_forge, nil)
+				_manager.On("ForgeByID", int64(1)).Return(_forge, nil)
 				_forge.On("Login", mock.Anything, mock.Anything).Return(user, "", nil)
 				_store.On("GetUserRemoteID", user.ForgeRemoteID, user.Login).Return(user, nil)
 				_store.On("OrgGet", user.OrgID).Return(org, nil)

server/config.go

@@ -38,6 +38,7 @@ var Config = struct {
 		LogStore   log.Service
 	}
 	Server struct {
+		JWTSecret           string
 		Key                 string
 		Cert                string
 		OAuthHost           string

server/services/manager.go

@@ -46,7 +46,7 @@ type Manager interface {
 	EnvironmentService() environment.Service
 	ForgeFromRepo(repo *model.Repo) (forge.Forge, error)
 	ForgeFromUser(user *model.User) (forge.Forge, error)
-	ForgeMain() (forge.Forge, error)
+	ForgeByID(id int64) (forge.Forge, error)
 }
 
 type manager struct {
@@ -120,18 +120,14 @@ func (m *manager) EnvironmentService() environment.Service {
 }
 
 func (m *manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) {
-	return m.getForgeByID(repo.ForgeID)
+	return m.ForgeByID(repo.ForgeID)
 }
 
 func (m *manager) ForgeFromUser(user *model.User) (forge.Forge, error) {
-	return m.getForgeByID(user.ForgeID)
+	return m.ForgeByID(user.ForgeID)
 }
 
-func (m *manager) ForgeMain() (forge.Forge, error) {
+func (m *manager) ForgeByID(id int64) (forge.Forge, error) {
-	return m.getForgeByID(1) // main forge is always 1 and is configured via environment variables
-}
-
-func (m *manager) getForgeByID(id int64) (forge.Forge, error) {
 	item := m.forgeCache.Get(id)
 	if item != nil && !item.IsExpired() {
 		return item.Value(), nil

server/services/mocks/manager.go

@@ -68,6 +68,36 @@ func (_m *Manager) EnvironmentService() environment.Service {
 	return r0
 }
 
+// ForgeByID provides a mock function with given fields: id
+func (_m *Manager) ForgeByID(id int64) (forge.Forge, error) {
+	ret := _m.Called(id)
+
+	if len(ret) == 0 {
+		panic("no return value specified for ForgeByID")
+	}
+
+	var r0 forge.Forge
+	var r1 error
+	if rf, ok := ret.Get(0).(func(int64) (forge.Forge, error)); ok {
+		return rf(id)
+	}
+	if rf, ok := ret.Get(0).(func(int64) forge.Forge); ok {
+		r0 = rf(id)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(forge.Forge)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(int64) error); ok {
+		r1 = rf(id)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // ForgeFromRepo provides a mock function with given fields: repo
 func (_m *Manager) ForgeFromRepo(repo *model.Repo) (forge.Forge, error) {
 	ret := _m.Called(repo)
@@ -128,36 +158,6 @@ func (_m *Manager) ForgeFromUser(user *model.User) (forge.Forge, error) {
 	return r0, r1
 }
 
-// ForgeMain provides a mock function with given fields:
-func (_m *Manager) ForgeMain() (forge.Forge, error) {
-	ret := _m.Called()
-
-	if len(ret) == 0 {
-		panic("no return value specified for ForgeMain")
-	}
-
-	var r0 forge.Forge
-	var r1 error
-	if rf, ok := ret.Get(0).(func() (forge.Forge, error)); ok {
-		return rf()
-	}
-	if rf, ok := ret.Get(0).(func() forge.Forge); ok {
-		r0 = rf()
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(forge.Forge)
-		}
-	}
-
-	if rf, ok := ret.Get(1).(func() error); ok {
-		r1 = rf()
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
 // RegistryService provides a mock function with given fields:
 func (_m *Manager) RegistryService() registry.Service {
 	ret := _m.Called()

shared/token/token.go

@@ -27,11 +27,12 @@ type SecretFunc func(*Token) (string, error)
 type Type string
 
 const (
-	UserToken  Type = "user" // user token (exp cli)
+	UserToken       Type = "user" // user token (exp cli)
-	SessToken  Type = "sess" // session token (ui token requires csrf check)
+	SessToken       Type = "sess" // session token (ui token requires csrf check)
-	HookToken  Type = "hook" // repo hook token
+	HookToken       Type = "hook" // repo hook token
-	CsrfToken  Type = "csrf"
+	CsrfToken       Type = "csrf"
-	AgentToken Type = "agent"
+	AgentToken      Type = "agent"
+	OAuthStateToken Type = "oauth-state"
 )
 
 // SignerAlgo id default algorithm used to sign JWT tokens.

web/src/assets/locales/en.json

@@ -452,5 +452,6 @@
   "oauth_error": "Error while authenticating against OAuth provider",
   "internal_error": "Some internal error occurred",
   "registration_closed": "The registration is closed",
-  "access_denied": "You are not allowed to access this instance"
+  "access_denied": "You are not allowed to access this instance",
+  "invalid_state": "The OAuth state is invalid"
 }

web/src/views/Login.vue

@@ -52,6 +52,7 @@ const authErrorMessages = {
   internal_error: i18n.t('internal_error'),
   registration_closed: i18n.t('registration_closed'),
   access_denied: i18n.t('access_denied'),
+  invalid_state: i18n.t('invalid_state'),
 };
 
 const errorMessage = ref<string>();