-
Notifications
You must be signed in to change notification settings - Fork 152
Allowed HTTP Methods for Actions #10
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
### Overview | ||
|
||
To improve security and logistics we need to allow limiting Actions to processing only requests with certain HTTP methods and add those limitations to as many existing Actions as possible. | ||
There are many vulnerabilities caused by actions processing both GET and POST requests and thus allowing bypassing security validations like form key validation. Also limiting actions to processing only requests with certain methods would serve as self-documentation for Action classes and improve consistency of server side for client code and functional tests. | ||
|
||
### Theory of Operation | ||
|
||
Since we use by-convention routing it would be inconsistent to defined actions’ allowed HTTP methods in a config file, so it’s better to create marker interfaces like these: | ||
|
||
>interface HttpGetActionInterface extends ActionInterface | ||
> | ||
>{ | ||
> | ||
>} | ||
> | ||
> | ||
>interface HttpPostActionInterface extends ActionInterface | ||
> | ||
>{ | ||
> | ||
>} | ||
> | ||
> | ||
>interface HttpPutActionInterface extends ActionInterface | ||
> | ||
>{ | ||
> | ||
>} | ||
|
||
etc. | ||
|
||
One interface per each HTTP method (using list of HTTP methods from \Zend\Http\Request::METHOD_* constants). We will have an exentable map *HTTP method* => *Interface* passed to the validator to allow custom HTTP methods to be processed. | ||
|
||
Request method will be validated in the FrontController before executing a matched action, somewhat like this: | ||
|
||
>if (!(array_key_exists($request->getMethod(), $interfaceMap) && $actionInstance instanceof $interfaceMap[$request->getMethod()])) { | ||
> | ||
> throw new NotFoundException(__(‘Page not found.’)); | ||
> | ||
>} | ||
|
||
|
||
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 will be investigated and either updated to implement multiple interfaces or left alone if proven too difficult to determine HTTP methods. | ||
|
||
To sway developers to specify HTTP methods for actions a new rule for code mess detector will be added that will require implementations of ActionInterface to implement also at least one of Http*Method*ActionInterface interfaces. | ||
|
||
### Work Breakdown | ||
* Add Http*Method*ActionInterface and implement request validation in the FrontController | ||
* Add a rule to code mess detector | ||
* Create logging mechanism for Actions | ||
* Log HTTP methods used when running full functional tests’ suite | ||
* Create script to add Http*Method*ActionInterface to Action classes we’ve collection information about | ||
* Update Action Classes |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with not having this if we going to have a mess detector rule.