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.

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

Jump to bottom

ServeDir in nested route causes invalid redirects #1731

Open
1 task done
yu-re-ka opened this issue Feb 2, 2023 · 11 comments · May be fixed by #2230
Open
1 task done

ServeDir in nested route causes invalid redirects #1731

yu-re-ka opened this issue Feb 2, 2023 · 11 comments · May be fixed by #2230
Labels
A-axum C-bug Category: This is a bug. E-hard Call for participation: Experience needed to fix: Hard / a lot

Comments

@yu-re-ka
Copy link

yu-re-ka commented Feb 2, 2023 •
edited
Loading

  • I have looked for existing issues (including closed) about this

Bug Report

Version

├── axum v0.5.13
│   ├── axum-core v0.2.7

│   ├── tower-http v0.3.4
└── tower-http v0.3.4 (*)

Platform

Linux yaya 6.1.7 #1-NixOS SMP PREEMPT_DYNAMIC Wed Jan 18 10:58:34 UTC 2023 x86_64 GNU/Linux

Description

https://yaya.yuka.dev/ is running a axum based webserver with a axum::Router. It has a nest route for /public that contains a ServeDir service with path /mnt/public/.

Now, in /mnt/public/, there exists an empty directory /mnt/public/foo.
I request https://yaya.yuka.dev/public/foo
And I get:

< HTTP/2 307 
< location: https://yaya.yuka.dev/foo/

while I would have expected a redirect to https://yaya.yuka.dev/public/foo/

This redirect is created here in ServeDir, and it uses the stripped path instead of the original uri to determine the redirect location.

Unsure how to solve this, but some ideas:

  • maybe the StripPrefix middleware and OriginalUri could be moved to tower_http and then tower_http can try to use the OriginalUri first and then fall back to the req.uri().

  • Or the nested route service could attempt to rewrite relative redirects, but that seems like it would not generally work since applications behind a reverse proxy could also have other ways of redirecting

  • Or ServeDir could be configured with a static prefix that is added to the redirects
@davidpdrsn davidpdrsn added C-bug Category: This is a bug. A-axum labels Feb 2, 2023
@davidpdrsn
Copy link
Member

maybe the StripPrefix middleware and OriginalUri could be moved to tower_http and then tower_http can try to use the OriginalUri first and then fall back to the req.uri().

That seems like the only viable solution to me. It just feels wrong to move axum specific stuff into tower-http to solve an, arguably, axum bug. StripPrefix also supports handling URL params for things like .nest("/:id", _). tower-http doesn't have anything like that currently. So I'd like to avoid moving things if at all possible. Will require some more thinking.

@davidpdrsn davidpdrsn added the E-hard Call for participation: Experience needed to fix: Hard / a lot label Feb 20, 2023
@jplatte
Copy link
Member

jplatte commented Feb 21, 2023

Yeah, this is definitely hard. We couldn't turn OriginalUri and the http::Request path modifications on its head either (putting the modified, rather than original path in an extension instead), because then ServeDir won't be able to find its files.

Maybe the best solution to be had is ServeDir getting a new knob where you can tell it that it's mounted at some non-root path?

@davidpdrsn
Copy link
Member

Maybe the best solution to be had is ServeDir getting a new knob where you can tell it that it's mounted at some non-root path?

Yeah that might be the simplest solution. It would be great if it "just worked" but not sure how.

@davidpdrsn davidpdrsn mentioned this issue Apr 11, 2023
Merged
@avdb13
Copy link
Contributor

avdb13 commented Apr 26, 2023

Was SpaRouter removed from axum-extra? OP might have been able to use that in the meanwhile as a temporary solution.

@davidpdrsn
Copy link
Member

SpaRouter was a thin wrapper around ServeDir so that would have been the same.

It was removed because ServeDir had gotten easier to use so SpaRouter didn't bring much value.

@gubsey
Copy link

gubsey commented Jul 19, 2023

Have there been any updates on this?

@davidpdrsn
Copy link
Member

No I don't think so.

davidpdrsn added a commit that referenced this issue Sep 17, 2023
@davidpdrsn
Add FixNestedRedirect middleware
c8b9f62 
This adds `FixNestedRedirectLayer` which uses `NestedPath` to change
redirects from nested services to include the path they're nested at.

Fixes #1731
@davidpdrsn davidpdrsn linked a pull request Sep 17, 2023 that will close this issue
Draft
@jplatte jplatte mentioned this issue Oct 3, 2023
Closed
80 tasks
@georgmu
Copy link

georgmu commented Oct 6, 2023

My approach to solve this would be to have a ServeDir::with_mount_point() (see georgmu/tower-http@32512d3 - it is still in testing and not yet in form to create a pull request).

This is still missing some mechanism to instantiate it without repeating the nested path in axum or reinsantiating ServeDir with every request...

@davidpdrsn
Copy link
Member

I've been putting together a middleware in axum to fix this #2230. Not quite there yet but that's another possible solution.

@austenadler
Copy link

@georgmu I like this patch and I know it isn't complete, but if it helps:

  • I have .nest_service("/abc", ServeDir::new(".").with_mount_point("/abc")) and it looks like axum isn't sending /abc, so this is always None
  • Redirects need to prepend the path

Not sure what the path forward is, but here's my fork that fits my needs when using axum: https://github.com/georgmu/tower-http/compare/serve-dir-mount-point...austenadler:tower-http:serve-dir-mount-point?expand=1

@georgmu
Copy link

georgmu commented Oct 8, 2023

From my point of view, the problem is the design decision to alter the URI for nested routers. I think that with the presence of NestedPath, this altering is not necessary any more, but modifying this behavior would result in a breaking change and I don't know if this is wanted for other use cases

@jplatte jplatte mentioned this issue Nov 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-axum C-bug Category: This is a bug. E-hard Call for participation: Experience needed to fix: Hard / a lot
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add FixNestedRedirect middleware tokio-rs/axum
7 participants
@davidpdrsn @jplatte @georgmu @austenadler @avdb13 @yu-re-ka @gubsey