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: Guards should not be executed for OPTIONS requests #2314

Closed
4 tasks done
v3ss0n opened this issue Sep 18, 2023 · 18 comments
Closed
4 tasks done

Bug: Guards should not be executed for OPTIONS requests #2314

v3ss0n opened this issue Sep 18, 2023 · 18 comments
Labels
Bug 🐛 This is something that is not working as expected Security This is related to our security features

Comments

@v3ss0n
Copy link
Contributor

v3ss0n commented Sep 18, 2023

Description

When guards are applied at Controller level , it is blocking the OPTIONS requests and causing error when accessing users on the guards.

  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/connection/base.py", line 228, in user
    raise ImproperlyConfiguredException("'user' is not defined in scope, install an AuthMiddleware to set it")
litestar.exceptions.http_exceptions.ImproperlyConfiguredException: 500: 'user' is not defined in scope, install an AuthMiddleware to set it

URL to code causing the issue

https://github.com/litestar-org/litestar-fullstack

MCVE

To be included

Steps to reproduce

curl 'http://0.0.0.0:9000/api/tags' \    
  -X 'OPTIONS' \
  -H 'Accept: */*' \
  -H 'Accept-Language: en-US,en;q=0.9' \
  -H 'Access-Control-Request-Headers: authorization,content-type' \
  -H 'Access-Control-Request-Method: POST' \
  -H 'Connection: keep-alive' \
  -H 'Origin: http://localhost:8000' \
  -H 'Referer: http://localhost:8000/' \
  -H 'Sec-Fetch-Mode: cors' \

Screenshots

"![SCREENSHOT_DESCRIPTION](SCREENSHOT_LINK.png)"

Logs

