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

Add OAuthStateManager for CSRF protection in GitHub OAuth flow #10702

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jNullj
Copy link
Contributor

@jNullj jNullj commented Nov 23, 2024

Introduce an OAuthStateManager class to manage states for CSRF protection during GitHub OAuth authentication, addressing issue #1805, This implementation generates and validates states to enhance security against CSRF attacks.

this pr is public as its an open public issue.
draft as i didn't test yet, and i need to add automatic tests as well

Copy link
Contributor

Warnings
⚠️

📚 Remember to ensure any changes to config.private in services/github/auth/acceptor.js are reflected in the server secrets documentation

Messages
📖 ✨ Thanks for your contribution to Shields, @jNullj!

Generated by 🚫 dangerJS against 18cd5a2

@jNullj jNullj added core Server, BaseService, GitHub auth, Shared helpers security Refer to our SECURITY.md policy before opening pull requests that address a security vulnerability labels Nov 23, 2024
@chris48s
Copy link
Member

I haven't really looked at this in much detail. Tbh, I've not 100% got my head round the attack vector here. However, there's one thing I want to flag quickly here before we go any further with this..

  • OAuthStateManager.states is a Map() that lives in memory on one server
  • In production, shields runs on lots of servers (at minimum 14)
  • If you assume that the server that processes the request to /github-auth and the server that processes the request to /github-auth/done will be 2 different servers (there is a >90% chance this will be true in production), does this approach work?

It looks to me like it wouldn't. i.e: if the computer that does the .set() isn't the same computer that later tries to do the .get() this check will always fail, right?

@jNullj
Copy link
Contributor Author

jNullj commented Nov 23, 2024

That's current, I wrongly assumed here we run on one instance, its easy to forget as I'm used to debug on a single container and usually write single instance applications.

I tried to think of a way to sync this info that we already used, github api tokens came to mind as they should also be persistent across servers, and i noticed we used postgresSQL, do you think using it is a viable strategy?

I would love to ditch persistent storage for keys altogether but then an attacker could forge a valid key for the CSRF attack for all clients, do you think we could bind the CSRF token to something else? like IP address & user agent mix hash or something similar?

I think i might need a bit more reading about this, it appears many save time by picking a framework to handle csrf for them.

@chris48s
Copy link
Member

Before we get any further into this, I've invited you to a private thread to try and dig into what problem we're actually trying to solve here. I think I need to understand what problem are trying to solve before we can design a solution to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Server, BaseService, GitHub auth, Shared helpers security Refer to our SECURITY.md policy before opening pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants