/api/public should not allow entity modification (#7100)

huridocs · Aug 5, 2024 · 10768ca · 10768ca
1 parent 8c40ddc
commit 10768ca
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 16 deletions.
diff --git a/app/api/files/jsRoutes.js b/app/api/files/jsRoutes.js
@@ -75,6 +75,11 @@ const routes = app => {
       const { allowedPublicTemplates } = await settings.get();
       const { entity, email } = req.body;
 
+      if (entity._id) {
+        next(createError('Unauthorized _id property', 403));
+        return;
+      }
+
       if (!allowedPublicTemplates || !allowedPublicTemplates.includes(entity.template)) {
         next(createError('Unauthorized public template', 403));
         return;

diff --git a/app/api/files/specs/fixtures.ts b/app/api/files/specs/fixtures.ts
@@ -14,7 +14,7 @@ const restrictedUploadId = db.id();
 const restrictedUploadId2 = db.id();
 const readOnlyUploadId = db.id();
 const customFileId = db.id();
-const templateId = fixturesFactory.id('template');
+const allowedPublicTemplate = fixturesFactory.id('template');
 const importTemplate = db.id();
 const writerUserId = db.id();
 const externalUrlFileId = db.id();
@@ -146,7 +146,7 @@ const fixtures: DBFixture = {
       sharedId: 'publicEntity',
       language: 'es',
       title: 'Public entity',
-      template: templateId,
+      template: allowedPublicTemplate,
       published: true,
     },
     {
@@ -155,18 +155,18 @@ const fixtures: DBFixture = {
       language: 'es',
       title: 'Gadgets 01 ES',
       generatedToc: true,
-      template: templateId,
+      template: allowedPublicTemplate,
     },
     {
       _id: entityEnId,
-      template: templateId,
+      template: allowedPublicTemplate,
       sharedId: 'sharedId1',
       language: 'en',
       title: 'Gadgets 01 EN',
     },
     {
       _id: restrictedEntityId,
-      template: templateId,
+      template: allowedPublicTemplate,
       sharedId: 'restrictedSharedId',
       language: 'en',
       title: 'Restricted Entity',
@@ -181,7 +181,7 @@ const fixtures: DBFixture = {
     },
     {
       _id: readOnlyEntity,
-      template: templateId,
+      template: allowedPublicTemplate,
       sharedId: 'readOnlySharedId',
       language: 'en',
       title: 'Read only shared id',
@@ -196,15 +196,15 @@ const fixtures: DBFixture = {
     },
   ],
   templates: [
-    { _id: templateId, default: true, name: 'mydoc', properties: [] },
+    { _id: allowedPublicTemplate, default: true, name: 'mydoc', properties: [] },
     { _id: importTemplate, default: true, name: 'import', properties: [] },
   ],
   settings: [
     {
       _id: db.id(),
       languages: [{ key: 'es', label: 'ES', default: true }],
       publicFormDestination: 'http://localhost:54321',
-      allowedPublicTemplates: [templateId.toString()],
+      allowedPublicTemplates: [allowedPublicTemplate.toString()],
     },
   ],
   users: [collabUser, writerUser, adminUser],
@@ -216,7 +216,7 @@ const fixtures: DBFixture = {
     {
       status: 'ready',
       entityId: 'sharedId1',
-      entityTemplate: templateId.toString(),
+      entityTemplate: allowedPublicTemplate.toString(),
       fileId: uploadId,
       language: 'en',
       propertyName: 'property 1',
@@ -228,7 +228,7 @@ const fixtures: DBFixture = {
     {
       status: 'ready',
       entityId: 'sharedId1',
-      entityTemplate: templateId.toString(),
+      entityTemplate: allowedPublicTemplate.toString(),
       fileId: uploadId,
       language: 'en',
       propertyName: 'property 2',
@@ -240,7 +240,7 @@ const fixtures: DBFixture = {
     {
       status: 'ready',
       entityId: 'restrictedSharedId',
-      entityTemplate: templateId.toString(),
+      entityTemplate: allowedPublicTemplate.toString(),
       fileId: restrictedUploadId,
       language: 'en',
       propertyName: 'property 1',
@@ -252,7 +252,7 @@ const fixtures: DBFixture = {
     {
       status: 'ready',
       entityId: 'restrictedSharedId',
-      entityTemplate: templateId.toString(),
+      entityTemplate: allowedPublicTemplate.toString(),
       fileId: restrictedUploadId,
       language: 'en',
       propertyName: 'property 2',
@@ -265,6 +265,7 @@ const fixtures: DBFixture = {
 };
 
 export {
+  allowedPublicTemplate,
   fixtures,
   entityId,
   entityEnId,
@@ -277,7 +278,7 @@ export {
   restrictedUploadId,
   restrictedUploadId2,
   readOnlyUploadId,
-  templateId,
+  allowedPublicTemplate as templateId,
   importTemplate,
   collabUser,
   adminUser,

diff --git a/app/api/files/specs/jsRoutes.spec.js b/app/api/files/specs/jsRoutes.spec.js
@@ -9,7 +9,7 @@ import express from 'express';
 import mailer from 'api/utils/mailer';
 // eslint-disable-next-line node/no-restricted-import
 import fs from 'fs/promises';
-import { fixtures, templateId } from './fixtures';
+import { allowedPublicTemplate, fixtures, templateId } from './fixtures';
 import instrumentRoutes from '../../utils/instrumentRoutes';
 import uploadRoutes from '../jsRoutes.js';
 import { legacyLogger } from '../../log';
@@ -120,10 +120,10 @@ describe('upload routes', () => {
     });
 
     it('should not create entity if template is not whitelisted in allowedPublicTemplates setting', async () => {
-      req.body.entity = JSON.stringify({
+      req.body.entity = {
         title: 'public submit',
         template: 'unauthorized_template_id',
-      });
+      };
       try {
         await routes.post('/api/public', req);
         fail('should return error');
@@ -134,6 +134,21 @@ describe('upload routes', () => {
       const res = await entities.get({ title: 'public submit' });
       expect(res.length).toBe(0);
     });
+
+    it('should not allow entity updates (sending entities with _id)', async () => {
+      req.body.entity = {
+        _id: 'an id',
+        title: 'public submit',
+        template: allowedPublicTemplate.toString(),
+      };
+      try {
+        await routes.post('/api/public', req);
+        fail('should return error');
+      } catch (e) {
+        expect(e.message).toMatch(/unauthorized _id property/i);
+        expect(e.code).toBe(403);
+      }
+    });
   });
 
   describe('/remotepublic', () => {