Repository: labstack/echo
Pull request #1274 (2019-02-01 07:38:01)

www redirects are too broad
Currently www redirect middleware functions catch false positives like `www-data.hostname.com`

This commit fixes false positives.

codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/1274?src=pr&el=h1) Report
> Merging [#1274](https://codecov.io/gh/labstack/echo/pull/1274?src=pr&el=desc) into [master](https://codecov.io/gh/labstack/echo/commit/6d9e043284aea2d07f5fcaf0d3a424eb7d9f6109?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

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

```diff
@@           Coverage Diff           @@
##           master    #1274   +/-   ##
=======================================
  Coverage   81.88%   81.88%           
=======================================
  Files          26       26           
  Lines        1932     1932           
=======================================
  Hits         1582     1582           
  Misses        242      242           
  Partials      108      108
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/1274?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [middleware/redirect.go](https://codecov.io/gh/labstack/echo/pull/1274/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9yZWRpcmVjdC5nbw==) | `88.23% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/1274?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/1274?src=pr&el=footer). Last update [6d9e043...db90afb](https://codecov.io/gh/labstack/echo/pull/1274?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

alexaandru: Thank you, looks like a great change to me.

That being said, I'd like to discuss something before merging: @vishr @im-kulikov thoughts on having the version incremented with each pull request? So rather than having that 3.3.dev (or 4.0.dev) or not changing the version in between releases, how about we change the process with this:

1. we ask with each PR for the version to be incremented accordingly. After all, that's how semver is supposed to work;
2. we cut releases once in a while. It would be overkill to cut a release for every single commit, so we let a few accumulate (~5? more? less? you tell me...) and then we cut a new release.

That way the version will evolve naturally, we don't need no "Bumped up version" commits and basically change the version in a commit that nothing happened :-) but we actually change it when things do happen. Thoughts?
im-kulikov: I still think releasing releases on every PR is too much and doesn't make much sense. Some changes can cause discussions, which will change the main idea and subsequently changed code
I suggest we accumulate changes.. 
We have two ways:
- release once in a while
- release by number of changes
alexaandru: @im-kulikov I don't think we're disagreeing here :-) Or maybe not entirely :-)

I do agree release on every PR doesn't make any sense. But I do think that if we want to follow semver, then we need to bump up version with every change. Like here, this is a backward compatible (ok, a little debatable, hopefully no one depends on redirecting all their www-* domains to root...), so as per semver, the version should be bumped up.

What I am saying is we shouldn't call it a release but DO bump up the version, so we have, for example, 4.0.1, 4.0.2. ... 4.0.5 - and then when we consider there are enough for a release, we cut a release at whatever number we are at. That would mean releases 4.0.6, 4.0.11, 4.0.20 - only those (just examples, not actual numbers) get tagged/released, but the Echo version increases with each change. That way, even for people pulling master, a given Echo version identifies a unique commit, whereas a 4.0.1-dev would identify any of the commits between 4.0.0 and 4.0.1.

Now indeed, that seems rather odd at 1st :-) It may even be a dumb idea, I just thought is worth discussing.

But let's look at it from the other end, let' look at the proposed process: in between releases, we use the <next_release>-dev version (so we're at 4.0.0 now, we should be using 4.0.1-dev). Is that really accurate? Are we actually pushing "dev" work to master? Work that may or may not be production ready? I don't think that's the case, so using such a versioning on master would be equally misleading. We only push work that we review and know it is safe to use. We may not want to release with every single commit to master, but we certainly don't merge WIP to master either...

Maybe we should consider a 3rd option - a `dev` branch. Now in there, a <next_release>-dev version does actually make sense. It's clearly not master, people should not be using it until released, etc. etc. When we're ready, we change the Echo version, merge to master, tag & release it. This however is not without downsides. Maybe we have tiny fixes/improvements, not enough for a release but usable and likely useful for some people (at the very least for the ones that suggested them...). Now keeping them in the dev branch is an indication that they should not be used.

As always, it's all about tradeoffs :-) I just personally don't like seeing "-dev" versions in a master branch, but that's just me :-) I don't really have a better approach, I just thought I should open it up for discussion. I don't like the version skipping approach either :-) 

Anyway, let's not get a good PR wait because of this discussion. Should've opened up a ticket instead, sorry @rdbell for hijacking the thread :-)

