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.

Sign up for GitHub

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

Jump to bottom

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

Open
frozturk opened this issue Jan 22, 2024 · 3 comments · May be fixed by #644
Open

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

frozturk opened this issue Jan 22, 2024 · 3 comments · May be fixed by #644
Assignees
@lepture
Labels
bug

Comments

@frozturk
Copy link

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).
@frozturk frozturk added the bug label Jan 22, 2024
@Wauplin Wauplin mentioned this issue Apr 11, 2024
Merged
@Wauplin
Copy link

Wauplin commented Apr 11, 2024

Also reporting the same issue. What I did to solve this in our integration (see gradio-app/gradio#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
Copy link
Owner

lepture commented Apr 11, 2024

@Wauplin A PR is welcome. Thank you.

@Wauplin Wauplin linked a pull request Apr 11, 2024 that will close this issue
Open
@Wauplin
Copy link

Wauplin commented Apr 11, 2024

@lepture I just opened #644. Please let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lepture lepture

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix ever-growing session cookie (starlette integration) Wauplin/authlib
3 participants
@lepture @Wauplin @frozturk