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

Implement CSRF token verification for OAuth example #2534

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

loganbnielsen
Copy link

Addresses #2511

Copy link

@iggyzuk iggyzuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and it works, I used it in my project.

// Create session to store csrf_token
let mut session = Session::new();
session
.insert("csrf_token", &csrf_token)
Copy link

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.

.context("unexpected error retrieving CSRF cookie value")?;

// Attach the session cookie to the response header
let cookie = format!("{COOKIE_NAME}={cookie}; SameSite=Lax; Path=/");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cookie security can be improved setting HttpOnly and Secure.

Copy link

Choose a reason for hiding this comment

The 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 csrf_token_validation_workflow is unable to find cookie COOKIE_NAME. Note: I'm using Google OAuth2, not Discord.

Choose a reason for hiding this comment

The 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants