Skip to content

bug: remediate Snyk SAST findings in routes/index.js (NoSQL injection, open redirect, rate limiting)#55

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/snyk-routes-injection-1782495857
Open

bug: remediate Snyk SAST findings in routes/index.js (NoSQL injection, open redirect, rate limiting)#55
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/snyk-routes-injection-1782495857

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jun 26, 2026

Copy link
Copy Markdown

Summary

Remediates one Snyk SAST group, all confined to routes/index.js (no other files touched).

Findings addressed

Snyk rule Severity Location (orig) Status after re-scan
javascript/NoSqli (CWE-943) High routes/index.js:39 Resolved
javascript/OR Open Redirect (CWE-601) Medium routes/index.js:61 Resolved
javascript/NoRateLimitingForExpensiveWebOperation (CWE-770) Medium :67,75,82,89,152,241,298 (7) Mitigated in-code; still flagged (see note)

snyk code test for routes/index.js: 9 → 7 findings. NoSqli and Open Redirect cleared; the 7 rate-limiting findings remain flagged (Snyk only credits known middleware libraries — see note).

NoSQL injection (loginHandler)

Reject non-string credentials before they reach the Mongo query, then coerce to primitive strings so operator objects ({$gt:""}, {$ne:null}) can't be injected:

if (typeof req.body.username !== 'string' || typeof req.body.password !== 'string')
  return res.status(400).send();
var username = String(req.body.username);
var password = String(req.body.password);
User.find({ username: username, password: password }, ...)

Open redirect (adminLoginSuccess)

Resolve the post-login target from a constant same-origin allowlist; the redirect always uses the matched constant, never the attacker-controlled value:

var ALLOWED_REDIRECTS = ['/admin', '/account_details', '/'];
var safeRedirect = ALLOWED_REDIRECTS.find(p => p === redirectPage);
return res.redirect(safeRedirect || '/admin');

Rate limiting (7 expensive handlers)

Added a minimal in-file fixed-window per-IP limiter and wrapped the flagged handlers (login, admin, get_account_details, save_account_details, create, import, about_new):

var defaultLimiter = createRateLimiter({ windowMs: 60_000, max: 100 });
exports.login = withRateLimit(defaultLimiter, function (req, res, next) { ... });

Note: Snyk's NoRateLimitingForExpensiveWebOperation only recognizes known middleware (e.g. express-rate-limit), which would require editing package.json — owned by a different remediation bucket, so it was intentionally left untouched. The in-file limiter provides equivalent runtime protection (verified: 100×200 then 429), but Snyk continues to flag these 7 until the dependency-based middleware is adopted.

Verification

  • node --check routes/index.js passes; npm install + npm run build (browserify) succeed.
  • App boots (node --openssl-legacy-provider app.js) against MongoDB; / and /login return 200.
  • Runtime checks: NoSQL operator-object login → 400; malicious redirectPage=https://evil.com → redirected to /admin; allowlisted /account_details honored; /about_new returns 429 after 100 req/min.
  • Re-scan diff: snyk code test 9 → 7 findings for routes/index.js (NoSqli + Open Redirect cleared).
  • git diff --name-only confirms only routes/index.js changed.

UI smoke test recording (TODO list create + login page render) attached in the session.

Link to Devin session: https://app.devin.ai/sessions/1114ed5c091f49ebb81c677f4f193b29
Requested by: @stephencornwell


Devin Review

Status Commit
⚪ Not started

Run Devin Review

Open in Devin Review (Staging)

…, open redirect, rate limiting)

Co-Authored-By: Stephen Cornwell <stephen@cognition.ai>
@stephencornwell stephencornwell self-assigned this Jun 26, 2026
Comment thread routes/index.js
hits.set(key, entry);
}
entry.count += 1;
if (entry.count > max) {
Comment thread routes/index.js
}
var username = String(req.body.username);
var password = String(req.body.password);
if (validator.isEmail(username)) {
Comment thread routes/index.js
if (redirectPage) {
return res.redirect(redirectPage)
// Resolve to a constant from the allowlist so an attacker-controlled value
// can never be used as the redirect target (prevents open redirect).
@devin-ai-integration

Copy link
Copy Markdown
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, CI, and merge conflict monitoring

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.

2 participants