[#6654] fixed S3 canonical uri parts escaping

CHANGELOG.md

@@ -1,3 +1,8 @@
+## v0.26.5
+
+- Fixed S3 canonical parts escaping when generating the SigV4 request signature ([#6654](https://github.com/pocketbase/pocketbase/issues/6654)).
+
+
 ## v0.26.4
 
 - Fixed `RecordErrorEvent.Error` and `CollectionErrorEvent.Error` sync with `ModelErrorEvent.Error` ([#6639](https://github.com/pocketbase/pocketbase/issues/6639)).

tools/filesystem/internal/s3blob/s3/error.go

@@ -6,6 +6,8 @@ import (
 	"strings"
 )
 
+var _ error = (*ResponseError)(nil)
+
 // ResponseError defines a general S3 response error.
 //
 // https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html
@@ -20,7 +22,7 @@ type ResponseError struct {
 }
 
 // Error implements the std error interface.
-func (err ResponseError) Error() string {
+func (err *ResponseError) Error() string {
 	var strBuilder strings.Builder
 
 	strBuilder.WriteString(strconv.Itoa(err.Status))

tools/filesystem/internal/s3blob/s3/error_test.go

@@ -19,7 +19,7 @@ func TestResponseErrorSerialization(t *testing.T) {
 		</Error>
 	`
 
-	respErr := s3.ResponseError{
+	respErr := &s3.ResponseError{
 		Status: 123,
 		Raw:    []byte("test"),
 	}
@@ -45,17 +45,17 @@ func TestResponseErrorSerialization(t *testing.T) {
 func TestResponseErrorErrorInterface(t *testing.T) {
 	scenarios := []struct {
 		name     string
-		err      s3.ResponseError
+		err      *s3.ResponseError
 		expected string
 	}{
 		{
 			"empty",
-			s3.ResponseError{},
+			&s3.ResponseError{},
 			"0 S3ResponseError",
 		},
 		{
 			"with code and message (nil raw)",
-			s3.ResponseError{
+			&s3.ResponseError{
 				Status:  123,
 				Code:    "test_code",
 				Message: "test_message",
@@ -64,7 +64,7 @@ func TestResponseErrorErrorInterface(t *testing.T) {
 		},
 		{
 			"with code and message (non-nil raw)",
-			s3.ResponseError{
+			&s3.ResponseError{
 				Status:  123,
 				Code:    "test_code",
 				Message: "test_message",

tools/filesystem/internal/s3blob/s3/s3.go

@@ -29,6 +29,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"slices"
 	"strings"
 	"time"
@@ -61,7 +62,7 @@ type S3 struct {
 }
 
 // URL constructs an S3 request URL based on the current configuration.
-func (s3 *S3) URL(key string) string {
+func (s3 *S3) URL(path string) string {
 	scheme := "https"
 	endpoint := strings.TrimRight(s3.Endpoint, "/")
 	if after, ok := strings.CutPrefix(endpoint, "https://"); ok {
@@ -71,13 +72,13 @@ func (s3 *S3) URL(key string) string {
 		scheme = "http"
 	}
 
-	key = strings.TrimLeft(key, "/")
+	path = strings.TrimLeft(path, "/")
 
 	if s3.UsePathStyle {
-		return fmt.Sprintf("%s://%s/%s/%s", scheme, endpoint, s3.Bucket, key)
+		return fmt.Sprintf("%s://%s/%s/%s", scheme, endpoint, s3.Bucket, path)
 	}
 
-	return fmt.Sprintf("%s://%s.%s/%s", scheme, s3.Bucket, endpoint, key)
+	return fmt.Sprintf("%s://%s.%s/%s", scheme, s3.Bucket, endpoint, path)
 }
 
 // SignAndSend signs the provided request per AWS Signature v4 and sends it.
@@ -150,8 +151,8 @@ func (s3 *S3) sign(req *http.Request) {
 
 	canonicalParts := []string{
 		req.Method,
-		req.URL.EscapedPath(),
+		escapePath(req.URL.Path),
-		encodeQuery(req),
+		escapeQuery(req.URL.Query()),
 		canonicalHeaders,
 		signedHeaders,
 		req.Header.Get("x-amz-content-sha256"),
@@ -202,12 +203,6 @@ func (s3 *S3) sign(req *http.Request) {
 	req.Header.Set("authorization", authorization)
 }
 
-// encodeQuery encodes the request query parameters according to the AWS requirements
-// (see UriEncode description in https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html).
-func encodeQuery(req *http.Request) string {
-	return strings.ReplaceAll(req.URL.Query().Encode(), "+", "%20")
-}
-
 func sha256Hex(content []byte) string {
 	h := sha256.New()
 	h.Write(content)
@@ -280,3 +275,96 @@ func extractMetadata(headers http.Header) map[string]string {
 
 	return result
 }
+
+// escapeQuery returns the URI encoded request query parameters according to the AWS S3 spec requirements
+// (it is similar to url.Values.Encode but instead of url.QueryEscape uses our own escape method).
+func escapeQuery(values url.Values) string {
+	if len(values) == 0 {
+		return ""
+	}
+
+	var buf strings.Builder
+
+	keys := make([]string, 0, len(values))
+	for k := range values {
+		keys = append(keys, k)
+	}
+	slices.Sort(keys)
+
+	for _, k := range keys {
+		vs := values[k]
+		keyEscaped := escape(k)
+		for _, values := range vs {
+			if buf.Len() > 0 {
+				buf.WriteByte('&')
+			}
+			buf.WriteString(keyEscaped)
+			buf.WriteByte('=')
+			buf.WriteString(escape(values))
+		}
+	}
+
+	return buf.String()
+}
+
+// escapePath returns the URI encoded request path according to the AWS S3 spec requirments.
+func escapePath(path string) string {
+	parts := strings.Split(path, "/")
+
+	for i, part := range parts {
+		parts[i] = escape(part)
+	}
+
+	return strings.Join(parts, "/")
+}
+
+const upperhex = "0123456789ABCDEF"
+
+// escape is similar to the std url.escape but implements the AWS [UriEncode requirements]:
+//   - URI encode every byte except the unreserved characters: 'A'-'Z', 'a'-'z', '0'-'9', '-', '.', '_', and '~'.
+//   - The space character is a reserved character and must be encoded as "%20" (and not as "+").
+//   - Each URI encoded byte is formed by a '%' and the two-digit hexadecimal value of the byte.
+//   - Letters in the hexadecimal value must be uppercase, for example "%1A".
+//
+// [UriEncode requirements]: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html
+func escape(s string) string {
+	hexCount := 0
+	for i := 0; i < len(s); i++ {
+		c := s[i]
+		if shouldEscape(c) {
+			hexCount++
+		}
+	}
+
+	if hexCount == 0 {
+		return s
+	}
+
+	result := make([]byte, len(s)+2*hexCount)
+
+	j := 0
+	for i := 0; i < len(s); i++ {
+		c := s[i]
+		if shouldEscape(c) {
+			result[j] = '%'
+			result[j+1] = upperhex[c>>4]
+			result[j+2] = upperhex[c&15]
+			j += 3
+		} else {
+			result[j] = c
+			j++
+		}
+	}
+
+	return string(result)
+}
+
+// > "URI encode every byte except the unreserved characters: 'A'-'Z', 'a'-'z', '0'-'9', '-', '.', '_', and '~'."
+func shouldEscape(c byte) bool {
+	isUnreserved := (c >= 'A' && c <= 'Z') ||
+		(c >= 'a' && c <= 'z') ||
+		(c >= '0' && c <= '9') ||
+		c == '-' || c == '.' || c == '_' || c == '~'
+
+	return !isUnreserved
+}

tools/filesystem/internal/s3blob/s3/s3_escape_test.go

@@ -0,0 +1,35 @@
+package s3
+
+import (
+	"net/url"
+	"testing"
+)
+
+func TestEscapePath(t *testing.T) {
+	t.Parallel()
+
+	escaped := escapePath("/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~ !@#$%^&*()+={}[]?><\\|,`'\"/@sub1/@sub2/a/b/c/1/2/3")
+
+	expected := "/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~%20%21%40%23%24%25%5E%26%2A%28%29%2B%3D%7B%7D%5B%5D%3F%3E%3C%5C%7C%2C%60%27%22/%40sub1/%40sub2/a/b/c/1/2/3"
+
+	if escaped != expected {
+		t.Fatalf("Expected\n%s\ngot\n%s", expected, escaped)
+	}
+}
+
+func TestEscapeQuery(t *testing.T) {
+	t.Parallel()
+
+	escaped := escapeQuery(url.Values{
+		"abc": []string{"123"},
+		"/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~ !@#$%^&*()+={}[]?><\\|,`'\"": []string{
+			"/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~ !@#$%^&*()+={}[]?><\\|,`'\"",
+		},
+	})
+
+	expected := "%2FABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~%20%21%40%23%24%25%5E%26%2A%28%29%2B%3D%7B%7D%5B%5D%3F%3E%3C%5C%7C%2C%60%27%22=%2FABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~%20%21%40%23%24%25%5E%26%2A%28%29%2B%3D%7B%7D%5B%5D%3F%3E%3C%5C%7C%2C%60%27%22&abc=123"
+
+	if escaped != expected {
+		t.Fatalf("Expected\n%s\ngot\n%s", expected, escaped)
+	}
+}

tools/filesystem/internal/s3blob/s3/s3_test.go

@@ -98,11 +98,13 @@ func TestS3SignAndSend(t *testing.T) {
 
 	scenarios := []struct {
 		name     string
+		path     string
 		reqFunc  func(req *http.Request)
 		s3Client *s3.S3
 	}{
 		{
 			"minimal",
+			"/test",
 			func(req *http.Request) {
 				req.Header.Set("x-amz-date", "20250102T150405Z")
 			},
@@ -129,6 +131,7 @@ func TestS3SignAndSend(t *testing.T) {
 		},
 		{
 			"minimal with different access and secret keys",
+			"/test",
 			func(req *http.Request) {
 				req.Header.Set("x-amz-date", "20250102T150405Z")
 			},
@@ -153,8 +156,36 @@ func TestS3SignAndSend(t *testing.T) {
 				}),
 			},
 		},
+		{
+			"minimal with special characters",
+			"/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~!@#$^&*()=/@sub?a=1&@b=@2",
+			func(req *http.Request) {
+				req.Header.Set("x-amz-date", "20250102T150405Z")
+			},
+			&s3.S3{
+				Region:    "test_region",
+				Bucket:    "test_bucket",
+				Endpoint:  "https://example.com/",
+				AccessKey: "456",
+				SecretKey: "def",
+				Client: tests.NewClient(&tests.RequestStub{
+					Method:   http.MethodGet,
+					URL:      "https://test_bucket.example.com/ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_.~!@#$%5E&*()=/@sub?a=1&@b=@2",
+					Response: testResponse(),
+					Match: func(req *http.Request) bool {
+						return tests.ExpectHeaders(req.Header, map[string]string{
+							"Authorization":        "AWS4-HMAC-SHA256 Credential=456/20250102/test_region/s3/aws4_request, SignedHeaders=host;x-amz-content-sha256;x-amz-date, Signature=e0001982deef1652704f74503203e77d83d4c88369421f9fca644d96f2a62a3c",
+							"Host":                 "test_bucket.example.com",
+							"X-Amz-Content-Sha256": "UNSIGNED-PAYLOAD",
+							"X-Amz-Date":           "20250102T150405Z",
+						})
+					},
+				}),
+			},
+		},
 		{
 			"with extra headers",
+			"/test",
 			func(req *http.Request) {
 				req.Header.Set("x-amz-date", "20250102T150405Z")
 				req.Header.Set("x-amz-content-sha256", "test_sha256")
@@ -191,7 +222,7 @@ func TestS3SignAndSend(t *testing.T) {
 
 	for _, s := range scenarios {
 		t.Run(s.name, func(t *testing.T) {
-			req, err := http.NewRequest(http.MethodGet, s.s3Client.URL("/test"), strings.NewReader("test_request"))
+			req, err := http.NewRequest(http.MethodGet, s.s3Client.URL(s.path), strings.NewReader("test_request"))
 			if err != nil {
 				t.Fatal(err)
 			}

tools/filesystem/internal/s3blob/s3blob.go

@@ -79,9 +79,13 @@ func (drv *driver) NormalizeError(err error) error {
 		return err
 	}
 
-	// normalize base on its S3 error code
+	// normalize base on its S3 error status or code
-	var ae s3.ResponseError
+	var ae *s3.ResponseError
 	if errors.As(err, &ae) {
+		if ae.Status == 404 {
+			return errors.Join(err, blob.ErrNotFound)
+		}
+
 		switch ae.Code {
 		case "NoSuchBucket", "NoSuchKey", "NotFound":
 			return errors.Join(err, blob.ErrNotFound)

tools/filesystem/internal/s3blob/s3blob_test.go

@@ -99,29 +99,39 @@ func TestDriverNormilizeError(t *testing.T) {
 			errors.New("test"),
 			false,
 		},
+		{
+			"response error with only status (non-404)",
+			&s3.ResponseError{Status: 123},
+			false,
+		},
+		{
+			"response error with only status (404)",
+			&s3.ResponseError{Status: 404},
+			true,
+		},
 		{
 			"response error with custom code",
-			s3.ResponseError{Code: "test"},
+			&s3.ResponseError{Code: "test"},
 			false,
 		},
 		{
 			"response error with NoSuchBucket code",
-			s3.ResponseError{Code: "NoSuchBucket"},
+			&s3.ResponseError{Code: "NoSuchBucket"},
 			true,
 		},
 		{
 			"response error with NoSuchKey code",
-			s3.ResponseError{Code: "NoSuchKey"},
+			&s3.ResponseError{Code: "NoSuchKey"},
 			true,
 		},
 		{
 			"response error with NotFound code",
-			s3.ResponseError{Code: "NotFound"},
+			&s3.ResponseError{Code: "NotFound"},
 			true,
 		},
 		{
 			"wrapped response error with NotFound code", // ensures that the entire error's tree is checked
-			fmt.Errorf("test: %w", s3.ResponseError{Code: "NotFound"}),
+			fmt.Errorf("test: %w", &s3.ResponseError{Code: "NotFound"}),
 			true,
 		},
 		{