From 4c7d7872868361d79796e87cca2d4cf5d0e95824 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 9 Jun 2020 16:26:44 +1000 Subject: Address MR feedback use correct reference --- acceptance_test.go | 34 +++++++++++++++------- app.go | 28 +++++++++--------- internal/auth/auth_test.go | 23 ++++++++++----- internal/domain/domain.go | 13 +++++---- internal/domain/domain_test.go | 7 ++--- .../group.auth.gitlab-example.com/public/404.html | 1 + .../group.auth/private.project.1/public/404.html | 1 + 7 files changed, 66 insertions(+), 41 deletions(-) create mode 100644 shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html create mode 100644 shared/pages/group.auth/private.project.1/public/404.html diff --git a/acceptance_test.go b/acceptance_test.go index 2036c263..2b0a7800 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1228,17 +1228,29 @@ func TestCustomErrorPageWithAuth(t *testing.T) { expectedErrorPage: "Private custom 404 error page", }, { - name: "public_namespace_with_private_unauthorized_project", - domain: "group.404.gitlab-example.com", + name: "public_namespace_with_private_unauthorized_project", + domain: "group.404.gitlab-example.com", + // /private_unauthorized/config.json resolves project ID to 2000 which will cause a 401 from the mock GitLab testServer path: "/private_unauthorized/unknown", expectedErrorPage: "Custom 404 group page", }, + { + name: "private_namespace_authorized", + domain: "group.auth.gitlab-example.com", + path: "/unknown", + expectedErrorPage: "group.auth.gitlab-example.com namespace custom 404", + }, + { + name: "private_namespace_with_private_project_auth_failed", + domain: "group.auth.gitlab-example.com", + // project ID is 2000 + path: "/private.project.1/unknown", + expectedErrorPage: "The page you're looking for could not be found.", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - domain := tt.domain - path := tt.path - rsp, err := GetRedirectPage(t, httpListener, domain, path) + rsp, err := GetRedirectPage(t, httpListener, tt.domain, tt.path) require.NoError(t, err) defer rsp.Body.Close() @@ -1248,7 +1260,7 @@ func TestCustomErrorPageWithAuth(t *testing.T) { require.NoError(t, err) state := url.Query().Get("state") - require.Equal(t, "http://"+domain, url.Query().Get("domain")) + require.Equal(t, "http://"+tt.domain, url.Query().Get("domain")) pagesrsp, err := GetRedirectPage(t, httpListener, url.Host, url.Path+"?"+url.RawQuery) require.NoError(t, err) @@ -1267,12 +1279,12 @@ func TestCustomErrorPageWithAuth(t *testing.T) { require.NoError(t, err) // Will redirect to custom domain - require.Equal(t, domain, url.Host) + require.Equal(t, tt.domain, url.Host) require.Equal(t, "1", url.Query().Get("code")) require.Equal(t, state, url.Query().Get("state")) // Run auth callback in custom domain - authrsp, err = GetRedirectPageWithCookie(t, httpListener, domain, "/auth?code=1&state="+ + authrsp, err = GetRedirectPageWithCookie(t, httpListener, tt.domain, "/auth?code=1&state="+ state, cookie) require.NoError(t, err) @@ -1286,10 +1298,10 @@ func TestCustomErrorPageWithAuth(t *testing.T) { require.NoError(t, err) // Will redirect to custom domain error page - require.Equal(t, "http://"+domain+path, url.String()) + require.Equal(t, "http://"+tt.domain+tt.path, url.String()) // Fetch page in custom domain - anotherResp, err := GetRedirectPageWithCookie(t, httpListener, domain, path, groupCookie) + anotherResp, err := GetRedirectPageWithCookie(t, httpListener, tt.domain, tt.path, groupCookie) require.NoError(t, err) require.Equal(t, http.StatusNotFound, anotherResp.StatusCode) @@ -1372,7 +1384,7 @@ func TestAccessControlGroupDomain404RedirectsAuth(t *testing.T) { teardown := RunPagesProcessWithAuth(t, *pagesBinary, listeners, "") defer teardown() - rsp, err := GetRedirectPage(t, httpListener, "group.auth.gitlab-example.com", "/nonexistent/") + rsp, err := GetRedirectPage(t, httpListener, "group.gitlab-example.com", "/nonexistent/") require.NoError(t, err) defer rsp.Body.Close() require.Equal(t, http.StatusFound, rsp.StatusCode) diff --git a/app.go b/app.go index 4ad4ffa3..5a195396 100644 --- a/app.go +++ b/app.go @@ -94,21 +94,18 @@ func (a *theApp) domain(host string) (*domain.Domain, error) { return a.domains.GetDomain(host) } -func (a *theApp) checkAuthenticationIfNotExists(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool { - if !domain.HasLookupPath(r) { - // Only if auth is supported - if a.Auth.IsAuthSupported() { - // To avoid user knowing if pages exist, we will force user to login and authorize pages - if a.Auth.CheckAuthenticationWithoutProject(w, r, domain) { - return true - } - } - - domain.ServeNotFoundAuthFailed(w, r) +// checkAuthAndServeNotFound performs the auth process if domain can't be found +// the main purpose of this process is to avoid leaking the project existence/not-existence +// by behaving the same if user has no access to the project or if project simply does not exists +func (a *theApp) checkAuthAndServeNotFound(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool { + // To avoid user knowing if pages exist, we will force user to login and authorize pages + if a.Auth.CheckAuthenticationWithoutProject(w, r, domain) { return true } - return false + // auth succeeded try to serve the correct 404 page + domain.ServeNotFoundAuthFailed(w, r) + return true } func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, https bool, host string, domain *domain.Domain) bool { @@ -127,8 +124,11 @@ func (a *theApp) tryAuxiliaryHandlers(w http.ResponseWriter, r *http.Request, ht return true } - if a.checkAuthenticationIfNotExists(domain, w, r) { - return true + if !domain.HasLookupPath(r) { + // redirect to auth and serve not found + if a.checkAuthAndServeNotFound(domain, w, r) { + return true + } } if !https && domain.IsHTTPSOnly(r) { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index ffaf5b6f..39a533b3 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -30,7 +30,8 @@ func defaultCookieStore() sessions.Store { } type domainMock struct { - projectID uint64 + projectID uint64 + notFoundContent string } func (dm *domainMock) GetProjectID(r *http.Request) uint64 { @@ -39,6 +40,7 @@ func (dm *domainMock) GetProjectID(r *http.Request) uint64 { func (dm *domainMock) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNotFound) + w.Write([]byte(dm.notFoundContent)) } // Gorilla's sessions use request context to save session @@ -195,7 +197,7 @@ func TestCheckAuthenticationWhenAccess(t *testing.T) { contentServed := auth.CheckAuthentication(result, r, &domainMock{projectID: 1000}) require.False(t, contentServed) - // content wasn't served so the default response from CheckAuthentication should be 200 + // notFoundContent wasn't served so the default response from CheckAuthentication should be 200 require.Equal(t, 200, result.Code) } @@ -223,7 +225,8 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) { "http://pages.gitlab-example.com/auth", apiServer.URL) - result := httptest.NewRecorder() + w := httptest.NewRecorder() + reqURL, err := url.Parse("/auth?code=1&state=state") require.NoError(t, err) reqURL.Scheme = request.SchemeHTTPS @@ -231,12 +234,18 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) { session, _ := store.Get(r, "gitlab-pages") session.Values["access_token"] = "abc" - session.Save(r, result) + session.Save(r, w) - contentServed := auth.CheckAuthentication(result, r, &domainMock{projectID: 1000}) + contentServed := auth.CheckAuthentication(w, r, &domainMock{projectID: 1000, notFoundContent: "Generic 404"}) require.True(t, contentServed) - // content wasn't served so the default response from CheckAuthentication should be 200 - require.Equal(t, 404, result.Code) + res := w.Result() + defer res.Body.Close() + + require.Equal(t, 404, res.StatusCode) + + body, err := ioutil.ReadAll(res.Body) + require.NoError(t, err) + require.Equal(t, string(body), "Generic 404") } func TestCheckAuthenticationWhenInvalidToken(t *testing.T) { diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 9fa843a3..7c1639a3 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -1,6 +1,7 @@ package domain import ( + "context" "crypto/tls" "errors" "net/http" @@ -173,16 +174,18 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { // that failed authentication so that we serve the custom namespace error page for // public namespace domains func (d *Domain) serveNamespaceNotFound(w http.ResponseWriter, r *http.Request) { - // override the path and try to resolve the domain name - r.URL.Path = "/" - namespaceDomain, err := d.Resolver.Resolve(r) + // clone r and override the path and try to resolve the domain name + clonedReq := r.Clone(context.Background()) + clonedReq.URL.Path = "/" + + namespaceDomain, err := d.Resolver.Resolve(clonedReq) if err != nil || namespaceDomain.LookupPath == nil { httperrors.Serve404(w) return } // for namespace domains that have no access control enabled - if namespaceDomain.LookupPath.IsNamespaceProject && !namespaceDomain.LookupPath.HasAccessControl { + if !namespaceDomain.LookupPath.HasAccessControl { namespaceDomain.ServeNotFoundHTTP(w, r) return } @@ -197,7 +200,7 @@ func (d *Domain) ServeNotFoundAuthFailed(w http.ResponseWriter, r *http.Request) httperrors.Serve404(w) return } - if d.IsNamespaceProject(r) { + if d.IsNamespaceProject(r) && !d.GetLookupPath(r).HasAccessControl { d.ServeNotFoundHTTP(w, r) return } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 9e89f0e5..fc5611ba 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -157,10 +157,7 @@ func chdirInPath(t require.TestingT, path string) func() { } } -func TestDomain_ServeNamespaceNotFound(t *testing.T) { - // defaultNotFound := "The page you're looking for could not be found." - // customNotFound := "Custom error page" - +func TestServeNamespaceNotFound(t *testing.T) { tests := []struct { name string domain string @@ -231,6 +228,8 @@ func TestDomain_ServeNamespaceNotFound(t *testing.T) { d.serveNamespaceNotFound(w, r) resp := w.Result() + defer resp.Body.Close() + require.Equal(t, http.StatusNotFound, resp.StatusCode) body, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) diff --git a/shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html b/shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html new file mode 100644 index 00000000..f345e8bc --- /dev/null +++ b/shared/pages/group.auth/group.auth.gitlab-example.com/public/404.html @@ -0,0 +1 @@ +group.auth.gitlab-example.com namespace custom 404 diff --git a/shared/pages/group.auth/private.project.1/public/404.html b/shared/pages/group.auth/private.project.1/public/404.html new file mode 100644 index 00000000..3b751385 --- /dev/null +++ b/shared/pages/group.auth/private.project.1/public/404.html @@ -0,0 +1 @@ +group.auth.gitlab-example.com/private.project.1 custom 404 -- cgit v1.2.3