Skip to content

fix: remediate SQL Injection and add rate limiting in src/routes/auth.js - #108

Closed
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1772173897-fix-sql-injection-auth
Closed

devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1772173897-fix-sql-injection-auth

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Feb 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes 3 high-severity SQL injection vulnerabilities (CodeQL alerts #25, #26, #28) in src/routes/auth.js by replacing string interpolation with parameterized queries using better-sqlite3's ? placeholder syntax.

Alert Endpoint Query Type Line
#25 POST /login SELECT 15–16
#26 POST /reset-password UPDATE 43
#28 POST /register INSERT 57

User-supplied values (username, password, email) are now passed as bound parameters to .get() / .run() instead of being interpolated directly into the SQL string.

Updates since last revision

Added express-rate-limit middleware to address 3 additional CodeQL "missing rate limiting" alerts on the same routes:

Endpoint Window Max Requests
POST /login 15 min 10
POST /reset-password 15 min 5
POST /register 1 hour 10

This adds express-rate-limit (^8.2.1) as a new dependency. Rate limiters use the default in-memory store, which is sufficient for single-instance deployments.

Review & Testing Checklist for Human

  • Verify the ? placeholder binding order matches the intended columns in each of the 3 queries (login SELECT, reset-password UPDATE, register INSERT)
  • Confirm the express-rate-limit middleware is correctly placed as the second argument (before the handler) in each router.post() call
  • Verify rate limit thresholds are acceptable for your use case (login: 10/15min, reset: 5/15min, register: 10/hr) — defaults may need tuning for production
  • Manually test POST /login, POST /reset-password, and POST /register endpoints to confirm they still function correctly with valid inputs, and that rate-limited requests receive a 429 response
  • Note: the login failure response still includes debug: \Query executed: ${query}`` — it now safely shows the parameterized template rather than interpolated user values, but still leaks query structure. This is a separate issue (CWE-209) not addressed here.
  • Note: the rate limiter uses an in-memory store; if the app is scaled to multiple instances, consider switching to an external store (e.g., rate-limit-redis)

Notes

  • No test suite exists in this repo, so there is no automated verification of these changes
  • Other flagged vulnerabilities (hardcoded secrets, MD5 hashing, cleartext passwords, missing auth) are out of scope for this PR
  • Requested by: @yubin-jee
  • Link to Devin run

Convert string interpolation to parameterized queries using ? placeholders
for better-sqlite3 prepared statements:

- Alert #25 (line 16): login query now uses .get(username, password)
- Alert #26 (line 43): reset-password UPDATE now uses .run(resetToken, expiry, email)
- Alert #28 (line 57): register INSERT now uses .run(username, password, email)

This prevents user-provided values from being interpreted as SQL code.

Co-Authored-By: yubinkjee <yubinkjee@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

Adds express-rate-limit middleware to prevent brute-force and abuse:
- /login: 10 requests per 15 minutes per IP
- /reset-password: 5 requests per 15 minutes per IP
- /register: 10 requests per hour per IP

Addresses CodeQL missing rate limiting alerts on lines 13, 38, 55.

Co-Authored-By: yubinkjee <yubinkjee@gmail.com>
@devin-ai-integration devin-ai-integration Bot changed the title fix: remediate SQL Injection in src/routes/auth.js fix: remediate SQL Injection and add rate limiting in src/routes/auth.js Feb 27, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Closing due to inactivity for more than 7 days. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant