From 9ffdc9d4f9406b0cb2e1d07346f65c557a58568a Mon Sep 17 00:00:00 2001 From: Daniel Erat Date: Thu, 18 Apr 2024 21:03:16 +0000 Subject: [PATCH] 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 Co-authored-by: Daniel Erat Co-committed-by: Daniel Erat --- server/upstream/redirects.go | 75 +++++++++++++------------------ server/upstream/redirects_test.go | 37 +++++++++++++++ 2 files changed, 68 insertions(+), 44 deletions(-) create mode 100644 server/upstream/redirects_test.go diff --git a/server/upstream/redirects.go b/server/upstream/redirects.go index dd36a84..e0601d8 100644 --- a/server/upstream/redirects.go +++ b/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 { - 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 } } diff --git a/server/upstream/redirects_test.go b/server/upstream/redirects_test.go new file mode 100644 index 0000000..c1088a9 --- /dev/null +++ b/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) + } + } +}