fix(changeprovider): Reject GitHub change URIs that name another host before calling the API

service/submitqueue/demo/provider/github/profiles.yaml

@@ -25,7 +25,7 @@ defaults:
 queues:
   - name: demo-queue
     # Reads pull request metadata from GitHub's GraphQL API. For GitHub
-    # Enterprise, add `github: {baseUrl: https://ghe.example.com/api/v3}`.
+    # Enterprise, add `github: {baseUrl: https://ghe.example.com/api/v3, host: ghe.example.com}`.
     changeProvider: {type: github}
 
     # Serializes batches that touch a shared directory, so changes land in

service/submitqueue/orchestrator/server/config.go

@@ -205,6 +205,8 @@ type githubProviderConfig struct {
 	TokenEnv string `yaml:"tokenEnv"`
 	// BaseURL is the GitHub API root, for GitHub Enterprise.
 	BaseURL string `yaml:"baseUrl"`
+	// Host is the GitHub instance change URIs must name.
+	Host string `yaml:"host"`
 	// Timeout bounds each API call, as a Go duration ("30s").
 	Timeout string `yaml:"timeout"`
 }
@@ -612,6 +614,9 @@ func (c *changeProviderConfig) ensureGitHub() {
 	if c.GitHub.BaseURL == "" {
 		c.GitHub.BaseURL = defaultGitHubBaseURL
 	}
+	if c.GitHub.Host == "" {
+		c.GitHub.Host = defaultGitHubHost
+	}
 }
 
 func (c *changeProviderConfig) ensurePhabricator(where string) error {

service/submitqueue/orchestrator/server/profiles.go

@@ -498,9 +498,7 @@ func (b *profileBuilder) newChangeProviderFactory(cfg changeProviderConfig, wher
 		if makeProvider == nil {
 			return nil, fmt.Errorf("%s: github change provider needs %s to be set", where, cfg.GitHub.TokenEnv)
 		}
-		return changeProviderFunc(func(c changeprovider.Config) (changeprovider.ChangeProvider, error) {
-			return makeProvider(c), nil
-		}), nil
+		return changeProviderFunc(makeProvider), nil
 
 	case changeProviderTypePhabricator:
 		makeProvider, err := b.newPhabChangeProvider(*cfg.Phabricator, where)
@@ -529,8 +527,9 @@ func (b *profileBuilder) newChangeProviderFactory(cfg changeProviderConfig, wher
 // unusable partway through a migration.
 func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfig, where string) (changeprovider.Factory, error) {
 	var (
-		makeGitHub, makePhab func(changeprovider.Config) changeprovider.ChangeProvider
-		err                  error
+		makeGitHub func(changeprovider.Config) (changeprovider.ChangeProvider, error)
+		makePhab   func(changeprovider.Config) changeprovider.ChangeProvider
+		err        error
 	)
 	if cfg.GitHub != nil {
 		if makeGitHub, err = b.newGitHubChangeProvider(*cfg.GitHub, where); err != nil {
@@ -552,9 +551,14 @@ func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfi
 	}
 
 	return changeProviderFunc(func(c changeprovider.Config) (changeprovider.ChangeProvider, error) {
-		var gh, phab changeprovider.ChangeProvider
+		var (
+			gh, phab changeprovider.ChangeProvider
+			err      error
+		)
 		if makeGitHub != nil {
-			gh = makeGitHub(c)
+			if gh, err = makeGitHub(c); err != nil {
+				return nil, fmt.Errorf("%s: %w", where, err)
+			}
 		}
 		if makePhab != nil {
 			phab = makePhab(c)
@@ -570,7 +574,7 @@ func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfi
 // newGitHubChangeProvider returns a function building the GitHub change provider
 // for a given queue, or nil when its token is unset. The HTTP client and metrics
 // scope are built once here; only the per-queue wrapper is built per call.
-func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where string) (func(changeprovider.Config) changeprovider.ChangeProvider, error) {
+func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where string) (func(changeprovider.Config) (changeprovider.ChangeProvider, error), error) {
 	token, ok := tokenFrom(cfg.TokenEnv)
 	if !ok {
 		return nil, nil
@@ -580,10 +584,11 @@ func (b *profileBuilder) newGitHubChangeProvider(cfg githubProviderConfig, where
 		return nil, err
 	}
 	scope := b.scope.SubScope("changeprovider.github")
-	return func(c changeprovider.Config) changeprovider.ChangeProvider {
+	return func(c changeprovider.Config) (changeprovider.ChangeProvider, error) {
 		return githubprovider.NewProvider(githubprovider.Params{
 			Config:       c,
 			HTTPClient:   client,
+			Host:         cfg.Host,
 			Logger:       b.logger.Sugar(),
 			MetricsScope: scope,
 		})

submitqueue/extension/changeprovider/github/provider.go

@@ -2,6 +2,7 @@ package github
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -23,6 +24,9 @@ type Params struct {
 	// HTTPClient is a pre-configured HTTP client. The caller is responsible for
 	// configuring the base URL (via BaseURLTransport) and auth (via a transport layer).
 	HTTPClient *http.Client
+	// Host is the GitHub instance change URIs must name (e.g. "github.com"):
+	// the one HTTPClient talks to.
+	Host string
 	// Logger is the structured logger.
 	Logger *zap.SugaredLogger
 	// MetricsScope is the metrics scope for instrumentation.
@@ -34,18 +38,24 @@ type provider struct {
 	// cfg is the per-queue identity this provider was built for.
 	cfg          changeprovider.Config
 	httpClient   *http.Client
+	host         string
 	logger       *zap.SugaredLogger
 	metricsScope tally.Scope
 }
 
-// NewProvider creates a new GitHub ChangeProvider.
-func NewProvider(params Params) changeprovider.ChangeProvider {
+// NewProvider creates a new GitHub ChangeProvider. It errors when Host is
+// missing.
+func NewProvider(params Params) (changeprovider.ChangeProvider, error) {
+	if params.Host == "" {
+		return nil, errors.New("github change provider: Host is required")
+	}
 	return &provider{
 		cfg:          params.Config,
 		httpClient:   params.HTTPClient,
+		host:         params.Host,
 		logger:       params.Logger.Named("github_changeprovider"),
 		metricsScope: params.MetricsScope.SubScope("github_changeprovider"),
-	}
+	}, nil
 }
 
 // Get retrieves change information from GitHub for the request's change.
@@ -71,8 +81,9 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity.
 		"uris", change.URIs,
 	)
 
-	// Validate stacked changes are consistent (same provider, org, and repo)
-	if err := validateChangeConsistency(changeIDs); err != nil {
+	// Checked before any API call: HTTPClient would read a PR on another host
+	// from this one.
+	if err := validateChangeConsistency(changeIDs, p.host); err != nil {
 		return nil, err
 	}
 

submitqueue/extension/changeprovider/github/provider_test.go

@@ -34,11 +34,41 @@ func newTestProvider(t *testing.T, serverURL string) changeprovider.ChangeProvid
 	t.Helper()
 	client, err := phttp.NewClient(serverURL)
 	require.NoError(t, err)
-	return NewProvider(Params{
+	p, err := NewProvider(Params{
 		HTTPClient:   client,
+		Host:         "github.example.com",
 		Logger:       zaptest.NewLogger(t).Sugar(),
 		MetricsScope: tally.NoopScope,
 	})
+	require.NoError(t, err)
+	return p
+}
+
+func TestNewProvider(t *testing.T) {
+	tests := []struct {
+		name    string
+		host    string
+		wantErr bool
+	}{
+		{name: "host set", host: "github.com"},
+		{name: "host missing", wantErr: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := NewProvider(Params{
+				HTTPClient:   http.DefaultClient,
+				Host:         tt.host,
+				Logger:       zaptest.NewLogger(t).Sugar(),
+				MetricsScope: tally.NoopScope,
+			})
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+		})
+	}
 }
 
 func servePR(t *testing.T, w http.ResponseWriter, data pullRequestData) {
@@ -88,6 +118,14 @@ func TestProvider_Get(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "PR on another host returns error without calling GitHub",
+			handler: func(w http.ResponseWriter, r *http.Request) {
+				t.Errorf("unexpected GitHub request %s %s", r.Method, r.URL.Path)
+			},
+			uris:    []string{"github://github.com/uber/submitqueue/pull/123/" + shaA},
+			wantErr: true,
+		},
 		{
 			name: "stale PR returns error",
 			handler: func(w http.ResponseWriter, r *http.Request) {

submitqueue/extension/changeprovider/github/validate.go

@@ -7,15 +7,21 @@ import (
 )
 
 // validateChangeConsistency validates that all changeIDs in the stack are consistent.
-// Stacked changes must have the same change provider (scheme), org, and repo.
+// Stacked changes must name host and have the same change provider (scheme), org,
+// and repo.
 func validateChangeConsistency(
 	changeIDs []entitygithub.ChangeID,
+	host string,
 ) error {
 	expectedScheme := changeIDs[0].Scheme
 	expectedOrg := changeIDs[0].Org
 	expectedRepo := changeIDs[0].Repo
 
 	for _, cid := range changeIDs {
+		if cid.Host != host {
+			return fmt.Errorf("PR #%d names host %q, not %q", cid.PRNumber, cid.Host, host)
+		}
+
 		// Validate same change provider (scheme)
 		if cid.Scheme != expectedScheme {
 			return fmt.Errorf("stacked changes must use same change provider: expected %s, got %s for PR #%d",

submitqueue/extension/changeprovider/github/validate_test.go

@@ -16,46 +16,68 @@ func TestValidateChangeConsistency(t *testing.T) {
 		{
 			name: "single PR",
 			changeIDs: []entitygithub.ChangeID{
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
 			},
 		},
 		{
 			name: "consistent stack",
 			changeIDs: []entitygithub.ChangeID{
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1},
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 2},
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 3},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 2},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 3},
 			},
 		},
+		{
+			name: "PR on another host",
+			changeIDs: []entitygithub.ChangeID{
+				{Scheme: "github", Host: "github.example.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+			},
+			wantErr: true,
+		},
+		{
+			name: "stack spanning hosts",
+			changeIDs: []entitygithub.ChangeID{
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+				{Scheme: "github", Host: "github.example.com", Org: "uber", Repo: "submitqueue", PRNumber: 2},
+			},
+			wantErr: true,
+		},
+		{
+			name: "host with a port it does not name",
+			changeIDs: []entitygithub.ChangeID{
+				{Scheme: "github", Host: "github.com:8443", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+			},
+			wantErr: true,
+		},
 		{
 			name: "different repo",
 			changeIDs: []entitygithub.ChangeID{
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1},
-				{Scheme: "github", Org: "uber", Repo: "other-repo", PRNumber: 2},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "other-repo", PRNumber: 2},
 			},
 			wantErr: true,
 		},
 		{
 			name: "different org",
 			changeIDs: []entitygithub.ChangeID{
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1},
-				{Scheme: "github", Org: "other-org", Repo: "submitqueue", PRNumber: 2},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+				{Scheme: "github", Host: "github.com", Org: "other-org", Repo: "submitqueue", PRNumber: 2},
 			},
 			wantErr: true,
 		},
 		{
 			name: "different scheme",
 			changeIDs: []entitygithub.ChangeID{
-				{Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 1},
-				{Scheme: "other", Org: "uber", Repo: "submitqueue", PRNumber: 2},
+				{Scheme: "github", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 1},
+				{Scheme: "other", Host: "github.com", Org: "uber", Repo: "submitqueue", PRNumber: 2},
 			},
 			wantErr: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := validateChangeConsistency(tt.changeIDs)
+			err := validateChangeConsistency(tt.changeIDs, "github.com")
 			if tt.wantErr {
 				require.Error(t, err)
 			} else {