feat: Enhance GetObject method to handle plus signs in URLs and improve error handling

pkg/api/signed_url.go

@@ -210,29 +210,44 @@ func (u *SignedURL) delete(client *retryablehttp.Client, artifact *Artifact) err
 }
 
 func (u *SignedURL) GetObject() (string, error) {
-	URL, _ := url.Parse(u.URL)
+	URL, err := url.Parse(u.URL)
+	if err != nil {
+		return "", fmt.Errorf("failed to parse URL '%s': %v", u.URL, err)
+	}
 
+	var obj string
 	switch host := URL.Host; {
 	case host == "storage.googleapis.com":
 		log.Debugf("Parsing GCS URL: %s\n", u.URL)
-		return parseGoogleStorageURL(URL)
+		obj, err = parseGoogleStorageURL(URL)
 
 	case strings.HasSuffix(host, "amazonaws.com"):
 		log.Debugf("Parsing S3 URL: %s\n", u.URL)
-		return parseS3URL(URL)
+		obj, err = parseS3URL(URL)
 
 	case strings.HasPrefix(host, "127.0.0.1"):
 		log.Debugf("Parsing localhost URL: %s\n", u.URL)
-		return parseLocalhostURL(URL)
+		obj, err = parseLocalhostURL(URL)
 
 	case customDomainRegex.Match([]byte(URL.String())):
 		log.Debugf("Parsing custom domain URL: %s\n", u.URL)
-		return parseCustomDomainURL(URL)
+		obj, err = parseCustomDomainURL(URL)
 
 	default:
 		log.Warnf("Failed to parse URL '%s' - unrecognized host '%s'\n", u.URL, host)
 		return "", fmt.Errorf("unrecognized host %s", host)
 	}
+
+	if err != nil {
+		return "", err
+	}
+
+	decoded, err := url.PathUnescape(obj)
+	if err != nil {
+		return "", fmt.Errorf("failed to decode object path '%s': %v", obj, err)
+	}
+
+	return decoded, nil
 }
 
 // GCS URLs follow the format 'https://storage.googleapis.com/<bucket-name>/<path>'
@@ -280,5 +295,5 @@ func parseCustomDomainURL(URL *url.URL) (string, error) {
 // Localhost URLs are used during tests
 func parseLocalhostURL(URL *url.URL) (string, error) {
 	// we don't want the leading slash
-	return URL.Path[1:], nil
+	return strings.TrimPrefix(URL.EscapedPath(), "/"), nil
 }

pkg/api/signed_url_test.go

@@ -14,6 +14,43 @@ func Test__GetObject(t *testing.T) {
 		assert.Equal(t, "artifacts/project/projectid/myfile.txt", obj)
 	})
 
