-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement CSRF token verification for OAuth example #2534
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ | |
//! CLIENT_ID=REPLACE_ME CLIENT_SECRET=REPLACE_ME cargo run -p example-oauth | ||
//! ``` | ||
|
||
use anyhow::{Context, Result}; | ||
use anyhow::{anyhow, Context, Result}; | ||
use async_session::{MemoryStore, Session, SessionStore}; | ||
use axum::{ | ||
async_trait, | ||
|
@@ -142,19 +142,37 @@ async fn index(user: Option<User>) -> impl IntoResponse { | |
} | ||
} | ||
|
||
async fn discord_auth(State(client): State<BasicClient>) -> impl IntoResponse { | ||
// TODO: this example currently doesn't validate the CSRF token during login attempts. That | ||
// makes it vulnerable to cross-site request forgery. If you copy code from this example make | ||
// sure to add a check for the CSRF token. | ||
// | ||
// Issue for adding check to this example https://github.com/tokio-rs/axum/issues/2511 | ||
let (auth_url, _csrf_token) = client | ||
async fn discord_auth( | ||
State(client): State<BasicClient>, | ||
State(store): State<MemoryStore>, | ||
) -> Result<impl IntoResponse, AppError> { | ||
let (auth_url, csrf_token) = client | ||
.authorize_url(CsrfToken::new_random) | ||
.add_scope(Scope::new("identify".to_string())) | ||
.url(); | ||
|
||
// Redirect to Discord's oauth service | ||
Redirect::to(auth_url.as_ref()) | ||
// Create session to store csrf_token | ||
let mut session = Session::new(); | ||
session | ||
.insert("csrf_token", &csrf_token) | ||
.context("failed in inserting CSRF token into session")?; | ||
|
||
// Store the session in MemoryStore and retrieve the session cookie | ||
let cookie = store | ||
mladedav marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.store_session(session) | ||
.await | ||
.context("failed to store CSRF token session")? | ||
.context("unexpected error retrieving CSRF cookie value")?; | ||
|
||
// Attach the session cookie to the response header | ||
let cookie = format!("{COOKIE_NAME}={cookie}; SameSite=Lax; Path=/"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cookie security can be improved setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my case setting SameSite to Strict breaks something "Application error: unexpected error getting cookie name". It happens when the login is authorized, so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, you are right. We can not use Strict here since the cookie is sent after a redirect. First comment updated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @loganbnielsen can you add the mentioned options? I get that we don't need to make the whole thing production-ready, but if these settings are more secure let's add them to the example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good for the |
||
let mut headers = HeaderMap::new(); | ||
headers.insert( | ||
SET_COOKIE, | ||
cookie.parse().context("failed to parse cookie")?, | ||
); | ||
|
||
Ok((headers, Redirect::to(auth_url.as_ref()))) | ||
} | ||
|
||
// Valid user session required. If there is none, redirect to the auth page | ||
|
@@ -195,11 +213,55 @@ struct AuthRequest { | |
state: String, | ||
} | ||
|
||
async fn csrf_token_validation_workflow( | ||
auth_request: &AuthRequest, | ||
cookies: &headers::Cookie, | ||
store: &MemoryStore, | ||
) -> Result<(), AppError> { | ||
// Extract the cookie from the request | ||
let cookie = cookies | ||
.get(COOKIE_NAME) | ||
.context("unexpected error getting cookie name")? | ||
.to_string(); | ||
|
||
// Load the session | ||
let session = match store | ||
.load_session(cookie) | ||
.await | ||
.context("failed to load session")? | ||
{ | ||
Some(session) => session, | ||
None => return Err(anyhow!("Session not found").into()), | ||
}; | ||
|
||
// Extract the CSRF token from the session | ||
let stored_csrf_token = session | ||
.get::<CsrfToken>("csrf_token") | ||
.context("CSRF token not found in session")? | ||
.to_owned(); | ||
|
||
// Cleanup the CSRF token session | ||
store | ||
.destroy_session(session) | ||
.await | ||
.context("Failed to destroy old session")?; | ||
|
||
// Validate CSRF token is the same as the one in the auth request | ||
if *stored_csrf_token.secret() != auth_request.state { | ||
return Err(anyhow!("CSRF token mismatch").into()); | ||
} | ||
|
||
Ok(()) | ||
} | ||
|
||
async fn login_authorized( | ||
Query(query): Query<AuthRequest>, | ||
State(store): State<MemoryStore>, | ||
State(oauth_client): State<BasicClient>, | ||
TypedHeader(cookies): TypedHeader<headers::Cookie>, | ||
) -> Result<impl IntoResponse, AppError> { | ||
csrf_token_validation_workflow(&query, &cookies, &store).await?; | ||
|
||
// Get an auth token | ||
let token = oauth_client | ||
.exchange_code(AuthorizationCode::new(query.code.clone())) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since "csrf_token" is used in two placed it might be good to define a constant value.
static CSRF_TOKEN: &str = "csrf_token";
similar to COOKIE_NAME. I suppose "user" also needs one.