[security] Fix Fortify-flagged path traversal, XSS and info-leak issues#7547
Open
ar2rsawseen wants to merge 2 commits intomasterfrom
Open
[security] Fix Fortify-flagged path traversal, XSS and info-leak issues#7547ar2rsawseen wants to merge 2 commits intomasterfrom
ar2rsawseen wants to merge 2 commits intomasterfrom
Conversation
Addresses real findings from a customer Fortify scan: - frontend/express/app.js: theme image handler built sendFile path from cookie + URL with only a prefix check, allowing `..` traversal outside /public/themes. Now resolved through common.resolvePathInBase. - 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: 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. - 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>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes several Fortify-verified security findings across the frontend static file handler, 2FA templates, shared API response helpers, and the SDK plugin endpoints to prevent path traversal, reflected XSS, sensitive log leakage, and internal error disclosure.
Changes:
- Prevent path traversal in the theme image/geodata handler by resolving requested paths via
common.resolvePathInBase. - Fix reflected XSS in 2FA templates by switching unescaped EJS output (
<%-) to escaped (<%=) for hiddenusername/passwordinputs. - Reduce sensitive data exposure in “output already closed” logging for
returnMessage/returnOutput, and avoid echoing internal errors to SDK clients.
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 | Escapes username/password when rendering into hidden input values to prevent attribute-breakout XSS. |
| plugins/two-factor-auth/frontend/public/templates/enter2fa_login.html | Escapes username/password in hidden fields to prevent reflected XSS during 2FA login. |
| plugins/sdk/api/api.js | Stops returning raw error details to clients; logs server-side and returns a generic error message. |
| frontend/express/app.js | Uses common.resolvePathInBase to keep theme asset requests contained within /public/themes. |
| api/utils/common.js | Avoids dumping full params object in a logging edge-case; logs a reduced summary instead. |
- frontend/express/app.js: replace explicit resolvePathInBase + fs.exists + sendFile with res.sendFile's `root` option. Express normalizes the path and rejects `..` traversal natively (recognized by CodeQL as a sanitizer, addresses alerts 1329 and 1330). Switched req.url to req.path so query strings don't bleed into filesystem lookups. - api/utils/common.js: log params.urlParts.pathname instead of req.url in the "output already closed" branch — req.url can carry api_key / auth_token in the query string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses real findings from a customer Fortify scan that we verified against current
master. False positives and out-of-repo (enterprise plugin / Web SDK) findings will be answered separately to the customer with the existing developer-notes justifications.Fixes
Path traversal — frontend/express/app.js:481
Theme image handler built
sendFilepath fromreq.cookies.theme + req.urlwith only a prefix check on/images/or/geodata/. A URL like/images/../../../etc/passwd(or a maliciousthemecookie) escaped the themes directory. Now resolved throughcommon.resolvePathInBase(the same helper used for/appimagesand/memberimagesimmediately below).Reflected XSS — plugins/two-factor-auth setup2fa.html / enter2fa_login.html
Hidden
username/passwordinputs used unescaped EJS (<%-). A username containing\"><script>broke out of the attribute and executed in the login origin. Switched to escaped<%=. (secret_tokenalready used<%=.)Sensitive data in logs — api/utils/common.js:1404, 1488
returnMessage/returnOutputdumped the entireparamsobject (incl.req.body,req.headers— passwords, session cookies) on the "output already closed" branch. Replaced with a non-sensitive summary (url,apiPath,qstringKeys).Internal error leak to client — plugins/sdk/api/api.js:29, 75
SDK config endpoints echoed
'Error: ' + errto clients. Now log details server-side viaconsole.errorand return a generic'Error retrieving SDK config'.Out of scope
Fortify findings against
frontend/express/public/sdk/web/countly.jsand the enterprise plugins (content,users,journey_engine,drill,attribution,surveys,cohorts,retention_segments,white-labeling,crashes-jira,adjust,groups) live in their own repositories.Test plan
/images/../../../etc/passwd(and via craftedthemecookie) — verifynext()is called instead ofsendFile\"><script>alert(1)</script>— verify it renders as text in the hidden input value, not as a script\"Error retrieving SDK config\", server logs the underlying errorreturnMessage/returnOutput— verify the log line no longer contains password/cookie material🤖 Generated with Claude Code