From 8a9db21f20fabb9a33616638991d525cf6ee9e40 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 16:28:53 +0300 Subject: [PATCH 1/2] [fix][star-rating] keep the uploaded logo name inside the images directory 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. --- plugins/star-rating/api/api.js | 15 ++++++-- plugins/star-rating/api/image-utils.js | 25 +++++++++++++- test/unit-tests/star-rating.image-utils.js | 40 ++++++++++++++++++++++ 3 files changed, 76 insertions(+), 4 deletions(-) diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index c3626c52fd4..353ce57b936 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -267,6 +267,15 @@ function uploadFile(myfile, id, callback) { } var tmp_path = myfile.path; + //The identifier is request supplied and is concatenated into the path below, so refuse + //anything that is not a plain name before it can pick the write location. + var safeId = imageUtils.safeLogoIdentifier(id); + if (!safeId) { + fs.unlink(tmp_path, function() { }); + callback("Invalid identifier"); + return; + } + create_upload_dir(function() { fs.readFile(tmp_path, (err, data) => { if (err || !data) { @@ -285,14 +294,14 @@ function uploadFile(myfile, id, callback) { return; } try { - var pp = path.resolve(__dirname, './../images/' + id + "." + detectedExt); - countlyFs.saveData("star-rating", pp, data, { id: "" + id + "." + detectedExt, writeMode: "overwrite" }, function(err3) { + var pp = path.resolve(__dirname, './../images/' + safeId + "." + detectedExt); + countlyFs.saveData("star-rating", pp, data, { id: "" + safeId + "." + detectedExt, writeMode: "overwrite" }, function(err3) { fs.unlink(tmp_path, function() { }); if (err3) { callback("Failed to upload image"); } else { - callback(true, id + "." + detectedExt); + callback(true, safeId + "." + detectedExt); } }); } diff --git a/plugins/star-rating/api/image-utils.js b/plugins/star-rating/api/image-utils.js index 3a7a649cc40..762ee4cb552 100644 --- a/plugins/star-rating/api/image-utils.js +++ b/plugins/star-rating/api/image-utils.js @@ -53,7 +53,30 @@ function parseFeedbackLogoName(name) { return {valid: true, isGlobal: !m[1], appId: m[1] || null}; } +// Allowed logo identifiers. The dashboard sends Date.now() as the identifier, so a plain +// filename fragment covers every real upload. This matters because the identifier is +// concatenated into the upload path: separators or leading dots in it would choose where +// the file lands rather than just what it is called. Kept here beside +// parseFeedbackLogoName, and dependency free so this module stays unit testable. +var LOGO_IDENTIFIER_RE = /^[A-Za-z0-9_-]{1,64}$/; + +/** + * Validate a logo upload identifier, which becomes the stored file's name. + * @param {string|number} id - candidate identifier, straight from the request + * @returns {string|null} the identifier when it is a plain name, otherwise null + */ +function safeLogoIdentifier(id) { + if (typeof id === "number" && isFinite(id)) { + id = String(id); + } + if (typeof id !== "string" || !LOGO_IDENTIFIER_RE.test(id)) { + return null; + } + return id; +} + module.exports = { sniffImageType: sniffImageType, - parseFeedbackLogoName: parseFeedbackLogoName + parseFeedbackLogoName: parseFeedbackLogoName, + safeLogoIdentifier: safeLogoIdentifier }; diff --git a/test/unit-tests/star-rating.image-utils.js b/test/unit-tests/star-rating.image-utils.js index 73aeb10036c..8f937760f89 100644 --- a/test/unit-tests/star-rating.image-utils.js +++ b/test/unit-tests/star-rating.image-utils.js @@ -184,3 +184,43 @@ describe("star-rating image-utils", function() { }); }); }); + +// The logo upload identifier becomes the name of the stored file, and it used to be +// concatenated into the upload path unchecked. These cases cover the shapes that would +// have chosen a write location instead of a file name. +describe("safeLogoIdentifier", function() { + it("keeps the identifier the dashboard actually sends", function() { + // the dropzone sends Date.now() + imageUtils.safeLogoIdentifier(1755100000000).should.equal("1755100000000"); + imageUtils.safeLogoIdentifier("1755100000000").should.equal("1755100000000"); + }); + it("keeps plain names with underscores and dashes", function() { + imageUtils.safeLogoIdentifier("feedback_logo").should.equal("feedback_logo"); + imageUtils.safeLogoIdentifier("my-logo_2").should.equal("my-logo_2"); + }); + it("refuses traversal out of the images directory", function() { + should.not.exist(imageUtils.safeLogoIdentifier("../../../../../../../../tmp/final_poc")); + should.not.exist(imageUtils.safeLogoIdentifier("../../frontend/express/public/appimages/6a41837e902bfd5369ddc610")); + should.not.exist(imageUtils.safeLogoIdentifier("..")); + should.not.exist(imageUtils.safeLogoIdentifier("..%2f..%2ftmp%2fx")); + }); + it("refuses separators and absolute paths in any form", function() { + should.not.exist(imageUtils.safeLogoIdentifier("/tmp/x")); + should.not.exist(imageUtils.safeLogoIdentifier("sub/dir")); + should.not.exist(imageUtils.safeLogoIdentifier("sub\\dir")); + should.not.exist(imageUtils.safeLogoIdentifier("C:x")); + }); + it("refuses names that are not plain identifiers", function() { + should.not.exist(imageUtils.safeLogoIdentifier("")); + should.not.exist(imageUtils.safeLogoIdentifier(".")); + should.not.exist(imageUtils.safeLogoIdentifier("a.b")); // the extension is server chosen + should.not.exist(imageUtils.safeLogoIdentifier("a b")); + // a NUL is the classic path truncation trick, so pin it explicitly + should.not.exist(imageUtils.safeLogoIdentifier("a\u0000b")); + should.not.exist(imageUtils.safeLogoIdentifier("a\u0009b")); + should.not.exist(imageUtils.safeLogoIdentifier(undefined)); + should.not.exist(imageUtils.safeLogoIdentifier(null)); + should.not.exist(imageUtils.safeLogoIdentifier({})); + should.not.exist(imageUtils.safeLogoIdentifier(NaN)); + }); +}); From 991686d67c491b08dcf7270e4d47651307d00bde Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 19:23:37 +0300 Subject: [PATCH 2/2] [fix][star-rating] do not let a logo upload replace another app's logo 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. --- plugins/star-rating/api/api.js | 65 +++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 17 deletions(-) diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 353ce57b936..060d44220c7 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -258,9 +258,10 @@ var SNIFFED_TYPE_TO_EXT = { * Used for file upload * @param {object} myfile - file object(if empty - returns) * @param {string} id - unique identifier +* @param {string} appId - id of the app the caller was authorized for * @param {function} callback = callback function **/ -function uploadFile(myfile, id, callback) { +function uploadFile(myfile, id, appId, callback) { if (!myfile) { callback(true); return; @@ -270,7 +271,9 @@ function uploadFile(myfile, id, callback) { //The identifier is request supplied and is concatenated into the path below, so refuse //anything that is not a plain name before it can pick the write location. var safeId = imageUtils.safeLogoIdentifier(id); - if (!safeId) { + //appId comes from the request that was just authorized, so if it is missing something + //upstream changed: refuse rather than compare widgets against the string "undefined" + if (!safeId || !appId) { fs.unlink(tmp_path, function() { }); callback("Invalid identifier"); return; @@ -293,21 +296,49 @@ function uploadFile(myfile, id, callback) { callback("Invalid image format. Must be png, jpeg, or gif"); return; } - try { - var pp = path.resolve(__dirname, './../images/' + safeId + "." + detectedExt); - countlyFs.saveData("star-rating", pp, data, { id: "" + safeId + "." + detectedExt, writeMode: "overwrite" }, function(err3) { + //The images directory is shared by every app and a widget's logo field holds + //just this file name, so the name alone decides whose logo is replaced. This + //request was authorized against the caller's own app, so a name that another + //app's widget already points at is not ours to overwrite. The sibling + ///i/feedback/upload route decodes its target app out of the name for the same + //reason. Matching on the full name including the extension is deliberate: a + //different extension is a different file and overwrites nothing. + var storedName = safeId + "." + detectedExt; + common.db.collection('feedback_widgets').findOne({logo: storedName, app_id: {$ne: appId + ""}}, {projection: {_id: 1}}, function(ownerErr, otherAppWidget) { + if (ownerErr) { fs.unlink(tmp_path, function() { }); - if (err3) { - callback("Failed to upload image"); - } - else { - callback(true, safeId + "." + detectedExt); - } - }); - } - catch (SyntaxError) { - fs.unlink(tmp_path, function() { }); - callback("Failed to upload image"); + callback("Failed to upload image"); + return; + } + if (otherAppWidget) { + fs.unlink(tmp_path, function() { }); + callback("Identifier is in use by another application"); + return; + } + doSave(); + }); + + /** + * Store the image once the name is known to be free + * @returns {void} void + **/ + function doSave() { + try { + var pp = path.resolve(__dirname, './../images/' + safeId + "." + detectedExt); + countlyFs.saveData("star-rating", pp, data, { id: "" + storedName, writeMode: "overwrite" }, function(err3) { + fs.unlink(tmp_path, function() { }); + if (err3) { + callback("Failed to upload image"); + } + else { + callback(true, storedName); + } + }); + } + catch (SyntaxError) { + fs.unlink(tmp_path, function() { }); + callback("Failed to upload image"); + } } }); }); @@ -995,7 +1026,7 @@ function uploadFile(myfile, id, callback) { plugins.register("/i/feedback/logo", function(ob) { var params = ob.params; validateCreate(params, FEATURE_NAME, function() { - uploadFile(params.files.logo, params.qstring.identifier, function(good, filename) { //will return as good if no file + uploadFile(params.files.logo, params.qstring.identifier, params.qstring.app_id, function(good, filename) { //will return as good if no file if (typeof good === 'boolean' && good) { common.returnMessage(params, 200, filename); }