[MM-53425]: Added additional checks for POST type APIs #209
Conversation
* [MI-3240]:Fixed security issue [MM-53425] of TODO plugin * [MI-3240]:Fixed review comments
|
Hello @Kshitij-Katiyar, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## master #209 +/- ##
=========================================
- Coverage 6.87% 6.42% -0.45%
=========================================
Files 10 11 +1
Lines 1600 1712 +112
=========================================
Hits 110 110
- Misses 1482 1594 +112
Partials 8 8
☔ View full report in Codecov by Sentry. |
Kshitij-Katiyar
force-pushed
the
MM-53425
branch
from
db67c8d to
9da7452
Compare
|
@Kshitij-Katiyar Could you please link the connected JIRA ticket in the PR description? |
|
Can we add a recovery handler to handle all API routes? Like it's done in the GitHub plugin here: https://github.com/mattermost/mattermost-plugin-github/blob/a336ac3f66347aca7b47963e89570024b184cdfd/server/plugin/api.go#L122 |
@hanzei , @Kshitij-Katiyar doesn't yet have access to JIRA. I'm posting for him below: |
* [MI-3276]:Added recovery handler and CheckAuth middleware * [MI-3276]:Fixed review comments
|
@mickmister @hanzei Added recovery handler middleware and there was also no middleware present to check for Mattermost-User-ID ,so i have created that as well in this PR only. |
* [MI-3423]: Added checks for every post API body * [MI-3423]:Fixed review comments * [MI-3423]:Fixed review comments
server/serializer.go
Outdated
| func IsAddIssuePayloadValid(a *AddAPIRequest) error { | ||
| if a == nil { | ||
| return errors.New("invalid request body") | ||
| } | ||
|
|
||
| if a.Message == "" { | ||
| return errors.New("message is required") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Let's make this a method and simplify the name to IsValid
| func IsAddIssuePayloadValid(a *AddAPIRequest) error { | |
| if a == nil { | |
| return errors.New("invalid request body") | |
| } | |
| if a.Message == "" { | |
| return errors.New("message is required") | |
| } | |
| return nil | |
| } | |
| func (a *AddAPIRequest) IsValid error { |
There was a problem hiding this comment.
@hanzei The reason why i was not able to do this because of the nill check as in the change you have suggested, We wont be able to access the functions of AddAPIRequest if it is already nill and will panic everytime.
There was a problem hiding this comment.
You can have a nil check in a method with a pointer receiver.
|
@DHaussermann Do you want to smoke test the PR? |
|
Tested and Passed
LGTM! |
hanzei
added
the
4: Reviews Complete
Summary
Added additional checks for POST type APIs in todo plugin.