From 18e670485c3eca7072f0e5b3e248285014c677a7 Mon Sep 17 00:00:00 2001 From: Petri-Johan Last Date: Thu, 1 Feb 2024 10:32:09 +0200 Subject: [PATCH] Move GItHub App creation routes to HTTP API (#59978) --- .../gitHubApps/CreateGitHubAppPage.tsx | 6 +- .../components/gitHubApps/GitHubAppPage.tsx | 2 +- cmd/frontend/internal/app/app.go | 6 +- cmd/frontend/internal/app/router/router.go | 6 - cmd/frontend/internal/app/ui/BUILD.bazel | 1 + cmd/frontend/internal/app/ui/router.go | 4 + cmd/frontend/internal/auth/BUILD.bazel | 1 - .../internal/auth/githubappauth/middleware.go | 539 ------------------ cmd/frontend/internal/auth/init.go | 2 - .../internal/batches/resolvers/BUILD.bazel | 4 +- .../internal/batches/resolvers/code_host.go | 2 +- .../internal/batches/resolvers/main_test.go | 2 +- cmd/frontend/internal/cli/http.go | 5 +- cmd/frontend/internal/cli/serve_cmd.go | 1 - .../githubappauth => githubapp}/BUILD.bazel | 16 +- cmd/frontend/internal/githubapp/httpapi.go | 517 +++++++++++++++++ .../httpapi_test.go} | 12 +- .../{auth/githubappauth => githubapp}/init.go | 4 +- .../githubappauth => githubapp}/resolver.go | 0 .../resolver_test.go | 0 cmd/frontend/internal/httpapi/httpapi.go | 57 +- cmd/frontend/shared/BUILD.bazel | 2 +- cmd/frontend/shared/shared.go | 2 +- 23 files changed, 580 insertions(+), 611 deletions(-) delete mode 100644 cmd/frontend/internal/auth/githubappauth/middleware.go rename cmd/frontend/internal/{auth/githubappauth => githubapp}/BUILD.bazel (87%) create mode 100644 cmd/frontend/internal/githubapp/httpapi.go rename cmd/frontend/internal/{auth/githubappauth/middleware_test.go => githubapp/httpapi_test.go} (98%) rename cmd/frontend/internal/{auth/githubappauth => githubapp}/init.go (79%) rename cmd/frontend/internal/{auth/githubappauth => githubapp}/resolver.go (100%) rename cmd/frontend/internal/{auth/githubappauth => githubapp}/resolver_test.go (100%) diff --git a/client/web/src/components/gitHubApps/CreateGitHubAppPage.tsx b/client/web/src/components/gitHubApps/CreateGitHubAppPage.tsx index 7029efb38ee..c2b8053fb5c 100644 --- a/client/web/src/components/gitHubApps/CreateGitHubAppPage.tsx +++ b/client/web/src/components/gitHubApps/CreateGitHubAppPage.tsx @@ -97,8 +97,8 @@ export const CreateGitHubAppPage: FC = ({ name: name.trim(), url: originURL, hook_attributes: webhookURL ? { url: webhookURL } : undefined, - redirect_url: new URL('/.auth/githubapp/redirect', originURL).href, - setup_url: new URL('/.auth/githubapp/setup', originURL).href, + redirect_url: new URL('/githubapp/redirect', originURL).href, + setup_url: new URL('/githubapp/setup', originURL).href, callback_urls: [new URL('/.auth/github/callback', originURL).href], setup_on_update: true, public: isPublic, @@ -139,7 +139,7 @@ export const CreateGitHubAppPage: FC = ({ setError(undefined) try { const response = await fetch( - `/.auth/githubapp/new-app-state?appName=${name}&webhookURN=${url}&domain=${appDomain}&baseURL=${url}` + `/githubapp/new-app-state?appName=${name}&webhookURN=${url}&domain=${appDomain}&baseURL=${url}` ) if (!response.ok) { if (response.body instanceof ReadableStream) { diff --git a/client/web/src/components/gitHubApps/GitHubAppPage.tsx b/client/web/src/components/gitHubApps/GitHubAppPage.tsx index 8070d4d17c7..ae0fa0e73f0 100644 --- a/client/web/src/components/gitHubApps/GitHubAppPage.tsx +++ b/client/web/src/components/gitHubApps/GitHubAppPage.tsx @@ -74,7 +74,7 @@ export const GitHubAppPage: FC = ({ telemetryService, headerParentBreadcr const onAddInstallation = async (app: NonNullable): Promise => { try { - const req = await fetch(`/.auth/githubapp/state?id=${app?.id}&domain=${app?.domain}`) + const req = await fetch(`/githubapp/state?id=${app?.id}&domain=${app?.domain}`) const state = await req.text() const trailingSlash = app.appURL.endsWith('/') ? '' : '/' window.location.assign(`${app.appURL}${trailingSlash}installations/new?state=${state}`) diff --git a/cmd/frontend/internal/app/app.go b/cmd/frontend/internal/app/app.go index eda17f9cbbd..73d5bfbf318 100644 --- a/cmd/frontend/internal/app/app.go +++ b/cmd/frontend/internal/app/app.go @@ -22,7 +22,7 @@ import ( // // 🚨 SECURITY: The caller MUST wrap the returned handler in middleware that checks authentication // and sets the actor in the request context. -func NewHandler(db database.DB, logger log.Logger, githubAppSetupHandler http.Handler) http.Handler { +func NewHandler(db database.DB, logger log.Logger) http.Handler { session.SetSessionStore(session.NewRedisStore(func() bool { return globals.ExternalURL().Scheme == "https" })) @@ -78,10 +78,6 @@ func NewHandler(db database.DB, logger log.Logger, githubAppSetupHandler http.Ha // Ping retrieval r.Get(router.LatestPing).Handler(trace.Route(latestPingHandler(db))) - // Sourcegraph GitHub App setup (Cloud and on-prem) - r.Get(router.SetupGitHubAppCloud).Handler(trace.Route(githubAppSetupHandler)) - r.Get(router.SetupGitHubApp).Handler(trace.Route(githubAppSetupHandler)) - r.Get(router.Editor).Handler(trace.Route(errorutil.Handler(serveEditor(db)))) r.Get(router.DebugHeaders).Handler(trace.Route(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/cmd/frontend/internal/app/router/router.go b/cmd/frontend/internal/app/router/router.go index b1106944058..ae758a310e8 100644 --- a/cmd/frontend/internal/app/router/router.go +++ b/cmd/frontend/internal/app/router/router.go @@ -40,9 +40,6 @@ const ( LatestPing = "pings.latest" - SetupGitHubAppCloud = "setup.github.app.cloud" - SetupGitHubApp = "setup.github.app" - OldToolsRedirect = "old-tools-redirect" OldTreeRedirect = "old-tree-redirect" @@ -103,9 +100,6 @@ func newRouter() *mux.Router { base.Path("/site-admin/pings/latest").Methods("GET").Name(LatestPing) - base.Path("/setup/github/app/cloud").Methods("GET").Name(SetupGitHubAppCloud) - base.Path("/setup/github/app").Methods("GET").Name(SetupGitHubApp) - repoPath := `/` + routevar.Repo repo := base.PathPrefix(repoPath + "/" + routevar.RepoPathDelim + "/").Subrouter() repo.Path("/badge.svg").Methods("GET").Name(RepoBadge) diff --git a/cmd/frontend/internal/app/ui/BUILD.bazel b/cmd/frontend/internal/app/ui/BUILD.bazel index 7170990a3e8..2967e61cdc5 100644 --- a/cmd/frontend/internal/app/ui/BUILD.bazel +++ b/cmd/frontend/internal/app/ui/BUILD.bazel @@ -32,6 +32,7 @@ go_library( "//cmd/frontend/internal/app/assetsutil", "//cmd/frontend/internal/app/jscontext", "//cmd/frontend/internal/app/ui/router", + "//cmd/frontend/internal/githubapp", "//cmd/frontend/internal/handlerutil", "//cmd/frontend/internal/routevar", "//cmd/frontend/internal/search", diff --git a/cmd/frontend/internal/app/ui/router.go b/cmd/frontend/internal/app/ui/router.go index 999e3e156bd..aa8935e93b5 100644 --- a/cmd/frontend/internal/app/ui/router.go +++ b/cmd/frontend/internal/app/ui/router.go @@ -18,6 +18,7 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/envvar" "github.com/sourcegraph/sourcegraph/cmd/frontend/globals" uirouter "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/app/ui/router" + "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/githubapp" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/routevar" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/search" "github.com/sourcegraph/sourcegraph/internal/conf" @@ -107,6 +108,9 @@ func InitRouter(db database.DB) { r.Path("/sign-in").Methods(http.MethodGet, http.MethodHead).Name(uirouter.RouteSignIn).Handler(handler(db, serveSignIn(db))) r.Path("/ping-from-self-hosted").Methods("GET", "OPTIONS").Name(uirouter.RoutePingFromSelfHosted).Handler(handler(db, servePingFromSelfHosted)) + ghAppRouter := r.PathPrefix("/githubapp/").Subrouter() + githubapp.SetupGitHubAppRoutes(ghAppRouter, db) + // Basic pages with static titles for _, p := range []struct { // Specify either path OR pathPrefix. diff --git a/cmd/frontend/internal/auth/BUILD.bazel b/cmd/frontend/internal/auth/BUILD.bazel index 285064b0f07..2a507ee2f7f 100644 --- a/cmd/frontend/internal/auth/BUILD.bazel +++ b/cmd/frontend/internal/auth/BUILD.bazel @@ -13,7 +13,6 @@ go_library( "//cmd/frontend/internal/auth/azureoauth", "//cmd/frontend/internal/auth/bitbucketcloudoauth", "//cmd/frontend/internal/auth/gerrit", - "//cmd/frontend/internal/auth/githubappauth", "//cmd/frontend/internal/auth/githuboauth", "//cmd/frontend/internal/auth/gitlaboauth", "//cmd/frontend/internal/auth/httpheader", diff --git a/cmd/frontend/internal/auth/githubappauth/middleware.go b/cmd/frontend/internal/auth/githubappauth/middleware.go deleted file mode 100644 index 0abd072e4cb..00000000000 --- a/cmd/frontend/internal/auth/githubappauth/middleware.go +++ /dev/null @@ -1,539 +0,0 @@ -package githubapp - -import ( - "crypto/rand" - "crypto/sha256" - "encoding/hex" - "encoding/json" - "fmt" - "io" - "net/http" - "net/url" - "strconv" - "strings" - "time" - - "github.com/google/uuid" - "github.com/gorilla/mux" - "github.com/graph-gophers/graphql-go" - "github.com/sourcegraph/log" - "go.opentelemetry.io/otel/attribute" - - "github.com/sourcegraph/sourcegraph/cmd/frontend/auth" - "github.com/sourcegraph/sourcegraph/cmd/frontend/backend" - authcheck "github.com/sourcegraph/sourcegraph/internal/auth" - "github.com/sourcegraph/sourcegraph/internal/database" - "github.com/sourcegraph/sourcegraph/internal/encryption" - "github.com/sourcegraph/sourcegraph/internal/encryption/keyring" - "github.com/sourcegraph/sourcegraph/internal/extsvc" - "github.com/sourcegraph/sourcegraph/internal/extsvc/github" - ghaauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" - ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" - "github.com/sourcegraph/sourcegraph/internal/httpcli" - "github.com/sourcegraph/sourcegraph/internal/rcache" - "github.com/sourcegraph/sourcegraph/internal/trace" - "github.com/sourcegraph/sourcegraph/internal/types" - "github.com/sourcegraph/sourcegraph/lib/errors" -) - -const authPrefix = auth.AuthURLPrefix + "/githubapp" - -func Middleware(db database.DB) *auth.Middleware { - return &auth.Middleware{ - API: func(next http.Handler) http.Handler { - return newMiddleware(db, authPrefix, true, next) - }, - App: func(next http.Handler) http.Handler { - return newMiddleware(db, authPrefix, false, next) - }, - } -} - -const cacheTTLSeconds = 60 * 60 // 1 hour - -func newMiddleware(db database.DB, authPrefix string, isAPIHandler bool, next http.Handler) http.Handler { - ghAppState := rcache.NewWithTTL("github_app_state", cacheTTLSeconds) - handler := newServeMux(db, authPrefix, ghAppState) - - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // This span should be manually finished before delegating to the next handler or - // redirecting. - span, _ := trace.New(r.Context(), "githubapp") - span.SetAttributes(attribute.Bool("isAPIHandler", isAPIHandler)) - span.End() - if strings.HasPrefix(r.URL.Path, authPrefix+"/") { - handler.ServeHTTP(w, r) - return - } - - next.ServeHTTP(w, r) - }) -} - -// checkSiteAdmin checks if the current user is a site admin and sets http error if not -func checkSiteAdmin(db database.DB, w http.ResponseWriter, req *http.Request) error { - err := authcheck.CheckCurrentUserIsSiteAdmin(req.Context(), db) - if err == nil { - return nil - } - status := http.StatusForbidden - if err == authcheck.ErrNotAuthenticated { - status = http.StatusUnauthorized - } - http.Error(w, "Bad request, user must be a site admin", status) - return err -} - -// RandomState returns a random sha256 hash that can be used as a state parameter. It is only -// exported for testing purposes. -func RandomState(n int) (string, error) { - data := make([]byte, n) - if _, err := io.ReadFull(rand.Reader, data); err != nil { - return "", err - } - - h := sha256.New() - h.Write(data) - return hex.EncodeToString(h.Sum(nil)), nil -} - -type GitHubAppResponse struct { - AppID int `json:"id"` - Slug string `json:"slug"` - Name string `json:"name"` - HtmlURL string `json:"html_url"` - ClientID string `json:"client_id"` - ClientSecret string `json:"client_secret"` - PEM string `json:"pem"` - WebhookSecret string `json:"webhook_secret"` - Permissions map[string]string `json:"permissions"` - Events []string `json:"events"` -} - -type gitHubAppStateDetails struct { - WebhookUUID string `json:"webhookUUID,omitempty"` - Domain string `json:"domain"` - AppID int `json:"app_id,omitempty"` - BaseURL string `json:"base_url,omitempty"` -} - -func newServeMux(db database.DB, prefix string, cache *rcache.Cache) http.Handler { - r := mux.NewRouter() - - r.Path(prefix + "/state").Methods("GET").HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - // 🚨 SECURITY: only site admins can create github apps - if err := checkSiteAdmin(db, w, req); err != nil { - http.Error(w, "User must be site admin", http.StatusForbidden) - return - } - - s, err := RandomState(128) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when generating state parameter: %s", err.Error()), http.StatusInternalServerError) - return - } - - gqlID := req.URL.Query().Get("id") - domain := req.URL.Query().Get("domain") - baseURL := req.URL.Query().Get("baseURL") - if gqlID == "" { - // we marshal an empty `gitHubAppStateDetails` struct because we want the values saved in the cache - // to always conform to the same structure. - stateDetails, err := json.Marshal(gitHubAppStateDetails{}) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) - return - } - cache.Set(s, stateDetails) - - _, _ = w.Write([]byte(s)) - return - } - - id64, err := UnmarshalGitHubAppID(graphql.ID(gqlID)) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error while unmarshalling App ID: %s", err.Error()), http.StatusBadRequest) - return - } - stateDetails, err := json.Marshal(gitHubAppStateDetails{ - AppID: int(id64), - Domain: domain, - BaseURL: baseURL, - }) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) - return - } - - cache.Set(s, stateDetails) - - _, _ = w.Write([]byte(s)) - }) - - r.Path(prefix + "/new-app-state").Methods("GET").HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - // 🚨 SECURITY: only site admins can create github apps - if err := checkSiteAdmin(db, w, req); err != nil { - http.Error(w, "User must be site admin", http.StatusForbidden) - return - } - - webhookURN := req.URL.Query().Get("webhookURN") - appName := req.URL.Query().Get("appName") - domain := req.URL.Query().Get("domain") - baseURL := req.URL.Query().Get("baseURL") - var webhookUUID string - if webhookURN != "" { - ws := backend.NewWebhookService(db, keyring.Default()) - hook, err := ws.CreateWebhook(req.Context(), appName, extsvc.KindGitHub, webhookURN, nil) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error while setting up webhook endpoint: %s", err.Error()), http.StatusInternalServerError) - return - } - webhookUUID = hook.UUID.String() - } - - s, err := RandomState(128) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when generating state parameter: %s", err.Error()), http.StatusInternalServerError) - return - } - - stateDetails, err := json.Marshal(gitHubAppStateDetails{ - WebhookUUID: webhookUUID, - Domain: domain, - BaseURL: baseURL, - }) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) - return - } - - cache.Set(s, stateDetails) - - resp := struct { - State string `json:"state"` - WebhookUUID string `json:"webhookUUID,omitempty"` - }{ - State: s, - WebhookUUID: webhookUUID, - } - - if err := json.NewEncoder(w).Encode(resp); err != nil { - http.Error(w, fmt.Sprintf("Unexpected error while writing response: %s", err.Error()), http.StatusInternalServerError) - } - }) - - r.Path(prefix + "/redirect").Methods("GET").HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - // 🚨 SECURITY: only site admins can setup github apps - if err := checkSiteAdmin(db, w, req); err != nil { - http.Error(w, "User must be site admin", http.StatusForbidden) - return - } - - query := req.URL.Query() - state := query.Get("state") - code := query.Get("code") - if state == "" || code == "" { - http.Error(w, "Bad request, code and state query params must be present", http.StatusBadRequest) - return - } - - // Check that the length of state is the expected length - if len(state) != 64 { - http.Error(w, "Bad request, state query param is wrong length", http.StatusBadRequest) - return - } - - stateValue, ok := cache.Get(state) - if !ok { - http.Error(w, "Bad request, state query param does not match", http.StatusBadRequest) - return - } - - var stateDetails gitHubAppStateDetails - err := json.Unmarshal(stateValue, &stateDetails) - if err != nil { - http.Error(w, "Bad request, invalid state", http.StatusBadRequest) - return - } - // Wait until we've validated the type of state before deleting it from the cache. - cache.Delete(state) - - webhookUUID, err := uuid.Parse(stateDetails.WebhookUUID) - if err != nil { - http.Error(w, fmt.Sprintf("Bad request, could not parse webhook UUID: %v", err), http.StatusBadRequest) - return - } - - baseURL, err := url.Parse(stateDetails.BaseURL) - if err != nil { - http.Error(w, fmt.Sprintf("Bad request, could not parse baseURL from state: %v, error: %v", stateDetails.BaseURL, err), http.StatusInternalServerError) - return - } - - apiURL, _ := github.APIRoot(baseURL) - u, err := url.JoinPath(apiURL.String(), "/app-manifests", code, "conversions") - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when building manifest endpoint URL: %v", err), http.StatusInternalServerError) - return - } - - domain, err := parseDomain(&stateDetails.Domain) - if err != nil { - http.Error(w, fmt.Sprintf("Unable to parse domain: %v", err), http.StatusBadRequest) - return - } - - app, err := createGitHubApp(u, *domain, httpcli.UncachedExternalClient) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error while converting github app: %s", err.Error()), http.StatusInternalServerError) - return - } - - id, err := db.GitHubApps().Create(req.Context(), app) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error while storing github app in DB: %s", err.Error()), http.StatusInternalServerError) - return - } - - webhookDB := db.Webhooks(keyring.Default().WebhookKey) - hook, err := webhookDB.GetByUUID(req.Context(), webhookUUID) - if err != nil { - http.Error(w, fmt.Sprintf("Error while fetching webhook: %s", err.Error()), http.StatusInternalServerError) - return - } - hook.Secret = encryption.NewUnencrypted(app.WebhookSecret) - hook.Name = app.Name - if _, err := webhookDB.Update(req.Context(), hook); err != nil { - http.Error(w, fmt.Sprintf("Error while updating webhook secret: %s", err.Error()), http.StatusInternalServerError) - return - } - - state, err = RandomState(128) - if err != nil { - http.Error(w, fmt.Sprintf("Unexpected error when creating state param: %s", err.Error()), http.StatusInternalServerError) - return - } - - newStateDetails, err := json.Marshal(gitHubAppStateDetails{ - Domain: stateDetails.Domain, - AppID: id, - }) - if err != nil { - http.Error(w, fmt.Sprintf("unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) - return - } - cache.Set(state, newStateDetails) - - // The installations page often takes a few seconds to become available after the - // app is first created, so we sleep for a bit to give it time to load. The exact - // length of time to sleep was determined empirically. - time.Sleep(3 * time.Second) - redirectURL, err := url.JoinPath(app.AppURL, "installations/new") - if err != nil { - // if there is an error, try to redirect to app url, which should show Install button as well - redirectURL = app.AppURL - } - http.Redirect(w, req, redirectURL+fmt.Sprintf("?state=%s", state), http.StatusSeeOther) - }) - - r.HandleFunc(prefix+"/setup", func(w http.ResponseWriter, req *http.Request) { - // 🚨 SECURITY: only site admins can setup github apps - if err := checkSiteAdmin(db, w, req); err != nil { - http.Error(w, "User must be site admin", http.StatusForbidden) - return - } - - query := req.URL.Query() - state := query.Get("state") - instID := query.Get("installation_id") - if state == "" || instID == "" { - // If neither state or installation ID is set, we redirect to the GitHub Apps page. - // This can happen when someone installs the App directly from GitHub, instead of - // following the link from within Sourcegraph. - http.Redirect(w, req, "/site-admin/github-apps", http.StatusFound) - return - } - - // Check that the length of state is the expected length - if len(state) != 64 { - http.Error(w, "Bad request, state query param is wrong length", http.StatusBadRequest) - return - } - - setupInfo, ok := cache.Get(state) - if !ok { - redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, state query param does not match")) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - var stateDetails gitHubAppStateDetails - err := json.Unmarshal(setupInfo, &stateDetails) - if err != nil { - redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, invalid state")) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - // Wait until we've validated the type of state before deleting it from the cache. - cache.Delete(state) - - installationID, err := strconv.Atoi(instID) - if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, nil, &stateDetails.AppID, nil, errors.New("Bad request, could not parse installation ID")) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - action := query.Get("setup_action") - if action == "install" { - ctx := req.Context() - app, err := db.GitHubApps().GetByID(ctx, stateDetails.AppID) - if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while fetching GitHub App from DB: %s", err.Error())) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - auther, err := ghaauth.NewGitHubAppAuthenticator(app.AppID, []byte(app.PrivateKey)) - if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while creating GitHubAppAuthenticator: %s", err.Error())) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - baseURL, err := url.Parse(app.BaseURL) - if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while parsing App base URL: %s", err.Error())) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - apiURL, _ := github.APIRoot(baseURL) - - logger := log.NoOp() - client := github.NewV3Client(logger, "", apiURL, auther, nil) - - // The installation often takes a few seconds to become available after the - // app is first installed, so we sleep for a bit to give it time to load. The exact - // length of time to sleep was determined empirically. - time.Sleep(3 * time.Second) - - remoteInstall, err := client.GetAppInstallation(ctx, int64(installationID)) - if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while fetching App installation details from GitHub: %s", err.Error())) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - _, err = db.GitHubApps().Install(ctx, ghtypes.GitHubAppInstallation{ - InstallationID: installationID, - AppID: app.ID, - URL: remoteInstall.GetHTMLURL(), - AccountLogin: remoteInstall.Account.GetLogin(), - AccountAvatarURL: remoteInstall.Account.GetAvatarURL(), - AccountURL: remoteInstall.Account.GetHTMLURL(), - AccountType: remoteInstall.Account.GetType(), - }) - if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, &app.Name, errors.Newf("Unexpected error while creating GitHub App installation: %s", err.Error())) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } - - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &app.ID, &app.Name, nil) - http.Redirect(w, req, redirectURL, http.StatusFound) - return - } else { - http.Error(w, fmt.Sprintf("Bad request; unsupported setup action: %s", action), http.StatusBadRequest) - return - } - }) - - return r -} - -func generateRedirectURL(domain *string, installationID, appID *int, appName *string, creationErr error) string { - // If we got an error but didn't even get far enough to parse a domain for the new - // GitHub App, we still want to route the user back to somewhere useful, so we send - // them back to the main site admin GitHub Apps page. - if domain == nil && creationErr != nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error())) - } - - parsedDomain, err := parseDomain(domain) - if err != nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("invalid domain: %s", *domain))) - } - - switch *parsedDomain { - case types.ReposGitHubAppDomain: - if creationErr != nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error())) - } - if installationID == nil || appID == nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape("missing installation ID or app ID")) - } - - return fmt.Sprintf("/site-admin/github-apps/%s?installation_id=%d", MarshalGitHubAppID(int64(*appID)), *installationID) - case types.BatchesGitHubAppDomain: - if creationErr != nil { - return fmt.Sprintf("/site-admin/batch-changes?success=false&error=%s", url.QueryEscape(creationErr.Error())) - } - - // This shouldn't really happen unless we also had an error, but we handle it just - // in case - if appName == nil { - return "/site-admin/batch-changes?success=true" - } - return fmt.Sprintf("/site-admin/batch-changes?success=true&app_name=%s", *appName) - default: - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("unsupported github apps domain: %v", parsedDomain))) - } -} - -var MockCreateGitHubApp func(conversionURL string, domain types.GitHubAppDomain) (*ghtypes.GitHubApp, error) - -func createGitHubApp(conversionURL string, domain types.GitHubAppDomain, httpClient *http.Client) (*ghtypes.GitHubApp, error) { - if MockCreateGitHubApp != nil { - return MockCreateGitHubApp(conversionURL, domain) - } - r, err := http.NewRequest(http.MethodPost, conversionURL, http.NoBody) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(r) - if err != nil { - return nil, err - } - if resp.StatusCode != http.StatusCreated { - return nil, errors.Newf("expected 201 statusCode, got: %d", resp.StatusCode) - } - - defer resp.Body.Close() - - var response GitHubAppResponse - if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { - return nil, err - } - - htmlURL, err := url.Parse(response.HtmlURL) - if err != nil { - return nil, err - } - - return &ghtypes.GitHubApp{ - AppID: response.AppID, - Name: response.Name, - Slug: response.Slug, - ClientID: response.ClientID, - ClientSecret: response.ClientSecret, - WebhookSecret: response.WebhookSecret, - PrivateKey: response.PEM, - BaseURL: htmlURL.Scheme + "://" + htmlURL.Host, - AppURL: htmlURL.String(), - Domain: domain, - Logo: fmt.Sprintf("%s://%s/identicons/app/app/%s", htmlURL.Scheme, htmlURL.Host, response.Slug), - }, nil -} diff --git a/cmd/frontend/internal/auth/init.go b/cmd/frontend/internal/auth/init.go index f700e97c197..1d358eaf3b3 100644 --- a/cmd/frontend/internal/auth/init.go +++ b/cmd/frontend/internal/auth/init.go @@ -17,7 +17,6 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/bitbucketcloudoauth" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/gerrit" - githubapp "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/githubappauth" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/githuboauth" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/gitlaboauth" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/httpheader" @@ -54,7 +53,6 @@ func Init(logger log.Logger, db database.DB) { gitlaboauth.Middleware(db), bitbucketcloudoauth.Middleware(db), azureoauth.Middleware(db), - githubapp.Middleware(db), ) // Register app-level sign-out handler app.RegisterSSOSignOutHandler(ssoSignOutHandler) diff --git a/cmd/frontend/internal/batches/resolvers/BUILD.bazel b/cmd/frontend/internal/batches/resolvers/BUILD.bazel index 40402968042..02acee78dd1 100644 --- a/cmd/frontend/internal/batches/resolvers/BUILD.bazel +++ b/cmd/frontend/internal/batches/resolvers/BUILD.bazel @@ -44,7 +44,7 @@ go_library( "//cmd/frontend/graphqlbackend", "//cmd/frontend/graphqlbackend/externallink", "//cmd/frontend/graphqlbackend/graphqlutil", - "//cmd/frontend/internal/auth/githubappauth", + "//cmd/frontend/internal/githubapp", "//internal/actor", "//internal/api", "//internal/auth", @@ -132,8 +132,8 @@ go_test( "//cmd/frontend/globals", "//cmd/frontend/graphqlbackend", "//cmd/frontend/graphqlbackend/externallink", - "//cmd/frontend/internal/auth/githubappauth", "//cmd/frontend/internal/batches/resolvers/apitest", + "//cmd/frontend/internal/githubapp", "//internal/actor", "//internal/api", "//internal/batches/graphql", diff --git a/cmd/frontend/internal/batches/resolvers/code_host.go b/cmd/frontend/internal/batches/resolvers/code_host.go index 2f57e95940c..81da61205ea 100644 --- a/cmd/frontend/internal/batches/resolvers/code_host.go +++ b/cmd/frontend/internal/batches/resolvers/code_host.go @@ -6,7 +6,7 @@ import ( "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend" - githubapp "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/githubappauth" + "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/githubapp" "github.com/sourcegraph/sourcegraph/internal/batches/store" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/database" diff --git a/cmd/frontend/internal/batches/resolvers/main_test.go b/cmd/frontend/internal/batches/resolvers/main_test.go index 4a6aa668e98..e7d70bdf36d 100644 --- a/cmd/frontend/internal/batches/resolvers/main_test.go +++ b/cmd/frontend/internal/batches/resolvers/main_test.go @@ -18,8 +18,8 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/backend" "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend" - githubapp "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/githubappauth" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/batches/resolvers/apitest" + "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/githubapp" "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/batches/store" diff --git a/cmd/frontend/internal/cli/http.go b/cmd/frontend/internal/cli/http.go index 894e40c3a1a..d14ff7f6d49 100644 --- a/cmd/frontend/internal/cli/http.go +++ b/cmd/frontend/internal/cli/http.go @@ -45,7 +45,6 @@ func newExternalHTTPHandler( rateLimitWatcher graphqlbackend.LimitWatcher, handlers *httpapi.Handlers, newExecutorProxyHandler enterprise.NewExecutorProxyHandler, - newGitHubAppSetupHandler enterprise.NewGitHubAppSetupHandler, ) (http.Handler, error) { logger := log.Scoped("external") @@ -81,10 +80,8 @@ func newExternalHTTPHandler( // 🚨 SECURITY: This handler implements its own token auth inside enterprise executorProxyHandler := newExecutorProxyHandler() - githubAppSetupHandler := newGitHubAppSetupHandler() - // App handler (HTML pages), the call order of middleware is LIFO. - appHandler := app.NewHandler(db, logger, githubAppSetupHandler) + appHandler := app.NewHandler(db, logger) if hooks.PostAuthMiddleware != nil { // 🚨 SECURITY: These all run after the auth handler so the client is authenticated. appHandler = hooks.PostAuthMiddleware(appHandler) diff --git a/cmd/frontend/internal/cli/serve_cmd.go b/cmd/frontend/internal/cli/serve_cmd.go index c94b8638068..fbdf50d87c0 100644 --- a/cmd/frontend/internal/cli/serve_cmd.go +++ b/cmd/frontend/internal/cli/serve_cmd.go @@ -365,7 +365,6 @@ func makeExternalAPI(db database.DB, logger sglog.Logger, schema *graphql.Schema NewCodeCompletionsHandler: enterprise.NewCodeCompletionsHandler, }, enterprise.NewExecutorProxyHandler, - enterprise.NewGitHubAppSetupHandler, ) if err != nil { return nil, errors.Errorf("create external HTTP handler: %v", err) diff --git a/cmd/frontend/internal/auth/githubappauth/BUILD.bazel b/cmd/frontend/internal/githubapp/BUILD.bazel similarity index 87% rename from cmd/frontend/internal/auth/githubappauth/BUILD.bazel rename to cmd/frontend/internal/githubapp/BUILD.bazel index 006d832b962..9d64b5b85cb 100644 --- a/cmd/frontend/internal/auth/githubappauth/BUILD.bazel +++ b/cmd/frontend/internal/githubapp/BUILD.bazel @@ -2,16 +2,15 @@ load("//dev:go_defs.bzl", "go_test") load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( - name = "githubappauth", + name = "githubapp", srcs = [ + "httpapi.go", "init.go", - "middleware.go", "resolver.go", ], - importpath = "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/githubappauth", + importpath = "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/githubapp", visibility = ["//cmd/frontend:__subpackages__"], deps = [ - "//cmd/frontend/auth", "//cmd/frontend/backend", "//cmd/frontend/enterprise", "//cmd/frontend/graphqlbackend", @@ -30,7 +29,6 @@ go_library( "//internal/httpcli", "//internal/observation", "//internal/rcache", - "//internal/trace", "//internal/types", "//lib/errors", "//schema", @@ -39,17 +37,16 @@ go_library( "@com_github_graph_gophers_graphql_go//:graphql-go", "@com_github_graph_gophers_graphql_go//relay", "@com_github_sourcegraph_log//:log", - "@io_opentelemetry_go_otel//attribute", ], ) go_test( - name = "githubappauth_test", + name = "githubapp_test", srcs = [ - "middleware_test.go", + "httpapi_test.go", "resolver_test.go", ], - embed = [":githubappauth"], + embed = [":githubapp"], tags = [ # Test requires localhost database "requires-network", @@ -67,6 +64,7 @@ go_test( "//internal/types", "//lib/errors", "@com_github_google_uuid//:uuid", + "@com_github_gorilla_mux//:mux", "@com_github_graph_gophers_graphql_go//errors", "@com_github_sourcegraph_log//logtest", "@com_github_stretchr_testify//assert", diff --git a/cmd/frontend/internal/githubapp/httpapi.go b/cmd/frontend/internal/githubapp/httpapi.go new file mode 100644 index 00000000000..c41502678e2 --- /dev/null +++ b/cmd/frontend/internal/githubapp/httpapi.go @@ -0,0 +1,517 @@ +package githubapp + +import ( + "crypto/rand" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "time" + + "github.com/google/uuid" + "github.com/gorilla/mux" + "github.com/graph-gophers/graphql-go" + "github.com/sourcegraph/log" + + "github.com/sourcegraph/sourcegraph/cmd/frontend/backend" + authcheck "github.com/sourcegraph/sourcegraph/internal/auth" + "github.com/sourcegraph/sourcegraph/internal/database" + "github.com/sourcegraph/sourcegraph/internal/encryption" + "github.com/sourcegraph/sourcegraph/internal/encryption/keyring" + "github.com/sourcegraph/sourcegraph/internal/extsvc" + "github.com/sourcegraph/sourcegraph/internal/extsvc/github" + ghaauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" + "github.com/sourcegraph/sourcegraph/internal/httpcli" + "github.com/sourcegraph/sourcegraph/internal/rcache" + "github.com/sourcegraph/sourcegraph/internal/types" + "github.com/sourcegraph/sourcegraph/lib/errors" +) + +const cacheTTLSeconds = 60 * 60 // 1 hour + +type gitHubAppServer struct { + cache *rcache.Cache + db database.DB +} + +func (srv *gitHubAppServer) siteAdminMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // 🚨 SECURITY: only site admins can create github apps + if err := authcheck.CheckCurrentUserIsSiteAdmin(r.Context(), srv.db); err != nil { + if errors.Is(err, authcheck.ErrMustBeSiteAdmin) { + http.Error(w, "User must be site admin", http.StatusForbidden) + return + } + + if errors.Is(err, authcheck.ErrNotAuthenticated) { + http.Error(w, "User must be authenticated", http.StatusUnauthorized) + return + } + + http.Error(w, "Something went wrong", http.StatusInternalServerError) + return + } + + next.ServeHTTP(w, r) + }) +} + +func (srv *gitHubAppServer) registerRoutes(router *mux.Router) { + router.Path("/state").Methods("GET").HandlerFunc(srv.stateHandler) + router.Path("/new-app-state").Methods("GET").HandlerFunc(srv.newAppStateHandler) + router.Path("/redirect").Methods("GET").HandlerFunc(srv.redirectHandler) + router.Path("/setup").Methods("GET").HandlerFunc(srv.setupHandler) + + router.Use(srv.siteAdminMiddleware) +} + +// SetupGitHubAppRoutes registers the routes for the GitHub App setup API. +func SetupGitHubAppRoutes(router *mux.Router, db database.DB) { + ghAppState := rcache.NewWithTTL("github_app_state", cacheTTLSeconds) + appServer := &gitHubAppServer{ + cache: ghAppState, + db: db, + } + + appServer.registerRoutes(router) +} + +// setupGitHubAppRoutesWithCache is the same as SetupGitHubAppRoutes but allows to pass a cache. +// Useful for testing. +func setupGitHubAppRoutesWithCache(router *mux.Router, db database.DB, cache *rcache.Cache) { + appServer := &gitHubAppServer{ + cache: cache, + db: db, + } + + appServer.registerRoutes(router) +} + +// randomState returns a random sha256 hash that can be used as a state parameter. It is only +// exported for testing purposes. +func randomState() (string, error) { + data := make([]byte, 128) + if _, err := io.ReadFull(rand.Reader, data); err != nil { + return "", err + } + + h := sha256.New() + h.Write(data) + return hex.EncodeToString(h.Sum(nil)), nil +} + +type GitHubAppResponse struct { + AppID int `json:"id"` + Slug string `json:"slug"` + Name string `json:"name"` + HtmlURL string `json:"html_url"` + ClientID string `json:"client_id"` + ClientSecret string `json:"client_secret"` + PEM string `json:"pem"` + WebhookSecret string `json:"webhook_secret"` + Permissions map[string]string `json:"permissions"` + Events []string `json:"events"` +} + +type gitHubAppStateDetails struct { + WebhookUUID string `json:"webhookUUID,omitempty"` + Domain string `json:"domain"` + AppID int `json:"app_id,omitempty"` + BaseURL string `json:"base_url,omitempty"` +} + +func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request) { + s, err := randomState() + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when generating state parameter: %s", err.Error()), http.StatusInternalServerError) + return + } + + gqlID := r.URL.Query().Get("id") + domain := r.URL.Query().Get("domain") + baseURL := r.URL.Query().Get("baseURL") + if gqlID == "" { + // we marshal an empty `gitHubAppStateDetails` struct because we want the values saved in the cache + // to always conform to the same structure. + stateDetails, err := json.Marshal(gitHubAppStateDetails{}) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) + return + } + srv.cache.Set(s, stateDetails) + + _, _ = w.Write([]byte(s)) + return + } + + id64, err := UnmarshalGitHubAppID(graphql.ID(gqlID)) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error while unmarshalling App ID: %s", err.Error()), http.StatusBadRequest) + return + } + stateDetails, err := json.Marshal(gitHubAppStateDetails{ + AppID: int(id64), + Domain: domain, + BaseURL: baseURL, + }) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) + return + } + + srv.cache.Set(s, stateDetails) + + _, _ = w.Write([]byte(s)) +} + +func (srv *gitHubAppServer) newAppStateHandler(w http.ResponseWriter, r *http.Request) { + webhookURN := r.URL.Query().Get("webhookURN") + appName := r.URL.Query().Get("appName") + domain := r.URL.Query().Get("domain") + baseURL := r.URL.Query().Get("baseURL") + var webhookUUID string + if webhookURN != "" { + ws := backend.NewWebhookService(srv.db, keyring.Default()) + hook, err := ws.CreateWebhook(r.Context(), appName, extsvc.KindGitHub, webhookURN, nil) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error while setting up webhook endpoint: %s", err.Error()), http.StatusInternalServerError) + return + } + webhookUUID = hook.UUID.String() + } + + s, err := randomState() + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when generating state parameter: %s", err.Error()), http.StatusInternalServerError) + return + } + + stateDetails, err := json.Marshal(gitHubAppStateDetails{ + WebhookUUID: webhookUUID, + Domain: domain, + BaseURL: baseURL, + }) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) + return + } + + srv.cache.Set(s, stateDetails) + + resp := struct { + State string `json:"state"` + WebhookUUID string `json:"webhookUUID,omitempty"` + }{ + State: s, + WebhookUUID: webhookUUID, + } + + if err := json.NewEncoder(w).Encode(resp); err != nil { + http.Error(w, fmt.Sprintf("Unexpected error while writing response: %s", err.Error()), http.StatusInternalServerError) + } +} + +func (srv *gitHubAppServer) redirectHandler(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + state := query.Get("state") + code := query.Get("code") + if state == "" || code == "" { + http.Error(w, "Bad request, code and state query params must be present", http.StatusBadRequest) + return + } + + // Check that the length of state is the expected length + if len(state) != 64 { + http.Error(w, "Bad request, state query param is wrong length", http.StatusBadRequest) + return + } + + stateValue, ok := srv.cache.Get(state) + if !ok { + http.Error(w, "Bad request, state query param does not match", http.StatusBadRequest) + return + } + + var stateDetails gitHubAppStateDetails + err := json.Unmarshal(stateValue, &stateDetails) + if err != nil { + http.Error(w, "Bad request, invalid state", http.StatusBadRequest) + return + } + // Wait until we've validated the type of state before deleting it from the cache. + srv.cache.Delete(state) + + webhookUUID, err := uuid.Parse(stateDetails.WebhookUUID) + if err != nil { + http.Error(w, fmt.Sprintf("Bad request, could not parse webhook UUID: %v", err), http.StatusBadRequest) + return + } + + baseURL, err := url.Parse(stateDetails.BaseURL) + if err != nil { + http.Error(w, fmt.Sprintf("Bad request, could not parse baseURL from state: %v, error: %v", stateDetails.BaseURL, err), http.StatusInternalServerError) + return + } + + apiURL, _ := github.APIRoot(baseURL) + u, err := url.JoinPath(apiURL.String(), "/app-manifests", code, "conversions") + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when building manifest endpoint URL: %v", err), http.StatusInternalServerError) + return + } + + domain, err := parseDomain(&stateDetails.Domain) + if err != nil { + http.Error(w, fmt.Sprintf("Unable to parse domain: %v", err), http.StatusBadRequest) + return + } + + app, err := createGitHubApp(u, *domain, httpcli.UncachedExternalClient) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error while converting github app: %s", err.Error()), http.StatusInternalServerError) + return + } + + id, err := srv.db.GitHubApps().Create(r.Context(), app) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error while storing github app in DB: %s", err.Error()), http.StatusInternalServerError) + return + } + + webhookDB := srv.db.Webhooks(keyring.Default().WebhookKey) + hook, err := webhookDB.GetByUUID(r.Context(), webhookUUID) + if err != nil { + http.Error(w, fmt.Sprintf("Error while fetching webhook: %s", err.Error()), http.StatusInternalServerError) + return + } + hook.Secret = encryption.NewUnencrypted(app.WebhookSecret) + hook.Name = app.Name + if _, err := webhookDB.Update(r.Context(), hook); err != nil { + http.Error(w, fmt.Sprintf("Error while updating webhook secret: %s", err.Error()), http.StatusInternalServerError) + return + } + + state, err = randomState() + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error when creating state param: %s", err.Error()), http.StatusInternalServerError) + return + } + + newStateDetails, err := json.Marshal(gitHubAppStateDetails{ + Domain: stateDetails.Domain, + AppID: id, + }) + if err != nil { + http.Error(w, fmt.Sprintf("unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) + return + } + srv.cache.Set(state, newStateDetails) + + // The installations page often takes a few seconds to become available after the + // app is first created, so we sleep for a bit to give it time to load. The exact + // length of time to sleep was determined empirically. + time.Sleep(3 * time.Second) + redirectURL, err := url.JoinPath(app.AppURL, "installations/new") + if err != nil { + // if there is an error, try to redirect to app url, which should show Install button as well + redirectURL = app.AppURL + } + http.Redirect(w, r, redirectURL+fmt.Sprintf("?state=%s", state), http.StatusSeeOther) +} + +func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + state := query.Get("state") + instID := query.Get("installation_id") + if state == "" || instID == "" { + // If neither state or installation ID is set, we redirect to the GitHub Apps page. + // This can happen when someone installs the App directly from GitHub, instead of + // following the link from within Sourcegraph. + http.Redirect(w, r, "/site-admin/github-apps", http.StatusFound) + return + } + + // Check that the length of state is the expected length + if len(state) != 64 { + http.Error(w, "Bad request, state query param is wrong length", http.StatusBadRequest) + return + } + + setupInfo, ok := srv.cache.Get(state) + if !ok { + redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, state query param does not match")) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + var stateDetails gitHubAppStateDetails + err := json.Unmarshal(setupInfo, &stateDetails) + if err != nil { + redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, invalid state")) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + // Wait until we've validated the type of state before deleting it from the cache. + srv.cache.Delete(state) + + installationID, err := strconv.Atoi(instID) + if err != nil { + redirectURL := generateRedirectURL(&stateDetails.Domain, nil, &stateDetails.AppID, nil, errors.New("Bad request, could not parse installation ID")) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + action := query.Get("setup_action") + if action == "install" { + ctx := r.Context() + app, err := srv.db.GitHubApps().GetByID(ctx, stateDetails.AppID) + if err != nil { + redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while fetching GitHub App from DB: %s", err.Error())) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + auther, err := ghaauth.NewGitHubAppAuthenticator(app.AppID, []byte(app.PrivateKey)) + if err != nil { + redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while creating GitHubAppAuthenticator: %s", err.Error())) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + baseURL, err := url.Parse(app.BaseURL) + if err != nil { + redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while parsing App base URL: %s", err.Error())) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + apiURL, _ := github.APIRoot(baseURL) + + logger := log.NoOp() + client := github.NewV3Client(logger, "", apiURL, auther, nil) + + // The installation often takes a few seconds to become available after the + // app is first installed, so we sleep for a bit to give it time to load. The exact + // length of time to sleep was determined empirically. + time.Sleep(3 * time.Second) + + remoteInstall, err := client.GetAppInstallation(ctx, int64(installationID)) + if err != nil { + redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while fetching App installation details from GitHub: %s", err.Error())) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + _, err = srv.db.GitHubApps().Install(ctx, ghtypes.GitHubAppInstallation{ + InstallationID: installationID, + AppID: app.ID, + URL: remoteInstall.GetHTMLURL(), + AccountLogin: remoteInstall.Account.GetLogin(), + AccountAvatarURL: remoteInstall.Account.GetAvatarURL(), + AccountURL: remoteInstall.Account.GetHTMLURL(), + AccountType: remoteInstall.Account.GetType(), + }) + if err != nil { + redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, &app.Name, errors.Newf("Unexpected error while creating GitHub App installation: %s", err.Error())) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + + redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &app.ID, &app.Name, nil) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } else { + http.Error(w, fmt.Sprintf("Bad request; unsupported setup action: %s", action), http.StatusBadRequest) + return + } +} + +func generateRedirectURL(domain *string, installationID, appID *int, appName *string, creationErr error) string { + // If we got an error but didn't even get far enough to parse a domain for the new + // GitHub App, we still want to route the user back to somewhere useful, so we send + // them back to the main site admin GitHub Apps page. + if domain == nil && creationErr != nil { + return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error())) + } + + parsedDomain, err := parseDomain(domain) + if err != nil { + return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("invalid domain: %s", *domain))) + } + + switch *parsedDomain { + case types.ReposGitHubAppDomain: + if creationErr != nil { + return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error())) + } + if installationID == nil || appID == nil { + return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape("missing installation ID or app ID")) + } + + return fmt.Sprintf("/site-admin/github-apps/%s?installation_id=%d", MarshalGitHubAppID(int64(*appID)), *installationID) + case types.BatchesGitHubAppDomain: + if creationErr != nil { + return fmt.Sprintf("/site-admin/batch-changes?success=false&error=%s", url.QueryEscape(creationErr.Error())) + } + + // This shouldn't really happen unless we also had an error, but we handle it just + // in case + if appName == nil { + return "/site-admin/batch-changes?success=true" + } + return fmt.Sprintf("/site-admin/batch-changes?success=true&app_name=%s", *appName) + default: + return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("unsupported github apps domain: %v", parsedDomain))) + } +} + +var MockCreateGitHubApp func(conversionURL string, domain types.GitHubAppDomain) (*ghtypes.GitHubApp, error) + +func createGitHubApp(conversionURL string, domain types.GitHubAppDomain, httpClient *http.Client) (*ghtypes.GitHubApp, error) { + if MockCreateGitHubApp != nil { + return MockCreateGitHubApp(conversionURL, domain) + } + r, err := http.NewRequest(http.MethodPost, conversionURL, http.NoBody) + if err != nil { + return nil, err + } + + resp, err := httpClient.Do(r) + if err != nil { + return nil, err + } + if resp.StatusCode != http.StatusCreated { + return nil, errors.Newf("expected 201 statusCode, got: %d", resp.StatusCode) + } + + defer resp.Body.Close() + + var response GitHubAppResponse + if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { + return nil, err + } + + htmlURL, err := url.Parse(response.HtmlURL) + if err != nil { + return nil, err + } + + return &ghtypes.GitHubApp{ + AppID: response.AppID, + Name: response.Name, + Slug: response.Slug, + ClientID: response.ClientID, + ClientSecret: response.ClientSecret, + WebhookSecret: response.WebhookSecret, + PrivateKey: response.PEM, + BaseURL: htmlURL.Scheme + "://" + htmlURL.Host, + AppURL: htmlURL.String(), + Domain: domain, + Logo: fmt.Sprintf("%s://%s/identicons/app/app/%s", htmlURL.Scheme, htmlURL.Host, response.Slug), + }, nil +} diff --git a/cmd/frontend/internal/auth/githubappauth/middleware_test.go b/cmd/frontend/internal/githubapp/httpapi_test.go similarity index 98% rename from cmd/frontend/internal/auth/githubappauth/middleware_test.go rename to cmd/frontend/internal/githubapp/httpapi_test.go index 5683607b7d5..56841357d5d 100644 --- a/cmd/frontend/internal/auth/githubappauth/middleware_test.go +++ b/cmd/frontend/internal/githubapp/httpapi_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "testing" + "github.com/gorilla/mux" "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/google/uuid" @@ -80,7 +81,7 @@ func TestGenerateRedirectURL(t *testing.T) { } } -func TestGithubAppAuthMiddleware(t *testing.T) { +func TestGithubAppHTTPAPI(t *testing.T) { t.Cleanup(func() { MockCreateGitHubApp = nil }) @@ -133,7 +134,10 @@ func TestGithubAppAuthMiddleware(t *testing.T) { rcache.SetupForTest(t) cache := rcache.NewWithTTL("test_cache", 200) - mux := newServeMux(db, "/githubapp", cache) + mux := mux.NewRouter() + subrouter := mux.PathPrefix("/githubapp/").Subrouter() + + setupGitHubAppRoutesWithCache(subrouter, db, cache) t.Run("/state", func(t *testing.T) { req := httptest.NewRequest("GET", "/githubapp/state", nil) @@ -256,7 +260,7 @@ func TestGithubAppAuthMiddleware(t *testing.T) { t.Run("/redirect", func(t *testing.T) { baseURL := "/githubapp/redirect" code := "2644896245sasdsf6dsd" - state, err := RandomState(128) + state, err := randomState() if err != nil { t.Fatalf("unexpected error generating random state: %s", err.Error()) } @@ -333,7 +337,7 @@ func TestGithubAppAuthMiddleware(t *testing.T) { t.Run("/setup", func(t *testing.T) { baseURL := "/githubapp/setup" - state, err := RandomState(128) + state, err := randomState() if err != nil { t.Fatalf("unexpected error generating random state: %s", err.Error()) } diff --git a/cmd/frontend/internal/auth/githubappauth/init.go b/cmd/frontend/internal/githubapp/init.go similarity index 79% rename from cmd/frontend/internal/auth/githubappauth/init.go rename to cmd/frontend/internal/githubapp/init.go index 8498d21f217..63146236d55 100644 --- a/cmd/frontend/internal/auth/githubappauth/init.go +++ b/cmd/frontend/internal/githubapp/init.go @@ -18,8 +18,8 @@ func Init( db database.DB, _ codeintel.Services, _ conftypes.UnifiedWatchable, - enterpriseServices *enterprise.Services, + s *enterprise.Services, ) error { - enterpriseServices.GitHubAppsResolver = NewResolver(log.Scoped("GitHubAppsResolver"), db) + s.GitHubAppsResolver = NewResolver(log.Scoped("GitHubAppsResolver"), db) return nil } diff --git a/cmd/frontend/internal/auth/githubappauth/resolver.go b/cmd/frontend/internal/githubapp/resolver.go similarity index 100% rename from cmd/frontend/internal/auth/githubappauth/resolver.go rename to cmd/frontend/internal/githubapp/resolver.go diff --git a/cmd/frontend/internal/auth/githubappauth/resolver_test.go b/cmd/frontend/internal/githubapp/resolver_test.go similarity index 100% rename from cmd/frontend/internal/auth/githubappauth/resolver_test.go rename to cmd/frontend/internal/githubapp/resolver_test.go diff --git a/cmd/frontend/internal/httpapi/httpapi.go b/cmd/frontend/internal/httpapi/httpapi.go index 3c221e48d59..63ce454f5b8 100644 --- a/cmd/frontend/internal/httpapi/httpapi.go +++ b/cmd/frontend/internal/httpapi/httpapi.go @@ -99,6 +99,7 @@ func NewHandler( m := mux.NewRouter().PathPrefix("/.api/").Subrouter() m.StrictSlash(true) + m.Use(trace.Route) jsonHandler := JsonMiddleware(&ErrorHandler{ Logger: logger, @@ -108,11 +109,11 @@ func NewHandler( WriteErrBody: env.InsecureDev, }) - m.PathPrefix("/registry").Methods("GET").Handler(trace.Route(jsonHandler(frontendregistry.HandleRegistry))) - m.PathPrefix("/scim/v2").Methods("GET", "POST", "PUT", "PATCH", "DELETE").Handler(trace.Route(handlers.SCIMHandler)) - m.Path("/graphql").Methods("POST").Handler(trace.Route(jsonHandler(serveGraphQL(logger, schema, rateLimiter, false)))) + m.PathPrefix("/registry").Methods("GET").Handler(jsonHandler(frontendregistry.HandleRegistry)) + m.PathPrefix("/scim/v2").Methods("GET", "POST", "PUT", "PATCH", "DELETE").Handler(handlers.SCIMHandler) + m.Path("/graphql").Methods("POST").Handler(jsonHandler(serveGraphQL(logger, schema, rateLimiter, false))) - m.Path("/opencodegraph").Methods("POST").Handler(trace.Route(jsonHandler(serveOpenCodeGraph(logger)))) + m.Path("/opencodegraph").Methods("POST").Handler(jsonHandler(serveOpenCodeGraph(logger))) // Webhooks // @@ -136,39 +137,39 @@ func NewHandler( // 🚨 SECURITY: This handler implements its own secret-based auth webhookMiddleware := webhooks.NewLogMiddleware(db.WebhookLogs(keyring.Default().WebhookLogKey)) webhookHandler := webhooks.NewHandler(logger, db, &wh) - m.Path("/webhooks/{webhook_uuid}").Methods("POST").Handler(trace.Route(webhookMiddleware.Logger(webhookHandler))) + m.Path("/webhooks/{webhook_uuid}").Methods("POST").Handler(webhookMiddleware.Logger(webhookHandler)) // Old, soon to be deprecated, webhook handlers gitHubWebhook := webhooks.GitHubWebhook{Router: &wh} - m.Path("/github-webhooks").Methods("POST").Handler(trace.Route(webhookMiddleware.Logger(&gitHubWebhook))) - m.Path("/gitlab-webhooks").Methods("POST").Handler(trace.Route(webhookMiddleware.Logger(handlers.BatchesGitLabWebhook))) - m.Path("/bitbucket-server-webhooks").Methods("POST").Handler(trace.Route(webhookMiddleware.Logger(handlers.BatchesBitbucketServerWebhook))) - m.Path("/bitbucket-cloud-webhooks").Methods("POST").Handler(trace.Route(webhookMiddleware.Logger(handlers.BatchesBitbucketCloudWebhook))) + m.Path("/github-webhooks").Methods("POST").Handler(webhookMiddleware.Logger(&gitHubWebhook)) + m.Path("/gitlab-webhooks").Methods("POST").Handler(webhookMiddleware.Logger(handlers.BatchesGitLabWebhook)) + m.Path("/bitbucket-server-webhooks").Methods("POST").Handler(webhookMiddleware.Logger(handlers.BatchesBitbucketServerWebhook)) + m.Path("/bitbucket-cloud-webhooks").Methods("POST").Handler(webhookMiddleware.Logger(handlers.BatchesBitbucketCloudWebhook)) // Other routes - m.Path("/files/batch-changes/{spec}/{file}").Methods("GET").Handler(trace.Route(handlers.BatchesChangesFileGetHandler)) - m.Path("/files/batch-changes/{spec}/{file}").Methods("HEAD").Handler(trace.Route(handlers.BatchesChangesFileExistsHandler)) - m.Path("/files/batch-changes/{spec}").Methods("POST").Handler(trace.Route(handlers.BatchesChangesFileUploadHandler)) - m.Path("/lsif/upload").Methods("POST").Handler(trace.Route(lsifDeprecationHandler)) - m.Path("/scip/upload").Methods("POST").Handler(trace.Route(handlers.NewCodeIntelUploadHandler(true))) - m.Path("/scip/upload").Methods("HEAD").Handler(trace.Route(noopHandler)) - m.Path("/compute/stream").Methods("GET", "POST").Handler(trace.Route(handlers.NewComputeStreamHandler())) - m.Path("/blame/" + routevar.Repo + routevar.RepoRevSuffix + "/stream/{Path:.*}").Methods("GET").Handler(trace.Route(handleStreamBlame(logger, db, gitserver.NewClient("http.blamestream")))) + m.Path("/files/batch-changes/{spec}/{file}").Methods("GET").Handler(handlers.BatchesChangesFileGetHandler) + m.Path("/files/batch-changes/{spec}/{file}").Methods("HEAD").Handler(handlers.BatchesChangesFileExistsHandler) + m.Path("/files/batch-changes/{spec}").Methods("POST").Handler(handlers.BatchesChangesFileUploadHandler) + m.Path("/lsif/upload").Methods("POST").Handler(lsifDeprecationHandler) + m.Path("/scip/upload").Methods("POST").Handler(handlers.NewCodeIntelUploadHandler(true)) + m.Path("/scip/upload").Methods("HEAD").Handler(noopHandler) + m.Path("/compute/stream").Methods("GET", "POST").Handler(handlers.NewComputeStreamHandler()) + m.Path("/blame/" + routevar.Repo + routevar.RepoRevSuffix + "/stream/{Path:.*}").Methods("GET").Handler(handleStreamBlame(logger, db, gitserver.NewClient("http.blamestream"))) // Set up the src-cli version cache handler (this will effectively be a // no-op anywhere other than dot-com). - m.Path("/src-cli/versions/{rest:.*}").Methods("GET", "POST").Handler(trace.Route(releasecache.NewHandler(logger))) + m.Path("/src-cli/versions/{rest:.*}").Methods("GET", "POST").Handler(releasecache.NewHandler(logger)) // Return the minimum src-cli version that's compatible with this instance - m.Path("/src-cli/{rest:.*}").Methods("GET").Handler(trace.Route(newSrcCliVersionHandler(logger))) - m.Path("/insights/export/{id}").Methods("GET").Handler(trace.Route(handlers.CodeInsightsDataExportHandler)) - m.Path("/search/stream").Methods("GET").Handler(trace.Route(frontendsearch.StreamHandler(db))) - m.Path("/search/export/{id}.jsonl").Methods("GET").Handler(trace.Route(handlers.SearchJobsDataExportHandler)) - m.Path("/search/export/{id}.log").Methods("GET").Handler(trace.Route(handlers.SearchJobsLogsHandler)) + m.Path("/src-cli/{rest:.*}").Methods("GET").Handler(newSrcCliVersionHandler(logger)) + m.Path("/insights/export/{id}").Methods("GET").Handler(handlers.CodeInsightsDataExportHandler) + m.Path("/search/stream").Methods("GET").Handler(frontendsearch.StreamHandler(db)) + m.Path("/search/export/{id}.jsonl").Methods("GET").Handler(handlers.SearchJobsDataExportHandler) + m.Path("/search/export/{id}.log").Methods("GET").Handler(handlers.SearchJobsLogsHandler) - m.Path("/completions/stream").Methods("POST").Handler(trace.Route(handlers.NewChatCompletionsStreamHandler())) - m.Path("/completions/code").Methods("POST").Handler(trace.Route(handlers.NewCodeCompletionsHandler())) + m.Path("/completions/stream").Methods("POST").Handler(handlers.NewChatCompletionsStreamHandler()) + m.Path("/completions/code").Methods("POST").Handler(handlers.NewCodeCompletionsHandler()) if envvar.SourcegraphDotComMode() { - m.Path("/license/check").Methods("POST").Name("dotcom.license.check").Handler(trace.Route(handlers.NewDotcomLicenseCheckHandler())) + m.Path("/license/check").Methods("POST").Name("dotcom.license.check").Handler(handlers.NewDotcomLicenseCheckHandler()) updatecheckHandler, err := updatecheck.ForwardHandler() if err != nil { @@ -177,7 +178,7 @@ func NewHandler( m.Path("/updates"). Methods(http.MethodGet, http.MethodPost). Name("updatecheck"). - Handler(trace.Route(updatecheckHandler)) + Handler(updatecheckHandler) } // repo contains routes that are NOT specific to a revision. In these routes, the URL may not contain a revspec after the repo (that is, no "github.com/foo/bar@myrevspec"). @@ -186,7 +187,7 @@ func NewHandler( // Additional paths added will be treated as a repo. To add a new path that should not be treated as a repo // add above repo paths. repo := m.PathPrefix(repoPath + "/" + routevar.RepoPathDelim + "/").Subrouter() - repo.Path("/shield").Methods("GET").Handler(trace.Route(jsonHandler(serveRepoShield()))) + repo.Path("/shield").Methods("GET").Handler(jsonHandler(serveRepoShield())) m.NotFoundHandler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { log.Printf("API no route: %s %s from %s", r.Method, r.URL, r.Referer()) diff --git a/cmd/frontend/shared/BUILD.bazel b/cmd/frontend/shared/BUILD.bazel index 76f50023c42..f576ae75310 100644 --- a/cmd/frontend/shared/BUILD.bazel +++ b/cmd/frontend/shared/BUILD.bazel @@ -12,7 +12,6 @@ go_library( deps = [ "//cmd/frontend/enterprise", "//cmd/frontend/internal/auth", - "//cmd/frontend/internal/auth/githubappauth", "//cmd/frontend/internal/authz", "//cmd/frontend/internal/batches", "//cmd/frontend/internal/cli", @@ -25,6 +24,7 @@ go_library( "//cmd/frontend/internal/dotcom", "//cmd/frontend/internal/embeddings", "//cmd/frontend/internal/executorqueue", + "//cmd/frontend/internal/githubapp", "//cmd/frontend/internal/guardrails", "//cmd/frontend/internal/insights", "//cmd/frontend/internal/licensing/init", diff --git a/cmd/frontend/shared/shared.go b/cmd/frontend/shared/shared.go index 14e9c72cc81..c100828c5ed 100644 --- a/cmd/frontend/shared/shared.go +++ b/cmd/frontend/shared/shared.go @@ -14,7 +14,6 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/enterprise" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth" - githubapp "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/auth/githubappauth" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/authz" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/batches" codeintelinit "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/codeintel" @@ -26,6 +25,7 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/dotcom" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/embeddings" executor "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/executorqueue" + "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/githubapp" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/guardrails" "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/insights" licensing "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/licensing/init"