Skip to content

Fix HttpMethodValidator produces 404 instead of 405 response #25047

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
Closed
wants to merge 3 commits into from
Closed

Fix HttpMethodValidator produces 404 instead of 405 response #25047

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 14, 2019 •
edited by ihor-sviziev
Loading

Description (*)

HttpMethodValidator produces 404 instead of 405 response

Fixed Issues (if relevant)

  1. Fixes HttpMethodValidator produces 404 instead of 405 response #24018 : HttpMethodValidator produces 404 instead of 405 response

Manual testing scenarios (*)

  1. Make a GET request to an action with HttpPostActionInterface
  2. Watch the status code received.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@ghost ghost requested review from buskamuza and paliarush as code owners October 14, 2019 14:18
@m2-assistant
Copy link

m2-assistant bot commented Oct 14, 2019

Hi @vishesh-webkul. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@buskamuza
Copy link
Contributor

Please see magento/architecture#10 (comment)

@buskamuza
Copy link
Contributor

Also, I think this can be considered a breaking change, as HTTP response codes should be considered public API. Though I don't thinks our docs cover it now. But it can potentially break someone.

@AlexMaxHorkun
Copy link
Contributor

https://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html
An origin server SHOULD return the status code 405 (Method Not Allowed) if the method is known by the origin server but not allowed for the requested resource, and 501 (Not Implemented) if the method is unrecognized or not implemented by the origin server. The methods GET and HEAD MUST be supported by all general-purpose servers

@AlexMaxHorkun
Copy link
Contributor

I think we initially wanted to return 405 but stumbled upon this specification. Also if I'm remembering correctly Varnish would not take 405 response to GET and HEAD requests well so it basically broke page-cache

@AlexMaxHorkun
Copy link
Contributor

I don't think functional tests that are being run during community PR employ varnish and it would be dangerous to merge this. Page-cache bugs are hard to fix and detect

@AlexMaxHorkun
Copy link
Contributor

Also I don't see Allow header being generated when the exception is thrown, am I missing something?

@buskamuza
Copy link
Contributor

buskamuza commented Oct 17, 2019 •
edited
Loading

IMO, 405 code makes sense. But according to specs (https://tools.ietf.org/html/rfc7231#section-6.5.5) The origin server MUST generate an Allow header field in a 405 response containing a list of the target resource's currently supported methods.. So if we move in this direction, could you please take a look at implementing the Allow header as well, and also please check items mentioned by @AlexMaxHorkun . And ... as usual, consider backward compatibility impact.
Could you please create a small document to https://github.com/magento/architecture repo with description of design changes? Thanks.

@ghost ghost mentioned this pull request Nov 20, 2019
Closed
41 tasks
@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:18
@sidolov sidolov added Priority: P3 May be fixed according to the position in the backlog. Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Aug 18, 2020
@ihor-sviziev ihor-sviziev added this to the 2.5 milestone Sep 7, 2020
@ihor-sviziev ihor-sviziev changed the title 24018 Fix HttpMethodValidator produces 404 instead of 405 response Sep 11, 2020
@orlangur orlangur self-assigned this Oct 8, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buskamuza buskamuza Awaiting requested review from buskamuza

@paliarush paliarush Awaiting requested review from paliarush

Assignees

@orlangur orlangur

Labels
Component: App Component: Exception Priority: P3 May be fixed according to the position in the backlog. Progress: review Release Line: 2.5 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Squashtoberfest 2019
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.

HttpMethodValidator produces 404 instead of 405 response
7 participants
@buskamuza @AlexMaxHorkun @orlangur @ihor-sviziev @gabrieldagama @sidolov @magento-engcom-team