Skip to content

Commit 0df27d7

Browse files
committed
Fix SigV4 signing for S3 keys containing colons
url.PathEscape does not encode colons (valid per RFC 3986), but AWS SigV4 requires only unreserved characters (A-Za-z0-9-._~) in the canonical URI. Keys like "app:assembleDebug" caused signature mismatches resulting in 403s silently treated as cache misses. Replace url.PathEscape with s3PathEscape using the AWS unreserved set. Add wrapper preservation checks to the integration test.
1 parent 6f2eefb commit 0df27d7

3 files changed

Lines changed: 56 additions & 2 deletions

File tree

cmd/gradle-cache/integration_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,10 @@ func TestIntegrationGradleBuildCycle(t *testing.T) {
213213
t.Fatalf("expected caches dir after restore: %v", err)
214214
}
215215

216+
if _, err := os.Stat(filepath.Join(gradleUserHome, "wrapper")); err != nil {
217+
t.Fatalf("expected wrapper dir after restore: %v", err)
218+
}
219+
216220
ccRestored := filepath.Join(projectDir, ".gradle", "configuration-cache")
217221
if _, err := os.Stat(ccRestored); err != nil {
218222
t.Log(" configuration-cache dir was NOT restored")
@@ -224,6 +228,12 @@ func TestIntegrationGradleBuildCycle(t *testing.T) {
224228
t.Log("Step 5: Rebuilding to verify cache hits...")
225229
output := gradleRun(t, projectDir, gradlew, gradleUserHome, "build")
226230

231+
// Verify the wrapper was NOT re-downloaded — if it was, the bundle didn't
232+
// include the wrapper directory (or the .ok marker file was missing).
233+
if strings.Contains(output, "Downloading") {
234+
t.Error("Gradle re-downloaded the wrapper distribution after restore; wrapper/ should be cached in the bundle")
235+
}
236+
227237
if strings.Contains(output, "Reusing configuration cache") {
228238
t.Log(" Configuration cache: reused")
229239
} else {

cmd/gradle-cache/main_test.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,27 @@ func TestBundleFilename(t *testing.T) {
6363
}
6464
}
6565

66+
func TestS3PathEscape(t *testing.T) {
67+
tests := []struct {
68+
input, want string
69+
}{
70+
{"simple", "simple"},
71+
{"apos-beta", "apos-beta"},
72+
{"address-typeahead-sample:assembleDebug", "address-typeahead-sample%3AassembleDebug"},
73+
{"a:b:c", "a%3Ab%3Ac"},
74+
{":leadingColon", "%3AleadingColon"},
75+
{"file.tar.zst", "file.tar.zst"},
76+
{"with spaces", "with%20spaces"},
77+
{"tilde~ok", "tilde~ok"},
78+
{"hash#bad", "hash%23bad"},
79+
}
80+
for _, tt := range tests {
81+
if got := s3PathEscape(tt.input); got != tt.want {
82+
t.Errorf("s3PathEscape(%q) = %q, want %q", tt.input, got, tt.want)
83+
}
84+
}
85+
}
86+
6687
func TestS3Key(t *testing.T) {
6788
tests := []struct {
6889
commit, cacheKey, bundleFile, want string
@@ -463,8 +484,10 @@ func TestTarZstdRoundTrip(t *testing.T) {
463484
must(t, os.WriteFile(filepath.Join(gradleHome, "caches", "8.14.3", "cc-keystore", "keystore.bin"), []byte("secret"), 0o644))
464485

465486
// wrapper/ source (under gradle-home) — includes a .zip that should be excluded
487+
// and a .ok marker file that must be preserved (Gradle checks for it to skip re-downloading)
466488
must(t, os.MkdirAll(filepath.Join(gradleHome, "wrapper", "dists", "gradle-8.14.3-bin", "abc123"), 0o755))
467489
must(t, os.WriteFile(filepath.Join(gradleHome, "wrapper", "dists", "gradle-8.14.3-bin", "abc123", "gradle-8.14.3-bin.zip"), []byte("should be excluded"), 0o644))
490+
must(t, os.WriteFile(filepath.Join(gradleHome, "wrapper", "dists", "gradle-8.14.3-bin", "abc123", "gradle-8.14.3-bin.zip.ok"), []byte(""), 0o644))
468491
must(t, os.MkdirAll(filepath.Join(gradleHome, "wrapper", "dists", "gradle-8.14.3-bin", "abc123", "gradle-8.14.3", "lib"), 0o755))
469492
must(t, os.WriteFile(filepath.Join(gradleHome, "wrapper", "dists", "gradle-8.14.3-bin", "abc123", "gradle-8.14.3", "lib", "gradle-core.jar"), []byte("wrapper data"), 0o644))
470493

@@ -495,6 +518,7 @@ func TestTarZstdRoundTrip(t *testing.T) {
495518
for _, rel := range []string{
496519
"caches/modules/entry.bin",
497520
"wrapper/dists/gradle-8.14.3-bin/abc123/gradle-8.14.3/lib/gradle-core.jar",
521+
"wrapper/dists/gradle-8.14.3-bin/abc123/gradle-8.14.3-bin.zip.ok",
498522
"configuration-cache/hash.bin",
499523
} {
500524
path := filepath.Join(dstDir, rel)

cmd/gradle-cache/s3.go

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,8 @@ func (c *s3Client) putStreamingMultipart(ctx context.Context, bucket, key string
516516
}
517517

518518
// objectURL returns the virtual-hosted S3 URL for the given bucket and key.
519-
// Each path segment of the key is percent-encoded per RFC 3986.
519+
// Each path segment is percent-encoded using the AWS SigV4 URI encoding rules
520+
// (only A-Za-z0-9 - . _ ~ are left unencoded).
520521
func (c *s3Client) objectURL(bucket, key string) string {
521522
var sb strings.Builder
522523
sb.WriteString("https://")
@@ -526,7 +527,26 @@ func (c *s3Client) objectURL(bucket, key string) string {
526527
sb.WriteString(".amazonaws.com")
527528
for _, seg := range strings.Split(key, "/") {
528529
sb.WriteByte('/')
529-
sb.WriteString(url.PathEscape(seg))
530+
sb.WriteString(s3PathEscape(seg))
531+
}
532+
return sb.String()
533+
}
534+
535+
// s3PathEscape percent-encodes a single path segment using the AWS SigV4
536+
// URI encoding rules: unreserved characters (A-Za-z0-9 - . _ ~) are left
537+
// as-is; everything else (including colons) is percent-encoded.
538+
// This is stricter than url.PathEscape which also leaves sub-delimiters
539+
// like : ; @ ! unencoded.
540+
func s3PathEscape(s string) string {
541+
var sb strings.Builder
542+
for i := 0; i < len(s); i++ {
543+
c := s[i]
544+
if (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') ||
545+
c == '-' || c == '.' || c == '_' || c == '~' {
546+
sb.WriteByte(c)
547+
} else {
548+
fmt.Fprintf(&sb, "%%%02X", c)
549+
}
530550
}
531551
return sb.String()
532552
}

0 commit comments

Comments
 (0)