Repository: gomods/athens
Pull request #1620 (2020-05-24 20:16:35)

Fix sumdb/* paths when config.PathPrefix is set
Fixes: #1619

http.StripPrefix will look at the entire request path when called,
if we do not include config.PathPrefix then the StripPrefix call
will never receive a valid path from the application and the user
will always get a 404 error.

There were no test where I could easily check this regression so
I also added a few endpoint tests, the last test will fail with
a 404 instead of 403 if this change in not applied.

<!-- 
    Welcome, Athenian! Can you do us two quick favors before you submit your PR?
    
    1. Briefly fill out the sections below. It will make it easy for us to review your code
    2. Put "[WIP]" at the beginning of your PR title if you're not ready to have this merged yet (we have a bot that will tell everyone that it's a work in progress)
-->

## What is the problem I am trying to address?

Describe the issue you have been trying to solve.

## How is the fix applied?

Mention briefly how you have applied the fix.

## What GitHub issue(s) does this PR fix or close?

<!--
    If it doesn't fix any GitHub Issues, that's ok. Can you please delete the below "Fixes #" line for us? It would help us out a lot. Thanks!

    Your PR might fix one or more GitHub issues. If so, please use the below "Fixes #<issue number>" notation below. If your PR fixes multiple issues, please put multiple lines of "Fixes #<issue number>", one for each issue. If you do that, when this PR is merged, it'll automatically close the issue(s) you reference.
-->

Fixes #

<!-- 
example: Fixes #123
-->


elliotmr: All changes looked good to me, I used TrimRight instead of TrimSuffix because I forgot that it existed.

Patch:
diff --git a/cmd/proxy/actions/app_proxy.go b/cmd/proxy/actions/app_proxy.go
index c3e74b8a..7a2e2685 100644
--- a/cmd/proxy/actions/app_proxy.go
+++ b/cmd/proxy/actions/app_proxy.go
@@ -47,7 +47,7 @@ func addProxyRoutes(
 		sumHandler := sumdbProxy(sumdbURL, c.NoSumPatterns)
 		pathPrefix := "/sumdb/" + sumdbURL.Host
 		r.PathPrefix(pathPrefix + "/").Handler(
-			http.StripPrefix(pathPrefix, sumHandler),
+			http.StripPrefix(strings.TrimSuffix(c.PathPrefix, "/")+pathPrefix, sumHandler),
 		)
 	}
 


Test patch:
diff --git a/cmd/proxy/actions/app_proxy_test.go b/cmd/proxy/actions/app_proxy_test.go
new file mode 100644
index 00000000..5dc48140
--- /dev/null
+++ b/cmd/proxy/actions/app_proxy_test.go
@@ -0,0 +1,91 @@
+package actions
+
+import (
+	"encoding/json"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/gomods/athens/pkg/build"
+	"github.com/gomods/athens/pkg/config"
+	"github.com/gomods/athens/pkg/log"
+	"github.com/gomods/athens/pkg/storage/mem"
+	"github.com/gorilla/mux"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type routeTest struct {
+	method string
+	path   string
+	body   string
+	test   func(t *testing.T, resp *http.Response)
+}
+
+func TestProxyRoutes(t *testing.T) {
+	r := mux.NewRouter()
+	s, err := mem.NewStorage()
+	require.NoError(t, err)
+	l := log.NoOpLogger()
+	c, err := config.Load("")
+	require.NoError(t, err)
+	c.NoSumPatterns = []string{"*"} // catch all patterns with noSumWrapper to ensure the sumdb handler doesn't make a real http request to the sumdb server.
+	c.PathPrefix = "/prefix"
+	subRouter := r.PathPrefix(c.PathPrefix).Subrouter()
+	err = addProxyRoutes(subRouter, s, l, c)
+	require.NoError(t, err)
+
+	baseURL := "https://athens.azurefd.net" + c.PathPrefix
+
+	testCases := []routeTest{
+		{"GET", "/", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusOK, resp.StatusCode)
+			body, err := ioutil.ReadAll(resp.Body)
+			require.NoError(t, err)
+			assert.Equal(t, `"Welcome to The Athens Proxy"`, string(body))
+		}},
+		{"GET", "/badz", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusNotFound, resp.StatusCode)
+		}},
+		{"GET", "/healthz", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusOK, resp.StatusCode)
+		}},
+		{"GET", "/readyz", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusOK, resp.StatusCode)
+		}},
+		{"GET", "/version", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusOK, resp.StatusCode)
+			details := build.Details{}
+			err := json.NewDecoder(resp.Body).Decode(&details)
+			require.NoError(t, err)
+			assert.EqualValues(t, build.Data(), details)
+		}},
+
+		// Default sumdb is sum.golang.org
+		{"GET", "/sumdb/sum.golang.org/supported", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusOK, resp.StatusCode)
+		}},
+		{"GET", "/sumdb/sum.rust-lang.org/supported", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusNotFound, resp.StatusCode)
+		}},
+		{"GET", "/sumdb/sum.golang.org/lookup/github.com/gomods/athens", "", func(t *testing.T, resp *http.Response) {
+			assert.Equal(t, http.StatusForbidden, resp.StatusCode)
+		}},
+	}
+
+	for _, tc := range testCases {
+		req := httptest.NewRequest(
+			tc.method,
+			baseURL+tc.path,
+			strings.NewReader(tc.body),
+		)
+		t.Run(req.RequestURI, func(t *testing.T) {
+			w := httptest.NewRecorder()
+			r.ServeHTTP(w, req)
+			tc.test(t, w.Result())
+		})
+	}
+
+}