Merge branch 'master' of github.com:labstack/echo into cors-allow-ori…

…gin-func

.github/workflows/echo.yml

@@ -4,20 +4,26 @@ on:
   push:
     branches:
       - master
+    paths:
+      - '**.go'
+      - 'go.*'
+      - '_fixture/**'
+      - '.github/**'
   pull_request:
     branches:
       - master
-
-env:
-  GO111MODULE: on
-  GOPROXY: https://proxy.golang.org
+    paths:
+      - '**.go'
+      - 'go.*'
+      - '_fixture/**'
+      - '.github/**'
 
 jobs:
   test:
     strategy:
       matrix:
         os: [ubuntu-latest, macos-latest, windows-latest]
-        go: [1.12, 1.13, 1.14]
+        go: [1.12, 1.13, 1.14, 1.15]
     name: ${{ matrix.os }} @ Go ${{ matrix.go }}
     runs-on: ${{ matrix.os }}
     steps:
@@ -28,10 +34,15 @@ jobs:
 
       - name: Set GOPATH and PATH
         run: |
-          echo "::set-env name=GOPATH::$(dirname $GITHUB_WORKSPACE)"
-          echo "::add-path::$(dirname $GITHUB_WORKSPACE)/bin"
+          echo "GOPATH=$(dirname $GITHUB_WORKSPACE)" >> $GITHUB_ENV
+          echo "$(dirname $GITHUB_WORKSPACE)/bin" >> $GITHUB_PATH
         shell: bash
 
+      - name: Set build variables
+        run: |
+          echo "GOPROXY=https://proxy.golang.org" >> $GITHUB_ENV
+          echo "GO111MODULE=on" >> $GITHUB_ENV
+
       - name: Checkout Code
         uses: actions/checkout@v1
         with:
@@ -51,3 +62,55 @@ jobs:
         with:
           token:
           fail_ci_if_error: false
+  benchmark:
+    needs: test
+    strategy:
+      matrix:
+        os: [ubuntu-latest]
+        go: [1.15]
+    name: Benchmark comparison ${{ matrix.os }} @ Go ${{ matrix.go }}
+    runs-on: ${{ matrix.os }}
+    steps:
+      - name: Set up Go ${{ matrix.go }}
+        uses: actions/setup-go@v1
+        with:
+          go-version: ${{ matrix.go }}
+
+      - name: Set GOPATH and PATH
+        run: |
+          echo "GOPATH=$(dirname $GITHUB_WORKSPACE)" >> $GITHUB_ENV
+          echo "$(dirname $GITHUB_WORKSPACE)/bin" >> $GITHUB_PATH
+        shell: bash
+
+      - name: Set build variables
+        run: |
+          echo "GOPROXY=https://proxy.golang.org" >> $GITHUB_ENV
+          echo "GO111MODULE=on" >> $GITHUB_ENV
+
+      - name: Checkout Code (Previous)
+        uses: actions/checkout@v2
+        with:
+          ref: ${{ github.base_ref }}
+          path: previous
+
+      - name: Checkout Code (New)
+        uses: actions/checkout@v2
+        with:
+          path: new
+
+      - name: Install Dependencies
+        run: go get -v golang.org/x/perf/cmd/benchstat
+
+      - name: Run Benchmark (Previous)
+        run: |
+          cd previous
+          go test -run="-" -bench=".*" -count=5 ./... > benchmark.txt
+
+      - name: Run Benchmark (New)
+        run: |
+          cd new
+          go test -run="-" -bench=".*" -count=5 ./... > benchmark.txt
+
+      - name: Run Benchstat
+        run: |
+          benchstat previous/benchmark.txt new/benchmark.txt

.travis.yml

@@ -1,3 +1,7 @@
+arch:
+  - amd64
+  - ppc64le
+
 language: go
 go:
   - 1.14.x

context.go

@@ -276,7 +276,11 @@ func (c *context) RealIP() string {
 	}
 	// Fall back to legacy behavior
 	if ip := c.request.Header.Get(HeaderXForwardedFor); ip != "" {
-		return strings.Split(ip, ", ")[0]
+		i := strings.IndexAny(ip, ", ")
+		if i > 0 {
+			return ip[:i]
+		}
+		return ip
 	}
 	if ip := c.request.Header.Get(HeaderXRealIP); ip != "" {
 		return ip
@@ -361,7 +365,7 @@ func (c *context) FormFile(name string) (*multipart.FileHeader, error) {
 	if err != nil {
 		return nil, err
 	}
-	defer f.Close()
+	f.Close()
 	return fh, nil
 }
 

context_test.go

@@ -72,6 +72,15 @@ func BenchmarkAllocXML(b *testing.B) {
 	}
 }
 
