Fix SQL injection auth bypass in /rest/user/login (bind parameters) - #332
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…n auth bypass Signed-off-by: Devin AI <devin-ai-integration[bot]@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| 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 = $email AND password = $password AND deletedAt IS NULL', { bind: { email: req.body.email || '', password: security.hash(req.body.password || '') }, model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge |
There was a problem hiding this comment.
🔴 Parameterized login disables five challenges
When $email binds payloads literally, injected users cannot reach verifyPostLoginChallenges. Five existing SQL-injection challenge flows fail; GDPR Data Erasure and Ephemeral Accountant become unsolvable.
Learn more
The login endpoint is part of the training behavior, not only an authentication boundary. Login Admin, Login Jim, and Login Bender are taught through SQL injection by their hacking instructors, and their completion checks run only after a successful login. GDPR Data Erasure requires logging into a soft-deleted account, while Ephemeral Accountant requires a synthetic user that does not exist in the database. The new bind parameters reject every payload those flows require. Updating tests to expect rejection does not remove or replace the live challenges, hints, instructor steps, scoring hooks, or coding exercises.
Example: A player submits acc0unt4nt@juice-sh.op through the documented UNION SELECT payload. The payload now becomes a literal email, returns no user, and never reaches the accountant completion check. Normal registration is explicitly blocked for that address, so the challenge cannot be solved.
Recommended fix: Preserve the intentionally injectable challenge endpoint, or introduce an approved alternate training path before parameterizing it. If these challenges are intentionally retired, remove or redesign all five challenge definitions, completion hooks, instructors, snippets, codefixes, documentation, and tests together.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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 = $email AND password = $password AND deletedAt IS NULL', { bind: { email: req.body.email || '', password: security.hash(req.body.password || '') }, model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge |
There was a problem hiding this comment.
🔍 Coding exercise metadata is stale
The fixed query remains the vuln-line for three coding exercises. Their existing codefix options now compare parameterization against already-parameterized source code.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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 = $email AND password = $password AND deletedAt IS NULL', { bind: { email: req.body.email || '', password: security.hash(req.body.password || '') }, model: UserModel, plain: true }) // vuln-code-snippet vuln-line loginAdminChallenge loginBenderChallenge loginJimChallenge |
There was a problem hiding this comment.
🔍 Required RSN follow-up is missing
This snippet edit requires RSN verification and synchronized codefixes. Neither codefix nor RSN cache changes accompany it.
Was this helpful? React with 👍 or 👎 to provide feedback.
Description
Fixes SQL injection in the
POST /rest/user/loginhandler (routes/login.ts:34).req.body.emailwas interpolated straight into the rawSELECT * FROM Users WHERE email = '...'template literal passed tosequelize.query, letting an unauthenticated attacker bypass authentication (' OR 1=1--logs in as the first/admin user) and exfiltrate data viaUNION SELECT.The query now uses Sequelize bind parameters:
so the email value is never parsed as SQL. The
vuln-code-snippetmarkers are kept intact so the coding-challenge snippet extraction still works.Tests: the
test/api/login.test.tsSQLi login cases now assert a401, and the CypressloginAdmin/loginJim/loginBenderexploit specs are replaced with a regression spec asserting the injection payloads are rejected.Resolved or fixed issue: none
AI Tool Disclosure
DevinDevin (Cognition AI)Fix SQL injection in routes/login.ts login handler using parameterized query; open a PR.Affirmation
Devin-Org: engineering