Validate GitHub App provider configuration on create

Adds the call to clients.ParseV1AppConfig to the code that creates GH apps and returns the providers.ErrProviderInvalidConfig if the parsing fails. Catches the providers.ErrProviderInvalidConfig in the handler code to display a user-friendly message. Fixes: mindersec#3510
jhrozek · Jun 5, 2024 · 0f2635b · 0f2635b
1 parent f4c9112
commit 0f2635b
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 0 deletions.
diff --git a/internal/controlplane/handlers_oauth.go b/internal/controlplane/handlers_oauth.go
@@ -354,8 +354,13 @@ func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter,
 
 		logger.BusinessRecord(ctx).Project = stateData.ProjectID
 
+		var confErr providers.ErrProviderInvalidConfig
 		_, err = s.ghProviders.CreateGitHubAppProvider(ctx, *token, stateData, installationID, state)
 		if err != nil {
+			if errors.As(err, &confErr) {
+				return newHttpError(http.StatusBadRequest, "Invalid provider config").SetContents(
+					"The provider configuration is invalid: " + confErr.Details)
+			}
 			if errors.Is(err, service.ErrInvalidTokenIdentity) {
 				return newHttpError(http.StatusForbidden, "User token mismatch").SetContents(
 					"The provided login token was associated with a different GitHub user.")

diff --git a/internal/controlplane/handlers_oauth_test.go b/internal/controlplane/handlers_oauth_test.go
@@ -783,6 +783,27 @@ func TestHandleGitHubAppCallback(t *testing.T) {
 				t.Helper()
 				assert.Equal(t, 403, resp.Code)
 			},
+		}, {
+			name:  "Invalid config",
+			state: validState,
+			buildStubs: func(store *mockdb.MockStore, service *mockprovsvc.MockGitHubProviderService, _ *mockgh.MockClientService) {
+				service.EXPECT().
+					ValidateGitHubInstallationId(gomock.Any(), gomock.Any(), installationID).
+					Return(nil)
+				store.EXPECT().
+					GetProjectIDBySessionState(gomock.Any(), validState).
+					Return(db.GetProjectIDBySessionStateRow{
+						ProjectID: uuid.New(),
+					}, nil)
+				service.EXPECT().
+					CreateGitHubAppProvider(gomock.Any(), gomock.Any(), gomock.Any(), installationID, gomock.Any()).
+					Return(nil, providers.NewErrProviderInvalidConfig("invalid config"))
+			},
+			checkResponse: func(t *testing.T, resp httptest.ResponseRecorder) {
+				t.Helper()
+				assert.Equal(t, http.StatusBadRequest, resp.Code)
+				assert.Contains(t, resp.Body.String(), "The provider configuration is invalid: invalid config")
+			},
 		}, {
 			name:        "Request to install",
 			state:       validState,

diff --git a/internal/providers/github/service/service.go b/internal/providers/github/service/service.go
@@ -262,6 +262,11 @@ func (p *ghProviderService) CreateGitHubAppProvider(
 			return nil, fmt.Errorf("error getting provider config: %w", err)
 		}
 
+		_, _, err = clients.ParseV1AppConfig(finalConfig)
+		if err != nil {
+			return nil, providers.NewErrProviderInvalidConfig(err.Error())
+		}
+
 		provider, err := createGitHubApp(
 			ctx,
 			qtx,

diff --git a/internal/providers/github/service/service_test.go b/internal/providers/github/service/service_test.go
@@ -46,6 +46,7 @@ import (
 	mockcrypto "github.com/stacklok/minder/internal/crypto/mock"
 	"github.com/stacklok/minder/internal/db"
 	"github.com/stacklok/minder/internal/db/embedded"
+	"github.com/stacklok/minder/internal/providers"
 	"github.com/stacklok/minder/internal/providers/credentials"
 	"github.com/stacklok/minder/internal/providers/github/clients"
 	mockclients "github.com/stacklok/minder/internal/providers/github/clients/mock"
@@ -276,6 +277,63 @@ func TestProviderService_VerifyProviderTokenIdentity(t *testing.T) {
 	require.ErrorIs(t, err, ErrInvalidTokenIdentity)
 }
 
+func TestProviderService_CreateGitHubOAuthProviderWithInvalidConfig(t *testing.T) {
+	t.Parallel()
+
+	const (
+		installationID = 123
+		accountLogin   = "test-user"
+		accountID      = 456
+		stateNonce     = "test-githubapp-nonce"
+	)
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	pvtKeyFile := testCreatePrivateKeyFile(t)
+	defer os.Remove(pvtKeyFile.Name())
+	cfg := &server.ProviderConfig{
+		GitHubApp: &server.GitHubAppConfig{
+			PrivateKey: pvtKeyFile.Name(),
+		},
+	}
+
+	clientFactory := mockclients.NewMockGitHubClientFactory(ctrl)
+
+	provSvc, mocks := testNewGitHubProviderService(t, ctrl, cfg, nil, clientFactory)
+	dbproj, err := mocks.fakeStore.CreateProject(context.Background(),
+		db.CreateProjectParams{
+			Name:     "test",
+			Metadata: []byte(`{}`),
+		})
+	require.NoError(t, err)
+
+	mocks.svcMock.EXPECT().
+		GetInstallation(gomock.Any(), int64(installationID), gomock.Any()).
+		Return(&github.Installation{
+			Account: &github.User{
+				Login: github.String(accountLogin),
+				ID:    github.Int64(accountID),
+			},
+		}, nil, nil)
+
+	dbProv, err := provSvc.CreateGitHubAppProvider(
+		context.Background(), oauth2.Token{},
+		db.GetProjectIDBySessionStateRow{
+			ProjectID: dbproj.ID,
+			RemoteUser: sql.NullString{
+				Valid:  true,
+				String: strconv.Itoa(accountID),
+			},
+			ProviderConfig: []byte(`{ "auto_registration": { "entities": { "blah": {"enabled": true }}}, "github-app": {}}`),
+		},
+		installationID,
+		stateNonce)
+	require.Error(t, err)
+	require.ErrorAs(t, err, &providers.ErrProviderInvalidConfig{})
+	require.Nil(t, dbProv)
+}
+
 func TestProviderService_CreateGitHubAppProvider(t *testing.T) {
 	t.Parallel()