Skip to content
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

Improvements to NoSQL manipulation code fixes #1691

Merged
merged 2 commits into from
Sep 29, 2021
Merged

Improvements to NoSQL manipulation code fixes #1691

merged 2 commits into from
Sep 29, 2021

Conversation

denkerszaf
Copy link

While reviewing your potential fixes, i noticed that none of them fixed the vulnerability.
Fix 1 checks the '$ne'-Attribute of the body itself, the fix needs to look into the id attribute... (Since this is a wrong solution I am not sure whether this is intentional or not)
Fix 3 checks if the value of id is not a number... This won't work, since the review ids are strings.

Feel free to edit as you like, Javascript is not my native language.

Stefan Denker added 2 commits September 29, 2021 14:43
Signed-off-by: Stefan Denker <Stefan@dn-kr.de>
Signed-off-by: Stefan Denker <Stefan@dn-kr.de>
@J12934
Copy link
Member

J12934 commented Sep 29, 2021

Yup, as the person who wrote these I can confirm that this is exactly how I wanted them to be. Thanks you for the fix 👍

@J12934 J12934 added the bug label Sep 29, 2021
@bkimminich bkimminich changed the base branch from master to develop September 29, 2021 16:18
@bkimminich bkimminich merged commit a7f7b9d into juice-shop:develop Sep 29, 2021
@bkimminich
Copy link
Member

Hi @denkerszaf, now that your PR made it into the official release, you're eligible for a sticker pack as a first time contributor! If you mail or Slack-PM me your address, I'll make sure you'll get one. It might take a while as I tend to pile up a few such packages before going to the post office... :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants