Update golangci-lint version, linting fixes. (#511)

Author: Michael Wilson

.github/workflows/lint.yml

@@ -15,4 +15,4 @@ jobs:
       - name: golangci-lint
         uses: golangci/golangci-lint-action@v2
         with:
-          version: v1.46.2
+          version: v1.52.2

.golangci.yml

@@ -7,41 +7,35 @@
 
 linters:
   enable:
-    - gofmt # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true]
-    - goimports # Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true]
-    - gosec # Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: true, auto-fix: false]
-    - misspell # Finds commonly misspelled English words in comments [fast: true, auto-fix: true]
-    - deadcode # Finds unused code [fast: true, auto-fix: false]
-    - revive # Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: true, auto-fix: false]
-    - unconvert # Remove unnecessary type conversions [fast: true, auto-fix: false]
-
-  disable:
-    # TODO(ross): fix errors reported by these checkers and enable them
     - bodyclose # checks whether HTTP response body is closed successfully [fast: false, auto-fix: false]
     - depguard # Go linter that checks if package imports are in a list of acceptable packages [fast: true, auto-fix: false]
-    - dupl # Tool for code clone detection [fast: true, auto-fix: false]
     - errcheck # Inspects source code for security problems [fast: true, auto-fix: false]
-    - gochecknoglobals # Checks that no globals are present in Go code [fast: true, auto-fix: false]
-    - gochecknoinits # Checks that no init functions are present in Go code [fast: true, auto-fix: false]
-    - goconst # Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false]
     - gocritic # The most opinionated Go source code linter [fast: true, auto-fix: false]
     - gocyclo # Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false]
+    - gofmt # Gofmt checks whether code was gofmt-ed. By default this tool runs with -s option to check for code simplification [fast: true, auto-fix: true]
+    - goimports # Goimports does everything that gofmt does. Additionally it checks unused imports [fast: true, auto-fix: true]
+    - gosec # Errcheck is a program for checking for unchecked errors in go programs. These unchecked errors can be critical bugs in some cases [fast: true, auto-fix: false]
     - gosimple # Linter for Go source code that specializes in simplifying a code [fast: false, auto-fix: false]
     - govet # Vet examines Go source code and reports suspicious constructs, such as Printf calls whose arguments do not align with the format string [fast: false, auto-fix: false]
     - ineffassign # Detects when assignments to existing variables are not used [fast: true, auto-fix: false]
-    - interfacer # Linter that suggests narrower interface types [fast: false, auto-fix: false]
-    - lll # Reports long lines [fast: true, auto-fix: false]
-    - maligned # Tool to detect Go structs that would take less memory if their fields were sorted [fast: true, auto-fix: false]
+    - misspell # Finds commonly misspelled English words in comments [fast: true, auto-fix: true]
     - nakedret # Finds naked returns in functions greater than a specified function length [fast: true, auto-fix: false]
     - prealloc # Finds slice declarations that could potentially be preallocated [fast: true, auto-fix: false]
-    - scopelint # Scopelint checks for unpinned variables in go programs [fast: true, auto-fix: false]
+    - revive # Golint differs from gofmt. Gofmt reformats Go source code, whereas golint prints out style mistakes [fast: true, auto-fix: false]
     - staticcheck # Staticcheck is a go vet on steroids, applying a ton of static analysis checks [fast: false, auto-fix: false]
-    - structcheck # Finds unused struct fields [fast: true, auto-fix: false]
     - stylecheck # Stylecheck is a replacement for golint [fast: false, auto-fix: false]
     - typecheck # Like the front-end of a Go compiler, parses and type-checks Go code [fast: true, auto-fix: false]
+    - unconvert # Remove unnecessary type conversions [fast: true, auto-fix: false]
     - unparam # Reports unused function parameters [fast: false, auto-fix: false]
     - unused # Checks Go code for unused constants, variables, functions and types [fast: false, auto-fix: false]
-    - varcheck # Finds unused global variables and constants [fast: true, auto-fix: false]
+
+  disable:
+    # TODO(ross): fix errors reported by these checkers and enable them
+    - dupl # Tool for code clone detection [fast: true, auto-fix: false]
+    - gochecknoglobals # Checks that no globals are present in Go code [fast: true, auto-fix: false]
+    - gochecknoinits # Checks that no init functions are present in Go code [fast: true, auto-fix: false]
+    - goconst # Finds repeated strings that could be replaced by a constant [fast: true, auto-fix: false]
+    - lll # Reports long lines [fast: true, auto-fix: false]
 linters-settings:
   goimports:
     local-prefixes: github.com/crewjam/saml

example/idp/idp.go

@@ -1,3 +1,4 @@
+// Package main contains an example identity provider implementation.
 package main
 
 import (

example/service.go

@@ -32,7 +32,7 @@ type Link struct {
 }
 
 // CreateLink handles requests to create links
-func CreateLink(c web.C, w http.ResponseWriter, r *http.Request) {
+func CreateLink(_ web.C, w http.ResponseWriter, r *http.Request) {
 	account := r.Header.Get("X-Remote-User")
 	l := Link{
 		ShortLink: uniuri.New(),
@@ -42,22 +42,20 @@ func CreateLink(c web.C, w http.ResponseWriter, r *http.Request) {
 	links[l.ShortLink] = l
 
 	fmt.Fprintf(w, "%s\n", l.ShortLink)
-	return
 }
 
 // ServeLink handles requests to redirect to a link
-func ServeLink(c web.C, w http.ResponseWriter, r *http.Request) {
+func ServeLink(_ web.C, w http.ResponseWriter, r *http.Request) {
 	l, ok := links[strings.TrimPrefix(r.URL.Path, "/")]
 	if !ok {
 		http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
 		return
 	}
 	http.Redirect(w, r, l.Target, http.StatusFound)
-	return
 }
 
 // ListLinks returns a list of the current user's links
-func ListLinks(c web.C, w http.ResponseWriter, r *http.Request) {
+func ListLinks(_ web.C, w http.ResponseWriter, r *http.Request) {
 	account := r.Header.Get("X-Remote-User")
 	for _, l := range links {
 		if l.Owner == account {
@@ -145,14 +143,24 @@ func main() {
 
 	spURL := *idpMetadataURL
 	spURL.Path = "/services/sp"
-	http.Post(spURL.String(), "text/xml", bytes.NewReader(spMetadataBuf))
+	resp, err := http.Post(spURL.String(), "text/xml", bytes.NewReader(spMetadataBuf))
+
+	if err != nil {
+		panic(err)
+	}
+
+	if err := resp.Body.Close(); err != nil {
+		panic(err)
+	}
 
 	goji.Handle("/saml/*", samlSP)
 
 	authMux := web.New()
 	authMux.Use(samlSP.RequireAccount)
 	authMux.Get("/whoami", func(w http.ResponseWriter, r *http.Request) {
-		pretty.Fprintf(w, "%# v", r)
+		if _, err := pretty.Fprintf(w, "%# v", r); err != nil {
+			panic(err)
+		}
 	})
 	authMux.Post("/", CreateLink)
 	authMux.Get("/", ListLinks)

example/trivial/trivial.go

@@ -1,3 +1,4 @@
+// Package main contains an example service provider implementation.
 package main
 
 import (
@@ -9,6 +10,7 @@ import (
 	"log"
 	"net/http"
 	"net/url"
+	"time"
 
 	"github.com/crewjam/saml/samlsp"
 )
@@ -69,8 +71,14 @@ func main() {
 	})
 	app := http.HandlerFunc(hello)
 	slo := http.HandlerFunc(logout)
+
 	http.Handle("/hello", samlMiddleware.RequireAccount(app))
 	http.Handle("/saml/", samlMiddleware)
 	http.Handle("/logout", slo)
-	log.Fatal(http.ListenAndServe(":8000", nil))
+
+	server := &http.Server{
+		Addr:              ":8080",
+		ReadHeaderTimeout: 5 * time.Second,
+	}
+	log.Fatal(server.ListenAndServe())
 }

identity_provider.go

@@ -196,10 +196,13 @@ func (idp *IdentityProvider) Handler() http.Handler {
 }
 
 // ServeMetadata is an http.HandlerFunc that serves the IDP metadata
-func (idp *IdentityProvider) ServeMetadata(w http.ResponseWriter, r *http.Request) {
+func (idp *IdentityProvider) ServeMetadata(w http.ResponseWriter, _ *http.Request) {
 	buf, _ := xml.MarshalIndent(idp.Metadata(), "", "  ")
 	w.Header().Set("Content-Type", "application/samlmetadata+xml")
-	w.Write(buf)
+	if _, err := w.Write(buf); err != nil {
+		idp.Logger.Printf("ERROR: %s", err)
+		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+	}
 }
 
 // ServeSSO handles SAML auth requests.
@@ -716,9 +719,7 @@ func (DefaultAssertionMaker) MakeAssertion(req *IdpAuthnRequest, session *Sessio
 		})
 	}
 
-	for _, ca := range session.CustomAttributes {
-		attributes = append(attributes, ca)
-	}
+	attributes = append(attributes, session.CustomAttributes...)
 
 	if len(session.Groups) != 0 {
 		groupMemberAttributeValues := []AttributeValue{}

identity_provider_go117_test.go

@@ -43,8 +43,10 @@ func TestIDPHTTPCanHandleSSORequest(t *testing.T) {
 		d := bytes.Replace(c, []byte("<AuthnRequest"), []byte("<AuthnRequest ::foo=\"bar\">]]"), 1)
 		f := bytes.Buffer{}
 		e, _ := flate.NewWriter(&f, flate.DefaultCompression)
-		e.Write(d)
-		e.Close()
+		_, err := e.Write(d)
+		assert.Check(t, err)
+		err = e.Close()
+		assert.Check(t, err)
 		g := base64.StdEncoding.EncodeToString(f.Bytes())
 		invalidRequest := url.QueryEscape(g)
 

identity_provider_test.go

@@ -207,8 +207,8 @@ func TestIDPHTTPCanHandleMetadataRequest(t *testing.T) {
 	test.IDP.Handler().ServeHTTP(w, r)
 	assert.Check(t, is.Equal(http.StatusOK, w.Code))
 	assert.Check(t, is.Equal("application/samlmetadata+xml", w.Header().Get("Content-type")))
-	assert.Check(t, strings.HasPrefix(string(w.Body.Bytes()), "<EntityDescriptor"),
-		string(w.Body.Bytes()))
+	assert.Check(t, strings.HasPrefix(w.Body.String(), "<EntityDescriptor"),
+		w.Body.String())
 }
 
 func TestIDPCanHandleRequestWithNewSession(t *testing.T) {
@@ -760,7 +760,7 @@ func TestIDPIDPInitiatedNewSession(t *testing.T) {
 	r, _ := http.NewRequest("GET", "https://idp.example.com/services/sp/whoami", nil)
 	test.IDP.ServeIDPInitiated(w, r, test.SP.MetadataURL.String(), "ThisIsTheRelayState")
 	assert.Check(t, is.Equal(200, w.Code))
-	assert.Check(t, is.Equal("RelayState: ThisIsTheRelayState", string(w.Body.Bytes())))
+	assert.Check(t, is.Equal("RelayState: ThisIsTheRelayState", w.Body.String()))
 }
 
 func TestIDPIDPInitiatedExistingSession(t *testing.T) {
@@ -1026,18 +1026,18 @@ func TestIDPRejectDecompressionBomb(t *testing.T) {
 		},
 	}
 
-	//w := httptest.NewRecorder()
-
 	data := bytes.Repeat([]byte("a"), 768*1024*1024)
 	var compressed bytes.Buffer
 	w, _ := flate.NewWriter(&compressed, flate.BestCompression)
-	w.Write(data)
-	w.Close()
+	_, err := w.Write(data)
+	assert.Check(t, err)
+	err = w.Close()
+	assert.Check(t, err)
 	encoded := base64.StdEncoding.EncodeToString(compressed.Bytes())
 
 	r, _ := http.NewRequest("GET", "/dontcare?"+url.Values{
 		"SAMLRequest": {encoded},
 	}.Encode(), nil)
-	_, err := NewIdpAuthnRequest(&test.IDP, r)
+	_, err = NewIdpAuthnRequest(&test.IDP, r)
 	assert.Error(t, err, "cannot decompress request: flate: uncompress limit exceeded (10485760 bytes)")
 }

logger/logger.go

@@ -1,3 +1,4 @@
+// Package logger provides a logging interface.
 package logger
 
 import (

metadata.go

@@ -65,7 +65,7 @@ type EntityDescriptor struct {
 }
 
 // MarshalXML implements xml.Marshaler
-func (m EntityDescriptor) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
+func (m EntityDescriptor) MarshalXML(e *xml.Encoder, _ xml.StartElement) error {
 	type Alias EntityDescriptor
 	aux := &struct {
 		ValidUntil    RelaxedTime `xml:"validUntil,attr,omitempty"`

samlidp/samlidp.go

@@ -47,9 +47,9 @@ type Server struct {
 // New returns a new Server
 func New(opts Options) (*Server, error) {
 	metadataURL := opts.URL
-	metadataURL.Path = metadataURL.Path + "/metadata"
+	metadataURL.Path += "/metadata"
 	ssoURL := opts.URL
-	ssoURL.Path = ssoURL.Path + "/sso"
+	ssoURL.Path += "/sso"
 	logr := opts.Logger
 	if logr == nil {
 		logr = logger.DefaultLogger

samlidp/samlidp_test.go

@@ -124,8 +124,8 @@ func TestHTTPCanHandleMetadataRequest(t *testing.T) {
 	test.Server.ServeHTTP(w, r)
 	assert.Check(t, is.Equal(http.StatusOK, w.Code))
 	assert.Check(t,
-		strings.HasPrefix(string(w.Body.Bytes()), "<EntityDescriptor"),
-		string(w.Body.Bytes()))
+		strings.HasPrefix(w.Body.String(), "<EntityDescriptor"),
+		w.Body.String())
 	golden.Assert(t, w.Body.String(), "http_metadata_response.html")
 }
 
@@ -139,7 +139,7 @@ func TestHTTPCanSSORequest(t *testing.T) {
 	test.Server.ServeHTTP(w, r)
 	assert.Check(t, is.Equal(http.StatusOK, w.Code))
 	assert.Check(t,
-		strings.HasPrefix(string(w.Body.Bytes()), "<html><p></p><form method=\"post\" action=\"https://idp.example.com/sso\">"),
-		string(w.Body.Bytes()))
+		strings.HasPrefix(w.Body.String(), "<html><p></p><form method=\"post\" action=\"https://idp.example.com/sso\">"),
+		w.Body.String())
 	golden.Assert(t, w.Body.String(), "http_sso_response.html")
 }

samlidp/service.go

@@ -25,7 +25,7 @@ type Service struct {
 // service provider ID, which is typically the service provider's
 // metadata URL. If an appropriate service provider cannot be found then
 // the returned error must be os.ErrNotExist.
-func (s *Server) GetServiceProvider(r *http.Request, serviceProviderID string) (*saml.EntityDescriptor, error) {
+func (s *Server) GetServiceProvider(_ *http.Request, serviceProviderID string) (*saml.EntityDescriptor, error) {
 	s.idpConfigMu.RLock()
 	defer s.idpConfigMu.RUnlock()
 	rv, ok := s.serviceProviders[serviceProviderID]
@@ -37,30 +37,38 @@ func (s *Server) GetServiceProvider(r *http.Request, serviceProviderID string) (
 
 // HandleListServices handles the `GET /services/` request and responds with a JSON formatted list
 // of service names.
-func (s *Server) HandleListServices(c web.C, w http.ResponseWriter, r *http.Request) {
+func (s *Server) HandleListServices(_ web.C, w http.ResponseWriter, _ *http.Request) {
 	services, err := s.Store.List("/services/")
 	if err != nil {
 		s.logger.Printf("ERROR: %s", err)
 		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
 		return
 	}
 
-	json.NewEncoder(w).Encode(struct {
+	err = json.NewEncoder(w).Encode(struct {
 		Services []string `json:"services"`
 	}{Services: services})
+	if err != nil {
+		s.logger.Printf("ERROR: %s", err)
+		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+	}
 }
 
 // HandleGetService handles the `GET /services/:id` request and responds with the service
 // metadata in XML format.
-func (s *Server) HandleGetService(c web.C, w http.ResponseWriter, r *http.Request) {
+func (s *Server) HandleGetService(c web.C, w http.ResponseWriter, _ *http.Request) {
 	service := Service{}
 	err := s.Store.Get(fmt.Sprintf("/services/%s", c.URLParams["id"]), &service)
 	if err != nil {
 		s.logger.Printf("ERROR: %s", err)
 		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
 		return
 	}
-	xml.NewEncoder(w).Encode(service.Metadata)
+	err = xml.NewEncoder(w).Encode(service.Metadata)
+	if err != nil {
+		s.logger.Printf("ERROR: %s", err)
+		http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+	}
 }
 
 // HandlePutService handles the `PUT /shortcuts/:id` request. It accepts the XML-formatted
@@ -92,7 +100,7 @@ func (s *Server) HandlePutService(c web.C, w http.ResponseWriter, r *http.Reques
 }
 
 // HandleDeleteService handles the `DELETE /services/:id` request.
-func (s *Server) HandleDeleteService(c web.C, w http.ResponseWriter, r *http.Request) {
+func (s *Server) HandleDeleteService(c web.C, w http.ResponseWriter, _ *http.Request) {
 	service := Service{}
 	err := s.Store.Get(fmt.Sprintf("/services/%s", c.URLParams["id"]), &service)
 	if err != nil {

samlidp/service_test.go

@@ -18,7 +18,7 @@ func TestServicesCrud(t *testing.T) {
 	r, _ := http.NewRequest("GET", "https://idp.example.com/services/", nil)
 	test.Server.ServeHTTP(w, r)
 	assert.Check(t, is.Equal(http.StatusOK, w.Code))
-	assert.Check(t, is.Equal("{\"services\":[]}\n", string(w.Body.Bytes())))
+	assert.Check(t, is.Equal("{\"services\":[]}\n", w.Body.String()))
 
 	w = httptest.NewRecorder()
 	r, _ = http.NewRequest("PUT", "https://idp.example.com/services/sp",
@@ -36,7 +36,7 @@ func TestServicesCrud(t *testing.T) {
 	r, _ = http.NewRequest("GET", "https://idp.example.com/services/", nil)
 	test.Server.ServeHTTP(w, r)
 	assert.Check(t, is.Equal(http.StatusOK, w.Code))
-	assert.Check(t, is.Equal("{\"services\":[\"sp\"]}\n", string(w.Body.Bytes())))
+	assert.Check(t, is.Equal("{\"services\":[\"sp\"]}\n", w.Body.String()))
 
 	assert.Check(t, is.Len(test.Server.serviceProviders, 2))
 
@@ -49,6 +49,6 @@ func TestServicesCrud(t *testing.T) {
 	r, _ = http.NewRequest("GET", "https://idp.example.com/services/", nil)
 	test.Server.ServeHTTP(w, r)
 	assert.Check(t, is.Equal(http.StatusOK, w.Code))
-	assert.Check(t, is.Equal("{\"services\":[]}\n", string(w.Body.Bytes())))
+	assert.Check(t, is.Equal("{\"services\":[]}\n", w.Body.String()))
 	assert.Check(t, is.Len(test.Server.serviceProviders, 1))
 }