Use 308 instead of 301 for trailing slash redirects (#682)

* Use 308 status instead of 301 when redirecting For redirects resulting from requests to paths with a trailing slash, use 308 instead of 301 to prevent non-GET requests (POST, PUT, etc) from being changed to GET. For example, (assuming a route for /path is defined)... - Old behavior results in: POST /path/ -> GET /path - New behavior results in: POST /path/ -> POST /path Fixes #681 * Add deprecation notice to found() Deprecates found() due to its use of HTTP 302 * rustfmt * Use dedicated redirect method Use Redirect::permanent instead of re-implementing its functionality * Remove deprecated method from example Replace usages of Redirect:found with Redirect::to and Redirect::temporary as appropriate * Fix panic in oauth example Previously the example would panic if a request was made without the `Cookie` header. Now the user is redirected to the login page as expected. * Update CHANGELOG * Revert pub TypedheaderRejection fields * Fix clippy lint * cargo fmt * Fix CHANGELOG link * Adhere to implicit line length limit
tokio-rs · Jan 12, 2022 · 007a0e8 · 007a0e8
1 parent 5512ebc
commit 007a0e8
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 34 deletions.
diff --git a/axum/CHANGELOG.md b/axum/CHANGELOG.md
@@ -9,8 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - **fixed:** Fix using incorrect path prefix when nesting `Router`s at `/` ([#691])
 - **fixed:** Make `nest("", service)` work and mean the same as `nest("/", service)` ([#691])
+- **fixed:** Replace response code `301` with `308` for trailing slash redirects. Also deprecates
+  `Redirect::found` (`302`) in favor of `Redirect::temporary` (`307`) or `Redirect::to` (`303`).
+  This is to prevent clients from changing non-`GET` requests to `GET` requests ([#682])
 
 [#691]: https://github.com/tokio-rs/axum/pull/691
+[#682]: https://github.com/tokio-rs/axum/pull/682
 
 # 0.4.3 (21. December, 2021)
 

diff --git a/axum/src/response/redirect.rs b/axum/src/response/redirect.rs
@@ -33,7 +33,8 @@ impl Redirect {
     /// This redirect instructs the client to change the method to GET for the subsequent request
     /// to the given `uri`, which is useful after successful form submission, file upload or when
     /// you generally don't want the redirected-to page to observe the original request method and
-    /// body (if non-empty).
+    /// body (if non-empty). If you want to preserve the request method and body,
+    /// [`Redirect::temporary`] should be used instead.
     ///
     /// # Panics
     ///
@@ -71,16 +72,21 @@ impl Redirect {
 
     /// Create a new [`Redirect`] that uses a [`302 Found`][mdn] status code.
     ///
-    /// This is the same as [`Redirect::temporary`], except the status code is older and thus
-    /// supported by some legacy applications that doesn't understand the newer one, but some of
-    /// those applications wrongly apply [`Redirect::to`] (`303 See Other`) semantics for this
-    /// status code. It should be avoided where possible.
+    /// This is the same as [`Redirect::temporary`] ([`307 Temporary Redirect`][mdn307]) except
+    /// this status code is older and thus supported by some legacy clients that don't understand
+    /// the newer one. Many clients wrongly apply [`Redirect::to`] ([`303 See Other`][mdn303])
+    /// semantics for this status code, so it should be avoided where possible.
     ///
     /// # Panics
     ///
     /// If `uri` isn't a valid [`HeaderValue`].
     ///
     /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/302
+    /// [mdn307]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307
+    /// [mdn303]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303
+    #[deprecated(
+        note = "This results in different behavior between clients, so Redirect::temporary or Redirect::to should be used instead"
+    )]
     pub fn found(uri: Uri) -> Self {
         Self::with_status_code(StatusCode::FOUND, uri)
     }

diff --git a/axum/src/routing/mod.rs b/axum/src/routing/mod.rs
@@ -7,12 +7,14 @@ use crate::{
         connect_info::{Connected, IntoMakeServiceWithConnectInfo},
         MatchedPath, OriginalUri,
     },
+    response::IntoResponse,
+    response::Redirect,
     response::Response,
     routing::strip_prefix::StripPrefix,
     util::{try_downcast, ByteStr, PercentDecodedByteStr},
     BoxError,
 };
-use http::{Request, StatusCode, Uri};
+use http::{Request, Uri};
 use std::{
     borrow::Cow,
     collections::HashMap,
@@ -490,12 +492,8 @@ where
                     } else {
                         with_path(req.uri(), &format!("{}/", path))
                     };
-                    let res = Response::builder()
-                        .status(StatusCode::MOVED_PERMANENTLY)
-                        .header(http::header::LOCATION, redirect_to.to_string())
-                        .body(crate::body::empty())
-                        .unwrap();
-                    RouterFuture::from_response(res)
+                    let res = Redirect::permanent(redirect_to);
+                    RouterFuture::from_response(res.into_response())
                 } else {
                     match &self.fallback {
                         Fallback::Default(inner) => {

diff --git a/axum/src/routing/tests/mod.rs b/axum/src/routing/tests/mod.rs
@@ -354,6 +354,30 @@ async fn with_and_without_trailing_slash() {
     assert_eq!(res.text().await, "without tsr");
 }
 
+// for https://github.com/tokio-rs/axum/issues/681
+#[tokio::test]
+async fn with_trailing_slash_post() {
+    let app = Router::new().route("/foo", post(|| async {}));
+
+    let client = TestClient::new(app);
+
+    // `TestClient` automatically follows redirects
+    let res = client.post("/foo/").send().await;
+    assert_eq!(res.status(), StatusCode::OK);
+}
+
+// for https://github.com/tokio-rs/axum/issues/681
+#[tokio::test]
+async fn without_trailing_slash_post() {
+    let app = Router::new().route("/foo/", post(|| async {}));
+
+    let client = TestClient::new(app);
+
+    // `TestClient` automatically follows redirects
+    let res = client.post("/foo").send().await;
+    assert_eq!(res.status(), StatusCode::OK);
+}
+
 // for https://github.com/tokio-rs/axum/issues/420
 #[tokio::test]
 async fn wildcard_with_trailing_slash() {
@@ -381,7 +405,7 @@ async fn wildcard_with_trailing_slash() {
         )
         .await
         .unwrap();
-    assert_eq!(res.status(), StatusCode::MOVED_PERMANENTLY);
+    assert_eq!(res.status(), StatusCode::PERMANENT_REDIRECT);
     assert_eq!(res.headers()["location"], "/user1/repo1/tree/");
 
     // check that the params are deserialized correctly

diff --git a/examples/oauth/Cargo.toml b/examples/oauth/Cargo.toml
@@ -15,3 +15,4 @@ serde = { version = "1.0", features = ["derive"] }
 # Use Rustls because it makes it easier to cross-compile on CI
 reqwest = { version = "0.11", default-features = false, features = ["rustls-tls", "json"] }
 headers = "0.3"
+http = "0.2"
diff --git a/examples/oauth/src/main.rs b/examples/oauth/src/main.rs
@@ -1,34 +1,33 @@
 //! Example OAuth (Discord) implementation.
 //!
-//! Run with
-//!
+//! 1) Create a new application at <https://discord.com/developers/applications>
+//! 2) Visit the OAuth2 tab to get your CLIENT_ID and CLIENT_SECRET
+//! 3) Add a new redirect URI (for this example: `http://127.0.0.1:3000/auth/authorized`)
+//! 4) Run with the following (replacing values appropriately):
 //! ```not_rust
-//! CLIENT_ID=123 CLIENT_SECRET=secret cargo run -p example-oauth
+//! CLIENT_ID=REPLACE_ME CLIENT_SECRET=REPLACE_ME cargo run -p example-oauth
 //! ```
 
 use async_session::{MemoryStore, Session, SessionStore};
 use axum::{
     async_trait,
-    extract::{Extension, FromRequest, Query, RequestParts, TypedHeader},
+    extract::{
+        rejection::TypedHeaderRejectionReason, Extension, FromRequest, Query, RequestParts,
+        TypedHeader,
+    },
     http::{header::SET_COOKIE, HeaderMap},
     response::{IntoResponse, Redirect, Response},
     routing::get,
     AddExtensionLayer, Router,
 };
+use http::header;
 use oauth2::{
     basic::BasicClient, reqwest::async_http_client, AuthUrl, AuthorizationCode, ClientId,
     ClientSecret, CsrfToken, RedirectUrl, Scope, TokenResponse, TokenUrl,
 };
 use serde::{Deserialize, Serialize};
 use std::{env, net::SocketAddr};
 
-// Quick instructions:
-// 1) create a new application at https://discord.com/developers/applications
-// 2) visit the OAuth2 tab to get your CLIENT_ID and CLIENT_SECRET
-// 3) add a new redirect URI (For this example: http://localhost:3000/auth/authorized)
-// 4) AUTH_URL and TOKEN_URL may stay the same for discord.
-// More information: https://discord.com/developers/applications/792730475856527411/oauth2
-
 static COOKIE_NAME: &str = "SESSION";
 
 #[tokio::main]
@@ -39,7 +38,7 @@ async fn main() {
     }
     tracing_subscriber::fmt::init();
 
-    // `MemoryStore` just used as an example. Don't use this in production.
+    // `MemoryStore` is just used as an example. Don't use this in production.
     let store = MemoryStore::new();
 
     let oauth_client = oauth_client();
@@ -64,8 +63,8 @@ async fn main() {
 
 fn oauth_client() -> BasicClient {
     // Environment variables (* = required):
-    // *"CLIENT_ID"     "123456789123456789";
-    // *"CLIENT_SECRET" "rAn60Mch4ra-CTErsSf-r04utHcLienT";
+    // *"CLIENT_ID"     "REPLACE_ME";
+    // *"CLIENT_SECRET" "REPLACE_ME";
     //  "REDIRECT_URL"  "http://127.0.0.1:3000/auth/authorized";
     //  "AUTH_URL"      "https://discord.com/api/oauth2/authorize?response_type=code";
     //  "TOKEN_URL"     "https://discord.com/api/oauth2/token";
@@ -119,7 +118,7 @@ async fn discord_auth(Extension(client): Extension<BasicClient>) -> impl IntoRes
         .url();
 
     // Redirect to Discord's oauth service
-    Redirect::found(auth_url.to_string().parse().unwrap())
+    Redirect::to(auth_url.to_string().parse().unwrap())
 }
 
 // Valid user session required. If there is none, redirect to the auth page
@@ -138,12 +137,12 @@ async fn logout(
     let session = match store.load_session(cookie.to_string()).await.unwrap() {
         Some(s) => s,
         // No session active, just redirect
-        None => return Redirect::found("/".parse().unwrap()),
+        None => return Redirect::to("/".parse().unwrap()),
     };
 
     store.destroy_session(session).await.unwrap();
 
-    Redirect::found("/".parse().unwrap())
+    Redirect::to("/".parse().unwrap())
 }
 
 #[derive(Debug, Deserialize)]
@@ -192,14 +191,14 @@ async fn login_authorized(
     let mut headers = HeaderMap::new();
     headers.insert(SET_COOKIE, cookie.parse().unwrap());
 
-    (headers, Redirect::found("/".parse().unwrap()))
+    (headers, Redirect::to("/".parse().unwrap()))
 }
 
 struct AuthRedirect;
 
 impl IntoResponse for AuthRedirect {
     fn into_response(self) -> Response {
-        Redirect::found("/auth/discord".parse().unwrap()).into_response()
+        Redirect::temporary("/auth/discord".parse().unwrap()).into_response()
     }
 }
 
@@ -218,8 +217,13 @@ where
 
         let cookies = TypedHeader::<headers::Cookie>::from_request(req)
             .await
-            .expect("could not get cookies");
-
+            .map_err(|e| match *e.name() {
+                header::COOKIE => match e.reason() {
+                    TypedHeaderRejectionReason::Missing => AuthRedirect,
+                    _ => panic!("unexpected error getting Cookie header(s): {}", e),
+                },
+                _ => panic!("unexpected error getting cookies: {}", e),
+            })?;
         let session_cookie = cookies.get(COOKIE_NAME).ok_or(AuthRedirect)?;
 
         let session = store