Return value of a lock not it's reference

When a lock is retrieved by using locks.get, a reference to an object is returned. By mutating this reference, a developer, could in theory, mutate the lock held by Spaces. This could lead to subtle and hard to debug issues.

This commit changes the .get and .getAll methods to return copies of objects instead. This breaks some of the internal mutations, so they are replaced by the already present .setLock method which will override the lock with a new object, severing any dependecies on references.

Author: dpiatek

src/Locks.test.ts

@@ -380,6 +380,22 @@ describe('Locks', () => {
       expect(lock3).toBeDefined();
     });
 
+    it<SpaceTestContext>('returns the lock by value, not reference', ({ space }) => {
+      const lockGet1 = space.locks.get('lock1');
+      const lockGet2 = space.locks.get('lock1');
+      expect(lockGet1).toEqual(expect.objectContaining({ status: 'locked' }));
+      // Note we are using toBe here on purpose, to check the reference, not value
+      expect(lockGet1).not.toBe(lockGet2);
+    });
+
+    it<SpaceTestContext>('returns locks by value, not reference', async ({ space }) => {
+      const lockGetAll1 = await space.locks.getAll();
+      const lockGetAll2 = await space.locks.getAll();
+      expect(lockGetAll1).toEqual(expect.arrayContaining([expect.objectContaining({ status: 'locked' })]));
+      // Note we are using toBe here on purpose, to check the reference, not value
+      expect(lockGetAll1[0]).not.toBe(lockGetAll2[0]);
+    });
+
     describe('getSelf', () => {
       it<SpaceTestContext>('returns all locks in the LOCKED state that belong to self', async ({ space }) => {
         const member1 = await space.members.getByConnectionId('1')!;
@@ -398,6 +414,13 @@ describe('Locks', () => {
           }
         }
       });
+
+      it<SpaceTestContext>('returns locks by value not reference', async ({ space }) => {
+        const locksGetSelf1 = await space.locks.getSelf();
+        const locksGetSelf2 = await space.locks.getSelf();
+        // Note we are using toBe here on purpose, to check the reference, not value
+        expect(locksGetSelf1[0]).not.toBe(locksGetSelf2[0]);
+      });
     });
 
     describe('getOthers', () => {

src/Locks.ts

@@ -133,7 +133,8 @@ export default class Locks extends EventEmitter<LocksEventMap> {
     if (!locks) return;
     for (const lock of locks.values()) {
       if (lock.status === 'locked') {
-        return lock;
+        // Return a copy instead of a reference to prevent mutations
+        return { ...lock };
       }
     }
   }
@@ -211,7 +212,8 @@ export default class Locks extends EventEmitter<LocksEventMap> {
     for (const locks of this.locks.values()) {
       for (const lock of locks.values()) {
         if (lock.status === 'locked') {
-          allLocks.push(lock);
+          // Return a copy instead of a reference to prevent mutations
+          allLocks.push({ ...lock });
         }
       }
     }
@@ -387,8 +389,7 @@ export default class Locks extends EventEmitter<LocksEventMap> {
     const lock = this.getLock(id, self.connectionId);
     if (!lock) return;
 
-    lock.status = 'unlocked';
-    lock.reason = undefined;
+    this.setLock({ ...lock, status: 'unlocked', reason: undefined });
     // Send presence update with the updated lock, but delete afterwards so when the
     // message is processed an update event is fired
     this.updatePresence(self);
@@ -563,9 +564,9 @@ export default class Locks extends EventEmitter<LocksEventMap> {
         const lock = locks.get(member.connectionId);
 
         if (lock) {
-          lock.status = 'unlocked';
-          lock.reason = undefined;
-          this.emit('update', lock);
+          const updatedLock = { ...lock, status: 'unlocked' as const, reason: undefined };
+          this.setLock(updatedLock);
+          this.emit('update', updatedLock);
           locks.delete(member.connectionId);
         }
       }
@@ -638,9 +639,9 @@ export default class Locks extends EventEmitter<LocksEventMap> {
       (pendingLock.timestamp == lock.timestamp && member.connectionId < lock.member.connectionId)
     ) {
       pendingLock.status = 'locked';
-      lock.status = 'unlocked';
-      lock.reason = ERR_LOCK_INVALIDATED();
-      this.emit('update', lock);
+      const updatedLock = { ...lock, status: 'unlocked' as const, reason: ERR_LOCK_INVALIDATED() };
+      this.setLock(updatedLock);
+      this.emit('update', updatedLock);
       return;
     }
 
@@ -684,7 +685,7 @@ export default class Locks extends EventEmitter<LocksEventMap> {
       const lock = locks.get(connectionId);
 
       if (lock) {
-        requests.push(lock);
+        requests.push({ ...lock });
       }
     }