From f4b0a7cfc95aba6a27e9aaf06a57716fc2057c76 Mon Sep 17 00:00:00 2001 From: guscarreon Date: Thu, 20 Aug 2020 14:19:37 -0400 Subject: [PATCH] Validate External Cache Host (#1422) * first draft * Little tweaks * Scott's review part 1 * Scott's review corrections part 2 * Scotts refactor * correction in config_test.go * Correction and refactor * Multiple return statements * Test case refactor Co-authored-by: Gus Carreon Co-authored-by: Gus Carreon Co-authored-by: Gus Carreon --- config/config.go | 36 +++++++++++++++++ config/config_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/config/config.go b/config/config.go index 9e6b1370128..e3b7d8ebda0 100755 --- a/config/config.go +++ b/config/config.go @@ -2,6 +2,7 @@ package config import ( "bytes" + "errors" "fmt" "net/url" "reflect" @@ -111,6 +112,7 @@ func (cfg *Configuration) validate() configErrors { errs = cfg.CurrencyConverter.validate(errs) errs = validateAdapters(cfg.Adapters, errs) errs = cfg.Debug.validate(errs) + errs = cfg.ExtCacheURL.validate(errs) return errs } @@ -128,6 +130,40 @@ func (cfg *AuctionTimeouts) validate(errs configErrors) configErrors { return errs } +func (data *ExternalCache) validate(errs configErrors) configErrors { + if data.Host == "" && data.Path == "" { + // Both host and path can be blank. No further validation needed + return errs + } + + // Either host or path or both not empty, validate. + if data.Host == "" && data.Path != "" || data.Host != "" && data.Path == "" { + return append(errs, errors.New("External cache Host and Path must both be specified")) + } + if strings.HasSuffix(data.Host, "/") { + return append(errs, errors.New(fmt.Sprintf("External cache Host '%s' must not end with a path separator", data.Host))) + } + if strings.ContainsAny(data.Host, "://") { + return append(errs, errors.New(fmt.Sprintf("External cache Host must not specify a protocol. '%s'", data.Host))) + } + if !strings.HasPrefix(data.Path, "/") { + return append(errs, errors.New(fmt.Sprintf("External cache Path '%s' must begin with a path separator", data.Path))) + } + + urlObj, err := url.Parse("https://" + data.Host + data.Path) + if err != nil { + return append(errs, errors.New(fmt.Sprintf("External cache Path validation error: %s ", err.Error()))) + } + if urlObj.Host != data.Host { + return append(errs, errors.New(fmt.Sprintf("External cache Host '%s' is invalid", data.Host))) + } + if urlObj.Path != data.Path { + return append(errs, errors.New("External cache Path is invalid")) + } + + return errs +} + // LimitAuctionTimeout returns the min of requested or cfg.MaxAuctionTimeout. // Both values treat "0" as "infinite". func (cfg *AuctionTimeouts) LimitAuctionTimeout(requested time.Duration) time.Duration { diff --git a/config/config_test.go b/config/config_test.go index 4774d9d6e46..3da3f72137b 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -14,6 +14,91 @@ import ( "github.com/stretchr/testify/assert" ) +func TestExternalCacheURLValidate(t *testing.T) { + testCases := []struct { + desc string + data ExternalCache + expErrors int + }{ + { + desc: "With http://", + data: ExternalCache{Host: "http://www.google.com", Path: "/path/v1"}, + expErrors: 1, + }, + { + desc: "Without http://", + data: ExternalCache{Host: "www.google.com", Path: "/path/v1"}, + expErrors: 0, + }, + { + desc: "No scheme but '//' prefix", + data: ExternalCache{Host: "//www.google.com", Path: "/path/v1"}, + expErrors: 1, + }, + { + desc: "// appears twice", + data: ExternalCache{Host: "//www.google.com//", Path: "path/v1"}, + expErrors: 1, + }, + { + desc: "Host has an only // value", + data: ExternalCache{Host: "//", Path: "path/v1"}, + expErrors: 1, + }, + { + desc: "only scheme host, valid path", + data: ExternalCache{Host: "http://", Path: "/path/v1"}, + expErrors: 1, + }, + { + desc: "No host, path only", + data: ExternalCache{Host: "", Path: "path/v1"}, + expErrors: 1, + }, + { + desc: "No host, nor path", + data: ExternalCache{Host: "", Path: ""}, + expErrors: 0, + }, + { + desc: "Invalid http at the end", + data: ExternalCache{Host: "www.google.com", Path: "http://"}, + expErrors: 1, + }, + { + desc: "Host has an unknown scheme", + data: ExternalCache{Host: "unknownscheme://host", Path: "/path/v1"}, + expErrors: 1, + }, + { + desc: "Wrong colon side in scheme", + data: ExternalCache{Host: "http//:www.appnexus.com", Path: "/path/v1"}, + expErrors: 1, + }, + { + desc: "Missing '/' in scheme", + data: ExternalCache{Host: "http:/www.appnexus.com", Path: "/path/v1"}, + expErrors: 1, + }, + { + desc: "host with scheme, no path", + data: ExternalCache{Host: "http://www.appnexus.com", Path: ""}, + expErrors: 1, + }, + { + desc: "scheme, no host nor path", + data: ExternalCache{Host: "http://", Path: ""}, + expErrors: 1, + }, + } + for _, test := range testCases { + var errs configErrors + errs = test.data.validate(errs) + + assert.Equal(t, test.expErrors, len(errs), "Test case threw unexpected number of errors. Desc: %s errMsg = %v \n", test.desc, errs) + } +} + func TestDefaults(t *testing.T) { v := viper.New() SetupViper(v, "") @@ -66,7 +151,7 @@ cache: query: uuid=%PBS_CACHE_UUID% external_cache: host: www.externalprebidcache.net - path: endpoints/cache + path: /endpoints/cache http_client: max_connections_per_host: 10 max_idle_connections: 500 @@ -223,7 +308,7 @@ func TestFullConfig(t *testing.T) { cmpStrings(t, "cache.host", cfg.CacheURL.Host, "prebidcache.net") cmpStrings(t, "cache.query", cfg.CacheURL.Query, "uuid=%PBS_CACHE_UUID%") cmpStrings(t, "external_cache.host", cfg.ExtCacheURL.Host, "www.externalprebidcache.net") - cmpStrings(t, "external_cache.path", cfg.ExtCacheURL.Path, "endpoints/cache") + cmpStrings(t, "external_cache.path", cfg.ExtCacheURL.Path, "/endpoints/cache") cmpInts(t, "http_client.max_connections_per_host", cfg.Client.MaxConnsPerHost, 10) cmpInts(t, "http_client.max_idle_connections", cfg.Client.MaxIdleConns, 500) cmpInts(t, "http_client.max_idle_connections_per_host", cfg.Client.MaxIdleConnsPerHost, 20)