Use http.NoBody as per linter (#231)

Reviewed-on: https://codeberg.org/Codeberg/pages-server/pulls/231
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

.woodpecker.yml

@@ -1,4 +1,4 @@
-pipeline:
+steps:
   # use vendor to cache dependencies
   vendor:
     image: golang:1.20

cmd/certs.go

@@ -98,9 +98,6 @@ func listCerts(ctx *cli.Context) error {
 
 	fmt.Printf("Domain\tValidTill\n\n")
 	for _, cert := range items {
-		if cert.Domain[0] == '.' {
-			cert.Domain = "*" + cert.Domain
-		}
 		fmt.Printf("%s\t%s\n",
 			cert.Domain,
 			time.Unix(cert.ValidTill, 0).Format(time.RFC3339))

cmd/flags.go

@@ -43,6 +43,18 @@ var (
 			EnvVars: []string{"GITEA_API_TOKEN"},
 			Value:   "",
 		},
+		&cli.BoolFlag{
+			Name:    "enable-lfs-support",
+			Usage:   "enable lfs support, require gitea >= v1.17.0 as backend",
+			EnvVars: []string{"ENABLE_LFS_SUPPORT"},
+			Value:   true,
+		},
+		&cli.BoolFlag{
+			Name:    "enable-symlink-support",
+			Usage:   "follow symlinks if enabled, require gitea >= v1.18.0 as backend",
+			EnvVars: []string{"ENABLE_SYMLINK_SUPPORT"},
+			Value:   true,
+		},
 
 		// ###########################
 		// ### Page Server Domains ###
@@ -73,7 +85,9 @@ var (
 			Value:   "https://docs.codeberg.org/codeberg-pages/raw-content/",
 		},
 
-		// Server
+		// #########################
+		// ### Page Server Setup ###
+		// #########################
 		&cli.StringFlag{
 			Name:    "host",
 			Usage:   "specifies host of listening address",
@@ -91,19 +105,6 @@ var (
 			// TODO: desc
 			EnvVars: []string{"ENABLE_HTTP_SERVER"},
 		},
-		// Server Options
-		&cli.BoolFlag{
-			Name:    "enable-lfs-support",
-			Usage:   "enable lfs support, require gitea v1.17.0 as backend",
-			EnvVars: []string{"ENABLE_LFS_SUPPORT"},
-			Value:   true,
-		},
-		&cli.BoolFlag{
-			Name:    "enable-symlink-support",
-			Usage:   "follow symlinks if enabled, require gitea v1.18.0 as backend",
-			EnvVars: []string{"ENABLE_SYMLINK_SUPPORT"},
-			Value:   true,
-		},
 		&cli.StringFlag{
 			Name:    "log-level",
 			Value:   "warn",
@@ -111,7 +112,9 @@ var (
 			EnvVars: []string{"LOG_LEVEL"},
 		},
 
-		// ACME
+		// ############################
+		// ### ACME Client Settings ###
+		// ############################
 		&cli.StringFlag{
 			Name:    "acme-api-endpoint",
 			EnvVars: []string{"ACME_API"},

cmd/main.go

@@ -44,7 +44,7 @@ func Serve(ctx *cli.Context) error {
 	}
 	log.Logger = zerolog.New(zerolog.ConsoleWriter{Out: os.Stderr}).With().Timestamp().Logger().Level(logLevel)
 
-	giteaRoot := strings.TrimSuffix(ctx.String("gitea-root"), "/")
+	giteaRoot := ctx.String("gitea-root")
 	giteaAPIToken := ctx.String("gitea-api-token")
 	rawDomain := ctx.String("raw-domain")
 	mainDomainSuffix := ctx.String("pages-domain")
@@ -68,7 +68,7 @@ func Serve(ctx *cli.Context) error {
 		allowedCorsDomains = append(allowedCorsDomains, rawDomain)
 	}
 
-	// Make sure MainDomain has a trailing dot, and GiteaRoot has no trailing slash
+	// Make sure MainDomain has a trailing dot
 	if !strings.HasPrefix(mainDomainSuffix, ".") {
 		mainDomainSuffix = "." + mainDomainSuffix
 	}

integration/get_test.go

@@ -20,7 +20,9 @@ func TestGetRedirect(t *testing.T) {
 	log.Println("=== TestGetRedirect ===")
 	// test custom domain redirect
 	resp, err := getTestHTTPSClient().Get("https://calciumdibromid.localhost.mock.directory:4430")
-	assert.NoError(t, err)
+	if !assert.NoError(t, err) {
+		t.FailNow()
+	}
 	if !assert.EqualValues(t, http.StatusTemporaryRedirect, resp.StatusCode) {
 		t.FailNow()
 	}

main.go

@@ -8,15 +8,13 @@ import (
 	"github.com/urfave/cli/v2"
 
 	"codeberg.org/codeberg/pages/cmd"
+	"codeberg.org/codeberg/pages/server/version"
 )
 
-// can be changed with -X on compile
-var version = "dev"
-
 func main() {
 	app := cli.NewApp()
 	app.Name = "pages-server"
-	app.Version = version
+	app.Version = version.Version
 	app.Usage = "pages server"
 	app.Action = cmd.Serve
 	app.Flags = cmd.ServerFlags

server/context/context.go

@@ -48,11 +48,9 @@ func (c *Context) Redirect(uri string, statusCode int) {
 	http.Redirect(c.RespWriter, c.Req, uri, statusCode)
 }
 
-// Path returns requested path.
+// Path returns the cleaned requested path.
-//
-// The returned bytes are valid until your request handler returns.
 func (c *Context) Path() string {
-	return c.Req.URL.Path
+	return utils.CleanPath(c.Req.URL.Path)
 }
 
 func (c *Context) Host() string {

server/database/interface.go

@@ -54,10 +54,14 @@ func toCert(name string, c *certificate.Resource) (*Cert, error) {
 	}
 	validTill := tlsCertificates[0].NotAfter.Unix()
 
-	// TODO: do we need this or can we just go with domain name for wildcard cert
+	// handle wildcard certs
-	// default *.mock cert is prefixed with '.'
+	if name[:1] == "." {
-	if name != c.Domain && name[1:] != c.Domain && name[0] != '.' {
+		name = "*" + name
-		return nil, fmt.Errorf("domain key and cert domain not equal")
+	}
+	if name != c.Domain {
+		err := fmt.Errorf("domain key '%s' and cert domain '%s' not equal", name, c.Domain)
+		log.Error().Err(err).Msg("toCert conversion did discover mismatch")
+		// TODO: fail hard: return nil, err
 	}
 
 	return &Cert{

server/database/xorm.go

@@ -3,7 +3,6 @@ package database
 import (
 	"errors"
 	"fmt"
-	"strings"
 
 	"github.com/rs/zerolog/log"
 
@@ -52,18 +51,38 @@ func (x xDB) Close() error {
 
 func (x xDB) Put(domain string, cert *certificate.Resource) error {
 	log.Trace().Str("domain", cert.Domain).Msg("inserting cert to db")
+
+	domain = integrationTestReplacements(domain)
 	c, err := toCert(domain, cert)
 	if err != nil {
 		return err
 	}
 
-	_, err = x.engine.Insert(c)
+	sess := x.engine.NewSession()
-	return err
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+	defer sess.Close()
+
+	if exist, _ := sess.ID(c.Domain).Exist(new(Cert)); exist {
+		if _, err := sess.ID(c.Domain).Update(c); err != nil {
+			return err
+		}
+	} else {
+		if _, err = sess.Insert(c); err != nil {
+			return err
+		}
+	}
+
+	return sess.Commit()
 }
 
 func (x xDB) Get(domain string) (*certificate.Resource, error) {
-	// TODO: do we need this or can we just go with domain name for wildcard cert
+	// handle wildcard certs
-	domain = strings.TrimPrefix(domain, ".")
+	if domain[:1] == "." {
+		domain = "*" + domain
+	}
+	domain = integrationTestReplacements(domain)
 
 	cert := new(Cert)
 	log.Trace().Str("domain", domain).Msg("get cert from db")
@@ -76,6 +95,12 @@ func (x xDB) Get(domain string) (*certificate.Resource, error) {
 }
 
 func (x xDB) Delete(domain string) error {
+	// handle wildcard certs
+	if domain[:1] == "." {
+		domain = "*" + domain
+	}
+	domain = integrationTestReplacements(domain)
+
 	log.Trace().Str("domain", domain).Msg("delete cert from db")
 	_, err := x.engine.ID(domain).Delete(new(Cert))
 	return err
@@ -119,3 +144,13 @@ func supportedDriver(driver string) bool {
 		return false
 	}
 }
+
+// integrationTestReplacements is needed because integration tests use a single domain cert,
+// while production use a wildcard cert
+// TODO: find a better way to handle this
+func integrationTestReplacements(domainKey string) string {
+	if domainKey == "*.localhost.mock.directory" {
+		return "localhost.mock.directory"
+	}
+	return domainKey
+}

server/database/xorm_test.go

@@ -0,0 +1,92 @@
+package database
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/go-acme/lego/v4/certificate"
+	"github.com/stretchr/testify/assert"
+	"xorm.io/xorm"
+)
+
+func newTestDB(t *testing.T) *xDB {
+	e, err := xorm.NewEngine("sqlite3", ":memory:")
+	assert.NoError(t, err)
+	assert.NoError(t, e.Sync2(new(Cert)))
+	return &xDB{engine: e}
+}
+
+func TestSanitizeWildcardCerts(t *testing.T) {
+	certDB := newTestDB(t)
+
+	_, err := certDB.Get(".not.found")
+	assert.True(t, errors.Is(err, ErrNotFound))
+
+	// TODO: cert key and domain mismatch are don not fail hard jet
+	// https://codeberg.org/Codeberg/pages-server/src/commit/d8595cee882e53d7f44f1ddc4ef8a1f7b8f31d8d/server/database/interface.go#L64
+	//
+	// assert.Error(t, certDB.Put(".wildcard.de", &certificate.Resource{
+	// 	Domain:      "*.localhost.mock.directory",
+	// 	Certificate: localhost_mock_directory_certificate,
+	// }))
+
+	// insert new wildcard cert
+	assert.NoError(t, certDB.Put(".wildcard.de", &certificate.Resource{
+		Domain:      "*.wildcard.de",
+		Certificate: localhost_mock_directory_certificate,
+	}))
+
+	// update existing cert
+	assert.NoError(t, certDB.Put(".wildcard.de", &certificate.Resource{
+		Domain:      "*.wildcard.de",
+		Certificate: localhost_mock_directory_certificate,
+	}))
+
+	c1, err := certDB.Get(".wildcard.de")
+	assert.NoError(t, err)
+	c2, err := certDB.Get("*.wildcard.de")
+	assert.NoError(t, err)
+	assert.EqualValues(t, c1, c2)
+}
+
+var localhost_mock_directory_certificate = []byte(`-----BEGIN CERTIFICATE-----
+MIIDczCCAlugAwIBAgIIJyBaXHmLk6gwDQYJKoZIhvcNAQELBQAwKDEmMCQGA1UE
+AxMdUGViYmxlIEludGVybWVkaWF0ZSBDQSA0OWE0ZmIwHhcNMjMwMjEwMDEwOTA2
+WhcNMjgwMjEwMDEwOTA2WjAjMSEwHwYDVQQDExhsb2NhbGhvc3QubW9jay5kaXJl
+Y3RvcnkwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDIU/CjzS7t62Gj
+neEMqvP7sn99ULT7AEUzEfWL05fWG2z714qcUg1hXkZLgdVDgmsCpplyddip7+2t
+ZH/9rLPLMqJphzvOL4CF6jDLbeifETtKyjnt9vUZFnnNWcP3tu8lo8iYSl08qsUI
+Pp/hiEriAQzCDjTbR5m9xUPNPYqxzcS4ALzmmCX9Qfc4CuuhMkdv2G4TT7rylWrA
+SCSRPnGjeA7pCByfNrO/uXbxmzl3sMO3k5sqgMkx1QIHEN412V8+vtx88mt2sM6k
+xjzGZWWKXlRq+oufIKX9KPplhsCjMH6E3VNAzgOPYDqXagtUcGmLWghURltO8Mt2
+zwM6OgjjAgMBAAGjgaUwgaIwDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsG
+AQUFBwMBBggrBgEFBQcDAjAMBgNVHRMBAf8EAjAAMB0GA1UdDgQWBBSMQvlJ1755
+sarf8i1KNqj7s5o/aDAfBgNVHSMEGDAWgBTcZcxJMhWdP7MecHCCpNkFURC/YzAj
+BgNVHREEHDAaghhsb2NhbGhvc3QubW9jay5kaXJlY3RvcnkwDQYJKoZIhvcNAQEL
+BQADggEBACcd7TT28OWwzQN2PcH0aG38JX5Wp2iOS/unDCfWjNAztXHW7nBDMxza
+VtyebkJfccexpuVuOsjOX+bww0vtEYIvKX3/GbkhogksBrNkE0sJZtMnZWMR33wa
+YxAy/kJBTmLi02r8fX9ZhwjldStHKBav4USuP7DXZjrgX7LFQhR4LIDrPaYqQRZ8
+ltC3mM9LDQ9rQyIFP5cSBMO3RUAm4I8JyLoOdb/9G2uxjHr7r6eG1g8DmLYSKBsQ
+mWGQDOYgR3cGltDe2yMxM++yHY+b1uhxGOWMrDA1+1k7yI19LL8Ifi2FMovDfu/X
+JxYk1NNNtdctwaYJFenmGQvDaIq1KgE=
+-----END CERTIFICATE-----
+-----BEGIN CERTIFICATE-----
+MIIDUDCCAjigAwIBAgIIKBJ7IIA6W1swDQYJKoZIhvcNAQELBQAwIDEeMBwGA1UE
+AxMVUGViYmxlIFJvb3QgQ0EgNTdmZjE2MCAXDTIzMDIwOTA1MzMxMloYDzIwNTMw
+MjA5MDUzMzEyWjAoMSYwJAYDVQQDEx1QZWJibGUgSW50ZXJtZWRpYXRlIENBIDQ5
+YTRmYjCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBANOvlqRx8SXQFWo2
+gFCiXxls53eENcyr8+meFyjgnS853eEvplaPxoa2MREKd+ZYxM8EMMfj2XGvR3UI
+aqR5QyLQ9ihuRqvQo4fG91usBHgH+vDbGPdMX8gDmm9HgnmtOVhSKJU+M2jfE1SW
+UuWB9xOa3LMreTXbTNfZEMoXf+GcWZMbx5WPgEga3DvfmV+RsfNvB55eD7YAyZgF
+ZnQ3Dskmnxxlkz0EGgd7rqhFHHNB9jARlL22gITADwoWZidlr3ciM9DISymRKQ0c
+mRN15fQjNWdtuREgJlpXecbYQMGhdTOmFrqdHkveD1o63rGSC4z+s/APV6xIbcRp
+aNpO7L8CAwEAAaOBgzCBgDAOBgNVHQ8BAf8EBAMCAoQwHQYDVR0lBBYwFAYIKwYB
+BQUHAwEGCCsGAQUFBwMCMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFNxlzEky
+FZ0/sx5wcIKk2QVREL9jMB8GA1UdIwQYMBaAFOqfkm9rebIz4z0SDIKW5edLg5JM
+MA0GCSqGSIb3DQEBCwUAA4IBAQBRG9AHEnyj2fKzVDDbQaKHjAF5jh0gwyHoIeRK
+FkP9mQNSWxhvPWI0tK/E49LopzmVuzSbDd5kZsaii73rAs6f6Rf9W5veo3AFSEad
+stM+Zv0f2vWB38nuvkoCRLXMX+QUeuL65rKxdEpyArBju4L3/PqAZRgMLcrH+ak8
+nvw5RdAq+Km/ZWyJgGikK6cfMmh91YALCDFnoWUWrCjkBaBFKrG59ONV9f0IQX07
+aNfFXFCF5l466xw9dHjw5iaFib10cpY3iq4kyPYIMs6uaewkCtxWKKjiozM4g4w3
+HqwyUyZ52WUJOJ/6G9DJLDtN3fgGR+IAp8BhYd5CqOscnt3h
+-----END CERTIFICATE-----`)

server/gitea/cache.go

@@ -80,7 +80,7 @@ type writeCacheReader struct {
 
 func (t *writeCacheReader) Read(p []byte) (n int, err error) {
 	n, err = t.originalReader.Read(p)
-	if err != nil {
+	if err != nil && err != io.EOF {
 		log.Trace().Err(err).Msgf("[cache] original reader for %q has returned an error", t.cacheKey)
 		t.hasError = true
 	} else if n > 0 {

server/handler/handler_test.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"net/http"
 	"net/http/httptest"
 	"testing"
 	"time"
@@ -24,7 +25,7 @@ func TestHandlerPerformance(t *testing.T) {
 
 	testCase := func(uri string, status int) {
 		t.Run(uri, func(t *testing.T) {
-			req := httptest.NewRequest("GET", uri, nil)
+			req := httptest.NewRequest("GET", uri, http.NoBody)
 			w := httptest.NewRecorder()
 
 			log.Printf("Start: %v\n", time.Now())

server/upstream/domains.go

@@ -1,6 +1,7 @@
 package upstream
 
 import (
+	"errors"
 	"strings"
 	"time"
 
@@ -30,8 +31,8 @@ func (o *Options) CheckCanonicalDomain(giteaClient *gitea.Client, actualDomain,
 	}
 
 	body, err := giteaClient.GiteaRawContent(o.TargetOwner, o.TargetRepo, o.TargetBranch, canonicalDomainConfig)
-	if err == nil || err == gitea.ErrorNotFound {
+	if err != nil && !errors.Is(err, gitea.ErrorNotFound) {
-		log.Info().Err(err).Msgf("could not read %s of %s/%s", canonicalDomainConfig, o.TargetOwner, o.TargetRepo)
+		log.Error().Err(err).Msgf("could not read %s of %s/%s", canonicalDomainConfig, o.TargetOwner, o.TargetRepo)
 	}
 
 	var domains []string
@@ -48,7 +49,7 @@ func (o *Options) CheckCanonicalDomain(giteaClient *gitea.Client, actualDomain,
 		}
 	}
 
-	// Add [owner].[pages-domain] as valid domnain.
+	// Add [owner].[pages-domain] as valid domain.
 	domains = append(domains, o.TargetOwner+mainDomainSuffix)
 	if domains[len(domains)-1] == actualDomain {
 		valid = true

server/utils/utils.go

@@ -1,6 +1,8 @@
 package utils
 
 import (
+	"net/url"
+	"path"
 	"strings"
 )
 
@@ -11,3 +13,15 @@ func TrimHostPort(host string) string {
 	}
 	return host
 }
+
+func CleanPath(uriPath string) string {
+	unescapedPath, _ := url.PathUnescape(uriPath)
+	cleanedPath := path.Join("/", unescapedPath)
+
+	// If the path refers to a directory, add a trailing slash.
+	if !strings.HasSuffix(cleanedPath, "/") && (strings.HasSuffix(unescapedPath, "/") || strings.HasSuffix(unescapedPath, "/.") || strings.HasSuffix(unescapedPath, "/..")) {
+		cleanedPath += "/"
+	}
+
+	return cleanedPath
+}

server/utils/utils_test.go

@@ -11,3 +11,59 @@ func TestTrimHostPort(t *testing.T) {
 	assert.EqualValues(t, "", TrimHostPort(":"))
 	assert.EqualValues(t, "example.com", TrimHostPort("example.com:80"))
 }
+
+// TestCleanPath is mostly copied from fasthttp, to keep the behaviour we had before migrating away from it.
+// Source (MIT licensed): https://github.com/valyala/fasthttp/blob/v1.48.0/uri_test.go#L154
+// Copyright (c) 2015-present Aliaksandr Valialkin, VertaMedia, Kirill Danshin, Erik Dubbelboer, FastHTTP Authors
+func TestCleanPath(t *testing.T) {
+	// double slash
+	testURIPathNormalize(t, "/aa//bb", "/aa/bb")
+
+	// triple slash
+	testURIPathNormalize(t, "/x///y/", "/x/y/")
+
+	// multi slashes
+	testURIPathNormalize(t, "/abc//de///fg////", "/abc/de/fg/")
+
+	// encoded slashes
+	testURIPathNormalize(t, "/xxxx%2fyyy%2f%2F%2F", "/xxxx/yyy/")
+
+	// dotdot
+	testURIPathNormalize(t, "/aaa/..", "/")
+
+	// dotdot with trailing slash
+	testURIPathNormalize(t, "/xxx/yyy/../", "/xxx/")
+
+	// multi dotdots
+	testURIPathNormalize(t, "/aaa/bbb/ccc/../../ddd", "/aaa/ddd")
+
+	// dotdots separated by other data
+	testURIPathNormalize(t, "/a/b/../c/d/../e/..", "/a/c/")
+
+	// too many dotdots
+	testURIPathNormalize(t, "/aaa/../../../../xxx", "/xxx")
+	testURIPathNormalize(t, "/../../../../../..", "/")
+	testURIPathNormalize(t, "/../../../../../../", "/")
+
+	// encoded dotdots
+	testURIPathNormalize(t, "/aaa%2Fbbb%2F%2E.%2Fxxx", "/aaa/xxx")
+
+	// double slash with dotdots
+	testURIPathNormalize(t, "/aaa////..//b", "/b")
+
+	// fake dotdot
+	testURIPathNormalize(t, "/aaa/..bbb/ccc/..", "/aaa/..bbb/")
+
+	// single dot
+	testURIPathNormalize(t, "/a/./b/././c/./d.html", "/a/b/c/d.html")
+	testURIPathNormalize(t, "./foo/", "/foo/")
+	testURIPathNormalize(t, "./../.././../../aaa/bbb/../../../././../", "/")
+	testURIPathNormalize(t, "./a/./.././../b/./foo.html", "/b/foo.html")
+}
+
+func testURIPathNormalize(t *testing.T, requestURI, expectedPath string) {
+	cleanedPath := CleanPath(requestURI)
+	if cleanedPath != expectedPath {
+		t.Fatalf("Unexpected path %q. Expected %q. requestURI=%q", cleanedPath, expectedPath, requestURI)
+	}
+}