@vishr - what are your thoughts on this change? Do you feel it may break backward compatibility or was it always the case that only `www` should've been redirected to root from the start? Though even if people rely on a buggy behaviour, fixing it is still called breaking backward compatibility, so technically, we are doing it... Though I guess if anyone needs the old behaviour, it's just a few lines of code away (writing a middleware to replicate it).
im-kulikov: Maybe then it would be more correct to release 4.X.0?
If there are corrective changes that do not break anything and only fix something in previous release (4.x.y)  - we release 4.X.y+1?

Patch:
diff --git a/context.go b/context.go
index d472270..75f59fd 100644
--- a/context.go
+++ b/context.go
@@ -13,6 +13,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 )
 
 type (
@@ -198,6 +199,7 @@ type (
 		handler  HandlerFunc
 		store    Map
 		echo     *Echo
+		lock     sync.RWMutex
 	}
 )
 
@@ -360,10 +362,15 @@ func (c *context) Cookies() []*http.Cookie {
 }
 
 func (c *context) Get(key string) interface{} {
+	c.lock.RLock()
+	defer c.lock.RUnlock()
 	return c.store[key]
 }
 
 func (c *context) Set(key string, val interface{}) {
+	c.lock.Lock()
+	defer c.lock.Unlock()
+
 	if c.store == nil {
 		c.store = make(Map)
 	}
@@ -597,4 +604,3 @@ func (c *context) Reset(r *http.Request, w http.ResponseWriter) {
 	// NOTE: Don't reset because it has to have length c.echo.maxParam at all times
 	// c.pvalues = nil
 }
-
diff --git a/echo.go b/echo.go
index 0fe6880..4f8209b 100644
--- a/echo.go
+++ b/echo.go
@@ -43,6 +43,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/ioutil"
 	stdLog "log"
 	"net"
 	"net/http"
@@ -269,6 +270,7 @@ var (
 	ErrRendererNotRegistered       = errors.New("renderer not registered")
 	ErrInvalidRedirectCode         = errors.New("invalid redirect status code")
 	ErrCookieNotFound              = errors.New("cookie not found")
+	ErrInvalidCertOrKeyType        = errors.New("invalid cert or key type, must be string or []byte")
 )
 
 // Error handlers
@@ -605,20 +607,40 @@ func (e *Echo) Start(address string) error {
 }
 
 // StartTLS starts an HTTPS server.
-func (e *Echo) StartTLS(address string, certFile, keyFile string) (err error) {
-	if certFile == "" || keyFile == "" {
-		return errors.New("invalid tls configuration")
+// If `certFile` or `keyFile` is `string` the values are treated as file paths.
+// If `certFile` or `keyFile` is `[]byte` the values are treated as the certificate or key as-is.
+func (e *Echo) StartTLS(address string, certFile, keyFile interface{}) (err error) {
+	var cert []byte
+	if cert, err = filepathOrContent(certFile); err != nil {
+		return
+	}
+
+	var key []byte
+	if key, err = filepathOrContent(keyFile); err != nil {
+		return
 	}
+
 	s := e.TLSServer
 	s.TLSConfig = new(tls.Config)
 	s.TLSConfig.Certificates = make([]tls.Certificate, 1)
-	s.TLSConfig.Certificates[0], err = tls.LoadX509KeyPair(certFile, keyFile)
-	if err != nil {
+	if s.TLSConfig.Certificates[0], err = tls.X509KeyPair(cert, key); err != nil {
 		return
 	}
+
 	return e.startTLS(address)
 }
 
+func filepathOrContent(fileOrContent interface{}) (content []byte, err error) {
+	switch v := fileOrContent.(type) {
+	case string:
+		return ioutil.ReadFile(v)
+	case []byte:
+		return v, nil
+	default:
+		return nil, ErrInvalidCertOrKeyType
+	}
+}
+
 // StartAutoTLS starts an HTTPS server using certificates automatically installed from https://letsencrypt.org.
 func (e *Echo) StartAutoTLS(address string) error {
 	s := e.TLSServer
@@ -764,13 +786,14 @@ type tcpKeepAliveListener struct {
 }
 
 func (ln tcpKeepAliveListener) Accept() (c net.Conn, err error) {
-	tc, err := ln.AcceptTCP()
-	if err != nil {
+	if c, err = ln.AcceptTCP(); err != nil {
+		return
+	} else if err = c.(*net.TCPConn).SetKeepAlive(true); err != nil {
+		return
+	} else if err = c.(*net.TCPConn).SetKeepAlivePeriod(3 * time.Minute); err != nil {
 		return
 	}
-	tc.SetKeepAlive(true)
-	tc.SetKeepAlivePeriod(3 * time.Minute)
-	return tc, nil
+	return
 }
 
 func newListener(address string) (*tcpKeepAliveListener, error) {
diff --git a/middleware/redirect.go b/middleware/redirect.go
index 30a2e40..813e5b8 100644
--- a/middleware/redirect.go
+++ b/middleware/redirect.go
@@ -21,7 +21,7 @@ type RedirectConfig struct {
 // 2) return the appropriate redirect url.
 type redirectLogic func(scheme, host, uri string) (ok bool, url string)
 
-const www = "www"
+const www = "www."
 
 // DefaultRedirectConfig is the default Redirect middleware config.
 var DefaultRedirectConfig = RedirectConfig{
@@ -60,7 +60,7 @@ func HTTPSWWWRedirect() echo.MiddlewareFunc {
 // See `HTTPSWWWRedirect()`.
 func HTTPSWWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc {
 	return redirect(config, func(scheme, host, uri string) (ok bool, url string) {
-		if ok = scheme != "https" && host[:3] != www; ok {
+		if ok = scheme != "https" && host[:4] != www; ok {
 			url = "https://www." + host + uri
 		}
 		return
@@ -80,7 +80,7 @@ func HTTPSNonWWWRedirect() echo.MiddlewareFunc {
 func HTTPSNonWWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc {
 	return redirect(config, func(scheme, host, uri string) (ok bool, url string) {
 		if ok = scheme != "https"; ok {
-			if host[:3] == www {
+			if host[:4] == www {
 				host = host[4:]
 			}
 			url = "https://" + host + uri
@@ -101,7 +101,7 @@ func WWWRedirect() echo.MiddlewareFunc {
 // See `WWWRedirect()`.
 func WWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc {
 	return redirect(config, func(scheme, host, uri string) (ok bool, url string) {
-		if ok = host[:3] != www; ok {
+		if ok = host[:4] != www; ok {
 			url = scheme + "://www." + host + uri
 		}
 		return
@@ -120,7 +120,7 @@ func NonWWWRedirect() echo.MiddlewareFunc {
 // See `NonWWWRedirect()`.
 func NonWWWRedirectWithConfig(config RedirectConfig) echo.MiddlewareFunc {
 	return redirect(config, func(scheme, host, uri string) (ok bool, url string) {
-		if ok = host[:3] == www; ok {
+		if ok = host[:4] == www; ok {
 			url = scheme + "://" + host[4:] + uri
 		}
 		return
diff --git a/middleware/slash.go b/middleware/slash.go
index 61d6e30..0492b33 100644
--- a/middleware/slash.go
+++ b/middleware/slash.go
@@ -1,6 +1,8 @@
 package middleware
 
 import (
+	"strings"
+
 	"github.com/labstack/echo/v4"
 )
 
@@ -49,7 +51,7 @@ func AddTrailingSlashWithConfig(config TrailingSlashConfig) echo.MiddlewareFunc
 			url := req.URL
 			path := url.Path
 			qs := c.QueryString()
-			if path != "/" && path[len(path)-1] != '/' {
+			if !strings.HasSuffix(path, "/") {
 				path += "/"
 				uri := path
 				if qs != "" {
@@ -97,7 +99,7 @@ func RemoveTrailingSlashWithConfig(config TrailingSlashConfig) echo.MiddlewareFu
 			path := url.Path
 			qs := c.QueryString()
 			l := len(path) - 1
-			if l >= 0 && path != "/" && path[l] == '/' {
+			if l > 0 && strings.HasSuffix(path, "/") {
 				path = path[:l]
 				uri := path
 				if qs != "" {


Test patch:
diff --git a/context_test.go b/context_test.go
index 3a47364..6118866 100644
--- a/context_test.go
+++ b/context_test.go
@@ -529,6 +529,21 @@ func TestContextStore(t *testing.T) {
 	testify.Equal(t, "Jon Snow", c.Get("name"))
 }
 
+func BenchmarkContext_Store(b *testing.B) {
+	e := &Echo{}
+
+	c := &context{
+		echo: e,
+	}
+
+	for n := 0; n < b.N; n++ {
+		c.Set("name", "Jon Snow")
+		if c.Get("name") != "Jon Snow" {
+			b.Fail()
+		}
+	}
+}
+
 func TestContextHandler(t *testing.T) {
 	e := New()
 	r := e.Router()
@@ -543,4 +558,3 @@ func TestContextHandler(t *testing.T) {
 	c.Handler()(c)
 	testify.Equal(t, "handler", b.String())
 }
-
diff --git a/echo_test.go b/echo_test.go
index 3ac10a7..dec713e 100644
--- a/echo_test.go
+++ b/echo_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	stdContext "context"
 	"errors"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"reflect"
@@ -12,6 +13,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 type (
@@ -428,6 +430,71 @@ func TestEchoStartTLS(t *testing.T) {
 	e.Close()
 }
 
+func TestEchoStartTLSByteString(t *testing.T) {
+	cert, err := ioutil.ReadFile("_fixture/certs/cert.pem")
+	require.NoError(t, err)
+	key, err := ioutil.ReadFile("_fixture/certs/key.pem")
+	require.NoError(t, err)
+
+	testCases := []struct {
+		cert        interface{}
+		key         interface{}
+		expectedErr error
+		name        string
+	}{
+		{
+			cert:        "_fixture/certs/cert.pem",
+			key:         "_fixture/certs/key.pem",
+			expectedErr: nil,
+			name:        `ValidCertAndKeyFilePath`,
+		},
+		{
+			cert:        cert,
+			key:         key,
+			expectedErr: nil,
+			name:        `ValidCertAndKeyByteString`,
+		},
+		{
+			cert:        cert,
+			key:         1,
+			expectedErr: ErrInvalidCertOrKeyType,
+			name:        `InvalidKeyType`,
+		},
+		{
+			cert:        0,
+			key:         key,
+			expectedErr: ErrInvalidCertOrKeyType,
+			name:        `InvalidCertType`,
+		},
+		{
+			cert:        0,
+			key:         1,
+			expectedErr: ErrInvalidCertOrKeyType,
+			name:        `InvalidCertAndKeyTypes`,
+		},
+	}
+
+	for _, test := range testCases {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			e := New()
+			e.HideBanner = true
+
+			go func() {
+				err := e.StartTLS(":0", test.cert, test.key)
+				if test.expectedErr != nil {
+					require.EqualError(t, err, test.expectedErr.Error())
+				} else if err != http.ErrServerClosed { // Prevent the test to fail after closing the servers
+					require.NoError(t, err)
+				}
+			}()
+			time.Sleep(200 * time.Millisecond)
+
+			require.NoError(t, e.Close())
+		})
+	}
+}
+
 func TestEchoStartAutoTLS(t *testing.T) {
 	e := New()
 	errChan := make(chan error, 0)
diff --git a/middleware/slash_test.go b/middleware/slash_test.go
index e60be74..2a8e9ee 100644
--- a/middleware/slash_test.go
+++ b/middleware/slash_test.go
@@ -10,6 +10,7 @@ import (
 )
 
 func TestAddTrailingSlash(t *testing.T) {
+	is := assert.New(t)
 	e := echo.New()
 	req := httptest.NewRequest(http.MethodGet, "/add-slash", nil)
 	rec := httptest.NewRecorder()
@@ -17,11 +18,20 @@ func TestAddTrailingSlash(t *testing.T) {
 	h := AddTrailingSlash()(func(c echo.Context) error {
 		return nil
 	})
-	h(c)
+	is.NoError(h(c))
+	is.Equal("/add-slash/", req.URL.Path)
+	is.Equal("/add-slash/", req.RequestURI)
 
-	assert := assert.New(t)
-	assert.Equal("/add-slash/", req.URL.Path)
-	assert.Equal("/add-slash/", req.RequestURI)
+	// Method Connect must not fail:
+	req = httptest.NewRequest(http.MethodConnect, "", nil)
+	rec = httptest.NewRecorder()
+	c = e.NewContext(req, rec)
+	h = AddTrailingSlash()(func(c echo.Context) error {
+		return nil
+	})
+	is.NoError(h(c))
+	is.Equal("/", req.URL.Path)
+	is.Equal("/", req.RequestURI)
 
 	// With config
 	req = httptest.NewRequest(http.MethodGet, "/add-slash?key=value", nil)
@@ -32,12 +42,13 @@ func TestAddTrailingSlash(t *testing.T) {
 	})(func(c echo.Context) error {
 		return nil
 	})
-	h(c)
-	assert.Equal(http.StatusMovedPermanently, rec.Code)
-	assert.Equal("/add-slash/?key=value", rec.Header().Get(echo.HeaderLocation))
+	is.NoError(h(c))
+	is.Equal(http.StatusMovedPermanently, rec.Code)
+	is.Equal("/add-slash/?key=value", rec.Header().Get(echo.HeaderLocation))
 }
 
 func TestRemoveTrailingSlash(t *testing.T) {
+	is := assert.New(t)
 	e := echo.New()
 	req := httptest.NewRequest(http.MethodGet, "/remove-slash/", nil)
 	rec := httptest.NewRecorder()
@@ -45,12 +56,20 @@ func TestRemoveTrailingSlash(t *testing.T) {
 	h := RemoveTrailingSlash()(func(c echo.Context) error {
 		return nil
 	})
-	h(c)
-
-	assert := assert.New(t)
+	is.NoError(h(c))
+	is.Equal("/remove-slash", req.URL.Path)
+	is.Equal("/remove-slash", req.RequestURI)
 
-	assert.Equal("/remove-slash", req.URL.Path)
-	assert.Equal("/remove-slash", req.RequestURI)
+	// Method Connect must not fail:
+	req = httptest.NewRequest(http.MethodConnect, "", nil)
+	rec = httptest.NewRecorder()
+	c = e.NewContext(req, rec)
+	h = RemoveTrailingSlash()(func(c echo.Context) error {
+		return nil
+	})
+	is.NoError(h(c))
+	is.Equal("", req.URL.Path)
+	is.Equal("", req.RequestURI)
 
 	// With config
 	req = httptest.NewRequest(http.MethodGet, "/remove-slash/?key=value", nil)
@@ -61,9 +80,9 @@ func TestRemoveTrailingSlash(t *testing.T) {
 	})(func(c echo.Context) error {
 		return nil
 	})
-	h(c)
-	assert.Equal(http.StatusMovedPermanently, rec.Code)
-	assert.Equal("/remove-slash?key=value", rec.Header().Get(echo.HeaderLocation))
+	is.NoError(h(c))
+	is.Equal(http.StatusMovedPermanently, rec.Code)
+	is.Equal("/remove-slash?key=value", rec.Header().Get(echo.HeaderLocation))
 
 	// With bare URL
 	req = httptest.NewRequest(http.MethodGet, "http://localhost", nil)
@@ -72,6 +91,6 @@ func TestRemoveTrailingSlash(t *testing.T) {
 	h = RemoveTrailingSlash()(func(c echo.Context) error {
 		return nil
 	})
-	h(c)
-	assert.Equal("", req.URL.Path)
+	is.NoError(h(c))
+	is.Equal("", req.URL.Path)
 }