+func BenchmarkRealIPForHeaderXForwardFor(b *testing.B) {
+	c := context{request: &http.Request{
+		Header: http.Header{HeaderXForwardedFor: []string{"127.0.0.1, 127.0.1.1, "}},
+	}}
+	for i := 0; i < b.N; i++ {
+		c.RealIP()
+	}
+}
+
 func (t *Template) Render(w io.Writer, name string, data interface{}, c Context) error {
 	return t.templates.ExecuteTemplate(w, name, data)
 }
@@ -847,6 +856,14 @@ func TestContext_RealIP(t *testing.T) {
 			},
 			"127.0.0.1",
 		},
+		{
+			&context{
+				request: &http.Request{
+					Header: http.Header{HeaderXForwardedFor: []string{"127.0.0.1"}},
+				},
+			},
+			"127.0.0.1",
+		},
 		{
 			&context{
 				request: &http.Request{

echo.go

@@ -362,10 +362,12 @@ func (e *Echo) DefaultHTTPErrorHandler(err error, c Context) {
 	// Issue #1426
 	code := he.Code
 	message := he.Message
-	if e.Debug {
-		message = err.Error()
-	} else if m, ok := message.(string); ok {
-		message = Map{"message": m}
+	if m, ok := he.Message.(string); ok {
+		if e.Debug {
+			message = Map{"message": m, "error": err.Error()}
+		} else {
+			message = Map{"message": m}
+		}
 	}
 
 	// Send response

echo_test.go

@@ -568,6 +568,49 @@ func TestHTTPError(t *testing.T) {
 	})
 }
 
+func TestDefaultHTTPErrorHandler(t *testing.T) {
+	e := New()
+	e.Debug = true
+	e.Any("/plain", func(c Context) error {
+		return errors.New("An error occurred")
+	})
+	e.Any("/badrequest", func(c Context) error {
+		return NewHTTPError(http.StatusBadRequest, "Invalid request")
+	})
+	e.Any("/servererror", func(c Context) error {
+		return NewHTTPError(http.StatusInternalServerError, map[string]interface{}{
+			"code":    33,
+			"message": "Something bad happened",
+			"error":   "stackinfo",
+		})
+	})
+	// With Debug=true plain response contains error message
+	c, b := request(http.MethodGet, "/plain", e)
+	assert.Equal(t, http.StatusInternalServerError, c)
+	assert.Equal(t, "{\n  \"error\": \"An error occurred\",\n  \"message\": \"Internal Server Error\"\n}\n", b)
+	// and special handling for HTTPError
+	c, b = request(http.MethodGet, "/badrequest", e)
+	assert.Equal(t, http.StatusBadRequest, c)
+	assert.Equal(t, "{\n  \"error\": \"code=400, message=Invalid request\",\n  \"message\": \"Invalid request\"\n}\n", b)
+	// complex errors are serialized to pretty JSON
+	c, b = request(http.MethodGet, "/servererror", e)
+	assert.Equal(t, http.StatusInternalServerError, c)
+	assert.Equal(t, "{\n  \"code\": 33,\n  \"error\": \"stackinfo\",\n  \"message\": \"Something bad happened\"\n}\n", b)
+
+	e.Debug = false
+	// With Debug=false the error response is shortened
+	c, b = request(http.MethodGet, "/plain", e)
+	assert.Equal(t, http.StatusInternalServerError, c)
+	assert.Equal(t, "{\"message\":\"Internal Server Error\"}\n", b)
+	c, b = request(http.MethodGet, "/badrequest", e)
+	assert.Equal(t, http.StatusBadRequest, c)
+	assert.Equal(t, "{\"message\":\"Invalid request\"}\n", b)
+	// No difference for error response with non plain string errors
+	c, b = request(http.MethodGet, "/servererror", e)
+	assert.Equal(t, http.StatusInternalServerError, c)
+	assert.Equal(t, "{\"code\":33,\"error\":\"stackinfo\",\"message\":\"Something bad happened\"}\n", b)
+}
+
 func TestEchoClose(t *testing.T) {
 	e := New()
 	errCh := make(chan error)

middleware/compress.go

@@ -8,6 +8,7 @@ import (
 	"net"
 	"net/http"
 	"strings"
+	"sync"
 
 	"github.com/labstack/echo/v4"
 )
@@ -58,6 +59,8 @@ func GzipWithConfig(config GzipConfig) echo.MiddlewareFunc {
 		config.Level = DefaultGzipConfig.Level
 	}
 
+	pool := gzipPool(config)
+
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
 		return func(c echo.Context) error {
 			if config.Skipper(c) {
@@ -68,11 +71,13 @@ func GzipWithConfig(config GzipConfig) echo.MiddlewareFunc {
 			res.Header().Add(echo.HeaderVary, echo.HeaderAcceptEncoding)
 			if strings.Contains(c.Request().Header.Get(echo.HeaderAcceptEncoding), gzipScheme) {
 				res.Header().Set(echo.HeaderContentEncoding, gzipScheme) // Issue #806
-				rw := res.Writer
-				w, err := gzip.NewWriterLevel(rw, config.Level)
-				if err != nil {
-					return err
+				i := pool.Get()
+				w, ok := i.(*gzip.Writer)
+				if !ok {
+					return echo.NewHTTPError(http.StatusInternalServerError, i.(error).Error())
 				}
+				rw := res.Writer
+				w.Reset(rw)
 				defer func() {
 					if res.Size == 0 {
 						if res.Header().Get(echo.HeaderContentEncoding) == gzipScheme {
@@ -85,6 +90,7 @@ func GzipWithConfig(config GzipConfig) echo.MiddlewareFunc {
 						w.Reset(ioutil.Discard)
 					}
 					w.Close()
+					pool.Put(w)
 				}()
 				grw := &gzipResponseWriter{Writer: w, ResponseWriter: rw}
 				res.Writer = grw
@@ -126,3 +132,15 @@ func (w *gzipResponseWriter) Push(target string, opts *http.PushOptions) error {
 	}
 	return http.ErrNotSupported
 }
+
+func gzipPool(config GzipConfig) sync.Pool {
+	return sync.Pool{
+		New: func() interface{} {
+			w, err := gzip.NewWriterLevel(ioutil.Discard, config.Level)
+			if err != nil {
+				return err
+			}
+			return w
+		},
+	}
+}

middleware/compress_test.go

@@ -120,6 +120,22 @@ func TestGzipErrorReturned(t *testing.T) {
 	assert.Empty(t, rec.Header().Get(echo.HeaderContentEncoding))
 }
 
+func TestGzipErrorReturnedInvalidConfig(t *testing.T) {
+	e := echo.New()
+	// Invalid level
+	e.Use(GzipWithConfig(GzipConfig{Level: 12}))
+	e.GET("/", func(c echo.Context) error {
+		c.Response().Write([]byte("test"))
+		return nil
+	})
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set(echo.HeaderAcceptEncoding, gzipScheme)
+	rec := httptest.NewRecorder()
+	e.ServeHTTP(rec, req)
+	assert.Equal(t, http.StatusInternalServerError, rec.Code)
+	assert.Contains(t, rec.Body.String(), "gzip")
+}
+
 // Issue #806
 func TestGzipWithStatic(t *testing.T) {
 	e := echo.New()
@@ -146,3 +162,25 @@ func TestGzipWithStatic(t *testing.T) {
 		}
 	}
 }
+
+func BenchmarkGzip(b *testing.B) {
+	e := echo.New()
+
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set(echo.HeaderAcceptEncoding, gzipScheme)
+
+	h := Gzip()(func(c echo.Context) error {
+		c.Response().Write([]byte("test")) // For Content-Type sniffing
+		return nil
+	})
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		// Gzip
+		rec := httptest.NewRecorder()
+		c := e.NewContext(req, rec)
+		h(c)
+	}
+}

middleware/cors.go

@@ -109,6 +109,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)
+			}
+
 			if config.AllowOriginFunc != nil {
 				allowed, err := config.AllowOriginFunc(origin)
 				if err != nil {
@@ -155,9 +166,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")
@@ -169,7 +187,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)