Repository: labstack/echo
Pull request #1651 (2020-10-09 09:29:47)

CORS: add an optional custom function to validate the origin
The CORS middleware is great and works very well if the list of allowed origins is static. However, in many use cases, this list need to be dynamic (for example, if coming from the database). This is a non-breaking change that makes the CORS middleware accepting an optional custom function to validate the origin.

PS: the diff looks weird for some unknown reason but the code change is minimal (mostly indentation).

Thanks!

codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=h1) Report
> Merging [#1651](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=desc) into [master](https://codecov.io/gh/labstack/echo/commit/151ed6b3f150163352985448b5630ab69de40aa5?el=desc) will **decrease** coverage by `1.33%`.
> The diff coverage is `91.30%`.

[![Impacted file tree graph](https://codecov.io/gh/labstack/echo/pull/1651/graphs/tree.svg?width=650&height=150&src=pr&token=vqnT76l79i)](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1651      +/-   ##
==========================================
- Coverage   85.28%   83.95%   -1.34%     
==========================================
  Files          28       28              
  Lines        2216     1913     -303     
==========================================
- Hits         1890     1606     -284     
+ Misses        212      194      -18     
+ Partials      114      113       -1     
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [middleware/cors.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9jb3JzLmdv) | `82.66% <91.30%> (+3.21%)` | :arrow_up: |
| [middleware/method\_override.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9tZXRob2Rfb3ZlcnJpZGUuZ28=) | `80.95% <0.00%> (-3.67%)` | :arrow_down: |
| [context.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-Y29udGV4dC5nbw==) | `86.38% <0.00%> (-3.34%)` | :arrow_down: |
| [middleware/redirect.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9yZWRpcmVjdC5nbw==) | `85.00% <0.00%> (-3.24%)` | :arrow_down: |
| [middleware/proxy.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9wcm94eS5nbw==) | `63.21% <0.00%> (-3.12%)` | :arrow_down: |
| [middleware/basic\_auth.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9iYXNpY19hdXRoLmdv) | `65.62% <0.00%> (-2.95%)` | :arrow_down: |
| [echo.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-ZWNoby5nbw==) | `83.58% <0.00%> (-2.36%)` | :arrow_down: |
| [middleware/util.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS91dGlsLmdv) | `88.88% <0.00%> (-1.74%)` | :arrow_down: |
| [response.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-cmVzcG9uc2UuZ28=) | `85.18% <0.00%> (-1.66%)` | :arrow_down: |
| [middleware/key\_auth.go](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9rZXlfYXV0aC5nbw==) | `66.66% <0.00%> (-1.13%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/labstack/echo/pull/1651/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=footer). Last update [151ed6b...3ddb825](https://codecov.io/gh/labstack/echo/pull/1651?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

shoenseiwaso: Codecov appears broken, it's saying coverage was reduced by 284 lines in files completely unrelated to the change. See also [this 3 line `README.md` change](https://github.com/labstack/echo/pull/1646) with a similar code coverage result. Echo maintainers, let us know if we've missed something here!
dahu33: @pafuent thank you very much for the review! @lammel, @vishr, any chance this could merged (I cannot merge myself) and land in the next release? 
dahu33: @lammel thanks the review. I have implemented your feedback and opened a PR here https://github.com/labstack/echox/pull/164 to document the feature. Could you please have a look?
lammel: Seems I was too eager merging another CORS related issue first.
Could you rebase please, will merge it than.
dahu33: > Seems I was too eager merging another CORS related issue first.
> Could you rebase please, will merge it than.

Done! The diff looks weird for some reason but the code change is the same as before.

Patch:
diff --git a/middleware/cors.go b/middleware/cors.go
index 07df0e5..d6ef896 100644
--- a/middleware/cors.go
+++ b/middleware/cors.go
@@ -19,6 +19,13 @@ type (
 		// Optional. Default value []string{"*"}.
 		AllowOrigins []string `yaml:"allow_origins"`
 
+		// AllowOriginFunc is a custom function to validate the origin. It takes the
+		// origin as an argument and returns true if allowed or false otherwise. If
+		// an error is returned, it is returned by the handler. If this option is
+		// set, AllowOrigins is ignored.
+		// Optional.
+		AllowOriginFunc func(origin string) (bool, error) `yaml:"allow_origin_func"`
+
 		// AllowMethods defines a list methods allowed when accessing the resource.
 		// This is used in response to a preflight request.
 		// Optional. Default value DefaultCORSConfig.AllowMethods.
@@ -113,40 +120,50 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
 				return c.NoContent(http.StatusNoContent)
 			}
 
-			// Check allowed origins
-			for _, o := range config.AllowOrigins {
-				if o == "*" && config.AllowCredentials {
-					allowOrigin = origin
-					break
-				}
-				if o == "*" || o == origin {
-					allowOrigin = o
-					break
+			if config.AllowOriginFunc != nil {
+				allowed, err := config.AllowOriginFunc(origin)
+				if err != nil {
+					return err
 				}
-				if matchSubdomain(origin, o) {
+				if allowed {
 					allowOrigin = origin
-					break
 				}
-			}
-
-			// Check allowed origin patterns
-			for _, re := range allowOriginPatterns {
-				if allowOrigin == "" {
-					didx := strings.Index(origin, "://")
-					if didx == -1 {
-						continue
+			} else {
+				// Check allowed origins
+				for _, o := range config.AllowOrigins {
+					if o == "*" && config.AllowCredentials {
+						allowOrigin = origin
+						break
 					}
-					domAuth := origin[didx+3:]
-					// to avoid regex cost by invalid long domain
-					if len(domAuth) > 253 {
+					if o == "*" || o == origin {
+						allowOrigin = o
 						break
 					}
-
-					if match, _ := regexp.MatchString(re, origin); match {
+					if matchSubdomain(origin, o) {
 						allowOrigin = origin
 						break
 					}
 				}
+
+				// Check allowed origin patterns
+				for _, re := range allowOriginPatterns {
+					if allowOrigin == "" {
+						didx := strings.Index(origin, "://")
+						if didx == -1 {
+							continue
+						}
+						domAuth := origin[didx+3:]
+						// to avoid regex cost by invalid long domain
+						if len(domAuth) > 253 {
+							break
+						}
+
+						if match, _ := regexp.MatchString(re, origin); match {
+							allowOrigin = origin
+							break
+						}
+					}
+				}
 			}
 
 			// Origin not allowed


Test patch:
diff --git a/middleware/cors_test.go b/middleware/cors_test.go
index fc34694..717abe4 100644
--- a/middleware/cors_test.go
+++ b/middleware/cors_test.go
@@ -1,6 +1,7 @@
 package middleware
 
 import (
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -360,3 +361,49 @@ func TestCorsHeaders(t *testing.T) {
 		}
 	}
 }
+
+func Test_allowOriginFunc(t *testing.T) {
+	returnTrue := func(origin string) (bool, error) {
+		return true, nil
+	}
+	returnFalse := func(origin string) (bool, error) {
+		return false, nil
+	}
+	returnError := func(origin string) (bool, error) {
+		return true, errors.New("this is a test error")
+	}
+
+	allowOriginFuncs := []func(origin string) (bool, error){
+		returnTrue,
+		returnFalse,
+		returnError,
+	}
+
+	const origin = "http://example.com"
+
+	e := echo.New()
+	for _, allowOriginFunc := range allowOriginFuncs {
+		req := httptest.NewRequest(http.MethodOptions, "/", nil)
+		rec := httptest.NewRecorder()
+		c := e.NewContext(req, rec)
+		req.Header.Set(echo.HeaderOrigin, origin)
+		cors := CORSWithConfig(CORSConfig{
+			AllowOriginFunc: allowOriginFunc,
+		})
+		h := cors(echo.NotFoundHandler)
+		err := h(c)
+
+		expected, expectedErr := allowOriginFunc(origin)
+		if expectedErr != nil {
+			assert.Equal(t, expectedErr, err)
+			assert.Equal(t, "", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+			continue
+		}
+
+		if expected {
+			assert.Equal(t, origin, rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+		} else {
+			assert.Equal(t, "", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+		}
+	}
+}