Validate the multi-repo map file before initializing any client

Follow-up to #750. That PR fixed the out-of-bounds access on a repository
with no URL, but left the check inside initTUFClients, where it runs
interleaved with client initialization.

Move all map file validation into a single validateRepoMap helper that runs
before initTUFClients, and extend it:

- Repository names and URLs are now validated in one place, so New either
  returns a fully initialized client or fails without having created cache
  directories for an arbitrary subset of the repositories.
- Repository names are checked in sorted order, so a map file with more than
  one defect reports the same error on every run instead of depending on map
  iteration order.
- A mapping that references a repository absent from the top-level
  repositories object is now rejected. Previously it left no TUF client for
  that name and GetTargetInfo dereferenced a nil *updater.Updater, panicking
  once a target path matched the mapping.
- A null mapping entry and a config with no repository map are rejected for
  the same reason, rather than panicking.

Empty-URL and empty-slice rejection now wraps the exported ErrMissingRepoURL
sentinel, matching the existing ErrInvalidRepoName convention so callers can
use errors.Is.

Adds regression tests for each case; #750 shipped without any.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

Author: rdimitrov

metadata/multirepo/multirepo.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"maps"
 	"os"
 	"path/filepath"
 	"regexp"
@@ -35,6 +36,14 @@ import (
 // components or is otherwise invalid for use as a directory name.
 var ErrInvalidRepoName = errors.New("invalid repository name")
 
+// ErrMissingRepoURL is returned when a repository listed in the map file has no
+// usable URL, i.e. an empty URL list or an empty first URL.
+var ErrMissingRepoURL = errors.New("repository has no URL configured")
+
+// ErrUnknownMappingRepo is returned when a mapping in the map file references a
+// repository that is not declared in the top-level repositories object.
+var ErrUnknownMappingRepo = errors.New("mapping references an unknown repository")
+
 // validRepoNamePattern defines the allowed characters for repository names.
 // Names must start with an alphanumeric character and may contain alphanumeric
 // characters, dots, hyphens, and underscores. This prevents path traversal
@@ -119,11 +128,11 @@ func New(config *MultiRepoConfig) (*MultiRepoClient, error) {
 		TUFClients: map[string]*updater.Updater{},
 	}
 
-	// validate repository names before using them as filesystem paths
-	for repoName := range config.RepoMap.Repositories {
-		if err := validateRepoName(repoName); err != nil {
-			return nil, fmt.Errorf("repository %q: %w", repoName, err)
-		}
+	// validate the map file before initializing anything, so that a malformed map
+	// file is reported as such instead of surfacing later as an obscure
+	// initialization failure or a panic during target lookup
+	if err := validateRepoMap(config.RepoMap); err != nil {
+		return nil, err
 	}
 
 	// create TUF clients for each repository listed in the map file
@@ -138,13 +147,9 @@ func (client *MultiRepoClient) initTUFClients() error {
 	log := metadata.GetLogger()
 
 	// loop through each repository listed in the map file and initialize it
+	// note: the map file has already been validated by validateRepoMap, so each
+	// repository is guaranteed to have a usable URL at index 0
 	for repoName, repoURL := range client.Config.RepoMap.Repositories {
-		
-		// Make sure we have at least one repo URL
-		if len(repoURL) == 0 || repoURL[0] == "" {
-			return fmt.Errorf("repository %q has no URL configured", repoName)
-		}
-		
 		log.Info("Initializing", "name", repoName, "url", repoURL[0])
 
 		// get the trusted root file from the location specified in the map file relevant to its path
@@ -393,6 +398,50 @@ func (cfg *MultiRepoConfig) EnsurePathsExist() error {
 	return nil
 }
 
+// validateRepoMap checks that a map file is internally consistent before any
+// repository is initialized. It enforces that every repository name is safe to
+// use as a filesystem path, that every repository has a usable URL, and that
+// every repository referenced by a mapping is actually declared.
+//
+// Validating up front keeps New atomic: it either returns a fully initialized
+// client or fails without having created cache directories for a subset of the
+// repositories. It also makes the reported error deterministic, which map
+// iteration order alone would not guarantee.
+func validateRepoMap(repoMap *MultiRepoMapType) error {
+	if repoMap == nil {
+		return fmt.Errorf("no repository map provided")
+	}
+
+	// sort the repository names so that a map file with more than one problem
+	// always reports the same error rather than a random one
+	for _, repoName := range slices.Sorted(maps.Keys(repoMap.Repositories)) {
+		if err := validateRepoName(repoName); err != nil {
+			return fmt.Errorf("repository %q: %w", repoName, err)
+		}
+
+		// only the first URL is used, as the client supports a single mirror per
+		// repository for the time being
+		if repoURL := repoMap.Repositories[repoName]; len(repoURL) == 0 || repoURL[0] == "" {
+			return fmt.Errorf("repository %q: %w", repoName, ErrMissingRepoURL)
+		}
+	}
+
+	// every repository named in a mapping must have a corresponding TUF client,
+	// otherwise GetTargetInfo would dereference a nil client during target lookup
+	for i, eachMap := range repoMap.Mapping {
+		if eachMap == nil {
+			return fmt.Errorf("mapping at index %d is null", i)
+		}
+		for _, repoName := range eachMap.Repositories {
+			if _, ok := repoMap.Repositories[repoName]; !ok {
+				return fmt.Errorf("mapping at index %d: %w - %s", i, ErrUnknownMappingRepo, repoName)
+			}
+		}
+	}
+
+	return nil
+}
+
 // validateRepoName checks that a repository name is safe to use as a directory
 // component. Repository names must start with an alphanumeric character and
 // contain only alphanumeric characters, dots, hyphens, and underscores.

metadata/multirepo/multirepo_test.go

@@ -122,4 +122,109 @@ func TestNewRejectsInvalidRepoNames(t *testing.T) {
 			}
 		})
 	}
-}
+}
+
+func TestNewRejectsRepositoriesWithoutURL(t *testing.T) {
+	tests := []struct {
+		name     string
+		repoURLs string
+	}{
+		{"empty URL list", `[]`},
+		{"null URL list", `null`},
+		{"empty URL string", `[""]`},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mapJSON := []byte(`{
+				"repositories": {
+					"my-repo": ` + tt.repoURLs + `
+				},
+				"mapping": []
+			}`)
+
+			rootBytes := []byte(`{"signatures":[],"signed":{}}`)
+
+			cfg, err := NewConfig(mapJSON, map[string][]byte{"my-repo": rootBytes})
+			if err != nil {
+				t.Fatalf("NewConfig() unexpected error: %v", err)
+			}
+
+			_, err = New(cfg)
+			if err == nil {
+				t.Fatalf("New() should reject repository with URLs %s", tt.repoURLs)
+			}
+
+			if !errors.Is(err, ErrMissingRepoURL) {
+				t.Errorf("New() error should wrap ErrMissingRepoURL, got: %v", err)
+			}
+		})
+	}
+}
+
+func TestNewRejectsMappingWithUnknownRepository(t *testing.T) {
+	// A mapping that references a repository absent from the top-level
+	// "repositories" object leaves no TUF client for that name, which makes
+	// GetTargetInfo dereference a nil *updater.Updater.
+	mapJSON := []byte(`{
+		"repositories": {
+			"real-repo": ["https://example.com/repo"]
+		},
+		"mapping": [
+			{
+				"paths": ["*"],
+				"repositories": ["typo-repo"],
+				"threshold": 1,
+				"terminating": true
+			}
+		]
+	}`)
+
+	rootBytes := []byte(`{"signatures":[],"signed":{}}`)
+
+	cfg, err := NewConfig(mapJSON, map[string][]byte{"real-repo": rootBytes})
+	if err != nil {
+		t.Fatalf("NewConfig() unexpected error: %v", err)
+	}
+
+	_, err = New(cfg)
+	if err == nil {
+		t.Fatal("New() should reject a mapping referencing an unknown repository")
+	}
+
+	if !errors.Is(err, ErrUnknownMappingRepo) {
+		t.Errorf("New() error should wrap ErrUnknownMappingRepo, got: %v", err)
+	}
+}
+
+func TestNewRejectsNullMapping(t *testing.T) {
+	// "mapping": [null] unmarshals into a []*Mapping holding a nil element,
+	// which GetTargetInfo would dereference while walking the mappings.
+	mapJSON := []byte(`{
+		"repositories": {
+			"real-repo": ["https://example.com/repo"]
+		},
+		"mapping": [null]
+	}`)
+
+	rootBytes := []byte(`{"signatures":[],"signed":{}}`)
+
+	cfg, err := NewConfig(mapJSON, map[string][]byte{"real-repo": rootBytes})
+	if err != nil {
+		t.Fatalf("NewConfig() unexpected error: %v", err)
+	}
+
+	_, err = New(cfg)
+	if err == nil {
+		t.Fatal("New() should reject a null mapping entry")
+	}
+}
+
+func TestNewRejectsConfigWithoutRepoMap(t *testing.T) {
+	// MultiRepoConfig has exported fields, so callers can build one directly
+	// without going through NewConfig and leave RepoMap unset.
+	_, err := New(&MultiRepoConfig{})
+	if err == nil {
+		t.Fatal("New() should reject a config with no repository map")
+	}
+}