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

Bug: Some session data got lost when trying to set session with big payload #3441

Closed
1 of 4 tasks
wallseat opened this issue Apr 27, 2024 · 6 comments · Fixed by #3446
Closed
1 of 4 tasks

Bug: Some session data got lost when trying to set session with big payload #3441

wallseat opened this issue Apr 27, 2024 · 6 comments · Fixed by #3446
Labels
Bug 🐛 This is something that is not working as expected

Comments

@wallseat
Copy link

wallseat commented Apr 27, 2024

Description

I'm trying to store acess and refresh tokens in request.session with some other user data, and got an error. In middleware AuthRequiredMiddleware i have no user data, but it should be. If I remove two long fields with tokens, it works normaly.

Additionaly, i got a second issue - SerializationException when it trying to decode session with big payload, but can't reproduce it

URL to code causing the issue

No response

MCVE

import secrets
import time
from typing import Any

from litestar import Litestar, Request, Router, get
from litestar.config.cors import CORSConfig
from litestar.middleware.base import DefineMiddleware, MiddlewareProtocol
from litestar.middleware.session.client_side import CookieBackendConfig
from litestar.openapi import OpenAPIConfig
from litestar.openapi.plugins import SwaggerRenderPlugin
from litestar.response import Redirect
from litestar.response.redirect import ASGIRedirectResponse
from litestar.types import ASGIApp, Receive, Scope, Send
from pydantic import BaseModel, Field

USER_SESSION_KEY = "user"
AUTH_REDIRECT_KEY = "auth_redirect_from"


class UserSchema(BaseModel):
    username: str
    fullname: str | None = Field(default=None)
    firstname: str | None = Field(default=None)
    lastname: str | None = Field(default=None)
    email: str | None = Field(default=None)
    position: str | None = Field(default=None)


class UserSessionSchema(BaseModel):
    user_info: UserSchema | None = Field(default=None)
    access_token: str | None = Field(default=None)
    refresh_token: str | None = Field(default=None)
    expire_at: int | None = Field(default=None)


class AuthRequiredMiddleware(MiddlewareProtocol):
    def __init__(
        self,
        app: ASGIApp,
        api_path: str,
        auth_controller_path: str,
        login_endpoint_path: str = "/login",
        **_: Any,
    ) -> None:
        self.app = app

        self.api_path = api_path
        self.auth_controller_path = auth_controller_path
        self.login_endpoint_path = login_endpoint_path
        self.login_url = self.api_path + self.auth_controller_path + self.login_endpoint_path

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        request = Request(scope)
        print("In middleware session:", request.session)

        user_session = UserSessionSchema.model_validate(request.session.get(USER_SESSION_KEY, {}))

        def _redirect_login():
            response = ASGIRedirectResponse(path=self.login_url)

            request.session[USER_SESSION_KEY] = user_session.model_dump()
            request.session[AUTH_REDIRECT_KEY] = str(request.url)

            return response(scope, receive, send)

        if self.auth_controller_path not in request.url.path:
            if not user_session.user_info:
                return await _redirect_login()

            if AUTH_REDIRECT_KEY in request.session:
                del request.session[AUTH_REDIRECT_KEY]

        await self.app(scope, receive, send)


@get("/protected_resource")
async def test(request: Request) -> str:
    user_session = UserSessionSchema.model_validate(request.session[USER_SESSION_KEY])

    assert user_session.user_info

    return f"Hello, {user_session.user_info.username}!"


@get("/auth/login")
async def login(request: Request) -> Redirect:
    request.session[USER_SESSION_KEY] = UserSessionSchema(
        user_info=UserSchema(
            username="some_username",
            fullname="Иванов Ivan Jhon",
            firstname="Ivan",
            lastname="Иванов",
            email="some-email-ivan@notmail.com",
            position="Sernior999lvl",
        ),
        expire_at=int(time.time()) + 900,
        # XXX: Uncoment to reproduce
        # access_token="a" * 2078,
        # refresh_token="b" * 1074,
    )

    print("In login session:", request.session)

    if AUTH_REDIRECT_KEY in request.session:
        return Redirect(request.session.pop(AUTH_REDIRECT_KEY))

    return Redirect("/")


