Skip to content

Commit

Permalink
Fix sumdb/* paths when config.PathPrefix is set (#1620)
Browse files Browse the repository at this point in the history
* 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 <marwan-at-work@github.com>

* Update cmd/proxy/actions/app_proxy_test.go

Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>

* Update cmd/proxy/actions/app_proxy_test.go

Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>

* Update cmd/proxy/actions/app_proxy_test.go

Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>

* Removed unneeded import of logrus

Co-authored-by: Marwan Sulaiman <marwan-at-work@github.com>
  • Loading branch information
elliotmr and Marwan Sulaiman authored Jun 5, 2020
1 parent 28d6069 commit c08aa89
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 1 deletion.
2 changes: 1 addition & 1 deletion cmd/proxy/actions/app_proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}

Expand Down
91 changes: 91 additions & 0 deletions cmd/proxy/actions/app_proxy_test.go
Original file line number Diff line number Diff line change
@@ -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())
})
}

}

0 comments on commit c08aa89

Please sign in to comment.