Merge pull request #827 from weseek/imprv/705-restrict-access-to-attachments

Imprv/705 restrict access to attachments

package.json

@@ -101,6 +101,7 @@
     "mongoose-paginate": "^5.0.3",
     "mongoose-unique-validator": "^2.0.2",
     "multer": "~1.4.0",
+    "multer-autoreap": "^1.0.3",
     "nodemailer": "^5.1.1",
     "nodemailer-ses-transport": "~1.5.0",
     "npm-run-all": "^4.1.2",

src/server/models/attachment.js

@@ -90,11 +90,10 @@ module.exports = function(crowi) {
   };
 
   attachmentSchema.statics.removeWithSubstanceById = async function(id) {
-    // retrieve data from DB
-    // because this instance fields are only partially populated
+    // retrieve data from DB to get a completely populated instance
     const attachment = await this.findById(id);
     await fileUploader.deleteFile(attachment);
-    return await this.remove();
+    return await attachment.remove();
   };
 
   return mongoose.model('Attachment', attachmentSchema);

src/server/routes/attachment.js

@@ -13,14 +13,53 @@ module.exports = function(crowi, app) {
   const fileUploader = require('../service/file-uploader')(crowi, app);
 
 
+  /**
+   * Check the user is accessible to the related page
+   *
+   * @param {User} user
+   * @param {Attachment} attachment
+   */
+  async function isAccessibleByViewer(user, attachment) {
+    if (attachment.page != null) {
+      return await Page.isAccessiblePageByViewer(attachment.page, user);
+    }
+    return true;
+  }
+
+  /**
+   * Check the user is accessible to the related page
+   *
+   * @param {User} user
+   * @param {Attachment} attachment
+   */
+  async function isDeletableByUser(user, attachment) {
+    const ownerId = attachment.creator._id || attachment.creator;
+    if (attachment.page == null) {  // when profile image
+      return user.id === ownerId.toString();
+    }
+    else {
+      return await Page.isAccessiblePageByViewer(attachment.page, user);
+    }
+  }
+
   /**
    * Common method to response
    *
    * @param {Response} res
+   * @param {User} user
    * @param {Attachment} attachment
    * @param {boolean} forceDownload
    */
-  async function responseForAttachment(res, attachment, forceDownload) {
+  async function responseForAttachment(res, user, attachment, forceDownload) {
+    if (attachment == null) {
+      return res.json(ApiResponse.error('attachment not found'));
+    }
+
+    const isAccessible = await isAccessibleByViewer(user, attachment);
+    if (!isAccessible) {
+      return res.json(ApiResponse.error(`Forbidden to access to the attachment '${attachment.id}'`));
+    }
+
     let fileStream;
     try {
       fileStream = await fileUploader.findDeliveryFile(attachment);
@@ -91,13 +130,7 @@ module.exports = function(crowi, app) {
 
     const attachment = await Attachment.findById(id);
 
-    if (attachment == null) {
-      return res.json(ApiResponse.error('attachment not found'));
-    }
-
-    // TODO for GC-1359: consider restriction
-
-    return responseForAttachment(res, attachment, true);
+    return responseForAttachment(res, req.user, attachment, true);
   };
 
   /**
@@ -112,13 +145,7 @@ module.exports = function(crowi, app) {
 
     const attachment = await Attachment.findById(id);
 
-    if (attachment == null) {
-      return res.json(ApiResponse.error('attachment not found'));
-    }
-
-    // TODO for GC-1359: consider restriction
-
-    return responseForAttachment(res, attachment);
+    return responseForAttachment(res, req.user, attachment);
   };
 
   /**
@@ -139,13 +166,7 @@ module.exports = function(crowi, app) {
 
     const attachment = await Attachment.findOne({ filePath });
 
-    if (attachment == null) {
-      return res.json(ApiResponse.error('attachment not found'));
-    }
-
-    // TODO for GC-1359: consider restriction
-
-    return responseForAttachment(res, attachment);
+    return responseForAttachment(res, req.user, attachment);
   };
 
   /**
@@ -214,6 +235,12 @@ module.exports = function(crowi, app) {
     }
     else {
       page = await Page.findById(pageId);
+
+      // check the user is accessible
+      const isAccessible = await Page.isAccessiblePageByViewer(page.id, req.user);
+      if (!isAccessible) {
+        return res.json(ApiResponse.error(`Forbidden to access to the page '${page.id}'`));
+      }
     }
 
     let attachment;
@@ -286,6 +313,17 @@ module.exports = function(crowi, app) {
   api.remove = async function(req, res) {
     const id = req.body.attachment_id;
 
+    const attachment = await Attachment.findById(id);
+
+    if (attachment == null) {
+      return res.json(ApiResponse.error('attachment not found'));
+    }
+
+    const isDeletable = await isDeletableByUser(req.user, attachment);
+    if (!isDeletable) {
+      return res.json(ApiResponse.error(`Forbidden to remove the attachment '${attachment.id}'`));
+    }
+
     try {
       await Attachment.removeWithSubstanceById(id);
     }

src/server/routes/index.js

@@ -1,6 +1,9 @@
+const multer = require('multer')
+const autoReap  = require('multer-autoreap');
+autoReap.options.reapOnError = true;  // continue reaping the file even if an error occurs
+
 module.exports = function(crowi, app) {
   const middleware = require('../util/middlewares')
-    , multer    = require('multer')
     , uploads   = multer({dest: crowi.tmpDir + 'uploads'})
     , form      = require('../form')
     , page      = require('./page')(crowi, app)
@@ -208,8 +211,8 @@ module.exports = function(crowi, app) {
   app.post('/_api/likes.add'          , accessTokenParser , loginRequired(crowi, app) , csrf, page.api.like);
   app.post('/_api/likes.remove'       , accessTokenParser , loginRequired(crowi, app) , csrf, page.api.unlike);
   app.get( '/_api/attachments.list'   , accessTokenParser , loginRequired(crowi, app, false) , attachment.api.list);
-  app.post('/_api/attachments.add'                  , uploads.single('file'), accessTokenParser, loginRequired(crowi, app) ,csrf, attachment.api.add);
-  app.post('/_api/attachments.uploadProfileImage'   , uploads.single('file'), accessTokenParser, loginRequired(crowi, app) ,csrf, attachment.api.uploadProfileImage);
+  app.post('/_api/attachments.add'                  , uploads.single('file'), autoReap, accessTokenParser, loginRequired(crowi, app) ,csrf, attachment.api.add);
+  app.post('/_api/attachments.uploadProfileImage'   , uploads.single('file'), autoReap, accessTokenParser, loginRequired(crowi, app) ,csrf, attachment.api.uploadProfileImage);
   app.post('/_api/attachments.remove' , accessTokenParser , loginRequired(crowi, app) , csrf, attachment.api.remove);
   app.get( '/_api/attachments.limit'  , accessTokenParser , loginRequired(crowi, app) , csrf, attachment.api.limit);
 

src/server/views/me/index.html

@@ -158,8 +158,7 @@
             </p>
             <p>
             <form id="remove-attachment" action="/_api/attachments.remove" method="post" class="form-horizontal"
-                style="{% if not user.imageAttachment %}display: none{% endif %}"
-                onsubmit="return window.confirm('{{ t('Delete this image?') }}');">
+                style="{% if not user.imageAttachment %}display: none{% endif %}">
               <input type="hidden" name="_csrf" value="{{ csrf() }}">
               <input type="hidden" name="attachment_id" value="{{ user.imageAttachment.id }}">
               <button type="submit" class="btn btn-danger">{{ t('Delete Image') }}</button>

yarn.lock

@@ -3126,6 +3126,11 @@ es-to-primitive@^1.1.1:
     is-date-object "^1.0.1"
     is-symbol "^1.0.1"
 
+es6-object-assign@^1.1.0:
+  version "1.1.0"
+  resolved "https://registry.yarnpkg.com/es6-object-assign/-/es6-object-assign-1.1.0.tgz#c2c3582656247c39ea107cb1e6652b6f9f24523c"
+  integrity sha1-wsNYJlYkfDnqEHyx5mUrb58kUjw=
+
 es6-promise@3.2.1:
   version "3.2.1"
   resolved "https://registry.yarnpkg.com/es6-promise/-/es6-promise-3.2.1.tgz#ec56233868032909207170c39448e24449dd1fc4"
@@ -6057,6 +6062,15 @@ ms@^2.0.0:
   version "2.1.1"
   resolved "https://registry.yarnpkg.com/ms/-/ms-2.1.1.tgz#30a5864eb3ebb0a66f2ebe6d727af06a09d86e0a"
 
+multer-autoreap@^1.0.3:
+  version "1.0.3"
+  resolved "https://registry.yarnpkg.com/multer-autoreap/-/multer-autoreap-1.0.3.tgz#a50aaeb713fa9407ac940807f6c112c6ce9df280"
+  integrity sha512-g0wISfylN2bchQglyAgQTIHoiLUcYQTXKmQh+fKJpheGay9aDqHmcMYRwWRNJ+tK95j9/NZ5QNFkqRytrgw34g==
+  dependencies:
+    debug "^3.1.0"
+    es6-object-assign "^1.1.0"
+    on-finished "^2.3.0"
+
 multer@~1.4.0:
   version "1.4.0"
   resolved "https://registry.yarnpkg.com/multer/-/multer-1.4.0.tgz#c951616c3f97a709b6294acec3a9952a10d33159"