Skip to content

Consistent QueryStrings on redirect (fixes #3430) #3431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
xavivars wants to merge 1 commit into
base: master
Choose a base branch
from
+8 −6
Open

Consistent QueryStrings on redirect (fixes #3430) #3431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions filters/builtin/redirect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ func getLocation(ctx filters.FilterContext, location *url.URL, typ redirectType)

if u.RawQuery == "" {
u.RawQuery = r.URL.RawQuery
} else {
u.RawQuery = r.URL.RawQuery + "&" + u.RawQuery
Copy link
Member

@szuecs szuecs Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the issue comment.
I think we need to have a separate input if you need this behavior, because we do not know how many users could have critical redirects that expect the current behavior.
I think the current behavior is correct.

We could have a 3rd optional parameter to mix the input query and configured query, see also doc.

Copy link
Contributor Author

@xavivars xavivars Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had read the docs, but they say nothing about query (I actually thought when reading the docs they were dropping whatever query was coming from the original request)

}

return u.String()
Expand Down
12 changes: 6 additions & 6 deletions filters/builtin/redirect_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func TestRedirect(t *testing.T) {
"schema, host, path and query",
http.StatusFound,
"http://redirect.example.org/some/other/path?newquery=3",
"http://redirect.example.org/some/other/path?newquery=3",
"http://redirect.example.org/some/other/path?foo=1&bar=2&newquery=3",
false,
}, {
"host only",
Expand All @@ -69,7 +69,7 @@ func TestRedirect(t *testing.T) {
"host, path and query",
http.StatusFound,
"//redirect.example.org/some/other/path?newquery=3",
"https://redirect.example.org/some/other/path?newquery=3",
"https://redirect.example.org/some/other/path?foo=1&bar=2&newquery=3",
false,
}, {
"path only",
Expand All @@ -81,13 +81,13 @@ func TestRedirect(t *testing.T) {
"path and query",
http.StatusFound,
"/some/other/path?newquery=3",
"https://incoming.example.org/some/other/path?newquery=3",
"https://incoming.example.org/some/other/path?foo=1&bar=2&newquery=3",
false,
}, {
"query only",
http.StatusFound,
"?newquery=3",
"https://incoming.example.org/some/path?newquery=3",
"https://incoming.example.org/some/path?foo=1&bar=2&newquery=3",
false,
}, {
"schema and path",
Expand All @@ -99,13 +99,13 @@ func TestRedirect(t *testing.T) {
"schema, path and query",
http.StatusFound,
"http:///some/other/path?newquery=3",
"http://incoming.example.org/some/other/path?newquery=3",
"http://incoming.example.org/some/other/path?foo=1&bar=2&newquery=3",
false,
}, {
"schema and query",
http.StatusFound,
"http://?newquery=3",
"http://incoming.example.org/some/path?newquery=3",
"http://incoming.example.org/some/path?foo=1&bar=2&newquery=3",
false,
}, {
"different code",
Expand Down