+	t.Run("GCS - file with plus", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://storage.googleapis.com/my-bucket1/artifacts/project/projectid/test_art%2Bifact.txt?Expires=231256754712"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/test_art+ifact.txt", obj)
+	})
+
+	t.Run("GCS - file with space", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://storage.googleapis.com/my-bucket1/artifacts/project/projectid/my%20file.txt?Expires=231256754712"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/my file.txt", obj)
+	})
+
+	t.Run("GCS - file with multiple special chars", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://storage.googleapis.com/my-bucket1/artifacts/project/projectid/build%2B%2B/output%20(1).txt?Expires=231256754712"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/build++/output (1).txt", obj)
+	})
+
+	t.Run("GCS - file with percent literal", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://storage.googleapis.com/my-bucket1/artifacts/project/projectid/100%25done.txt?Expires=231256754712"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/100%done.txt", obj)
+	})
+
+	t.Run("GCS - file with literal %2B in name", func(t *testing.T) {
+		// Filename is literally "file%2Bname.txt" on disk.
+		// SDK double-encodes: %2B -> %252B in the signed URL.
+		signedURL := SignedURL{URL: "https://storage.googleapis.com/my-bucket1/artifacts/project/projectid/file%252Bname.txt?Expires=231256754712"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/file%2Bname.txt", obj)
+	})
+
 	t.Run("GCS - file inside directory", func(t *testing.T) {
 		signedURL := SignedURL{URL: "https://storage.googleapis.com/my-bucket1/artifacts/project/projectid/mydir/myfile.txt?Expires=231256754712"}
 		obj, err := signedURL.GetObject()
@@ -28,6 +65,50 @@ func Test__GetObject(t *testing.T) {
 		assert.Equal(t, "artifacts/project/projectid/myfile.txt", obj)
 	})
 
+	t.Run("S3 - file with plus", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.us-east-1.amazonaws.com/projectid/artifacts/project/projectid/test_art%2Bifact.txt?X-Amz-Whatever"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/test_art+ifact.txt", obj)
+	})
+
+	t.Run("S3 - file with space", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.us-east-1.amazonaws.com/projectid/artifacts/project/projectid/my%20file.txt?X-Amz-Whatever"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/my file.txt", obj)
+	})
+
+	t.Run("S3 - file with multiple special chars", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.us-east-1.amazonaws.com/projectid/artifacts/project/projectid/build%2B%2B/output%20(1).txt?X-Amz-Whatever"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/build++/output (1).txt", obj)
+	})
+
+	t.Run("S3 - file with percent literal", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.us-east-1.amazonaws.com/projectid/artifacts/project/projectid/100%25done.txt?X-Amz-Whatever"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/100%done.txt", obj)
+	})
+
+	t.Run("S3 - file with literal %2B in name", func(t *testing.T) {
+		// Filename is literally "file%2Bname.txt" on disk.
+		// SDK double-encodes: %2B -> %252B in the signed URL.
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.us-east-1.amazonaws.com/projectid/artifacts/project/projectid/file%252Bname.txt?X-Amz-Whatever"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/file%2Bname.txt", obj)
+	})
+
+	t.Run("S3 region-less - file with plus", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.amazonaws.com/projectid/artifacts/project/projectid/test_art%2Bifact.txt?X-Amz-Whatever"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/test_art+ifact.txt", obj)
+	})
+
 	t.Run("S3 with region-less URL - file", func(t *testing.T) {
 		signedURL := SignedURL{URL: "https://my-bucket1.s3.amazonaws.com/projectid/artifacts/project/projectid/myfile.txt?X-Amz-Whatever"}
 		obj, err := signedURL.GetObject()
@@ -63,6 +144,27 @@ func Test__GetObject(t *testing.T) {
 		assert.Equal(t, "artifacts/project/projectid/mydir/myfile.txt", obj)
 	})
 
+	t.Run("127.0.0.1 - file with plus", func(t *testing.T) {
+		signedURL := SignedURL{URL: "http://127.0.0.1:8080/artifacts/project/projectid/test_art%2Bifact.txt"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/test_art+ifact.txt", obj)
+	})
+
+	t.Run("127.0.0.1 - file with space", func(t *testing.T) {
+		signedURL := SignedURL{URL: "http://127.0.0.1:8080/artifacts/project/projectid/my%20file.txt"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/my file.txt", obj)
+	})
+
+	t.Run("127.0.0.1 - file with literal %2B in name", func(t *testing.T) {
+		signedURL := SignedURL{URL: "http://127.0.0.1:8080/artifacts/project/projectid/file%252Bname.txt"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/file%2Bname.txt", obj)
+	})
+
 	t.Run("custom domain - file", func(t *testing.T) {
 		signedURL := SignedURL{URL: "https://artifacts.somedomain.com/my-bucket1/projectid/artifacts/project/projectid/myfile.txt?X-Amz-Algorithm"}
 		obj, err := signedURL.GetObject()
@@ -77,6 +179,41 @@ func Test__GetObject(t *testing.T) {
 		assert.Equal(t, "artifacts/project/projectid/mydir/myfile.txt", obj)
 	})
 
+	t.Run("custom domain - file with plus", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://artifacts.somedomain.com/my-bucket1/projectid/artifacts/project/projectid/test_art%2Bifact.txt?X-Amz-Algorithm"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/test_art+ifact.txt", obj)
+	})
+
+	t.Run("custom domain - file with space", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://artifacts.somedomain.com/my-bucket1/projectid/artifacts/project/projectid/my%20file.txt?X-Amz-Algorithm"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/my file.txt", obj)
+	})
+
+	t.Run("custom domain - file with multiple special chars", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://artifacts.somedomain.com/my-bucket1/projectid/artifacts/project/projectid/build%2B%2B/output%20(1).txt?X-Amz-Algorithm"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/build++/output (1).txt", obj)
+	})
+
+	t.Run("custom domain - file with literal %2B in name", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://artifacts.somedomain.com/my-bucket1/projectid/artifacts/project/projectid/file%252Bname.txt?X-Amz-Algorithm"}
+		obj, err := signedURL.GetObject()
+		assert.Nil(t, err)
+		assert.Equal(t, "artifacts/project/projectid/file%2Bname.txt", obj)
+	})
+
+	t.Run("S3 - malformed URL escape returns parse error", func(t *testing.T) {
+		signedURL := SignedURL{URL: "https://my-bucket1.s3.us-east-1.amazonaws.com/projectid/artifacts/project/projectid/bad%2Xname.txt?X-Amz-Whatever"}
+		_, err := signedURL.GetObject()
+		assert.NotNil(t, err)
+		assert.Contains(t, err.Error(), "failed to parse URL")
+	})
+
 	t.Run("bad URL", func(t *testing.T) {
 		signedURL := SignedURL{URL: "http://somehost.com/projectid/artifacts/project/projectid/myfile.txt"}
 		_, err := signedURL.GetObject()

pkg/storage/pull.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path"
+	"strings"
 
 	api "github.com/semaphoreci/artifact/pkg/api"
 	"github.com/semaphoreci/artifact/pkg/files"
@@ -60,7 +61,11 @@ func buildArtifacts(signedURLs []*api.SignedURL, paths *files.ResolvedPath, forc
 			return nil, err
 		}
 
-		localPath := path.Join(paths.Destination, obj[len(paths.Source):])
+		relative, ok := objectRelativeToSource(obj, paths.Source)
+		if !ok {
+			return nil, fmt.Errorf("failed to resolve local path: remote object '%s' does not match source '%s'", obj, paths.Source)
+		}
+		localPath := path.Join(paths.Destination, relative)
 
 		if !force {
 			if _, err := os.Stat(localPath); err == nil {
@@ -78,6 +83,19 @@ func buildArtifacts(signedURLs []*api.SignedURL, paths *files.ResolvedPath, forc
 	return artifacts, nil
 }
 
+func objectRelativeToSource(object, source string) (string, bool) {
+	if object == source {
+		return "", true
+	}
+
+	sourcePrefix := strings.TrimSuffix(source, "/") + "/"
+	if strings.HasPrefix(object, sourcePrefix) {
+		return strings.TrimPrefix(object, sourcePrefix), true
+	}
+
+	return "", false
+}
+
 func doPull(artifacts []*api.Artifact) (*PullStats, error) {
 	client := newHTTPClient()
 	stats := &PullStats{}