Skip to content

Commit

Permalink
Add info file for "Forged Review" coding challenge
Browse files Browse the repository at this point in the history
(also adds neutral lines)
  • Loading branch information
bkimminich committed Nov 12, 2021
1 parent 5f1746f commit 0cbc30f
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 3 deletions.
12 changes: 12 additions & 0 deletions data/static/codefixes/forgedReviewChallenge.info.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
fixes:
- id: 1
explanation: "This solution would reassign an updated review to the last editor, but it would not prevent to change other user's reviews in the first place."
- id: 2
explanation: 'Setting the author on server-side based on the user retrieved from the authentication token in the HTTP request is the right call. It prevents users from just passing any author email they like along with the request.'
- id: 3
explanation: "Removing the option to update multiple documents at once is a good idea and might actually help against another flaw in this code. But it does not fix the problem of allowing users to update other user's reviews."
hints:
- "To find the culprit lines, you need to understand how MongoDB handles updating records."
- "Did you notice that the developers retrieved a reference to the user but never actually use it for anything? This might be part of the problem."
- "Another problematic line you need to select, is actually missing something that ties the user to the review."

8 changes: 7 additions & 1 deletion data/static/i18n/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -438,5 +438,11 @@
"Manually encoding the angular brackets of the HTML tags does not add any security. It is likely to break descriptions with legitimate HTML tags for styling or links, though.": "Manually encoding the angular brackets of the HTML tags does not add any security. It is likely to break descriptions with legitimate HTML tags for styling or links, though.",
"The removed code block deals with handling of different screen sizes and is entirely unrelated to the given XSS vulnerability.": "The removed code block deals with handling of different screen sizes and is entirely unrelated to the given XSS vulnerability.",
"Using bypassSecurityTrustScript() instead of bypassSecurityTrustHtml() changes the context for which input sanitization is bypassed. If at all, this switch might only accidentally keep XSS prevention intact.": "Using bypassSecurityTrustScript() instead of bypassSecurityTrustHtml() changes the context for which input sanitization is bypassed. If at all, this switch might only accidentally keep XSS prevention intact.",
"Removing the bypass of sanitization entirely is the best way to fix the XSS vulnerability here. It should be noted, that XSS is only a consequence of broken autheorization in this case, as users should not be allowed to change product descriptions in the first place.": "Removing the bypass of sanitization entirely is the best way to fix the XSS vulnerability here. It should be noted, that XSS is only a consequence of broken autheorization in this case, as users should not be allowed to change product descriptions in the first place."
"Removing the bypass of sanitization entirely is the best way to fix the XSS vulnerability here. It should be noted, that XSS is only a consequence of broken autheorization in this case, as users should not be allowed to change product descriptions in the first place.": "Removing the bypass of sanitization entirely is the best way to fix the XSS vulnerability here. It should be noted, that XSS is only a consequence of broken autheorization in this case, as users should not be allowed to change product descriptions in the first place.",
"To find the culprit lines, you need to understand how MongoDB handles updating records.": "To find the culprit lines, you need to understand how MongoDB handles updating records.",
"Did you notice that the developers retrieved a reference to the user but never actually use it for anything? This might be part of the problem.": "Did you notice that the developers retrieved a reference to the user but never actually use it for anything? This might be part of the problem.",
"Another problematic line you need to select, is actually missing something that ties the user to the review.": "Another problematic line you need to select, is actually missing something that ties the user to the review.",
"This solution would reassign an updated review to the last editor, but it would not prevent to change other user's reviews in the first place.": "This solution would reassign an updated review to the last editor, but it would not prevent to change other user's reviews in the first place.",
"Removing the option to update multiple documents at once is a good idea and might actually help against another flaw in this code. But it does not fix the problem of allowing users to update other user's reviews.": "Removing the option to update multiple documents at once is a good idea and might actually help against another flaw in this code. But it does not fix the problem of allowing users to update other user's reviews.",
"Setting the author on server-side based on the user retrieved from the authentication token in the HTTP request is the right call. It prevents users from just passing any author email they like along with the request.": "Setting the author on server-side based on the user retrieved from the authentication token in the HTTP request is the right call. It prevents users from just passing any author email they like along with the request."
}
6 changes: 4 additions & 2 deletions routes/updateProductReviews.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@ const challenges = require('../data/datacache').challenges
const db = require('../data/mongodb')
const security = require('../lib/insecurity')

// TODO Merge with file createProductReviews.ts to create a bigger Coding Challenge snippet covering creation and updates at the same time. Alternatively support for multiple source files would be needed in Coding Challenges.

// vuln-code-snippet start noSqlReviewsChallenge forgedReviewChallenge
module.exports = function productReviews () {
return (req, res, next) => {
const user = security.authenticatedUsers.from(req) // vuln-code-snippet vuln-line forgedReviewChallenge
db.reviews.update(
{ _id: req.body.id }, // vuln-code-snippet vuln-line noSqlReviewsChallenge
db.reviews.update( // vuln-code-snippet neutral-line forgedReviewChallenge
{ _id: req.body.id }, // vuln-code-snippet vuln-line noSqlReviewsChallenge forgedReviewChallenge
{ $set: { message: req.body.message } },
{ multi: true } // vuln-code-snippet vuln-line noSqlReviewsChallenge
).then(
Expand Down

0 comments on commit 0cbc30f

Please sign in to comment.