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

feat: Add Allow Header on 405 #560

Merged
merged 4 commits into from Mar 13, 2021

Conversation

ahilke
Copy link
Contributor

@ahilke ahilke commented Mar 12, 2021

Resolves #467

A few notes:

  1. This works with the default express error handler. If you use a custom error handler, you might need to add the headers manually.
  2. The test case uses the default express error handler to showcase this. Sadly this means the test prints an error to the console:
express-openapi-validator
  ✓ adds allow header to 405 - Method Not Allowed
Method Not Allowed: PUT method not allowed
  at /home/ahilke/projects/express-openapi-validator/src/middlewares/openapi.metadata.ts:31:15
  at /home/ahilke/projects/express-openapi-validator/src/openapi.validator.ts:146:18
  at processTicksAndRejections (internal/process/task_queues.js:93:5)
  1. I hope I considered all cases, as it was a tad more complex than I initially thought. Things I considered and added to the test:
    • base path (server)
    • keys under the path that are not a HTTP methods (e.g. parameters)
    • paths with parameters, so that express and open API route are different

The Allow header must be sent if the server responds with 405 to
indicate which request methods can be used.

See cdimascio#467
Adds Allow header to 405 error. This is required by RFC 7231
and indicates which request methods can be used.

Resolves cdimascio#467
When using a custom error handler like in the NestJS example, one needs
to set the headers on the response explicitly. The same is true when
using a custom express error handler.
Copy link
Owner

@cdimascio cdimascio left a comment

Choose a reason for hiding this comment

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

Great stuff. thanks!

@cdimascio cdimascio merged commit 45a40b7 into cdimascio:master Mar 13, 2021
@ahilke ahilke deleted the add-allow-header-on-405 branch March 13, 2021 03:06
@cdimascio
Copy link
Owner

@ahilke I'm seeing a test failure on node 10 that is related to allow header.

Appreciate it if u can have a look. Thanks!

363 passing (7s)
  4 pending
  1 failing
  1) express-openapi-validator
       adds allow header to 405 - Method Not Allowed:
      AssertionError: expected { Object (x-powered-by, allow, ...) } to have property 'allow' of 'POST, GET', but got 'GET, POST'
      + expected - actual
      -GET, POST
      +POST, GET
      
      at request.put.expect.then (test/allow.header.spec.ts:26:36)
      at process._tickCallback (internal/process/next_tick.js:68:7)

@ahilke
Copy link
Contributor Author

ahilke commented Mar 13, 2021

@cdimascio Interesting, I was not aware that different node versions could treat key order differently. The order of the methods in the Allow header should not matter, so I adjusted the test case to ignore order in #562.

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.

How to add an "Allow" header to the response when a 405 error occures?
2 participants