diff --git a/AGENTS.md b/AGENTS.md index a27026b5..e2c8c449 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,6 +74,9 @@ Restricted strictly to read-only views and review execution. - **Subscriptions**: - `GET /api/v1/subscriptions/:id` ➔ `subscriptionsHandler.GetSubscription` - `GET /api/v1/subscriptions/current` ➔ `subscriptionsHandler.GetCurrentSubscription` +- **GitHub App** (a deliberate exception to read-only: anyone who can access a GitHub account on GitHub may connect it to their org; disconnecting stays owner-only): + - `POST /api/v1/github-app/start` ➔ `s.GitHubAppConnectStart` + - `POST /api/v1/github-app/installations/:connector_id/connect` ➔ `s.GitHubAppConnect` #### Dynamic Role Checks & Middlewares diff --git a/config/osv-scanner.toml b/config/osv-scanner.toml index 1680e928..42184f51 100644 --- a/config/osv-scanner.toml +++ b/config/osv-scanner.toml @@ -2,3 +2,8 @@ [[ignoredVulns]] id = "GO-2026-5932" reason = "False positive — golang.org/x/crypto/openpgp sub-package is never imported by this project; only golang.org/x/crypto/bcrypt is used." + +[[ignoredVulns]] +id = "GHSA-gcqx-w48m-w2h4" +ignoreUntil = 2027-01-28 +reason = "No patched release exists. deepmerge 4.3.1 is only pulled in by dev tooling (jest, fork-ts-checker-webpack-plugin); `npm ls deepmerge --omit=dev` is empty, so it never ships and never merges untrusted input. Re-check when a fixed version is published." diff --git a/docs/audit-events.md b/docs/audit-events.md index 1b368ca1..33f1fbef 100644 --- a/docs/audit-events.md +++ b/docs/audit-events.md @@ -74,6 +74,7 @@ Generated from `internal/auditlog/catalog.go`. | `chat.file.upload` | `chat` | file | info | Chat file uploaded | A file was attached to a chat. | | `connector.create` | `connector` | connector | warning | Integration connected | A source-control integration was connected. | | `connector.delete` | `connector` | connector | warning | Integration removed | A source-control integration was removed. | +| `connector.github_app.start` | `connector` | connector | info | GitHub App connection started | A member started connecting a GitHub account or organization through the GitHub App. | | `connector.reconnect` | `connector` | connector | info | Integration reconnected | A source-control integration was reconnected. | | `connector.update` | `connector` | connector | warning | Integration updated | A source-control integration was updated. | | `export.audit` | `export` | audit | warning | Audit log exported | Audit events were exported. | diff --git a/go.mod b/go.mod index b4f854cd..cbc6c77c 100644 --- a/go.mod +++ b/go.mod @@ -140,7 +140,7 @@ require ( github.com/felixge/httpsnoop v1.0.4 // indirect github.com/fsnotify/fsnotify v1.10.1 // indirect github.com/gitleaks/go-gitdiff v0.9.1 // indirect - github.com/go-jose/go-jose/v4 v4.1.4 // indirect + github.com/go-jose/go-jose/v4 v4.1.6 // indirect github.com/go-logr/logr v1.4.4 // indirect github.com/go-logr/stdr v1.2.2 // indirect github.com/go-openapi/jsonpointer v0.22.5 // indirect @@ -257,7 +257,7 @@ require ( golang.org/x/arch v0.25.0 // indirect golang.org/x/exp v0.0.0-20251023183803-a4bb9ffd2546 // indirect golang.org/x/mod v0.41.0 // indirect - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.24.0 // indirect golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index c8b57d9c..3f04f308 100644 --- a/go.sum +++ b/go.sum @@ -233,6 +233,8 @@ github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9 github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= +github.com/go-jose/go-jose/v4 v4.1.6 h1:sNRkftxFJxwsILiQukqp5z83QBSmntVwmgRY7AqBDO4= +github.com/go-jose/go-jose/v4 v4.1.6/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= github.com/go-logr/logr v1.4.4 h1:tG4xh9yMsRCAiodLVTxyrkzSZ9+o0L1Kg/+cPVcbP/8= github.com/go-logr/logr v1.4.4/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= @@ -713,6 +715,8 @@ golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= diff --git a/internal/api/audit_middleware.go b/internal/api/audit_middleware.go index 4f183207..42efbb33 100644 --- a/internal/api/audit_middleware.go +++ b/internal/api/audit_middleware.go @@ -170,8 +170,8 @@ func isAuthAuditSkipped(path string) bool { // audited registers a route and its audit spec atomically, so the two cannot // drift. It returns the Echo route (with the full path) for callers that need // it. -func (s *Server) audited(g *echo.Group, method, path string, spec auditlog.Spec, h echo.HandlerFunc) *echo.Route { - r := g.Add(method, path, h) +func (s *Server) audited(g *echo.Group, method, path string, spec auditlog.Spec, h echo.HandlerFunc, m ...echo.MiddlewareFunc) *echo.Route { + r := g.Add(method, path, h, m...) if s.auditRegistry != nil { s.auditRegistry.Register(method, r.Path, spec) } diff --git a/internal/api/auth/middleware.go b/internal/api/auth/middleware.go index baa8fff8..fdd99d97 100644 --- a/internal/api/auth/middleware.go +++ b/internal/api/auth/middleware.go @@ -1,6 +1,7 @@ package auth import ( + "context" "database/sql" "fmt" "net/http" @@ -252,11 +253,20 @@ func (am *AuthMiddleware) EnforceSubscriptionLimits() echo.MiddlewareFunc { } if err := loadPlanState(); err != nil { - if err == sql.ErrNoRows { + if err != sql.ErrNoRows { + fmt.Printf("[Subscription] plan lookup failed user=%d org=%d err=%v\n", user.ID, orgID, err) + return echo.NewHTTPError(http.StatusInternalServerError, "failed to check subscription") + } + // Not a member: only a super admin may continue. + plan, isSuperAdmin, err := am.loadSuperAdminPlan(reqCtx, user.ID, orgID) + if err != nil { + fmt.Printf("[Subscription] super admin plan lookup failed user=%d org=%d err=%v\n", user.ID, orgID, err) + return echo.NewHTTPError(http.StatusInternalServerError, "failed to check subscription") + } + if !isSuperAdmin { return echo.NewHTTPError(http.StatusForbidden, "no access to this organization") } - fmt.Printf("[Subscription] plan lookup failed user=%d org=%d err=%v\n", user.ID, orgID, err) - return echo.NewHTTPError(http.StatusInternalServerError, "failed to check subscription") + planType = plan } // Resolve plan type safely (NULL or empty -> free) @@ -280,6 +290,27 @@ func (am *AuthMiddleware) EnforceSubscriptionLimits() echo.MiddlewareFunc { } } +// loadSuperAdminPlan reads an org's plan for a super admin, who can open any org without being a member. +// isSuperAdmin is false (and plan empty) for everyone else; an org with no billing row yields an empty plan. +func (am *AuthMiddleware) loadSuperAdminPlan(ctx context.Context, userID, orgID int64) (plan sql.NullString, isSuperAdmin bool, err error) { + err = am.db.QueryRowContext(ctx, ` + SELECT EXISTS( + SELECT 1 FROM user_roles ur + JOIN roles r ON ur.role_id = r.id + WHERE ur.user_id = $1 AND r.name = $2 + ) + `, userID, RoleSuperAdmin).Scan(&isSuperAdmin) + if err != nil || !isSuperAdmin { + return plan, false, err + } + err = am.db.QueryRowContext(ctx, + `SELECT current_plan_code FROM org_billing_state WHERE org_id = $1`, orgID).Scan(&plan) + if err == sql.ErrNoRows { + err = nil + } + return plan, true, err +} + // BuildOrgContextFromHeader middleware extracts org_id from X-Org-Context header and validates org exists func (am *AuthMiddleware) BuildOrgContextFromHeader() echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { diff --git a/internal/api/github_app_auth.go b/internal/api/github_app_auth.go index c5997c34..225af467 100644 --- a/internal/api/github_app_auth.go +++ b/internal/api/github_app_auth.go @@ -24,6 +24,7 @@ import ( "github.com/livereview/internal/aidefault" "github.com/livereview/internal/api/auth" + "github.com/livereview/internal/auditlog" "github.com/livereview/internal/githubapp" "github.com/livereview/internal/license" githubprovider "github.com/livereview/internal/provider_input/github" @@ -64,8 +65,19 @@ func (s *Server) registerGitHubAppRoutes(public, v1 *echo.Group, authMiddleware authMiddleware.ValidateOrgAccess(), authMiddleware.BuildPermissionContext(), ) - // Session-only and org owners only, like disconnecting: it adds a GitHub account's repositories to the org. - v1.POST("/github-app/installations/:connector_id/connect", s.GitHubAppConnect, + // Session-only, any member of the org: it adds a GitHub account's repositories to the org. + s.audited(v1, echo.POST, "/github-app/installations/:connector_id/connect", + auditlog.Spec{Action: auditlog.ActionConnectorConnect, ResourceIDParam: "connector_id"}, + s.GitHubAppConnect, + authMiddleware.RequireAuth(), + authMiddleware.BuildOrgContextFromHeader(), + authMiddleware.ValidateOrgAccess(), + authMiddleware.BuildPermissionContext(), + ) + // Session-only, any member of the org: starts a GitHub round trip that connects to this org and keeps the session. + s.audited(v1, echo.POST, "/github-app/start", + auditlog.Spec{Action: auditlog.ActionGitHubAppConnectStart}, + s.GitHubAppConnectStart, authMiddleware.RequireAuth(), authMiddleware.BuildOrgContextFromHeader(), authMiddleware.ValidateOrgAccess(), @@ -242,15 +254,13 @@ func (s *Server) GitHubAppDisconnect(c echo.Context) error { // GitHubAppConnect connects an installation the caller's org has available but not connected: one it disconnected, // or one it was offered at sign-in because another LiveReview org already uses it. It keeps the repository scope // worked out at the org's last GitHub sign-in (every repository, or only those its GitHub user can access), so it -// needs no trip to GitHub; minting a token proves the App is still installed. +// needs no trip to GitHub; minting a token proves the App is still installed. Any member of the org may connect +// (the route's middleware checks membership); disconnecting stays owner-only. func (s *Server) GitHubAppConnect(c echo.Context) error { orgID, ok := auth.GetOrgIDFromContext(c) if !ok { return c.JSON(http.StatusUnauthorized, map[string]string{"error": "Org context not found"}) } - if pc := auth.GetPermissionContext(c); pc == nil || pc.RequireOrgOwner() != nil { - return c.JSON(http.StatusForbidden, map[string]string{"error": "Only an organization owner can connect a GitHub installation"}) - } connectorID, err := strconv.ParseInt(c.Param("connector_id"), 10, 64) if err != nil || connectorID <= 0 { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid connector id"}) @@ -318,28 +328,33 @@ func (s *Server) GitHubAppConnect(c echo.Context) error { // --------------------------------------------------------------------------------------------------------- // start -// GitHubAppStart begins the GitHub round trip: a signed state (mirrored in a cookie) then a redirect to the -// install page, or to the OAuth authorize page for users who already installed the App. +// GitHubAppStart begins the /gh-login round trip ("Continue with GitHub"): a signed state (mirrored in a cookie) then +// a redirect to the OAuth authorize page. It always signs in as the GitHub user's email, so it never installs: +// installing from inside the app goes through GitHubAppConnectStart, which keeps the user's session. func (s *Server) GitHubAppStart(c echo.Context) error { - // target_id (install mode only) is the numeric GitHub id of the account or organization to install on, so - // GitHub opens straight at that account instead of listing them; anything that is not a positive number is ignored. - var targetID int64 - if id, err := strconv.ParseInt(c.QueryParam("target_id"), 10, 64); err == nil && id > 0 { - targetID = id - } - mode := sanitizeGHMode(c.QueryParam("mode")) - // Only /gh-login starts in sign-in mode; the in-app install buttons store their own return page. - return s.redirectToGitHub(c, sanitizeGHSrc(c.QueryParam("src")), mode, targetID, mode == ghModeSignin) + return s.redirectToGitHub(c, sanitizeGHSrc(c.QueryParam("src")), ghModeSignin, 0, true) } +// redirectToGitHub sends an unbound (/gh-login) round trip to GitHub. func (s *Server) redirectToGitHub(c echo.Context, src, mode string, targetID int64, getStarted bool) error { - nonce, err := newGHNonce() + target, err := s.githubAppStartURL(c, ghState{Src: src, Mode: mode, GetStarted: getStarted}, targetID) if err != nil { return s.githubAppErrorPage(c, http.StatusInternalServerError, "Could not start sign-in. Please try again.") } - signed, err := signGHState(ghState{Nonce: nonce, Src: src, Mode: mode, Exp: time.Now().Add(ghStateTTL).Unix(), GetStarted: getStarted}, s.githubAppSecret) + return c.Redirect(http.StatusFound, target) +} + +// githubAppStartURL signs st (adding the nonce and expiry), mirrors the nonce in the state cookie and returns the +// GitHub page to send the browser to: the install page (on targetID when set), or the authorize page in sign-in mode. +func (s *Server) githubAppStartURL(c echo.Context, st ghState, targetID int64) (string, error) { + nonce, err := newGHNonce() if err != nil { - return s.githubAppErrorPage(c, http.StatusInternalServerError, "Could not start sign-in. Please try again.") + return "", err + } + st.Nonce, st.Exp = nonce, time.Now().Add(ghStateTTL).Unix() + signed, err := signGHState(st, s.githubAppSecret) + if err != nil { + return "", err } // Lax, not Strict: the return trip from GitHub is a cross-site top-level GET, which Strict would drop. @@ -349,14 +364,61 @@ func (s *Server) redirectToGitHub(c echo.Context, src, mode string, targetID int MaxAge: int(ghStateTTL.Seconds()), HttpOnly: true, Secure: c.Scheme() == "https", SameSite: http.SameSiteLaxMode, }) - target := s.githubApp.InstallURL(signed) - if mode == ghModeInstall && targetID > 0 { - target = s.githubApp.InstallURLForTarget(signed, targetID) + switch { + case st.Mode == ghModeSignin: + return s.githubApp.AuthorizeURL(signed), nil + case targetID > 0: + return s.githubApp.InstallURLForTarget(signed, targetID), nil + default: + return s.githubApp.InstallURL(signed), nil + } +} + +type githubAppConnectStartRequest struct { + // "install" (default): GitHub's install page, to install the App or pick repositories. "discover": GitHub's + // authorize page, which connects or offers every account the GitHub user can already see the App on. + Mode string `json:"mode"` + Src string `json:"src"` + // TargetID (install only) is the numeric GitHub id of the account or organization to install on, so GitHub opens + // straight at it instead of listing them; anything that is not a positive number is ignored. + TargetID int64 `json:"target_id"` + // Return is the page to come back to: "git" (default), "get-started" or "review/". + Return string `json:"return"` +} + +// GitHubAppConnectStart begins a GitHub round trip for the signed-in user's current org and returns the GitHub URL +// to open (a navigation can't carry the session token, so the UI asks for the URL first). The state names the org +// and user, so the callback connects there and leaves the session alone: the GitHub account only proves which +// installations it can access, whatever its email. +func (s *Server) GitHubAppConnectStart(c echo.Context) error { + orgID, ok := auth.GetOrgIDFromContext(c) + user := auth.GetUser(c) + if !ok || user == nil { + return c.JSON(http.StatusUnauthorized, map[string]string{"error": "Org context not found"}) } - if mode == ghModeSignin { - target = s.githubApp.AuthorizeURL(signed) + var req githubAppConnectStartRequest + if err := c.Bind(&req); err != nil { + return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request"}) } - return c.Redirect(http.StatusFound, target) + mode, targetID := ghModeInstall, req.TargetID + switch req.Mode { + case "", ghModeInstall: + case "discover": + mode, targetID = ghModeSignin, 0 + default: + return c.JSON(http.StatusBadRequest, map[string]string{"error": "mode must be install or discover"}) + } + if targetID < 0 { + targetID = 0 + } + target, err := s.githubAppStartURL(c, ghState{ + Src: sanitizeGHSrc(req.Src), Mode: mode, OrgID: orgID, UserID: user.ID, Return: sanitizeGHReturn(req.Return), + }, targetID) + if err != nil { + log.Printf("[GitHubApp] connect start (org %d): %v", orgID, err) + return c.JSON(http.StatusInternalServerError, map[string]string{"error": "Could not start connecting to GitHub. Please try again."}) + } + return c.JSON(http.StatusOK, map[string]string{"url": target}) } // --------------------------------------------------------------------------------------------------------- @@ -371,15 +433,20 @@ func pickPrimaryVerifiedEmail(emails []networkgithub.UserEmail) string { return "" } -// GitHubAppCallback finishes the round trip: verify the state, prove who the GitHub user is, prove they own the -// installation, sign them in through Hexmos (cloud), connect the installation to their org, and hand the -// browser to the normal Cloud login with the Hexmos JWT. +// GitHubAppCallback finishes the round trip: verify the state, prove who the GitHub user is and which installations +// they can access, then either connect them to the signed-in org that started the trip (bound: see +// finishBoundGitHubAppCallback), or sign the GitHub user in through Hexmos (cloud), connect the installation to +// their org, and hand the browser to the normal Cloud login with the Hexmos JWT (/gh-login). func (s *Server) GitHubAppCallback(c echo.Context) error { cfg := s.githubApp ctx := c.Request().Context() if c.QueryParam("setup_action") == "request" { - // A member asked an owner to approve the install; there is no installation (and no code) yet. + // A member asked an owner to approve the install; there is no installation (and no code) yet. The state is + // only read to pick the page to go back to; nothing is connected here. + if st, err := verifyGHState(c.QueryParam("state"), s.githubAppSecret, time.Now()); err == nil && st.OrgID != 0 { + return s.githubAppBoundErrorPage(c, st, http.StatusOK, "Installation requested. An owner of the GitHub organization needs to approve it on GitHub; you'll get an email when they do. Then come back here and choose Find my GitHub accounts.") + } return s.githubAppErrorPage(c, http.StatusOK, "Installation requested. An owner of the GitHub organization needs to approve it on GitHub; you'll get an email when they do. Then start again from the sign-in page.") } @@ -391,35 +458,32 @@ func (s *Server) GitHubAppCallback(c echo.Context) error { if err != nil || cookieErr != nil || !hmac.Equal([]byte(cookie.Value), []byte(state.Nonce)) { return s.githubAppErrorPage(c, http.StatusBadRequest, "This sign-in link expired or was opened in a different browser. Please start again.") } + // From here on a bound round trip's errors link back to the app page it started on. + fail := func(status int, message string) error { + if state.OrgID != 0 { + return s.githubAppBoundErrorPage(c, state, status, message) + } + return s.githubAppErrorPage(c, status, message) + } code := c.QueryParam("code") if code == "" { - return s.githubAppErrorPage(c, http.StatusBadRequest, "GitHub did not return an authorization code. Please start again.") + return fail(http.StatusBadRequest, "GitHub did not return an authorization code. Please start again.") } // 2. the GitHub user. userTok, err := networkgithub.ExchangeUserCode(ctx, cfg.BaseURL, cfg.ClientID, cfg.ClientSecret, code) if err != nil { log.Printf("[GitHubApp] code exchange failed: %v", err) - return s.githubAppErrorPage(c, http.StatusBadRequest, "GitHub rejected the sign-in code. Please start again.") + return fail(http.StatusBadRequest, "GitHub rejected the sign-in code. Please start again.") } userToken := userTok.AccessToken - emails, err := networkgithub.ListUserEmails(ctx, cfg.APIBaseURL(), userToken) - if err != nil { - log.Printf("[GitHubApp] list emails failed: %v", err) - return s.githubAppErrorPage(c, http.StatusBadGateway, "Could not read your GitHub email address. Please try again.") - } - email := pickPrimaryVerifiedEmail(emails) - if email == "" { - return s.githubAppErrorPage(c, http.StatusBadRequest, "Your GitHub account needs a verified primary email address to sign in.") - } - // 3. the installation must belong to this GitHub user; otherwise anyone could claim someone else's by editing the URL. installs, err := networkgithub.ListUserInstallations(ctx, cfg.APIBaseURL(), userToken) if err != nil { log.Printf("[GitHubApp] list installations failed: %v", err) - return s.githubAppErrorPage(c, http.StatusBadGateway, "Could not read your GitHub installations. Please try again.") + return fail(http.StatusBadGateway, "Could not read your GitHub installations. Please try again.") } var targets []networkgithub.Installation explicit := c.QueryParam("installation_id") != "" @@ -433,8 +497,23 @@ func (s *Server) GitHubAppCallback(c echo.Context) error { targets = append(targets, inst) } if explicit && len(targets) == 0 { - return s.githubAppErrorPage(c, http.StatusForbidden, "That installation doesn't belong to your GitHub account.") + return fail(http.StatusForbidden, "That installation doesn't belong to your GitHub account.") } + if state.OrgID != 0 { + return s.finishBoundGitHubAppCallback(c, state, targets, explicit, userToken) + } + + // The rest signs in as the GitHub user (/gh-login), so it needs their verified email. + emails, err := networkgithub.ListUserEmails(ctx, cfg.APIBaseURL(), userToken) + if err != nil { + log.Printf("[GitHubApp] list emails failed: %v", err) + return s.githubAppErrorPage(c, http.StatusBadGateway, "Could not read your GitHub email address. Please try again.") + } + email := pickPrimaryVerifiedEmail(emails) + if email == "" { + return s.githubAppErrorPage(c, http.StatusBadRequest, "Your GitHub account needs a verified primary email address to sign in.") + } + // Signing in with GitHub (no installation_id) is allowed with nothing installed yet: the account is created // below, then the user is sent straight on to GitHub's install page (no LiveReview session yet, so there is no // login screen flash in between). The callback that follows the install signs them in. @@ -559,6 +638,89 @@ func (s *Server) GitHubAppCallback(c echo.Context) error { return c.Redirect(http.StatusFound, "/?data="+url.QueryEscape(string(data))) } +// finishBoundGitHubAppCallback ends a round trip started inside the app (GitHubAppConnectStart). GitHub only proves +// which installations this GitHub user can access; the org and the user come from the signed state, so the browser +// keeps its session whatever email the GitHub account uses, and no Hexmos account, LiveReview user or org is +// created. Each installation is connected exactly as on a GitHub sign-in (see connectGitHubAppInstallation): explicit +// (just installed or configured) connects it; otherwise one another org already uses is only offered (Connect). +func (s *Server) finishBoundGitHubAppCallback(c echo.Context, state *ghState, targets []networkgithub.Installation, explicit bool, userToken string) error { + ctx := c.Request().Context() + fail := func(status int, message string) error { return s.githubAppBoundErrorPage(c, state, status, message) } + + // The session may have changed during the round trip (removed from the org, deactivated): check again. + hasAccess, isSuperAdmin, err := storageusers.NewUserStore(s.db).ActiveOrgAccess(ctx, state.UserID, state.OrgID) + if err != nil { + log.Printf("[GitHubApp] bound callback: access check for user %d in org %d failed: %v", state.UserID, state.OrgID, err) + return fail(http.StatusInternalServerError, "Could not check your LiveReview access. Please try again.") + } + if !hasAccess { + return fail(http.StatusForbidden, "You no longer have access to this LiveReview organization, so nothing was connected.") + } + if len(targets) == 0 && state.Mode != ghModeSignin { + return fail(http.StatusBadRequest, "The LiveReview GitHub App isn't installed on that GitHub account yet. Please try again and choose Install.") + } + + connected, failed := 0, 0 + for _, inst := range targets { + connectorID, created, resync, err := s.connectGitHubAppInstallation(ctx, state.OrgID, inst, userToken, explicit) + if errors.Is(err, storagegithub.ErrAppConnectorDisconnected) || errors.Is(err, storagegithub.ErrAppConnectorOffered) { + continue // recorded as available: Connect on the GitHub App page connects it + } + if err != nil { + log.Printf("[GitHubApp] bound callback: connecting installation %d to org %d failed: %v", inst.ID, state.OrgID, err) + if explicit { + return fail(http.StatusBadGateway, "Could not connect your GitHub installation. Please try again.") + } + failed++ + continue + } + connected++ + s.syncGitHubAppConnectorAsync(connectorID, created || resync) + s.auditBoundGitHubAppConnect(ctx, state, isSuperAdmin, connectorID, created, inst) + } + if connected == 0 && failed > 0 { + return fail(http.StatusBadGateway, "Could not connect your GitHub installation. Please try again.") + } + + // Open the org the trip started in, even if another tab switched orgs meanwhile (read and cleared by index.tsx). + // nosemgrep: go.lang.security.audit.net.cookie-missing-secure.cookie-missing-secure, go.lang.security.audit.net.cookie-missing-httponly.cookie-missing-httponly -- the UI must read it from JS (index.tsx); holds only an org id; Secure whenever HTTPS + c.SetCookie(&http.Cookie{ + Name: ghOrgCookieName, Value: strconv.FormatInt(state.OrgID, 10), Path: "/", + MaxAge: 300, Secure: c.Scheme() == "https", SameSite: http.SameSiteLaxMode, + }) + return c.Redirect(http.StatusFound, "/"+ghReturnHash(state.Return)) +} + +// auditBoundGitHubAppConnect records a bound connection under the user who started it. It is best effort: the +// connection is already made, and a missing audit row must not strand the user on an error page. +func (s *Server) auditBoundGitHubAppConnect(ctx context.Context, state *ghState, isSuperAdmin bool, connectorID int64, created bool, inst networkgithub.Installation) { + if s.auditRecorder == nil { + return + } + action := auditlog.ActionConnectorUpdate + if created { + action = auditlog.ActionConnectorCreate + } + userID := state.UserID + ev := auditlog.Event{ + OrgID: state.OrgID, + ActorUserID: &userID, + ActorKind: auditlog.ActorKindMember, + ActorCapacity: auditlog.ActorCapacityMember, + AuthMethod: "github_app", + Action: action, + ResourceType: "connector", + ResourceID: strconv.FormatInt(connectorID, 10), + Details: auditlog.Details("provider", "github_app", "installation_id", inst.ID, "account_login", inst.AccountLogin), + } + if isSuperAdmin { + ev.ActorKind, ev.ActorCapacity = auditlog.ActorKindSuperAdmin, auditlog.ActorCapacityAdmin + } + if err := s.auditRecorder.Record(ctx, ev); err != nil { + log.Printf("[GitHubApp] bound callback: audit for connector %d failed: %v", connectorID, err) + } +} + // ghSrcLaunchTrial is the /gh-login?src= value of the Product Hunt launch CTA. New orgs created through it may // run reviews on the LiveReview model while on the Free plan (within its monthly LOC limit). const ghSrcLaunchTrial = "launch-trial" @@ -1210,14 +1372,24 @@ a.btn{display:inline-block;background:var(--accent);color:#fff;text-decoration:n a.alt{margin-left:14px;color:var(--accent)}

