-
-
Notifications
You must be signed in to change notification settings - Fork 343
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
this library is "insecure" to CSRF by default, security mechanisms confusing to use and enable #164
Comments
According to #117 (comment) this library is also abandoned... |
You are covering a lot of topics in this issue, and there's a lot of incorrect statements. To start with, this library is not abandoned. Next, you say that state support was added in #138, which is not entirely accurate. This library has long supported (I can't even remember how long)
#138 added more enhancements that allowed controlling of state on a per-route basis, rather than globally in the constructor, as described here. I can assure you this functionality works and is comprehensively covered by unit tests. If it is not working in your application, it may be misconfigured or used incorrectly, but there's not enough detail or code samples provided here to make that determination. It would help to understand which OAuth strategies and versions you are using to make a determination. Finally, |
Unit tests can sometimes not match up with e2e tests. I'm not alleging you abandoned your library [merely linking to allegations others have made, which went uncontested], nor am I alleging you intended a security lapse intentionally, but I did want to make sure my issue was strongly worded to catch people's attention, so thank you for the quick reply. I can work on a minimal reproduction, but I am confident on our end we are not doing anything to disable the CSRF protection, and what we observe is that the outbound redirect has no The only way we got it to work was by passing
My apologies for not being familiar with the history of this library. If it works, that is still opt in, and according to https://www.passportjs.org/tutorials/google/state/ it is supposed to happen by default within Passport:
But this is not what we observed, nor do I see any code to handle
Awesome, thanks! Yeah adding a warning to the current major version, and perhaps a new major version where it is enabled by default would be warranted in my opinion.
CSRF protection is required, and this library defaults to no CSRF protection (via state or otherwise) |
Actually I just personally verified this on our end. It's also a type error in Typescript, see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/passport/index.d.ts#L67 I then noticed definitely typed has separate In our code, we import
Perhaps there is an |
The tutorial you link to is using passport-google-oidc, which internally uses passport-openidconnect. It does not use passport-oauth2, so the state handling is slightly different (mainly, its default to on, as documented). I am unable to reproduce the issue you are filing. You should also be able to confirm the intended operation (primarily, that state is included when enabled) by running the sample application here: https://github.com/passport/todos-express-google-oauth2 From the information you have provided, it seems that the most likely cause is how Passport and this strategy is being integrated into your application. In order to provide any further help, can you provide exact code of how you are initializing strategies, and how you are using them in routes? It would also help to know which OAuth 2 strategies and versions you are using. |
re: the 'question' label, I don't have any question here.
The feedback I am giving is that as a user it added confusion, since this [unrelated] lib is publishing misleading or incorrect information in their docs about Passport itself. It may not be directly under your control, however my feedback is that you could mitigate this by backfilling the missing documentation [for your strategy] in this repo's readme file.
As mentioned above that's because you're passing
As the author of your lib, I understand you are not as susceptible to this footgun, but try to imagine as a new user (would it be obvious?). I can say for myself, it was not obvious, and others have raised this feedback that they could not figure out why CSRF is not working either.
No, I don't have any issue at this time. I got CSRF protection working in my app prior to opening this issue. The purpose of my issue report is to deliver feedback that you are defaulting to an insecure setup, and there is a lack of docs [thereby making it unnecessarily confusing for users to integrate it properly]. I am clear on how to integrate it, in hindsight. In summary, there are 3 pieces of feedback from me:
My issue is filed on the repo corresponding to the strategy I am using, there is no CSRF protection by default [even on latest version], and there are no docs on how to enable it in your current readme on FYI I am unsubscribing from this thread because I feel like I have made my feedback clear, I don't need any help, and feel like I am allowing myself to come across as abrasive, which is not intentional. I do appreciate this library, and certainly would hope my feedback can be constructively received to make the library even better |
It would help me if you could clearly articulate if you are reporting an issue, or just struggling with integrating this package into your application. In the filing of this issue, you are clearly indicating that you don't believe the package is correctly adding If you want to pass a state option to
If you do not have any state to stare, omit the I understand you found a different solution, but it is relying on undocumented behavior. I would like to confirm that the intended behavior is correct and functional. Can you confirm this? |
Hello @jaredhanson Is this state stored the hole time? After login i can see that in the db: I am using Node 18 with Thank you for the passport libs. Working with them for years now |
Our security team flagged that we were missing CSRF protection, which was surprising because we chose a open source library that is widely used, assuming that it would be secure by default if it was so widely used.
In #138 support for
state
param was added. It states that you can passtrue
to enable CSRF protection; however my team found that even after setting it totrue
, the library did not append anystate
pram to the redirect URL.Finally we were able to store a token into our session manually and then pass it as the state param to
passport.authenticate()
's 2nd argument. This isn't even documented in the README for the project.There is also this issue that is related from five years ago that has gone unaddressed where another user flagged that the library is insecure [by default] and could not figure out how to enable the security. #74
Is there any reason to not have CSRF protection either on by default, or always on? At the very least there should be more prominent warnings logged to stderr or in your a big warning in your README so that people know that they are not running a production ready set up when they choose this library, without further efforts.
An example of an "attack" is that an attacker can stop their own login flow and send a URL with their code to a victim, upon clicking the link the victim is logged in as the attacker. This is a security problem because the victim could unintentionally pay the attacker's bill, or could enter their social security number into a form that gets saved to the account that the attacker controls, etc..
It is also required to have CSRF protection on both the client and the server, according to the Oauth2 spec
The text was updated successfully, but these errors were encountered: