Repository: ossf/package-analysis (language: python)

Issue:
Save analyzed package
Closes https://github.com/ossf/package-analysis/issues/227

Same as #734, but conflicts are resolved.

This PR:
- adds `getArchiveURL` method for `crates.io`, `Packagist` and `RubyGems`
- changes the filename for archives downloaded from `crates.io` and `Packagist`, since their filenames don't contain info about the packages
- appends hash of file to the end of the filename
- uploads those analyzed packages

Examples:
```
chalk-5.2.0.tgz-9687d5d17a063e1b2aceaae5b83f8765eaeaf1c4bae8d74a21c1e9aeba1e4862
pipx-1.2.0.tar.gz-d1908041d24d525cafebeb177efb686133d719499cb55c54f596c95add579286
rand-0.8.5.tar.gz-34af8d1a0e25924bc5b7c43c079c942339d8f0a8b57c39049bef581b46327404
ruby-macho-3.0.0.gem-a5b971f72ad0d9015afe5a49c46cc048a2ff581b2e56bf2fada9379af19fe069
```



Discussion / hints:
Thank you for the PR! We're taking a look now
There are two larger issues which we might need to think about more, and may not solve in this PR:

1. When dynamic analysis and static analysis are run together, it seems like the package will be downloaded twice and possibly saved twice to the bucket. Is there any way you can think of to avoid this?
2. [Definitely out of scope for this PR] The downloaded archive should be the same one actually used for analysis, instead of it being downloaded separately after the analysis ends. This means we have to download it first, then copy it into the sandbox(es), and then run the analysis, and then upload it to the bucket. We don't have to do that in this PR, but when we merge this PR, I will create an issue for it. You are most welcome to help with that issue too if you like! It would be much appreciated :) 
Thanks for your reviews!

> When dynamic analysis and static analysis are run together, it seems like the package will be downloaded twice and possibly saved twice to the bucket. Is there any way you can think of to avoid this?

This is solved by introducing a new variable to indicate whether the package was saved already.

> We don't have to do that in this PR, but when we merge this PR, I will create an issue for it. You are most welcome to help with that issue too if you like! It would be much appreciated :)

I will be glad to work on this! :D
Nice work, almost there!

Please address the comment above, and then there is one more change I'd like to ask of you: 

https://github.com/ossf/package-analysis/pull/763 introduced a way to globally toggle features on and off. Could you please add an entry to internal/featureflags/featureflags.go for saving the analyzed package (disabled by default), and then add code to prevent saving if the feature is disabled? You can check that PR to see how it was done for the file writes case (in internal/strace/strace.go and internal/worker/save_data.go)
Hi @dukecat0, I recognise that there are a lot of things being merged to main at the moment and it could be annoying to have to keep up and fix merges, etc.

If you would like me to finish the remaining steps of this PR and merge it, I'm very happy to do so. But no worries if you would like to do it yourself too! Just let me know :) 
Hi @maxfisher-g, thanks! Please feel free to finish the remaining steps of this PR! :D 
It should only consist of a few lines of change to the code.

Sure, no worries :) 
Hi @maxfisher-g, I have some time now and can work on this again if you don't have time at the moment :)
> Hi @maxfisher-g, I have some time now and can work on this again if you don't have time at the moment :)

Sure, if you would like to it's all yours :) There's no rush on this btw, I was just offering in case you were tired of merging all the conflicts haha
Some new changes I've made:
- Moved `analyzedPackageSaved` to `ResultStores`
- Reset  `analyzedPackageSaved` to false in `handleMessage`
- Pass resultStores as a pointer to several functions
- Add an entry `AnalyzedPackage` to `featureflags`
- Remove the check for `p.archiveURL` since this method is now implemented for every ecosystem.
@dukecat0 thank you so much, this is looking great! It's very close to being ready for merging :) 
Oops, I clicked the approve button a bit early, but once the nits are resolved I think we're ready to merge! :D 
Should be ok now! :D
Btw, I think commands in `test/e2e/README.md` should be updated since some of them don't work.
Wonderful, thank you! Let's merge it!

