Middleware that return early break Router::or

[Flowneee]

Bug Report

Version

├── axum v0.2.8

Platform

Linux fdr12.tcsbank.ru 5.14.9-200.fc34.x86_64 #1 SMP Thu Sep 30 11:55:35 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Description

When I try to split app into multiple Routers (like in docs https://docs.rs/axum/0.2.8/axum/#to-groups-of-routes), layers from "leaf" Routers affect other "leaf" Routers.

I tried this code:

use axum::{
    handler::{get, post, Handler},
    response::{Html, IntoResponse},
    Router,
};
use hyper::StatusCode;
use std::net::SocketAddr;
use tower_http::auth::RequireAuthorizationLayer;

async fn handler_404() -> impl IntoResponse {
    StatusCode::NOT_FOUND
}

#[tokio::main]
async fn main() {
    let foo = Router::new()
        .route("/", get(|| async {}))
        .route("/foo", post(|| async {}));

    let bar = Router::new()
        .route("/requires-auth", get(|| async {}))
        .layer(RequireAuthorizationLayer::basic("a", "a"));

    let app = bar.or(foo).or(handler_404.into_service());  # NOTE: bar is first, not foo

    let addr = "0.0.0.0:8080".parse::<SocketAddr>().unwrap();
    axum::Server::try_bind(&addr)
        .unwrap()
        .serve(app.into_make_service())
        .await;
}

I expected to see this happen: when I do curl localhost:8080/, I get 200 OK

Instead, this happened: 401 Unauthorized.

Basically now all routes require auth from bar "leaf" Router.

If I apply auth layer to handler directly, everything works, but this mean that I cannot apply layer to group of routes, only to all of them at once.

I think that layers should apply only to current Router, at least this is how I understand current documentation.

[davidpdrsn]

I believe this is the same issue as #348 and is actually a bug with or. Unfortunately we don't have a solution for it yet. The workaround described here should work.

[Flowneee]

#363 won't fix this problem?

[Flowneee]

Don't you think that current documentation is misleading then?

[davidpdrsn]

#363 won't fix this problem?

I don't believe so.

Don't you think that current documentation is misleading then?

You're right, we probably shouldn't use the particular example we do since it wont work.

[Flowneee]

I looked at #363, and (if I understood correctly), now it will

1. get RouteId by URL;
2. call router as it was before, but route will be selected by RouteId, not by URL.

If so, I think it is possible to somehow store IDs of routes in Router itself, and before call Router's inners (when handling new request), check, whether Router have current ID or not, so we could skip all Routers but the one we need. Do you think it is viable solution for axum?

[davidpdrsn]

I'm not sure. Made a note to look into it. You're welcome to test it if you want.

[davidpdrsn]

I've tweaked the title a bit to make it more clear what the issue is. I think this is something we should attempt to fix for 0.3.

[oberzs]

the same problem is encountered in this code. /ping and /version endpoints work fine without authorization, but the nested /public route does not (was asked to post this here from the tokio Discord):

let no_auth_routes = Router::new()
    .nest(
        "/public",
        service::get(ServeDir::new("./public")).handle_error(|_| {
            (
                StatusCode::INTERNAL_SERVER_ERROR,
                "Unhandled internal error",
            )
        }),
    )
    .route("/ping", get(get_ping))
    .route("/version", get(get_version));

let auth_routes = Router::new()
    .route("/auth", get(get_auth))
    // the rest of authorized routes
    .layer(RequireAuthorizationLayer::bearer("********"));

let app = no_auth_routes.or(auth_routes);

[davidpdrsn]

This was fixed in #408

[davidpdrsn]

Turns out I was too quick on this one. Fixing this requires doing routing first, before running any middleware. Because if we run the middleware first they might return early and hijack something that would otherwise be a 404. However by running middleware after routing we break other middleware like logging which expect to be called even for 404s... So regardless which path we break something.

I think the best compromise is:

• Go back to the way it worked in 0.2, which means logging middleware will continue to work
• Require users to structure their routers like shown here to workaround this issue
• Or add auth middleware to the handlers individually that need it

I feel this is the best approach for now. Maybe we'll come up with a general solution in the future.

See #422 for more details.

[Flowneee]

I think renaming or to merge kind of solves problem, because (at least for me) semantically mergeis more like concatenation of two lists, and that is what merge do exactly - it make single list from 2 lists of Servicees (which Router is roughly), and or is more like creating tree-like structure. nest is also creating tree IMO, which was slightly confusing.

But I think it would be cool to have something like nest, but with non-unique prefix, so you can mount 2 Routers under single prefix and processing request would be like this:

• check routes in first Router. If successful - proceed with first Router, if not - next step;
• check routes in second Router. If successful - proceed with second Router, if not - repeat until you run out of Routers with current prefix;
• if none matched, proceed as 404 or whatever default route is.

This will allow to create tree-like structure of Routers, where layers can be applied only to certain routes, which do not share common prefix.

[davidpdrsn]

How is that different from .nest("/foo", router_a.merge(router_b))?

[Flowneee]

Layers from both routers will mix again. What I think is something like

let router_a = Router::new().route("/metrics", ...).route("/health", ...);
let router_b = Router::new().route("/shutdown", ...).route("/kill", ...).layer(SomeAuthLayer {});

let router = Route::new().nest("/manage", router_a).nest("/manage", router_b);

Ofc there is always option to apply layers to handlers directly, even though it is verbose.

[davidpdrsn]

Layers from both routers will mix again

What do mean by this? Layers only apply to routes added to the router at the time the layer is added.

[Flowneee]

Seems I got it wrong. If replace or with merge, on new axum version code from first message works as expected

use axum::{
    handler::Handler,
    response::IntoResponse,
    routing::{get, post},
    Router,
};
use hyper::StatusCode;
use std::net::SocketAddr;
use tower_http::auth::RequireAuthorizationLayer;

async fn handler_404() -> impl IntoResponse {
    StatusCode::NOT_FOUND
}

#[tokio::main]
async fn main() {
    let foo = Router::new()
        .route("/", get(|| async {}))
        .route("/foo", post(|| async {}));

    let bar = Router::new()
        .route("/requires-auth", get(|| async {}))
        .layer(RequireAuthorizationLayer::basic("a", "a"));

    let app = bar.merge(foo).fallback(handler_404.into_service()); //NOTE: bar is first, not foo

    let addr = "0.0.0.0:8080".parse::<SocketAddr>().unwrap();
    axum::Server::try_bind(&addr)
        .unwrap()
        .serve(app.into_make_service())
        .await;
}

Thanks!

[davidpdrsn]

A proper fix for this was released in 0.3.2: Route::route_layer which applies a middleware that only runs if the request matches a route. This means middleware that return early wont hijack 404s.