From c08aa890cbe2e76f001801e24ab769f4a28fe61a Mon Sep 17 00:00:00 2001 From: Elliot Morrison-Reed Date: Fri, 5 Jun 2020 12:01:38 -0400 Subject: [PATCH] Fix sumdb/* paths when config.PathPrefix is set (#1620) * Fix sumdb/* paths when config.PathPrefix is set 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. * Update cmd/proxy/actions/app_proxy.go Co-authored-by: Marwan Sulaiman * Update cmd/proxy/actions/app_proxy_test.go Co-authored-by: Marwan Sulaiman * Update cmd/proxy/actions/app_proxy_test.go Co-authored-by: Marwan Sulaiman * Update cmd/proxy/actions/app_proxy_test.go Co-authored-by: Marwan Sulaiman * Removed unneeded import of logrus Co-authored-by: Marwan Sulaiman --- cmd/proxy/actions/app_proxy.go | 2 +- cmd/proxy/actions/app_proxy_test.go | 91 +++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 cmd/proxy/actions/app_proxy_test.go diff --git a/cmd/proxy/actions/app_proxy.go b/cmd/proxy/actions/app_proxy.go index c3e74b8a1..7a2e26853 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), ) } diff --git a/cmd/proxy/actions/app_proxy_test.go b/cmd/proxy/actions/app_proxy_test.go new file mode 100644 index 000000000..5dc481404 --- /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()) + }) + } + +}