fix(repository): validate configured remotes before cache access (BUG-001)

config/config.go

@@ -16,8 +16,12 @@ package config
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
+	neturl "net/url"
 	"os"
+	"path/filepath"
+	"strings"
 
 	yaml "github.com/goccy/go-yaml"
 )
@@ -96,8 +100,8 @@ func ParseBytes(yamlBytes []byte) (*Config, error) {
 	config.repositoryByRemote = make(map[string]*RepositoryConfig, len(config.Repository))
 	for i := range config.Repository {
 		remote := config.Repository[i].Remote
-		if remote == "" {
-			return nil, fmt.Errorf("repository[%d].remote must not be empty", i)
+		if err := validateRepositoryRemote(remote); err != nil {
+			return nil, fmt.Errorf("repository[%d].remote: %w", i, err)
 		}
 		if _, exists := config.repositoryByRemote[remote]; exists {
 			return nil, fmt.Errorf("duplicate repository remote %q", remote)
@@ -113,3 +117,87 @@ func ParseBytes(yamlBytes []byte) (*Config, error) {
 	}
 	return &config, nil
 }
+
+func validateRepositoryRemote(remote string) error {
+	safeRemote := redactRepositoryPassword(remote)
+	if remote == "" {
+		return fmt.Errorf("remote %q must not be empty", safeRemote)
+	}
+	if strings.TrimSpace(remote) != remote {
+		return fmt.Errorf("remote %q must not contain leading or trailing whitespace", safeRemote)
+	}
+	if strings.ContainsAny(remote, "\x00\r\n") {
+		return fmt.Errorf("remote %q must not contain control characters", safeRemote)
+	}
+	if !strings.Contains(remote, "://") && !strings.HasPrefix(strings.ToLower(remote), "file:") {
+		if filepath.IsAbs(remote) || isSCPLikeRemote(remote) {
+			return nil
+		}
+		return fmt.Errorf("relative local remote %q is not allowed", safeRemote)
+	}
+
+	parsed, err := neturl.Parse(remote)
+	if err != nil {
+		var urlError *neturl.Error
+		if errors.As(err, &urlError) {
+			err = urlError.Err
+		}
+		return fmt.Errorf("invalid repository URL %q: %v", safeRemote, err)
+	}
+	safeRemote = parsed.Redacted()
+	scheme := strings.ToLower(parsed.Scheme)
+	if parsed.User != nil {
+		_, hasPassword := parsed.User.Password()
+		if hasPassword {
+			return fmt.Errorf("repository URL %q must not contain a password", safeRemote)
+		}
+		if scheme == "http" || scheme == "https" {
+			return fmt.Errorf("repository URL %q must not contain HTTP(S) userinfo", safeRemote)
+		}
+	}
+	filePath := parsed.Path
+	if filePath == "" {
+		filePath = parsed.Opaque
+	}
+	if scheme == "file" && !filepath.IsAbs(filePath) {
+		return fmt.Errorf("file repository URL %q must contain an absolute path", safeRemote)
+	}
+	if parsed.RawQuery != "" {
+		return fmt.Errorf("repository URL %q must not contain query %q", safeRemote, parsed.RawQuery)
+	}
+	if parsed.Fragment != "" {
+		return fmt.Errorf("repository URL %q must not contain fragment %q", safeRemote, parsed.Fragment)
+	}
+	return nil
+}
+
+func redactRepositoryPassword(remote string) string {
+	schemeEnd := strings.Index(remote, "://")
+	if schemeEnd < 0 {
+		return remote
+	}
+	authorityStart := schemeEnd + len("://")
+	authorityEnd := len(remote)
+	if offset := strings.IndexAny(remote[authorityStart:], "/?#"); offset >= 0 {
+		authorityEnd = authorityStart + offset
+	}
+	authority := remote[authorityStart:authorityEnd]
+	userinfoEnd := strings.LastIndexByte(authority, '@')
+	if userinfoEnd < 0 {
+		return remote
+	}
+	passwordStart := strings.IndexByte(authority[:userinfoEnd], ':')
+	if passwordStart < 0 {
+		return remote
+	}
+	passwordStart += authorityStart
+	userinfoEnd += authorityStart
+	return remote[:passwordStart+1] + "xxxxx" + remote[userinfoEnd:]
+}
+
+func isSCPLikeRemote(remote string) bool {
+	colon := strings.IndexByte(remote, ':')
+	return colon > 0 &&
+		colon < len(remote)-1 &&
+		!strings.ContainsAny(remote[:colon], `/\`)
+}

config/config_test.go

@@ -15,6 +15,7 @@
 package config
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -208,6 +209,73 @@ service:
 	require.Error(t, err)
 }
 
+func TestParseBytes_InvalidRemoteRejected(t *testing.T) {
+	tests := []struct {
+		name          string
+		remote        string
+		wantError     string
+		forbidInError string
+	}{
+		{name: "leading whitespace", remote: " https://example.com/repo.git"},
+		{name: "trailing whitespace", remote: "https://example.com/repo.git "},
+		{name: "newline", remote: "https://example.com/repo.git\nother"},
+		{name: "http userinfo", remote: "https://token@example.com/repo.git", wantError: "token"},
+		{name: "password", remote: "ssh://git:secret@example.com/org/repo.git", wantError: "example.com/org/repo.git", forbidInError: "secret"},
+		{name: "malformed URL with password", remote: "https://git:secret%zz@example.com/repo.git", wantError: "example.com/repo.git", forbidInError: "secret"},
+		{name: "query", remote: "https://example.com/repo.git?token=secret", wantError: "token=secret"},
+		{name: "fragment", remote: "https://example.com/repo.git#main", wantError: "main"},
+		{name: "relative local path", remote: "../../outside", wantError: "../../outside"},
+		{name: "relative file URL", remote: "file:../outside", wantError: "../outside"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			yamlStr := fmt.Sprintf(`
+repository:
+  - remote: %q
+service:
+  max_worker_pool_size: 1
+  workspaces_root_path: "/tmp/tango-repo-manager"
+`, tt.remote)
+			_, err := ParseBytes([]byte(yamlStr))
+			require.Error(t, err)
+			if tt.wantError != "" {
+				assert.Contains(t, err.Error(), tt.wantError)
+			}
+			if tt.forbidInError != "" {
+				assert.NotContains(t, err.Error(), tt.forbidInError)
+			}
+		})
+	}
+}
+
+func TestParseBytes_ExactRemoteValuesRemainDistinct(t *testing.T) {
+	yamlStr := `
+repository:
+  - remote: "git@github.com:org/repo.git"
+  - remote: "ssh://git@github.com:22/org/repo"
+service:
+  max_worker_pool_size: 1
+  workspaces_root_path: "/tmp/tango-repo-manager"
+`
+	cfg, err := ParseBytes([]byte(yamlStr))
+	require.NoError(t, err)
+	assert.Len(t, cfg.Repository, 2)
+}
+
+func TestParseBytes_AbsoluteLocalRemoteAccepted(t *testing.T) {
+	yamlStr := `
+repository:
+  - remote: "/var/lib/tango/repo"
+  - remote: "file:///var/lib/tango/other-repo"
+service:
+  max_worker_pool_size: 1
+  workspaces_root_path: "/tmp/tango-repo-manager"
+`
+	cfg, err := ParseBytes([]byte(yamlStr))
+	require.NoError(t, err)
+	assert.Len(t, cfg.Repository, 2)
+}
+
 func TestParseBytes_DuplicateRemoteRejected(t *testing.T) {
 	yamlStr := `
 repository:

config/repository_config.go

@@ -27,7 +27,9 @@ type RepositoryConfig struct {
 	// Remote is the URL used to `git clone` the repository. Tango clones from
 	// this URL and uses it as the lookup key for per-repo settings. Must be
 	// unique across all entries and match exactly what clients send in
-	// BuildDescription.remote.
+	// BuildDescription.remote. Local paths must be absolute. HTTP(S)
+	// credentials, URL queries, fragments, surrounding whitespace, and control
+	// characters are rejected.
 	Remote string `yaml:"remote"`
 	// TODO: FullHashRepos, ExcludedFiles, and StreamBazelLogs are not
 	// documented in config/README.md. Delete them if they turn out to be

controller/controller.go

@@ -20,23 +20,29 @@ import (
 
 	"github.com/uber-go/tally"
 	"github.com/uber/tango/config"
+	tangoerrors "github.com/uber/tango/core/errors"
 	"github.com/uber/tango/core/storage"
+	"github.com/uber/tango/internal/url"
 	"github.com/uber/tango/observability/metrics"
 	"github.com/uber/tango/orchestrator"
 	pb "github.com/uber/tango/tangopb"
 	"go.uber.org/fx"
 	"go.uber.org/zap"
 )
 
+const unknownRepositoryMetricLabel = "unknown"
+
 // Params are the parameters for the controller.
 type Params struct {
 	fx.In
 	Logger          *zap.Logger
 	Storage         storage.Storage
 	Orchestrator    orchestrator.Orchestrator
-	Scope           tally.Scope                     `optional:"true"`
-	MaxMessageBytes int                             `optional:"true"`
-	RepoConfig      config.RepositoryConfigProvider `optional:"true"`
+	Scope           tally.Scope `optional:"true"`
+	MaxMessageBytes int         `optional:"true"`
+	// RepoConfig, when set, is the authoritative repository allowlist. RPC
+	// remotes must match it exactly before the controller performs cache I/O.
+	RepoConfig config.RepositoryConfigProvider `optional:"true"`
 	// GraphFormat mirrors ServiceConfig.GraphFormat; empty defaults to gob.
 	// It must match the orchestrator's configured format — both are wired
 	// from the same ServiceConfig.
@@ -47,6 +53,22 @@ type Params struct {
 	ShadowCompare bool `optional:"true"`
 }
 
+// resolveRepository verifies that a request remote exactly matches service
+// configuration before the controller performs cache I/O. A nil provider is
+// supported for custom orchestrators that do not use Tango repository config.
+func (c *controller) resolveRepository(remote string) (string, error) {
+	if c.repoConfig == nil {
+		return url.RepositoryMetricLabel(remote), nil
+	}
+	repo, ok := c.repoConfig.GetRepositoryConfig(remote)
+	if !ok {
+		return unknownRepositoryMetricLabel, tangoerrors.NewUser(
+			errors.New("repository is not configured"),
+		)
+	}
+	return url.RepositoryMetricLabel(repo.Remote), nil
+}
+
 type controller struct {
 	logger          *zap.Logger
 	storage         storage.Storage

controller/getchangedtargets.go

@@ -32,7 +32,6 @@ import (
 	"github.com/uber/tango/internal/targetdiff"
 	"github.com/uber/tango/internal/tgb"
 	"github.com/uber/tango/internal/tgbdiff"
-	"github.com/uber/tango/internal/url"
 	"github.com/uber/tango/observability/metrics"
 	pb "github.com/uber/tango/tangopb"
 	"go.uber.org/zap"
@@ -82,12 +81,16 @@ type job struct {
 // client disconnects, the stream's context is cancelled and the function
 // returns with context.Canceled.
 func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) {
-	repo := url.ToShortRemote(request.GetFirstRevision().GetRemote())
+	validationErr := validateGetChangedTargetsRequest(request)
+	repo := unknownRepositoryMetricLabel
+	var repositoryErr error
+	if validationErr == nil {
+		repo, repositoryErr = c.resolveRepository(request.GetFirstRevision().GetRemote())
+	}
 	e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo})
 	op := metrics.Begin(e, opGetChangedTargets, metrics.SlowDurationBuckets)
 	logger := c.logger.WithLazy(
-		zap.Any("first_revision", request.GetFirstRevision()),
-		zap.Any("second_revision", request.GetSecondRevision()),
+		zap.String("repository", repo),
 	)
 	defer func() {
 		op.Complete(retErr)
@@ -96,8 +99,11 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str
 			retErr = toWireError(retErr)
 		}
 	}()
-	if err := validateGetChangedTargetsRequest(request); err != nil {
-		return tangoerrors.NewUser(err)
+	if validationErr != nil {
+		return tangoerrors.NewUser(validationErr)
+	}
+	if repositoryErr != nil {
+		return repositoryErr
 	}
 	ctx, cancelLink := c.linkRequestCtx(stream.Context())
 	defer cancelLink()

controller/getchangedtargets_test.go

@@ -309,6 +309,38 @@ func TestGetChangedTargets_ValidationError(t *testing.T) {
 	require.Error(t, err)
 }
 
+func TestGetChangedTargets_UnconfiguredRemoteRejectedBeforeCacheLookup(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl)
+	store := storagemock.NewMockStorage(ctrl)
+	orch := orchestratormock.NewMockOrchestrator(ctrl)
+	configuredRemote := "git@github.com:org/repo.git"
+
+	c := NewController(context.Background(), Params{
+		Logger:       zaptest.NewLogger(t),
+		Storage:      store,
+		Orchestrator: orch,
+		RepoConfig: fakeRepoConfigProvider{
+			configuredRemote: {Remote: configuredRemote},
+		},
+	})
+	request := &pb.GetChangedTargetsRequest{
+		FirstRevision: &pb.BuildDescription{
+			Strategy: pb.COMPUTATION_STRATEGY_UNSET,
+			Remote:   "ssh://git@github.com:22/org/repo",
+			BaseSha:  "sha1",
+		},
+		SecondRevision: &pb.BuildDescription{
+			Strategy: pb.COMPUTATION_STRATEGY_UNSET,
+			Remote:   "ssh://git@github.com:22/org/repo",
+			BaseSha:  "sha2",
+		},
+	}
+
+	err := c.GetChangedTargets(request, stream)
+	require.Error(t, err)
+}
+
 func TestGetChangedTargets_CacheHit(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl)

controller/gettargetgraph.go

@@ -26,7 +26,6 @@ import (
 	tangoerrors "github.com/uber/tango/core/errors"
 	"github.com/uber/tango/entity"
 	"github.com/uber/tango/internal/mapper"
-	"github.com/uber/tango/internal/url"
 	"github.com/uber/tango/observability/metrics"
 
 	"github.com/uber/tango/core/storage"
@@ -36,11 +35,16 @@ import (
 
 // GetTargetGraph returns the target graph for a given request.
 func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) {
-	repo := url.ToShortRemote(request.GetBuildDescription().GetRemote())
+	entityReq, mappingErr := mapper.ProtoToGetTargetGraphRequest(request)
+	repo := unknownRepositoryMetricLabel
+	var repositoryErr error
+	if mappingErr == nil {
+		repo, repositoryErr = c.resolveRepository(entityReq.Build.Remote)
+	}
 	e := c.emitter.Tagged(map[string]string{metrics.TagRepo: repo})
 	op := metrics.Begin(e, opGetTargetGraph, metrics.SlowDurationBuckets)
 	logger := c.logger.WithLazy(
-		zap.Any("build_description", request.GetBuildDescription()),
+		zap.String("repository", repo),
 	)
 	defer func() {
 		op.Complete(retErr)
@@ -52,9 +56,11 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb
 	start := time.Now()
 	ctx, cancelLink := c.linkRequestCtx(stream.Context())
 	defer cancelLink()
-	entityReq, err := mapper.ProtoToGetTargetGraphRequest(request)
-	if err != nil {
-		return tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", err))
+	if mappingErr != nil {
+		return tangoerrors.NewUser(fmt.Errorf("convert get target graph request: %w", mappingErr))
+	}
+	if repositoryErr != nil {
+		return repositoryErr
 	}
 	graphReader, err := c.getGraph(ctx, e, entityReq)
 	if err != nil {