Repository: gorilla/mux (go)

Issue:
[bug] CORSMethodMiddleware can add the allowed methods of the wrong routes
**Describe the bug**
When using `CORSMethodMiddleware` in a Subrouter it can add allowed methods from other routes that shouldn't be present.

The issue seems to be that in `getAllMethodsForRoute` the matcher matches substrings.

**Versions**
> Go version: `go version`
```
% go version
go version go1.12.1 darwin/amd64
```

> package version: run `git rev-parse HEAD` inside the repo

Sorry, I'm not using the repo of mux, but my `go.mod` has

`github.com/gorilla/mux v1.7.3`


**Steps to Reproduce**
> How can the bug be triggered?

Create a PathPrefix with a sub router, add two routes with paths where one is a substring of the other. Eg. `/hello` and `/hello/name`. Add different allowed methods to these routes.

You can see the allowed methods of both routes when requesting the route with the longer path.

Using the code example below the response header for `/test/hello/name` looks like

`Access-Control-Allow-Methods: GET,OPTIONS,POST,GET,OPTIONS`

**Expected behavior**
> What output or behaviour were you expecting instead?

I would expect to see only the allowed methods of the route in the `Access-Control-Allow-Methods` which would be `Access-Control-Allow-Methods: GET,OPTIONS`

**Code Snippets**
> A minimum viable code snippet can be useful! (use backticks to format it).
```golang
package main

import (
	"fmt"
	"net/http"

	"github.com/gorilla/mux"
)

func Hello(w http.ResponseWriter, r *http.Request) {
	fmt.Fprint(w, "Hello")
}

func HelloName(w http.ResponseWriter, r *http.Request) {
	fmt.Fprint(w, "Hello")
}

func main() {
	router := mux.NewRouter().StrictSlash(true)

	subrouter := router.PathPrefix("/test").Subrouter()
	subrouter.HandleFunc("/hello", Hello).Methods(http.MethodGet, http.MethodOptions, http.MethodPost)
	subrouter.HandleFunc("/hello/{name}", HelloName).Methods(http.MethodGet, http.MethodOptions)

	subrouter.Use(mux.CORSMethodMiddleware(subrouter))

	http.ListenAndServe(":8081", router)
}

```

Pull request:
Fix the CORSMethodMiddleware bug with subrouters
Fixes #534 

**Summary of Changes**

1. Adds a test case for the repro given in issue #534
2. Fixes the logic in CORSMethodMiddleware to handle matching routes better (now more closely mirrors the way routes are actually matched, just ignoring method mismatch errors)


