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

Starlette 0.15.0 breaks SessionMiddleware by adding ASGI Path for Subroutes using Mount #1261

Closed
2 tasks done
devsetgo opened this issue Aug 7, 2021 · 6 comments · Fixed by #1512
Closed
2 tasks done
Labels
sessions Session cookies

Comments

@devsetgo
Copy link

devsetgo commented Aug 7, 2021

Checklist

  • The bug is reproducible against the latest release and/or master.
  • There are no similar issues or pull requests to fix it yet.

Describe the bug

As discussed with @mahmoudhossam on #1147, I am creating this issue.

Update 0.15.0 broke my session checking function to ensure users are logged in. I mount all the sub routes (users, etc..). The request.session has data in it when set, but when I redirect back to the start page then session is now null. I see no documentation on how to have the session across Mounts?

Below is example of what I am doing...

# Routes
routes = [
    Route("/", main_pages.homepage, name="dashboard", methods=["GET", "POST"]),
    Route("/about", main_pages.about_page, methods=["GET"]),
    Mount("/user", routes=
    [
        Route(
            "/forgot", routes=user_pages.forgot_password, methods=["GET", "POST"]
        ),
        Route("/login", routes=user_pages.login, methods=["GET", "POST"]),
        Route("/logout", routes=user_pages.logout, methods=["GET", "POST"]),
        Route(
            "/password-change",
            routes=user_pages.password_change,
            methods=["GET", "POST"],
        ),
        Route("/profile", routes=user_pages.profile, methods=["GET"]),
        Route("/register", routes=user_pages.register, methods=["GET", "POST"]),
    ],
    name="user",
    ),

# Session Checking
def require_login(endpoint: Callable) -> Callable:
    async def check_login(request: Request) -> Response:
        if "user_name" not in request.session:
            logger.error(
                f"user page access without being logged in from {request.client.host}"
            )
            return RedirectResponse(url="/user/login", status_code=303)

        else:
            one_twenty = datetime.now() - timedelta(
                minutes=config_settings.login_timeout
            )
            current: bool = one_twenty < datetime.strptime(
                request.session["updated"], "%Y-%m-%d %H:%M:%S.%f"
            )

            if current == False:
                logger.error(
                    f"user {request.session['user_name']} outside window: {current}"
                )
                return RedirectResponse(url="/user/login", status_code=303)

            # update datetime of last use
            logger.info(
                f"user {request.session['id']} within timeout window: {current}"
            )
            request.session["updated"] = str(datetime.now())
        return await endpoint(request)

    return check_login

To reproduce

Create basic app, using greater than 0.14.2, using Mount add subroutes and then check the session cookies for the subroutes as there will be a new session cookie as it it now acting like a submounted app, instead of subroutes.

Expected behavior

Mounts should not create a new session cookie, unless Mounting a sub application. See Routing and using Mounts in the documentation. It shows you need to add "app=" for the submounted app. If left out/none, it is just a submounted route and should not be treated as an submounted app.

Mount("/static", app=StaticFiles(directory="static"), name="static")

Actual behavior

Using Mount it will create new session cookies.

Debugging material

Environment

Additional context

I would suggest checking if in Mount app=None as a way to determine whether to use ASGI path or "/".

@JayH5 JayH5 added the sessions Session cookies label Aug 16, 2021
@gamgi
Copy link

gamgi commented Nov 9, 2021

I was able to reproduce this issue in a notebook: tinnable.com/tins/11ccfe75-e709-47ee-b085-f1755e844bef.
It follows @devsetgo's case closely. Hope this is helpful.

@florimondmanca
Copy link
Member

I bumped into this myself as well. Taking a look…

@dfitzpatrick
Copy link

Hi all,
Just got bit by this too. Thankfully found this issue. Removing Mount() and specifying individual Route() will fix the error.

@ghost
Copy link

ghost commented Jan 15, 2022

Hello everyone,
I've bumped into this one as well. Could you, please, fix this bug the sooner the better. Thank you in advance.

@encode encode locked and limited conversation to collaborators Jan 15, 2022
@Kludex
Copy link
Member

Kludex commented Jan 15, 2022

#1351 already created and waiting for feedback.

@tomchristie
Copy link
Member

Alright. After another look at this we've got this... #1512

@encode encode unlocked this conversation Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sessions Session cookies
Projects
None yet
7 participants