Preserve the query-string for non browse case

[jaygooby]

when doing canonical redirect. Under caddy v2.6.2 the query-string is lost.

This is related to https://github.com/caddyserver/caddy/pull/5253 which does the same thing when browsing is enabled, but does not fix it for the non-browsing case.

Test like this:

mkdir -p /tmp/caddy/example
echo "hello" >  /tmp/caddy/example/index.html

caddy file-server --root /tmp/caddy

curl "http://127.0.0.1/example?query=retained"
<a href="/example/">Permanent Redirect</a>.

with the fix in place, the query-string is correctly preserved:

curl "http://127.0.0.1/example?query=retained"
<a href="/example/?query=retained">Permanent Redirect</a>.

[francislavoie]

origReq should not be manipulated directly. That can cause side effects. Instead, the URL should be cloned, and that clone can be manipulated.

Either way, I'm not convinced this is safe. There's subtle issues with carefully crafted requests that can cause open redirects to occur.

[jaygooby]

I’m happy to work on this, to get the correct behaviour. Are there a set of test cases that show requests that can cause open redirects?

The background to this, is that I have a simple setup that isn’t working and after trying a load of different Caddyfile configs started to look at whether the issue was a bug with caddy itself: https://stackoverflow.com/questions/74802005/retain-query-on-caddy-canonical-redirect-for-file-server-directive

[mholt]

Hi Jay, I'll take a look at this as soon as I can and try to figure something out together :)

[jaygooby]

Thanks @mholt I'm a golang newbie. origReq is assigned from the request, and then I'm returning a string representing the final URL, so it didn't seem like it would be a dangerous operation 😅

[mholt]

We might be able to safely append the query string to the to variable if there is one.

@jaygooby Armed with the docs for https://pkg.go.dev/net/url#URL, think you can try it out?

[jaygooby]

Thanks for the docs link! Yup, I'll give it a go later today

[mholt]

Awesome. FWIW if you go to the git history for that file, this commit shows up:

https://github.com/caddyserver/caddy/commit/8848df9c5d372a559d01512b7a4ef00e38867b55

Where I revert a commit that did attempt to preserve the QS. Here's the background: https://github.com/caddyserver/caddy/issues/4205 (it seems complicated).

[jaygooby]

Thanks, I'll also bear in mind the three test cases you note in https://github.com/caddyserver/caddy/issues/4205#issuecomment-863352037

[jaygooby]

Would it help to have a separate caddytest/integration/redirect_test.go with the various different cases in?

[mholt]

Sure. Tests don't hurt :)

[jaygooby]

OK, have started to get my head around tests.

I've implemented a basic redirect and Test case 1 from your https://github.com/caddyserver/caddy/issues/4205#issuecomment-863352037 in this integration test

I'll get your test case 2 & 3 implemented too, along with one to test the querystring preservation, then I'll try the to variable appending to get it passing

[mholt]

@jaygooby Thanks for working on this, I'm trying to circle back around very soon and inspect this carefully.

[mholt]

Superseded by #6109. Thank you for helping to get this started! Sorry it took me so long.