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

Do not validate challenge if "bid" is "NaN" #1512

Merged
merged 2 commits into from
Dec 4, 2020
Merged

Do not validate challenge if "bid" is "NaN" #1512

merged 2 commits into from
Dec 4, 2020

Conversation

cnotin
Copy link
Contributor

@cnotin cnotin commented Nov 17, 2020

This can occur if the Session Storage was cleared after login. "bid" was modified but it was surely unintentional and thus should not validate challenge.

CANCELLED: Also, do not rely on "user.bid" as it is defined at login but it is not set if we restart the app, keep our JWT but do not go through login again. Compare instead with the userid of the basket object we got from DB, through the given id

@cnotin cnotin marked this pull request as draft November 17, 2020 18:34
@cnotin

This comment has been minimized.

@cnotin

This comment has been minimized.

@cnotin cnotin marked this pull request as ready for review November 17, 2020 19:06
@bkimminich
Copy link
Member

Hi @cnotin! Excluding NaN sounds reasonable. The change breaks an API test, though. See https://travis-ci.com/github/bkimminich/juice-shop/jobs/441859273#L1190.

@cnotin cnotin closed this Nov 20, 2020
@cnotin cnotin reopened this Nov 20, 2020
@cnotin cnotin marked this pull request as draft November 20, 2020 17:26
@cnotin cnotin marked this pull request as ready for review November 20, 2020 21:44
@cnotin cnotin closed this Nov 20, 2020
@cnotin cnotin reopened this Nov 20, 2020
@cnotin
Copy link
Contributor Author

cnotin commented Nov 20, 2020

I caught the bug and fixed it!
I didn't see it on my machine because the database wasn't cleared and the state wasn't clean before running the tests.

I guess Travis-CI is running in another PR and will come back to this one after :)

@cnotin cnotin marked this pull request as draft November 20, 2020 22:02
@cnotin cnotin marked this pull request as ready for review November 20, 2020 22:32
@cnotin cnotin changed the base branch from master to develop November 21, 2020 16:06
This can occur if the Session Storage was cleared after login. "bid" was modified but it was surely unintentional and thus should not validate challenge

Signed-off-by: Clément Notin <clement@notin.org>
It happens if we restart the app, we are somewhat still connected
because of the JWT that remained but since we did not go through
login then "user.bid" is not defined

Signed-off-by: Clément Notin <clement@notin.org>
@bkimminich
Copy link
Member

ℹ️ Unfortunately the Travis-CI pipeline is still unavailable due to missing OSS credits. I've pinged them on the respective support ticket once again, but haven't heard back from them yet. Reviews and merging of open PRs will resume once our CI/CD is back online. Thanks for your contribution and patience! 👍

@bkimminich bkimminich merged commit 486f56b into juice-shop:develop Dec 4, 2020
@cnotin cnotin deleted the pr-basket-chall-NaN-false-positive branch December 4, 2020 18:58
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2021
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

2 participants