Skip to content

[security][backport 24.05] Fortify-flagged path traversal, XSS, and info-leak fixes#7555

Merged
ar2rsawseen merged 2 commits into
release.24.05from
backport/fortify-fixes-24.05
May 11, 2026
Merged

[security][backport 24.05] Fortify-flagged path traversal, XSS, and info-leak fixes#7555
ar2rsawseen merged 2 commits into
release.24.05from
backport/fortify-fixes-24.05

Conversation

@ar2rsawseen
Copy link
Copy Markdown
Member

Summary

Backport of #7547 to release.24.05.

Addresses real findings from a customer Fortify scan against this branch. False positives and out-of-repo (enterprise plugin / Web SDK) findings will be answered separately to the customer with technical justifications.

Fixes

  • Path traversal — frontend/express/app.js
    Theme image handler built sendFile path from req.cookies.theme + req.url with only a prefix check on /images/ or /geodata/. A URL like /images/../../../etc/passwd (or a malicious theme cookie) escaped the themes directory. Now uses res.sendFile with the root option — express normalizes the path and rejects .. traversal natively (recognized by CodeQL as a sanitizer). Also switched req.urlreq.path so query strings don't bleed into the filesystem lookup.

  • Reflected XSS — plugins/two-factor-auth setup2fa.html / enter2fa_login.html
    Hidden username / password inputs used unescaped EJS (<%-). A username containing \"><script> broke out of the attribute and executed in the login origin. Switched to escaped <%=. (secret_token already used <%=.)

  • Sensitive data in logs — api/utils/common.js
    returnRaw / returnMessage / returnOutput dumped the entire params object (incl. req.body, req.headers — passwords, session cookies) on the "output already closed" branch. Replaced with a non-sensitive summary (pathname, apiPath, qstringKeys). Note: 24.05 has three sites here versus two on master — returnRaw exists in this branch and was fixed for consistency.

  • Internal error leak to client — plugins/sdk/api/api.js
    SDK config endpoints echoed 'Error: ' + err to clients. Now log details server-side via console.error and return a generic 'Error retrieving SDK config'.

Differences from #7547

  • common.js fix covers three call sites in 24.05 vs two in master (returnRaw was removed/refactored on master).
  • All four fixes were rebased onto 24.05's slightly older surrounding code; no logic differences.

Test plan

  • Hit /images/../../../etc/passwd (and via crafted theme cookie) — verify next() is called instead of sendFile
  • Log in with a 2FA-enabled user whose username contains \"><script>alert(1)</script> — verify it renders as text in the hidden input value, not as a script
  • Trigger SDK config error path — verify client receives \"Error retrieving SDK config\", server logs the underlying error
  • Force the "output already closed" branch in returnMessage/returnOutput/returnRaw — verify the log line no longer contains password/cookie material

🤖 Generated with Claude Code

…k fixes from PR #7547

Backport of #7547 to
release.24.05.

- frontend/express/app.js: theme image handler built sendFile path from
  cookie + URL with only a prefix check, allowing `..` traversal outside
  /public/themes. Now uses res.sendFile with `root` option so express
  normalizes the path and rejects traversal natively.
- plugins/two-factor-auth setup2fa.html / enter2fa_login.html: hidden
  username/password inputs used unescaped EJS (`<%-`), enabling reflected
  XSS via crafted credentials. Switched to escaped `<%=`.
- api/utils/common.js: returnRaw / returnMessage / returnOutput logged
  the entire params object on the "output already closed" branch, which
  can include req.body/req.headers (passwords, session cookies). Replaced
  with a small non-sensitive summary (pathname only, no query string).
  Note: 24.05 has three sites vs two on master (returnRaw exists here).
- plugins/sdk/api/api.js: SDK config endpoints echoed raw `'Error: ' +
  err` to clients, leaking internal details. Now log details server-side
  and return a generic message.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 11, 2026 07:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Backport of security hardening fixes to release.24.05, addressing Fortify-flagged findings around path traversal, reflected XSS, sensitive-log leakage, and internal error leakage to clients.

Changes:

  • Escape EJS output for hidden username/password fields in 2FA templates to prevent reflected XSS.
  • Reduce sensitive logging in api/utils/common.js when responses are already closed.
  • Stop returning raw internal errors from SDK config endpoints; log server-side and return a generic message.
  • Rework theme static file serving to use res.sendFile(..., { root }) and req.path (but see security comment re: absolute-path bypass).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugins/two-factor-auth/frontend/public/templates/setup2fa.html Switch unescaped EJS output to escaped for hidden credential fields.
plugins/two-factor-auth/frontend/public/templates/enter2fa_login.html Switch unescaped EJS output to escaped for hidden credential fields.
plugins/sdk/api/api.js Replace client-facing 'Error: ' + err with server-side logging and generic error response.
frontend/express/app.js Update theme image handler to use req.path and res.sendFile with root to mitigate traversal.
api/utils/common.js Replace full params logging with a non-sensitive summary when output is already closed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread frontend/express/app.js
Comment thread frontend/express/app.js
- Reject `theme` cookie values that contain path separators, leading
  dots, or control chars via common.sanitizeFilename. Defense-in-depth
  on top of res.sendFile's `root` normalization (which already converts
  absolute paths to relative and blocks `..` traversal via send's
  `'.' + sep + path` prefix).
- Log non-ENOENT / non-403 / non-404 errors from sendFile server-side
  instead of swallowing them silently; still fall through to next() so
  a theme misconfiguration doesn't 500 unrelated routes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ar2rsawseen added a commit that referenced this pull request May 11, 2026
Addresses two Copilot review comments on this PR's theme handler.

- Reject `theme` cookie values that contain path separators, leading
  dots, or control chars via common.sanitizeFilename. Defense-in-depth
  on top of res.sendFile's `root` normalization (which already converts
  absolute paths to relative and blocks `..` traversal via send's
  `'.' + sep + path` prefix).
- Log non-ENOENT / non-403 / non-404 errors from sendFile server-side
  instead of swallowing them silently; still fall through to next() so
  a theme misconfiguration doesn't 500 unrelated routes.

Mirrors the same hardening applied on the 24.05 backport (#7555).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ar2rsawseen ar2rsawseen merged commit 2878f61 into release.24.05 May 11, 2026
5 of 7 checks passed
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.

3 participants