fix: Minor error in download-sources

Author: Antonio Salinas

internal/app/azldev/cmds/downloadsources/downloadsources.go

@@ -19,10 +19,11 @@ import (
 
 // DownloadSourcesOptions holds the options for the download-sources command.
 type DownloadSourcesOptions struct {
-	Directory         string
-	OutputDir         string
-	LookasideBaseURIs []string
-	PackageName       string
+	Directory           string
+	OutputDir           string
+	LookasideBaseURIs   []string
+	PackageName         string
+	LookasideDownloader fedorasource.FedoraSourceDownloader
 }
 
 // OnAppInit registers the download-sources command as a subcommand of the given parent.
@@ -102,28 +103,30 @@ func DownloadSources(env *azldev.Env, options *DownloadSourcesOptions) error {
 	event := env.StartEvent("Downloading sources", "packageName", packageName)
 	defer event.End()
 
-	// Build retry config from environment.
-	retryCfg := retry.DefaultConfig()
-	if env.NetworkRetries() > 0 {
-		retryCfg.MaxAttempts = env.NetworkRetries()
+	lookasideDownloader := options.LookasideDownloader
+	if lookasideDownloader == nil {
+		lookasideDownloader, err = createLookasideDownloader(env)
+		if err != nil {
+			return err
+		}
 	}
 
-	// Create the HTTP downloader and lookaside source downloader.
-	httpDownloader, err := downloader.NewHTTPDownloader(env, env, env.FS())
-	if err != nil {
-		return fmt.Errorf("failed to create HTTP downloader:\n%w", err)
+	// Build extract options.
+	var extractOpts []fedorasource.ExtractOption
+	if options.OutputDir != "" {
+		extractOpts = append(extractOpts, fedorasource.WithOutputDir(options.OutputDir))
 	}
 
-	lookasideDownloader, err := fedorasource.NewFedoraRepoExtractorImpl(
-		env, env.FS(), httpDownloader, retryCfg,
-	)
+	// Verify the 'sources' file exists before attempting downloads.
+	sourcesPath := filepath.Join(options.Directory, "sources")
+
+	sourcesExists, err := fileutils.Exists(env.FS(), sourcesPath)
 	if err != nil {
-		return fmt.Errorf("failed to create lookaside downloader:\n%w", err)
+		return fmt.Errorf("failed to check for sources file at %#q:\n%w", sourcesPath, err)
 	}
 
-	downloadDir, err := prepareDownloadDir(env, options)
-	if err != nil {
-		return err
+	if !sourcesExists {
+		return fmt.Errorf("no 'sources' file found in %#q", options.Directory)
 	}
 
 	// Try each lookaside base URI until one succeeds.
@@ -133,7 +136,7 @@ func DownloadSources(env *azldev.Env, options *DownloadSourcesOptions) error {
 		slog.Info("Trying lookaside base URI", "uri", uri)
 
 		downloadErr = lookasideDownloader.ExtractSourcesFromRepo(
-			env, downloadDir, packageName, uri, nil,
+			env, options.Directory, packageName, uri, nil, extractOpts...,
 		)
 		if downloadErr == nil {
 			break
@@ -147,54 +150,38 @@ func DownloadSources(env *azldev.Env, options *DownloadSourcesOptions) error {
 		return fmt.Errorf("failed to download sources from any lookaside URI:\n%w", downloadErr)
 	}
 
-	slog.Info("Sources downloaded successfully", "outputDir", downloadDir)
+	outputDir := options.Directory
+	if options.OutputDir != "" {
+		outputDir = options.OutputDir
+	}
+
+	absOutputDir, _ := filepath.Abs(outputDir)
+	slog.Info("Sources downloaded successfully", "outputDir", absOutputDir)
 
 	return nil
 }
 
-// prepareDownloadDir resolves the download directory, verifies the 'sources' file exists,
-// and copies it to the output directory if needed.
-func prepareDownloadDir(env *azldev.Env, options *DownloadSourcesOptions) (string, error) {
-	sourceDir, err := filepath.Abs(options.Directory)
-	if err != nil {
-		return "", fmt.Errorf("failed to resolve absolute path for source directory %#q:\n%w", options.Directory, err)
+// createLookasideDownloader builds the default [fedorasource.FedoraSourceDownloader]
+// from the environment's network and filesystem configuration.
+func createLookasideDownloader(env *azldev.Env) (fedorasource.FedoraSourceDownloader, error) {
+	retryCfg := retry.DefaultConfig()
+	if env.NetworkRetries() > 0 {
+		retryCfg.MaxAttempts = env.NetworkRetries()
 	}
 
-	// Verify the 'sources' file exists before attempting downloads.
-	sourcesFilePath := filepath.Join(sourceDir, "sources")
-
-	sourcesExists, err := fileutils.Exists(env.FS(), sourcesFilePath)
+	httpDownloader, err := downloader.NewHTTPDownloader(env, env, env.FS())
 	if err != nil {
-		return "", fmt.Errorf("failed to check for sources file at %#q:\n%w", sourcesFilePath, err)
+		return nil, fmt.Errorf("failed to create HTTP downloader:\n%w", err)
 	}
 
-	if !sourcesExists {
-		return "", fmt.Errorf("no 'sources' file found in %#q", sourceDir)
-	}
-
-	downloadDir := sourceDir
-
-	if options.OutputDir != "" {
-		downloadDir, err = filepath.Abs(options.OutputDir)
-		if err != nil {
-			return "", fmt.Errorf("failed to resolve absolute path for output directory %#q:\n%w", options.OutputDir, err)
-		}
-	}
-
-	// If downloading to a different directory, copy the 'sources' file there.
-	if downloadDir != sourceDir {
-		dstPath := filepath.Join(downloadDir, "sources")
-
-		if err := fileutils.MkdirAll(env.FS(), downloadDir); err != nil {
-			return "", fmt.Errorf("failed to create output directory %#q:\n%w", downloadDir, err)
-		}
-
-		if err := fileutils.CopyFile(env, env.FS(), sourcesFilePath, dstPath, fileutils.CopyFileOptions{}); err != nil {
-			return "", fmt.Errorf("failed to copy sources file to output directory:\n%w", err)
-		}
+	lookasideDownloader, err := fedorasource.NewFedoraRepoExtractorImpl(
+		env, env.FS(), httpDownloader, retryCfg,
+	)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create lookaside downloader:\n%w", err)
 	}
 
-	return downloadDir, nil
+	return lookasideDownloader, nil
 }
 
 // resolveDownloadParams determines the package name and lookaside URIs.

internal/app/azldev/cmds/downloadsources/downloadsources_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/microsoft/azure-linux-dev-tools/internal/app/azldev/core/testutils"
 	"github.com/microsoft/azure-linux-dev-tools/internal/global/opctx/opctx_test"
 	"github.com/microsoft/azure-linux-dev-tools/internal/projectconfig"
+	"github.com/microsoft/azure-linux-dev-tools/internal/providers/sourceproviders/fedorasource/fedorasource_test"
 	"github.com/microsoft/azure-linux-dev-tools/internal/utils/fileperms"
 	"github.com/microsoft/azure-linux-dev-tools/internal/utils/fileutils"
 	"github.com/spf13/cobra"
@@ -58,18 +59,22 @@ func TestNewDownloadSourcesCmd(t *testing.T) {
 
 func TestResolveFromDirectory_DerivesPackageName(t *testing.T) {
 	testEnv := testutils.NewTestEnv(t)
+	ctrl := gomock.NewController(t)
 
-	// Create a directory named after the package with a sources file.
 	pkgDir := "/project/curl"
 	require.NoError(t, fileutils.MkdirAll(testEnv.TestFS, pkgDir))
 	require.NoError(t, fileutils.WriteFile(testEnv.TestFS, pkgDir+"/sources", []byte(""), fileperms.PrivateFile))
 
+	mockDownloader := fedorasource_test.NewMockFedoraSourceDownloader(ctrl)
+	mockDownloader.EXPECT().
+		ExtractSourcesFromRepo(gomock.Any(), pkgDir, "curl", gomock.Any(), gomock.Any()).
+		Return(nil)
+
 	options := &downloadsources.DownloadSourcesOptions{
-		Directory: pkgDir,
+		Directory:           pkgDir,
+		LookasideDownloader: mockDownloader,
 	}
 
-	// With a valid directory, valid distro config, and an empty sources file,
-	// the command should succeed and derive the package name from the directory.
 	err := downloadsources.DownloadSources(testEnv.Env, options)
 	require.NoError(t, err)
 }
@@ -104,6 +109,7 @@ func TestDownloadSources_NoSourcesFile(t *testing.T) {
 
 func TestResolveLookasideURI_FollowsUpstreamDistro(t *testing.T) {
 	testEnv := testutils.NewTestEnv(t)
+	ctrl := gomock.NewController(t)
 
 	// Reconfigure: default distro has NO lookaside URI, but points to an upstream that does.
 	testEnv.Config.Distros["test-distro"] = projectconfig.DistroDefinition{
@@ -121,23 +127,29 @@ func TestResolveLookasideURI_FollowsUpstreamDistro(t *testing.T) {
 		},
 	}
 
+	expectedURI := "https://upstream.example.com/lookaside/$pkg/$filename/$hashtype/$hash/$filename"
+
 	testEnv.Config.Distros["upstream-distro"] = projectconfig.DistroDefinition{
-		LookasideBaseURI: "https://upstream.example.com/lookaside/$pkg/$filename/$hashtype/$hash/$filename",
+		LookasideBaseURI: expectedURI,
 		Versions: map[string]projectconfig.DistroVersionDefinition{
 			"42": {},
 		},
 	}
 
-	specDir := "/project/testpkg"
-	require.NoError(t, fileutils.MkdirAll(testEnv.TestFS, specDir))
-	require.NoError(t, fileutils.WriteFile(testEnv.TestFS, specDir+"/sources", []byte(""), fileperms.PrivateFile))
+	pkgDir := "/project/testpkg"
+	require.NoError(t, fileutils.MkdirAll(testEnv.TestFS, pkgDir))
+	require.NoError(t, fileutils.WriteFile(testEnv.TestFS, pkgDir+"/sources", []byte(""), fileperms.PrivateFile))
+
+	mockDownloader := fedorasource_test.NewMockFedoraSourceDownloader(ctrl)
+	mockDownloader.EXPECT().
+		ExtractSourcesFromRepo(gomock.Any(), pkgDir, "testpkg", expectedURI, gomock.Any()).
+		Return(nil)
 
 	options := &downloadsources.DownloadSourcesOptions{
-		Directory: specDir,
+		Directory:           pkgDir,
+		LookasideDownloader: mockDownloader,
 	}
 
-	// With the upstream distro providing the lookaside URI and an empty sources file,
-	// the command should succeed.
 	err := downloadsources.DownloadSources(testEnv.Env, options)
 	require.NoError(t, err)
 }

internal/providers/sourceproviders/fedorasource/fedorasource.go

@@ -27,12 +27,31 @@ type FedoraSourceDownloader interface {
 	// ExtractSourcesFromRepo processes a git repository by downloading any required
 	// lookaside cache files into the repository directory. Files whose names appear
 	// in skipFilenames are not downloaded (e.g., files already fetched separately).
+	// Optional [ExtractOption] values can override default behavior.
 	ExtractSourcesFromRepo(
 		ctx context.Context, repoDir string, packageName string,
 		lookasideBaseURI string, skipFilenames []string,
+		opts ...ExtractOption,
 	) error
 }
 
+// extractOptions holds optional configuration for [ExtractSourcesFromRepo].
+type extractOptions struct {
+	outputDir string
+}
+
+// ExtractOption is a functional option for [ExtractSourcesFromRepo].
+type ExtractOption func(*extractOptions)
+
+// WithOutputDir specifies a separate directory for downloaded files.
+// When set, the sources file is read from repoDir but files are
+// downloaded into outputDir instead.
+func WithOutputDir(dir string) ExtractOption {
+	return func(o *extractOptions) {
+		o.outputDir = dir
+	}
+}
+
 // FedoraSourceDownloaderImpl is an implementation of GitRepoExtractor.
 type FedoraSourceDownloaderImpl struct {
 	dryRunnable opctx.DryRunnable
@@ -123,6 +142,7 @@ func NewFedoraRepoExtractorImpl(
 // lookaside cache files into the repository directory.
 func (g *FedoraSourceDownloaderImpl) ExtractSourcesFromRepo(
 	ctx context.Context, repoDir string, packageName string, lookasideBaseURI string, skipFileNames []string,
+	opts ...ExtractOption,
 ) error {
 	if repoDir == "" {
 		return errors.New("repository directory cannot be empty")
@@ -132,6 +152,12 @@ func (g *FedoraSourceDownloaderImpl) ExtractSourcesFromRepo(
 		return errors.New("lookaside base URI cannot be empty")
 	}
 
+	// Apply functional options.
+	var options extractOptions
+	for _, opt := range opts {
+		opt(&options)
+	}
+
 	repoDirExists, err := fileutils.Exists(g.fileSystem, repoDir)
 	if err != nil {
 		return fmt.Errorf("failed to check if repository directory exists at %#q:\n%w", repoDir, err)
@@ -168,7 +194,13 @@ func (g *FedoraSourceDownloaderImpl) ExtractSourcesFromRepo(
 		skipSet[name] = true
 	}
 
-	err = g.downloadAndVerifySources(ctx, sourceFiles, repoDir, skipSet)
+	// Determine where to write downloaded files.
+	destDir := repoDir
+	if options.outputDir != "" {
+		destDir = options.outputDir
+	}
+
+	err = g.downloadAndVerifySources(ctx, sourceFiles, destDir, skipSet)
 	if err != nil {
 		return fmt.Errorf("failed to download sources:\n%w", err)
 	}
@@ -271,6 +303,11 @@ func parseSourcesFile(content string, packageName string, lookasideBaseURI strin
 			return nil, fmt.Errorf("failed to build lookaside URL for file %#q:\n%w", entry.Filename, err)
 		}
 
+		// Validate the filename to prevent path traversal attacks from crafted sources entries.
+		if err := fileutils.ValidateFilename(entry.Filename); err != nil {
+			return nil, fmt.Errorf("unsafe filename in sources file %#q:\n%w", entry.Filename, err)
+		}
+
 		sourceFiles = append(sourceFiles, sourceFileInfo{
 			fileName:     entry.Filename,
 			uri:          sourceURI,

internal/providers/sourceproviders/fedorasource/fedorasource_test.go

@@ -268,6 +268,24 @@ func TestParseSourcesFile(t *testing.T) {
 		require.NoError(t, err)
 		assert.Empty(t, sources)
 	})
+
+	t.Run("path traversal filename is rejected", func(t *testing.T) {
+		content := "SHA512 (../../etc/passwd) = abc123\n"
+
+		_, err := parseSourcesFile(content, "pkg", "https://example.com/$hashtype/$hash/$pkg/$filename")
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unsafe filename")
+	})
+
+	t.Run("absolute path filename is rejected", func(t *testing.T) {
+		content := "SHA512 (/etc/passwd) = abc123\n"
+
+		_, err := parseSourcesFile(content, "pkg", "https://example.com/$hashtype/$hash/$pkg/$filename")
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "unsafe filename")
+	})
 }
 
 func TestBuildLookasideURL(t *testing.T) {

internal/providers/sourceproviders/fedorasource/fedorasource_test/fedorasource_mocks.go

@@ -18,6 +18,7 @@ import (
 	"github.com/microsoft/azure-linux-dev-tools/internal/projectconfig"
 	"github.com/microsoft/azure-linux-dev-tools/internal/providers/rpmprovider/rpmprovider_test"
 	"github.com/microsoft/azure-linux-dev-tools/internal/providers/sourceproviders"
+	"github.com/microsoft/azure-linux-dev-tools/internal/providers/sourceproviders/fedorasource"
 	"github.com/microsoft/azure-linux-dev-tools/internal/rpm/rpm_test"
 	"github.com/microsoft/azure-linux-dev-tools/internal/utils/fileperms"
 	"github.com/microsoft/azure-linux-dev-tools/internal/utils/fileutils"
@@ -291,7 +292,7 @@ func newNoOpDownloader() *noOpDownloader {
 type noOpDownloader struct{}
 
 func (d *noOpDownloader) ExtractSourcesFromRepo(
-	_ context.Context, _, _, _ string, _ []string,
+	_ context.Context, _, _, _ string, _ []string, _ ...fedorasource.ExtractOption,
 ) error {
 	return nil
 }