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

Using Invalid Method Should Return 405 #1100

Open
jamesyarrington opened this issue Mar 15, 2018 · 5 comments
Open

Using Invalid Method Should Return 405 #1100

jamesyarrington opened this issue Mar 15, 2018 · 5 comments

Comments

@jamesyarrington
Copy link

When accessing some endpoints, it appears that only GET, OPTIONS and HEAD are appropriate HTTP Request Methods. If an invalid HTTP Request Method is used, the response is coded as 404 Not Found, but should be 405 Method Not Allowed.

To Reproduce:

  1. Send a GET to /search/structured?locality=foo (200 OK returned)
  2. Send a POST to that same URL.

Expected Response:
Server should respond with a 405 Method Not Allowed, with an appropriate message in the response body.
Actual Response:
404 Not Found

{
    "error": "not found: invalid path"
}

Additional Information:
Endpoints I've seen with this behavior:

  • /
  • /search (a POST actually returns a 200 OK here)
  • /autocomplete
  • /search/structured
  • /place
  • /reverse
@orangejulius
Copy link
Member

Hey @jamesyarrington,
Thanks for the report. This is a good catch. Do you happen to know how to fix this with Express? We would definitely accept a pull request to do it :)

@missinglink
Copy link
Member

This could be fairly easy to add in https://github.com/pelias/api/blob/master/routes/v1.js#L405-L418 using the method mentioned in expressjs/express#2414 (comment)

@orangejulius
Copy link
Member

We can also revisit supporting POST for some of our endpoints (see #188). I don't believe its very necessary, but it could be helpful in some cases.

@moar55
Copy link

moar55 commented Sep 1, 2018

Hey there. I am working on this issue: moar55@2ed668e.
I want to know where to add tests for this issue. Also is this coding in the commit good? Can I make this more DRY?

@missinglink
Copy link
Member

hey @moar55, the code looks good, I didn't have time to test it yet.

regarding testing, we used to have an HTTP test suite called ciao but it's not really been maintained, it's a framework that I created many years ago and we'd actually love to replace it with something more standard.

I tried running the tests using the npm run ciao method described in the README and it failed, probably because the code was developed on node ~0.6 (about 5 years old).

We would be very happy to accept a PR for a new HTTP test suite, if possible we'd also like to port over our existing tests https://github.com/pelias/api/tree/master/test/ciao

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

No branches or pull requests

4 participants