[fix][star-rating] keep the uploaded logo name inside the images directory

plugins/star-rating/api/api.js

@@ -258,15 +258,27 @@ 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;
     }
     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);
+    //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;
+    }
+
     create_upload_dir(function() {
         fs.readFile(tmp_path, (err, data) => {
             if (err || !data) {
@@ -284,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/' + id + "." + detectedExt);
-                countlyFs.saveData("star-rating", pp, data, { id: "" + id + "." + 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, id + "." + 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");
+                }
             }
         });
     });
@@ -986,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);
                 }

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
 };

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));
+    });
+});