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

🐛 Fix OAuth2PasswordRequestForm and OAuth2PasswordRequestFormStrict fixed grant_type "password" RegEx #9783

Merged
merged 18 commits into from
Jan 30, 2025
Merged

🐛 Fix OAuth2PasswordRequestForm and OAuth2PasswordRequestFormStrict fixed grant_type "password" RegEx #9783

merged 18 commits into from
Jan 30, 2025
+57 −30

Conversation

skarfie123
Copy link
Contributor

When using the OAuth2 password request form, the regex for the grant_type is not strict enough. It currently allows any string that contains the word "password" in it.

The docstring says:

grant_type: the OAuth2 spec says it is required and MUST be the fixed string "password"

This PR changes the regex to ensure the exact string.

I also added extra test cases to cover this.

Minimal example:

from typing import Annotated
from fastapi import FastAPI, Depends
from fastapi.security import OAuth2PasswordRequestForm

app = FastAPI()

@app.post("/login")
async def login(form_data: Annotated[OAuth2PasswordRequestForm, Depends()]):
    pass

Using the Swagger UI docs to test, with grant_type=passwordblah it returns 200 before but 422 after the change, as expected.

@iudeen
Copy link
Contributor

iudeen commented Jul 12, 2023

I wonder if we should let the user decide the pattern, rather than forcing a stricter requirement by default.

@skarfie123
Copy link
Contributor Author

skarfie123 commented Jul 14, 2023 •
edited
Loading

My understanding is that this class is specifically for the OAuth2 password flow
https://www.oauth.com/oauth2-servers/access-tokens/password-grant/
That specifies that the grant_type must be password. The docstring also mentions this.

If the user wants a different flow they can implement it themselves, or a more general class should be provided.
In the current state I think it is misleading.

@tiangolo tiangolo added bug Something isn't working p2 labels Jan 14, 2024
YuriiMotov
YuriiMotov approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@YuriiMotov YuriiMotov left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed this PR. Everything is good!

  1. Tests fail before changes (returns response=200 instead of 422 when passing grant_type 'prefix password' or 'password suffix').
  2. Tests are Ok after applying changes.
  3. Documentation is Ok (there is no need to change anything)

@skarfie123
Copy link
Contributor Author

I've resolved the merge conflicts.

@alejsdev alejsdev self-assigned this Aug 31, 2024
@alejsdev
Copy link
Member

Hi @skarfie123, thanks for your interest in contributing to FastAPI. We appreciate your effort and the time you have taken to submit this PR. We have a high volume of PRs and we're working hard to review and classify them. We appreciate your patience as we managed the queue, we'll review your PR soon. Thanks! 🙇‍♀️

@skarfie123
Copy link
Contributor Author

No worries @alejsdev ! I've resolved the merge conflicts again.

@alejsdev alejsdev changed the title Fix oauth2 password regex 🐛 Fix OAuth2 password regex Jan 3, 2025
@alejsdev
Copy link
Member

alejsdev commented Jan 3, 2025

I already checked it and it's ok, from my point of view it's ready to be merged, I'll leave the final decision to @tiangolo 🚀
I appreciate your patience and help! Thanks @skarfie123 🙇‍♀️

@alejsdev alejsdev removed their assignment Jan 3, 2025
@svlandeg
Copy link
Member

I merged in master and resolved the conflicts - thanks to the recently simplied test suite, there are much less test files that need correction now 😎

@tiangolo tiangolo changed the title 🐛 Fix OAuth2 password regex 🐛 Fix OAuth2PasswordRequestForm and OAuth2PasswordRequestFormStrict fixed grant_type "password" RegEx Jan 30, 2025
tiangolo
tiangolo approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Nice, thank you! 🚀 🐛

And thanks for the reviews and help everyone! ☕

@tiangolo tiangolo enabled auto-merge (squash) January 30, 2025 12:14
@tiangolo tiangolo merged commit d5ecbac into fastapi:master Jan 30, 2025
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YuriiMotov YuriiMotov YuriiMotov approved these changes

@tiangolo tiangolo tiangolo approved these changes

Assignees
No one assigned
Labels
bug Something isn't working p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@skarfie123 @iudeen @alejsdev @svlandeg @tiangolo @YuriiMotov