fix(api): refuse file uploads unless a plugin declared the path

api/api.js

@@ -18,10 +18,15 @@ const versionInfo = require('../frontend/express/version.info.js');
 const moment = require("moment");
 const tracker = require('./parts/mgmt/tracker.js');
 const { RateLimiterMemory } = require("rate-limiter-flexible");
+const uploadTemp = require('./utils/upload-temp.js');
 
 var t = ["countly:", "api"];
 common.processRequest = processRequest;
 
+// Core endpoints that read params.files. Plugins declare their own.
+plugins.uploadPaths.push({path: "/i/apps/create"});
+plugins.uploadPaths.push({path: "/i/apps/update"});
+
 if (cluster.isMaster) {
     console.log("Starting Countly", "version", versionInfo.version, "package", pack.version);
     if (!common.checkDatabaseConfigMatch(countlyConfig.mongodb, frontendConfig.mongodb)) {
@@ -482,6 +487,27 @@ function handleRequest(req, res) {
         if (countlyConfig.api.maxUploadFileSize) {
             formidableOptions.maxFileSize = countlyConfig.api.maxUploadFileSize;
         }
+        // The body is parsed before the request is routed or authorized, so
+        // uploads are refused unless a plugin declared that this exact path
+        // reads params.files
+        Object.assign(formidableOptions, uploadTemp.parseOptions(req.url, common.config && common.config.path, plugins.uploadPaths));
+
+        // Whatever did get written is removed once the response is done with.
+        // 'close' matters as well as 'finish': an upload aborted mid-stream
+        // leaves a partial file that 'finish' never sees. Handlers that consume
+        // an upload unlink it themselves; this only catches what they did not,
+        // which is every request rejected before reaching a handler at all.
+        /**
+         * Remove any upload temp file the handlers did not consume
+         * @returns {void}
+         */
+        const discardOnce = () => {
+            res.removeListener('finish', discardOnce);
+            res.removeListener('close', discardOnce);
+            uploadTemp.discardUploads(params);
+        };
+        res.on('finish', discardOnce);
+        res.on('close', discardOnce);
 
         const form = new formidable.IncomingForm(formidableOptions);
         if (/crash_symbols\/(add_symbol|upload_symbol)/.test(req.url)) {
@@ -517,6 +543,9 @@ function handleRequest(req, res) {
                 }
             }
             params.files = files;
+            //snapshot the paths formidable produced, before a handler can
+            //repoint params.files[x].path at something that must not be removed
+            uploadTemp.trackUploads(params, files);
             if (multiFormData) {
                 let formDataUrl = [];
                 for (const i in fields) {

api/parts/mgmt/apps.js

@@ -320,8 +320,15 @@ appsApi.createApp = async function(params) {
                     appId: app.ops[0]._id,
                     data: newApp
                 });
-                iconUpload(Object.assign({}, params, {app_id: app.ops[0]._id}));
-                common.returnOutput(params, newApp);
+                //respond only once the icon has been processed, so the uploaded
+                //temp file is not removed while jimp is still reading it
+                iconUpload(Object.assign({}, params, {app_id: app.ops[0]._id}))
+                    .catch(function() {
+                        //iconUpload logs its own failures, the app was created either way
+                    })
+                    .then(function() {
+                        common.returnOutput(params, newApp);
+                    });
             }
             else {
                 common.returnMessage(params, 500, "Error creating App: " + err);
@@ -497,8 +504,15 @@ appsApi.updateApp = function(params) {
                                 update: updatedApp
                             }
                         });
-                        iconUpload(params);
-                        common.returnOutput(params, updatedApp);
+                        //respond only once the icon has been processed, so the
+                        //uploaded temp file is not removed while jimp is reading it
+                        iconUpload(params)
+                            .catch(function() {
+                                //iconUpload logs its own failures, the app was updated either way
+                            })
+                            .then(function() {
+                                common.returnOutput(params, updatedApp);
+                            });
                     });
                 }
                 else {

api/utils/upload-temp.js

@@ -0,0 +1,158 @@
+/**
+ * @module api/utils/upload-temp
+ * @description Bounds what the formidable parse in api/api.js is allowed to
+ * write to disk, and removes whatever it wrote that no handler consumed.
+ *
+ * api/api.js parses every POST body with formidable, before the request is
+ * routed or authorized. formidable writes multipart parts and raw
+ * application/octet-stream bodies to disk, so without a gate any POST carrying
+ * such a body leaves a file behind - including requests to paths that no
+ * handler serves. This module provides:
+ *
+ *  - parseOptions: formidable options that allow a file to be written only
+ *    where a plugin declared that the path reads params.files
+ *  - discardUploads: removal of the files a request left behind
+ */
+
+'use strict';
+
+const fs = require('fs');
+
+/**
+ * Strip the query string and the installation subpath from a request url.
+ * requestProcessor strips the subpath before routing, so it has to be stripped
+ * here too or uploads would be refused on subdirectory installs.
+ * @param {string} url - request url, query string included
+ * @param {string} [installPath] - installation subpath (common.config.path)
+ * @returns {string} the path as requestProcessor will see it
+ */
+function normalizePath(url, installPath) {
+    let reqPath = (url || '').split('?')[0];
+
+    if (installPath && installPath !== '/'
+        && (reqPath === installPath || reqPath.indexOf(installPath + '/') === 0)) {
+        reqPath = reqPath.substring(installPath.length) || '/';
+    }
+
+    //so that /i and /i/ are the same path
+    if (reqPath.length > 1 && reqPath.charAt(reqPath.length - 1) === '/') {
+        reqPath = reqPath.substring(0, reqPath.length - 1);
+    }
+
+    return reqPath;
+}
+
+/**
+ * Find the declaration a plugin registered for this path, if any.
+ *
+ * Matching is exact: a declaration for /i must not let /i/anything through,
+ * otherwise declaring the SDK write endpoint would reopen the whole /i tree.
+ * @param {Array} uploadPaths - plugins.uploadPaths
+ * @param {string} reqPath - normalized request path
+ * @returns {object|null} the declaration, or null when uploads are not allowed
+ */
+function findDeclaration(uploadPaths, reqPath) {
+    if (!Array.isArray(uploadPaths) || !reqPath) {
+        return null;
+    }
+
+    for (let i = 0; i < uploadPaths.length; i++) {
+        const entry = uploadPaths[i];
+        const declared = (typeof entry === 'string') ? entry : (entry && entry.path);
+        if (declared && declared === reqPath) {
+            return (typeof entry === 'string') ? {path: entry} : entry;
+        }
+    }
+
+    return null;
+}
+
+/**
+ * formidable options restricting what this request may write to disk.
+ *
+ * Returns the options to merge into the IncomingForm options: an empty object
+ * where uploads are expected, and otherwise the entries that suppress them.
+ * Both suppressions are needed and cover different parsers - `filter` is only
+ * consulted for multipart parts, while formidable's octetstream plugin creates
+ * its file directly via _newFile() and never calls `filter`, so the only way to
+ * stop a raw body reaching disk is to leave that plugin out.
+ *
+ * The remaining parsers are kept enabled either way, so urlencoded and json
+ * bodies still populate params.qstring exactly as before.
+ * @param {string} url - request url, query string included
+ * @param {string} [installPath] - installation subpath (common.config.path)
+ * @param {Array} [uploadPaths] - plugins.uploadPaths, the declared upload paths
+ * @returns {object} options to merge into the IncomingForm options
+ */
+function parseOptions(url, installPath, uploadPaths) {
+    const declaration = findDeclaration(uploadPaths, normalizePath(url, installPath));
+    const options = {};
+
+    if (!declaration) {
+        options.filter = function() {
+            return false;
+        };
+    }
+
+    if (!declaration || !declaration.raw) {
+        options.enabledPlugins = ['querystring', 'multipart', 'json'];
+    }
+
+    return options;
+}
+
+/**
+ * Record the paths formidable produced for this request, so they can be removed
+ * later without trusting params.files.
+ *
+ * The snapshot is taken before any handler runs, because a handler may repoint
+ * params.files[x].path at something else entirely - crash_symbolication points
+ * it at files shipped with the plugin when serving populator data, which must
+ * never be deleted.
+ * @param {object} params - request params
+ * @param {object} files - the files formidable parsed
+ * @returns {void}
+ */
+function trackUploads(params, files) {
+    const tracked = [];
+
+    Object.keys(files || {}).forEach((key) => {
+        const file = files[key];
+        const target = file && (file.filepath || file.path);
+        if (target) {
+            tracked.push(target);
+        }
+    });
+
+    params.uploadTempPaths = tracked;
+}
+
+/**
+ * Remove the files formidable wrote for this request.
+ *
+ * Handlers that consume an upload unlink it themselves, so in the normal case
+ * these are already gone and the unlink is a no-op. What is left is every
+ * request that was rejected before reaching a handler at all, which is not
+ * enumerable at the rejection sites - rights.js alone rejects in 63 places.
+ *
+ * Only the snapshot from trackUploads is touched, never params.files, so a
+ * handler substituting a path cannot redirect this at another file.
+ * @param {object} params - request params
+ * @returns {void}
+ */
+function discardUploads(params) {
+    if (!params || !params.uploadTempPaths || !params.uploadTempPaths.length) {
+        return;
+    }
+
+    params.uploadTempPaths.forEach((target) => {
+        fs.unlink(target, function() {});
+    });
+    params.uploadTempPaths = [];
+}
+
+module.exports = {
+    parseOptions,
+    trackUploads,
+    discardUploads
+};

plugins/pluginManager.js

@@ -79,6 +79,20 @@ var pluginManager = function pluginManager() {
      * @type {{collection: string, db: mongodb.Db, property: string, expireAfterSeconds: number}[]}
      */
     this.ttlCollections = [];
+    /**
+     * Request paths that are allowed to carry a file upload.
+     *
+     * The API parses POST bodies before a request is routed or authorized, and
+     * formidable writes multipart parts and raw application/octet-stream bodies
+     * to disk. Uploads are therefore refused by default and a plugin has to
+     * declare the paths where it actually reads params.files, so a body sent
+     * anywhere else is never written to disk. Set raw when the endpoint reads an
+     * application/octet-stream body rather than a multipart part.
+     *
+     * Paths are matched exactly, after the installation subpath is stripped.
+     * @type {{path: string, raw: boolean=}[]}
+     */
+    this.uploadPaths = [];
     /**
      *  Custom configuration files for different databases for docker env
      */

plugins/star-rating/api/api.js

@@ -488,6 +488,8 @@ function uploadFile(myfile, id, callback) {
      *  "result": "Missing parameter "api_key" or "auth_token""
      * }
     */
+    //also declared by the surveys plugin, which serves this path when enabled
+    plugins.uploadPaths.push({path: "/i/feedback/upload"});
     plugins.register("/i/feedback/upload", function(ob) {
         // do not respond if this isn't feedback fetch request
         // or surveys plugin enabled
@@ -980,6 +982,7 @@ function uploadFile(myfile, id, callback) {
     *  "result": "Missing parameter \"api_key\" or \"auth_token\""" 
     * }
     */
+    plugins.uploadPaths.push({path: "/i/feedback/logo"});
     plugins.register("/i/feedback/logo", function(ob) {
         var params = ob.params;
         validateCreate(params, FEATURE_NAME, function() {