tokio-rs / axum

Ergonomic and modular web framework built with Tokio, Tower, and Hyper
19.19k stars 1.06k forks source link

bug with / in nest #2267

Closed mortifia closed 8 months ago

mortifia commented 1 year ago

Bug Report

Version

── axum v0.6.20
│   ├── axum-core v0.3.4
│   │       ├── axum v0.6.20 (*)
    ├── axum v0.6.20 (*)

Platform

Linux mortifia 6.5.5-1-MANJARO #1 SMP PREEMPT_DYNAMIC Sat Sep 23 12:48:15 UTC 2023 x86_64 GNU/Linux

Description

    let app = Router::new()
        .route("/", get(hello_world))
        .nest("/advertisement", advertisement::router())

  ...... (advertisement::router)

  pub fn router() -> Router {
    Router::new()
        .route("/", get(advertisement))
        .route("/:id", get(advertisement_id))
}

GET http://localhost:3000/advertisement -> .route("/", get(advertisement)) GET http://localhost:3000/advertisement/xxxx -> .route("/:id", get(advertisement_id)) GET http://localhost:3000/advertisement/ -> HTTP ERROR 404 // is the bug

but is the same path

expected

GET http://localhost:3000/advertisement/ -> .route("/", get(advertisement))

pauliyobo commented 1 year ago

Hello. Is it a bug, though? By looking at the code such as:

https://github.com/tokio-rs/axum/blob/8854e660e9ab07404e5bb8e30b92311d3848de05/axum/src/routing/tests/mod.rs?#L319-L330

Or

https://github.com/tokio-rs/axum/blob/8854e660e9ab07404e5bb8e30b92311d3848de05/axum/src/routing/path_router.rs?#L479-L490

it would seem that this behaviour is actually deliberate. I might be wrong though and please correct me if so.

mortifia commented 1 year ago

what the RFC says is that if a route with the / at the end is not clearly defined it will match the route without the / at the end

(implicit rule added by the crawler if I understood correctly)

https://www.rfc-editor.org/rfc/rfc3986

Conner-PYS commented 1 year ago

I encountered this same behavior while testing my server; though, I don't use any nested routers. From what I could find in other issues and discussions was that this behavior is intended.

As a work-around in my implementation, I have used tower_http's NormalizePath layer, which has an implementation to strip trailing slashes. Docs are at https://docs.rs/tower-http/latest/tower_http/normalize_path/index.html.

kekorder commented 1 year ago

I find this behaviour odd, if its a route with only a slash it ignores that slash for the route, but if it has has /test/ for example it forces the trailing slash, while I also want the /nest/ to force a trailing slash.

use axum::{routing::get, Router};

#[tokio::main]
async fn main() {
    let nest_routes = Router::new()
        .route("/", get(|| async { "http://127.0.0.1/nest/\n" })) // both http://127.0.0.1/nest (only works without a trailing slash)
        .route("/test/", get(|| async { "http://127.0.0.1/nest/test/\n" })); // both http://127.0.0.1/nest/test/ (only works with a traling slash)

    let app = Router::new()
        .route("/", get(|| async { "http://127.0.0.1/\n" })) // both http://127.0.0.1 and http://127.0.0.1/
        .route("/test/", get(|| async { "http://127.0.0.1/test/\n" })) // http://127.0.0.1/test/ (only works with a trailing slash)
        .nest("/nest", nest_routes);
    axum::Server::bind(&"127.0.0.1:8080".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}
rdbo commented 9 months ago

I just came across the same issue as well. I thought that by nesting /:lang and adding the route /, it would match /:lang/, but instead, it matches /:lang only. To fix it, I had to match /:lang/ and add my routes / and /login, but that makes me think it would match /:lang// /:lang//login, which isn't the case Just telling my experience and also saying that I expected the same result as OP.

Rodion-Bozhenko commented 9 months ago

Hey everyone, I also stumbled upon an with this issue. The Axum docs suggest a workaround using middleware to rewrite the request URI https://docs.rs/axum/latest/axum/middleware/index.html#rewriting-request-uri-in-middleware, so I came up with this function to remove the trailing slash before the request hits the router:

fn rewrite_request_uri<B>(mut req: Request<B>) -> Request<B> {
        let uri = req.uri().clone();
        let path = uri.path();

        if path.ends_with('/') && !path.ends_with("//") && path.len() > 1 {
            let new_path = path.trim_end_matches('/');
            let new_uri = format!(
                "{}{}",
                new_path,
                uri.query().map_or(String::from(""), |q| format!("?{}", q))
            );

            *req.uri_mut() = new_uri.parse().expect("Failed to parse new URI");
        }

        req
    }

Note !path.ends_with("//") condition which prevents rewriting urls with multiple slashes. If you need to strip away all trailing slashes remove this part.

I'm not entirely sure if this is the best approach, but it's been working well for me. If anyone has suggestions for improvement or alternative methods, I'd love to hear them.

jplatte commented 8 months ago

It took me a while to understand this bug report. It is actually intended behavior, there's some previous discussion about this here. I'm going to close this as it's framed as a bug report while the behavior is actually intentional, but have created #2659 as a point of discussion about changing this behavior.

mladedav commented 8 months ago

First, there is route_with_tsr in axum-extra which adds redirects between the , so this could be of interest?

There used to be automatic redirects in axum but they were removed in 0.6 #1119

RFC 3386 was mentioned, but I read it as saying that the two paths with and without a trailing slash should be considered equivalent only if the server hints that by e.g. redirecting from one to the other.

RFC excerpt

``` Substantial effort to reduce the incidence of false negatives is often cost-effective for web spiders. Therefore, they implement even more aggressive techniques in URI comparison. For example, if they observe that a URI such as http://example.com/data redirects to a URI differing only in the trailing slash http://example.com/data/ they will likely regard the two as equivalent in the future. This kind of technique is only appropriate when equivalence is clearly indicated by both the result of accessing the resources and the common conventions of their scheme's dereference algorithm (in this case, use of redirection by HTTP origin servers to avoid problems with relative references). ```

So I think what axum does now is correct and it should not create multiple paths on its own unless instructed to do so.