refact pkg/cwhub (#3666)

* cwhub: stricter SafePath()
* pkg/cwhub: compute hash on the fly
* pkg/cwhub: named return values
* pkg/cwhub: don't call panic() in tests
* pkg/cwhub: refact relativePathComponents
* pkg/cwhub: refact sync.go
* pkg/cwhub: extract method collectSpecs()
* pkg/cwhub: simplify url handling

Author: mmetc

pkg/cwhub/cwhub.go

@@ -1,10 +1,7 @@
 package cwhub
 
 import (
-	"fmt"
 	"net/http"
-	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/crowdsecurity/crowdsec/pkg/apiclient/useragent"
@@ -25,22 +22,3 @@ var HubClient = &http.Client{
 	Timeout:   120 * time.Second,
 	Transport: &hubTransport{http.DefaultTransport},
 }
-
-// SafePath returns a joined path and ensures that it does not escape the base directory.
-func SafePath(dir, filePath string) (string, error) {
-	absBaseDir, err := filepath.Abs(filepath.Clean(dir))
-	if err != nil {
-		return "", err
-	}
-
-	absFilePath, err := filepath.Abs(filepath.Join(dir, filePath))
-	if err != nil {
-		return "", err
-	}
-
-	if !strings.HasPrefix(absFilePath, absBaseDir) {
-		return "", fmt.Errorf("path %s escapes base directory %s", filePath, dir)
-	}
-
-	return absFilePath, nil
-}

pkg/cwhub/cwhub_test.go

@@ -75,7 +75,7 @@ func testHubOld(t *testing.T, update bool) *Hub {
 
 // envSetup initializes the temporary hub and mocks the http client.
 func envSetup(t *testing.T) *Hub {
-	setResponseByPath()
+	setResponseByPath(t)
 	log.SetLevel(log.DebugLevel)
 
 	defaultTransport := HubClient.Transport
@@ -121,27 +121,23 @@ func (t *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	return response, nil
 }
 
-func fileToStringX(path string) string {
+func fileToStringX(t *testing.T, path string) string {
 	f, err := os.Open(path)
-	if err != nil {
-		panic(err)
-	}
+	require.NoError(t, err)
 	defer f.Close()
 
 	data, err := io.ReadAll(f)
-	if err != nil {
-		panic(err)
-	}
+	require.NoError(t, err)
 
 	return strings.ReplaceAll(string(data), "\r\n", "\n")
 }
 
-func setResponseByPath() {
+func setResponseByPath(t *testing.T) {
 	responseByPath = map[string]string{
-		"/crowdsecurity/master/parsers/s01-parse/crowdsecurity/foobar_parser.yaml":    fileToStringX("./testdata/foobar_parser.yaml"),
-		"/crowdsecurity/master/parsers/s01-parse/crowdsecurity/foobar_subparser.yaml": fileToStringX("./testdata/foobar_parser.yaml"),
-		"/crowdsecurity/master/collections/crowdsecurity/test_collection.yaml":        fileToStringX("./testdata/collection_v1.yaml"),
-		"/crowdsecurity/master/.index.json":                                           fileToStringX("./testdata/index1.json"),
+		"/crowdsecurity/master/parsers/s01-parse/crowdsecurity/foobar_parser.yaml":    fileToStringX(t, "./testdata/foobar_parser.yaml"),
+		"/crowdsecurity/master/parsers/s01-parse/crowdsecurity/foobar_subparser.yaml": fileToStringX(t, "./testdata/foobar_parser.yaml"),
+		"/crowdsecurity/master/collections/crowdsecurity/test_collection.yaml":        fileToStringX(t, "./testdata/collection_v1.yaml"),
+		"/crowdsecurity/master/.index.json":                                           fileToStringX(t, "./testdata/index1.json"),
 		"/crowdsecurity/master/scenarios/crowdsecurity/foobar_scenario.yaml": `filter: true
 name: crowdsecurity/foobar_scenario`,
 		"/crowdsecurity/master/scenarios/crowdsecurity/barfoo_scenario.yaml": `filter: true

pkg/cwhub/download.go

@@ -32,51 +32,39 @@ type ContentProvider interface {
 }
 
 // urlTo builds the URL to download a file from the remote hub.
-func (d *Downloader) urlTo(remotePath string) (string, error) {
+func (d *Downloader) urlTo(remotePath string) (*url.URL, error) {
 	// the template must contain two string placeholders
 	if fmt.Sprintf(d.URLTemplate, "%s", "%s") != d.URLTemplate {
-		return "", fmt.Errorf("invalid URL template '%s'", d.URLTemplate)
+		return nil, fmt.Errorf("invalid URL template '%s'", d.URLTemplate)
 	}
 
-	return fmt.Sprintf(d.URLTemplate, d.Branch, remotePath), nil
-}
+	raw := fmt.Sprintf(d.URLTemplate, d.Branch, remotePath)
 
-// addURLParam adds a parameter with a value (ex. "with_content=true") to the URL if it's not already present.
-func addURLParam(rawURL string, param string, value string) (string, error) {
-	parsedURL, err := url.Parse(rawURL)
+	parsed, err := url.Parse(raw)
 	if err != nil {
-		return "", fmt.Errorf("failed to parse URL: %w", err)
+		return nil, fmt.Errorf("failed to parse URL: %w", err)
 	}
 
-	query := parsedURL.Query()
-
-	if _, exists := query[param]; !exists {
-		query.Add(param, value)
-	}
-
-	parsedURL.RawQuery = query.Encode()
-
-	return parsedURL.String(), nil
+	return parsed, nil
 }
 
 // FetchIndex downloads the index from the hub and writes it to the filesystem.
 // It uses a temporary file to avoid partial downloads, and won't overwrite the original
 // if it has not changed.
 // Return true if the file has been updated, false if already up to date.
-func (d *Downloader) FetchIndex(ctx context.Context, destPath string, withContent bool, logger *logrus.Logger) (bool, error) {
+func (d *Downloader) FetchIndex(ctx context.Context, destPath string, withContent bool, logger *logrus.Logger) (downloaded bool, err error) {
 	url, err := d.urlTo(".index.json")
 	if err != nil {
 		return false, fmt.Errorf("failed to build hub index request: %w", err)
 	}
 
 	if withContent {
-		url, err = addURLParam(url, "with_content", "true")
-		if err != nil {
-			return false, fmt.Errorf("failed to add 'with_content' parameter to URL: %w", err)
-		}
+		q := url.Query()
+		q.Set("with_content", "true")
+		url.RawQuery = q.Encode()
 	}
 
-	downloaded, err := downloader.
+	downloaded, err = downloader.
 		New().
 		WithHTTPClient(HubClient).
 		ToFile(destPath).
@@ -86,7 +74,7 @@ func (d *Downloader) FetchIndex(ctx context.Context, destPath string, withConten
 		BeforeRequest(func(_ *http.Request) {
 			fmt.Println("Downloading " + destPath)
 		}).
-		Download(ctx, url)
+		Download(ctx, url.String())
 	if err != nil {
 		return false, err
 	}
@@ -97,13 +85,13 @@ func (d *Downloader) FetchIndex(ctx context.Context, destPath string, withConten
 // FetchContent downloads the content to the specified path, through a temporary file
 // to avoid partial downloads.
 // If the hash does not match, it will not overwrite and log a warning.
-func (d *Downloader) FetchContent(ctx context.Context, remotePath, destPath, wantHash string, logger *logrus.Logger) (bool, string, error) {
-	url, err := d.urlTo(remotePath)
+func (d *Downloader) FetchContent(ctx context.Context, remotePath, destPath, wantHash string, logger *logrus.Logger) (downloaded bool, url string, err error) {
+	u, err := d.urlTo(remotePath)
 	if err != nil {
 		return false, "", fmt.Errorf("failed to build request: %w", err)
 	}
 
-	downloaded, err := downloader.
+	downloaded, err = downloader.
 		New().
 		WithHTTPClient(HubClient).
 		ToFile(destPath).
@@ -112,7 +100,7 @@ func (d *Downloader) FetchContent(ctx context.Context, remotePath, destPath, wan
 		WithLogger(logger.WithField("url", url)).
 		CompareContent().
 		VerifyHash("sha256", wantHash).
-		Download(ctx, url)
+		Download(ctx, u.String())
 
 	var hasherr downloader.HashMismatchError
 
@@ -123,5 +111,5 @@ func (d *Downloader) FetchContent(ctx context.Context, remotePath, destPath, wan
 		return false, "", err
 	}
 
-	return downloaded, url, nil
+	return downloaded, u.String(), nil
 }

pkg/cwhub/fetch.go

@@ -1,13 +1,17 @@
 package cwhub
 
 import (
+	"bytes"
 	"context"
 	"crypto"
 	"encoding/base64"
 	"encoding/hex"
 	"fmt"
+	"io"
 	"os"
 	"path/filepath"
+
+	"github.com/crowdsecurity/go-cs-lib/downloader"
 )
 
 // writeEmbeddedContentTo writes the embedded content to the specified path and checks the hash.
@@ -24,24 +28,32 @@ func (i *Item) writeEmbeddedContentTo(destPath, wantHash string) error {
 	}
 
 	dir := filepath.Dir(destPath)
+	reader := bytes.NewReader(content)
+	hash := crypto.SHA256.New()
 
+	tee := io.TeeReader(reader, hash)
 	if err := os.MkdirAll(dir, 0o755); err != nil {
 		return fmt.Errorf("while creating %s: %w", dir, err)
 	}
 
-	// check sha256
-	hash := crypto.SHA256.New()
-	if _, err := hash.Write(content); err != nil {
-		return fmt.Errorf("while hashing %s: %w", i.Name, err)
+	f, err := os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)
+	if err != nil {
+		return err
 	}
 
-	gotHash := hex.EncodeToString(hash.Sum(nil))
-	if gotHash != wantHash {
-		return fmt.Errorf("hash mismatch: expected %s, got %s. The index file is invalid, please run 'cscli hub update' and try again", wantHash, gotHash)
+	defer f.Close()
+
+	if _, err := io.Copy(f, tee); err != nil {
+		return err
 	}
 
-	if err := os.WriteFile(destPath, content, 0o600); err != nil {
-		return fmt.Errorf("while writing %s: %w", destPath, err)
+	gotHash := hex.EncodeToString(hash.Sum(nil))
+	if gotHash != wantHash {
+		return fmt.Errorf("%w. The index file is invalid, please run 'cscli hub update' and try again",
+			downloader.HashMismatchError{
+				Expected: wantHash,
+				Got: gotHash,
+			})
 	}
 
 	return nil

pkg/cwhub/relativepath.go

@@ -5,24 +5,25 @@ import (
 	"strings"
 )
 
-// relativePathComponents returns the list of path components after baseDir.
-// If path is not inside baseDir, it returns an empty slice.
-func relativePathComponents(path string, baseDir string) []string {
+// relativePathComponents returns the list of path components after baseDir,
+// and a boolean indicating whether path is inside baseDir at all.
+func relativePathComponents(path string, baseDir string) ([]string, bool) {
 	absPath, err := filepath.Abs(path)
 	if err != nil {
-		return []string{}
+		// cwd disappeared??
+		return nil, false
 	}
 
 	absBaseDir, err := filepath.Abs(baseDir)
 	if err != nil {
-		return []string{}
+		return nil, false
 	}
 
 	// is path inside baseDir?
 	relPath, err := filepath.Rel(absBaseDir, absPath)
 	if err != nil || strings.HasPrefix(relPath, "..") || relPath == "." {
-		return []string{}
+		return nil, false
 	}
 
-	return strings.Split(relPath, string(filepath.Separator))
+	return strings.Split(relPath, string(filepath.Separator)), true
 }

pkg/cwhub/relativepath_test.go

@@ -11,62 +11,72 @@ func TestRelativePathComponents(t *testing.T) {
 		name     string
 		path     string
 		baseDir  string
-		expected []string
+		wantSubs []string
+		wantOk   bool
 	}{
 		{
 			name:     "Path within baseDir",
 			path:     "/home/user/project/src/file.go",
 			baseDir:  "/home/user/project",
-			expected: []string{"src", "file.go"},
+			wantSubs: []string{"src", "file.go"},
+			wantOk:   true,
 		},
 		{
 			name:     "Path is baseDir",
 			path:     "/home/user/project",
 			baseDir:  "/home/user/project",
-			expected: []string{},
+			wantSubs: nil,
+			wantOk:   false,
 		},
 		{
 			name:     "Path outside baseDir",
 			path:     "/home/user/otherproject/src/file.go",
 			baseDir:  "/home/user/project",
-			expected: []string{},
+			wantSubs: nil,
+			wantOk:   false,
 		},
 		{
 			name:     "Path is subdirectory of baseDir",
 			path:     "/home/user/project/src/",
 			baseDir:  "/home/user/project",
-			expected: []string{"src"},
+			wantSubs: []string{"src"},
+			wantOk:   true,
 		},
 		{
 			name:     "Relative paths",
 			path:     "project/src/file.go",
 			baseDir:  "project",
-			expected: []string{"src", "file.go"},
+			wantSubs: []string{"src", "file.go"},
+			wantOk:   true,
 		},
 		{
 			name:     "BaseDir with trailing slash",
 			path:     "/home/user/project/src/file.go",
 			baseDir:  "/home/user/project/",
-			expected: []string{"src", "file.go"},
+			wantSubs: []string{"src", "file.go"},
+			wantOk:   true,
 		},
 		{
 			name:     "Empty baseDir",
 			path:     "/home/user/project/src/file.go",
 			baseDir:  "",
-			expected: []string{},
+			wantSubs: nil,
+			wantOk:   false,
 		},
 		{
 			name:     "Empty path",
 			path:     "",
 			baseDir:  "/home/user/project",
-			expected: []string{},
+			wantSubs: nil,
+			wantOk:   false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := relativePathComponents(tt.path, tt.baseDir)
-			assert.Equal(t, tt.expected, result)
+			got, ok := relativePathComponents(tt.path, tt.baseDir)
+			assert.Equal(t, tt.wantSubs, got)
+			assert.Equal(t, tt.wantOk, ok)
 		})
 	}
 }