casdoor / casdoor-python-sdk

Python client SDK for Casdoor
https://pypi.org/project/casdoor
Apache License 2.0
34 stars 36 forks source link

Regression of sorts in get_auth_link #36

Closed kaithar closed 9 months ago

kaithar commented 1 year ago

I was looking to preserve request state across the oauth login, so request can continue after login, and found something confusing.

According to the OAuth 2.0 documentation I've read, the state parameter of the oauth request is intended to be random single-use token that identifies the login flow. The primary use, aside from maintaining session association across the redirects, is to prevent CSRF attacks and replay attacks in general. To do that, the state parameter must be unique and impossible to guess, hence the random single-use bit.

I am thus quite confused by PR #24 which replaces that state parameter with the application name, which is effectively a constant for the application, completely defeating any use of it. I'm not sure if this is an error or if the state parameter is intended for something completely different.

That change is also potentially breaking, since state wasn't an optional parameter to the get_auth_link function and the parameter after it could silently take and use the string value if it wasn't passed as a keyword. I'm reluctant to call this a security vulnerability but it probably has potential for abuse, depending on how the library is used.

casbin-bot commented 1 year ago

@seriouszyx @ComradeProgrammer @Resulte

hsluoyz commented 1 year ago

@kaithar are you using SPA like React/Vue/Angular as frontend? Or using Python template as frontend? Actually we already support random state in our JS SDK: https://github.com/casdoor/casdoor-js-sdk/pull/31 . It's a NPM package which is more suitable for SPA scenario. If you are using pure Python, we can also port it to this Python SDK.

/cc @leo220yuyaodog

kaithar commented 1 year ago

@hsluoyz uh, no, I'm explicitly referring to this Python SDK not supporting it, which is a problem if you want to use this package for backend auth.

kaithar commented 9 months ago

Why is this closed as completed? The stated security issue is still in the code.