From f82d8419550e63dc351de212309dff095da858d3 Mon Sep 17 00:00:00 2001 From: Nazar <50199774+NazyS@users.noreply.github.com> Date: Wed, 12 Aug 2026 13:13:05 +0300 Subject: [PATCH] fix(github): repair GitHub App registration and installation auth (#2511) Co-authored-by: Carlos Florencio --- .../internal/auth/httpmw/middleware.go | 12 +++- .../internal/auth/httpmw/middleware_test.go | 16 +++++ .../github/app_installation_service.go | 58 ++++++++++++++----- .../github/app_installation_service_test.go | 20 ++++++- .../github/app_registration_import_test.go | 4 +- .../app_registration_lifecycle_service.go | 16 ++--- .../app_registration_test_helpers_test.go | 4 +- .../controller_app_registration_test.go | 56 +++++++++++++++++- .../internal/github/controller_auth.go | 12 +++- .../github/deployment_app_manifest.go | 35 ++++++----- .../github/deployment_app_manifest_test.go | 18 +++--- .../internal/github/service_app_auth_test.go | 2 +- .../github/github-app-import-form.test.tsx | 2 +- .../github/github-app-import-guide.test.tsx | 15 ++++- .../github/github-app-import-guide.tsx | 10 ++-- .../github-app-onboarding-model.test.ts | 5 ++ .../github/github-app-policy-dialog.tsx | 6 +- .../settings/github-app-registration.spec.ts | 16 ++++- .../mobile-github-app-registration.spec.ts | 7 ++- apps/web/src/locales/en/github.json | 2 +- ...ace-selectable-github-app-registrations.md | 6 +- docs/public/integrations.md | 14 +++-- .../integrations/github-authentication.md | 18 +++--- 23 files changed, 259 insertions(+), 95 deletions(-) diff --git a/apps/backend/internal/auth/httpmw/middleware.go b/apps/backend/internal/auth/httpmw/middleware.go index 619dcea37f..3a6a9a2ea9 100644 --- a/apps/backend/internal/auth/httpmw/middleware.go +++ b/apps/backend/internal/auth/httpmw/middleware.go @@ -2,8 +2,9 @@ // authentication. It runs after CORS on every request (see // backendapp.buildHTTPServer) and implements the allowlist policy from // docs/specs/auth: identity injection in disabled mode, credential resolution -// (session cookie, then PAT bearer), self-authenticating webhook passthrough, -// the office agent-JWT deferral, and SPA-shell availability for the login page. +// (session cookie, then PAT bearer), self-authenticating callback and webhook +// passthrough, the office agent-JWT deferral, and SPA-shell availability for +// the login page. // // CSRF note: cross-origin browser requests are already rejected by // backendapp.corsMiddleware (httpmw.AllowedOrigin) before this middleware @@ -113,6 +114,13 @@ func isPublicPath(method, path string) bool { case strings.HasPrefix(path, "/api/v1/office/channels/") && strings.HasSuffix(path, "/inbound"): // HMAC-SHA256 / provider token verified by the channel handler. return true + case strings.HasPrefix(path, "/api/v1/github/app/registrations/") && + (strings.HasSuffix(path, "/manifest/callback") || + strings.HasSuffix(path, "/install/callback") || + strings.HasSuffix(path, "/personal/callback")): + // GitHub redirects through a public hostname that cannot carry the + // Kandev session cookie. The handlers validate expiring, single-use state. + return method == http.MethodGet case strings.HasPrefix(path, "/api/v1/github/app/registrations/") && strings.HasSuffix(path, "/webhook"): // GitHub App webhook delivery; HMAC (X-Hub-Signature-256) verified by // the handler, not request identity. diff --git a/apps/backend/internal/auth/httpmw/middleware_test.go b/apps/backend/internal/auth/httpmw/middleware_test.go index 232530219e..9812f374d8 100644 --- a/apps/backend/internal/auth/httpmw/middleware_test.go +++ b/apps/backend/internal/auth/httpmw/middleware_test.go @@ -211,6 +211,22 @@ func TestEnabledModeAllowlistMatrix(t *testing.T) { name: "github app webhook", method: http.MethodPost, path: "/api/v1/github/app/registrations/reg1/webhook", }, + { + name: "github app manifest callback", method: http.MethodGet, + path: "/api/v1/github/app/registrations/reg1/manifest/callback", + }, + { + name: "github app installation callback", method: http.MethodGet, + path: "/api/v1/github/app/registrations/reg1/install/callback", + }, + { + name: "github app personal callback", method: http.MethodGet, + path: "/api/v1/github/app/registrations/reg1/personal/callback", + }, + { + name: "github app callback wrong method", method: http.MethodPost, + path: "/api/v1/github/app/registrations/reg1/manifest/callback", blocked: true, + }, {name: "office without bearer", method: http.MethodGet, path: "/api/v1/office/tasks/t1", blocked: true}, { diff --git a/apps/backend/internal/github/app_installation_service.go b/apps/backend/internal/github/app_installation_service.go index fefaea16a0..5350bb4e10 100644 --- a/apps/backend/internal/github/app_installation_service.go +++ b/apps/backend/internal/github/app_installation_service.go @@ -70,11 +70,12 @@ type AppInstallationResult struct { } type AppInstallationService struct { - config AppInstallationConfig - flows *OAuthFlowManager - store appInstallationStore - app appInstallationVerifier - oauth githubAppOAuth + config AppInstallationConfig + flows *OAuthFlowManager + store appInstallationStore + app appInstallationVerifier + oauth githubAppOAuth + installationAccessRetryDelays []time.Duration } func NewAppInstallationService( @@ -84,7 +85,12 @@ func NewAppInstallationService( app appInstallationVerifier, oauth githubAppOAuth, ) *AppInstallationService { - return &AppInstallationService{config: config, flows: flows, store: store, app: app, oauth: oauth} + return &AppInstallationService{ + config: config, flows: flows, store: store, app: app, oauth: oauth, + installationAccessRetryDelays: []time.Duration{ + 250 * time.Millisecond, 500 * time.Millisecond, time.Second, 2 * time.Second, + }, + } } func (s *AppInstallationService) Start( @@ -140,7 +146,7 @@ func (s *AppInstallationService) Complete( if err != nil { return AppInstallationResult{}, err } - installation, user, err := s.verifyInstallationCallback(ctx, callback, flow) + installation, user, err := s.verifyInstallationCallback(ctx, callback) if err != nil { return AppInstallationResult{}, err } @@ -158,14 +164,20 @@ func (s *AppInstallationService) Complete( func (s *AppInstallationService) verifyInstallationCallback( ctx context.Context, callback AppInstallationCallback, - flow *AuthFlow, ) (AppInstallation, GitHubOAuthUser, error) { - if callback.InstallationID <= 0 || callback.Code == "" || - (callback.SetupAction != "install" && callback.SetupAction != "update") { - return AppInstallation{}, GitHubOAuthUser{}, ErrInstallationAssociationUnverified + if callback.InstallationID <= 0 { + return AppInstallation{}, GitHubOAuthUser{}, fmt.Errorf( + "validate GitHub App installation ID: %w", ErrInstallationAssociationUnverified, + ) + } + if callback.Code == "" { + return AppInstallation{}, GitHubOAuthUser{}, fmt.Errorf( + "validate GitHub App authorizer code: %w", ErrInstallationAssociationUnverified, + ) } - tokens, err := s.oauth.ExchangeUserCode(ctx, callback.Code, flow.PKCEVerifier, s.config.CallbackURL) + // GitHub starts OAuth-on-install without a caller-supplied PKCE challenge. + tokens, err := s.oauth.ExchangeUserCode(ctx, callback.Code, "", s.config.CallbackURL) if err != nil || tokens.AccessToken == "" { return AppInstallation{}, GitHubOAuthUser{}, fmt.Errorf( "exchange GitHub App authorizer code: %w", associationError(err), @@ -177,7 +189,7 @@ func (s *AppInstallationService) verifyInstallationCallback( "verify GitHub App authorizing user: %w", associationError(err), ) } - accessible, err := s.oauth.UserCanAccessInstallation(ctx, tokens.AccessToken, callback.InstallationID) + accessible, err := s.userCanAccessInstallation(ctx, tokens.AccessToken, callback.InstallationID) if err != nil || !accessible { return AppInstallation{}, GitHubOAuthUser{}, fmt.Errorf( "verify GitHub App installation access: %w", associationError(err), @@ -192,6 +204,26 @@ func (s *AppInstallationService) verifyInstallationCallback( return installation, user, nil } +func (s *AppInstallationService) userCanAccessInstallation( + ctx context.Context, + accessToken string, + installationID int64, +) (bool, error) { + for attempt := 0; ; attempt++ { + accessible, err := s.oauth.UserCanAccessInstallation(ctx, accessToken, installationID) + if err != nil || accessible || attempt >= len(s.installationAccessRetryDelays) { + return accessible, err + } + timer := time.NewTimer(s.installationAccessRetryDelays[attempt]) + select { + case <-ctx.Done(): + timer.Stop() + return false, ctx.Err() + case <-timer.C: + } + } +} + func (s *AppInstallationService) newInstallationConnection( flow *AuthFlow, installation AppInstallation, diff --git a/apps/backend/internal/github/app_installation_service_test.go b/apps/backend/internal/github/app_installation_service_test.go index 2ac85523ae..7a5d0bed2c 100644 --- a/apps/backend/internal/github/app_installation_service_test.go +++ b/apps/backend/internal/github/app_installation_service_test.go @@ -6,6 +6,7 @@ import ( "net/url" "strings" "testing" + "time" ) type appInstallationMemoryStore struct { @@ -50,6 +51,8 @@ type fakeGitHubAppOAuth struct { user GitHubOAuthUser userErr error canAccess bool + accessResults []bool + accessCalls int exchangeCode string verifier string exchangeStarted chan struct{} @@ -82,6 +85,12 @@ func (f *fakeGitHubAppOAuth) GetOAuthUser(_ context.Context, _ string) (GitHubOA } func (f *fakeGitHubAppOAuth) UserCanAccessInstallation(_ context.Context, _ string, _ int64) (bool, error) { + if f.accessCalls < len(f.accessResults) { + result := f.accessResults[f.accessCalls] + f.accessCalls++ + return result, nil + } + f.accessCalls++ return f.canAccess, nil } @@ -110,7 +119,7 @@ func TestAppInstallationStartBuildsStateBoundInstallURL(t *testing.T) { } } -func TestAppInstallationCompleteAcceptsOAuthDuringInstallCallbackShape(t *testing.T) { +func TestAppInstallationCompleteAcceptsAutomaticOAuthInstallCallback(t *testing.T) { flowStore := &oauthFlowMemoryStore{} flows := NewOAuthFlowManager(flowStore) flows.random = strings.NewReader(strings.Repeat("e", oauthRandomBytes)) @@ -124,19 +133,20 @@ func TestAppInstallationCompleteAcceptsOAuthDuringInstallCallbackShape(t *testin }} oauth := &fakeGitHubAppOAuth{ tokens: GitHubOAuthTokens{AccessToken: "temporary-user-token"}, - user: GitHubOAuthUser{ID: 11, Login: "octocat"}, canAccess: true, + user: GitHubOAuthUser{ID: 11, Login: "octocat"}, accessResults: []bool{false, true}, } service := NewAppInstallationService( AppInstallationConfig{RegistrationID: "registration-test", Slug: "kandev-app", CallbackURL: "https://kandev.example/callback"}, flows, store, verifier, oauth, ) + service.installationAccessRetryDelays = []time.Duration{0} started, err := service.Start(context.Background(), "workspace-1", "user-1") if err != nil { t.Fatalf("Start() error = %v", err) } result, err := service.Complete(context.Background(), AppInstallationCallback{ - State: started.State, Code: "oauth-code", SetupAction: "install", InstallationID: 42, + State: started.State, Code: "oauth-code", InstallationID: 42, }) if err != nil { t.Fatalf("Complete() error = %v", err) @@ -144,6 +154,9 @@ func TestAppInstallationCompleteAcceptsOAuthDuringInstallCallbackShape(t *testin if result.AuthorizingUser.Login != "octocat" || result.Installation.AccountLogin != "acme" { t.Fatalf("Complete() = %+v", result) } + if oauth.exchangeCode != "oauth-code" || oauth.verifier != "" || oauth.accessCalls != 2 { + t.Fatalf("OAuth exchange code = %q, verifier = %q, access calls = %d", oauth.exchangeCode, oauth.verifier, oauth.accessCalls) + } if store.connection.Source != ConnectionSourceGitHubAppInstallation || store.connection.CredentialGeneration != 4 || store.connection.InstallationID == nil || *store.connection.InstallationID != 42 { @@ -163,6 +176,7 @@ func TestAppInstallationCompleteRejectsSpoofedAssociation(t *testing.T) { &fakeAppInstallationVerifier{installation: AppInstallation{ID: 42}}, &fakeGitHubAppOAuth{tokens: GitHubOAuthTokens{AccessToken: "token"}, user: GitHubOAuthUser{ID: 11}, canAccess: false}, ) + service.installationAccessRetryDelays = nil started, err := service.Start(context.Background(), "workspace-1", "user-1") if err != nil { t.Fatalf("Start() error = %v", err) diff --git a/apps/backend/internal/github/app_registration_import_test.go b/apps/backend/internal/github/app_registration_import_test.go index a48c742890..412552732a 100644 --- a/apps/backend/internal/github/app_registration_import_test.go +++ b/apps/backend/internal/github/app_registration_import_test.go @@ -173,7 +173,9 @@ func TestAppRegistrationImportAcceptsDocumentedAuthenticatedAppShapeAndPersists( } return appRegistrationVerifierFunc(func(context.Context) (AuthenticatedApp, error) { verified = true - return authenticatedAppForImport(t, request), nil + app := authenticatedAppForImport(t, request) + app.Events = []string{"push", "check_run"} + return app, nil }), nil }) diff --git a/apps/backend/internal/github/app_registration_lifecycle_service.go b/apps/backend/internal/github/app_registration_lifecycle_service.go index ae7b3ae3a7..057fef5793 100644 --- a/apps/backend/internal/github/app_registration_lifecycle_service.go +++ b/apps/backend/internal/github/app_registration_lifecycle_service.go @@ -116,7 +116,7 @@ type AppRegistrationImportPreparationResult struct { InstallCallbackURL string `json:"install_callback_url"` PersonalCallbackURL string `json:"personal_callback_url"` WebhookURL string `json:"webhook_url"` - SetupURL string `json:"setup_url"` + SetupURL string `json:"setup_url"` // legacy field; empty with OAuth-on-install Permissions map[string]string `json:"permissions"` Events []string `json:"events"` ExpiresAt time.Time `json:"expires_at"` @@ -253,14 +253,14 @@ func (s *AppRegistrationLifecycleService) StartManifest( if err := s.store.CreateDeploymentAppRegistrationFlow(ctx, flow); err != nil { return AppRegistrationManifestStart{}, fmt.Errorf("persist GitHub App manifest state: %w", err) } - callback, err := url.Parse(submission.Manifest.RedirectURL) + registrationURL, err := url.Parse(submission.RegistrationURL) if err != nil { - return AppRegistrationManifestStart{}, errors.New("build GitHub App manifest callback") + return AppRegistrationManifestStart{}, errors.New("build GitHub App registration URL") } - query := callback.Query() + query := registrationURL.Query() query.Set("state", state) - callback.RawQuery = query.Encode() - submission.Manifest.RedirectURL = callback.String() + registrationURL.RawQuery = query.Encode() + submission.RegistrationURL = registrationURL.String() return AppRegistrationManifestStart{ RegistrationID: registrationID, WorkspaceID: request.WorkspaceID, State: state, ExpiresAt: flow.ExpiresAt, Revision: submission.Revision, @@ -341,7 +341,7 @@ func (s *AppRegistrationLifecycleService) consumeManifestFlow( } return nil, fmt.Errorf("consume GitHub App manifest state: %w", err) } - if flow == nil || flow.RegistrationID != registrationID || flow.UserID != DefaultUserID || + if flow == nil || flow.RegistrationID != registrationID || strings.TrimSpace(flow.UserID) == "" || flow.ManifestRevision != DeploymentAppManifestRevision { return nil, ErrDeploymentAppManifestStateUnavailable } @@ -452,7 +452,7 @@ func (s *AppRegistrationLifecycleService) PrepareImport( ManifestCallbackURL: registrationBaseURL + "/manifest/callback", InstallCallbackURL: installCallbackURL, PersonalCallbackURL: registrationBaseURL + "/personal/callback", - WebhookURL: registrationBaseURL + "/webhook", SetupURL: installCallbackURL, + WebhookURL: registrationBaseURL + "/webhook", SetupURL: "", Permissions: policy.Manifest.DefaultPermissions, Events: policy.Manifest.DefaultEvents, ExpiresAt: preparation.ExpiresAt, }, nil diff --git a/apps/backend/internal/github/app_registration_test_helpers_test.go b/apps/backend/internal/github/app_registration_test_helpers_test.go index 1f82f3ff79..f9af67190c 100644 --- a/apps/backend/internal/github/app_registration_test_helpers_test.go +++ b/apps/backend/internal/github/app_registration_test_helpers_test.go @@ -25,8 +25,6 @@ func testManifestConversion(t *testing.T, ownerLogin, ownerType string) Manifest "metadata": "read", "pull_requests": "write", "statuses": "read", "workflows": "write", }, - Events: []string{ - "installation", "installation_repositories", "github_app_authorization", "push", "check_run", - }, + Events: []string{"push", "check_run"}, } } diff --git a/apps/backend/internal/github/controller_app_registration_test.go b/apps/backend/internal/github/controller_app_registration_test.go index 88f0d18d86..d52c33306f 100644 --- a/apps/backend/internal/github/controller_app_registration_test.go +++ b/apps/backend/internal/github/controller_app_registration_test.go @@ -7,6 +7,8 @@ import ( "net/http" "net/http/httptest" "net/netip" + "net/url" + "reflect" "strings" "sync" "testing" @@ -132,6 +134,20 @@ func TestHTTPAppRegistrationManifestIsRouteBoundAndReturnsToWorkspace(t *testing if err := json.Unmarshal(start.Body.Bytes(), &started); err != nil || started.RegistrationID == "" { t.Fatalf("start response = %s, error = %v", start.Body.String(), err) } + registrationURL, err := url.Parse(started.RegistrationURL) + if err != nil { + t.Fatalf("registration URL = %q: %v", started.RegistrationURL, err) + } + if registrationURL.Query().Get("state") != started.State { + t.Fatalf("registration URL state = %q, want %q", registrationURL.Query().Get("state"), started.State) + } + redirectURL, err := url.Parse(started.Manifest.RedirectURL) + if err != nil { + t.Fatalf("manifest redirect URL = %q: %v", started.Manifest.RedirectURL, err) + } + if redirectURL.RawQuery != "" { + t.Fatalf("manifest redirect URL query = %q, want empty", redirectURL.RawQuery) + } wrong := httptest.NewRecorder() router.ServeHTTP(wrong, httptest.NewRequest(http.MethodGet, @@ -168,6 +184,41 @@ func TestHTTPAppRegistrationManifestIsRouteBoundAndReturnsToWorkspace(t *testing } } +func TestHTTPAppRegistrationManifestCallbackAllowsNonDefaultAdmin(t *testing.T) { + router, _, store := setupAppRegistrationController(t) + seedConnectionWorkspaces(t, store, "workspace-1") + + startRequest := httptest.NewRequest(http.MethodPost, + "/api/v1/github/app/registrations/manifest/start", strings.NewReader(`{ + "workspace_id":"workspace-1","display_name":"Work automation", + "owner_type":"organization","owner_login":"acme","visibility":"private", + "public_base_url":"https://kandev.example" + }`)) + startRequest.Header.Set("Content-Type", "application/json") + startRequest = startRequest.WithContext(authn.WithIdentity(startRequest.Context(), authn.Identity{ + UserID: "admin-1", Role: authn.RoleAdmin, + })) + start := httptest.NewRecorder() + router.ServeHTTP(start, startRequest) + if start.Code != http.StatusOK { + t.Fatalf("start = %d %s", start.Code, start.Body.String()) + } + var started AppRegistrationManifestStart + if err := json.Unmarshal(start.Body.Bytes(), &started); err != nil || started.RegistrationID == "" { + t.Fatalf("start response = %s, error = %v", start.Body.String(), err) + } + + callback := httptest.NewRecorder() + router.ServeHTTP(callback, httptest.NewRequest(http.MethodGet, + "/api/v1/github/app/registrations/"+started.RegistrationID+ + "/manifest/callback?state="+started.State+"&code=one-time-code", nil)) + assertPrivateGitHubRedirect(t, callback) + if location := callback.Header().Get("Location"); !strings.Contains(location, "workspace_id=workspace-1") || + !strings.Contains(location, "github_result=app_registered") { + t.Fatalf("callback location = %q", location) + } +} + func TestHTTPAppRegistrationCatalogRequiresAdminIdentity(t *testing.T) { router, _, store := setupAppRegistrationController(t) seedConnectionWorkspaces(t, store, "workspace-1") @@ -495,8 +546,9 @@ func TestHTTPAppRegistrationImportPreparationIsSingleUse(t *testing.T) { preparation.ManifestCallbackURL != base+"/manifest/callback" || preparation.InstallCallbackURL != base+"/install/callback" || preparation.PersonalCallbackURL != base+"/personal/callback" || - preparation.WebhookURL != base+"/webhook" || preparation.SetupURL != preparation.InstallCallbackURL || - preparation.Permissions["contents"] != "write" || !containsString(preparation.Events, "installation") { + preparation.WebhookURL != base+"/webhook" || preparation.SetupURL != "" || + preparation.Permissions["contents"] != "write" || + !reflect.DeepEqual(preparation.Events, []string{"push", "check_run"}) { t.Fatalf("preparation = %+v", preparation) } _, privateKey := testAppPrivateKey(t) diff --git a/apps/backend/internal/github/controller_auth.go b/apps/backend/internal/github/controller_auth.go index b0cc52a7a4..f97f126b82 100644 --- a/apps/backend/internal/github/controller_auth.go +++ b/apps/backend/internal/github/controller_auth.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/gin-gonic/gin" + "go.uber.org/zap" "github.com/kandev/kandev/internal/auth/authn" ) @@ -140,7 +141,16 @@ func (c *Controller) httpCompleteAppInstallation(ctx *gin.Context) { InstallationID: installationID, }) if err != nil { - redirectGitHubCallback(ctx, workspaceID, githubAuthErrorCode(err)) + resultCode := githubAuthErrorCode(err) + if c.logger != nil { + c.logger.Warn("GitHub App installation callback verification failed", + zap.String("registration_id", ctx.Param("registrationId")), + zap.String("workspace_id", workspaceID), + zap.String("result_code", resultCode), + zap.Error(err), + ) + } + redirectGitHubCallback(ctx, workspaceID, resultCode) return } redirectGitHubCallback(ctx, result.WorkspaceID, "app_connected") diff --git a/apps/backend/internal/github/deployment_app_manifest.go b/apps/backend/internal/github/deployment_app_manifest.go index 940f21788f..a9a2cfd515 100644 --- a/apps/backend/internal/github/deployment_app_manifest.go +++ b/apps/backend/internal/github/deployment_app_manifest.go @@ -17,7 +17,7 @@ import ( ) const ( - DeploymentAppManifestRevision = 1 + DeploymentAppManifestRevision = 2 deploymentAppManifestFlowTTL = time.Hour deploymentAppNameMaxLength = 34 ) @@ -102,7 +102,7 @@ type DeploymentAppManifest struct { HookAttributes DeploymentAppManifestHook `json:"hook_attributes"` RedirectURL string `json:"redirect_url"` CallbackURLs []string `json:"callback_urls"` - SetupURL string `json:"setup_url"` + SetupURL string `json:"setup_url,omitempty"` Public bool `json:"public"` DefaultPermissions map[string]string `json:"default_permissions"` DefaultEvents []string `json:"default_events"` @@ -143,10 +143,14 @@ func BuildAppRegistrationManifest( } baseURL := strings.TrimRight(submission.Manifest.URL, "/") + "/api/v1/github/app/registrations/" + registrationID.String() + installCallbackURL := baseURL + "/install/callback" submission.Manifest.HookAttributes.URL = baseURL + "/webhook" submission.Manifest.RedirectURL = baseURL + "/manifest/callback" - submission.Manifest.CallbackURLs = []string{baseURL + "/personal/callback"} - submission.Manifest.SetupURL = baseURL + "/install/callback" + submission.Manifest.CallbackURLs = []string{ + installCallbackURL, + baseURL + "/personal/callback", + } + submission.Manifest.SetupURL = "" submission.Manifest.Public = public return submission, nil } @@ -218,26 +222,21 @@ func BuildDeploymentAppManifest( HookAttributes: DeploymentAppManifestHook{ URL: baseURL + "/api/v1/github/app/webhook", Active: true, }, - RedirectURL: baseURL + "/api/v1/github/app/registration/callback", - CallbackURLs: []string{baseURL + "/api/v1/github/personal-connection/callback"}, - SetupURL: baseURL + "/api/v1/github/app/install/callback", - Public: false, + RedirectURL: baseURL + "/api/v1/github/app/registration/callback", + CallbackURLs: []string{ + baseURL + "/api/v1/github/app/install/callback", + baseURL + "/api/v1/github/personal-connection/callback", + }, + Public: false, DefaultPermissions: map[string]string{ "actions": "read", "administration": "read", "checks": "read", "contents": "write", "issues": "write", "members": "read", "metadata": "read", "pull_requests": "write", "statuses": "read", "workflows": "write", }, - // push and check_run enable webhook-driven github_push/github_ci automation - // triggers. Deployment caveat: GitHub Apps only deliver events a user has - // explicitly subscribed to at installation time, so existing installations - // created before this manifest change must be re-accepted (reinstalled, or - // updated via the App's GitHub settings page) before push/check_run - // webhooks start arriving for them. - DefaultEvents: []string{ - "installation", "installation_repositories", "github_app_authorization", - "push", "check_run", - }, + // GitHub delivers App lifecycle events automatically. Only configurable + // webhook subscriptions belong in default_events. + DefaultEvents: []string{"push", "check_run"}, RequestOAuthOnInstall: true, SetupOnUpdate: false, }, diff --git a/apps/backend/internal/github/deployment_app_manifest_test.go b/apps/backend/internal/github/deployment_app_manifest_test.go index b8882d47d0..bd9e1bd710 100644 --- a/apps/backend/internal/github/deployment_app_manifest_test.go +++ b/apps/backend/internal/github/deployment_app_manifest_test.go @@ -38,8 +38,8 @@ func TestDeploymentAppManifestExactPolicyAndURLs(t *testing.T) { if err != nil { t.Fatalf("BuildDeploymentAppManifest() error = %v", err) } - if submission.Revision != DeploymentAppManifestRevision { - t.Fatalf("Revision = %d, want %d", submission.Revision, DeploymentAppManifestRevision) + if submission.Revision != 2 { + t.Fatalf("Revision = %d, want 2", submission.Revision) } if submission.RegistrationURL != "https://github.com/settings/apps/new" { t.Fatalf("RegistrationURL = %q", submission.RegistrationURL) @@ -51,9 +51,10 @@ func TestDeploymentAppManifestExactPolicyAndURLs(t *testing.T) { if manifest.URL != "https://kandev.example" || manifest.RedirectURL != "https://kandev.example/api/v1/github/app/registration/callback" || !reflect.DeepEqual(manifest.CallbackURLs, []string{ + "https://kandev.example/api/v1/github/app/install/callback", "https://kandev.example/api/v1/github/personal-connection/callback", }) || - manifest.SetupURL != "https://kandev.example/api/v1/github/app/install/callback" || + manifest.SetupURL != "" || manifest.HookAttributes.URL != "https://kandev.example/api/v1/github/app/webhook" || !manifest.HookAttributes.Active { t.Fatalf("manifest URLs = %+v", manifest) @@ -77,9 +78,7 @@ func TestDeploymentAppManifestExactPolicyAndURLs(t *testing.T) { if !reflect.DeepEqual(manifest.DefaultPermissions, wantPermissions) { t.Fatalf("DefaultPermissions = %#v, want %#v", manifest.DefaultPermissions, wantPermissions) } - wantEvents := []string{ - "installation", "installation_repositories", "github_app_authorization", "push", "check_run", - } + wantEvents := []string{"push", "check_run"} if !reflect.DeepEqual(manifest.DefaultEvents, wantEvents) { t.Fatalf("DefaultEvents = %#v, want %#v", manifest.DefaultEvents, wantEvents) } @@ -98,9 +97,12 @@ func TestDeploymentAppManifestUsesPreallocatedRegistrationRoutes(t *testing.T) { wantPrefix := "https://kandev.example/api/v1/github/app/registrations/" + registrationID manifest := submission.Manifest if manifest.RedirectURL != wantPrefix+"/manifest/callback" || - manifest.SetupURL != wantPrefix+"/install/callback" || + manifest.SetupURL != "" || manifest.HookAttributes.URL != wantPrefix+"/webhook" || - !reflect.DeepEqual(manifest.CallbackURLs, []string{wantPrefix + "/personal/callback"}) { + !reflect.DeepEqual(manifest.CallbackURLs, []string{ + wantPrefix + "/install/callback", + wantPrefix + "/personal/callback", + }) { t.Fatalf("registration-specific manifest URLs = %+v", manifest) } if manifest.Public { diff --git a/apps/backend/internal/github/service_app_auth_test.go b/apps/backend/internal/github/service_app_auth_test.go index 4621eabd90..9ae0e2a0a7 100644 --- a/apps/backend/internal/github/service_app_auth_test.go +++ b/apps/backend/internal/github/service_app_auth_test.go @@ -231,7 +231,7 @@ func TestServicePreparesImportAndRejectsWrongBinding(t *testing.T) { t.Fatal(err) } if prepared.RegistrationID == "" || prepared.PublicBaseURL != "https://kandev.example" || - prepared.SetupURL != prepared.InstallCallbackURL { + prepared.SetupURL != "" { t.Fatalf("prepared import = %+v", prepared) } _, err = service.ImportAppRegistration(context.Background(), DefaultUserID, AppRegistrationImportRequest{ diff --git a/apps/web/components/github/github-app-import-form.test.tsx b/apps/web/components/github/github-app-import-form.test.tsx index 1b5663fd49..23920b9178 100644 --- a/apps/web/components/github/github-app-import-form.test.tsx +++ b/apps/web/components/github/github-app-import-form.test.tsx @@ -13,7 +13,7 @@ const preparation: PrepareGitHubAppImportResponse = { install_callback_url: "https://kandev.example/install/callback", personal_callback_url: "https://kandev.example/personal/callback", webhook_url: "https://kandev.example/webhook", - setup_url: "https://kandev.example/install/callback", + setup_url: "", permissions: { contents: "write" }, events: ["installation"], expires_at: "2026-07-22T00:00:00Z", diff --git a/apps/web/components/github/github-app-import-guide.test.tsx b/apps/web/components/github/github-app-import-guide.test.tsx index ecd9fa98d1..2307df3f96 100644 --- a/apps/web/components/github/github-app-import-guide.test.tsx +++ b/apps/web/components/github/github-app-import-guide.test.tsx @@ -12,7 +12,7 @@ const preparation: PrepareGitHubAppImportResponse = { install_callback_url: "https://kandev.example/install/callback", personal_callback_url: "https://kandev.example/personal/callback", webhook_url: "https://kandev.example/webhook", - setup_url: "https://kandev.example/install/callback", + setup_url: "", permissions: { contents: "write" }, events: ["installation"], expires_at: "2026-07-22T00:00:00Z", @@ -27,8 +27,17 @@ describe("GitHubAppImportGuide", () => { />, ); expect(screen.getByText("Homepage URL")).toBeTruthy(); - expect(screen.getByText("User authorization callback URL")).toBeTruthy(); - expect(screen.getByText("Setup URL")).toBeTruthy(); + expect(screen.getByText("User authorization callback URL 1")).toBeTruthy(); + expect(screen.getByText("User authorization callback URL 2")).toBeTruthy(); + expect( + screen + .getAllByText(/^https:\/\/kandev\.example\/(?:install|personal)\/callback$/) + .map((element) => element.textContent), + ).toEqual([ + "https://kandev.example/install/callback", + "https://kandev.example/personal/callback", + ]); + expect(screen.queryByText("Setup URL")).toBeNull(); expect(screen.getByText("Webhook URL")).toBeTruthy(); expect(screen.getByText(/application\/json/)).toBeTruthy(); expect(screen.getByText(/SSL verification enabled/)).toBeTruthy(); diff --git a/apps/web/components/github/github-app-import-guide.tsx b/apps/web/components/github/github-app-import-guide.tsx index 3c3a312478..a46fc3ae64 100644 --- a/apps/web/components/github/github-app-import-guide.tsx +++ b/apps/web/components/github/github-app-import-guide.tsx @@ -10,13 +10,13 @@ import { Trans, useTranslation } from "react-i18next"; import { formatDateTime } from "@/lib/i18n/formats"; // These are the verbatim field names on GitHub's own "Register a new GitHub App" -// form, which is English-only. The guide tells the user which field to paste each -// value into, so translating them would break that mapping — they are references -// to another product's UI, not our copy. +// form, which is English-only. The numeric suffixes preserve the order GitHub +// uses for OAuth during installation; they are references to another product's +// UI, not our copy. const urlLabels: [keyof PrepareGitHubAppImportResponse, string][] = [ ["public_base_url", "Homepage URL"], - ["personal_callback_url", "User authorization callback URL"], - ["setup_url", "Setup URL"], + ["install_callback_url", "User authorization callback URL 1"], + ["personal_callback_url", "User authorization callback URL 2"], ["webhook_url", "Webhook URL"], ]; diff --git a/apps/web/components/github/github-app-onboarding-model.test.ts b/apps/web/components/github/github-app-onboarding-model.test.ts index 9120542877..1d3698383e 100644 --- a/apps/web/components/github/github-app-onboarding-model.test.ts +++ b/apps/web/components/github/github-app-onboarding-model.test.ts @@ -8,6 +8,7 @@ import { validateImportSecrets, validateRegistrationBasics, } from "./github-app-onboarding-model"; +import { githubAppEvents } from "./github-app-policy-dialog"; describe("GitHub App onboarding model", () => { it("requires a public HTTPS origin", () => { @@ -64,4 +65,8 @@ describe("GitHub App onboarding model", () => { submit.mockRestore(); form?.remove(); }); + + it("lists only configurable GitHub App webhook subscriptions", () => { + expect(githubAppEvents).toEqual(["push", "check_run"]); + }); }); diff --git a/apps/web/components/github/github-app-policy-dialog.tsx b/apps/web/components/github/github-app-policy-dialog.tsx index 7e997eafc6..02429453c6 100644 --- a/apps/web/components/github/github-app-policy-dialog.tsx +++ b/apps/web/components/github/github-app-policy-dialog.tsx @@ -25,11 +25,7 @@ export const githubAppPermissions: Record = { workflows: "write", }; -export const githubAppEvents = [ - "installation", - "installation_repositories", - "github_app_authorization", -]; +export const githubAppEvents = ["push", "check_run"]; function title(value: string) { return value.replaceAll("_", " ").replace(/\b\w/g, (letter) => letter.toUpperCase()); diff --git a/apps/web/e2e/tests/settings/github-app-registration.spec.ts b/apps/web/e2e/tests/settings/github-app-registration.spec.ts index 9c29a9e3cf..1ccc8086d0 100644 --- a/apps/web/e2e/tests/settings/github-app-registration.spec.ts +++ b/apps/web/e2e/tests/settings/github-app-registration.spec.ts @@ -108,12 +108,17 @@ test.describe("Workspace GitHub App onboarding", () => { const policy = testPage.getByRole("dialog", { name: "Required GitHub App policy" }); await expect(policy.getByText("Contents", { exact: true })).toBeVisible(); await expect(policy.getByText("Workflows", { exact: true })).toBeVisible(); + await expect(policy.getByText("Push", { exact: true })).toBeVisible(); + await expect(policy.getByText("Check Run", { exact: true })).toBeVisible(); + await expect(policy.getByText("Installation", { exact: true })).toHaveCount(0); await policy.getByRole("button", { name: "Close" }).click(); let manifest: Record | undefined; + let registrationState: string | null = null; await testPage.route( - "https://github.com/organizations/acme/settings/apps/new", + "https://github.com/organizations/acme/settings/apps/new?**", async (route) => { + registrationState = new URL(route.request().url()).searchParams.get("state"); const fields = new URLSearchParams(route.request().postData() ?? ""); manifest = JSON.parse(fields.get("manifest") ?? "{}") as Record; await route.fulfill({ @@ -130,8 +135,10 @@ test.describe("Workspace GitHub App onboarding", () => { await expect( testPage.getByRole("heading", { name: "Confirm Work automation on GitHub" }), ).toBeVisible(); + expect(registrationState).toBeTruthy(); const redirectUrl = String(manifest?.redirect_url ?? ""); + expect(new URL(redirectUrl).search).toBe(""); const registrationId = redirectUrl.match( /\/app\/registrations\/([^/]+)\/manifest\/callback/, )?.[1]; @@ -139,15 +146,17 @@ test.describe("Workspace GitHub App onboarding", () => { expect(manifest).toMatchObject({ url: "https://1.1.1.1", public: false, + default_events: ["push", "check_run"], hook_attributes: { url: `https://1.1.1.1/api/v1/github/app/registrations/${registrationId}/webhook`, active: true, }, callback_urls: [ + `https://1.1.1.1/api/v1/github/app/registrations/${registrationId}/install/callback`, `https://1.1.1.1/api/v1/github/app/registrations/${registrationId}/personal/callback`, ], - setup_url: `https://1.1.1.1/api/v1/github/app/registrations/${registrationId}/install/callback`, }); + expect(manifest).not.toHaveProperty("setup_url"); await apiClient.mockGitHubSetAppRegistration({ id: registrationId!, @@ -194,6 +203,9 @@ test.describe("Workspace GitHub App onboarding", () => { await expect( connection.getByText(preparation.install_callback_url, { exact: true }), ).toBeVisible(); + await expect(connection.getByText("User authorization callback URL 1")).toBeVisible(); + await expect(connection.getByText("User authorization callback URL 2")).toBeVisible(); + await expect(connection.getByText("Setup URL")).toHaveCount(0); await expect(connection.getByText(preparation.webhook_url, { exact: true })).toBeVisible(); await expect(connection.getByText(/application\/json/)).toBeVisible(); await expect(connection.getByText(/SSL verification enabled/)).toBeVisible(); diff --git a/apps/web/e2e/tests/settings/mobile-github-app-registration.spec.ts b/apps/web/e2e/tests/settings/mobile-github-app-registration.spec.ts index 06e6878c93..c3e9fc592d 100644 --- a/apps/web/e2e/tests/settings/mobile-github-app-registration.spec.ts +++ b/apps/web/e2e/tests/settings/mobile-github-app-registration.spec.ts @@ -86,7 +86,12 @@ test.describe("Mobile workspace GitHub App onboarding", () => { await drawer.getByLabel("Public Kandev URL").fill("https://1.1.1.1"); await drawer.getByRole("button", { name: "Generate setup instructions" }).click(); await expect(drawer.getByText("Configure the existing App on GitHub")).toBeVisible(); - await expect(drawer.getByText("User authorization callback URL")).toBeVisible(); + await expect( + drawer.getByText("User authorization callback URL 1", { exact: true }), + ).toBeVisible(); + await expect( + drawer.getByText("User authorization callback URL 2", { exact: true }), + ).toBeVisible(); await expect(drawer.getByText("Webhook URL", { exact: true })).toBeVisible(); await expectTouchTarget(drawer.getByRole("button", { name: "Copy Webhook URL" })); await expectTouchTarget(drawer.getByRole("button", { name: "Verify and import App" })); diff --git a/apps/web/src/locales/en/github.json b/apps/web/src/locales/en/github.json index 238f5eb32c..f1a12234ef 100644 --- a/apps/web/src/locales/en/github.json +++ b/apps/web/src/locales/en/github.json @@ -489,7 +489,7 @@ "selectWorkspace": "Select workspace", "selectedRepositories": "Selected repositories", "selectWorkspaceBeforeRequestingReview": "Select a workspace before requesting review.", - "setTheseExactUrlsCreateAClientSecret": "Set these exact URLs, create a client secret and webhook secret, and download a private key. This one-time setup expires {{expiresAt}}.", + "setTheseExactUrlsCreateAClientSecret": "Add the two user authorization callback URLs in the order shown, set the remaining exact values, create a client secret and webhook secret, and download a private key. This one-time setup expires {{expiresAt}}.", "showGithubApiLimits": "Show GitHub API limits", "showLess": "Show less", "showMore": "Show more", diff --git a/docs/decisions/2026-07-21-workspace-selectable-github-app-registrations.md b/docs/decisions/2026-07-21-workspace-selectable-github-app-registrations.md index 65681e2e26..dd9a3f4520 100644 --- a/docs/decisions/2026-07-21-workspace-selectable-github-app-registrations.md +++ b/docs/decisions/2026-07-21-workspace-selectable-github-app-registrations.md @@ -30,9 +30,9 @@ carry registration identity as well as workspace identity. Registration management is entered from workspace GitHub settings. A user can: 1. select a known registration and install it for the current workspace; -2. import an existing App by applying Kandev's documented callback, setup URL, webhook, permission, - and event policy, then submitting its App ID, OAuth client material, private key, and webhook - secret for verification and encrypted storage; or +2. import an existing App by applying Kandev's documented callback, OAuth-on-install, webhook, + permission, and event policy, then submitting its App ID, OAuth client material, private key, + and webhook secret for verification and encrypted storage; or 3. create a registration through a workspace-bound App Manifest flow. Catalog scope permits deliberate reuse without duplicating root secrets. Reuse shares the App diff --git a/docs/public/integrations.md b/docs/public/integrations.md index f8e300f8e5..64dad7a3cb 100644 --- a/docs/public/integrations.md +++ b/docs/public/integrations.md @@ -199,15 +199,17 @@ copyable settings for that pending import. It is short-lived and single-use. 1. Start **Add existing App** from the workspace GitHub connection flow. Enter its owner, label, visibility, and the public Kandev origin. 2. Open the App under the owning user's or organization's **Settings > Developer settings > GitHub - Apps**, then apply every URL, permission, and event shown by Kandev. Set the App homepage to the - public Kandev origin, use JSON webhook delivery with SSL verification, request user - authorization during installation, and keep expiring user tokens enabled. + Apps**, then apply every URL, permission, and event shown by Kandev. Add the workspace + installation callback as the first user authorization callback and the personal identity + callback as the second. Set the App homepage to the public Kandev origin, use JSON webhook + delivery with SSL verification, request user authorization during installation, leave the setup + URL empty, and keep expiring user tokens enabled. 3. Return before the preparation expires and provide the App ID, OAuth client ID and secret, App slug, webhook secret, and an RSA private key generated for that App. Treat every secret field as a root credential and never put it in workspace environment variables or executor profiles. 4. Kandev authenticates as the App, verifies its ID, owner, slug, homepage, permissions, events, and webhook settings where GitHub exposes them, then encrypts the credential bundle. You must - confirm callback/setup settings that GitHub's API does not expose. + confirm callback settings that GitHub's API does not expose. 5. Select and install the imported registration. Importing alone never replaces the workspace's current PAT, CLI account, or App installation. @@ -220,7 +222,7 @@ templates are useful when checking a proxy or GitHub setting: | Purpose | URL path | |---|---| | Manifest creation callback | `/api/v1/github/app/registrations/{registrationId}/manifest/callback` | -| Workspace installation setup | `/api/v1/github/app/registrations/{registrationId}/install/callback` | +| Workspace installation OAuth callback | `/api/v1/github/app/registrations/{registrationId}/install/callback` | | Personal identity OAuth callback | `/api/v1/github/app/registrations/{registrationId}/personal/callback` | | Signed webhook delivery | `/api/v1/github/app/registrations/{registrationId}/webhook` | @@ -244,7 +246,7 @@ For full Kandev behavior, request the smallest applicable repository/organizatio | Members | Read | Organization/team membership lookups. | | Workflows | Write | Changes under `.github/workflows`; omit when agents must not edit workflow files. | -Subscribe only to `installation`, `installation_repositories`, and `github_app_authorization`. Kandev uses these events to track installation suspension/deletion, repository access changes, and revoked personal authorizations. PR, issue, review, and CI watches continue to poll and do not require their corresponding webhooks. GitHub's [registration guide](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app), [App permission reference](https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps), and [webhook guide](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/using-webhooks-with-github-apps) describe the provider-side settings. +Subscribe to `push` and `check_run`. GitHub sends `installation`, `installation_repositories`, and `github_app_authorization` lifecycle events automatically; they do not appear as selectable subscriptions. Kandev uses the lifecycle events to track installation suspension/deletion, repository access changes, and revoked personal authorizations. PR, issue, and review watches continue to poll and do not require their corresponding webhooks. GitHub's [registration guide](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app), [App permission reference](https://docs.github.com/en/rest/authentication/permissions-required-for-github-apps), and [webhook guide](https://docs.github.com/en/apps/creating-github-apps/registering-a-github-app/using-webhooks-with-github-apps) describe the provider-side settings. To delete a registration, first disconnect every workspace using it and remove personal identities issued through it. Kandev blocks deletion while any workspace or personal connection is bound, diff --git a/docs/specs/integrations/github-authentication.md b/docs/specs/integrations/github-authentication.md index 3a4ca6ead7..6b14486439 100644 --- a/docs/specs/integrations/github-authentication.md +++ b/docs/specs/integrations/github-authentication.md @@ -168,13 +168,14 @@ as the workspace installation. Kandev-created Apps request repository metadata read; contents read/write; pull requests read/write; issues read/write; checks, statuses, and Actions read; administration read; organization members read; and workflows write. The UI exposes this policy through a permissions button and dialog, not -a row of chips. The App subscribes to `installation`, `installation_repositories`, and -`github_app_authorization`. +a row of chips. The App subscribes to the configurable `push` and `check_run` events. GitHub sends +`installation`, `installation_repositories`, and `github_app_authorization` lifecycle events +automatically, so they are handled but are not part of the requested event policy. -An imported App must meet the same callback, setup URL, webhook, event, and permission requirements. -Kandev validates the App identity and reports missing capabilities after installation. It does not -silently change an imported App's GitHub settings. The guide provides exact values and GitHub links -for the user to apply. +An imported App must meet the same callback, OAuth-on-install, webhook, event, and permission +requirements. Kandev validates the App identity and reports missing capabilities after installation. +It does not silently change an imported App's GitHub settings. The guide provides exact values and +GitHub links for the user to apply. ## Data Model @@ -300,8 +301,9 @@ before exposing registration management. - `POST /api/v1/github/app/registrations/import/prepare` creates a short-lived, single-use import preparation for the initiating workspace. It returns `registration_id`, `public_base_url`, `manifest_callback_url`, `install_callback_url`, `personal_callback_url`, `webhook_url`, - `setup_url`, `permissions`, `events`, and `expires_at` so the user can configure the existing App - before submitting any root credentials. + `permissions`, `events`, and `expires_at` so the user can configure the existing App before + submitting any root credentials. The legacy `setup_url` response field is retained for client + compatibility and is always empty because OAuth-on-install disables GitHub's Setup URL. - `POST /api/v1/github/app/registrations/import` consumes the prepared `registration_id` and accepts the workspace context, label, App ID, client ID/secret, slug, private key, webhook secret, owner, and visibility. It verifies the App via GitHub before atomically persisting it. Duplicate