Skip to content

Validate the multi-repo map file before initializing any client - #760

Merged
rdimitrov merged 3 commits into
masterfrom
rdimitrov/multirepo-map-validation
Aug 7, 2026
Merged

Validate the multi-repo map file before initializing any client#760
rdimitrov merged 3 commits into
masterfrom
rdimitrov/multirepo-map-validation

Conversation

@rdimitrov

@rdimitrov rdimitrov commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #750, addressing the review findings on that PR.

#750 correctly fixed the out-of-bounds access on repoURL[0] when a repository is declared with no URL. This follow-up relocates and extends that check.

What changes

Validation moves into a single validateRepoMap helper that runs before initTUFClients.

New() already had a pre-flight loop calling validateRepoName; #750 put the URL check in a second place, inside the initialization loop. Consolidating them means:

  • New becomes atomic. Map iteration order is randomized, so with the check inside initTUFClients an arbitrary number of repositories get EnsurePathsExist() and updater.New() run before the error surfaces — leaving cache directories on disk for a call that returns nil, err and gives the caller no handle to clean up.
  • Errors become deterministic. Repository names are now checked in sorted order, so a map file with more than one defect reports the same error on every run.

This ordering problem is not hypothetical. Before the move, a map file whose mapping was malformed failed with:

mkdir : no such file or directory

because initialization began before validation finished, masking the real defect.

Bugs fixed beyond #750

A mapping referencing an undeclared repository panicked. Mapping.repositories was never cross-checked against the top-level repositories object. An unknown name leaves no entry in TUFClients, and GetTargetInfo indexes it without an ok check, unlike DownloadTarget which does guard this. Confirmed against the pre-fix code:

PANIC CONFIRMED: runtime error: invalid memory address or nil pointer dereference

It needs both a matching path pattern and a typo'd repository name in the same mapping to trigger, which is likely why it went unnoticed.

"mapping": [null] panicked. It unmarshals into a []*Mapping holding a nil element, dereferenced while walking mappings.

A MultiRepoConfig with no RepoMap panicked. The struct has exported fields, so callers can build one directly without going through NewConfig.

Other

  • Empty-URL rejection now wraps an exported ErrMissingRepoURL sentinel, matching the existing ErrInvalidRepoName convention so callers can use errors.Is. Unknown mapping repositories get ErrUnknownMappingRepo.
  • Fixes the trailing-tab whitespace on the two blank lines Ensure repository URL exists before initialization #750 added (the repo's .golangci.yml enables no formatters, so CI does not currently catch gofmt drift).

Compatibility

A map file with a mapping pointing at an undeclared repository is now rejected by New() rather than accepted and panicking later on a matching target lookup. That is a behaviour change, but it converts a latent runtime panic into an upfront configuration error. The shipped examples/multirepo/repository/targets/map.json was verified to pass validation unchanged.

Tests

#750 shipped without tests; this adds regression coverage for every case above — empty/null/empty-string URL lists, unknown mapping repository, null mapping entry, and missing repository map. Each was written first and watched fail before the fix.

$ go test ./...        # all packages ok
$ go vet ./...         # clean
$ gofmt -l ./metadata/ # clean
$ golangci-lint run ./metadata/multirepo/...
0 issues.

🤖 Generated with Claude Code

@rdimitrov
rdimitrov force-pushed the rdimitrov/multirepo-map-validation branch from 12bc099 to 2a99deb Compare August 7, 2026 11:25
@rdimitrov
rdimitrov marked this pull request as ready for review August 7, 2026 11:26
@rdimitrov
rdimitrov requested a review from a team as a code owner August 7, 2026 11:26
Copilot AI lite review requested due to automatic review settings August 7, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates multi-repo map validation into a single validateRepoMap helper that runs before any repository initialization, preventing partial initialization side effects and converting several panic cases into upfront configuration errors.

Changes:

  • Added validateRepoMap to validate repository names, repository URLs, and mapping-referenced repositories before calling initTUFClients.
  • Introduced exported sentinel errors (ErrMissingRepoURL, ErrUnknownMappingRepo) to support errors.Is checks by callers.
  • Added regression tests covering missing/empty URLs, unknown mapping repositories, null mapping entries, and missing repo map config.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
metadata/multirepo/multirepo.go Adds centralized pre-init map validation and new sentinel errors; removes URL validation from initialization loop.
metadata/multirepo/multirepo_test.go Adds tests to ensure invalid maps/configs fail fast with the expected sentinel errors or non-nil errors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread metadata/multirepo/multirepo.go
@rdimitrov
rdimitrov force-pushed the rdimitrov/multirepo-map-validation branch from 6fc5422 to e024e5f Compare August 7, 2026 11:39
rdimitrov and others added 2 commits August 7, 2026 14:42
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>
New dereferenced config.RepoMap without checking the config pointer, so
New(nil) panicked. NewConfig returns a nil config alongside its error, so a
caller that ignores the error reaches this path.

Also move client construction after validation, so the invalid-input paths
no longer allocate a client they immediately discard.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
@rdimitrov
rdimitrov force-pushed the rdimitrov/multirepo-map-validation branch from e024e5f to 5ff859f Compare August 7, 2026 11:42

@kommendorkapten kommendorkapten left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@rdimitrov
rdimitrov enabled auto-merge (squash) August 7, 2026 12:40
@rdimitrov
rdimitrov merged commit 0b10964 into master Aug 7, 2026
26 checks passed
@rdimitrov
rdimitrov deleted the rdimitrov/multirepo-map-validation branch August 7, 2026 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants