JS: split request forgery query into server-side and client-side variants#8054
Merged
Conversation
4c719b7 to
a73b2fc
Compare
a73b2fc to
69995d5
Compare
erik-krogh
reviewed
Feb 21, 2022
erik-krogh
left a comment
Contributor
There was a problem hiding this comment.
Looks good 👍
I just got a few minor comments.
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
erik-krogh
approved these changes
Feb 22, 2022
hubwriter
reviewed
Feb 23, 2022
| </p> | ||
|
|
||
| <p> | ||
| In example below, the input has been restricted to a number so the endpoint cannot be altered: |
Contributor
There was a problem hiding this comment.
Suggested change
| In example below, the input has been restricted to a number so the endpoint cannot be altered: | |
| In example below, the input has been restricted to a number so that the endpoint cannot be altered: |
Contributor
Author
There was a problem hiding this comment.
Looks like the PR was merged a bit early. I'll open a new PR with this change
hubwriter
approved these changes
Feb 23, 2022
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
The
RequestForgeryquery was previously@precision mediumand I'd like to raise this tohigh.However, it flags both server-side request forgery (SSRF) and client-side request forgery (which is the same, except the browser/client is sending the request) and I only want to promote the actual SSRF variant.
The client-side variant tends to be much less severe than SSRF, and as far as I can tell, it doesn't have a dedicated CWE number like SSRF. Even the name is something I had to make up.
Note that what I call "client-side request forgery" should not to be confused with cross-site request forgery, which is what CSRF and XSRF typically stand for. We could also call it
FrontendRequestForgeryif we want to avoid confusion with CSRF. 🤷The query has been split as follows:
js/request-forgeryis now specific to SSRF and promoted to@precision high, which means being shown by default.js/client-side-request-forgerycovers the client-side cases, and remains insecurity-extended.5.0. There's no real CWE number to derive this from, so I chose it to be 1 point below XSS, as it can sometimes lead to XSS, and it is unlikely to escalate to anything worse than that.The old qhelp and metadata was a bit unclear on what exactly causes request forgery to happen. I rewrote parts of it to emphasize how the different parts of the the URL play a role in request forgery.
The metadata changes were also ported to other languages, and involve a full
@namechange toServer-side request forgery. The only non-JS changes are in 38afb0c. Other language teams: please review and let me know what you think.Evaluation looks fine. 5 results were moved from
js/request-forgerytojs/client-side-request-forgery.