Merge branch 'imprv/improve-link-edit-modal' into fix/enable-switch-radio

CHANGES.md

@@ -1,6 +1,22 @@
 # CHANGES
 
-## v4.2.0-RC
+## v4.2.3-RC
+
+* 
+
+## v4.2.2
+
+* Fix: Consecutive save operations with built-in editor fail
+    * Introduced by v4.2.1
+
+## v4.2.1
+
+* Fix: Consecutive save operations with HackMD fail
+    * Introduced by v4.2.0
+* Fix: Switching theme to kibela fail
+    * Introduced by v4.2.0
+
+## v4.2.0
 
 ### BREAKING CHANGES
 

package.json

@@ -1,6 +1,6 @@
 {
   "name": "growi",
-  "version": "4.2.0-RC",
+  "version": "4.2.3-RC",
   "description": "Team collaboration software using markdown",
   "tags": [
     "wiki",

src/client/js/components/PageEditor.jsx

@@ -166,7 +166,7 @@ class PageEditor extends React.Component {
       // when if created newly
       if (res.pageCreated) {
         logger.info('Page is created', res.page._id);
-        pageContainer.updateStateAfterSave(res.page);
+        pageContainer.updateStateAfterSave(res.page, res.tags, res.revision);
         editorContainer.setState({ grant: res.page.grant });
       }
     }

src/client/js/components/PageEditorByHackmd.jsx

@@ -145,8 +145,10 @@ class PageEditorByHackmd extends React.Component {
       }
 
       this.props.pageContainer.setState({
+        isHackmdDraftUpdatingInRealtime: false,
         hasDraftOnHackmd: false,
         pageIdOnHackmd: res.pageIdOnHackmd,
+        remoteRevisionId: res.revisionIdHackmdSynced,
         revisionIdHackmdSynced: res.revisionIdHackmdSynced,
       });
     }

src/client/js/services/NavigationContainer.js

@@ -115,6 +115,7 @@ export default class NavigationContainer extends Container {
     if (editorMode === 'edit') {
       $('body').addClass('on-edit');
       $('body').addClass('builtin-editor');
+      $('body').removeClass('hackmd');
       window.location.hash = '#edit';
     }
 

src/client/js/services/PageContainer.js

@@ -339,19 +339,20 @@ export default class PageContainer extends Container {
    * save success handler
    * @param {object} page Page instance
    * @param {Array[Tag]} tags Array of Tag
+   * @param {object} revision Revision instance
    */
-  updateStateAfterSave(page, tags) {
+  updateStateAfterSave(page, tags, revision) {
     const { editorMode } = this.navigationContainer.state;
 
     // update state of PageContainer
     const newState = {
       pageId: page._id,
-      revisionId: page.revision._id,
-      revisionCreatedAt: new Date(page.revision.createdAt).getTime() / 1000,
-      remoteRevisionId: page.revision._id,
+      revisionId: revision._id,
+      revisionCreatedAt: new Date(revision.createdAt).getTime() / 1000,
+      remoteRevisionId: revision._id,
       revisionIdHackmdSynced: page.revisionHackmdSynced,
       hasDraftOnHackmd: page.hasDraftOnHackmd,
-      markdown: page.revision.body,
+      markdown: revision.body,
       createdAt: page.createdAt,
       updatedAt: page.updatedAt,
     };
@@ -408,7 +409,7 @@ export default class PageContainer extends Container {
       res = await this.updatePage(pageId, revisionId, markdown, options);
     }
 
-    this.updateStateAfterSave(res.page, res.tags);
+    this.updateStateAfterSave(res.page, res.tags, res.revision);
     return res;
   }
 
@@ -471,7 +472,7 @@ export default class PageContainer extends Container {
     if (!res.ok) {
       throw new Error(res.error);
     }
-    return { page: res.page, tags: res.tags };
+    return res;
   }
 
   async updatePage(pageId, revisionId, markdown, tmpParams) {
@@ -489,7 +490,7 @@ export default class PageContainer extends Container {
     if (!res.ok) {
       throw new Error(res.error);
     }
-    return { page: res.page, tags: res.tags };
+    return res;
   }
 
   deletePage(isRecursively, isCompletely) {

src/server/models/serializers/page-serializer.js

@@ -7,11 +7,6 @@ function depopulate(page, attributeName) {
   }
 }
 
-function depopulateRevisions(page) {
-  depopulate(page, 'revision');
-  depopulate(page, 'revisionHackmdSynced');
-}
-
 function serializeInsecureUserAttributes(page) {
   if (page.lastUpdateUser != null && page.lastUpdateUser._id != null) {
     page.lastUpdateUser = serializeUserSecurely(page.lastUpdateUser);
@@ -25,7 +20,7 @@ function serializeInsecureUserAttributes(page) {
   return page;
 }
 
-function serializePageSecurely(page, shouldDepopulateRevisions = false) {
+function serializePageSecurely(page) {
   let serialized = page;
 
   // invoke toObject if page is a model instance
@@ -33,10 +28,9 @@ function serializePageSecurely(page, shouldDepopulateRevisions = false) {
     serialized = page.toObject();
   }
 
-  // optional depopulation
-  if (shouldDepopulateRevisions) {
-    depopulateRevisions(serialized);
-  }
+  // depopulate revision and revisionHackmdSynced
+  depopulate(serialized, 'revision');
+  depopulate(serialized, 'revisionHackmdSynced');
 
   serializeInsecureUserAttributes(serialized);
 

src/server/models/serializers/revision-serializer.js

@@ -0,0 +1,20 @@
+const { serializeUserSecurely } = require('./user-serializer');
+
+function serializeInsecureUserAttributes(revision) {
+  if (revision.author != null && revision.author._id != null) {
+    revision.author = serializeUserSecurely(revision.author);
+  }
+  return revision;
+}
+
+function serializeRevisionSecurely(revision) {
+  const serialized = revision;
+
+  serializeInsecureUserAttributes(serialized);
+
+  return serialized;
+}
+
+module.exports = {
+  serializeRevisionSecurely,
+};

src/server/models/vo/s2c-message.js

@@ -7,7 +7,7 @@ class S2cMessagePageUpdated {
 
 
   constructor(page, user) {
-    const serializedPage = serializePageSecurely(page, true);
+    const serializedPage = serializePageSecurely(page);
 
     const {
       _id, revision, revisionHackmdSynced, hasDraftOnHackmd,

src/server/routes/admin.js

@@ -22,7 +22,7 @@ module.exports = function(crowi, app) {
   const MAX_PAGE_LIST = 50;
   const actions = {};
 
-  const { check } = require('express-validator');
+  const { check, param } = require('express-validator');
 
   const api = {};
 
@@ -316,13 +316,29 @@ module.exports = function(crowi, app) {
 
   // Export management
   actions.export = {};
+  actions.export.api = api;
+  api.validators.export = {};
+
   actions.export.index = (req, res) => {
     return res.render('admin/export');
   };
 
+  api.validators.export.download = function() {
+    const validator = [
+      // https://regex101.com/r/mD4eZs/4
+      // prevent from pass traversal attack
+      param('fileName').not().matches(/(\.\.\/|\.\.\\)/),
+    ];
+    return validator;
+  };
+
   actions.export.download = (req, res) => {
-    // TODO: add express validator
     const { fileName } = req.params;
+    const { validationResult } = require('express-validator');
+    const errors = validationResult(req);
+    if (!errors.isEmpty()) {
+      return res.status(422).json({ errors: `${fileName} is invalid. Do not use path like '../'.` });
+    }
 
     try {
       const zipFile = exportService.getFile(fileName);

src/server/routes/apiv3/export.js

@@ -4,6 +4,7 @@ const logger = loggerFactory('growi:routes:apiv3:export');
 const fs = require('fs');
 
 const express = require('express');
+const { param } = require('express-validator');
 
 const router = express.Router();
 
@@ -41,6 +42,7 @@ module.exports = (crowi) => {
   const accessTokenParser = require('../../middlewares/access-token-parser')(crowi);
   const loginRequired = require('../../middlewares/login-required')(crowi);
   const adminRequired = require('../../middlewares/admin-required')(crowi);
+  const apiV3FormValidator = require('../../middlewares/apiv3-form-validator')(crowi);
   const csrf = require('../../middlewares/csrf')(crowi);
 
   const { exportService, socketIoService } = crowi;
@@ -58,6 +60,14 @@ module.exports = (crowi) => {
     socketIoService.getAdminSocket().emit('admin:onTerminateForExport', data);
   });
 
+  const validator = {
+    deleteFile: [
+      // https://regex101.com/r/mD4eZs/4
+      // prevent from unexpecting attack doing delete file (path traversal attack)
+      param('fileName').not().matches(/(\.\.\/|\.\.\\)/),
+    ],
+  };
+
 
   /**
    * @swagger
@@ -150,7 +160,7 @@ module.exports = (crowi) => {
    *              schema:
    *                type: object
    */
-  router.delete('/:fileName', accessTokenParser, loginRequired, adminRequired, csrf, async(req, res) => {
+  router.delete('/:fileName', accessTokenParser, loginRequired, adminRequired, validator.deleteFile, apiV3FormValidator, csrf, async(req, res) => {
     // TODO: add express validator
     const { fileName } = req.params;
 

src/server/routes/apiv3/mongo.js

@@ -14,6 +14,9 @@ const router = express.Router();
  */
 
 module.exports = (crowi) => {
+  const loginRequiredStrictly = require('../../middlewares/login-required')(crowi);
+  const adminRequired = require('../../middlewares/admin-required')(crowi);
+
   /**
    * @swagger
    *
@@ -35,7 +38,7 @@ module.exports = (crowi) => {
    *                    items:
    *                      type: string
    */
-  router.get('/collections', async(req, res) => {
+  router.get('/collections', loginRequiredStrictly, adminRequired, async(req, res) => {
     const listCollectionsResult = await mongoose.connection.db.listCollections().toArray();
     const collections = listCollectionsResult.map(collectionObj => collectionObj.name);
 

src/server/routes/attachment.js

@@ -3,6 +3,9 @@
 
 const logger = require('@alias/logger')('growi:routes:attachment');
 
+const { serializePageSecurely } = require('../models/serializers/page-serializer');
+const { serializeRevisionSecurely } = require('../models/serializers/revision-serializer');
+
 const ApiResponse = require('../util/apiResponse');
 
 /**
@@ -466,7 +469,8 @@ module.exports = function(crowi, app) {
     }
 
     const result = {
-      page: page.toObject(),
+      page: serializePageSecurely(page),
+      revision: serializeRevisionSecurely(page.revision),
       attachment: attachment.toObject({ virtuals: true }),
       pageCreated,
     };

src/server/routes/index.js

@@ -111,7 +111,7 @@ module.exports = function(crowi, app) {
 
   // export management for admin
   app.get('/admin/export'                       , loginRequiredStrictly , adminRequired ,admin.export.index);
-  app.get('/admin/export/:fileName'             , loginRequiredStrictly , adminRequired ,admin.export.download);
+  app.get('/admin/export/:fileName'             , loginRequiredStrictly , adminRequired ,admin.export.api.validators.export.download(), admin.export.download);
 
   app.get('/me'                       , loginRequiredStrictly , me.index);
   // external-accounts

src/server/routes/page.js

@@ -1,4 +1,5 @@
 const { serializePageSecurely } = require('../models/serializers/page-serializer');
+const { serializeRevisionSecurely } = require('../models/serializers/revision-serializer');
 
 /**
  * @swagger
@@ -728,6 +729,8 @@ module.exports = function(crowi, app) {
    *                      $ref: '#/components/schemas/V1Response/properties/ok'
    *                    page:
    *                      $ref: '#/components/schemas/Page'
+   *                    revision:
+   *                      $ref: '#/components/schemas/Revision'
    *          403:
    *            $ref: '#/components/responses/403'
    *          500:
@@ -781,7 +784,11 @@ module.exports = function(crowi, app) {
       savedTags = await PageTagRelation.listTagNamesByPage(createdPage.id);
     }
 
-    const result = { page: serializePageSecurely(createdPage), tags: savedTags };
+    const result = {
+      page: serializePageSecurely(createdPage),
+      revision: serializeRevisionSecurely(createdPage.revision),
+      tags: savedTags,
+    };
     res.json(ApiResponse.success(result));
 
     // update scopes for descendants
@@ -840,6 +847,8 @@ module.exports = function(crowi, app) {
    *                      $ref: '#/components/schemas/V1Response/properties/ok'
    *                    page:
    *                      $ref: '#/components/schemas/Page'
+   *                    revision:
+   *                      $ref: '#/components/schemas/Revision'
    *          403:
    *            $ref: '#/components/responses/403'
    *          500:
@@ -910,7 +919,11 @@ module.exports = function(crowi, app) {
       savedTags = await PageTagRelation.listTagNamesByPage(pageId);
     }
 
-    const result = { page: serializePageSecurely(page), tags: savedTags };
+    const result = {
+      page: serializePageSecurely(page),
+      revision: serializeRevisionSecurely(page.revision),
+      tags: savedTags,
+    };
     res.json(ApiResponse.success(result));
 
     // update scopes for descendants

src/server/service/file-uploader/aws.js

@@ -18,13 +18,9 @@ module.exports = function(crowi) {
     };
   }
 
-  function S3Factory(isUploadable) {
+  function S3Factory() {
     const awsConfig = getAwsConfig();
 
-    if (!isUploadable) {
-      throw new Error('AWS is not configured.');
-    }
-
     aws.config.update({
       accessKeyId: awsConfig.accessKeyId,
       secretAccessKey: awsConfig.secretAccessKey,
@@ -83,7 +79,11 @@ module.exports = function(crowi) {
   };
 
   lib.deleteFileByFilePath = async function(filePath) {
-    const s3 = S3Factory(this.getIsUploadable());
+    if (!this.getIsUploadable()) {
+      throw new Error('AWS is not configured.');
+    }
+
+    const s3 = S3Factory();
     const awsConfig = getAwsConfig();
 
     const params = {
@@ -102,9 +102,13 @@ module.exports = function(crowi) {
   };
 
   lib.uploadFile = function(fileStream, attachment) {
+    if (!this.getIsUploadable()) {
+      throw new Error('AWS is not configured.');
+    }
+
     logger.debug(`File uploading: fileName=${attachment.fileName}`);
 
-    const s3 = S3Factory(this.getIsUploadable());
+    const s3 = S3Factory();
     const awsConfig = getAwsConfig();
 
     const filePath = getFilePathOnStorage(attachment);
@@ -126,7 +130,11 @@ module.exports = function(crowi) {
    * @return {stream.Readable} readable stream
    */
   lib.findDeliveryFile = async function(attachment) {
-    const s3 = S3Factory(this.getIsUploadable());
+    if (!this.getIsReadable()) {
+      throw new Error('AWS is not configured.');
+    }
+
+    const s3 = S3Factory();
     const awsConfig = getAwsConfig();
     const filePath = getFilePathOnStorage(attachment);
 

src/server/service/file-uploader/gcs.js

@@ -15,10 +15,7 @@ module.exports = function(crowi) {
     return configManager.getConfig('crowi', 'gcs:bucket');
   }
 
-  function getGcsInstance(isUploadable) {
-    if (!isUploadable) {
-      throw new Error('GCS is not configured.');
-    }
+  function getGcsInstance() {
     if (_instance == null) {
       const keyFilename = configManager.getConfig('crowi', 'gcs:apiKeyJsonPath');
       // see https://googleapis.dev/nodejs/storage/latest/Storage.html
@@ -59,7 +56,11 @@ module.exports = function(crowi) {
   };
 
   lib.deleteFileByFilePath = async function(filePath) {
-    const gcs = getGcsInstance(this.getIsUploadable());
+    if (!this.getIsUploadable()) {
+      throw new Error('GCS is not configured.');
+    }
+
+    const gcs = getGcsInstance();
     const myBucket = gcs.bucket(getGcsBucket());
     const file = myBucket.file(filePath);
 
@@ -74,9 +75,13 @@ module.exports = function(crowi) {
   };
 
   lib.uploadFile = function(fileStream, attachment) {
+    if (!this.getIsUploadable()) {
+      throw new Error('GCS is not configured.');
+    }
+
     logger.debug(`File uploading: fileName=${attachment.fileName}`);
 
-    const gcs = getGcsInstance(this.getIsUploadable());
+    const gcs = getGcsInstance();
     const myBucket = gcs.bucket(getGcsBucket());
     const filePath = getFilePathOnStorage(attachment);
     const options = {
@@ -93,7 +98,11 @@ module.exports = function(crowi) {
    * @return {stream.Readable} readable stream
    */
   lib.findDeliveryFile = async function(attachment) {
-    const gcs = getGcsInstance(this.getIsUploadable());
+    if (!this.getIsReadable()) {
+      throw new Error('GCS is not configured.');
+    }
+
+    const gcs = getGcsInstance();
     const myBucket = gcs.bucket(getGcsBucket());
     const filePath = getFilePathOnStorage(attachment);
     const file = myBucket.file(filePath);

src/server/service/file-uploader/uploader.js

@@ -12,6 +12,11 @@ class Uploader {
     return !this.configManager.getConfig('crowi', 'app:fileUploadDisabled') && this.isValidUploadSettings();
   }
 
+  // File reading is possible even if uploading is disabled
+  getIsReadable() {
+    return this.isValidUploadSettings();
+  }
+
   isValidUploadSettings() {
     throw new Error('Implement this');
   }

src/server/service/import.js

@@ -369,6 +369,14 @@ class ImportService {
 
     unzipStream.on('entry', (entry) => {
       const fileName = entry.path;
+      // https://regex101.com/r/mD4eZs/4
+      // prevent from unexpecting attack doing unzip file (path traversal attack)
+      // FOR EXAMPLE
+      // ../../src/server/views/admin/markdown.html
+      if (fileName.match(/(\.\.\/|\.\.\\)/)) {
+        logger.error('File path is not appropriate.', fileName);
+        return;
+      }
 
       if (fileName === this.growiBridgeService.getMetaFileName()) {
         // skip meta.json