diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index c3626c52fd4..060d44220c7 100644 --- a/plugins/star-rating/api/api.js +++ b/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); } 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)); + }); +});