LiveReview

{{.Message}}

-Try againOther ways to sign in +{{if .BackURL}}Back to LiveReview{{else}}Try againOther ways to sign in{{end}}
`)) // githubAppErrorPage renders a small self-contained error page; the GitHub round trip is a browser navigation, // so a JSON error would just strand the user. func (s *Server) githubAppErrorPage(c echo.Context, status int, message string) error { + return s.renderGitHubAppErrorPage(c, status, message, "") +} + +// githubAppBoundErrorPage is githubAppErrorPage for a round trip started inside the app: the user is still signed +// in, so the button goes back to the page they started on instead of the sign-in page. +func (s *Server) githubAppBoundErrorPage(c echo.Context, state *ghState, status int, message string) error { + return s.renderGitHubAppErrorPage(c, status, message, "/"+ghReturnHash(state.Return)) +} + +func (s *Server) renderGitHubAppErrorPage(c echo.Context, status int, message, backURL string) error { var buf bytes.Buffer - if err := githubAppErrorTemplate.Execute(&buf, map[string]string{"Message": message}); err != nil { + if err := githubAppErrorTemplate.Execute(&buf, map[string]string{"Message": message, "BackURL": backURL}); err != nil { return c.String(status, message) } return c.HTMLBlob(status, buf.Bytes()) diff --git a/internal/api/github_app_auth_test.go b/internal/api/github_app_auth_test.go index 84620930..8fb4703b 100644 --- a/internal/api/github_app_auth_test.go +++ b/internal/api/github_app_auth_test.go @@ -24,6 +24,7 @@ import ( "github.com/livereview/internal/api/auth" "github.com/livereview/internal/database" "github.com/livereview/internal/githubapp" + "github.com/livereview/pkg/models" storagegithub "github.com/livereview/storage/providers/github" ) @@ -211,12 +212,13 @@ func (s *Server) validState(t *testing.T, mode string) (state, nonce string) { return state, nonce } -func TestGitHubAppStart_RedirectsToInstallOrAuthorize(t *testing.T) { +// The public start is /gh-login's: it always signs in, so an old in-app install link can no longer swap the session. +func TestGitHubAppStart_AlwaysSignsIn(t *testing.T) { f := newFakeGitHub(t) s, _ := newGitHubAppTestServer(t, f) - for mode, wantPath := range map[string]string{"": "/github-apps/lr-test/installations/new", "signin": "/login/oauth/authorize"} { - req := httptest.NewRequest(http.MethodGet, "/api/v1/auth/github-app/start?src=Home&mode="+mode, nil) + for _, mode := range []string{"", "install", "signin"} { + req := httptest.NewRequest(http.MethodGet, "/api/v1/auth/github-app/start?src=Home&target_id=42&mode="+mode, nil) rec := httptest.NewRecorder() if err := s.GitHubAppStart(echo.New().NewContext(req, rec)); err != nil { t.Fatal(err) @@ -225,11 +227,11 @@ func TestGitHubAppStart_RedirectsToInstallOrAuthorize(t *testing.T) { t.Fatalf("mode %q: status %d", mode, rec.Code) } loc, _ := url.Parse(rec.Header().Get("Location")) - if loc.Path != wantPath { - t.Errorf("mode %q: redirected to %s, want path %s", mode, loc.Path, wantPath) + if loc.Path != "/login/oauth/authorize" { + t.Errorf("mode %q: redirected to %s, want the authorize page", mode, loc.Path) } st, err := verifyGHState(loc.Query().Get("state"), s.githubAppSecret, time.Now()) - if err != nil || st.Src != "home" { + if err != nil || st.Src != "home" || st.Mode != ghModeSignin || !st.GetStarted || st.OrgID != 0 || st.UserID != 0 { t.Fatalf("mode %q: bad state in redirect: %v %+v", mode, err, st) } var cookie *http.Cookie @@ -270,9 +272,15 @@ func TestGitHubAppCallback_SetupActionRequestIsInformational(t *testing.T) { f := newFakeGitHub(t) s, _ := newGitHubAppTestServer(t, f) rec := s.doCallback(t, callbackParams{setupAction: "request"}) - if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "needs to approve") { + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "needs to approve") || !strings.Contains(rec.Body.String(), `href="/#/gh-login"`) { t.Fatalf("status %d body %q", rec.Code, rec.Body.String()) } + // Requested from inside the app: the page goes back there. + state, _ := s.boundState(t, 3, 4, ghModeInstall, ghNextGetStarted) + rec = s.doCallback(t, callbackParams{setupAction: "request", state: state}) + if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "Find my GitHub accounts") || !strings.Contains(rec.Body.String(), `href="/#/get-started/github"`) { + t.Fatalf("bound: status %d body %q", rec.Code, rec.Body.String()) + } } func TestGitHubAppCallback_RejectsUnverifiedEmailBadCodeAndForeignInstallation(t *testing.T) { @@ -285,6 +293,7 @@ func TestGitHubAppCallback_RejectsUnverifiedEmailBadCodeAndForeignInstallation(t } f.emails = `[{"email":"a@b.test","primary":true,"verified":false}]` + f.installations = f.installation(555, "mine") state, nonce = s.validState(t, ghModeInstall) rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: "555"}) if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "verified primary email") { @@ -1019,7 +1028,7 @@ func TestGitHubAppDisconnect_SharedInstallation(t *testing.T) { } } -func TestGitHubAppConnect_OwnersOnlyOrgScopedAndListedAsAvailable(t *testing.T) { +func TestGitHubAppConnect_MembersOrgScopedAndListedAsAvailable(t *testing.T) { f := newFakeGitHub(t) s, db := newGitHubAppTestServer(t, f) store := storagegithub.NewTokenStore(db) @@ -1082,14 +1091,12 @@ func TestGitHubAppConnect_OwnersOnlyOrgScopedAndListedAsAvailable(t *testing.T) recs, _ := store.ListAppConnectorsForOrg(orgB) return len(recs) == 1 } - if rec := call(orgB, false); rec.Code != http.StatusForbidden || live() { - t.Fatalf("a plain member must not connect: %d", rec.Code) - } if rec := call(orgA, true); rec.Code != http.StatusNotFound || live() { t.Fatalf("another org's owner must get 404: %d", rec.Code) } - if rec := call(orgB, true); rec.Code != http.StatusOK || !live() { - t.Fatalf("owner connect: %d %s", rec.Code, rec.Body.String()) + // Any member of the org may connect (disconnecting stays owner-only). + if rec := call(orgB, false); rec.Code != http.StatusOK || !live() { + t.Fatalf("member connect: %d %s", rec.Code, rec.Body.String()) } rec, _ := store.GetGitHubTokenByConnectorID(connB) if sc := connectorMeta(t, rec); rec.PatToken != "ghs_inst" || sc.RepoScope != storagegithub.RepoScopeUser || len(sc.AllowedRepoIDs) != 1 { @@ -1103,34 +1110,273 @@ func TestGitHubAppConnect_OwnersOnlyOrgScopedAndListedAsAvailable(t *testing.T) } } -func TestGitHubAppStart_TargetIDOpensInstallPageOnThatAccount(t *testing.T) { +// connectStart calls GitHubAppConnectStart as user userID in org orgID (0 leaves the user out of the context). +func (s *Server) connectStart(t *testing.T, orgID, userID int64, body string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/api/v1/github-app/start", strings.NewReader(body)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + rec := httptest.NewRecorder() + c := echo.New().NewContext(req, rec) + c.Set("org_id", orgID) + if userID != 0 { + c.Set(string(auth.UserContextKey), &models.User{ID: userID}) + } + if err := s.GitHubAppConnectStart(c); err != nil { + t.Fatal(err) + } + return rec +} + +func TestGitHubAppConnectStart_BindsSessionOrgAndOpensTarget(t *testing.T) { f := newFakeGitHub(t) s, _ := newGitHubAppTestServer(t, f) - start := func(query string) *url.URL { - req := httptest.NewRequest(http.MethodGet, "/api/v1/auth/github-app/start?"+query, nil) - rec := httptest.NewRecorder() - if err := s.GitHubAppStart(echo.New().NewContext(req, rec)); err != nil { - t.Fatal(err) + start := func(body string) (*url.URL, *ghState, *http.Cookie) { + rec := s.connectStart(t, 31, 47, body) + if rec.Code != http.StatusOK { + t.Fatalf("body %s: status %d %s", body, rec.Code, rec.Body.String()) } - loc, _ := url.Parse(rec.Header().Get("Location")) - return loc + var out struct { + URL string `json:"url"` + } + _ = json.Unmarshal(rec.Body.Bytes(), &out) + loc, err := url.Parse(out.URL) + if err != nil || out.URL == "" { + t.Fatalf("body %s: no url in %s", body, rec.Body.String()) + } + st, err := verifyGHState(loc.Query().Get("state"), s.githubAppSecret, time.Now()) + if err != nil { + t.Fatalf("body %s: bad state: %v", body, err) + } + var cookie *http.Cookie + for _, c := range rec.Result().Cookies() { + if c.Name == ghStateCookieName { + cookie = c + } + } + return loc, st, cookie } - loc := start("mode=install&src=x&target_id=42") + loc, st, cookie := start(`{"mode":"install","src":"git-providers","target_id":42,"return":"get-started"}`) if loc.Path != "/github-apps/lr-test/installations/new/permissions" || loc.Query().Get("target_id") != "42" { t.Errorf("install with target_id went to %s", loc) } - if _, err := verifyGHState(loc.Query().Get("state"), s.githubAppSecret, time.Now()); err != nil { - t.Errorf("state missing or bad on the targeted install URL: %v", err) + if st.OrgID != 31 || st.UserID != 47 || st.Mode != ghModeInstall || st.Return != ghNextGetStarted || st.GetStarted || st.Src != "git-providers" { + t.Errorf("state = %+v, want bound to org 31 / user 47, returning to get-started", st) + } + if cookie == nil || cookie.Value != st.Nonce || !cookie.HttpOnly || cookie.Path != "/api/v1/auth/github-app" { + t.Errorf("state cookie wrong: %+v", cookie) } - for _, bad := range []string{"abc", "-5", "0", "1.5"} { - if loc := start("mode=install&src=x&target_id=" + bad); loc.Path != "/github-apps/lr-test/installations/new" { + for _, bad := range []string{"-5", "0"} { + if loc, _, _ := start(`{"target_id":` + bad + `}`); loc.Path != "/github-apps/lr-test/installations/new" { t.Errorf("target_id=%s must be ignored, went to %s", bad, loc) } } - if loc := start("mode=signin&src=x&target_id=42"); loc.Path != "/login/oauth/authorize" { - t.Errorf("sign-in must ignore target_id, went to %s", loc) + if loc, st, _ := start(`{"mode":"discover","target_id":42,"return":"https://evil.example"}`); loc.Path != "/login/oauth/authorize" || + st.Mode != ghModeSignin || st.Return != ghNextGitProviders || st.OrgID != 31 { + t.Errorf("discover went to %s with state %+v", loc, st) + } + + if rec := s.connectStart(t, 31, 47, `{"mode":"signin"}`); rec.Code != http.StatusBadRequest { + t.Errorf("unknown mode: status %d, want 400", rec.Code) + } + if rec := s.connectStart(t, 31, 0, `{}`); rec.Code != http.StatusUnauthorized { + t.Errorf("no user: status %d, want 401", rec.Code) + } +} + +// boundState is a state from GitHubAppConnectStart: user userID in org orgID. +func (s *Server) boundState(t *testing.T, orgID, userID int64, mode, ret string) (state, nonce string) { + t.Helper() + nonce = "nonce-" + fmt.Sprint(time.Now().UnixNano()) + state, err := signGHState(ghState{Nonce: nonce, Src: "test", Mode: mode, Exp: time.Now().Add(time.Minute).Unix(), + OrgID: orgID, UserID: userID, Return: ret}, s.githubAppSecret) + if err != nil { + t.Fatal(err) + } + return state, nonce +} + +// mkGitHubAppOrg creates an org named name (removed after the test). +func mkGitHubAppOrg(t *testing.T, db *sql.DB, name string) int64 { + t.Helper() + cleanupGitHubAppTenant(t, db, name) + var id int64 + if err := db.QueryRow(`INSERT INTO orgs (name, description) VALUES ($1, 'github app bound test') RETURNING id`, name).Scan(&id); err != nil { + t.Fatal(err) + } + return id +} + +// mkGitHubAppUser creates an email-login user with role in each of orgs (removed after the test, before the orgs). +func mkGitHubAppUser(t *testing.T, db *sql.DB, email, role string, orgs ...int64) int64 { + t.Helper() + cleanupGitHubAppTenant(t, db, email) + var id int64 + if err := db.QueryRow(`INSERT INTO users (email, password_hash) VALUES ($1, 'x') RETURNING id`, email).Scan(&id); err != nil { + t.Fatal(err) + } + for _, org := range orgs { + if _, err := db.Exec(`INSERT INTO user_roles (user_id, org_id, role_id) VALUES ($1, $2, (SELECT id FROM roles WHERE name = $3))`, id, org, role); err != nil { + t.Fatal(err) + } + } + return id +} + +func cookieValue(rec *httptest.ResponseRecorder, name string) string { + for _, c := range rec.Result().Cookies() { + if c.Name == name { + return c.Value + } + } + return "" +} + +// Rows 1, 3 and 13 of the GitHub App case table: someone signed in as a@domain installs from the app with a GitHub +// account whose email is different (and here not even verified), and that email has its own LiveReview account. +func TestGitHubAppCallback_BoundInstallConnectsSessionOrgWithoutSigningIn(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + suffix := time.Now().UnixNano() + domainEmail := fmt.Sprintf("a-%d@domain.test", suffix) + gmail := fmt.Sprintf("a-%d@gmail.test", suffix) + orgX := mkGitHubAppOrg(t, db, domainEmail) + userX := mkGitHubAppUser(t, db, domainEmail, "owner", orgX) + gmailOrg := mkGitHubAppOrg(t, db, gmail) + mkGitHubAppUser(t, db, gmail, "owner", gmailOrg) + + f.emails = fmt.Sprintf(`[{"email":%q,"primary":true,"verified":false}]`, gmail) + newFakeParse(t, gmail, true) // a Hexmos sign-up would fail the callback: it must not happen + inst := suffix % 1_000_000_000_000 + f.installations = f.installation(inst, "hexmostech") + + state, nonce := s.boundState(t, orgX, userX, ghModeInstall, ghNextGitProviders) + rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: fmt.Sprint(inst), setupAction: "install"}) + if rec.Code != http.StatusFound { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + if loc := rec.Header().Get("Location"); loc != "/#/git/github-app" { + t.Fatalf("redirected to %q, want straight back to the GitHub App page with no login hand-off", loc) + } + if got := cookieValue(rec, ghOrgCookieName); got != fmt.Sprint(orgX) { + t.Errorf("org cookie %q, want the session's org %d", got, orgX) + } + if got := cookieValue(rec, ghNextCookieName); got != "" { + t.Errorf("a bound trip sets no login destination; next = %q", got) + } + store := storagegithub.NewTokenStore(db) + conn, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, orgX) + if err != nil || conn.PatToken != "ghs_inst" || connectorMeta(t, conn).Removed { + t.Fatalf("session org connector = %+v, %v; want connected", conn, err) + } + if _, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, gmailOrg); !errors.Is(err, sql.ErrNoRows) { + t.Fatalf("the GitHub email's own org must get nothing; err = %v", err) + } + var users int + _ = db.QueryRow(`SELECT COUNT(*) FROM users WHERE email IN ($1, $2)`, domainEmail, gmail).Scan(&users) + if users != 2 { + t.Fatalf("got %d users, want the 2 that existed (no new account)", users) + } +} + +// Row 2 and "Find my GitHub accounts": the installation is already used by another org. +func TestGitHubAppCallback_BoundDiscoverOffersSharedAndInstallConnectsIt(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + suffix := time.Now().UnixNano() + orgA := mkGitHubAppOrg(t, db, fmt.Sprintf("gh-bound-a-%d@example.test", suffix)) + orgB := mkGitHubAppOrg(t, db, fmt.Sprintf("gh-bound-b-%d@domain.test", suffix)) + userB := mkGitHubAppUser(t, db, fmt.Sprintf("b-%d@domain.test", suffix), "member", orgB) + f.emails = `[{"email":"b@gmail.test","primary":true,"verified":true}]` + + shared, own := suffix%1_000_000_000_000, suffix%1_000_000_000_000+1 + store := storagegithub.NewTokenStore(db) + if _, _, err := store.UpsertAppInstallationConnector(storagegithub.AppConnectorUpsert{ + OrgID: orgA, BaseURL: f.srv.URL, InstallationID: shared, AccountLogin: "hexmostech", AccountType: "Organization", + Token: "ghs_a", ExpiresAt: time.Now().Add(time.Hour), RepoScope: storagegithub.RepoScopeInstaller, + }); err != nil { + t.Fatal(err) + } + f.installations = "[" + strings.Trim(f.installation(shared, "hexmostech"), "[]") + "," + strings.Trim(f.installation(own, "bsv"), "[]") + "]" + f.instRepos = `[{"id":1,"name":"api","full_name":"hexmostech/api"},{"id":2,"name":"web","full_name":"hexmostech/web"}]` + f.userRepos = `[{"id":1}]` + + // Discover: the shared installation is only offered, the one nobody uses is connected. + state, nonce := s.boundState(t, orgB, userB, ghModeSignin, "review/9") + rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code"}) + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/#/reviews/9" { + t.Fatalf("discover: status %d location %q: %s", rec.Code, rec.Header().Get("Location"), rec.Body.String()) + } + connShared, err := store.GetAppConnectorByInstallation(f.srv.URL, shared, orgB) + if err != nil || !connectorMeta(t, connShared).Removed || connShared.PatToken != "" { + t.Fatalf("shared installation in org B = %+v, %v; want offered, not connected", connShared, err) + } + if avail, _ := store.ListNotConnectedAppConnectorsForOrg(orgB); len(avail) != 1 || avail[0].ID != connShared.ID { + t.Fatalf("org B isn't offered the shared installation: %v", avail) + } + if connOwn, err := store.GetAppConnectorByInstallation(f.srv.URL, own, orgB); err != nil || connectorMeta(t, connOwn).Removed { + t.Fatalf("unshared installation in org B = %+v, %v; want connected", connOwn, err) + } + + // Install (Configure on GitHub) connects the shared one, limited to the repositories this GitHub user can access. + state, nonce = s.boundState(t, orgB, userB, ghModeInstall, "") + rec = s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: fmt.Sprint(shared), setupAction: "update"}) + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/#/git/github-app" { + t.Fatalf("install: status %d location %q: %s", rec.Code, rec.Header().Get("Location"), rec.Body.String()) + } + connShared, _ = store.GetAppConnectorByInstallation(f.srv.URL, shared, orgB) + if sc := connectorMeta(t, connShared); sc.Removed || sc.RepoScope != storagegithub.RepoScopeUser || len(sc.AllowedRepoIDs) != 1 || sc.AllowedRepoIDs[0] != 1 { + t.Fatalf("org B connector = %+v, want connected with user scope [1]", sc) + } + if connA, _ := store.GetAppConnectorByInstallation(f.srv.URL, shared, orgA); connectorMeta(t, connA).Removed || connectorMeta(t, connA).RepoScope != storagegithub.RepoScopeInstaller { + t.Fatal("the installer org's connector changed") + } +} + +// Rows 11 and 16: the org comes from the state (not the user's other orgs), and access is checked again on return. +func TestGitHubAppCallback_BoundUsesStateOrgAndRechecksAccess(t *testing.T) { + f := newFakeGitHub(t) + s, db := newGitHubAppTestServer(t, f) + suffix := time.Now().UnixNano() + orgX := mkGitHubAppOrg(t, db, fmt.Sprintf("gh-multi-x-%d@example.test", suffix)) + orgY := mkGitHubAppOrg(t, db, fmt.Sprintf("gh-multi-y-%d@example.test", suffix)) + user := mkGitHubAppUser(t, db, fmt.Sprintf("multi-%d@domain.test", suffix), "member", orgX, orgY) + inst := suffix % 1_000_000_000_000 + f.installations = f.installation(inst, "acme") + store := storagegithub.NewTokenStore(db) + + state, nonce := s.boundState(t, orgY, user, ghModeInstall, "") + if rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: fmt.Sprint(inst)}); rec.Code != http.StatusFound || cookieValue(rec, ghOrgCookieName) != fmt.Sprint(orgY) { + t.Fatalf("status %d org cookie %q: %s", rec.Code, cookieValue(rec, ghOrgCookieName), rec.Body.String()) + } + if _, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, orgY); err != nil { + t.Fatalf("state org Y has no connector: %v", err) + } + if _, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, orgX); !errors.Is(err, sql.ErrNoRows) { + t.Fatalf("the user's other org X must get nothing; err = %v", err) + } + + // Removed from org X while away on GitHub: nothing is connected, and the error page links back into the app. + if _, err := db.Exec(`DELETE FROM user_roles WHERE user_id = $1 AND org_id = $2`, user, orgX); err != nil { + t.Fatal(err) + } + state, nonce = s.boundState(t, orgX, user, ghModeInstall, "review/5") + rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code", installation: fmt.Sprint(inst)}) + if rec.Code != http.StatusForbidden || !strings.Contains(rec.Body.String(), `href="/#/reviews/5"`) { + t.Fatalf("removed member: status %d body %s", rec.Code, rec.Body.String()) + } + if _, err := store.GetAppConnectorByInstallation(f.srv.URL, inst, orgX); !errors.Is(err, sql.ErrNoRows) { + t.Fatalf("a removed member must not connect anything; err = %v", err) + } + + // A deactivated user is refused too. + if _, err := db.Exec(`UPDATE users SET is_active = FALSE WHERE id = $1`, user); err != nil { + t.Fatal(err) + } + state, nonce = s.boundState(t, orgY, user, ghModeSignin, "") + if rec := s.doCallback(t, callbackParams{state: state, cookieNonce: nonce, code: "good-code"}); rec.Code != http.StatusForbidden { + t.Fatalf("deactivated user: status %d", rec.Code) } } diff --git a/internal/api/github_app_state.go b/internal/api/github_app_state.go index 83bb7a51..f8233764 100644 --- a/internal/api/github_app_state.go +++ b/internal/api/github_app_state.go @@ -38,6 +38,34 @@ type ghState struct { // GetStarted: the round trip began on /gh-login, so a successful sign-in lands on get-started. Only the // callback decides this (nothing is stored before leaving), so a failed attempt leaves no stale destination. GetStarted bool `json:"g,omitempty"` + // OrgID and UserID bind the round trip to a signed-in LiveReview session (POST /github-app/start): the callback + // connects to that org and keeps the session, whatever email the GitHub account uses. Zero for /gh-login. + OrgID int64 `json:"o,omitempty"` + UserID int64 `json:"u,omitempty"` + // Return is the page a bound round trip lands on, as a sanitizeGHReturn key (never a URL). + Return string `json:"r,omitempty"` +} + +var ghReturnReviewPattern = regexp.MustCompile(`^review/[1-9][0-9]{0,18}$`) + +// sanitizeGHReturn keeps only the pages a bound round trip may land on; anything else becomes the GitHub App card. +func sanitizeGHReturn(ret string) string { + if ret == ghNextGetStarted || ghReturnReviewPattern.MatchString(ret) { + return ret + } + return ghNextGitProviders +} + +// ghReturnHash is the UI route for a sanitizeGHReturn key. +func ghReturnHash(ret string) string { + switch { + case ret == ghNextGetStarted: + return "#/get-started/github" + case ghReturnReviewPattern.MatchString(ret): + return "#/reviews/" + strings.TrimPrefix(ret, "review/") + default: + return "#/git/github-app" + } } // sanitizeGHSrc keeps the campaign tag short and harmless; anything else becomes "unknown". @@ -49,13 +77,6 @@ func sanitizeGHSrc(src string) string { return "unknown" } -func sanitizeGHMode(mode string) string { - if mode == ghModeSignin { - return ghModeSignin - } - return ghModeInstall -} - func newGHNonce() (string, error) { b := make([]byte, 16) if _, err := rand.Read(b); err != nil { diff --git a/internal/api/github_app_state_test.go b/internal/api/github_app_state_test.go index c7108bac..456e64d0 100644 --- a/internal/api/github_app_state_test.go +++ b/internal/api/github_app_state_test.go @@ -54,13 +54,25 @@ func TestGHState_Rejections(t *testing.T) { } } -func TestSanitizeGHSrcAndMode(t *testing.T) { +func TestSanitizeGHSrcAndReturn(t *testing.T) { for in, want := range map[string]string{"Home": "home", "start-free_1": "start-free_1", "": "unknown", "a b": "unknown", "