Traceback (most recent call last):
  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/middleware/exceptions/middleware.py", line 191, in __call__
    await self.app(scope, receive, send)
  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/routes/http.py", line 77, in handle
    await route_handler.authorize_connection(connection=request)
  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/handlers/base.py", line 481, in authorize_connection
    await guard(connection, copy(self))  # type: ignore
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/utils/sync.py", line 65, in __call__
    return await self.ref.value(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/utils/sync.py", line 101, in wrapper
    return await run_sync(applied_kwarg, *args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/app/.venv/lib/python3.11/site-packages/anyio/to_thread.py", line 33, in run_sync
    return await get_async_backend().run_sync_in_worker_thread(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/app/.venv/lib/python3.11/site-packages/anyio/_backends/_asyncio.py", line 2106, in run_sync_in_worker_thread
    return await future
           ^^^^^^^^^^^^
  File "/workspace/app/.venv/lib/python3.11/site-packages/anyio/_backends/_asyncio.py", line 833, in run
    result = context.run(func, *args)
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspace/app/src/app/domain/accounts/guards.py", line 27, in requires_active_user
    if connection.user.is_active:
       ^^^^^^^^^^^^^^^
  File "/workspace/app/.venv/lib/python3.11/site-packages/litestar/connection/base.py", line 228, in user
    raise ImproperlyConfiguredException("'user' is not defined in scope, install an AuthMiddleware to set it")
litestar.exceptions.http_exceptions.ImproperlyConfiguredException: 500: 'user' is not defined in scope, install an AuthMiddleware to set it

Litestar Version

2.0.1

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)
@v3ss0n v3ss0n added Bug 🐛 This is something that is not working as expected Triage Required 🏥 This requires triage labels Sep 18, 2023
@v3ss0n v3ss0n changed the title Bug: When Guards are applied at controlller level , they are blocking OPTIONS breaking CORS Bug: When Guards are applied at controlller level, they are blocking OPTIONS breaking CORS Sep 18, 2023
@cofin cofin changed the title Bug: When Guards are applied at controlller level, they are blocking OPTIONS breaking CORS Bug: Guards should not be executed for OPTIONS requests Sep 18, 2023
@cofin cofin removed the Triage Required 🏥 This requires triage label Sep 18, 2023
@cofin
Copy link
Member

cofin commented Sep 18, 2023

@v3ss0n Does the updated title better reflect the issue? From your error, it looks like the guard is executing when you send in an OPTIONS request and it should not?

@v3ss0n
Copy link
Contributor Author

v3ss0n commented Sep 26, 2023

Sorry for late reply , after raising this issue i got sick due to food poison . @guacs already fixed it and i am marking it as closed.

I am thinking if it is fit better with your title because it happens on Controller and APP level. What if we apply directly to options decorator ? ( should not do that but for whatever reason , if someone want to do?) should this still execute?

@guacs
Copy link
Member

guacs commented Sep 26, 2023

Sorry for late reply , after raising this issue i got sick due to food poison . @guacs already fixed it and i am marking it as closed.

I am thinking if it is fit better with your title because it happens on Controller and APP level. What if we apply directly to options decorator ? ( should not do that but for whatever reason , if someone want to do?) should this still execute?

The fix that was made in #2325 does NOT fix this issue by preventing guards from called for OPTIONS requests. It only prevents the middleware from being executed for OPTIONS requests by default.

There was already a mechanism for avoiding calling the middleware based on the HTTP method, but there is not an in-built mechanism for guards as of now.

@bwalendz
Copy link

My workaround for now.

async def example_guard(connection: ASGIConnection, _: BaseRouteHandler) -> None:
    if connection.scope.get("method") == "OPTIONS":
        return

    # guard logic

@v3ss0n
Copy link
Contributor Author

v3ss0n commented Sep 27, 2023

My workaround for now.

async def example_guard(connection: ASGIConnection, _: BaseRouteHandler) -> None:
    if connection.scope.get("method") == "OPTIONS":
        return

    # guard logic

is that for 2.1 ? I think 2.1 should be fine for that now.

@bwalendz
Copy link

bwalendz commented Sep 27, 2023

My workaround for now.

async def example_guard(connection: ASGIConnection, _: BaseRouteHandler) -> None:
    if connection.scope.get("method") == "OPTIONS":
        return

    # guard logic

is that for 2.1 ? I think 21 should be fine for that now.

Just updated to 2.1.1final0 and still behaves the same, the preflight OPTIONS is still being passed to the guard. I think it's similar to what @guacs is describing.

@guacs
Copy link
Member

guacs commented Sep 28, 2023

My workaround for now.

async def example_guard(connection: ASGIConnection, _: BaseRouteHandler) -> None:
    if connection.scope.get("method") == "OPTIONS":
        return

    # guard logic

is that for 2.1 ? I think 21 should be fine for that now.

Just updated to 2.1.1final0 and still behaves the same, the preflight OPTIONS is still being passed to the guard. I think it's similar to what @guacs is describing.

Yeah, that behavior should still be there since it hasn't been fixed yet :P

@v3ss0n
Copy link
Contributor Author

v3ss0n commented Nov 6, 2023

This problem still exist , i had tried with 2.3.1 .
What should be the best way to fix this.

@Kumzy
Copy link
Member

Kumzy commented Nov 26, 2023

Just encountered this one today on the latest version 2.3.2

@peterschutt
Copy link
Contributor

It would be easy enough to bypass calling guards for OPTIONS requests - my question is, should it only apply to the options handlers that we generate, or should it be a blanket rule for all OPTIONS method handlers, or be configurable in some manner?

@JacobCoffee JacobCoffee added the Security This is related to our security features label Dec 7, 2023
@bwalendz
Copy link

It would be easy enough to bypass calling guards for OPTIONS requests - my question is, should it only apply to the options handlers that we generate, or should it be a blanket rule for all OPTIONS method handlers, or be configurable in some manner?

I think the ideal would be that the guard should only apply to what was defined in the controller/action. So the controller guard should only be applied to GET /home and POST /profile.

class Site(Controller):
   guards = [permission_guard]

   @get(path="/home")
   async def home(self) -> None:
      pass

   @post(path="/profile")
   async def profile(self) -> None:
      pass

The action guard should only apply to its respective action method handled, GET or POST.

class Site(Controller):
   @get(
      path="/home",
      guards=[permission_guard],
   )
   async def home(self) -> None:
      pass

   @post(
      path="/profile",
      guards=[role_guard],
   )
   async def profile(self) -> None:
      pass

@peterschutt
Copy link
Contributor

peterschutt commented Jan 27, 2024

It would be easy enough to bypass calling guards for OPTIONS requests - my question is, should it only apply to the options handlers that we generate, or should it be a blanket rule for all OPTIONS method handlers, or be configurable in some manner?

I think the ideal would be that the guard should only apply to what was defined in the controller/action. So the controller guard should only be applied to GET /home and POST /profile.

Should we try to handle if an options handler is manually created?

class Site(Controller):
   guards = [permission_guard]

   @get(path="/home")
   async def home(self) -> None:
      pass

    @route(path="/home", http_method="OPTIONS")
    async def home_options_handler(self) -> Response[str | None]:
        ...

@provinzkraut
Copy link
Member

My issue with excluding generated OPTION handlers by default is: What if you want to include guards for them? IMO all security related things should be strict by default and have an option to make it lax. Having a handler automagically generated circumvents your auth without any way to interfere or possibly even know that this is happening sounds like a potential security nightmare to me and not a good user experience.

The workaround presented in this comment is the ideal way to handle this IMO; It requires you to explicitly exclude something from the authentication.

@bwalendz
Copy link

bwalendz commented Feb 1, 2024

My issue with excluding generated OPTION handlers by default is: What if you want to include guards for them? IMO all security related things should be strict by default and have an option to make it lax. Having a handler automagically generated circumvents your auth without any way to interfere or possibly even know that this is happening sounds like a potential security nightmare to me and not a good user experience.

The workaround presented in this comment is the ideal way to handle this IMO; It requires you to explicitly exclude something from the authentication.

The main point of my example is i am not explicitly handling OPTIONS requests but the guard is being applied. Preflight OPTIONS are a special case as well because they're forced by the browser for CORS.

Most other frameworks, through middleware or whatever, will swallow the preflight OPTIONS requests so they don't even make it to your controller because this is the most common use case. So that is another approach instead of needing to workaround every guard that exists.

@guacs
Copy link
Member

guacs commented Feb 5, 2024

My issue with excluding generated OPTION handlers by default is: What if you want to include guards for them? IMO all security related things should be strict by default and have an option to make it lax. Having a handler automagically generated circumvents your auth without any way to interfere or possibly even know that this is happening sounds like a potential security nightmare to me and not a good user experience.
The workaround presented in this comment is the ideal way to handle this IMO; It requires you to explicitly exclude something from the authentication.

The main point of my example is i am not explicitly handling OPTIONS requests but the guard is being applied. Preflight OPTIONS are a special case as well because they're forced by the browser for CORS.

Most other frameworks, through middleware or whatever, will swallow the preflight OPTIONS requests so they don't even make it to your controller because this is the most common use case. So that is another approach instead of needing to workaround every guard that exists.

I agree with this as well. OPTIONS is a special case which means we should have special handling of OPTIONS requests as well. I think we should make it configurable somehow, though I'm not sure how easy that would be to implement. This way, the user should be able to specify whether to apply the guards for the automatically created OPTIONS handler as well as a way to specify the same for manually created OPTIONS handler as well.

@jayantraizada
Copy link

We have faced the exact same issue with guards. Took us couple of hours to troubleshoot. If we are not fixing it at least can we all agree to document it?

@guacs
Copy link
Member

guacs commented Mar 22, 2024

@jayantraizada thanks for reminding me to document this! I've added this to the docs and linked to this issue in #3230.

@provinzkraut
Copy link
Member

In any case, I don't think this is a bug, since the current behaviour works as intended, and is now also explicitly documented. Closing for now. We might revisit the autogenerated OPTIONS handler behaviour in the future.

@provinzkraut provinzkraut closed this as not planned Won't fix, can't repro, duplicate, stale Mar 29, 2024
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 Security This is related to our security features
Projects
Status: Closed
Development

No branches or pull requests

9 participants