Avoid extra slashes in redirects with :splat (#308)

Remove leading slashes from captured portions of paths when
redirecting using splats. This makes a directive like
"/articles/*  /posts/:splat  302" behave as described in
FEATURES.md, i.e. "/articles/foo" now redirects to
"/posts/foo" rather than to "/posts//foo". Fixes #269.

This also changes the behavior of a redirect like
"/articles/*  /posts:splat  302". "/articles/foo" will now
redirect to "/postsfoo" rather than to "/posts/foo".

This change also fixes an issue where paths like
"/articles123" would be incorrectly matched by the above
patterns.

Reviewed-on: https://codeberg.org/Codeberg/pages-server/pulls/308
Reviewed-by: crapStone <codeberg@crapstone.dev>
Co-authored-by: Daniel Erat <dan@erat.org>
Co-committed-by: Daniel Erat <dan@erat.org>

README.md

@@ -80,6 +80,7 @@ and especially have a look at [this section of the haproxy.cfg](https://codeberg
 - `ENABLE_HTTP_SERVER` (default: false): Set this to true to enable the HTTP-01 challenge and redirect all other HTTP requests to HTTPS. Currently only works with port 80.
 - `DNS_PROVIDER` (default: use self-signed certificate): Code of the ACME DNS provider for the main domain wildcard.  
   See <https://go-acme.github.io/lego/dns/> for available values & additional environment variables.
+- `NO_DNS_01` (default: `false`): Disable the use of ACME DNS. This means that the wildcard certificate is self-signed and all domains and subdomains will have a distinct certificate. Because this may lead to a rate limit from the ACME provider, this option is not recommended for Gitea/Forgejo instances with open registrations or a great number of users/orgs.
 - `LOG_LEVEL` (default: warn): Set this to specify the level of logging.
 
 ## Contributing to the development

cli/flags.go

@@ -178,6 +178,11 @@ var (
 			Usage:   "Use DNS-Challenge for main domain. Read more at: https://go-acme.github.io/lego/dns/",
 			EnvVars: []string{"DNS_PROVIDER"},
 		},
+		&cli.BoolFlag{
+			Name:    "no-dns-01",
+			Usage:   "Always use individual certificates instead of a DNS-01 wild card certificate",
+			EnvVars: []string{"NO_DNS_01"},
+		},
 		&cli.StringFlag{
 			Name:    "acme-account-config",
 			Usage:   "json file of acme account",

config/config.go

@@ -42,5 +42,6 @@ type ACMEConfig struct {
 	EAB_HMAC          string
 	EAB_KID           string
 	DNSProvider       string
+	NoDNS01           bool   `default:"false"`
 	AccountConfigFile string `default:"acme-account.json"`
 }

config/setup.go

@@ -141,6 +141,9 @@ func mergeACMEConfig(ctx *cli.Context, config *ACMEConfig) {
 	if ctx.IsSet("dns-provider") {
 		config.DNSProvider = ctx.String("dns-provider")
 	}
+	if ctx.IsSet("no-dns-01") {
+		config.NoDNS01 = ctx.Bool("no-dns-01")
+	}
 	if ctx.IsSet("acme-account-config") {
 		config.AccountConfigFile = ctx.String("acme-account-config")
 	}

config/setup_test.go

@@ -166,6 +166,7 @@ func TestMergeConfigShouldReplaceAllExistingValuesGivenAllArgsExist(t *testing.T
 					EAB_HMAC:          "original",
 					EAB_KID:           "original",
 					DNSProvider:       "original",
+					NoDNS01:           false,
 					AccountConfigFile: "original",
 				},
 			}
@@ -205,6 +206,7 @@ func TestMergeConfigShouldReplaceAllExistingValuesGivenAllArgsExist(t *testing.T
 					EAB_HMAC:          "changed",
 					EAB_KID:           "changed",
 					DNSProvider:       "changed",
+					NoDNS01:           true,
 					AccountConfigFile: "changed",
 				},
 			}
@@ -243,6 +245,7 @@ func TestMergeConfigShouldReplaceAllExistingValuesGivenAllArgsExist(t *testing.T
 			"--acme-eab-hmac", "changed",
 			"--acme-eab-kid", "changed",
 			"--dns-provider", "changed",
+			"--no-dns-01",
 			"--acme-account-config", "changed",
 		},
 	)
@@ -517,6 +520,7 @@ func TestMergeACMEConfigShouldReplaceAllExistingValuesGivenAllArgsExist(t *testi
 				EAB_HMAC:          "original",
 				EAB_KID:           "original",
 				DNSProvider:       "original",
+				NoDNS01:           false,
 				AccountConfigFile: "original",
 			}
 
@@ -530,6 +534,7 @@ func TestMergeACMEConfigShouldReplaceAllExistingValuesGivenAllArgsExist(t *testi
 				EAB_HMAC:          "changed",
 				EAB_KID:           "changed",
 				DNSProvider:       "changed",
+				NoDNS01:           true,
 				AccountConfigFile: "changed",
 			}
 
@@ -545,6 +550,7 @@ func TestMergeACMEConfigShouldReplaceAllExistingValuesGivenAllArgsExist(t *testi
 			"--acme-eab-hmac", "changed",
 			"--acme-eab-kid", "changed",
 			"--dns-provider", "changed",
+			"--no-dns-01",
 			"--acme-account-config", "changed",
 		},
 	)
@@ -563,6 +569,7 @@ func TestMergeACMEConfigShouldReplaceOnlyOneValueExistingValueGivenOnlyOneArgExi
 		{args: []string{"--acme-eab-hmac", "changed"}, callback: func(gc *ACMEConfig) { gc.EAB_HMAC = "changed" }},
 		{args: []string{"--acme-eab-kid", "changed"}, callback: func(gc *ACMEConfig) { gc.EAB_KID = "changed" }},
 		{args: []string{"--dns-provider", "changed"}, callback: func(gc *ACMEConfig) { gc.DNSProvider = "changed" }},
+		{args: []string{"--no-dns-01"}, callback: func(gc *ACMEConfig) { gc.NoDNS01 = true }},
 		{args: []string{"--acme-account-config", "changed"}, callback: func(gc *ACMEConfig) { gc.AccountConfigFile = "changed" }},
 	}
 

server/acme/client.go

@@ -13,8 +13,8 @@ var ErrAcmeMissConfig = errors.New("ACME client has wrong config")
 
 func CreateAcmeClient(cfg config.ACMEConfig, enableHTTPServer bool, challengeCache cache.ICache) (*certificates.AcmeClient, error) {
 	// check config
-	if (!cfg.AcceptTerms || cfg.DNSProvider == "") && cfg.APIEndpoint != "https://acme.mock.directory" {
-		return nil, fmt.Errorf("%w: you must set $ACME_ACCEPT_TERMS and $DNS_PROVIDER, unless $ACME_API is set to https://acme.mock.directory", ErrAcmeMissConfig)
+	if (!cfg.AcceptTerms || (cfg.DNSProvider == "" && !cfg.NoDNS01)) && cfg.APIEndpoint != "https://acme.mock.directory" {
+		return nil, fmt.Errorf("%w: you must set $ACME_ACCEPT_TERMS and $DNS_PROVIDER or $NO_DNS_01, unless $ACME_API is set to https://acme.mock.directory", ErrAcmeMissConfig)
 	}
 	if cfg.EAB_HMAC != "" && cfg.EAB_KID == "" {
 		return nil, fmt.Errorf("%w: ACME_EAB_HMAC also needs ACME_EAB_KID to be set", ErrAcmeMissConfig)

server/certificates/acme_client.go

@@ -56,11 +56,8 @@ func NewAcmeClient(cfg config.ACMEConfig, enableHTTPServer bool, challengeCache
 		log.Error().Err(err).Msg("Can't create ACME client, continuing with mock certs only")
 	} else {
 		if cfg.DNSProvider == "" {
-			// using mock server, don't use wildcard certs
-			err := mainDomainAcmeClient.Challenge.SetTLSALPN01Provider(AcmeTLSChallengeProvider{challengeCache})
-			if err != nil {
-				log.Error().Err(err).Msg("Can't create TLS-ALPN-01 provider")
-			}
+			// using mock wildcard certs
+			mainDomainAcmeClient = nil
 		} else {
 			// use DNS-Challenge https://go-acme.github.io/lego/dns/
 			provider, err := dns.NewDNSChallengeProviderByName(cfg.DNSProvider)

server/certificates/certificates.go

@@ -33,6 +33,8 @@ func TLSConfig(mainDomainSuffix string,
 	firstDefaultBranch string,
 	keyCache, challengeCache, dnsLookupCache, canonicalDomainCache cache.ICache,
 	certDB database.CertDB,
+	noDNS01 bool,
+	rawDomain string,
 ) *tls.Config {
 	return &tls.Config{
 		// check DNS name & get certificate from Let's Encrypt
@@ -64,9 +66,24 @@ func TLSConfig(mainDomainSuffix string,
 
 			targetOwner := ""
 			mayObtainCert := true
+
 			if strings.HasSuffix(domain, mainDomainSuffix) || strings.EqualFold(domain, mainDomainSuffix[1:]) {
-				// deliver default certificate for the main domain (*.codeberg.page)
-				domain = mainDomainSuffix
+				if noDNS01 {
+					// Limit the domains allowed to request a certificate to pages-server domains
+					// and domains for an existing user of org
+					if !strings.EqualFold(domain, mainDomainSuffix[1:]) && !strings.EqualFold(domain, rawDomain) {
+						targetOwner := strings.TrimSuffix(domain, mainDomainSuffix)
+						owner_exist, err := giteaClient.GiteaCheckIfOwnerExists(targetOwner)
+						mayObtainCert = owner_exist
+						if err != nil {
+							log.Error().Err(err).Msgf("Failed to check '%s' existence on the forge: %s", targetOwner, err)
+							mayObtainCert = false
+						}
+					}
+				} else {
+					// deliver default certificate for the main domain (*.codeberg.page)
+					domain = mainDomainSuffix
+				}
 			} else {
 				var targetRepo, targetBranch string
 				targetOwner, targetRepo, targetBranch = dnsutils.GetTargetFromDNS(domain, mainDomainSuffix, firstDefaultBranch, dnsLookupCache)
@@ -199,9 +216,6 @@ func (c *AcmeClient) retrieveCertFromDB(sni, mainDomainSuffix string, useDnsProv
 
 func (c *AcmeClient) obtainCert(acmeClient *lego.Client, domains []string, renew *certificate.Resource, user string, useDnsProvider bool, mainDomainSuffix string, keyDatabase database.CertDB) (*tls.Certificate, error) {
 	name := strings.TrimPrefix(domains[0], "*")
-	if useDnsProvider && domains[0] != "" && domains[0][0] == '*' {
-		domains = domains[1:]
-	}
 
 	// lock to avoid simultaneous requests
 	_, working := c.obtainLocks.LoadOrStore(name, struct{}{})
@@ -219,7 +233,11 @@ func (c *AcmeClient) obtainCert(acmeClient *lego.Client, domains []string, renew
 	defer c.obtainLocks.Delete(name)
 
 	if acmeClient == nil {
-		return mockCert(domains[0], "ACME client uninitialized. This is a server error, please report!", mainDomainSuffix, keyDatabase)
+		if useDnsProvider {
+			return mockCert(domains[0], "DNS ACME client is not defined", mainDomainSuffix, keyDatabase)
+		} else {
+			return mockCert(domains[0], "ACME client uninitialized. This is a server error, please report!", mainDomainSuffix, keyDatabase)
+		}
 	}
 
 	// request actual cert

server/database/xorm.go

@@ -52,7 +52,6 @@ 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
@@ -82,7 +81,6 @@ func (x xDB) Get(domain string) (*certificate.Resource, error) {
 	if domain[:1] == "." {
 		domain = "*" + domain
 	}
-	domain = integrationTestReplacements(domain)
 
 	cert := new(Cert)
 	log.Trace().Str("domain", domain).Msg("get cert from db")
@@ -99,7 +97,6 @@ func (x xDB) Delete(domain string) error {
 	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))
@@ -139,13 +136,3 @@ 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/gitea/cache.go

@@ -26,6 +26,9 @@ const (
 	// TODO: move as option into cache interface
 	fileCacheTimeout = 5 * time.Minute
 
+	// ownerExistenceCacheTimeout specifies the timeout for the existence of a repo/org
+	ownerExistenceCacheTimeout = 5 * time.Minute
+
 	// fileCacheSizeLimit limits the maximum file size that will be cached, and is set to 1 MB by default.
 	fileCacheSizeLimit = int64(1000 * 1000)
 )

server/gitea/client.go

@@ -28,6 +28,7 @@ const (
 	branchTimestampCacheKeyPrefix = "branchTime"
 	defaultBranchCacheKeyPrefix   = "defaultBranch"
 	rawContentCacheKeyPrefix      = "rawContent"
+	ownerExistenceKeyPrefix       = "ownerExist"
 
 	// pages server
 	PagesCacheIndicatorHeader = "X-Pages-Cache"
@@ -266,6 +267,38 @@ func (client *Client) GiteaGetRepoDefaultBranch(repoOwner, repoName string) (str
 	return branch, nil
 }
 
+func (client *Client) GiteaCheckIfOwnerExists(owner string) (bool, error) {
+	cacheKey := fmt.Sprintf("%s/%s", ownerExistenceKeyPrefix, owner)
+
+	if exist, ok := client.responseCache.Get(cacheKey); ok && exist != nil {
+		return exist.(bool), nil
+	}
+
+	_, resp, err := client.sdkClient.GetUserInfo(owner)
+	if resp.StatusCode == http.StatusOK && err == nil {
+		if err := client.responseCache.Set(cacheKey, true, ownerExistenceCacheTimeout); err != nil {
+			log.Error().Err(err).Msg("[cache] error on cache write")
+		}
+		return true, nil
+	} else if resp.StatusCode != http.StatusNotFound {
+		return false, err
+	}
+
+	_, resp, err = client.sdkClient.GetOrg(owner)
+	if resp.StatusCode == http.StatusOK && err == nil {
+		if err := client.responseCache.Set(cacheKey, true, ownerExistenceCacheTimeout); err != nil {
+			log.Error().Err(err).Msg("[cache] error on cache write")
+		}
+		return true, nil
+	} else if resp.StatusCode != http.StatusNotFound {
+		return false, err
+	}
+	if err := client.responseCache.Set(cacheKey, false, ownerExistenceCacheTimeout); err != nil {
+		log.Error().Err(err).Msg("[cache] error on cache write")
+	}
+	return false, nil
+}
+
 func (client *Client) getMimeTypeByExtension(resource string) string {
 	mimeType := mime.TypeByExtension(path.Ext(resource))
 	mimeTypeSplit := strings.SplitN(mimeType, ";", 2)

server/handler/try.go

@@ -1,6 +1,7 @@
 package handler
 
 import (
+	"fmt"
 	"net/http"
 	"strings"
 
@@ -41,7 +42,7 @@ func tryUpstream(ctx *context.Context, giteaClient *gitea.Client,
 
 	// Try to request the file from the Gitea API
 	if !options.Upstream(ctx, giteaClient, redirectsCache) {
-		html.ReturnErrorPage(ctx, "forge client failed", ctx.StatusCode)
+		html.ReturnErrorPage(ctx, fmt.Sprintf("Forge returned %d %s", ctx.StatusCode, http.StatusText(ctx.StatusCode)), ctx.StatusCode)
 	}
 }
 

server/startup.go

@@ -110,6 +110,8 @@ func Serve(ctx *cli.Context) error {
 		cfg.Server.PagesBranches[0],
 		keyCache, challengeCache, dnsLookupCache, canonicalDomainCache,
 		certDB,
+		cfg.ACME.NoDNS01,
+		cfg.Server.RawDomain,
 	))
 
 	interval := 12 * time.Hour

server/upstream/redirects.go

@@ -17,6 +17,27 @@ type Redirect struct {
 	StatusCode int
 }
 
+// rewriteURL returns the destination URL and true if r matches reqURL.
+func (r *Redirect) rewriteURL(reqURL string) (dstURL string, ok bool) {
+	// check if from url matches request url
+	if strings.TrimSuffix(r.From, "/") == strings.TrimSuffix(reqURL, "/") {
+		return r.To, true
+	}
+	// handle wildcard redirects
+	if strings.HasSuffix(r.From, "/*") {
+		trimmedFromURL := strings.TrimSuffix(r.From, "/*")
+		if reqURL == trimmedFromURL || strings.HasPrefix(reqURL, trimmedFromURL+"/") {
+			if strings.Contains(r.To, ":splat") {
+				matched := strings.TrimPrefix(reqURL, trimmedFromURL)
+				matched = strings.TrimPrefix(matched, "/")
+				return strings.ReplaceAll(r.To, ":splat", matched), true
+			}
+			return r.To, true
+		}
+	}
+	return "", false
+}
+
 // redirectsCacheTimeout specifies the timeout for the redirects cache.
 var redirectsCacheTimeout = 10 * time.Minute
 
@@ -64,52 +85,21 @@ func (o *Options) getRedirects(giteaClient *gitea.Client, redirectsCache cache.I
 }
 
 func (o *Options) matchRedirects(ctx *context.Context, giteaClient *gitea.Client, redirects []Redirect, redirectsCache cache.ICache) (final bool) {
-	if len(redirects) > 0 {
-		for _, redirect := range redirects {
-			reqUrl := ctx.Req.RequestURI
-			// remove repo and branch from request url
-			reqUrl = strings.TrimPrefix(reqUrl, "/"+o.TargetRepo)
-			reqUrl = strings.TrimPrefix(reqUrl, "/@"+o.TargetBranch)
+	reqURL := ctx.Req.RequestURI
+	// remove repo and branch from request url
+	reqURL = strings.TrimPrefix(reqURL, "/"+o.TargetRepo)
+	reqURL = strings.TrimPrefix(reqURL, "/@"+o.TargetBranch)
 
-			// check if from url matches request url
-			if strings.TrimSuffix(redirect.From, "/") == strings.TrimSuffix(reqUrl, "/") {
-				// do rewrite if status code is 200
-				if redirect.StatusCode == 200 {
-					o.TargetPath = redirect.To
-					o.Upstream(ctx, giteaClient, redirectsCache)
-					return true
-				} else {
-					ctx.Redirect(redirect.To, redirect.StatusCode)
-					return true
-				}
-			}
-
-			// handle wildcard redirects
-			trimmedFromUrl := strings.TrimSuffix(redirect.From, "/*")
-			if strings.HasSuffix(redirect.From, "/*") && strings.HasPrefix(reqUrl, trimmedFromUrl) {
-				if strings.Contains(redirect.To, ":splat") {
-					splatUrl := strings.ReplaceAll(redirect.To, ":splat", strings.TrimPrefix(reqUrl, trimmedFromUrl))
-					// do rewrite if status code is 200
-					if redirect.StatusCode == 200 {
-						o.TargetPath = splatUrl
-						o.Upstream(ctx, giteaClient, redirectsCache)
-						return true
-					} else {
-						ctx.Redirect(splatUrl, redirect.StatusCode)
-						return true
-					}
-				} else {
-					// do rewrite if status code is 200
-					if redirect.StatusCode == 200 {
-						o.TargetPath = redirect.To
-						o.Upstream(ctx, giteaClient, redirectsCache)
-						return true
-					} else {
-						ctx.Redirect(redirect.To, redirect.StatusCode)
-						return true
-					}
-				}
+	for _, redirect := range redirects {
+		if dstURL, ok := redirect.rewriteURL(reqURL); ok {
+			// do rewrite if status code is 200
+			if redirect.StatusCode == 200 {
+				o.TargetPath = dstURL
+				o.Upstream(ctx, giteaClient, redirectsCache)
+			} else {
+				ctx.Redirect(dstURL, redirect.StatusCode)
 			}
+			return true
 		}
 	}
 

server/upstream/redirects_test.go

@@ -0,0 +1,36 @@
+package upstream
+
+import (
+	"testing"
+)
+
+func TestRedirect_rewriteURL(t *testing.T) {
+	for _, tc := range []struct {
+		redirect   Redirect
+		reqURL     string
+		wantDstURL string
+		wantOk     bool
+	}{
+		{Redirect{"/", "/dst", 200}, "/", "/dst", true},
+		{Redirect{"/", "/dst", 200}, "/foo", "", false},
+		{Redirect{"/src", "/dst", 200}, "/src", "/dst", true},
+		{Redirect{"/src", "/dst", 200}, "/foo", "", false},
+		{Redirect{"/src", "/dst", 200}, "/src/foo", "", false},
+		{Redirect{"/*", "/dst", 200}, "/", "/dst", true},
+		{Redirect{"/*", "/dst", 200}, "/src", "/dst", true},
+		{Redirect{"/src/*", "/dst/:splat", 200}, "/src", "/dst/", true},
+		{Redirect{"/src/*", "/dst/:splat", 200}, "/src/", "/dst/", true},
+		{Redirect{"/src/*", "/dst/:splat", 200}, "/src/foo", "/dst/foo", true},
+		{Redirect{"/src/*", "/dst/:splat", 200}, "/src/foo/bar", "/dst/foo/bar", true},
+		{Redirect{"/src/*", "/dst/:splatsuffix", 200}, "/src/foo", "/dst/foosuffix", true},
+		{Redirect{"/src/*", "/dst:splat", 200}, "/src/foo", "/dstfoo", true},
+		{Redirect{"/src/*", "/dst", 200}, "/srcfoo", "", false},
+		// This is the example from FEATURES.md:
+		{Redirect{"/articles/*", "/posts/:splat", 302}, "/articles/2022/10/12/post-1/", "/posts/2022/10/12/post-1/", true},
+	} {
+		if dstURL, ok := tc.redirect.rewriteURL(tc.reqURL); dstURL != tc.wantDstURL || ok != tc.wantOk {
+			t.Errorf("%#v.rewriteURL(%q) = %q, %v; want %q, %v",
+				tc.redirect, tc.reqURL, dstURL, ok, tc.wantDstURL, tc.wantOk)
+		}
+	}
+}