From ad09630d4ef5a759cc975db241c79b6c94c05129 Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Thu, 31 Aug 2017 22:02:11 +0100 Subject: use beforeWrite method of CollectionFS instead of collection-hooks --- models/attachments.js | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/models/attachments.js b/models/attachments.js index 1c9878c7..3e5d4437 100644 --- a/models/attachments.js +++ b/models/attachments.js @@ -3,7 +3,24 @@ Attachments = new FS.Collection('attachments', { // XXX Add a new store for cover thumbnails so we don't load big images in // the general board view - new FS.Store.GridFS('attachments'), + new FS.Store.GridFS('attachments', { + // If the uploaded document is not an image we need to enforce browser + // download instead of execution. This is particularly important for HTML + // files that the browser will just execute if we don't serve them with the + // appropriate `application/octet-stream` MIME header which can lead to user + // data leaks. I imagine other formats (like PDF) can also be attack vectors. + // See https://github.com/wekan/wekan/issues/99 + // XXX Should we use `beforeWrite` option of CollectionFS instead of + // collection-hooks? + // We should use `beforeWrite`. + beforeWrite: (fileObj) => { + if (!fileObj.isImage()) { + return { + type: 'application/octet-stream' + }; + } + }, + }), ], }); @@ -36,23 +53,6 @@ if (Meteor.isServer) { // XXX Enforce a schema for the Attachments CollectionFS -Attachments.files.before.insert((userId, doc) => { - const file = new FS.File(doc); - doc.userId = userId; - - // If the uploaded document is not an image we need to enforce browser - // download instead of execution. This is particularly important for HTML - // files that the browser will just execute if we don't serve them with the - // appropriate `application/octet-stream` MIME header which can lead to user - // data leaks. I imagine other formats (like PDF) can also be attack vectors. - // See https://github.com/wekan/wekan/issues/99 - // XXX Should we use `beforeWrite` option of CollectionFS instead of - // collection-hooks? - if (!file.isImage()) { - file.original.type = 'application/octet-stream'; - } -}); - if (Meteor.isServer) { Attachments.files.after.insert((userId, doc) => { Activities.insert({ -- cgit v1.2.3-1-g7c22 From 4ad4c6ea2274919ced917e541120ad50c7512b80 Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Thu, 31 Aug 2017 22:14:57 +0100 Subject: Assign user to attachment before inserting --- client/components/cards/attachments.js | 3 ++- models/trelloCreator.js | 1 + models/wekanCreator.js | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/client/components/cards/attachments.js b/client/components/cards/attachments.js index 4e3e0b19..95cb9f55 100644 --- a/client/components/cards/attachments.js +++ b/client/components/cards/attachments.js @@ -62,7 +62,7 @@ Template.cardAttachmentsPopup.events({ const file = new FS.File(f); file.boardId = card.boardId; file.cardId = card._id; - + file.userId = Meteor.userId(); Attachments.insert(file); Popup.close(); }); @@ -109,6 +109,7 @@ Template.previewClipboardImagePopup.events({ file.updatedAt(new Date()); file.boardId = card.boardId; file.cardId = card._id; + file.userId = Meteor.userId(); Attachments.insert(file); pastedResults = null; $(document.body).pasteImageReader(() => {}); diff --git a/models/trelloCreator.js b/models/trelloCreator.js index 60207546..44030ea5 100644 --- a/models/trelloCreator.js +++ b/models/trelloCreator.js @@ -322,6 +322,7 @@ export class TrelloCreator { file.attachData(att.url, function (error) { file.boardId = boardId; file.cardId = cardId; + file.userId = this._user(att.idMemberCreator); if (error) { throw(error); } else { diff --git a/models/wekanCreator.js b/models/wekanCreator.js index 1a02339a..d96ad2ca 100644 --- a/models/wekanCreator.js +++ b/models/wekanCreator.js @@ -312,6 +312,7 @@ export class WekanCreator { file.attachData(att.url, function (error) { file.boardId = boardId; file.cardId = cardId; + file.userId = this._user(att.userId); if (error) { throw(error); } else { @@ -330,6 +331,7 @@ export class WekanCreator { file.name(att.name); file.boardId = boardId; file.cardId = cardId; + file.userId = this._user(att.userId); if (error) { throw(error); } else { -- cgit v1.2.3-1-g7c22 From 6ff0cf91e2fe6fd0e777225eb7afb3b37ac313e7 Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Sat, 2 Sep 2017 07:17:42 +0100 Subject: Add source field to imported attachments We use this field to prevent adding attachments' related activities automatically only. Then this field will be removed. --- models/attachments.js | 25 +++++++++++++++++-------- models/trelloCreator.js | 3 +++ models/wekanCreator.js | 6 ++++++ 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/models/attachments.js b/models/attachments.js index 3e5d4437..40adda49 100644 --- a/models/attachments.js +++ b/models/attachments.js @@ -55,14 +55,23 @@ if (Meteor.isServer) { if (Meteor.isServer) { Attachments.files.after.insert((userId, doc) => { - Activities.insert({ - userId, - type: 'card', - activityType: 'addAttachment', - attachmentId: doc._id, - boardId: doc.boardId, - cardId: doc.cardId, - }); + // If the attachment doesn't have a source field + // or its source is different than import + if (!doc.source || doc.source !== 'import') { + // Add activity about adding the attachment + Activities.insert({ + userId, + type: 'card', + activityType: 'addAttachment', + attachmentId: doc._id, + boardId: doc.boardId, + cardId: doc.cardId, + }); + } else { + // Don't add activity about adding the attachment as the activity + // be imported and delete source field + Attachments.update( {_id: doc._id} , {$unset: { source : "" } } ); + } }); Attachments.files.after.remove((userId, doc) => { diff --git a/models/trelloCreator.js b/models/trelloCreator.js index 44030ea5..b0e3325b 100644 --- a/models/trelloCreator.js +++ b/models/trelloCreator.js @@ -323,6 +323,9 @@ export class TrelloCreator { file.boardId = boardId; file.cardId = cardId; file.userId = this._user(att.idMemberCreator); + // The field source will only be used to prevent adding + // attachments' related activities automatically + file.source = 'import'; if (error) { throw(error); } else { diff --git a/models/wekanCreator.js b/models/wekanCreator.js index d96ad2ca..3d0a2397 100644 --- a/models/wekanCreator.js +++ b/models/wekanCreator.js @@ -313,6 +313,9 @@ export class WekanCreator { file.boardId = boardId; file.cardId = cardId; file.userId = this._user(att.userId); + // The field source will only be used to prevent adding + // attachments' related activities automatically + file.source = 'import'; if (error) { throw(error); } else { @@ -332,6 +335,9 @@ export class WekanCreator { file.boardId = boardId; file.cardId = cardId; file.userId = this._user(att.userId); + // The field source will only be used to prevent adding + // attachments' related activities automatically + file.source = 'import'; if (error) { throw(error); } else { -- cgit v1.2.3-1-g7c22 From bc4ea6fd9a6e05965ae3977e6453ce1d36ce4078 Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Sat, 2 Sep 2017 13:17:03 +0100 Subject: Add idMemberCreator to Trello attachment We need this field to attribute attachment to the user who uploaded it --- models/trelloCreator.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/models/trelloCreator.js b/models/trelloCreator.js index b0e3325b..33d777be 100644 --- a/models/trelloCreator.js +++ b/models/trelloCreator.js @@ -456,6 +456,8 @@ export class TrelloCreator { // In that case Trello still reports its addition, but removes its 'url' field. // So we test for that const trelloAttachment = action.data.attachment; + // We need the idMemberCreator + trelloAttachment.idMemberCreator = action.idMemberCreator; if(trelloAttachment.url) { // we cannot actually create the Wekan attachment, because we don't yet // have the cards to attach it to, so we store it in the instance variable. -- cgit v1.2.3-1-g7c22 From 255df20da068a203589dba15a5ad249183cd50f5 Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Sat, 2 Sep 2017 13:18:29 +0100 Subject: Put 'this' in 'self' variable For some reason, TrelloCreator didn't keep 'this' reference --- models/trelloCreator.js | 5 +++-- models/wekanCreator.js | 7 ++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/models/trelloCreator.js b/models/trelloCreator.js index 33d777be..4f64edd6 100644 --- a/models/trelloCreator.js +++ b/models/trelloCreator.js @@ -318,11 +318,12 @@ export class TrelloCreator { // - HEAD returns null, which causes exception down the line // - the template then tries to display the url to the attachment which causes other errors // so we make it server only, and let UI catch up once it is done, forget about latency comp. + const self = this; if(Meteor.isServer) { file.attachData(att.url, function (error) { file.boardId = boardId; file.cardId = cardId; - file.userId = this._user(att.idMemberCreator); + file.userId = self._user(att.idMemberCreator); // The field source will only be used to prevent adding // attachments' related activities automatically file.source = 'import'; @@ -332,7 +333,7 @@ export class TrelloCreator { const wekanAtt = Attachments.insert(file, () => { // we do nothing }); - this.attachmentIds[att.id] = wekanAtt._id; + self.attachmentIds[att.id] = wekanAtt._id; // if(trelloCoverId === att.id) { Cards.direct.update(cardId, { $set: {coverId: wekanAtt._id}}); diff --git a/models/wekanCreator.js b/models/wekanCreator.js index 3d0a2397..7e320817 100644 --- a/models/wekanCreator.js +++ b/models/wekanCreator.js @@ -307,12 +307,13 @@ export class WekanCreator { // - HEAD returns null, which causes exception down the line // - the template then tries to display the url to the attachment which causes other errors // so we make it server only, and let UI catch up once it is done, forget about latency comp. + const self = this; if(Meteor.isServer) { if (att.url) { file.attachData(att.url, function (error) { file.boardId = boardId; file.cardId = cardId; - file.userId = this._user(att.userId); + file.userId = self._user(att.userId); // The field source will only be used to prevent adding // attachments' related activities automatically file.source = 'import'; @@ -322,7 +323,7 @@ export class WekanCreator { const wekanAtt = Attachments.insert(file, () => { // we do nothing }); - this.attachmentIds[att._id] = wekanAtt._id; + self.attachmentIds[att._id] = wekanAtt._id; // if(wekanCoverId === att._id) { Cards.direct.update(cardId, { $set: {coverId: wekanAtt._id}}); @@ -334,7 +335,7 @@ export class WekanCreator { file.name(att.name); file.boardId = boardId; file.cardId = cardId; - file.userId = this._user(att.userId); + file.userId = self._user(att.userId); // The field source will only be used to prevent adding // attachments' related activities automatically file.source = 'import'; -- cgit v1.2.3-1-g7c22 From 2e845a51343fccf6f8b6a2b9e1142d771f2cf40d Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Sat, 2 Sep 2017 13:19:31 +0100 Subject: Uncomment code about adding attachments' related activities --- models/trelloCreator.js | 30 ++++++++++++------------------ models/wekanCreator.js | 30 ++++++++++++------------------ 2 files changed, 24 insertions(+), 36 deletions(-) diff --git a/models/trelloCreator.js b/models/trelloCreator.js index 4f64edd6..b296efdf 100644 --- a/models/trelloCreator.js +++ b/models/trelloCreator.js @@ -547,24 +547,18 @@ export class TrelloCreator { // Comment related activities // Trello doesn't export the comment id // Attachment related activities - // TODO: We can't add activities related to adding attachments - // because when we import an attachment, an activity is - // autmatically created. We need to directly insert the attachment - // without calling the "Attachments.files.after.insert" hook first, - // then we can uncomment the code below - // case 'addAttachment': { - // console.log(this.attachmentIds); - // Activities.direct.insert({ - // userId: this._user(activity.userId), - // type: 'card', - // activityType: activity.activityType, - // attachmentId: this.attachmentIds[activity.attachmentId], - // cardId: this.cards[activity.cardId], - // boardId, - // createdAt: this._now(activity.createdAt), - // }); - // break; - // } + case 'addAttachmentToCard': { + Activities.direct.insert({ + userId: this._user(action.idMemberCreator), + type: 'card', + activityType: 'addAttachment', + attachmentId: this.attachmentIds[action.data.attachment.id], + cardId: this.cards[action.data.card.id], + boardId, + createdAt: this._now(action.date), + }); + break; + } // Checklist related activities case 'addChecklistToCard': { Activities.direct.insert({ diff --git a/models/wekanCreator.js b/models/wekanCreator.js index 7e320817..9c7f50ec 100644 --- a/models/wekanCreator.js +++ b/models/wekanCreator.js @@ -550,24 +550,18 @@ export class WekanCreator { break; } // Attachment related activities - // TODO: We can't add activities related to adding attachments - // because when we import an attachment, an activity is - // autmatically created. We need to directly insert the attachment - // without calling the "Attachments.files.after.insert" hook first, - // then we can uncomment the code below - // case 'addAttachment': { - // console.log(this.attachmentIds); - // Activities.direct.insert({ - // userId: this._user(activity.userId), - // type: 'card', - // activityType: activity.activityType, - // attachmentId: this.attachmentIds[activity.attachmentId], - // cardId: this.cards[activity.cardId], - // boardId, - // createdAt: this._now(activity.createdAt), - // }); - // break; - // } + case 'addAttachment': { + Activities.direct.insert({ + userId: this._user(activity.userId), + type: 'card', + activityType: activity.activityType, + attachmentId: this.attachmentIds[activity.attachmentId], + cardId: this.cards[activity.cardId], + boardId, + createdAt: this._now(activity.createdAt), + }); + break; + } // Checklist related activities case 'addChecklist': { Activities.direct.insert({ -- cgit v1.2.3-1-g7c22 From 181af6f2508405c0459197445f3b02f8368cb490 Mon Sep 17 00:00:00 2001 From: Ghassen Rjab Date: Sat, 2 Sep 2017 13:42:23 +0100 Subject: Fix lint errors --- models/attachments.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/models/attachments.js b/models/attachments.js index 40adda49..560bec99 100644 --- a/models/attachments.js +++ b/models/attachments.js @@ -16,9 +16,10 @@ Attachments = new FS.Collection('attachments', { beforeWrite: (fileObj) => { if (!fileObj.isImage()) { return { - type: 'application/octet-stream' + type: 'application/octet-stream', }; } + return {}; }, }), ], @@ -70,7 +71,7 @@ if (Meteor.isServer) { } else { // Don't add activity about adding the attachment as the activity // be imported and delete source field - Attachments.update( {_id: doc._id} , {$unset: { source : "" } } ); + Attachments.update( {_id: doc._id}, {$unset: { source : '' } } ); } }); -- cgit v1.2.3-1-g7c22