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.

Sign up for GitHub

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

Jump to bottom

Partner Login for NewPortal (DO NOT MERGE - DEMO EXAMPLE OF PR) #33

Closed
wants to merge 1 commit into from
Closed

Partner Login for NewPortal (DO NOT MERGE - DEMO EXAMPLE OF PR) #33

wants to merge 1 commit into from

Conversation

shawnsnyk
Copy link

new partner login

Description

A clear and concise summary of the change and which issue (if any) it fixes. Should also include relevant motivation and context.

Resolved or fixed issue:

Affirmation

@shawnsnyk
NewPortal
202f681 
new partner login
@dylansnyk
Copy link

dylansnyk commented Dec 13, 2024 •
edited
Loading

⛔ Snyk checks have failed. 1 issues have been found so far.

Icon Severity Issues
Critical 0
High 1
Medium 0
Low 0

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

⛔ code/snyk check is complete. 1 issues have been found. (View Details)

dylansnyk
dylansnyk reviewed Dec 13, 2024
View reviewed changes
routes/loginPartner.ts

return (req: Request, res: Response, next: NextFunction) => {
verifyPreLoginChallenges(req) // vuln-code-snippet hide-line
models.sequelize.query(`SELECT * FROM Users WHERE email = '${req.body.email || ''}' AND password = '${security.hash(req.body.password || '')}' AND deletedAt IS NULL`, { model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  SQL Injection

Unsanitized input from the HTTP request body flows into query, where it is used in an SQL query. This may result in an SQL Injection vulnerability.

Line 36 | CWE-89 | Priority score 773 | Learn more about this vulnerability
Data flow: 5 steps

Step 1 - 3

juice-shop-goof/routes/loginPartner.ts

Line 61 in 202f681

challengeUtils.solveIf(challenges.weakPasswordChallenge, () => { return req.body.email === 'admin@' + config.get('application.domain') && req.body.password === 'admin123' })

Step 4 - 5

juice-shop-goof/routes/loginPartner.ts

Line 36 in 202f681

models.sequelize.query(`SELECT * FROM Users WHERE email = '${req.body.email || ''}' AND password = '${security.hash(req.body.password || '')}' AND deletedAt IS NULL`, { model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 13, 2024
View reviewed changes
routes/loginPartner.ts

return (req: Request, res: Response, next: NextFunction) => {
verifyPreLoginChallenges(req) // vuln-code-snippet hide-line
models.sequelize.query(`SELECT * FROM Users WHERE email = '${req.body.email || ''}' AND password = '${security.hash(req.body.password || '')}' AND deletedAt IS NULL`, { model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge

Check failure

Code scanning / CodeQL

Database query built from user-controlled sources High

This query string depends on a
user-provided value
Loading
.

Copilot Autofix AI 10 days ago

To fix the problem, we should use query parameters to safely embed the user-provided data into the SQL query. This approach prevents SQL injection by ensuring that the user input is treated as data rather than executable code. Specifically, we will modify the query on line 36 to use parameterized queries provided by the Sequelize library.

Suggested changeset 1
routes/loginPartner.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/routes/loginPartner.ts b/routes/loginPartner.ts
--- a/routes/loginPartner.ts
+++ b/routes/loginPartner.ts
@@ -35,3 +35,7 @@
     verifyPreLoginChallenges(req) // vuln-code-snippet hide-line
-    models.sequelize.query(`SELECT * FROM Users WHERE email = '${req.body.email || ''}' AND password = '${security.hash(req.body.password || '')}' AND deletedAt IS NULL`, { model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge
+    models.sequelize.query('SELECT * FROM Users WHERE email = ? AND password = ? AND deletedAt IS NULL', {
+      replacements: [req.body.email || '', security.hash(req.body.password || '')],
+      model: UserModel,
+      plain: true
+    }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge
       .then((authenticatedUser: { data: User }) => { // vuln-code-snippet neutral-line loginAdminChallenge loginBenderChallenge loginJimChallenge
EOF
@@ -35,3 +35,7 @@
verifyPreLoginChallenges(req) // vuln-code-snippet hide-line
models.sequelize.query(`SELECT * FROM Users WHERE email = '${req.body.email || ''}' AND password = '${security.hash(req.body.password || '')}' AND deletedAt IS NULL`, { model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge
models.sequelize.query('SELECT * FROM Users WHERE email = ? AND password = ? AND deletedAt IS NULL', {
replacements: [req.body.email || '', security.hash(req.body.password || '')],
model: UserModel,
plain: true
}) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge
.then((authenticatedUser: { data: User }) => { // vuln-code-snippet neutral-line loginAdminChallenge loginBenderChallenge loginJimChallenge
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@shawnsnyk shawnsnyk closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylansnyk dylansnyk dylansnyk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shawnsnyk @dylansnyk