From 69fb22a9e793e92e23e74900950ac9f60a049f9d Mon Sep 17 00:00:00 2001 From: Daniel Erat Date: Sat, 20 Apr 2024 11:00:15 +0000 Subject: [PATCH] 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 Co-authored-by: Daniel Erat Co-committed-by: Daniel Erat --- server/upstream/redirects.go | 15 +++++++++------ server/upstream/redirects_test.go | 17 ++++++++--------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/server/upstream/redirects.go b/server/upstream/redirects.go index e0601d8..ddde375 100644 --- a/server/upstream/redirects.go +++ b/server/upstream/redirects.go @@ -24,13 +24,16 @@ func (r *Redirect) rewriteURL(reqURL string) (dstURL string, ok bool) { 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 + 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 r.To, true } return "", false } diff --git a/server/upstream/redirects_test.go b/server/upstream/redirects_test.go index c1088a9..6118a70 100644 --- a/server/upstream/redirects_test.go +++ b/server/upstream/redirects_test.go @@ -19,15 +19,14 @@ func TestRedirect_rewriteURL(t *testing.T) { {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}, + {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",