Skip to content

fix(S5146): remediate SonarQube AZhSVLrd4wErqc9Ey1Y4 - validate admin login redirect target - #276

Open
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/sonarqube-fix-AZhSVLrd4wErqc9Ey1Y4
Open

devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/sonarqube-fix-AZhSVLrd4wErqc9Ey1Y4

Conversation

@devin-ai-integration

Copy link
Copy Markdown

Summary

SonarQube issue AZhSVLrd4wErqc9Ey1Y4 (rule jssecurity:S5146, BLOCKER): adminLoginSuccess in routes/index.js called res.redirect(redirectPage) with redirectPage taken straight from req.query.redirectPage, allowing an open redirect to attacker-controlled hosts after a successful admin login (phishing vector).

Fix: add isSafeRedirectUrl(url) and only follow the redirect when it passes; otherwise fall back to /admin.

function isSafeRedirectUrl(url) {
  typeof url === 'string'
    && url.startsWith('/')          // relative, same-origin only
    && !url.startsWith('//')        // protocol-relative
    && !url.startsWith('/\\')       // browser-normalised to //
    && !url.includes('://')         // embedded scheme
    && !/[\\\r\n]/.test(url)        // backslash / header injection
}

Scope is limited to this one finding; no other code or dependencies changed.

Devin-Org: engineering

Link to Devin session: https://app.devin.ai/sessions/1ceeb094e53b4f81af67c6b5eeaf3e40
Open in Devin Desktop: https://app.devin.ai/desktop/session/1ceeb094e53b4f81af67c6b5eeaf3e40?variant=devin
Requested by: @joao-cognition


Note

Devin errored when opening this Pull Request as joao-cognition.
As a fallback, Devin opened this PR as itself.

devin-ai-integration Bot and others added 2 commits August 27, 2026 09:05
Co-Authored-By: Joao Esteves <joao.esteves@cognition.ai>
… jssecurity:S5146

Co-Authored-By: Joao Esteves <joao.esteves@cognition.ai>
@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

@devin-ai-integration

Copy link
Copy Markdown
Author

Remediation notes — SonarQube AZhSVLrd4wErqc9Ey1Y4 / rule jssecurity:S5146 (Open redirect, BLOCKER)

What was flagged: routes/index.js adminLoginSuccess() passed the user-controlled redirectPage query parameter directly to res.redirect(), so a crafted login link (e.g. ?redirectPage=https://evil.[REDACTED SECRET]) would bounce an authenticated admin to an external site.

What changed (only routes/index.js):

  • Added isSafeRedirectUrl(url), which accepts only same-origin relative paths: must be a string starting with a single /; rejects protocol-relative //, /\ (browser-normalised to //), any embedded :// scheme, backslashes and CR/LF (header injection).
  • adminLoginSuccess now redirects to redirectPage only when isSafeRedirectUrl(redirectPage) is true, otherwise falls back to /admin (the pre-existing default).

No other findings, files, or dependencies were touched. Verified the file parses with node --check routes/index.js.

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.

0 participants