Use 308 status instead of 301 when redirecting #682

nashley · 2022-01-01T02:57:12Z

Motivation

Assuming a route for /path is defined,
POST /path/ currently may redirect to GET /path when it should redirect to POST /path (depending on the client).

This was outlined in #681.

This has potential security implications (e.g., if the POST contained sensitive data, that data may be saved by the user's client in their browser history and/or in the server's access logs).

It's worth noting that the internal test client, a wrapper for the popular reqwest client, is susceptible to this incorrect behavior.

Solution

This PR replaces the 301 redirect with a 308 and adds corresponding tests to verify the behavior (note that these tests fail without the accompanying code change).

Determine if this counts as a breaking change
- if so, this should be rebased onto axum-next
- if not (or regardless), an entry in CHANGELOG.md should be added

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 tokio-rs#681

nashley · 2022-01-01T02:58:24Z

Should this PR be against main or axum-next? The behavior is changing in a way that could be impactful [1][2], but I'd argue this fits more in line with a bugfix, so I went ahead and made it against main.

[1] e.g., if a server had a different route defined for the GET requests and the application somehow depended on the broken redirect functionality sending requests with a trailing / to the GET handler instead of the typically-desired handler
[2] Some clients (circa 2012) only support 301 and not 308 (e.g., Internet Explorer before version 11, Firefox before 14, Chrome before 36, etc). See caniuse for more info. Such clients would stop following trailing slash redirects properly.

nashley · 2022-01-01T03:45:20Z

Converted to draft while I add deprecation attributes to methods that return ambiguous response codes (viz., 301 and 302)

Deprecates found() due to its use of HTTP 302

Use Redirect::permanent instead of re-implementing its functionality

nashley · 2022-01-01T05:53:00Z

It looks like there's currently no built-in method for 301 redirects -- just 302. I refrained from adding that in this PR (since it seems odd to add a new method that's immediately deprecated), but I can do so if desired.

Replace usages of Redirect:found with Redirect::to and Redirect::temporary as appropriate

nashley · 2022-01-01T06:38:43Z

I'm not yet marking this as ready for review, since I haven't tested examples/oauth/src/main.rs after making changes to it (though I expect it will work without a hitch).

ttys3 · 2022-01-02T15:09:51Z

if we change 301 -> 308
I think leave 302 alone is strange. maybe we also need change 302 to 307

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307

The only difference between 307 and 302 is that 307 guarantees that the method and the body will not be changed when the redirected request is made. With 302, some old clients were incorrectly changing the method to GET: the behavior with non-GET methods and 302 is then unpredictable on the Web, whereas the behavior with 307 is predictable. For GET requests, their behavior is identical.

nashley · 2022-01-02T16:18:30Z

@ttys3 I agree. I had already marked Redirect::found as deprecated and updated the examples to reflect that. Did I miss any other instances of 302?
> rg StatusCode::FOUND -B 4 yields only the aforementioned hit:

axum/src/response/redirect.rs
87-    #[deprecated(
88-        note = "This results in different behavior between clients, so Redirect::temporary or Redirect::to should be used instead"
89-    )]
90-    pub fn found(uri: Uri) -> Self {
91:        Self::with_status_code(StatusCode::FOUND, uri)

davidpdrsn

Thanks for fixing this!

IMO its not a breaking change so doesn't have to merged into axum-next.

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.

nashley · 2022-01-04T04:48:24Z

I verified that the example still works and made some small improvements along the way (though I had to expose some previously-private fields to nicely destructure an existing error when requests without a Cookie header were made).

I'd like confirmation that exposing TypedHeaderRejection's fields directly instead of via accessor methods isn't a bad idea for some reason I can't foresee. But otherwise I think this is ready.

nashley · 2022-01-04T04:59:26Z

examples/oauth/src/main.rs

+impl From<TypedHeaderRejection> for AuthRedirect {
+    fn from(error: TypedHeaderRejection) -> Self {
+        match error {
+            TypedHeaderRejection {
+                name: &header::COOKIE,
+                reason: TypedHeaderRejectionReason::Missing,
+            } => AuthRedirect,
+            _ => panic!("unexpected error getting Cookie header(s)"),
+        }
    }
 }


I'm not hugely fond of this, but I couldn't think of a better way to do it without introducing a wrapping type or using anyhow in a way I'm not familiar with. Suggestions welcome.

I'm not very familiar with this example but the fields should stay private. Users should not be able to construct this type directly or mutate the fields.

davidpdrsn

The fields of TypedHeaderRejection should stay private.

nashley · 2022-01-12T06:25:58Z

Apologies for all of the extra commits. I'm not squashing to abide by the contributing guidelines.

davidpdrsn

LGMT! Thanks 😃

davidpdrsn · 2022-01-12T14:13:28Z

Apologies for all of the extra commits. I'm not squashing to abide by the contributing guidelines.

Appreciated! Makes reviewing changes since my last review much easier.

jplatte · 2022-01-12T14:17:11Z

Appreciated! Makes reviewing changes since my last review much easier.

You know GitHub can also render force-push diffs though, right? (need to click the text "force-pushed" in the comments view)

davidpdrsn · 2022-01-12T14:43:01Z

Maybe 🤷 I remember not being able to figure it out

jplatte · 2022-01-12T14:45:32Z

It is definitely weird, but it works (just requires a few extra clicks because the notification will lead you to a "we couldn't find these commits" page). Example: ruma/ruma#796 (comment)

davidpdrsn · 2022-01-12T14:47:20Z

omg must subtle link ever! No wonder I never noticed 🧐

- **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]: #691 [#682]: #682

