Commit cde4a2d1 authored by Kamil Trzciński's avatar Kamil Trzciński

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

Disable escaping project bucket in cache operations

Closes #3589

See merge request !1029
parents 73ba801b fdec6dfc
Pipeline #31040107 passed with stages
in 55 minutes and 41 seconds
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
......
......@@ -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)
}
func TestGetCacheObjectNameWhenPathIsSetThenUrlContainsIt(t *testing.T) {
cache := defaultCacheConfig()
cache.Path = "whatever"
cacheBuild := defaultBuild(cache)
url := generateObjectName(cacheBuild, cache, "key")
assert.Equal(t, "whatever/runner/longtoke/project/10/key", url)
}
type generateObjectNameTestCase struct {
cache *common.CacheConfig
build *common.Build
func TestGetCacheObjectNameWhenPathHasMultipleSegmentIsSetThenUrlContainsIt(t *testing.T) {
cache := defaultCacheConfig()
cache.Path = "some/other/path/goes/here"
cacheBuild := defaultBuild(cache)
key string
path string
shared bool
url := generateObjectName(cacheBuild, cache, "key")
assert.Equal(t, "some/other/path/goes/here/runner/longtoke/project/10/key", url)
expectedObjectName string
expectedError string
}
func TestGetCacheObjectNameWhenPathIsNotSetThenUrlDoesNotContainIt(t *testing.T) {
func TestGenerateObjectName(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)
}
})
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment