Skip to content
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.

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

fileserver: Redirect within the original URL #4179

Merged
merged 1 commit into from Jun 7, 2021

Conversation

diamondburned
Copy link
Contributor

This commit changes the file_server directive to redirect using the original request's URL instead of the possibly trimmed URL. This should make file_server work with handle_path.

This fix is taken from mholt's comment in https://caddy.community/t/file-servers-on-different-paths-not-working/11698/11.


For testing purposes, below Caddyfile should reproduce the before and afters:

{
	debug
	admin off
}

http://127.0.0.1:28475 {
	root * /home/diamond

	handle_path /browse* {
		file_server browse
	}

	respond "Hi"
}

When accessing http://127.0.0.1:28475/browse, the redirection would lead to /; after the changes, it would lead to /browse/.

@CLAassistant
Copy link

CLAassistant commented May 25, 2021

CLA assistant check
All committers have signed the CLA.

diamondburned added a commit to diamondburned/caddy-upshare that referenced this pull request May 25, 2021
@francislavoie francislavoie added the under review 🧐 Review is pending before merging label May 26, 2021
@francislavoie francislavoie added this to the v2.5.0 milestone May 26, 2021
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Thanks for this! I'd been meaning to look into this at some point. Glad someone got to it before me 😁

FWIW, I realized this was a problem in https://caddy.community/t/route-directive-file-server-not-picking-up-index-html/12562 as well.

modules/caddyhttp/fileserver/browse.go Show resolved Hide resolved
@francislavoie
Copy link
Member

I'm also worried about the situation where the original URL already has a / but the rewritten URL doesn't (can happen when the rewrite directive is used, instead of it being inside of a handle_path).

I think the original request path needs to be considered as well to not do a redirect when it doesn't make sense.

This commit changes the file_server directive to redirect using the
original request's URL instead of the possibly trimmed URL. This should
make file_server work with handle_path.

This fix is taken from mholt's comment in
https://caddy.community/t/file-servers-on-different-paths-not-working/11698/11.
@diamondburned
Copy link
Contributor Author

I think the original request path needs to be considered as well to not do a redirect when it doesn't make sense.

Do you mean both the original path and the new path, or just the original path? I've changed the code to check the original path instead; I think it makes more sense that way as well.

@francislavoie francislavoie requested a review from mholt May 26, 2021 01:48
@francislavoie
Copy link
Member

Do you mean both the original path and the new path, or just the original path? I've changed the code to check the original path instead; I think it makes more sense that way as well.

I was thinking both, but yeah I think that makes sense to only look at the original request, because the point of the canonicalization redirect is to make sure that from the browser's perspective, the URL does have a trailing / so that relative path links go to the right place. 👍

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for this change, let's try it out.

Edit: This PR turned out to be a regression and has been reverted. See #4205.

Edit 2: I have pushed a more correct implementation of this change in fbd6560, where we only redirect if the base (last element, a.k.a. filename) of the path was not rewritten, that will be released in v2.4.3.

@mholt mholt modified the milestones: v2.5.0, v2.4.2 Jun 7, 2021
@mholt mholt merged commit f9b5445 into caddyserver:master Jun 7, 2021
@mholt mholt removed the under review 🧐 Review is pending before merging label Jun 7, 2021
diamondburned added a commit to diamondburned/caddy that referenced this pull request Jun 7, 2021
This commit is a follow up to PR caddyserver#4179 that introduced a bug where
browse redirections to the right URL would not preserve query
parameters.
diamondburned added a commit to diamondburned/caddy that referenced this pull request Jun 7, 2021
This commit is a follow up to PR caddyserver#4179 that introduced a bug where
browse redirections to the right URL would not preserve query
parameters.
diamondburned added a commit to diamondburned/caddy that referenced this pull request Jun 7, 2021
This commit is a follow up to PR caddyserver#4179 that introduced a bug where
browse redirections to the right URL would not preserve query
parameters.
mholt pushed a commit that referenced this pull request Jun 7, 2021
This commit is a follow up to PR #4179 that introduced a bug where
browse redirections to the right URL would not preserve query
parameters.
mholt added a commit that referenced this pull request Jun 14, 2021
mholt added a commit that referenced this pull request Jun 17, 2021
This is the more correct implementation of  23dadc0 (#4179)... I think. This commit effectively undoes the revert in 8848df9, but with corrections to the logic.

We *do* need to use the original request path (the path the browser knows) for redirects, since they are external, and rewrites are only internal.

However, if the path was rewritten to a non-canonical path, we should not redirect to canonicalize that, since rewrites are intentional by the site owner. Canonicalizing the path involves modifying only the suffix (base element, or filename) of the path. Thus, if a rewrite involves only the prefix (like how handle_path strips a path prefix), then we can (hopefully!) safely redirect using the original URI since the filename was not rewritten.

So basically, if rewrites modify the filename, we should not canonicalize those requests. If rewrites only modify another part of the path (commonly a prefix), we should be OK to redirect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants