Skip to content

[fix][star-rating] keep the uploaded logo name inside the images directory (24.05) - #7931

Open
ar2rsawseen wants to merge 2 commits into
release.24.05from
backport/feedback-logo-identifier-path-2405
Open

[fix][star-rating] keep the uploaded logo name inside the images directory (24.05)#7931
ar2rsawseen wants to merge 2 commits into
release.24.05from
backport/feedback-logo-identifier-path-2405

Conversation

@ar2rsawseen

@ar2rsawseen ar2rsawseen commented Aug 14, 2026

Copy link
Copy Markdown
Member

Backport of #7930 to release.24.05. Note this branch's config sample ships fileStorage: "fs", so the path did reach disk here by default.

Two problems with the same request parameter on /i/feedback/logo: where the file lands, and whose file it replaces.

1. The identifier chose the write location

uploadFile() concatenated the identifier into the upload path:

var pp = path.resolve(__dirname, './../images/' + id + "." + detectedExt);

id is params.qstring.identifier, so separators or leading dots in it selected a directory rather than only a file name. With fileStorage set to "fs" the destination reaches fs.writeFile unchanged, so the write could land in any directory that already exists. Under the shipped default of "gridfs" the same call keeps only the basename, so the path never reaches disk.

Fix: validate the identifier in image-utils.js, next to the existing parseFeedbackLogoName, and use the validated value for the path, the stored id and the returned name. The dashboard sends Date.now(), so accepting a plain name covers every real upload.

2. The identifier also chose whose logo was replaced

The route is authorized with validateCreate against the caller's own app_id, but every app's logos share one directory and a widget's logo field holds just the file name. So the identifier alone decided which app's logo was overwritten, and an account with create rights on its own app could replace the logo shown in another app's end-user feedback popup. The name is not private: it comes back with the widget from the sdk facing by-id lookups.

Fix: refuse a name that a widget belonging to a different app already points at.

The sibling /i/feedback/upload route already does this, decoding its target app out of the file name with a comment saying an admin of one app must not be able to plant a logo for another. This brings the older route into line.

Two details worth stating:

  • Matching on the full name including the extension is deliberate. A different extension is a different file and overwrites nothing.
  • Re-uploading your own app's logo is unaffected, because the check only considers widgets whose app differs from the caller's.

Scope

Every place a request supplied value reaches a file write, with a verdict:

site verdict
plugins/star-rating/api/api.js uploadFile (/i/feedback/logo) both problems fixed here
plugins/star-rating/api/api.js uploadFeedbackFile (/i/feedback/upload) already validated, and already app scoped by name
api/parts/mgmt/apps.js iconUpload already sanitized via common.sanitizeFilename
api/utils/render.js screenshot path name is server generated
countly-enterprise-plugins crash_symbolication symbol_id passes common.db.ObjectID() and a findOne match before the write

uploadFile has exactly one caller, so both checks cover the whole reachable surface.

Verification

  • test/unit-tests/star-rating.image-utils.js, 29 cases: traversal, separators in both slash directions, absolute and drive style paths, NUL and tab truncation, plus the numeric identifiers the dashboard sends and plain names with dashes and underscores. Reverting the validator to the previous pass through fails four of them.
  • The cross-app check is a single query against feedback_widgets and is not covered by an automated case: the plugin suite has one application, so a second one would have to be created first. Verified by reading the query and its two branches instead, and noted here rather than implied.
  • Lint and node --check clean on the changed files.

…ctory

uploadFile() took the identifier straight from the request and concatenated it into the
upload path:

    var pp = path.resolve(__dirname, './../images/' + id + "." + detectedExt);

so separators or leading dots in the identifier chose where the file was written rather
than only what it was called. With fileStorage set to "fs" the destination reaches
fs.writeFile unchanged, so the write could land in any directory that already exists.

Validate the identifier before it is used, next to the existing parseFeedbackLogoName in
image-utils.js, and use the validated value for the path, for the stored file id and for
the name returned to the caller. The dashboard sends Date.now() as the identifier, so a
plain name is all that ever needs to be accepted.

The sibling /i/feedback/upload already validates its target name this way, and app icon
uploads in api/parts/mgmt/apps.js sanitize theirs, so this brings the last of the three
into line.

test/unit-tests/star-rating.image-utils.js covers the traversal, separator, absolute path
and truncation shapes, plus the identifiers the dashboard actually sends. Reverting the
validator to the previous pass-through fails four of them.
The logo upload is authorized with validateCreate against the caller's own app_id, but the
file it writes is named only by the request's identifier, and every app's logos live in one
shared directory. A widget's logo field holds just that file name, so the identifier alone
decided whose logo was replaced. The name is not private either: it comes back with the
widget from the sdk facing by-id lookups.

Refuse a name that a widget belonging to a different app already points at. The sibling
/i/feedback/upload route decodes its target app out of the file name for the same reason,
with a comment saying an admin of one app must not be able to plant a logo for another, so
this brings the older route into line.

Matching on the full name including the extension is deliberate: a different extension is a
different file and overwrites nothing. Re-uploading your own app's logo is unaffected, since
the check only looks at widgets whose app differs from the caller's.
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.

1 participant