router = Router("/", route_handlers=[test, login])

app = Litestar(
    route_handlers=[router],
    openapi_config=OpenAPIConfig(
        title="Insourcing API",
        version="0.1.0",
        render_plugins=[SwaggerRenderPlugin()],
        path="/api/v1/docs/",
    ),
    cors_config=CORSConfig(
        allow_origins=["http://localhost:8000"],
        allow_credentials=True,
    ),
    middleware=[
        CookieBackendConfig(secret=secrets.token_urlsafe(24).encode()).middleware,
        DefineMiddleware(
            middleware=AuthRequiredMiddleware,
            api_path="",
            auth_controller_path="/auth",
        ),
    ],
    debug=True,
)

Steps to reproduce

1. Install litestar + pydantic2 + uvicorn + uvloop
2. Do not uncomment lines and run with uvicorn FILE_NAME:app
3. open localhost:8000/protected_resource. it should work without errors and you'll see - Hello, some_username!
4. Uncomment lines and run again. You will fall into redirect cycle

Screenshots

No response

Logs

No response

Litestar Version

2.8.2

Platform

WSL 2.1.5.0 + Ubuntu 22.04

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@wallseat wallseat added the Bug 🐛 This is something that is not working as expected label Apr 27, 2024
@peterschutt
Copy link
Contributor

Thanks @wallseat - good find!

Once cookies get above a certain size, we chunk them and store across multiple cookies. When this happens, cookies get stored with an enumeration, e.g., session-0, session-1 etc.

In this case, the session is persisted in the headers for the first time when the request to /protected_route is redirected to the auth route. At this time, the session cookie is not greater than the chunk size and so it gets stored in the cookie under the name session.

After authentication, when the size of the session is much larger due to the presence of the tokens, the serialized session is greater than the chunk size, so the session cookie gets chunked and stored under session-0, session-1.

There is an issue with the algorithm that detects cookies that should be cleared under the condition where the cookie grows in size greater than a single chunk, and that is what we're hitting here. The original session cookie was not being cleared when it is superseded by a cookie called session-0.

peterschutt added a commit that referenced this issue Apr 28, 2024
Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441
peterschutt added a commit that referenced this issue Apr 28, 2024
Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441
peterschutt added a commit that referenced this issue Apr 28, 2024
Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441
@peterschutt
Copy link
Contributor

@all-contributors add @wallseat for bug

Copy link
Contributor

@peterschutt

I've put up a pull request to add @wallseat! 🎉

peterschutt added a commit that referenced this issue Apr 28, 2024
Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441
peterschutt added a commit that referenced this issue Apr 28, 2024
* fix: clear session cookie if new session gt CHUNK_SIZE

Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441

* Update litestar/middleware/session/client_side.py

Co-authored-by: Jacob Coffee <jacob@z7x.org>

* refactor: use dataclass utils to iterate over Cookie fields

---------

Co-authored-by: Jacob Coffee <jacob@z7x.org>
Copy link

This issue has been closed in #3446. The change will be included in the upcoming patch release.

@wallseat
Copy link
Author

Thx @peterschutt !

peterschutt added a commit that referenced this issue May 1, 2024
* fix: clear session cookie if new session gt CHUNK_SIZE

Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441

* Update litestar/middleware/session/client_side.py

Co-authored-by: Jacob Coffee <jacob@z7x.org>

* refactor: use dataclass utils to iterate over Cookie fields

---------

Co-authored-by: Jacob Coffee <jacob@z7x.org>
(cherry picked from commit 0670551)
peterschutt added a commit that referenced this issue May 2, 2024
* fix: clear session cookie if new session gt CHUNK_SIZE

Fix an issue where the connection session cookie is not cleared if the response session is stored across multiple cookies.

Closes #3441

* Update litestar/middleware/session/client_side.py

Co-authored-by: Jacob Coffee <jacob@z7x.org>

* refactor: use dataclass utils to iterate over Cookie fields

---------

Co-authored-by: Jacob Coffee <jacob@z7x.org>
(cherry picked from commit 0670551)
Copy link

github-actions bot commented Jun 2, 2024

A fix for this issue has been released in v2.9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 This is something that is not working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants