Only allow enabling/disabling recovery alerts for modules of given Sa…

…fe (#1267) This includes a check that the specified `moduleAddress` is enabled on that `safeAddress` to the `EnableRecoveryAlertsGuard`/`DisableRecoveryAlertsGuard` guards: - Add check that `moduleAddress` is enabled on `safeAddress` in `EnableRecoveryAlertsGuard`/`DisableRecoveryAlertsGuard` - Add test coverage
safe-global · Mar 13, 2024 · 1fb9f5c · 1fb9f5c
1 parent a6a5cb6
commit 1fb9f5c
Show file tree

Hide file tree

Showing 5 changed files with 292 additions and 7 deletions.
diff --git a/src/routes/recovery/guards/disable-recovery-alerts.guard.spec.ts b/src/routes/recovery/guards/disable-recovery-alerts.guard.spec.ts
@@ -14,6 +14,12 @@ import { faker } from '@faker-js/faker';
 import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
 import { Hash } from 'viem';
 import { DisableRecoveryAlertsGuard } from '@/routes/recovery/guards/disable-recovery-alerts.guard';
+import { ISafeRepository } from '@/domain/safe/safe.repository.interface';
+
+const safeRepository = {
+  getSafesByModule: jest.fn(),
+} as jest.MockedObjectDeep<ISafeRepository>;
+const safeRepositoryMock = jest.mocked(safeRepository);
 
 @Controller()
 class TestController {
@@ -54,6 +60,12 @@ describe('DisableRecoveryAlertsGuard guard tests', () => {
     const moduleFixture: TestingModule = await Test.createTestingModule({
       imports: [TestLoggingModule, ConfigurationModule.register(configuration)],
       controllers: [TestController],
+      providers: [
+        {
+          provide: ISafeRepository,
+          useValue: safeRepositoryMock,
+        },
+      ],
     }).compile();
     app = await new TestAppProvider().provide(moduleFixture);
     await app.init();
@@ -63,7 +75,11 @@ describe('DisableRecoveryAlertsGuard guard tests', () => {
     await app.close();
   });
 
-  it('returns 200 for a valid signature', async () => {
+  it('returns 200 for a valid signature for module on given Safe', async () => {
+    safeRepositoryMock.getSafesByModule.mockResolvedValue({
+      safes: [safeAddress],
+    });
+
     await request(app.getHttpServer())
       .delete(`/test/${chainId}/${safeAddress}/${moduleAddress}`)
       .set('Safe-Wallet-Signature', signature)
@@ -75,6 +91,22 @@ describe('DisableRecoveryAlertsGuard guard tests', () => {
       .expect(200);
   });
 
+  it('returns 403 for a valid signature for module not on given Safe', async () => {
+    safeRepositoryMock.getSafesByModule.mockResolvedValue({
+      safes: [],
+    });
+
+    await request(app.getHttpServer())
+      .delete(`/test/${chainId}/${safeAddress}/${moduleAddress}`)
+      .set('Safe-Wallet-Signature', signature)
+      .set('Safe-Wallet-Signature-Timestamp', timestamp.toString())
+      .send({
+        moduleAddress,
+        signer: signer.address,
+      })
+      .expect(403);
+  });
+
   it('returns 403 for an invalid signature', async () => {
     const invalidSignature = await signer.signMessage({
       message: 'some invalid message',

diff --git a/src/routes/recovery/guards/disable-recovery-alerts.guard.ts b/src/routes/recovery/guards/disable-recovery-alerts.guard.ts
@@ -5,7 +5,8 @@ import {
   Injectable,
 } from '@nestjs/common';
 import { ILoggingService, LoggingService } from '@/logging/logging.interface';
-import { verifyMessage } from 'viem';
+import { getAddress, verifyMessage } from 'viem';
+import { ISafeRepository } from '@/domain/safe/safe.repository.interface';
 
 /**
  * The DisableRecoveryAlertsGuard guard should be used on routes that require
@@ -29,6 +30,7 @@ import { verifyMessage } from 'viem';
 @Injectable()
 export class DisableRecoveryAlertsGuard implements CanActivate {
   constructor(
+    @Inject(ISafeRepository) private readonly safeRepository: ISafeRepository,
     @Inject(LoggingService) private readonly loggingService: ILoggingService,
   ) {}
 
@@ -58,11 +60,22 @@ export class DisableRecoveryAlertsGuard implements CanActivate {
     const message = `${DisableRecoveryAlertsGuard.ACTION_PREFIX}-${chainId}-${safeAddress}-${moduleAddress}-${signer}-${timestamp}`;
 
     try {
-      return await verifyMessage({
+      const isValid = await verifyMessage({
         address: signer,
         message,
         signature,
       });
+
+      if (!isValid) {
+        return false;
+      }
+
+      const { safes } = await this.safeRepository.getSafesByModule({
+        chainId,
+        moduleAddress,
+      });
+
+      return safes.some((safe) => getAddress(safe) === getAddress(safeAddress));
     } catch (e) {
       this.loggingService.debug(e);
       return false;

diff --git a/src/routes/recovery/guards/enable-recovery-alerts.guard.spec.ts b/src/routes/recovery/guards/enable-recovery-alerts.guard.spec.ts
@@ -9,6 +9,12 @@ import { faker } from '@faker-js/faker';
 import { generatePrivateKey, privateKeyToAccount } from 'viem/accounts';
 import { Hash } from 'viem';
 import { EnableRecoveryAlertsGuard } from '@/routes/recovery/guards/enable-recovery-alerts.guard';
+import { ISafeRepository } from '@/domain/safe/safe.repository.interface';
+
+const safeRepository = {
+  getSafesByModule: jest.fn(),
+} as jest.MockedObjectDeep<ISafeRepository>;
+const safeRepositoryMock = jest.mocked(safeRepository);
 
 @Controller()
 class TestController {
@@ -45,6 +51,12 @@ describe('EnableRecoveryAlertsGuard guard tests', () => {
     const moduleFixture: TestingModule = await Test.createTestingModule({
       imports: [TestLoggingModule, ConfigurationModule.register(configuration)],
       controllers: [TestController],
+      providers: [
+        {
+          provide: ISafeRepository,
+          useValue: safeRepositoryMock,
+        },
+      ],
     }).compile();
     app = await new TestAppProvider().provide(moduleFixture);
     await app.init();
@@ -54,7 +66,11 @@ describe('EnableRecoveryAlertsGuard guard tests', () => {
     await app.close();
   });
 
-  it('returns 201 for a valid signature', async () => {
+  it('returns 201 for a valid signature for module on given Safe', async () => {
+    safeRepositoryMock.getSafesByModule.mockResolvedValue({
+      safes: [safeAddress],
+    });
+
     await request(app.getHttpServer())
       .post(`/test/${chainId}/${safeAddress}`)
       .set('Safe-Wallet-Signature', signature)
@@ -66,6 +82,22 @@ describe('EnableRecoveryAlertsGuard guard tests', () => {
       .expect(201);
   });
 
+  it('returns 403 for a valid signature for module not on given Safe', async () => {
+    safeRepositoryMock.getSafesByModule.mockResolvedValue({
+      safes: [],
+    });
+
+    await request(app.getHttpServer())
+      .post(`/test/${chainId}/${safeAddress}`)
+      .set('Safe-Wallet-Signature', signature)
+      .set('Safe-Wallet-Signature-Timestamp', timestamp.toString())
+      .send({
+        moduleAddress,
+        signer: signer.address,
+      })
+      .expect(403);
+  });
+
   it('returns 403 for an invalid signature', async () => {
     const invalidSignature = await signer.signMessage({
       message: 'some invalid message',

diff --git a/src/routes/recovery/guards/enable-recovery-alerts.guard.ts b/src/routes/recovery/guards/enable-recovery-alerts.guard.ts
@@ -5,7 +5,8 @@ import {
   Injectable,
 } from '@nestjs/common';
 import { ILoggingService, LoggingService } from '@/logging/logging.interface';
-import { verifyMessage } from 'viem';
+import { getAddress, verifyMessage } from 'viem';
+import { ISafeRepository } from '@/domain/safe/safe.repository.interface';
 
 /**
  * The EnableRecoveryAlertsGuard guard should be used on routes that require
@@ -29,6 +30,7 @@ import { verifyMessage } from 'viem';
 @Injectable()
 export class EnableRecoveryAlertsGuard implements CanActivate {
   constructor(
+    @Inject(ISafeRepository) private readonly safeRepository: ISafeRepository,
     @Inject(LoggingService) private readonly loggingService: ILoggingService,
   ) {}
 
@@ -58,11 +60,22 @@ export class EnableRecoveryAlertsGuard implements CanActivate {
     const message = `${EnableRecoveryAlertsGuard.ACTION_PREFIX}-${chainId}-${safeAddress}-${moduleAddress}-${signer}-${timestamp}`;
 
     try {
-      return await verifyMessage({
+      const isValid = await verifyMessage({
         address: signer,
         message,
         signature,
       });
+
+      if (!isValid) {
+        return false;
+      }
+
+      const { safes } = await this.safeRepository.getSafesByModule({
+        chainId,
+        moduleAddress,
+      });
+
+      return safes.some((safe) => getAddress(safe) === getAddress(safeAddress));
     } catch (e) {
       this.loggingService.debug(e);
       return false;