Implement CSRF token verification for OAuth example #2534
Conversation
loganbnielsen
force-pushed
the
add-csrf-verification
branch
from
e98be93
to
87b9b91
Compare
examples/oauth/src/main.rs
Outdated
| // Create session to store csrf_token | ||
| let mut session = Session::new(); | ||
| session | ||
| .insert("csrf_token", &csrf_token) |
There was a problem hiding this comment.
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.
examples/oauth/src/main.rs
Outdated
| .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.
The cookie security can be improved setting HttpOnly and Secure.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@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.
All good for the HttpOnly and Secure flags. This comment can be resolved.
|
Just wanted to follow up if there were any changes expected before this can be merged. I think there's some nit improvements which can be made which is understandable. This example (along with others already merged) is an MVP to set up security and probably doesn't need to be industry ready. As long as there are no fundamental security mistakes, I think merging it would be best with potential enhancements provided in subsequent PRs being encouraged We can link the Leptos OAUTH example so users have 2 implementations they can check review before implementing into their own app |
|
Noticed that this has been open for almost a year. Lmk if I can do anything to make this mergeable |
examples/oauth/src/main.rs
Outdated
| .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.
@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.
loganbnielsen
force-pushed
the
add-csrf-verification
branch
from
d414e8b
to
b281a19
Compare
|
updated |
|
Thanks. I'll just try to run it tomorrow when I get to a PC and if there aren't any issues, I'll merge it. |
|
Sounds good, I sometimes hit a race condition when I run it where the token read attempt occurs before the write has happened so you might need to try a second time if you get that error (I think exponential back off for second would fix this but feels out of scope for a minimal example) |
Addresses #2511