Merge pull request #3893 from Nixxx19/fix-mass-assignment-updateProject-3875

Fix: mass assignment update project #3875

Author: raclim

server/controllers/project.controller.js

@@ -48,10 +48,18 @@ export async function updateProject(req, res) {
       });
       return;
     }
+    // only allow whitelisted fields so ownership/slug etc can't be overwritten
+    const allowedFields = ['name', 'files', 'updatedAt', 'visibility'];
+    const updateData = {};
+    allowedFields.forEach((field) => {
+      if (req.body[field] !== undefined) {
+        updateData[field] = req.body[field];
+      }
+    });
     const updatedProject = await Project.findByIdAndUpdate(
       req.params.project_id,
       {
-        $set: req.body
+        $set: updateData
       },
       {
         new: true,

server/controllers/project.controller/__test__/updateProject.test.js

@@ -0,0 +1,143 @@
+/**
+ * @jest-environment node
+ */
+import { Request, Response } from 'jest-express';
+
+import Project from '../../../models/project';
+import { updateProject } from '../../project.controller';
+
+jest.mock('../../../models/project');
+
+describe('project.controller', () => {
+  describe('updateProject()', () => {
+    let request;
+    let response;
+
+    beforeEach(() => {
+      request = new Request();
+      response = new Response();
+      response.send = jest.fn();
+      response.json = jest.fn();
+      response.status = jest.fn().mockReturnThis();
+      Project.findById = jest.fn().mockReturnValue({ exec: jest.fn() });
+      Project.findByIdAndUpdate = jest.fn();
+      jest.clearAllMocks();
+    });
+
+    afterEach(() => {
+      request.resetMocked();
+      response.resetMocked();
+    });
+
+    it('returns 404 if project does not exist', async () => {
+      request.setParams({ project_id: 'some-id' });
+      request.user = { _id: 'owner-id' };
+      request.body = { name: 'Test' };
+
+      Project.findById().exec.mockResolvedValue(null);
+
+      await updateProject(request, response);
+
+      expect(response.status).toHaveBeenCalledWith(404);
+      expect(response.send).toHaveBeenCalledWith({
+        success: false,
+        message: 'Project with that id does not exist.'
+      });
+      expect(Project.findByIdAndUpdate).not.toHaveBeenCalled();
+    });
+
+    it('returns 403 if project is not owned by authenticated user', async () => {
+      const project = {
+        user: { equals: jest.fn().mockReturnValue(false) }
+      };
+      request.setParams({ project_id: 'some-id' });
+      request.user = { _id: 'other-user-id' };
+      request.body = { name: 'Test' };
+
+      Project.findById().exec.mockResolvedValue(project);
+
+      await updateProject(request, response);
+
+      expect(response.status).toHaveBeenCalledWith(403);
+      expect(response.send).toHaveBeenCalledWith({
+        success: false,
+        message: 'Session does not match owner of project.'
+      });
+      expect(Project.findByIdAndUpdate).not.toHaveBeenCalled();
+    });
+
+    it('only updates whitelisted fields (user and slug in body are ignored)', async () => {
+      const ownerId = 'owner-123';
+      const project = {
+        _id: 'proj-1',
+        user: { equals: (id) => id === ownerId },
+        updatedAt: new Date('2024-01-01'),
+        files: []
+      };
+      const updatedDoc = {
+        _id: 'proj-1',
+        user: ownerId,
+        name: 'Updated name',
+        slug: 'original-slug',
+        files: []
+      };
+
+      request.setParams({ project_id: 'proj-1' });
+      request.user = { _id: ownerId };
+      request.body = {
+        name: 'Updated name',
+        user: 'another-user-id',
+        slug: 'hacked-slug'
+      };
+
+      Project.findById().exec.mockResolvedValue(project);
+      Project.findByIdAndUpdate.mockReturnValue({
+        populate: jest.fn().mockReturnThis(),
+        exec: jest.fn().mockResolvedValue(updatedDoc)
+      });
+
+      await updateProject(request, response);
+
+      expect(Project.findByIdAndUpdate).toHaveBeenCalled();
+      const updateArg = Project.findByIdAndUpdate.mock.calls[0][1];
+      const setPayload = updateArg.$set;
+
+      expect(setPayload).toHaveProperty('name', 'Updated name');
+      expect(setPayload).not.toHaveProperty('user');
+      expect(setPayload).not.toHaveProperty('slug');
+    });
+
+    it('allows updating whitelisted fields (name, visibility, updatedAt)', async () => {
+      const ownerId = 'owner-123';
+      const project = {
+        _id: 'proj-1',
+        user: { equals: (id) => id === ownerId },
+        updatedAt: new Date('2024-01-01'),
+        files: []
+      };
+
+      request.setParams({ project_id: 'proj-1' });
+      request.user = { _id: ownerId };
+      request.body = {
+        name: 'New name',
+        visibility: 'Private',
+        updatedAt: new Date('2024-01-02')
+      };
+
+      Project.findById().exec.mockResolvedValue(project);
+      Project.findByIdAndUpdate.mockReturnValue({
+        populate: jest.fn().mockReturnThis(),
+        exec: jest.fn().mockResolvedValue({})
+      });
+
+      await updateProject(request, response);
+
+      const updateArg = Project.findByIdAndUpdate.mock.calls[0][1];
+      const setPayload = updateArg.$set;
+
+      expect(setPayload).toHaveProperty('name', 'New name');
+      expect(setPayload).toHaveProperty('visibility', 'Private');
+      expect(setPayload).toHaveProperty('updatedAt');
+    });
+  });
+});

server/models/project.js

@@ -34,7 +34,7 @@ const projectSchema = new Schema(
       default: "Hello p5.js, it's the server",
       maxlength: 128
     },
-    user: { type: Schema.Types.ObjectId, ref: 'User' },
+    user: { type: Schema.Types.ObjectId, ref: 'User', immutable: true },
     serveSecure: { type: Boolean, default: false },
     files: { type: [fileSchema] },
     _id: { type: String, default: shortid.generate },
@@ -43,7 +43,7 @@ const projectSchema = new Schema(
       enum: ['Private', 'Public'],
       default: 'Public'
     },
-    slug: { type: String }
+    slug: { type: String, immutable: true }
   },
   { timestamps: true }
 );