From 687ef9bc62a0531bcea0a14e673fcf09551fdf40 Mon Sep 17 00:00:00 2001 From: Nick Randall Date: Sun, 7 Jul 2019 06:12:53 -0400 Subject: [PATCH] add validation to allowed-origins to include path (#265) * add validation to allowed-origins to include path * Origins with a path receive a trailing '/' and added tests for origins with paths * [NR] add some documentation on how works --- README.md | 15 ++++++- imaginary.go | 7 ++- source_http.go | 10 ++--- source_http_test.go | 104 +++++++++++++++++++++++++++++++++++++++----- 4 files changed, 117 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 7633c0ea..0a2edda2 100644 --- a/README.md +++ b/README.md @@ -318,7 +318,7 @@ Options: -forward-headers Forwards custom headers to the image source server. -enable-url-source flag must be defined. -enable-url-signature Enable URL signature (URL-safe Base64-encoded HMAC digest) [default: false] -url-signature-key The URL signature key (32 characters minimum) - -allowed-origins Restrict remote image source processing to certain origins (separated by commas) + -allowed-origins Restrict remote image source processing to certain origins (separated by commas). Note: Origins are validated against host *AND* path. -max-allowed-size Restrict maximum size of http image source (in bytes) -certfile TLS certificate file path -keyfile TLS private key file path @@ -441,6 +441,19 @@ curl -O "http://localhost:8088/crop?width=500&height=200&gravity=smart&url=https ## HTTP API +### Allowed Origins + +imaginary can be configured to block all requests for images with a src URL this is not specified in the `allowed-origins` list. Imaginary will validate that the remote url matches the hostname and path of at least one origin in allowed list. Perhaps the easiest way to show how this works is to show some examples. + +| `allowed-origins` setting | image url | is valid | +| ------------------------- | --------- | -------- | +| `--allowed-origns s3.amazonaws.com/some-bucket/` | `s3.amazonaws.com/some-bucket/images/image.png` | VALID | +| `--allowed-origns s3.amazonaws.com/some-bucket/` | `s3.amazonaws.com/images/image.png` | NOT VALID (no matching basepath) | +| `--allowed-origns *.amazonaws.com/some-bucket/` | `anysubdomain.amazonaws.com/some-bucket/images/image.png` | VALID | +| `--allowed-origns *.amazonaws.com` | `anysubdomain.amazonaws.comimages/image.png` | VALID | +| `--allowed-origns *.amazonaws.com` | `www.notaws.comimages/image.png` | NOT VALID (no matching host) | +| `--allowed-origns *.amazonaws.com, foo.amazonaws.com/some-bucket/` | `bar.amazonaws.com/some-other-bucket/image.png` | VALID (matches first condition but not second) | + ### Authorization imaginary supports a simple token-based API authorization. diff --git a/imaginary.go b/imaginary.go index ac13b20c..f38da133 100644 --- a/imaginary.go +++ b/imaginary.go @@ -31,7 +31,7 @@ var ( aEnablePlaceholder = flag.Bool("enable-placeholder", false, "Enable image response placeholder to be used in case of error") aEnableURLSignature = flag.Bool("enable-url-signature", false, "Enable URL signature (URL-safe Base64-encoded HMAC digest)") aURLSignatureKey = flag.String("url-signature-key", "", "The URL signature key (32 characters minimum)") - aAllowedOrigins = flag.String("allowed-origins", "", "Restrict remote image source processing to certain origins (separated by commas)") + aAllowedOrigins = flag.String("allowed-origins", "", "Restrict remote image source processing to certain origins (separated by commas). Note: Origins are validated against host *AND* path.") aMaxAllowedSize = flag.Int("max-allowed-size", 0, "Restrict maximum size of http image source (in bytes)") aKey = flag.String("key", "", "Define API key for authorization") aMount = flag.String("mount", "", "Mount server local directory") @@ -292,6 +292,11 @@ func parseOrigins(origins string) []*url.URL { if err != nil { continue } + + if u.Path != "" && u.Path[len(u.Path)-1:] != "/" { + u.Path += "/" + } + urls = append(urls, u) } return urls diff --git a/source_http.go b/source_http.go index 004ae87a..6cdbbd54 100644 --- a/source_http.go +++ b/source_http.go @@ -30,7 +30,7 @@ func (s *HTTPImageSource) GetImage(req *http.Request) ([]byte, error) { return nil, ErrInvalidImageURL } if shouldRestrictOrigin(url, s.Config.AllowedOrigins) { - return nil, fmt.Errorf("not allowed remote URL origin: %s", url.Host) + return nil, fmt.Errorf("not allowed remote URL origin: %s%s", url.Host, url.Path) } return s.fetchImage(url, req) } @@ -89,7 +89,7 @@ func (s *HTTPImageSource) setAuthorizationHeader(req *http.Request, ireq *http.R func (s *HTTPImageSource) setForwardHeaders(req *http.Request, ireq *http.Request) { headers := s.Config.ForwardHeaders for _, header := range headers { - if _,ok := ireq.Header[header]; ok { + if _, ok := ireq.Header[header]; ok { req.Header.Set(header, ireq.Header.Get(header)) } } @@ -123,19 +123,19 @@ func shouldRestrictOrigin(url *url.URL, origins []*url.URL) bool { for _, origin := range origins { if origin.Host == url.Host { - return false + return !strings.HasPrefix(url.Path, origin.Path) } if origin.Host[0:2] == "*." { // Testing if "*.example.org" matches "example.org" if url.Host == origin.Host[2:] { - return false + return !strings.HasPrefix(url.Path, origin.Path) } // Testing if "*.example.org" matches "foo.example.org" if strings.HasSuffix(url.Host, origin.Host[1:]) { - return false + return !strings.HasPrefix(url.Path, origin.Path) } } } diff --git a/source_http_test.go b/source_http_test.go index e558065d..aebd11b4 100755 --- a/source_http_test.go +++ b/source_http_test.go @@ -179,9 +179,9 @@ func TestHttpImageSourceForwardedHeadersNotOverride(t *testing.T) { url := createURL("http://bar.com", t) - r, _ := http.NewRequest(http.MethodGet, "http://foo/bar?url="+url.String() , nil) + r, _ := http.NewRequest(http.MethodGet, "http://foo/bar?url="+url.String(), nil) r.Header.Set("Authorization", "foobar") - + source := &HTTPImageSource{&SourceConfig{Authorization: "ValidAPIKey", ForwardHeaders: cases}} if !source.Matches(r) { t.Fatal("Cannot match the request") @@ -298,16 +298,18 @@ func TestHttpImageSourceExceedsMaximumAllowedLength(t *testing.T) { } func TestShouldRestrictOrigin(t *testing.T) { - plainOrigins := []*url.URL{ - createURL("https://example.org", t), - } + plainOrigins := parseOrigins( + "https://example.org", + ) - wildCardOrigins := []*url.URL{ - createURL("https://localhost", t), - createURL("https://*.example.org", t), - createURL("https://some.s3.bucket.on.aws.org", t), - createURL("https://*.s3.bucket.on.aws.org", t), - } + wildCardOrigins := parseOrigins( + "https://localhost,https://*.example.org,https://some.s3.bucket.on.aws.org,https://*.s3.bucket.on.aws.org", + ) + + withPathOrigins := parseOrigins( + "https://localhost/foo/bar/,https://*.example.org/foo/,https://some.s3.bucket.on.aws.org/my/bucket/," + + "https://*.s3.bucket.on.aws.org/my/bucket/,https://no-leading-path-slash.example.org/assets", + ) t.Run("Plain origin", func(t *testing.T) { testURL := createURL("https://example.org/logo.jpg", t) @@ -334,7 +336,7 @@ func TestShouldRestrictOrigin(t *testing.T) { }) t.Run("Wildcard origin, sub-sub domain URL", func(t *testing.T) { - testURL := createURL("https://n.s3.bucket.on.aws.org/logo.jpg", t) + testURL := createURL("https://n.s3.bucket.on.aws.org/our/bucket/logo.jpg", t) if shouldRestrictOrigin(testURL, wildCardOrigins) { t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, wildCardOrigins) @@ -352,6 +354,84 @@ func TestShouldRestrictOrigin(t *testing.T) { t.Errorf("Expected '%s' to not be allowed with wildcard origins: %+v", testURL, wildCardOrigins) } }) + + t.Run("Loopback origin with path, correct URL", func(t *testing.T) { + testURL := createURL("https://localhost/foo/bar/logo.png", t) + + if shouldRestrictOrigin(testURL, withPathOrigins) { + t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, withPathOrigins) + } + }) + + t.Run("Wildcard origin with path, correct URL", func(t *testing.T) { + testURL := createURL("https://our.company.s3.bucket.on.aws.org/my/bucket/logo.gif", t) + + if shouldRestrictOrigin(testURL, withPathOrigins) { + t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, withPathOrigins) + } + }) + + t.Run("Wildcard origin with partial path, correct URL", func(t *testing.T) { + testURL := createURL("https://our.company.s3.bucket.on.aws.org/my/bucket/a/b/c/d/e/logo.gif", t) + + if shouldRestrictOrigin(testURL, withPathOrigins) { + t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, withPathOrigins) + } + }) + + t.Run("Wildcard origin with partial path, correct URL double slashes", func(t *testing.T) { + testURL := createURL("https://static.example.org/foo//a//b//c/d/e/logo.webp", t) + + if shouldRestrictOrigin(testURL, withPathOrigins) { + t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, withPathOrigins) + } + }) + + t.Run("Wildcard origin with path missing trailing slash, correct URL", func(t *testing.T) { + testURL := createURL("https://no-leading-path-slash.example.org/assets/logo.webp", t) + + if shouldRestrictOrigin(testURL, parseOrigins("https://*.example.org/assets")) { + t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, withPathOrigins) + } + }) + + t.Run("Loopback origin with path, incorrect URL", func(t *testing.T) { + testURL := createURL("https://localhost/wrong/logo.png", t) + + if !shouldRestrictOrigin(testURL, withPathOrigins) { + t.Errorf("Expected '%s' to be allowed with origins: %+v", testURL, withPathOrigins) + } + }) +} + +func TestParseOrigins(t *testing.T) { + t.Run("Appending a trailing slash on paths", func(t *testing.T) { + origins := parseOrigins("http://foo.example.org/assets") + if origins[0].Path != "/assets/" { + t.Errorf("Expected the path to have a trailing /, instead it was: %q", origins[0].Path) + } + }) + + t.Run("Paths should not receive multiple trailing slashes", func(t *testing.T) { + origins := parseOrigins("http://foo.example.org/assets/") + if origins[0].Path != "/assets/" { + t.Errorf("Expected the path to have a single trailing /, instead it was: %q", origins[0].Path) + } + }) + + t.Run("Empty paths are fine", func(t *testing.T) { + origins := parseOrigins("http://foo.example.org") + if origins[0].Path != "" { + t.Errorf("Expected the path to remain empty, instead it was: %q", origins[0].Path) + } + }) + + t.Run("Root paths are fine", func(t *testing.T) { + origins := parseOrigins("http://foo.example.org/") + if origins[0].Path != "/" { + t.Errorf("Expected the path to remain a slash, instead it was: %q", origins[0].Path) + } + }) } func createURL(urlStr string, t *testing.T) *url.URL {