Allowed HTTP Methods for Actions #10
Conversation
Limit actions to processing only requests with certain HTTP methods in a declarative way
| >} | ||
|
|
||
|
|
||
| To limit already existing Actions and get developers to know these new interfaces a temporary event listener will be added for controller_action_postdispatch event to log HTTP methods used with actions and then the full suite of functional tests (MTF and MFTF) would be ran to get information on as much actions as possible. |
There was a problem hiding this comment.
Can we leave logging in place to let extension developers know that they need to update their extensions?
There was a problem hiding this comment.
I remember we discussed this, I just forgot to mention it - we can add a debug message each time an action without HTTP method declared s processed
There was a problem hiding this comment.
I'm ok with not having this if we going to have a mess detector rule.
| > | ||
| > if ($actionInstance instanceof $interface && $request->getMethod() !== $httpMethod) { | ||
| > | ||
| > throw new NotFoundException(__(‘Page not found.’)); |
There was a problem hiding this comment.
I remember that we agreed to return 405 response code or no?
There was a problem hiding this comment.
No, we chose 404 because it's more consistent with what we do now and server must never return a 405 to GET and HEAD requests (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/405)
|
Everybody's ok with adding a debug message each time an action without HTTP method restriction is found? |
|
|
||
| Request method will be validated in the FrontController before executing a matched action, somewhat like this: | ||
|
|
||
| >foreach ($interfaceMap as $interface => $httpMethod) { |
There was a problem hiding this comment.
It shouldn't be necessary to iterate through all methods. You can do if (isset($interfaceMap[$request->getMethod()]). If doesn't exist, the method is not supported by the application. If $actionInstance does not implement the interface, also - an error.
The map will be opposite:
POST => HttpPostInterface
GET => HttpGetInterface
etc.
There was a problem hiding this comment.
ok, I'll update that part
|
|
||
| To limit already existing Actions and get developers to know these new interfaces a temporary event listener will be added for controller_action_postdispatch event to log HTTP methods used with actions and then the full suite of functional tests (MTF and MFTF) would be ran to get information on as much actions as possible. | ||
|
|
||
| After corresponding Http*Method*ActionInterface will be added to every Action class we have information on. If an action is logged to be accessed only by GET methods it will allow only GET, if only by POST – allowing only POST etc. If by more than one methods – the Action class will not be updated. |
There was a problem hiding this comment.
If by more than one methods – the Action class will not be updated
Why not make it implement multiple interfaces? Or if it's not clear why it really supports multiple methods, create a task to review it (as it can be a security issue).
There was a problem hiding this comment.
because we cannot be sure that an action actually expects multiple request methods or a test is invalid without investigation and if too many cases like that would be found it can take too match time to investigate them. I think I can write something like this: "If by more than one methods – the Action class will be updated to implement multiple interfaces or ignored." Would that be ok?
There was a problem hiding this comment.
I agree with Olga and would propose for now agree that we will need to review each case and discuss one more time if turns out that there are too many cases that need to be reviewed. Or you already know?
|
I think adding a rule to dev/tests/static/framework/Magento/CodeMessDetector/* that will detect classes implementing ActionInterface but not any HTTP method aware interfaces should be sufficient enough to alert developers about using these new interfaces |
validations algorithm, updating actions and static tests
|
I have updated the document to address the concerns |
It may be an overkill, if used in production mode. |
|
In the example, a |
Limit actions to processing only requests with certain HTTP methods in a declarative way