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

bug with / in nest #2267

Closed
1 task done
mortifia opened this issue Oct 11, 2023 · 8 comments
Closed
1 task done

bug with / in nest #2267

mortifia opened this issue Oct 11, 2023 · 8 comments
Labels
A-axum C-bug Category: This is a bug.
Milestone

Comments

@mortifia
Copy link

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

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))

@jplatte jplatte added C-bug Category: This is a bug. A-axum labels Oct 11, 2023
@pauliyobo
Copy link

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

#[crate::test]
async fn not_found_for_extra_trailing_slash() {
let app = Router::new().route("/foo", get(|| async {}));
let client = TestClient::new(app);
let res = client.get("/foo/").send().await;
assert_eq!(res.status(), StatusCode::NOT_FOUND);
let res = client.get("/foo").send().await;
assert_eq!(res.status(), StatusCode::OK);
}

Or

pub(crate) fn path_for_nested_route<'a>(prefix: &'a str, path: &'a str) -> Cow<'a, str> {
debug_assert!(prefix.starts_with('/'));
debug_assert!(path.starts_with('/'));
if prefix.ends_with('/') {
format!("{prefix}{}", path.trim_start_matches('/')).into()
} else if path == "/" {
prefix.into()
} else {
format!("{prefix}{path}").into()
}
}

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

@mortifia
Copy link
Author

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
Copy link

Conner-PYS commented Nov 1, 2023

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
Copy link

kekorder commented Nov 3, 2023

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
Copy link
Contributor

rdbo commented Jan 26, 2024

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
Copy link

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
Copy link
Member

jplatte commented Mar 17, 2024

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.

@jplatte jplatte closed this as not planned Won't fix, can't repro, duplicate, stale Mar 17, 2024
@mladedav
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

8 participants