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

Add "Allow" headers to 405 Method Not Allowed responses #1434

Closed
wants to merge 2 commits into from

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jan 26, 2022

This PR was inspired based on a FastAPI issue reported by @hellocoldworld.

As we can see on the RFC 7231, responses with 405 status code MUST provide the allow headers. Starlette doesn't honor that.

What this PR introduces is a potential solution, which I'd like to discuss - to understand if it's the best solution. Changes here:

  • ExceptionMiddleware adds allow headers when HTTPException.status_code == 405. (I'm not sure if this makes sense yet).
  • Add allow headers when PlainResponse.status_code == 405.

There's still a case that is not being handled on this PR:

  • When we have AnyResponse(status_code=405), it doesn't add the allow headers. It should, right?

Important resources:

@adriangb
Copy link
Member

Could we instead add a headers parameter to HTTPException and thus handle this in Route? I don't particularly like iterating through routes from within the middleware.

For context, FastAPI already has a subclass that does this, so that PR would also upstream their customization: https://github.com/tiangolo/fastapi/blob/291180bf2d8c39e84860c2426b1d58b6c80f6fef/fastapi/exceptions.py#L13

@Kludex
Copy link
Member Author

Kludex commented Jan 26, 2022

The code will look faaaaar more clear with that. I've created #1435, as it's another issue.

But maybe none of those two PRs are needed, as we need to solve the question I asked above:

When we have AnyResponse(status_code=405), it doesn't add the allow headers. It should, right?

Should we add a NotAllowedResponse class that accepts allowed_methods? 🤔

@tomchristie
Copy link
Member

Should we add a xyz class

Default answer to this is nope, thanks. 😅

Let's not extend the API of Starlette at all if we can help it.

return PlainTextResponse(
"Method Not Allowed",
status_code=405,
headers={"allow": ", ".join(allowed_methods)},
Copy link
Member

Choose a reason for hiding this comment

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

I guess we'd may as well use standard HTTP casing here - Allow.

(Eg. I checked https://github.com/encode/starlette/blob/master/starlette/middleware/gzip.py to see what we use internally elsewhere)

@adriangb
Copy link
Member

But maybe none of those two PRs are needed, as we need to solve the question I asked above:

When we have AnyResponse(status_code=405), it doesn't add the allow headers. It should, right?

Should we add a NotAllowedResponse class that accepts allowed_methods? 🤔

My thought was that once #1435 gets merged we'd just use that feature in Route.handle and be done with this.

I don't think we need to automagically enforce this for any response with status_code=405. I think that if Starlette itself does this when it returns a 405, that's good enough.

@Kludex
Copy link
Member Author

Kludex commented Jan 26, 2022

Replaced by #1436

@Kludex Kludex closed this Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants