lepture / authlib

The ultimate Python library in building OAuth, OpenID Connect clients and servers. JWS,JWE,JWK,JWA,JWT included.
https://authlib.org/
BSD 3-Clause "New" or "Revised" License
4.52k stars 452 forks source link

Session cookie grows indefinitely, results in CSRF Warning. #622

Closed frozturk closed 3 months ago

frozturk commented 8 months ago

Describe the bug

When session cookie exceeds 4096 bytes, user is getting CSRF Warning. set_state_data assumes cookie can grow indefinitely

Error

CSRF Warning! State not equal in request and response.'

To Reproduce

Use SessionMiddleware from starlette, ie. fastapi demo. Go to login page several times but never login. Eventually set-cookie will try to write more than 4096 bytes, browsers won't save the cookie, resulting in CSRF error because latest state is not in cookie.

Expected behavior

A cookie should never exceed 4096 bytes, so users can login without CSRF errors.

Additional context

This issue is potentially caused by commit Refactor whole client integrations. · lepture/authlib@179dc8a which were aiming to solve issue Allowing multiple authentication flows in parallel (e.g., in different tabs) · Issue #285 · lepture/authlib

Also see: Browser Cookie Limits: If you want to support most browsers, then don't exceed 50 cookies per domain, and don't exceed 4093** bytes per domain (i.e. total size of all cookies <= 4093 bytes).

Wauplin commented 5 months ago

Also reporting the same issue. What I did to solve this in our integration (see https://github.com/gradio-app/gradio/pull/8000) is to catch the MismatchingStateError when it happens and delete all legacy states from the session cookie.

try:
    oauth_info = await oauth.huggingface.authorize_access_token(request)
except MismatchingStateError:
    # Delete all keys that are related to the OAuth state
    for key in list(request.session.keys()):
        if key.startswith("_state_huggingface"):
            request.session.pop(key)

    # Redirect back to login
    return RedirectResponse(login_uri)

Thanks @frozturk when founding out the root cause and hope this snippet will help future users until this bug is fixed.

(Regarding how to fix things in authlib itself, I don't know what should be the best approach. A naive solution is to clear previous states when adding a new one but it might cause (minor) discrepancies if a user is working in several tabs. @lepture Happy to open a PR though if this solution looks fine for you.)

lepture commented 5 months ago

@Wauplin A PR is welcome. Thank you.

Wauplin commented 5 months ago

@lepture I just opened https://github.com/lepture/authlib/pull/644. Please let me know what you think.