diff --git a/.golangci.yml b/.golangci.yml index 2df4432484a47762e2a266fe95115e1246344826..5c60dcfba2e3e14cde4725aef3181b639767c6c5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -29,10 +29,9 @@ linters-settings: linters: disable-all: true enable: -# TODO: enable these linters on a separate MR https://gitlab.com/gitlab-org/gitlab-pages/-/issues/385#linters -# - bodyclose -# - deadcode -# - dogsled + - bodyclose + - deadcode + - dogsled - goconst - gocyclo - goimports @@ -40,14 +39,14 @@ linters: - gosimple - govet - gosec -# - ineffassign -# - misspell -# - structcheck -# - typecheck -# - unconvert -# - unused -# - varcheck -# - whitespace + - ineffassign + - misspell + - structcheck + - typecheck + - unconvert + - unused + - varcheck + - whitespace fast: false issues: diff --git a/acceptance_test.go b/acceptance_test.go index 4a1b30286df06c23ad0bc5028af4baad81811cfa..1cd854d319a4e012213d83f2c7cf6fdb2b6a09e8 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -291,7 +291,6 @@ func TestKnownHostWithPortReturns200(t *testing.T) { rsp.Body.Close() require.Equal(t, http.StatusOK, rsp.StatusCode) } - } func TestHttpToHttpsRedirectDisabled(t *testing.T) { @@ -576,7 +575,6 @@ func TestArtifactProxyRequest(t *testing.T) { t.Log("Artifact server URL", artifactServerURL) for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { teardown := RunPagesProcessWithSSLCertFile( t, @@ -674,7 +672,6 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { t.Log("Artifact server URL", artifactServerURL) for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { configFile, cleanup := defaultConfigFileWith(t, "artifacts-server="+artifactServerURL, @@ -731,6 +728,7 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { // Request auth callback in project domain authrsp, err = GetRedirectPageWithCookie(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery, cookie) + require.NoError(t, err) // server returns the ticket, user will be redirected to the project page require.Equal(t, http.StatusFound, authrsp.StatusCode) @@ -847,6 +845,7 @@ func TestWhenAuthIsEnabledPrivateWillRedirectToAuthorize(t *testing.T) { url, err := url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) rsp, err = GetRedirectPage(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery) + require.NoError(t, err) require.Equal(t, http.StatusFound, rsp.StatusCode) require.Equal(t, 1, len(rsp.Header["Location"])) @@ -1074,7 +1073,6 @@ func sleepIfAuthorized(t *testing.T, authorization string, w http.ResponseWriter } else { w.WriteHeader(http.StatusNotFound) } - } func TestAccessControlUnderCustomDomain(t *testing.T) { @@ -1139,6 +1137,7 @@ func TestAccessControlUnderCustomDomain(t *testing.T) { // Fetch page in custom domain authrsp, err = GetRedirectPageWithCookie(t, httpListener, "private.domain.com", "/", cookie) + require.NoError(t, err) require.Equal(t, http.StatusOK, authrsp.StatusCode) } @@ -1204,6 +1203,7 @@ func TestAccessControlUnderCustomDomainWithHTTPSProxy(t *testing.T) { // Fetch page in custom domain authrsp, err = GetProxyRedirectPageWithCookie(t, proxyListener, "private.domain.com", "/", cookie, true) + require.NoError(t, err) require.Equal(t, http.StatusOK, authrsp.StatusCode) } @@ -1337,7 +1337,6 @@ func TestAccessControl(t *testing.T) { } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { teardown := RunPagesProcessWithAuthServerWithSSL(t, *pagesBinary, listeners, "", certFile, testServer.URL) defer teardown() @@ -1380,6 +1379,7 @@ func TestAccessControl(t *testing.T) { // Request auth callback in project domain authrsp, err = GetRedirectPageWithCookie(t, httpsListener, url.Host, url.Path+"?"+url.RawQuery, cookie) + require.NoError(t, err) // server returns the ticket, user will be redirected to the project page require.Equal(t, http.StatusFound, authrsp.StatusCode) diff --git a/app.go b/app.go index 8cfeef00222cd0bf8dc0c592186a578a06531500..217f9db70aecdb31847dea458b44e94008ecbf0f 100644 --- a/app.go +++ b/app.go @@ -2,7 +2,6 @@ package main import ( "crypto/tls" - "errors" "fmt" "net" "net/http" @@ -15,7 +14,7 @@ import ( "gitlab.com/gitlab-org/labkit/errortracking" labmetrics "gitlab.com/gitlab-org/labkit/metrics" "gitlab.com/gitlab-org/labkit/monitoring" - mimedb "gitlab.com/lupine/go-mimedb" + "gitlab.com/lupine/go-mimedb" "gitlab.com/gitlab-org/gitlab-pages/internal/acme" "gitlab.com/gitlab-org/gitlab-pages/internal/artifact" @@ -32,20 +31,13 @@ import ( ) const ( - xForwardedProto = "X-Forwarded-Proto" - xForwardedHost = "X-Forwarded-Host" - xForwardedProtoHTTPS = "https" + xForwardedHost = "X-Forwarded-Host" ) var ( corsHandler = cors.New(cors.Options{AllowedMethods: []string{"GET"}}) ) -var ( - errStartListener = errors.New("Could not start listener") - errX509KeyPair = errors.New("Could not initialize KeyPair") -) - type theApp struct { appConfig domains *source.Domains @@ -103,10 +95,8 @@ func (a *theApp) domain(host string) (*domain.Domain, error) { func (a *theApp) checkAuthenticationIfNotExists(domain *domain.Domain, w http.ResponseWriter, r *http.Request) bool { if domain == nil || !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) { return true @@ -275,7 +265,6 @@ func (a *theApp) serveFileOrNotFoundHandler() http.Handler { // because the projects override the paths of the namespace project and they might be private even though // namespace project is public. if domain.IsNamespaceProject(r) { - if a.Auth.CheckAuthenticationWithoutProject(w, r) { return } @@ -292,7 +281,6 @@ func (a *theApp) serveFileOrNotFoundHandler() http.Handler { // httpInitialMiddleware sets up HTTP requests func (a *theApp) httpInitialMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - handler.ServeHTTP(w, setRequestScheme(r)) }) } diff --git a/internal/artifact/artifact_test.go b/internal/artifact/artifact_test.go index a55eda7ba91cd3aa11b6af4968edd531443e8b3b..ab25d16f0897abafca7e76f9ea6afa9aad0d4a5e 100644 --- a/internal/artifact/artifact_test.go +++ b/internal/artifact/artifact_test.go @@ -84,7 +84,6 @@ func TestTryMakeRequest(t *testing.T) { require.Equal(t, c.Length, result.Header().Get("Content-Length")) require.Equal(t, c.CacheControl, result.Header().Get("Cache-Control")) require.Equal(t, c.Content, result.Body.String()) - }) } } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index a89dd5999611a35534f1b21b89ba0ae657db56d3..6dce1ab86a076c5f581c6f443f77af1e50c2873f 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -47,7 +47,6 @@ var ( errFailAuth = errors.New("Failed to authenticate request") errAuthNotConfigured = errors.New("Authentication is not configured") errQueryParameter = errors.New("Failed to parse domain query parameter") - errAuthInvalidToken = errors.New("Invalid token supplied") ) // Auth handles authenticating users with GitLab API @@ -88,7 +87,6 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*sessions.Session, error) { } func (a *Auth) checkSession(w http.ResponseWriter, r *http.Request) (*sessions.Session, error) { - // Create or get session session, errsession := a.getSessionFromStore(r) @@ -149,7 +147,6 @@ func (a *Auth) TryAuthenticate(w http.ResponseWriter, r *http.Request, domains s } func (a *Auth) checkAuthenticationResponse(session *sessions.Session, w http.ResponseWriter, r *http.Request) { - if !validateState(r, session) { // State is NOT ok logRequest(r).Warn("Authentication state did not match expected") @@ -484,7 +481,6 @@ func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, proje // CheckAuthenticationWithoutProject checks if user is authenticated and has a valid token func (a *Auth) CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request) bool { - if a == nil { // No auth supported return false diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 4a5d63fa9680d80a7e2cdc5004af20ded50b7943..fc8ddb449b8579de2298992c5be1631f1026b354 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -362,6 +362,7 @@ func TestGetTokenIfExistsWhenTokenExists(t *testing.T) { session.Save(r, result) token, err := auth.GetTokenIfExists(result, r) + require.NoError(t, err) require.Equal(t, "abc", token) } diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index e22ff7a6b097f3dc4e8c04112ba380509a0cc23c..49d46cb3e2e18e469166f9b7570c24d71fa99cde 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -1,11 +1,7 @@ package domain import ( - "compress/gzip" - "io/ioutil" "net/http" - "net/http/httptest" - "net/url" "os" "testing" @@ -88,33 +84,6 @@ func TestIsHTTPSOnly(t *testing.T) { } } -func testHTTPGzip(t *testing.T, handler http.HandlerFunc, mode, url string, values url.Values, acceptEncoding string, str interface{}, contentType string, ungzip bool) { - w := httptest.NewRecorder() - req, err := http.NewRequest(mode, url+"?"+values.Encode(), nil) - require.NoError(t, err) - if acceptEncoding != "" { - req.Header.Add("Accept-Encoding", acceptEncoding) - } - handler(w, req) - - if ungzip { - reader, err := gzip.NewReader(w.Body) - require.NoError(t, err) - defer reader.Close() - - contentEncoding := w.Header().Get("Content-Encoding") - require.Equal(t, "gzip", contentEncoding, "Content-Encoding") - - bytes, err := ioutil.ReadAll(reader) - require.NoError(t, err) - require.Contains(t, string(bytes), str) - } else { - require.Contains(t, w.Body.String(), str) - } - - require.Equal(t, contentType, w.Header().Get("Content-Type")) -} - func TestPredefined404ServeHTTP(t *testing.T) { cleanup := setUpTests(t) defer cleanup() diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 5791e793267c19f3d4694ce43e5c2ff062f9d383..fb47fc55beccb5523abe9fa9b38ea3e3caff7527 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -24,9 +24,7 @@ func New(auth internal.Auth, artifact internal.Artifact) *Handlers { func (a *Handlers) checkIfLoginRequiredOrInvalidToken(w http.ResponseWriter, r *http.Request, token string) func(*http.Response) bool { return func(resp *http.Response) bool { - if resp.StatusCode == http.StatusNotFound { - if token == "" { if !a.Auth.IsAuthSupported() { // Auth is not supported, probably means no access or does not exist but we cannot try with auth @@ -63,5 +61,8 @@ func (a *Handlers) HandleArtifactRequest(host string, w http.ResponseWriter, r * return true } + // nolint: bodyclose + // a.checkIfLoginRequiredOrInvalidToken returns a response.Body, closing this body is responsibility + // of the TryMakeRequest implementation return a.Artifact.TryMakeRequest(host, w, r, token, a.checkIfLoginRequiredOrInvalidToken(w, r, token)) } diff --git a/internal/httptransport/transport_test.go b/internal/httptransport/transport_test.go index e3270ebe9ba479d5c52b59130b12df080b58f322..330d102248e9dab5f79cf469bb37a3ad6c8abd68 100644 --- a/internal/httptransport/transport_test.go +++ b/internal/httptransport/transport_test.go @@ -14,7 +14,6 @@ import ( ) func Test_withRoundTripper(t *testing.T) { - tests := []struct { name string statusCode int diff --git a/internal/rollout/rollout.go b/internal/rollout/rollout.go index ec592c3874e8137b916136267625aca389732e30..aaf2528916e8ad18a6d80a7ee23b2c21b9a94565 100644 --- a/internal/rollout/rollout.go +++ b/internal/rollout/rollout.go @@ -7,7 +7,7 @@ import ( ) // Rollout returns true and no error when during this run something should -// happen for given actor according to the stickiness and likelyhood passed +// happen for given actor according to the stickiness and likelihood passed // as a percentage value to this function. It returns false rollout and an // error if the percentage value is negative or higher than 100. func Rollout(actor string, percentage int, stickiness string) (bool, error) { diff --git a/internal/source/domains.go b/internal/source/domains.go index 7a376bc76c2ddc2dd8471d34d776105270c5f02b..77e1aa1d41704f57ca44c6c30ed33fcd2a8b28a4 100644 --- a/internal/source/domains.go +++ b/internal/source/domains.go @@ -18,7 +18,7 @@ var ( gitlabSourceConfig gitlabsourceconfig.GitlabSourceConfig // serverlessDomainRegex is a regular expression we use to check if a domain - // is a serverless domain, to short circut gitlab source rollout. It can be + // is a serverless domain, to short circuit gitlab source rollout. It can be // removed after the rollout is done serverlessDomainRegex = regexp.MustCompile(`^[^.]+-[[:xdigit:]]{2}a1[[:xdigit:]]{10}f2[[:xdigit:]]{2}[[:xdigit:]]+-?.*`) ) diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go index 9fffe4a91b8f50d510bfeee3c9207a59ac096bd2..24008b08e28ac9f29e04674ce36388d8c2a3aa54 100644 --- a/internal/source/domains_test.go +++ b/internal/source/domains_test.go @@ -163,7 +163,6 @@ func TestGetDomainWithIncrementalrolloutOfGitLabSource(t *testing.T) { type testDomain struct { name string source string - times int } tests := map[string]struct { diff --git a/internal/source/gitlab/cache/cache.go b/internal/source/gitlab/cache/cache.go index 00fca92bda1095baa0f2f6c967f231fef23070d1..c8d166b5a8fd1239f8b8d5233cd44d3c5255ed36 100644 --- a/internal/source/gitlab/cache/cache.go +++ b/internal/source/gitlab/cache/cache.go @@ -18,9 +18,8 @@ var defaultCacheConfig = cacheConfig{ // Cache is a short and long caching mechanism for GitLab source type Cache struct { - client api.Client - store Store - cacheConfig *cacheConfig + client api.Client + store Store } type cacheConfig struct { diff --git a/internal/source/gitlab/cache/entry.go b/internal/source/gitlab/cache/entry.go index 191ef789d8e91185139dd679313ce9d197d7455f..d33d2758c6422c0f3c4f71e5e905e6638adf6e00 100644 --- a/internal/source/gitlab/cache/entry.go +++ b/internal/source/gitlab/cache/entry.go @@ -25,7 +25,6 @@ type Entry struct { } func newCacheEntry(domain string, refreshTimeout time.Duration, retriever *Retriever) *Entry { - return &Entry{ domain: domain, created: time.Now(), diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index d703769bcc6f784331c0b104e404f6a7f42c0d80..422a3d9a6b659f62da881defa80290997189efe9 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -30,7 +30,6 @@ func AssertHTTP404(t *testing.T, handler http.HandlerFunc, mode, url string, val // AssertRedirectTo asserts that handler redirects to particular URL func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string, url string, values url.Values, expectedURL string) { - require.HTTPRedirect(t, handler, method, url, values) recorder := httptest.NewRecorder() @@ -41,7 +40,6 @@ func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string, handler(recorder, req) require.Equal(t, expectedURL, recorder.Header().Get("Location")) - } // AssertLogContains checks that wantLogEntry is contained in at least one of the log entries