nashley marked this pull request as draft January 1, 2022 03:38

Nick Ashley added 3 commits January 1, 2022 04:49

Add deprecation notice to found()

ef54b7b

Deprecates found() due to its use of HTTP 302

rustfmt

977eb94

Use dedicated redirect method

1dade55

Use Redirect::permanent instead of re-implementing its functionality

Remove deprecated method from example

06432b7

Replace usages of Redirect:found with Redirect::to and Redirect::temporary as appropriate

davidpdrsn approved these changes Jan 3, 2022

View reviewed changes

Fix panic in oauth example

b6da3d6

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.

nashley marked this pull request as ready for review January 4, 2022 04:48

nashley requested a review from davidpdrsn January 4, 2022 04:51

nashley commented Jan 4, 2022

View reviewed changes

Update CHANGELOG

d20e22b

davidpdrsn requested changes Jan 4, 2022

View reviewed changes

Nick Ashley added 5 commits January 12, 2022 03:57

Merge in main

3dab929

Revert pub TypedheaderRejection fields

21360fc

Fix clippy lint

91716cd

cargo fmt

64f1655

Fix CHANGELOG link

5d16e55

Adhere to implicit line length limit

6b1070f

nashley requested a review from davidpdrsn January 12, 2022 06:36

davidpdrsn approved these changes Jan 12, 2022

View reviewed changes

davidpdrsn merged commit 007a0e8 into tokio-rs:main Jan 12, 2022

nashley deleted the use-308-for-tsr branch January 12, 2022 14:58

davidpdrsn mentioned this pull request Jan 13, 2022

axum: Version 0.4.4 #706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use 308 status instead of 301 when redirecting #682

Use 308 status instead of 301 when redirecting #682

nashley commented Jan 1, 2022 •

edited

Loading

nashley commented Jan 1, 2022 •

edited

Loading

nashley commented Jan 1, 2022

nashley commented Jan 1, 2022 •

edited

Loading

nashley commented Jan 1, 2022

ttys3 commented Jan 2, 2022 •

edited

Loading

nashley commented Jan 2, 2022 •

edited

Loading

davidpdrsn left a comment

nashley commented Jan 4, 2022

nashley Jan 4, 2022

davidpdrsn Jan 4, 2022 •

edited

Loading

davidpdrsn left a comment

nashley commented Jan 12, 2022

davidpdrsn left a comment

davidpdrsn commented Jan 12, 2022

jplatte commented Jan 12, 2022

davidpdrsn commented Jan 12, 2022

jplatte commented Jan 12, 2022 •

edited

Loading

davidpdrsn commented Jan 12, 2022

Use 308 status instead of 301 when redirecting #682

Use 308 status instead of 301 when redirecting #682

Conversation

nashley commented Jan 1, 2022 • edited Loading

Motivation

Solution

nashley commented Jan 1, 2022 • edited Loading

nashley commented Jan 1, 2022

nashley commented Jan 1, 2022 • edited Loading

nashley commented Jan 1, 2022

ttys3 commented Jan 2, 2022 • edited Loading

nashley commented Jan 2, 2022 • edited Loading

davidpdrsn left a comment

Choose a reason for hiding this comment

nashley commented Jan 4, 2022

nashley Jan 4, 2022

Choose a reason for hiding this comment

davidpdrsn Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

davidpdrsn left a comment

Choose a reason for hiding this comment

nashley commented Jan 12, 2022

davidpdrsn left a comment

Choose a reason for hiding this comment

davidpdrsn commented Jan 12, 2022

jplatte commented Jan 12, 2022

davidpdrsn commented Jan 12, 2022

jplatte commented Jan 12, 2022 • edited Loading

davidpdrsn commented Jan 12, 2022

nashley commented Jan 1, 2022 •

edited

Loading

nashley commented Jan 1, 2022 •

edited

Loading

nashley commented Jan 1, 2022 •

edited

Loading

ttys3 commented Jan 2, 2022 •

edited

Loading

nashley commented Jan 2, 2022 •

edited

Loading

davidpdrsn Jan 4, 2022 •

edited

Loading

jplatte commented Jan 12, 2022 •

edited

Loading