Repository: labstack/echo (go)

Issue:
CORS middleware sends empty and/or unnecessary headers
### Issue Description

I have noticed two issues that CORS middleware has:
1. It sends empty `Access-Control-Allow-Origin` header if
    * `Origin` header in the request is missing or
    * `Origin` header is not allowed
2. It sends CORS headers (`Access-Control-*`) even if there is no `Origin` header in the request or provided `Origin` is not allowed. To my understanding if a request is supposed to use CORS it [should have](https://w3c.github.io/webappsec-cors-for-developers) an `Origin` header. I tested this behaviour with [PlayFramework](https://github.com/playframework/playframework) and [Gin](https://github.com/gin-contrib/cors), they are not sending CORS related headers in this case.

### Checklist

- [X] Dependencies installed
- [X] No typos
- [X] Searched existing issues and docs

### Expected behaviour

* CORS middleware should not send empty header `Access-Control-Allow-Origin:`.
* CORS middleware should only provide `Access-Control-*` headers when `Origin` header is provided.

### Actual behaviour

Described in Issue Description section.

### Steps to reproduce

After running the code provided below (in "Working code to debug" part) it can be reproduced with the following curl commands:

#### Empty Access-Control-Allow-Origin when Origin is not allowed:

```shell
curl -X OPTIONS -H "Origin: http://bar.com" -I http://localhost:1323/
HTTP/1.1 204 No Content
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Access-Control-Allow-Origin:
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
Date: Fri, 06 Nov 2020 01:11:30 GMT
```

#### Empty Access-Control-Allow-Origin when Origin is not provided:

```shell
curl -X OPTIONS -I http://localhost:1323/
HTTP/1.1 204 No Content
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Access-Control-Allow-Origin:
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
Date: Fri, 06 Nov 2020 01:12:56 GMT
```

#### Access-Control-* headers when Origin is not provided:

**NOTE**: For this first change middleware line to `e.Use(middleware.CORS())` (by default AllowOrigin is `*`)

```shell
curl -X OPTIONS -I http://localhost:1323/
HTTP/1.1 204 No Content
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Access-Control-Allow-Origin: *
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
Date: Fri, 06 Nov 2020 01:21:06 GMT
```

### Working code to debug

```go
package main

import (
    "net/http"

    "github.com/labstack/echo/v4"
    "github.com/labstack/echo/v4/middleware"
)

func main() {
    e := echo.New()
    e.Use(middleware.CORSWithConfig(middleware.CORSConfig{
        AllowOrigins: []string{"http://example.com"},
    }))
    e.GET("/", func(c echo.Context) error {
        return c.String(http.StatusOK, "Hello, World!")
    })
    e.Logger.Fatal(e.Start(":1323"))
}
```

### Version/commit

v4.1.17

Pull request:
Fix empty/incorrect CORS headers
Hi, this PR attempts to fix #1668.

Patch:
diff --git a/middleware/cors.go b/middleware/cors.go
index c263f73..07df0e5 100644
--- a/middleware/cors.go
+++ b/middleware/cors.go
@@ -102,6 +102,17 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
 			origin := req.Header.Get(echo.HeaderOrigin)
 			allowOrigin := ""
 
+			preflight := req.Method == http.MethodOptions
+			res.Header().Add(echo.HeaderVary, echo.HeaderOrigin)
+
+			// No Origin provided
+			if origin == "" {
+				if !preflight {
+					return next(c)
+				}
+				return c.NoContent(http.StatusNoContent)
+			}
+
 			// Check allowed origins
 			for _, o := range config.AllowOrigins {
 				if o == "*" && config.AllowCredentials {
@@ -138,9 +149,16 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
 				}
 			}
 
+			// Origin not allowed
+			if allowOrigin == "" {
+				if !preflight {
+					return next(c)
+				}
+				return c.NoContent(http.StatusNoContent)
+			}
+
 			// Simple request
-			if req.Method != http.MethodOptions {
-				res.Header().Add(echo.HeaderVary, echo.HeaderOrigin)
+			if !preflight {
 				res.Header().Set(echo.HeaderAccessControlAllowOrigin, allowOrigin)
 				if config.AllowCredentials {
 					res.Header().Set(echo.HeaderAccessControlAllowCredentials, "true")
@@ -152,7 +170,6 @@ func CORSWithConfig(config CORSConfig) echo.MiddlewareFunc {
 			}
 
 			// Preflight request
-			res.Header().Add(echo.HeaderVary, echo.HeaderOrigin)
 			res.Header().Add(echo.HeaderVary, echo.HeaderAccessControlRequestMethod)
 			res.Header().Add(echo.HeaderVary, echo.HeaderAccessControlRequestHeaders)
 			res.Header().Set(echo.HeaderAccessControlAllowOrigin, allowOrigin)


Test patch:
diff --git a/middleware/cors_test.go b/middleware/cors_test.go
index ca92232..fc34694 100644
--- a/middleware/cors_test.go
+++ b/middleware/cors_test.go
@@ -17,19 +17,31 @@ func TestCORS(t *testing.T) {
 	rec := httptest.NewRecorder()
 	c := e.NewContext(req, rec)
 	h := CORS()(echo.NotFoundHandler)
+	req.Header.Set(echo.HeaderOrigin, "localhost")
 	h(c)
 	assert.Equal(t, "*", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
 
+	// Wildcard AllowedOrigin with no Origin header in request
+	req = httptest.NewRequest(http.MethodGet, "/", nil)
+	rec = httptest.NewRecorder()
+	c = e.NewContext(req, rec)
+	h = CORS()(echo.NotFoundHandler)
+	h(c)
+	assert.NotContains(t, rec.Header(), echo.HeaderAccessControlAllowOrigin)
+
 	// Allow origins
 	req = httptest.NewRequest(http.MethodGet, "/", nil)
 	rec = httptest.NewRecorder()
 	c = e.NewContext(req, rec)
 	h = CORSWithConfig(CORSConfig{
-		AllowOrigins: []string{"localhost"},
+		AllowOrigins:     []string{"localhost"},
+		AllowCredentials: true,
+		MaxAge:           3600,
 	})(echo.NotFoundHandler)
 	req.Header.Set(echo.HeaderOrigin, "localhost")
 	h(c)
 	assert.Equal(t, "localhost", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+	assert.Equal(t, "true", rec.Header().Get(echo.HeaderAccessControlAllowCredentials))
 
 	// Preflight request
 	req = httptest.NewRequest(http.MethodOptions, "/", nil)
@@ -67,6 +79,22 @@ func TestCORS(t *testing.T) {
 	assert.Equal(t, "true", rec.Header().Get(echo.HeaderAccessControlAllowCredentials))
 	assert.Equal(t, "3600", rec.Header().Get(echo.HeaderAccessControlMaxAge))
 
+	// Preflight request with Access-Control-Request-Headers
+	req = httptest.NewRequest(http.MethodOptions, "/", nil)
+	rec = httptest.NewRecorder()
+	c = e.NewContext(req, rec)
+	req.Header.Set(echo.HeaderOrigin, "localhost")
+	req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON)
+	req.Header.Set(echo.HeaderAccessControlRequestHeaders, "Special-Request-Header")
+	cors = CORSWithConfig(CORSConfig{
+		AllowOrigins: []string{"*"},
+	})
+	h = cors(echo.NotFoundHandler)
+	h(c)
+	assert.Equal(t, "*", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+	assert.Equal(t, "Special-Request-Header", rec.Header().Get(echo.HeaderAccessControlAllowHeaders))
+	assert.NotEmpty(t, rec.Header().Get(echo.HeaderAccessControlAllowMethods))
+
 	// Preflight request with `AllowOrigins` which allow all subdomains with *
 	req = httptest.NewRequest(http.MethodOptions, "/", nil)
 	rec = httptest.NewRecorder()
@@ -126,7 +154,7 @@ func Test_allowOriginScheme(t *testing.T) {
 		if tt.expected {
 			assert.Equal(t, tt.domain, rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
 		} else {
-			assert.Equal(t, "", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+			assert.NotContains(t, rec.Header(), echo.HeaderAccessControlAllowOrigin)
 		}
 	}
 }
@@ -217,7 +245,118 @@ func Test_allowOriginSubdomain(t *testing.T) {
 		if tt.expected {
 			assert.Equal(t, tt.domain, rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
 		} else {
-			assert.Equal(t, "", rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+			assert.NotContains(t, rec.Header(), echo.HeaderAccessControlAllowOrigin)
+		}
+	}
+}
+
+func TestCorsHeaders(t *testing.T) {
+	tests := []struct {
+		domain, allowedOrigin, method string
+		expected                      bool
+	}{
+		{
+			domain:        "", // Request does not have Origin header
+			allowedOrigin: "*",
+			method:        http.MethodGet,
+			expected:      false,
+		},
+		{
+			domain:        "http://example.com",
+			allowedOrigin: "*",
+			method:        http.MethodGet,
+			expected:      true,
+		},
+		{
+			domain:        "", // Request does not have Origin header
+			allowedOrigin: "http://example.com",
+			method:        http.MethodGet,
+			expected:      false,
+		},
+		{
+			domain:        "http://bar.com",
+			allowedOrigin: "http://example.com",
+			method:        http.MethodGet,
+			expected:      false,
+		},
+		{
+			domain:        "http://example.com",
+			allowedOrigin: "http://example.com",
+			method:        http.MethodGet,
+			expected:      true,
+		},
+		{
+			domain:        "", // Request does not have Origin header
+			allowedOrigin: "*",
+			method:        http.MethodOptions,
+			expected:      false,
+		},
+		{
+			domain:        "http://example.com",
+			allowedOrigin: "*",
+			method:        http.MethodOptions,
+			expected:      true,
+		},
+		{
+			domain:        "", // Request does not have Origin header
+			allowedOrigin: "http://example.com",
+			method:        http.MethodOptions,
+			expected:      false,
+		},
+		{
+			domain:        "http://bar.com",
+			allowedOrigin: "http://example.com",
+			method:        http.MethodGet,
+			expected:      false,
+		},
+		{
+			domain:        "http://example.com",
+			allowedOrigin: "http://example.com",
+			method:        http.MethodOptions,
+			expected:      true,
+		},
+	}
+
+	e := echo.New()
+	for _, tt := range tests {
+		req := httptest.NewRequest(tt.method, "/", nil)
+		rec := httptest.NewRecorder()
+		c := e.NewContext(req, rec)
+		if tt.domain != "" {
+			req.Header.Set(echo.HeaderOrigin, tt.domain)
+		}
+		cors := CORSWithConfig(CORSConfig{
+			AllowOrigins: []string{tt.allowedOrigin},
+			//AllowCredentials: true,
+			//MaxAge:           3600,
+		})
+		h := cors(echo.NotFoundHandler)
+		h(c)
+
+		assert.Equal(t, echo.HeaderOrigin, rec.Header().Get(echo.HeaderVary))
+
+		expectedAllowOrigin := ""
+		if tt.allowedOrigin == "*" {
+			expectedAllowOrigin = "*"
+		} else {
+			expectedAllowOrigin = tt.domain
+		}
+
+		switch {
+		case tt.expected && tt.method == http.MethodOptions:
+			assert.Contains(t, rec.Header(), echo.HeaderAccessControlAllowMethods)
+			assert.Equal(t, expectedAllowOrigin, rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+			assert.Equal(t, 3, len(rec.Header()[echo.HeaderVary]))
+		case tt.expected && tt.method == http.MethodGet:
+			assert.Equal(t, expectedAllowOrigin, rec.Header().Get(echo.HeaderAccessControlAllowOrigin))
+			assert.Equal(t, 1, len(rec.Header()[echo.HeaderVary])) // Vary: Origin
+		default:
+			assert.NotContains(t, rec.Header(), echo.HeaderAccessControlAllowOrigin)
+			assert.Equal(t, 1, len(rec.Header()[echo.HeaderVary])) // Vary: Origin
+		}
+
+		if tt.method == http.MethodOptions {
+			assert.Equal(t, http.StatusNoContent, rec.Code)
 		}
 	}
 }