fix: all virustotal scans erroring and not scanning the files (#95)

* fix: all scans erroring, properly handle errors from previous virustotal analysis

* fix: only empty files passed to virustotal

* chore: enable govet shadow linter

* fix: return virustotal scan error in activity when scan fails

Author: mircearoata

.golangci.yml

@@ -15,7 +15,7 @@ linters-settings:
       - github.com/satisfactorymodding/smr-api/*
 
   govet:
-    disable:
+    enable:
       - shadow
 
   gocritic:
@@ -54,6 +54,9 @@ issues:
     - ./generated/
   exclude:
     - should pass the context parameter
+  exclude-rules:
+    - text: 'shadow: declaration of "err" shadows declaration at'
+      linters: [ govet ]
 
 linters:
   disable-all: true

validation/virustotal.go

@@ -1,8 +1,10 @@
 package validation
 
 import (
+	"bytes"
 	"context"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -26,11 +28,8 @@ func InitializeVirusTotal() {
 
 type AnalysisResults struct {
 	Attributes struct {
-		Stats *struct {
-			Suspicious *int `json:"suspicious,omitempty"`
-			Malicious  *int `json:"malicious,omitempty"`
-		} `json:"stats,omitempty"`
-		Status string `json:"status"`
+		Stats  *AnalysisStats `json:"stats,omitempty"`
+		Status string         `json:"status"`
 	} `json:"attributes,omitempty"`
 	Meta struct {
 		FileInfo struct {
@@ -41,16 +40,18 @@ type AnalysisResults struct {
 
 type PreviousAnalysisResults struct {
 	Attributes struct {
-		Stats *struct {
-			Suspicious *int `json:"suspicious,omitempty"`
-			Malicious  *int `json:"malicious,omitempty"`
-		} `json:"last_analysis_stats,omitempty"`
+		Stats *AnalysisStats `json:"last_analysis_stats,omitempty"`
 	} `json:"attributes,omitempty"`
 }
 
+type AnalysisStats struct {
+	Suspicious *int `json:"suspicious,omitempty"`
+	Malicious  *int `json:"malicious,omitempty"`
+}
+
 type ScanResult struct {
 	Safe     bool
-	Hash     *string
+	Hash     string
 	FileName string
 }
 
@@ -67,11 +68,7 @@ func ScanFiles(ctx context.Context, files []io.Reader, names []string) ([]ScanRe
 			if err != nil {
 				return fmt.Errorf("failed to scan %s: %w", names[count], err)
 			}
-			select {
-			case c <- *scanResult:
-			case <-gctx.Done():
-				return gctx.Err()
-			}
+			c <- *scanResult
 			return nil
 		})
 	}
@@ -94,85 +91,89 @@ func ScanFiles(ctx context.Context, files []io.Reader, names []string) ([]ScanRe
 }
 
 func scanFile(ctx context.Context, file io.Reader, name string) (*ScanResult, error) {
-	scanResult := ScanResult{
-		Safe:     false,
-		FileName: name,
+	hash := sha256.New()
+	fileBytes, err := io.ReadAll(file)
+	if err != nil {
+		return nil, fmt.Errorf("failed read file: %w", err)
 	}
 
-	hash := sha256.New()
-	if _, err := io.Copy(hash, file); err != nil {
-		return nil, fmt.Errorf("failed to generate hash for file %w", err)
+	_, err = hash.Write(fileBytes)
+	if err != nil {
+		return nil, fmt.Errorf("failed to hash file: %w", err)
 	}
 
 	checksum := hash.Sum(nil)
-	var previousAnalysisResults PreviousAnalysisResults
+	hashStr := fmt.Sprintf("%x", checksum)
 
-	_, err := client.GetData(vt.URL("files/%x", checksum), &previousAnalysisResults)
+	var previousAnalysisResults PreviousAnalysisResults
 
-	alreadyScanned := false
-	analysisID := ""
+	hasPreviousAnalysis := true
+	_, err = client.GetData(vt.URL("files/%x", checksum), &previousAnalysisResults)
+	if err != nil {
+		var vtErr vt.Error
+		if errors.As(err, &vtErr) {
+			if vtErr.Code != "NotFoundError" {
+				return nil, fmt.Errorf("failed to get previous analysis results: %w", err)
+			}
+		} else {
+			return nil, fmt.Errorf("failed to get previous analysis results: %w", err)
+		}
+		hasPreviousAnalysis = false
+	}
 
-	if err == nil {
-		alreadyScanned = true
-		slox.Info(ctx, "file already scanned, skipping upload", slog.String("file", name))
-		hash := fmt.Sprintf("%x", checksum)
-		scanResult.Hash = &hash
-	} else {
-		scan, err := client.NewFileScanner().Scan(file, name, nil)
+	if hasPreviousAnalysis {
+		slox.Info(ctx, "file already scanned", slog.String("file", name), slog.String("hash", hashStr))
+		if previousAnalysisResults.Attributes.Stats == nil {
+			return nil, fmt.Errorf("no stats available on previous analysis")
+		}
+		safe, err := isResultSafe(*previousAnalysisResults.Attributes.Stats)
 		if err != nil {
-			return &scanResult, fmt.Errorf("failed to scan file: %w", err)
+			return nil, fmt.Errorf("failed to determine if file is safe: %w", err)
 		}
-		analysisID := scan.ID()
-		slox.Info(ctx, "uploaded virus scan", slog.String("file", name), slog.String("analysis_id", analysisID))
+		return &ScanResult{
+			Safe:     safe,
+			Hash:     hashStr,
+			FileName: name,
+		}, nil
 	}
 
-	for {
-		var analysisResults AnalysisResults
-		var malicious int
-		var suspicious int
-
-		if !alreadyScanned {
-			time.Sleep(time.Second * 15)
-
-			_, err = client.GetData(vt.URL("analyses/%s", analysisID), &analysisResults)
-			if err != nil {
-				scanResult.Safe = false
-				return nil, fmt.Errorf("failed to get analysis results: %w", err)
-			}
-			scanResult.Hash = &analysisResults.Meta.FileInfo.SHA256
-
-			if !alreadyScanned && analysisResults.Attributes.Status != "completed" {
-				continue
-			}
+	scan, err := client.NewFileScanner().Scan(bytes.NewReader(fileBytes), name, nil)
+	if err != nil {
+		return nil, fmt.Errorf("failed to scan file: %w", err)
+	}
+	analysisID := scan.ID()
+	slox.Info(ctx, "uploaded virus scan", slog.String("file", name), slog.String("analysis_id", analysisID))
 
-			if analysisResults.Attributes.Stats == nil {
-				slox.Error(ctx, "no stats available", slog.Any("err", err), slog.String("file", name))
-				scanResult.Safe = false
-				return &scanResult, nil
-			}
+	var analysisResults AnalysisResults
+	for analysisResults.Attributes.Status != "completed" {
+		time.Sleep(time.Second * 15)
 
-			if analysisResults.Attributes.Stats.Malicious == nil || analysisResults.Attributes.Stats.Suspicious == nil {
-				slox.Error(ctx, "unable to determine malicious or suspicious file", slog.Any("err", err), slog.String("file", name))
-				scanResult.Safe = false
-				return &scanResult, nil
-			}
-			malicious = *analysisResults.Attributes.Stats.Malicious
-			suspicious = *analysisResults.Attributes.Stats.Suspicious
-		} else {
-			malicious = *previousAnalysisResults.Attributes.Stats.Malicious
-			suspicious = *previousAnalysisResults.Attributes.Stats.Suspicious
+		_, err := client.GetData(vt.URL("analyses/%s", analysisID), &analysisResults)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get analysis results: %w", err)
 		}
+	}
 
-		// Why 1? Well because some company made a shitty AI and it flags random mods.
-		if malicious > 1 || suspicious > 1 {
-			slox.Error(ctx, "suspicious or malicious file found", slog.Any("err", err), slog.String("file", name))
-			scanResult.Safe = false
-			return &scanResult, nil
-		}
+	if analysisResults.Attributes.Stats == nil {
+		return nil, fmt.Errorf("no stats available")
+	}
+
+	safe, err := isResultSafe(*analysisResults.Attributes.Stats)
+	if err != nil {
+		return nil, fmt.Errorf("failed to determine if file is safe: %w", err)
+	}
+	return &ScanResult{
+		Safe:     safe,
+		Hash:     hashStr,
+		FileName: name,
+	}, nil
+}
 
-		scanResult.Safe = true
-		break
+func isResultSafe(stats AnalysisStats) (bool, error) {
+	if stats.Malicious == nil || stats.Suspicious == nil {
+		return false, fmt.Errorf("missing malicious or suspicious stats")
 	}
 
-	return &scanResult, nil
+	// Why 1? Well because some company made a shitty AI and it flags random mods.
+	return *stats.Malicious <= 1 && *stats.Suspicious <= 1, nil
 }

workflows/versionupload/scan_mod_on_virus_total.go

@@ -93,21 +93,20 @@ func scanAndSaveResults(ctx context.Context, toScan []io.Reader, names []string,
 	defer span.End()
 
 	scanResults, scanErr := validation.ScanFiles(ctx, toScan, names)
-	if scanErr != nil {
-		span.SetStatus(codes.Error, scanErr.Error())
-		span.RecordError(scanErr)
-		if len(scanResults) == 0 {
-			return false, scanErr
-		}
-		slox.Error(ctx, scanErr.Error())
-	}
+	// Check error later, because we can have partial results to save, even in the case of an error
 
 	if err := saveScanResults(ctx, scanResults, args); err != nil {
 		span.SetStatus(codes.Error, err.Error())
 		span.RecordError(err)
 		return false, err
 	}
 
+	if scanErr != nil {
+		span.SetStatus(codes.Error, scanErr.Error())
+		span.RecordError(scanErr)
+		return false, scanErr
+	}
+
 	if len(scanResults) != len(toScan) {
 		return false, nil
 	}
@@ -127,7 +126,7 @@ func saveScanResults(ctx context.Context, scanResults []validation.ScanResult, a
 			c.SetSafe(scanResults[i].Safe).
 				SetVersionID(args.VersionID).
 				SetFileName(scanResults[i].FileName).
-				SetHash(*scanResults[i].Hash)
+				SetHash(scanResults[i].Hash)
 		},
 	).OnConflict().
 		DoNothing().