Patch:
diff --git a/context.go b/context.go
new file mode 100644
index 0000000..665940a
--- /dev/null
+++ b/context.go
@@ -0,0 +1,18 @@
+package mux
+
+import (
+	"context"
+	"net/http"
+)
+
+func contextGet(r *http.Request, key interface{}) interface{} {
+	return r.Context().Value(key)
+}
+
+func contextSet(r *http.Request, key, val interface{}) *http.Request {
+	if val == nil {
+		return r
+	}
+
+	return r.WithContext(context.WithValue(r.Context(), key, val))
+}
diff --git a/middleware.go b/middleware.go
index cf2b26d..cb51c56 100644
--- a/middleware.go
+++ b/middleware.go
@@ -58,22 +58,17 @@ func CORSMethodMiddleware(r *Router) MiddlewareFunc {
 func getAllMethodsForRoute(r *Router, req *http.Request) ([]string, error) {
 	var allMethods []string
 
-	err := r.Walk(func(route *Route, _ *Router, _ []*Route) error {
-		for _, m := range route.matchers {
-			if _, ok := m.(*routeRegexp); ok {
-				if m.Match(req, &RouteMatch{}) {
-					methods, err := route.GetMethods()
-					if err != nil {
-						return err
-					}
-
-					allMethods = append(allMethods, methods...)
-				}
-				break
+	for _, route := range r.routes {
+		var match RouteMatch
+		if route.Match(req, &match) || match.MatchErr == ErrMethodMismatch {
+			methods, err := route.GetMethods()
+			if err != nil {
+				return nil, err
 			}
+
+			allMethods = append(allMethods, methods...)
 		}
-		return nil
-	})
+	}
 
-	return allMethods, err
+	return allMethods, nil
 }
diff --git a/mux.go b/mux.go
index c9ba647..26f9582 100644
--- a/mux.go
+++ b/mux.go
@@ -5,7 +5,6 @@
 package mux
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -59,7 +58,8 @@ type Router struct {
 
 	// If true, do not clear the request context after handling the request.
 	//
-	// Deprecated: No effect, since the context is stored on the request itself.
+	// Deprecated: No effect when go1.7+ is used, since the context is stored
+	// on the request itself.
 	KeepContext bool
 
 	// Slice of middlewares to be called after a match is found
@@ -195,8 +195,8 @@ func (r *Router) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	var handler http.Handler
 	if r.Match(req, &match) {
 		handler = match.Handler
-		req = requestWithVars(req, match.Vars)
-		req = requestWithRoute(req, match.Route)
+		req = setVars(req, match.Vars)
+		req = setCurrentRoute(req, match.Route)
 	}
 
 	if handler == nil && match.MatchErr == ErrMethodMismatch {
@@ -426,7 +426,7 @@ const (
 
 // Vars returns the route variables for the current request, if any.
 func Vars(r *http.Request) map[string]string {
-	if rv := r.Context().Value(varsKey); rv != nil {
+	if rv := contextGet(r, varsKey); rv != nil {
 		return rv.(map[string]string)
 	}
 	return nil
@@ -438,20 +438,18 @@ func Vars(r *http.Request) map[string]string {
 // after the handler returns, unless the KeepContext option is set on the
 // Router.
 func CurrentRoute(r *http.Request) *Route {
-	if rv := r.Context().Value(routeKey); rv != nil {
+	if rv := contextGet(r, routeKey); rv != nil {
 		return rv.(*Route)
 	}
 	return nil
 }
 
-func requestWithVars(r *http.Request, vars map[string]string) *http.Request {
-	ctx := context.WithValue(r.Context(), varsKey, vars)
-	return r.WithContext(ctx)
+func setVars(r *http.Request, val interface{}) *http.Request {
+	return contextSet(r, varsKey, val)
 }
 
-func requestWithRoute(r *http.Request, route *Route) *http.Request {
-	ctx := context.WithValue(r.Context(), routeKey, route)
-	return r.WithContext(ctx)
+func setCurrentRoute(r *http.Request, val interface{}) *http.Request {
+	return contextSet(r, routeKey, val)
 }
 
 // ----------------------------------------------------------------------------


Test patch:
diff --git a/context_test.go b/context_test.go
new file mode 100644
index 0000000..d8a56b4
--- /dev/null
+++ b/context_test.go
@@ -0,0 +1,30 @@
+package mux
+
+import (
+	"context"
+	"net/http"
+	"testing"
+	"time"
+)
+
+func TestNativeContextMiddleware(t *testing.T) {
+	withTimeout := func(h http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			ctx, cancel := context.WithTimeout(r.Context(), time.Minute)
+			defer cancel()
+			h.ServeHTTP(w, r.WithContext(ctx))
+		})
+	}
+
+	r := NewRouter()
+	r.Handle("/path/{foo}", withTimeout(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		vars := Vars(r)
+		if vars["foo"] != "bar" {
+			t.Fatal("Expected foo var to be set")
+		}
+	})))
+
+	rec := NewRecorder()
+	req := newRequest("GET", "/path/bar")
+	r.ServeHTTP(rec, req)
+}
diff --git a/middleware_test.go b/middleware_test.go
index 27647af..e9f0ef5 100644
--- a/middleware_test.go
+++ b/middleware_test.go
@@ -478,6 +478,26 @@ func TestCORSMethodMiddleware(t *testing.T) {
 	}
 }
 
+func TestCORSMethodMiddlewareSubrouter(t *testing.T) {
+	router := NewRouter().StrictSlash(true)
+
+	subrouter := router.PathPrefix("/test").Subrouter()
+	subrouter.HandleFunc("/hello", stringHandler("a")).Methods(http.MethodGet, http.MethodOptions, http.MethodPost)
+	subrouter.HandleFunc("/hello/{name}", stringHandler("b")).Methods(http.MethodGet, http.MethodOptions)
+
+	subrouter.Use(CORSMethodMiddleware(subrouter))
+
+	rw := NewRecorder()
+	req := newRequest("GET", "/test/hello/asdf")
+	router.ServeHTTP(rw, req)
+
+	actualMethods := rw.Header().Get("Access-Control-Allow-Methods")
+	expectedMethods := "GET,OPTIONS"
+	if actualMethods != expectedMethods {
+		t.Fatalf("expected methods %q but got: %q", expectedMethods, actualMethods)
+	}
+}
+
 func TestMiddlewareOnMultiSubrouter(t *testing.T) {
 	first := "first"
 	second := "second"
diff --git a/mux_test.go b/mux_test.go
index 1c90668..9a740bb 100644
--- a/mux_test.go
+++ b/mux_test.go
@@ -7,7 +7,6 @@ package mux
 import (
 	"bufio"
 	"bytes"
-	"context"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -17,7 +16,6 @@ import (
 	"reflect"
 	"strings"
 	"testing"
-	"time"
 )
 
 func (r *Route) GoString() string {
@@ -2806,28 +2804,6 @@ func TestSubrouterNotFound(t *testing.T) {
 	}
 }
 
-func TestContextMiddleware(t *testing.T) {
-	withTimeout := func(h http.Handler) http.Handler {
-		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			ctx, cancel := context.WithTimeout(r.Context(), time.Minute)
-			defer cancel()
-			h.ServeHTTP(w, r.WithContext(ctx))
-		})
-	}
-
-	r := NewRouter()
-	r.Handle("/path/{foo}", withTimeout(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		vars := Vars(r)
-		if vars["foo"] != "bar" {
-			t.Fatal("Expected foo var to be set")
-		}
-	})))
-
-	rec := NewRecorder()
-	req := newRequest("GET", "/path/bar")
-	r.ServeHTTP(rec, req)
-}
-
 // mapToPairs converts a string map to a slice of string pairs
 func mapToPairs(m map[string]string) []string {
 	var i int
diff --git a/test_helpers.go b/test_helpers.go
index 5f5c496..32ecffd 100644
--- a/test_helpers.go
+++ b/test_helpers.go
@@ -15,5 +15,5 @@ import "net/http"
 // can be set by making a route that captures the required variables,
 // starting a server and sending the request to that server.
 func SetURLVars(r *http.Request, val map[string]string) *http.Request {
-	return requestWithVars(r, val)
+	return setVars(r, val)
 }