Skip to content

bug: parameterize login query to fix SQL injection (routes/login.ts) #354

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

Open
matthewguerra-cog wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

bug: parameterize login query to fix SQL injection (routes/login.ts) #354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion routes/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export function login () {

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
models.sequelize.query('SELECT * FROM Users WHERE email = $1 AND password = $2 AND deletedAt IS NULL', { bind: [req.body.email || '', security.hash(req.body.password || '')], model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge

@devin-ai-integration devin-ai-integration Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Four login challenges lose exploits

Parameterizing sequelize.query rejects the payloads for Login Admin, Login Bender, Login Jim, and Ephemeral Accountant. Their documented SQL-injection path no longer works, leaving several training challenges unsolvable.

Learn more

This application deliberately exposes selected vulnerabilities as training features. The login handler is the sole exploit path for the three marked login challenges and for ephemeralAccountantChallenge. The parameterized query treats each attack string as an email value, so the query returns no user and verifyPostLoginChallenges never runs. The same change also makes the source shown by the Fix It exercises identical in principle to their correct prepared-statement answer.

Example: Posting jim@juice-sh.op'-- previously selected Jim without his password and solved Login Jim. It now searches for that literal email, returns 401, and leaves the challenge unsolved. The UNION payload that synthesizes the accountant user fails in the same way, with no alternative route to create that ephemeral identity.

Recommended fix: Preserve the intentional challenge behavior or redesign and migrate all four affected challenges together. If the vulnerability is removed, replace their exploit and detection paths, update challenge metadata and codefix assets, and change the five API assertions to validate the new supported solutions. Run the full API, E2E, and RSN suites after the migration.

Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Sep 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Intentional. This PR exists specifically to remediate the CodeQL js/sql-injection finding (CWE-89, 8.8) on this line in this fork; losing the Login Admin/Bender/Jim and Ephemeral Accountant exploit paths is the expected consequence and is called out in the description. Challenge/test migration is out of scope here — the finding owner has accepted the training-challenge regression in favor of closing the vulnerability.

.then((authenticatedUser) => { // vuln-code-snippet neutral-line loginAdminChallenge loginBenderChallenge loginJimChallenge
const user = utils.queryResultToJson(authenticatedUser)
if (user.data?.id && user.data.totpSecret !== '') {
Expand Down
Loading