-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Get request should not write to database note added. #16329
Conversation
@@ -645,6 +645,8 @@ match 'photos', to: 'photos#show', via: :all | |||
|
|||
NOTE: Routing both `GET` and `POST` requests to a single action has security implications. In general, you should avoid routing all verbs to an action unless you have a good reason to. | |||
|
|||
NOTE: 'GET' in rails don't check for CSRF token. According to security guideline's you should not write to database from 'GET' requests. More on [this](security.html#csrf-countermeasures) |
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.
"..in Rails doesn't check for CSRF token."
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.
"You should never write to the database from 'GET' requests, for more information see the security guide on CSRF countermeasures."
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.
You are right @zzak Thanks for edit. How should I add them or will you do this when merging it?
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.
@deependersingla you can fix those statements yourself and git commit --amend
in your branch. Also, make sure to add a [skip ci]
in the commits message as this is just doc changes.
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.
@arthurnn Done deependersingla@4acde5a.
Thanks
@deependersingla Could you squash it to one commit? And don't forget "[ci skip]" in the commit message. Then I can merge this. Thank you! |
This is one way to squash those 2 commits in one.
|
@zzak Done |
GET request should not write to database note added. [ci skip]
@deependersingla Thank you! |
Get Request should not write to database according to security guideline's note added to Section 3.7 HTTP verb constraint.