> Btw, I think commands in `test/e2e/README.md` should be updated since some of them don't work.

Oh, which ones? Could you make an issue please?
Great work on this PR @dukecat0 :tada: 
Thanks for your review!
> Oh, which ones? Could you make an issue please?

I think it should be easy to fix, maybe I will just open a PR for it. :D
> > Oh, which ones? Could you make an issue please?
> 
> I think it should be easy to fix, maybe I will just open a PR for it. :D

Sounds good! Thanks!

Fix patch:
diff --git a/cmd/downloader/main.go b/cmd/downloader/main.go
index c0134415..28603fcb 100644
--- a/cmd/downloader/main.go
+++ b/cmd/downloader/main.go
@@ -34,16 +34,15 @@ func newCmdError(message string) error {
 	return &cmdError{message}
 }
 
-func downloadPackage(purl packageurl.PackageURL, downloadDir string) error {
+func downloadPackage(purl packageurl.PackageURL, dir string) error {
 	pkg, err := worker.ResolvePurl(purl)
-
 	if err != nil {
 		return err
 	}
 
 	fmt.Printf("[%s] %s@%s\n", pkg.EcosystemName(), pkg.Name(), pkg.Version())
 
-	if _, err := pkg.Manager().DownloadArchive(pkg.Name(), pkg.Version(), downloadDir); err != nil {
+	if _, err := pkg.Manager().DownloadArchive(pkg.Name(), pkg.Version(), dir); err != nil {
 		return err
 	}
 
diff --git a/internal/pkgmanager/download.go b/internal/pkgmanager/download.go
index 255cea85..181cd156 100644
--- a/internal/pkgmanager/download.go
+++ b/internal/pkgmanager/download.go
@@ -5,69 +5,44 @@ import (
 	"io"
 	"net/http"
 	"os"
-	"path/filepath"
-	"strings"
-
-	"github.com/ossf/package-analysis/internal/utils"
 )
 
 /*
-downloadToDirectory downloads a file from the given URL to the given directory.
-On successful download, the full path to the downloaded file is returned.
-
-fileName argument is required with no default value, and the hash of the file
-is appended to the given filename. If an error occurs during hashing, then the
-original filename is used.
-*/
-func downloadToDirectory(dir string, url string, fileName string) (string, error) {
-	filePath := filepath.Join(dir, fileName)
+downloadToPath creates (and/or truncates) a file at the given path, then writes
+contents of whatever is at the given URL to that given file using downloadToFile,
+and finally closes the file.
 
-	if err := downloadToPath(filePath, url); err != nil {
-		return "", err
-	}
+If any error occurs, the created file is removed.
 
-	hash, err := utils.HashFile(filePath)
-	if err != nil {
-		// Error is ignored and the path with the original filename is returned
-		// We treat hashing as 'best-effort' rather than a strictly necessary part of the function
-		return filePath, nil
+Callers must ensure that path and url are nonempty, otherwise the function will panic.
+*/
+func downloadToPath(path, url string) error {
+	if path == "" {
+		panic("path is empty")
 	}
-
-	hashedFilePath := strings.Join([]string{filePath, hash[7:]}, "-")
-
-	err = os.Rename(filePath, hashedFilePath)
-	if err != nil {
-		// See above comment
-		return filePath, nil
+	if url == "" {
+		panic("url is empty")
 	}
 
-	return hashedFilePath, nil
-}
-
-/*
-downloadToPath creates (and/or truncates) a file at the given path, then writes
-contents of whatever is at the given URL to that given file using downloadToFile,
-and finally closes the file.
-*/
-func downloadToPath(path string, url string) (err error) {
 	file, err := os.Create(path)
 	if err != nil {
 		return err
 	}
 
-	defer func() {
-		closeErr := file.Close()
-		if err == nil {
-			err = closeErr
-		} else if closeErr != nil {
-			err = fmt.Errorf("%w; error closing file %s: %v", err, path, closeErr)
+	if downloadErr := downloadToFile(file, url); downloadErr != nil {
+		// cleanup file
+		if removeErr := os.Remove(path); removeErr != nil {
+			return fmt.Errorf("%w\n%v", downloadErr, removeErr)
 		}
-	}()
-
-	err = downloadToFile(file, url)
+		return downloadErr
+	}
 
-	if err != nil {
-		return err
+	if closeErr := file.Close(); closeErr != nil {
+		// cleanup file
+		if removeErr := os.Remove(path); removeErr != nil {
+			return fmt.Errorf("%w\n%v", closeErr, removeErr)
+		}
+		return closeErr
 	}
 
 	return nil
@@ -77,8 +52,14 @@ func downloadToPath(path string, url string) (err error) {
 downloadToFile writes the contents of whatever is at the given URL to the
 given file, without opening or closing the file. If any errors occur while
 making the network request, then no file operations will be performed.
+
+Callers must ensure that url is nonempty, otherwise the function will panic.
 */
 func downloadToFile(dest *os.File, url string) error {
+	if url == "" {
+		panic("url is empty")
+	}
+
 	resp, err := http.Get(url)
 	if err != nil {
 		return err
@@ -90,8 +71,7 @@ func downloadToFile(dest *os.File, url string) error {
 		return fmt.Errorf("http status %s", resp.Status)
 	}
 
-	_, err = io.Copy(dest, resp.Body)
-	if err != nil {
+	if _, err := io.Copy(dest, resp.Body); err != nil {
 		return err
 	}
 
diff --git a/internal/pkgmanager/ecosystem.go b/internal/pkgmanager/ecosystem.go
index 4d110c3d..4ac09941 100644
--- a/internal/pkgmanager/ecosystem.go
+++ b/internal/pkgmanager/ecosystem.go
@@ -3,6 +3,7 @@ package pkgmanager
 import (
 	"fmt"
 	"path"
+	"path/filepath"
 	"strings"
 
 	"github.com/ossf/package-analysis/pkg/api/pkgecosystem"
@@ -11,10 +12,10 @@ import (
 // PkgManager represents how packages from a common ecosystem are accessed.
 type PkgManager struct {
 	ecosystem       pkgecosystem.Ecosystem
-	latestVersion   func(string) (string, error)
-	archiveURL      func(string, string) (string, error)
-	archiveFilename func(string, string, string) string
-	extractArchive  func(string, string) error
+	latestVersion   func(name string) (string, error)
+	archiveURL      func(name, version string) (string, error)
+	archiveFilename func(name, version, downloadURL string) string
+	extractArchive  func(path, outputDir string) error
 }
 
 var (
@@ -27,7 +28,7 @@ var (
 	}
 )
 
-func DefaultArchiveFilename(pkgName, version, downloadURL string) string {
+func defaultArchiveFilename(_, _, downloadURL string) string {
 	return path.Base(downloadURL)
 }
 
@@ -74,9 +75,20 @@ func (p *PkgManager) Package(name, version string) *Pkg {
 	}
 }
 
+/*
+DownloadArchive downloads an archive of the given package name and version
+to the specified directory, and returns the path to the downloaded archive.
+
+The archive file is named according to ecosystem-specific rules.
+An empty string can be passed for directory, in which case the current
+directory is used.
+
+If an error occurs during download of the file, it is returned along with
+an empty path value.
+*/
 func (p *PkgManager) DownloadArchive(name, version, directory string) (string, error) {
 	if directory == "" {
-		return "", fmt.Errorf("no directory specified")
+		directory = "."
 	}
 
 	downloadURL, err := p.archiveURL(name, version)
@@ -87,15 +99,17 @@ func (p *PkgManager) DownloadArchive(name, version, directory string) (string, e
 		return "", fmt.Errorf("no url found for package %s, version %s", name, version)
 	}
 
-	fileName := p.archiveFilename(name, version, downloadURL)
-
-	archivePath, err := downloadToDirectory(directory, downloadURL, fileName)
+	baseFilename := p.archiveFilename(name, version, downloadURL)
+	if baseFilename == "" {
+		panic("base filename for archive is empty")
+	}
 
-	if err != nil {
+	destPath := filepath.Join(directory, baseFilename)
+	if err := downloadToPath(destPath, downloadURL); err != nil {
 		return "", err
 	}
 
-	return archivePath, nil
+	return destPath, nil
 }
 
 func (p *PkgManager) ExtractArchive(archivePath, outputDir string) error {
diff --git a/internal/pkgmanager/npm.go b/internal/pkgmanager/npm.go
index 495c88e1..ef69526e 100644
--- a/internal/pkgmanager/npm.go
+++ b/internal/pkgmanager/npm.go
@@ -74,6 +74,6 @@ var npmPkgManager = PkgManager{
 	ecosystem:       pkgecosystem.NPM,
 	latestVersion:   getNPMLatest,
 	archiveURL:      getNPMArchiveURL,
-	archiveFilename: DefaultArchiveFilename,
+	archiveFilename: defaultArchiveFilename,
 	extractArchive:  utils.ExtractTarGzFile,
 }
diff --git a/internal/pkgmanager/pypi.go b/internal/pkgmanager/pypi.go
index b77c37a2..65cccf9e 100644
--- a/internal/pkgmanager/pypi.go
+++ b/internal/pkgmanager/pypi.go
@@ -81,6 +81,6 @@ var pypiPkgManager = PkgManager{
 	ecosystem:       pkgecosystem.PyPI,
 	latestVersion:   getPyPILatest,
 	archiveURL:      getPyPIArchiveURL,
-	archiveFilename: DefaultArchiveFilename,
+	archiveFilename: defaultArchiveFilename,
 	extractArchive:  utils.ExtractTarGzFile,
 }
diff --git a/internal/pkgmanager/rubygems.go b/internal/pkgmanager/rubygems.go
index daec6092..c37938ee 100644
--- a/internal/pkgmanager/rubygems.go
+++ b/internal/pkgmanager/rubygems.go
@@ -44,5 +44,5 @@ var rubygemsPkgManager = PkgManager{
 	ecosystem:       pkgecosystem.RubyGems,
 	latestVersion:   getRubyGemsLatest,
 	archiveURL:      getRubyGemsArchiveURL,
-	archiveFilename: DefaultArchiveFilename,
+	archiveFilename: defaultArchiveFilename,
 }
diff --git a/internal/resultstore/resultstore.go b/internal/resultstore/resultstore.go
index 06ea6b52..f16061da 100644
--- a/internal/resultstore/resultstore.go
+++ b/internal/resultstore/resultstore.go
@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"io"
+	"os"
 	"path/filepath"
 	"time"
 
@@ -15,8 +16,8 @@ import (
 	_ "gocloud.dev/blob/s3blob"
 
 	"github.com/ossf/package-analysis/internal/log"
-	"github.com/ossf/package-analysis/internal/utils"
 	"github.com/ossf/package-analysis/internal/pkgmanager"
+	"github.com/ossf/package-analysis/internal/utils"
 )
 
 type ResultStore struct {
@@ -113,29 +114,53 @@ func (rs *ResultStore) SaveTempFilesToZip(ctx context.Context, p Pkg, fileName s
 }
 
 func (rs *ResultStore) SaveAnalyzedPackage(ctx context.Context, manager *pkgmanager.PkgManager, pkg Pkg) error {
+	archivePath, err := manager.DownloadArchive(pkg.Name(), pkg.Version(), "")
+	if err != nil {
+		return err
+	}
+
+	defer func() {
+		if err := os.Remove(archivePath); err != nil {
+			log.Error("could not clean up downloaded archive", "error", err)
+		}
+	}()
+
+	hash, err := utils.SHA256Hash(archivePath)
+	if err != nil {
+		return err
+	}
+
 	bkt, err := blob.OpenBucket(ctx, rs.bucket)
 	if err != nil {
 		return err
 	}
 	defer bkt.Close()
 
-	uploadPath := rs.generatePath(pkg)
-	log.Info("Uploading analyzed package",
-		"bucket", rs.bucket,
-		"path", uploadPath)
+	uploadPath := rs.generatePath(pkg) + "-" + hash
+	log.Info("Uploading analyzed package", "bucket", rs.bucket, "path", uploadPath)
 
-	w, err := bkt.NewWriter(ctx, uploadPath, nil)
+	f, err := os.Open(archivePath)
 	if err != nil {
 		return err
 	}
-	defer w.Close()
-
-	_, err = manager.DownloadArchive(pkg.Name(), pkg.Version(), uploadPath)
+	defer f.Close()
 
+	w, err := bkt.NewWriter(ctx, uploadPath, nil)
 	if err != nil {
 		return err
 	}
 
+	_, writeErr := io.Copy(w, f)
+	closeErr := w.Close()
+
+	if writeErr != nil {
+		// TODO golang 1.20: use errors.Join(writeErr, closeErr)
+		return writeErr
+	}
+	if closeErr != nil {
+		return closeErr
+	}
+
 	return nil
 }
 
diff --git a/internal/staticanalysis/basic_data.go b/internal/staticanalysis/basic_data.go
index 257ee875..16d4dd00 100644
--- a/internal/staticanalysis/basic_data.go
+++ b/internal/staticanalysis/basic_data.go
@@ -145,10 +145,11 @@ func GetBasicData(fileList []string, pathInArchive map[string]string) (*BasicPac
 		}
 
 		var fileHash string
-		if hash, err := utils.HashFile(filePath); err != nil {
+		if hash, err := utils.SHA256Hash(filePath); err != nil {
 			log.Error("Error hashing file", "path", archivePath, "error", err)
 		} else {
-			fileHash = hash
+			// append label to hash
+			fileHash = "sha256:" + hash
 		}
 
 		var lineLengthCounts map[int]int
diff --git a/internal/utils/hash_file.go b/internal/utils/hash_file.go
index 3eaf48fb..7c9bb93c 100644
--- a/internal/utils/hash_file.go
+++ b/internal/utils/hash_file.go
@@ -7,16 +7,18 @@ import (
 	"os"
 )
 
-func HashFile(path string) (string, error) {
+// SHA256Hash returns the SHA256 hashsum of a file.
+func SHA256Hash(path string) (string, error) {
 	f, err := os.Open(path)
 	if err != nil {
 		return "", err
 	}
 	defer f.Close()
+
 	hash := sha256.New()
-	_, err = io.Copy(hash, f)
-	if err != nil {
+	if _, err = io.Copy(hash, f); err != nil {
 		return "", err
 	}
-	return fmt.Sprintf("sha256:%x", hash.Sum([]byte{})), nil
+
+	return fmt.Sprintf("%x", hash.Sum([]byte{})), nil
 }


Test patch:
diff --git a/internal/pkgmanager/download_test.go b/internal/pkgmanager/download_test.go
index cb70aa5a..ca749a48 100644
--- a/internal/pkgmanager/download_test.go
+++ b/internal/pkgmanager/download_test.go
@@ -1,145 +1,149 @@
 package pkgmanager
 
 import (
-	"fmt"
 	"os"
 	"testing"
 
+	"github.com/ossf/package-analysis/internal/utils"
 	"github.com/ossf/package-analysis/pkg/api/pkgecosystem"
 )
 
-type downloadTestSpec struct {
-	name       string
-	pkgName    string
-	pkgVersion string
-	wantErr    bool
+type downloadTestCase struct {
+	name        string
+	ecosystem   pkgecosystem.Ecosystem
+	pkgName     string
+	pkgVersion  string
+	archiveHash string
+	wantErr     bool
 }
 
-func testDownload(t *testing.T, tests []downloadTestSpec, manager *PkgManager) {
-	t.Helper()
-	tmpDir, err := os.MkdirTemp("", "package-analysis-test-npm-dl")
-	if err != nil {
-		t.Fatalf("Could not create temp dir for downloads: %v", err)
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			downloadPath, err := manager.DownloadArchive(tt.pkgName, tt.pkgVersion, tmpDir)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("Want error: %v; got error: %v", tt.wantErr, err)
-				return
-			}
-
-			if err != nil {
-				// File wasn't meant to download properly
-				return
-			}
-
-			if downloadPath == "" {
-				t.Errorf("downloadNPMArchive() returned no error but empty path")
-				return
-			}
-
-			err = os.Remove(downloadPath)
-			if err != nil {
-				t.Errorf("Error removing file: %v", err)
-			}
-		})
-	}
-
-	err = os.RemoveAll(tmpDir)
-	if err != nil {
-		t.Errorf("error removing temp dir (%s): %v", tmpDir, err)
-	}
+var downloadTestCases = []downloadTestCase{
+	{
+		name:       "NPM async 'latest' version",
+		ecosystem:  pkgecosystem.NPM,
+		pkgName:    "async",
+		pkgVersion: "latest",
+		wantErr:    false,
+	},
+	{
+		name:       "NPM async valid version",
+		ecosystem:  pkgecosystem.NPM,
+		pkgName:    "async",
+		pkgVersion: "3.2.4",
+		wantErr:    false,
+	},
+	{
+		name:       "NPM async invalid version",
+		ecosystem:  pkgecosystem.NPM,
+		pkgName:    "async",
+		pkgVersion: "3.2.4444444",
+		wantErr:    true,
+	},
+	{
+		name:       "NPM invalid package name",
+		ecosystem:  pkgecosystem.NPM,
+		pkgName:    "fr(2t5j923)",
+		pkgVersion: "latest",
+		wantErr:    true,
+	},
+	{
+		name:       "PyPI urllib3 valid version",
+		ecosystem:  pkgecosystem.PyPI,
+		pkgName:    "urllib3",
+		pkgVersion: "1.26.11",
+		wantErr:    false,
+	},
+	{
+		name:       "PyPI urllib3 invalid version",
+		ecosystem:  pkgecosystem.PyPI,
+		pkgName:    "urllib3",
+		pkgVersion: "1.26.111",
+		wantErr:    true,
+	},
+	{
+		name:       "PyPI invalid package name",
+		ecosystem:  pkgecosystem.PyPI,
+		pkgName:    "fr(2t5j923)",
+		pkgVersion: "123",
+		wantErr:    true,
+	},
+	{
+		name:       "crates.io rand valid version",
+		ecosystem:  pkgecosystem.CratesIO,
+		pkgName:    "rand",
+		pkgVersion: "0.8.5",
+		wantErr:    false,
+	},
+	{
+		name:       "crates.io rand invalid version",
+		ecosystem:  pkgecosystem.CratesIO,
+		pkgName:    "rand",
+		pkgVersion: "0.8.55",
+		wantErr:    true,
+	},
+	{
+		name:       "crates.io invalid package name",
+		ecosystem:  pkgecosystem.CratesIO,
+		pkgName:    "fr(2t5j923)",
+		pkgVersion: "123",
+		wantErr:    true,
+	},
+	{
+		name:        "pypi black 23.3.0",
+		ecosystem:   pkgecosystem.PyPI,
+		pkgName:     "black",
+		pkgVersion:  "23.3.0",
+		archiveHash: "1c7b8d606e728a41ea1ccbd7264677e494e87cf630e399262ced92d4a8dac940",
+		wantErr:     false,
+	},
+	{
+		name:        "npm invalid package name",
+		ecosystem:   pkgecosystem.PyPI,
+		pkgName:     "3i3ii3ii3i3i",
+		pkgVersion:  "",
+		archiveHash: "",
+		wantErr:     true,
+	},
+	{
+		name:        "pypi black invalid version",
+		ecosystem:   pkgecosystem.PyPI,
+		pkgName:     "black",
+		pkgVersion:  "23333.3.0",
+		archiveHash: "",
+		wantErr:     true,
+	},
 }
 
-func TestDownloadNpmArchive(t *testing.T) {
-	tests := []downloadTestSpec{
-		{"pkgname=async version='latest'", "async", "latest", false},
-		{"pkgname=async version=(valid)", "async", "3.2.4", false},
-		{"pkgname=async version=(invalid)", "async", "3.2.4444444", true},
-		{"pkgname=(invalid)", "fr(2t5j923)", "latest", true},
-	}
-
-	testDownload(t, tests, Manager(pkgecosystem.NPM))
-}
-
-func TestDownloadPyPIArchive(t *testing.T) {
-	tests := []downloadTestSpec{
-		{"pkgname=urllib3 version=(valid)", "urllib3", "1.26.11", false},
-		{"pkgname=urllib3 version=(invalid)", "urllib3", "1.26.111", true},
-		{"pkgname=(invalid)", "fr(2t5j923)", "123", true},
-	}
-
-	testDownload(t, tests, Manager(pkgecosystem.PyPI))
-}
-
-func TestDownloadCratesArchive(t *testing.T) {
-	tests := []downloadTestSpec{
-		{"pkgname=rand version=(valid)", "rand", "0.8.5", false},
-		{"pkgname=rand version=(invalid)", "rand", "0.8.55", true},
-		{"pkgname=(invalid)", "fr(2t5j923)", "123", true},
-	}
-
-	testDownload(t, tests, Manager(pkgecosystem.CratesIO))
-}
-
-func TestDownloadToDirectory(t *testing.T) {
-	tmpDir, err := os.MkdirTemp("", "package-analysis-test-dl")
-	if err != nil {
-		t.Fatalf("Could not create temp dir for downloads: %v", err)
-	}
-
-	testPkg := "black"
-	version := "23.3.0"
-	checksum := "1c7b8d606e728a41ea1ccbd7264677e494e87cf630e399262ced92d4a8dac940"
-	fileName := fmt.Sprintf("%s-%s.tar.gz-%s", testPkg, version, checksum)
-
-	testURL, err := getPyPIArchiveURL(testPkg, version)
-	if err != nil {
-		t.Fatalf("Could not get PyPI archive url")
-	}
-
-	tests := []struct {
-		name     string
-		url      string
-		dir      string
-		filename string
-		wantErr  bool
-	}{
-		{testURL + " (plain)", testURL, tmpDir, fileName, false},
-		{testURL + "123 (invalid URL)", testURL + "123", tmpDir, testURL + "123", true},
-		{testURL + " (invalid dir)", testURL, "/tmp/does/not/exist", fileName, true},
-	}
-	for _, tt := range tests {
+func TestDownload(t *testing.T) {
+	for _, tt := range downloadTestCases {
 		t.Run(tt.name, func(t *testing.T) {
-			downloadedFile, err := downloadToDirectory(tt.dir, tt.url, DefaultArchiveFilename(testPkg, version, testURL))
+			downloadDir := t.TempDir()
+			downloadPath, err := Manager(tt.ecosystem).DownloadArchive(tt.pkgName, tt.pkgVersion, downloadDir)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("Want error: %v; got error: %v", tt.wantErr, err)
 				return
 			}
-
 			if err != nil {
 				// File wasn't meant to download properly
 				return
 			}
 
-			stat, err := os.Stat(downloadedFile)
-			if err != nil {
-				t.Errorf("stat() returned error: %v", err)
-				return
-			}
-			if stat.Name() != tt.filename {
-				t.Errorf("Expected filename %s, got filename %s", tt.filename, stat.Name())
+			if tt.archiveHash != "" {
+				gotHash, err := utils.SHA256Hash(downloadPath)
+				if err != nil {
+					// hashing isn't meant to throw an error
+					t.Errorf("hashing failed: %v", err)
+					return
+				}
+
+				if tt.archiveHash != gotHash {
+					t.Errorf("Expected hash %s, got %s", tt.archiveHash, gotHash)
+				}
 			}
-			err = os.Remove(downloadedFile)
-			if err != nil {
-				t.Errorf("Error removing file: %v", err)
+
+			if err := os.Remove(downloadPath); err != nil {
+				t.Errorf("Error removing downloaded archive: %v", err)
 			}
 		})
 	}
-	err = os.RemoveAll(tmpDir)
-	if err != nil {
-		t.Errorf("error removing temp dir (%s): %v", tmpDir, err)
-	}
 }
diff --git a/internal/utils/hash_file_test.go b/internal/utils/hash_file_test.go
index 60addd06..42a861ba 100644
--- a/internal/utils/hash_file_test.go
+++ b/internal/utils/hash_file_test.go
@@ -8,6 +8,14 @@ import (
 	"github.com/ossf/package-analysis/internal/utils"
 )
 
+// pairs of strings and their SHA256 hash digests
+var hashPairs = [][2]string{
+	{"", "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"},
+	{"Hello, World!", "dffd6021bb2bd5b0af676290809ec3a53191dd81c7f70a4b28688a362182986f"},
+	{"Hello,\nWorld!", "d62b51d504f02642dab5003959af0c1557094c7d49dcc544aba37a0a5d8d1d0d"},
+	{"Hello,\nWorld!\n", "f5651768767f5e83d7001136251b6558a6d01550b04e12c1678ea3a0ca1e8a30"},
+}
+
 func TestHashFile(t *testing.T) {
 	tests := []struct {
 		name     string
@@ -16,23 +24,23 @@ func TestHashFile(t *testing.T) {
 	}{
 		{
 			name:     "empty file",
-			contents: "",
-			want:     "sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
+			contents: hashPairs[0][0],
+			want:     hashPairs[0][1],
 		},
 		{
 			name:     "single line",
-			contents: "Hello, World!",
-			want:     "sha256:dffd6021bb2bd5b0af676290809ec3a53191dd81c7f70a4b28688a362182986f",
+			contents: hashPairs[1][0],
+			want:     hashPairs[1][1],
 		},
 		{
-			name:     "mutli line",
-			contents: "Hello,\nWorld!",
-			want:     "sha256:d62b51d504f02642dab5003959af0c1557094c7d49dcc544aba37a0a5d8d1d0d",
+			name:     "multi line",
+			contents: hashPairs[2][0],
+			want:     hashPairs[2][1],
 		},
 		{
 			name:     "trailing new line",
-			contents: "Hello,\nWorld!\n",
-			want:     "sha256:f5651768767f5e83d7001136251b6558a6d01550b04e12c1678ea3a0ca1e8a30",
+			contents: hashPairs[3][0],
+			want:     hashPairs[3][1],
 		},
 	}
 	for _, test := range tests {
@@ -42,12 +50,12 @@ func TestHashFile(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Failed to prepare hash file: %v", err)
 			}
-			got, err := utils.HashFile(f)
+			got, err := utils.SHA256Hash(f)
 			if err != nil {
 				t.Fatalf("Failed to generate hash: %v", err)
 			}
 			if got != test.want {
-				t.Errorf("HashFile() = %v; want %v", got, test.want)
+				t.Errorf("SHA256Hash() = %v; want %v", got, test.want)
 			}
 		})
 	}
@@ -55,11 +63,11 @@ func TestHashFile(t *testing.T) {
 
 func TestHashFile_MissingFile(t *testing.T) {
 	f := filepath.Join(t.TempDir(), "missing.txt")
-	got, err := utils.HashFile(f)
+	got, err := utils.SHA256Hash(f)
 	if err == nil {
-		t.Error("HashFile() returned no error; want an error")
+		t.Error("SHA256Hash() returned no error; want an error")
 	}
 	if got != "" {
-		t.Errorf("HashFile() = %v; want ''", got)
+		t.Errorf("SHA256Hash() = %v; want ''", got)
 	}
 }