Repository: gorilla/mux (go)

Issue:
Make CORSMethodMiddleware actually make sense
I was the one that added `CORSMethodMiddleware`, and I feel bad about it looking back, because it's implementation is terrible. I didn't understand enough about CORS—just had some half-baked idea about something that might be cool and decided to PR it. It's understandably [caused confusion](https://github.com/gorilla/mux/issues/381). I would like to change it in the following ways so it's actually useful:

* Only set the header if an `OPTIONS` method matcher exists for the route
* Set `Access-Control-Allow-Methods` on all requests (not just preflight, see https://stackoverflow.com/questions/24264574/cors-headers-present-only-on-preflight-or-every-request)
* Do not return after setting the header. Since people need to create their own `OPTIONS` handlers for this middleware to work at all, and they might want to do other things in the `OPTIONS` handler (such as set other CORS headers), it doesn't make sense to return
* Do not append `OPTIONS` to the allowed methods. This is currently a bug. If there isn't an `OPTIONS` matcher the middleware will only set the header on non-`OPTIONS` requests (which is invalid) and it will say that there is an `OPTIONS` method when there isn't. If there is an `OPTIONS` matcher it will return an `Access-Control-Allow-Methods` with two `OPTIONS` (not really invalid, but bad)
* Vastly improve the documentation to document all this behavior
* Add more (better documented) tests
* Add an example test

I believe making these changes would both make the middleware more intuitive and practical to use. It is a fairly breaking change but I think it's worth it since the current behavior is practically useless.

Pull request description:
Improve CORS Method Middleware
This PR implements the behavior outlined in #476:

* Sets `Access-Control-Allow-Methods` on all requests to a route with an OPTIONS method matcher
* Does not return after setting the `Access-Control-Allow-Methods` header
* Does not append `OPTIONS` header to `Access-Control-Allow-Methods` regardless of whether there is an OPTIONS method matcher. Only returns `OPTIONS` if there is an actual matcher
* Adds tests for the above behavior

Closes #476

Patch:
diff --git a/.circleci/config.yml b/.circleci/config.yml
index c0fb9de..d7d96d1 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -11,8 +11,20 @@ jobs:
       - checkout
       - run: go version
       - run: go get -t -v ./...
-      - run: diff -u <(echo -n) <(gofmt -d .)
-      - run: if [[ "$LATEST" = true ]]; then go vet -v .; fi
+      # Only run gofmt, vet & lint against the latest Go version
+      - run: >
+          if [[ "$LATEST" = true ]]; then
+            go get -u golang.org/x/lint/golint
+            golint ./...
+          fi
+      - run: >
+          if [[ "$LATEST" = true ]]; then
+            diff -u <(echo -n) <(gofmt -d .)
+          fi
+      - run: >
+          if [[ "$LATEST" = true ]]; then
+            go vet -v .
+          fi
       - run: go test -v -race ./...
 
   "latest":
diff --git a/README.md b/README.md
index 08f63a1..92e422e 100644
--- a/README.md
+++ b/README.md
@@ -30,6 +30,7 @@ The name mux stands for "HTTP request multiplexer". Like the standard `http.Serv
 * [Walking Routes](#walking-routes)
 * [Graceful Shutdown](#graceful-shutdown)
 * [Middleware](#middleware)
+* [Handling CORS Requests](#handling-cors-requests)
 * [Testing Handlers](#testing-handlers)
 * [Full Example](#full-example)
 
@@ -492,6 +493,73 @@ r.Use(amw.Middleware)
 
 Note: The handler chain will be stopped if your middleware doesn't call `next.ServeHTTP()` with the corresponding parameters. This can be used to abort a request if the middleware writer wants to. Middlewares _should_ write to `ResponseWriter` if they _are_ going to terminate the request, and they _should not_ write to `ResponseWriter` if they _are not_ going to terminate it.
 
+### Handling CORS Requests
+
+[CORSMethodMiddleware](https://godoc.org/github.com/gorilla/mux#CORSMethodMiddleware) intends to make it easier to strictly set the `Access-Control-Allow-Methods` response header.
+
+* You will still need to use your own CORS handler to set the other CORS headers such as `Access-Control-Allow-Origin`
+* The middleware will set the `Access-Control-Allow-Methods` header to all the method matchers (e.g. `r.Methods(http.MethodGet, http.MethodPut, http.MethodOptions)` -> `Access-Control-Allow-Methods: GET,PUT,OPTIONS`) on a route
+* If you do not specify any methods, then:
+> _Important_: there must be an `OPTIONS` method matcher for the middleware to set the headers.
+
+Here is an example of using `CORSMethodMiddleware` along with a custom `OPTIONS` handler to set all the required CORS headers:
+
+```go
+package main
+
+import (
+	"net/http"
+	"github.com/gorilla/mux"
+)
+
+func main() {
+    r := mux.NewRouter()
+
+    // IMPORTANT: you must specify an OPTIONS method matcher for the middleware to set CORS headers
+    r.HandleFunc("/foo", fooHandler).Methods(http.MethodGet, http.MethodPut, http.MethodPatch, http.MethodOptions)
+    r.Use(mux.CORSMethodMiddleware(r))
+    
+    http.ListenAndServe(":8080", r)
+}
+
+func fooHandler(w http.ResponseWriter, r *http.Request) {
+    w.Header().Set("Access-Control-Allow-Origin", "*")
+    if r.Method == http.MethodOptions {
+        return
+    }
+
+    w.Write([]byte("foo"))
+}
+```
+
+And an request to `/foo` using something like:
+
+```bash
+curl localhost:8080/foo -v
+```
+
+Would look like:
+
+```bash
+*   Trying ::1...
+* TCP_NODELAY set
+* Connected to localhost (::1) port 8080 (#0)
+> GET /foo HTTP/1.1
+> Host: localhost:8080
+> User-Agent: curl/7.59.0
+> Accept: */*
+> 
+< HTTP/1.1 200 OK
+< Access-Control-Allow-Methods: GET,PUT,PATCH,OPTIONS
+< Access-Control-Allow-Origin: *
+< Date: Fri, 28 Jun 2019 20:13:30 GMT
+< Content-Length: 3
+< Content-Type: text/plain; charset=utf-8
+< 
+* Connection #0 to host localhost left intact
+foo
+```
+
 ### Testing Handlers
 
 Testing handlers in a Go web application is straightforward, and _mux_ doesn't complicate this any further. Given two files: `endpoints.go` and `endpoints_test.go`, here's how we'd test an application using _mux_.
diff --git a/middleware.go b/middleware.go
index ceb812c..cf2b26d 100644
--- a/middleware.go
+++ b/middleware.go
@@ -32,37 +32,19 @@ func (r *Router) useInterface(mw middleware) {
 	r.middlewares = append(r.middlewares, mw)
 }
 
-// CORSMethodMiddleware sets the Access-Control-Allow-Methods response header
-// on a request, by matching routes based only on paths. It also handles
-// OPTIONS requests, by settings Access-Control-Allow-Methods, and then
-// returning without calling the next http handler.
+// CORSMethodMiddleware automatically sets the Access-Control-Allow-Methods response header
+// on requests for routes that have an OPTIONS method matcher to all the method matchers on
+// the route. Routes that do not explicitly handle OPTIONS requests will not be processed
+// by the middleware. See examples for usage.
 func CORSMethodMiddleware(r *Router) MiddlewareFunc {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-			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
-					}
-				}
-				return nil
-			})
-
+			allMethods, err := getAllMethodsForRoute(r, req)
 			if err == nil {
-				w.Header().Set("Access-Control-Allow-Methods", strings.Join(append(allMethods, "OPTIONS"), ","))
-
-				if req.Method == "OPTIONS" {
-					return
+				for _, v := range allMethods {
+					if v == http.MethodOptions {
+						w.Header().Set("Access-Control-Allow-Methods", strings.Join(allMethods, ","))
+					}
 				}
 			}
 
@@ -70,3 +52,28 @@ func CORSMethodMiddleware(r *Router) MiddlewareFunc {
 		})
 	}
 }
+
+// getAllMethodsForRoute returns all the methods from method matchers matching a given
+// request.
+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
+			}
+		}
+		return nil
+	})
+
+	return allMethods, err
+}


Test patch:
diff --git a/example_cors_method_middleware_test.go b/example_cors_method_middleware_test.go
new file mode 100644
index 0000000..00929fc
--- /dev/null
+++ b/example_cors_method_middleware_test.go
@@ -0,0 +1,37 @@
+package mux_test
+
+import (
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+
+	"github.com/gorilla/mux"
+)
+
+func ExampleCORSMethodMiddleware() {
+	r := mux.NewRouter()
+
+	r.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
+		// Handle the request
+	}).Methods(http.MethodGet, http.MethodPut, http.MethodPatch)
+	r.HandleFunc("/foo", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Access-Control-Allow-Origin", "http://example.com")
+		w.Header().Set("Access-Control-Max-Age", "86400")
+	}).Methods(http.MethodOptions)
+
+	r.Use(mux.CORSMethodMiddleware(r))
+
+	rw := httptest.NewRecorder()
+	req, _ := http.NewRequest("OPTIONS", "/foo", nil)                 // needs to be OPTIONS
+	req.Header.Set("Access-Control-Request-Method", "POST")           // needs to be non-empty
+	req.Header.Set("Access-Control-Request-Headers", "Authorization") // needs to be non-empty
+	req.Header.Set("Origin", "http://example.com")                    // needs to be non-empty
+
+	r.ServeHTTP(rw, req)
+
+	fmt.Println(rw.Header().Get("Access-Control-Allow-Methods"))
+	fmt.Println(rw.Header().Get("Access-Control-Allow-Origin"))
+	// Output:
+	// GET,PUT,PATCH,OPTIONS
+	// http://example.com
+}
diff --git a/middleware_test.go b/middleware_test.go
index 30df274..27647af 100644
--- a/middleware_test.go
+++ b/middleware_test.go
@@ -2,9 +2,7 @@ package mux
 
 import (
 	"bytes"
-	"fmt"
 	"net/http"
-	"net/http/httptest"
 	"testing"
 )
 
@@ -367,42 +365,114 @@ func TestMiddlewareMethodMismatchSubrouter(t *testing.T) {
 }
 
 func TestCORSMethodMiddleware(t *testing.T) {
-	router := NewRouter()
-
-	cases := []struct {
-		path                   string
-		response               string
-		method                 string
-		testURL                string
-		expectedAllowedMethods string
+	testCases := []struct {
+		name                                    string
+		registerRoutes                          func(r *Router)
+		requestHeader                           http.Header
+		requestMethod                           string
+		requestPath                             string
+		expectedAccessControlAllowMethodsHeader string
+		expectedResponse                        string
 	}{
-		{"/g/{o}", "a", "POST", "/g/asdf", "POST,PUT,GET,OPTIONS"},
-		{"/g/{o}", "b", "PUT", "/g/bla", "POST,PUT,GET,OPTIONS"},
-		{"/g/{o}", "c", "GET", "/g/orilla", "POST,PUT,GET,OPTIONS"},
-		{"/g", "d", "POST", "/g", "POST,OPTIONS"},
+		{
+			name: "does not set without OPTIONS matcher",
+			registerRoutes: func(r *Router) {
+				r.HandleFunc("/foo", stringHandler("a")).Methods(http.MethodGet, http.MethodPut, http.MethodPatch)
+			},
+			requestMethod:                           "GET",
+			requestPath:                             "/foo",
+			expectedAccessControlAllowMethodsHeader: "",
+			expectedResponse:                        "a",
+		},
+		{
+			name: "sets on non OPTIONS",
+			registerRoutes: func(r *Router) {
+				r.HandleFunc("/foo", stringHandler("a")).Methods(http.MethodGet, http.MethodPut, http.MethodPatch)
+				r.HandleFunc("/foo", stringHandler("b")).Methods(http.MethodOptions)
+			},
+			requestMethod:                           "GET",
+			requestPath:                             "/foo",
+			expectedAccessControlAllowMethodsHeader: "GET,PUT,PATCH,OPTIONS",
+			expectedResponse:                        "a",
+		},
+		{
+			name: "sets without preflight headers",
+			registerRoutes: func(r *Router) {
+				r.HandleFunc("/foo", stringHandler("a")).Methods(http.MethodGet, http.MethodPut, http.MethodPatch)
+				r.HandleFunc("/foo", stringHandler("b")).Methods(http.MethodOptions)
+			},
+			requestMethod:                           "OPTIONS",
+			requestPath:                             "/foo",
+			expectedAccessControlAllowMethodsHeader: "GET,PUT,PATCH,OPTIONS",
+			expectedResponse:                        "b",
+		},
+		{
+			name: "does not set on error",
+			registerRoutes: func(r *Router) {
+				r.HandleFunc("/foo", stringHandler("a"))
+			},
+			requestMethod:                           "OPTIONS",
+			requestPath:                             "/foo",
+			expectedAccessControlAllowMethodsHeader: "",
+			expectedResponse:                        "a",
+		},
+		{
+			name: "sets header on valid preflight",
+			registerRoutes: func(r *Router) {
+				r.HandleFunc("/foo", stringHandler("a")).Methods(http.MethodGet, http.MethodPut, http.MethodPatch)
+				r.HandleFunc("/foo", stringHandler("b")).Methods(http.MethodOptions)
+			},
+			requestMethod: "OPTIONS",
+			requestPath:   "/foo",
+			requestHeader: http.Header{
+				"Access-Control-Request-Method":  []string{"GET"},
+				"Access-Control-Request-Headers": []string{"Authorization"},
+				"Origin":                         []string{"http://example.com"},
+			},
+			expectedAccessControlAllowMethodsHeader: "GET,PUT,PATCH,OPTIONS",
+			expectedResponse:                        "b",
+		},
+		{
+			name: "does not set methods from unmatching routes",
+			registerRoutes: func(r *Router) {
+				r.HandleFunc("/foo", stringHandler("c")).Methods(http.MethodDelete)
+				r.HandleFunc("/foo/bar", stringHandler("a")).Methods(http.MethodGet, http.MethodPut, http.MethodPatch)
+				r.HandleFunc("/foo/bar", stringHandler("b")).Methods(http.MethodOptions)
+			},
+			requestMethod: "OPTIONS",
+			requestPath:   "/foo/bar",
+			requestHeader: http.Header{
+				"Access-Control-Request-Method":  []string{"GET"},
+				"Access-Control-Request-Headers": []string{"Authorization"},
+				"Origin":                         []string{"http://example.com"},
+			},
+			expectedAccessControlAllowMethodsHeader: "GET,PUT,PATCH,OPTIONS",
+			expectedResponse:                        "b",
+		},
 	}
 
-	for _, tt := range cases {
-		router.HandleFunc(tt.path, stringHandler(tt.response)).Methods(tt.method)
-	}
+	for _, tt := range testCases {
+		t.Run(tt.name, func(t *testing.T) {
+			router := NewRouter()
 
-	router.Use(CORSMethodMiddleware(router))
+			tt.registerRoutes(router)
 
-	for i, tt := range cases {
-		t.Run(fmt.Sprintf("cases[%d]", i), func(t *testing.T) {
-			rr := httptest.NewRecorder()
-			req := newRequest(tt.method, tt.testURL)
+			router.Use(CORSMethodMiddleware(router))
 
-			router.ServeHTTP(rr, req)
+			rw := NewRecorder()
+			req := newRequest(tt.requestMethod, tt.requestPath)
+			req.Header = tt.requestHeader
 
-			if rr.Body.String() != tt.response {
-				t.Errorf("Expected body '%s', found '%s'", tt.response, rr.Body.String())
-			}
+			router.ServeHTTP(rw, req)
 
-			allowedMethods := rr.Header().Get("Access-Control-Allow-Methods")
+			actualMethodsHeader := rw.Header().Get("Access-Control-Allow-Methods")
+			if actualMethodsHeader != tt.expectedAccessControlAllowMethodsHeader {
+				t.Fatalf("Expected Access-Control-Allow-Methods to equal %s but got %s", tt.expectedAccessControlAllowMethodsHeader, actualMethodsHeader)
+			}
 
-			if allowedMethods != tt.expectedAllowedMethods {
-				t.Errorf("Expected Access-Control-Allow-Methods '%s', found '%s'", tt.expectedAllowedMethods, allowedMethods)
+			actualResponse := rw.Body.String()
+			if actualResponse != tt.expectedResponse {
+				t.Fatalf("Expected response to equal %s but got %s", tt.expectedResponse, actualResponse)
 			}
 		})
 	}