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

Fix APIKeyCookie to accept websocket #9591

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

elysium31
Copy link

No description provided.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 56beb8f at: https://6475ce858bbc31005a94dcc4--fastapi.netlify.app

@tiangolo tiangolo added p4 bug Something isn't working labels Jan 14, 2024
Copy link
Contributor

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's quite useful PR.

I will be ready to approve it after the author makes the small corrections that I mentioned in the review.

I would also think about making the same with APIKeyQuery. Code is almost identical.

And.. I think adding the description to this PR will increase chances that it will be accepted soon!

websocket: WebSocket, current_user: User = Depends(get_current_user)
):
await websocket.accept()
await websocket.send_json(current_user.dict())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dict method is deprecated, use model_dump

Comment on lines +84 to +89
async def __call__(
self,
request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
api_key = (request or websocket).cookies.get(self.model.name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get rid of type errors we need to use request: HTTPConnection. It's common base class for both Request and WebSocket, and it has all necessary fields.
So, the code should be:

    async def __call__(self, request: HTTPConnection) -> Optional[str]:
        api_key = request.cookies.get(self.model.name)

Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async def __call__(
self,
request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
api_key = (request or websocket).cookies.get(self.model.name)
async def __call__(self, conn: HTTPConnection) -> Optional[str]:
api_key = conn.cookies.get(self.model.name)

Comment on lines +84 to +88
async def __call__(
self,
request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async def __call__(
self,
request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
async def __call__(self, conn: HTTPConnection) -> Optional[str]:

Comment on lines +84 to +89
async def __call__(
self,
request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
api_key = (request or websocket).cookies.get(self.model.name)
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
async def __call__(
self,
request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
api_key = (request or websocket).cookies.get(self.model.name)
async def __call__(self, conn: HTTPConnection) -> Optional[str]:
api_key = conn.cookies.get(self.model.name)

request: Request = None,
websocket: WebSocket = None,
) -> Optional[str]:
api_key = (request or websocket).cookies.get(self.model.name)
if not api_key:
if self.auto_error:
raise HTTPException(
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will work as expected after Starlette 0.37.0 is merged, since we will use the WebSocket Denial Extension.

@YuriiMotov
Copy link
Contributor

Just found PR 10147 which also fixes this issue, but that PR is wider since it also fixes the same problem in other security tools.
I think we should close this PR and work on PR 10147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants