Skip to content

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

Closed
Kludex wants to merge 2 commits intomasterfrom
handle-405-allow-headers
Closed

Add "Allow" headers to 405 Method Not Allowed responses#1434
Kludex wants to merge 2 commits intomasterfrom
handle-405-allow-headers

Conversation

@Kludex
Copy link
Owner

@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:

@Kludex Kludex requested a review from lovelydinosaur January 26, 2022 00:18
@adriangb
Copy link
Contributor

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
Owner 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? 🤔

@lovelydinosaur
Copy link
Contributor

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
Contributor

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
Contributor

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
Owner 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