Refactor redirect code and add tests (#304)

Move repetitive code from Options.matchRedirects into a new
Redirect.rewriteURL method and add a new test file.

No functional changes are intended; this is in preparation
for a later change to address #269.

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

server/upstream/redirects.go

@@ -17,6 +17,24 @@ 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
+	trimmedFromURL := strings.TrimSuffix(r.From, "/*")
+	if strings.HasSuffix(r.From, "/*") && strings.HasPrefix(reqURL, trimmedFromURL) {
+		if strings.Contains(r.To, ":splat") {
+			splatURL := strings.ReplaceAll(r.To, ":splat", strings.TrimPrefix(reqURL, trimmedFromURL))
+			return splatURL, true
+		}
+		return r.To, true
+	}
+	return "", false
+}
+
 // redirectsCacheTimeout specifies the timeout for the redirects cache.
 var redirectsCacheTimeout = 10 * time.Minute
 
@@ -64,52 +82,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 {
+	reqURL := ctx.Req.RequestURI
-		for _, redirect := range redirects {
+	// remove repo and branch from request url
-			reqUrl := ctx.Req.RequestURI
+	reqURL = strings.TrimPrefix(reqURL, "/"+o.TargetRepo)
-			// remove repo and branch from request url
+	reqURL = strings.TrimPrefix(reqURL, "/@"+o.TargetBranch)
-			reqUrl = strings.TrimPrefix(reqUrl, "/"+o.TargetRepo)
-			reqUrl = strings.TrimPrefix(reqUrl, "/@"+o.TargetBranch)
 
-			// check if from url matches request url
+	for _, redirect := range redirects {
-			if strings.TrimSuffix(redirect.From, "/") == strings.TrimSuffix(reqUrl, "/") {
+		if dstURL, ok := redirect.rewriteURL(reqURL); ok {
-				// do rewrite if status code is 200
+			// do rewrite if status code is 200
-				if redirect.StatusCode == 200 {
+			if redirect.StatusCode == 200 {
-					o.TargetPath = redirect.To
+				o.TargetPath = dstURL
-					o.Upstream(ctx, giteaClient, redirectsCache)
+				o.Upstream(ctx, giteaClient, redirectsCache)
-					return true
+			} else {
-				} else {
+				ctx.Redirect(dstURL, redirect.StatusCode)
-					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
-					}
-				}
 			}
+			return true
 		}
 	}
 

server/upstream/redirects_test.go

@@ -0,0 +1,37 @@
+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},
+		// TODO: There shouldn't be double-slashes in these URLs:
+		// https://codeberg.org/Codeberg/pages-server/issues/269
+		{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},
+		// TODO: This is a workaround for the above bug. Should it be preserved?
+		{Redirect{"/src/*", "/dst:splat", 200}, "/src/foo", "/dst/foo", true},
+		// TODO: This behavior is incorrect; no redirect should be performed.
+		{Redirect{"/src/*", "/dst", 200}, "/srcfoo", "/dst", 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)
+		}
+	}
+}