Merge branch 'fix-cache-key-escape-for-11-3' into 'master'

Disable escaping project bucket in cache operations

Closes #3589

See merge request gitlab-org/gitlab-runner!1029

cache/cache.go

 package cache
 
 import (
+	"fmt"
 	"net/url"
 	"path"
+	"path/filepath"
 	"strconv"
+	"strings"
 
 	"github.com/sirupsen/logrus"
 
@@ -20,17 +23,33 @@ func getCacheConfig(build *common.Build) *common.CacheConfig {
 	return build.Runner.Cache
 }
 
-func generateObjectName(build *common.Build, config *common.CacheConfig, key string) string {
-	if key == "" {
-		return ""
-	}
-
+func generateBaseObjectName(build *common.Build, config *common.CacheConfig) string {
 	runnerSegment := ""
 	if !config.GetShared() {
 		runnerSegment = path.Join("runner", build.Runner.ShortDescription())
 	}
 
-	return path.Join(config.GetPath(), runnerSegment, "project", strconv.Itoa(build.JobInfo.ProjectID), key)
+	return path.Join(config.GetPath(), runnerSegment, "project", strconv.Itoa(build.JobInfo.ProjectID))
+}
+
+func generateObjectName(build *common.Build, config *common.CacheConfig, key string) (string, error) {
+	if key == "" {
+		return "", nil
+	}
+
+	basePath := generateBaseObjectName(build, config)
+	path := path.Join(basePath, key)
+
+	relative, err := filepath.Rel(basePath, path)
+	if err != nil {
+		return "", fmt.Errorf("cache path correctness check failed with: %v", err)
+	}
+
+	if strings.HasPrefix(relative, ".."+string(filepath.Separator)) {
+		return "", fmt.Errorf("computed cache path outside of project bucket. Please remove `../` from cache key")
+	}
+
+	return path, nil
 }
 
 func onAdapter(build *common.Build, key string, handler func(adapter Adapter) *url.URL) *url.URL {
@@ -40,7 +59,12 @@ func onAdapter(build *common.Build, key string, handler func(adapter Adapter) *u
 		return nil
 	}
 
-	objectName := generateObjectName(build, config, key)
+	objectName, err := generateObjectName(build, config, key)
+	if err != nil {
+		logrus.WithError(err).Error("Error while generating cache bucket.")
+		return nil
+	}
+
 	if objectName == "" {
 		logrus.Warning("Empty cache key. Skipping adapter selection.")
 		return nil

cache/cache_test.go

@@ -173,63 +173,92 @@ func defaultBuild(cacheConfig *common.CacheConfig) *common.Build {
 	}
 }
 
-func TestGenerateObjectNameWhenKeyIsEmptyResultIsAlsoEmpty(t *testing.T) {
-	cache := defaultCacheConfig()
-	cacheBuild := defaultBuild(cache)
-
-	url := generateObjectName(cacheBuild, cache, "")
-	assert.Empty(t, url)
-}
-
-func TestGetCacheObjectName(t *testing.T) {
-	cache := defaultCacheConfig()
-	cacheBuild := defaultBuild(cache)
-
-	url := generateObjectName(cacheBuild, cache, "key")
-	assert.Equal(t, "runner/longtoke/project/10/key", url)
-}
+type generateObjectNameTestCase struct {
+	cache *common.CacheConfig
+	build *common.Build
 
-func TestGetCacheObjectNameWhenPathIsSetThenUrlContainsIt(t *testing.T) {
-	cache := defaultCacheConfig()
-	cache.Path = "whatever"
-	cacheBuild := defaultBuild(cache)
+	key    string
+	path   string
+	shared bool
 
-	url := generateObjectName(cacheBuild, cache, "key")
-	assert.Equal(t, "whatever/runner/longtoke/project/10/key", url)
+	expectedObjectName string
+	expectedError      string
 }
 
-func TestGetCacheObjectNameWhenPathHasMultipleSegmentIsSetThenUrlContainsIt(t *testing.T) {
+func TestGenerateObjectName(t *testing.T) {
 	cache := defaultCacheConfig()
-	cache.Path = "some/other/path/goes/here"
 	cacheBuild := defaultBuild(cache)
 
-	url := generateObjectName(cacheBuild, cache, "key")
-	assert.Equal(t, "some/other/path/goes/here/runner/longtoke/project/10/key", url)
-}
-
-func TestGetCacheObjectNameWhenPathIsNotSetThenUrlDoesNotContainIt(t *testing.T) {
-	cache := defaultCacheConfig()
-	cache.Path = ""
-	cacheBuild := defaultBuild(cache)
-
-	url := generateObjectName(cacheBuild, cache, "key")
-	assert.Equal(t, "runner/longtoke/project/10/key", url)
-}
-
-func TestGetCacheObjectNameWhenSharedFlagIsFalseThenRunnerSegmentExistsInTheUrl(t *testing.T) {
-	cache := defaultCacheConfig()
-	cache.Shared = false
-	cacheBuild := defaultBuild(cache)
+	tests := map[string]generateObjectNameTestCase{
+		"default usage": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "key",
+			expectedObjectName: "runner/longtoke/project/10/key",
+		},
+		"empty key": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "",
+			expectedObjectName: "",
+		},
+		"short path is set": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "key",
+			path:               "whatever",
+			expectedObjectName: "whatever/runner/longtoke/project/10/key",
+		},
+		"multiple segment path is set": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "key",
+			path:               "some/other/path/goes/here",
+			expectedObjectName: "some/other/path/goes/here/runner/longtoke/project/10/key",
+		},
+		"path is empty": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "key",
+			path:               "",
+			expectedObjectName: "runner/longtoke/project/10/key",
+		},
+		"shared flag is set to true": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "key",
+			shared:             true,
+			expectedObjectName: "project/10/key",
+		},
+		"shared flag is set to false": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "key",
+			shared:             false,
+			expectedObjectName: "runner/longtoke/project/10/key",
+		},
+		"key escapes project namespace": {
+			cache:              cache,
+			build:              cacheBuild,
+			key:                "../9/key",
+			expectedObjectName: "",
+			expectedError:      "computed cache path outside of project bucket. Please remove `../` from cache key",
+		},
+	}
 
-	url := generateObjectName(cacheBuild, cache, "key")
-	assert.Equal(t, "runner/longtoke/project/10/key", url)
-}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			cache.Path = test.path
+			cache.Shared = test.shared
 
-func TestGetCacheObjectNameWhenSharedFlagIsFalseThenRunnerSegmentShouldNotBePresent(t *testing.T) {
-	cache := defaultCacheConfig()
-	cache.Shared = true
-	cacheBuild := defaultBuild(cache)
+			objectName, err := generateObjectName(test.build, test.cache, test.key)
 
-	url := generateObjectName(cacheBuild, cache, "key")
-	assert.Equal(t, "project/10/key", url)
+			assert.Equal(t, test.expectedObjectName, objectName)
+			if len(test.expectedError) == 0 {
+				assert.NoError(t, err)
+			} else {
+				assert.EqualError(t, err, test.expectedError)
+			}
+		})
+	}
 }