Merge pull request 'Security Sprint M7.1: Fix P1 Security Issues (#283, #288, #289, #290)' (#319) from fix/283-connection-status-validation into develop

Reviewed-on: #319

apps/api/src/federation/command.service.spec.ts

@@ -154,6 +154,15 @@ describe("CommandService", () => {
           signature: "signature-123",
         })
       );
+
+      // Verify status was checked in the query
+      expect(prisma.federationConnection.findUnique).toHaveBeenCalledWith({
+        where: {
+          id: mockConnectionId,
+          workspaceId: mockWorkspaceId,
+          status: FederationConnectionStatus.ACTIVE,
+        },
+      });
     });
 
     it("should throw error if connection not found", async () => {
@@ -165,19 +174,21 @@ describe("CommandService", () => {
     });
 
     it("should throw error if connection is not active", async () => {
-      const mockConnection = {
-        id: mockConnectionId,
-        workspaceId: mockWorkspaceId,
-        status: FederationConnectionStatus.SUSPENDED,
-      };
-
-      vi.spyOn(prisma.federationConnection, "findUnique").mockResolvedValue(
-        mockConnection as never
-      );
+      // Connection should not be found by query because it's not ACTIVE
+      vi.spyOn(prisma.federationConnection, "findUnique").mockResolvedValue(null);
 
       await expect(
         service.sendCommand(mockWorkspaceId, mockConnectionId, "test", {})
-      ).rejects.toThrow("Connection is not active");
+      ).rejects.toThrow("Connection not found");
+
+      // Verify status was checked in the query
+      expect(prisma.federationConnection.findUnique).toHaveBeenCalledWith({
+        where: {
+          id: mockConnectionId,
+          workspaceId: mockWorkspaceId,
+          status: FederationConnectionStatus.ACTIVE,
+        },
+      });
     });
 
     it("should mark command as failed if sending fails", async () => {

apps/api/src/federation/command.service.ts

@@ -41,19 +41,19 @@ export class CommandService {
     commandType: string,
     payload: Record<string, unknown>
   ): Promise<CommandMessageDetails> {
-    // Validate connection exists and is active
+    // Validate connection exists and is active (enforced in query)
     const connection = await this.prisma.federationConnection.findUnique({
-      where: { id: connectionId, workspaceId },
+      where: {
+        id: connectionId,
+        workspaceId,
+        status: FederationConnectionStatus.ACTIVE,
+      },
     });
 
     if (!connection) {
       throw new Error("Connection not found");
     }
 
-    if (connection.status !== FederationConnectionStatus.ACTIVE) {
-      throw new Error("Connection is not active");
-    }
-
     // Get local instance identity
     const identity = await this.federationService.getInstanceIdentity();
 
@@ -132,7 +132,7 @@ export class CommandService {
       throw new Error("Command timestamp is outside acceptable range");
     }
 
-    // Find connection for remote instance
+    // Find connection for remote instance (status enforced in query)
     const connection = await this.prisma.federationConnection.findFirst({
       where: {
         remoteInstanceId: commandMessage.instanceId,
@@ -144,11 +144,6 @@ export class CommandService {
       throw new Error("No connection found for remote instance");
     }
 
-    // Validate connection is active
-    if (connection.status !== FederationConnectionStatus.ACTIVE) {
-      throw new Error("Connection is not active");
-    }
-
     // Verify signature
     const { signature, ...messageToVerify } = commandMessage;
     const verificationResult = await this.signatureService.verifyMessage(

apps/api/src/federation/crypto.service.spec.ts

@@ -2,10 +2,11 @@
  * Crypto Service Tests
  */
 
-import { describe, it, expect, beforeEach } from "vitest";
+import { describe, it, expect, beforeEach, vi } from "vitest";
 import { Test, TestingModule } from "@nestjs/testing";
 import { ConfigService } from "@nestjs/config";
 import { CryptoService } from "./crypto.service";
+import { Logger } from "@nestjs/common";
 
 describe("CryptoService", () => {
   let service: CryptoService;
@@ -137,6 +138,31 @@ describe("CryptoService", () => {
       // Act & Assert
       expect(() => service.decrypt(corrupted)).toThrow("Failed to decrypt data");
     });
+
+    it("should not log sensitive data in error messages", () => {
+      // Arrange
+      const loggerErrorSpy = vi.spyOn(Logger.prototype, "error");
+      const corruptedData = "corrupted:data:here";
+
+      // Act & Assert
+      expect(() => service.decrypt(corruptedData)).toThrow("Failed to decrypt data");
+
+      // Verify logger was called with safe message
+      expect(loggerErrorSpy).toHaveBeenCalled();
+      const logCall = loggerErrorSpy.mock.calls[0];
+
+      // First argument should contain error type but not sensitive data
+      expect(logCall[0]).toMatch(/Decryption failed:/);
+
+      // Should NOT log the actual error object with stack traces
+      expect(logCall.length).toBe(1); // Only one argument (the message)
+
+      // Verify the corrupted data is not in the log
+      const logMessage = logCall[0] as string;
+      expect(logMessage).not.toContain(corruptedData);
+
+      loggerErrorSpy.mockRestore();
+    });
   });
 
   describe("encrypt/decrypt round-trip", () => {

apps/api/src/federation/crypto.service.ts

@@ -90,7 +90,10 @@ export class CryptoService {
 
       return decrypted;
     } catch (error) {
-      this.logger.error("Decryption failed", error);
+      // Security: Do not log error details which may contain sensitive data
+      // Only log error type/code without stack trace or encrypted content
+      const errorType = error instanceof Error ? error.constructor.name : "Unknown";
+      this.logger.error(`Decryption failed: ${errorType}`);
       throw new Error("Failed to decrypt data");
     }
   }

apps/api/src/federation/federation.service.spec.ts

@@ -198,6 +198,18 @@ describe("FederationService", () => {
       expect(result1.publicKey).not.toEqual(result2.publicKey);
       expect(result1.privateKey).not.toEqual(result2.privateKey);
     });
+
+    it("should generate RSA-4096 key pairs for future-proof security", () => {
+      // Act
+      const result = service.generateKeypair();
+
+      // Assert - Verify key size by checking approximate length
+      // RSA-4096 keys are significantly larger than RSA-2048
+      // Private key in PKCS8 format: RSA-2048 ≈ 1700 bytes, RSA-4096 ≈ 3200 bytes
+      // Public key in SPKI format: RSA-2048 ≈ 400 bytes, RSA-4096 ≈ 800 bytes
+      expect(result.privateKey.length).toBeGreaterThan(3000);
+      expect(result.publicKey.length).toBeGreaterThan(700);
+    });
   });
 
   describe("regenerateKeypair", () => {

apps/api/src/federation/federation.service.ts

@@ -57,10 +57,11 @@ export class FederationService {
 
   /**
    * Generate a new RSA key pair for instance signing
+   * Uses RSA-4096 for future-proof security (NIST recommendation)
    */
   generateKeypair(): KeyPair {
     const { publicKey, privateKey } = generateKeyPairSync("rsa", {
-      modulusLength: 2048,
+      modulusLength: 4096,
       publicKeyEncoding: {
         type: "spki",
         format: "pem",

apps/api/src/federation/identity-linking.controller.spec.ts

@@ -90,6 +90,15 @@ describe("IdentityLinkingController", () => {
   });
 
   describe("POST /identity/verify", () => {
+    it("should have AuthGuard and Throttle decorators applied", () => {
+      // Verify that the endpoint has proper guards and rate limiting
+      const verifyMetadata = Reflect.getMetadata(
+        "__guards__",
+        IdentityLinkingController.prototype.verifyIdentity
+      );
+      expect(verifyMetadata).toBeDefined();
+    });
+
     it("should verify identity with valid request", async () => {
       const dto: VerifyIdentityDto = {
         localUserId: "local-user-id",

apps/api/src/federation/identity-linking.controller.ts

@@ -5,6 +5,7 @@
  */
 
 import { Controller, Post, Get, Patch, Delete, Body, Param, UseGuards } from "@nestjs/common";
+import { Throttle } from "@nestjs/throttler";
 import { AuthGuard } from "../auth/guards/auth.guard";
 import { IdentityLinkingService } from "./identity-linking.service";
 import { IdentityResolutionService } from "./identity-resolution.service";
@@ -45,8 +46,11 @@ export class IdentityLinkingController {
    *
    * Verify a user's identity from a remote instance.
    * Validates signature and OIDC token.
+   * Rate limit: "strict" tier (10 req/min) - public endpoint requiring authentication
    */
   @Post("verify")
+  @UseGuards(AuthGuard)
+  @Throttle({ strict: { limit: 10, ttl: 60000 } })
   async verifyIdentity(@Body() dto: VerifyIdentityDto): Promise<IdentityVerificationResponse> {
     return this.identityLinkingService.verifyIdentity(dto);
   }

apps/api/src/federation/query.service.spec.ts

@@ -143,7 +143,11 @@ describe("QueryService", () => {
       expect(result.messageType).toBe(FederationMessageType.QUERY);
       expect(result.query).toBe(query);
       expect(mockPrisma.federationConnection.findUnique).toHaveBeenCalledWith({
-        where: { id: connectionId, workspaceId },
+        where: {
+          id: connectionId,
+          workspaceId,
+          status: FederationConnectionStatus.ACTIVE,
+        },
       });
       expect(mockPrisma.federationMessage.create).toHaveBeenCalled();
       expect(mockHttpService.post).toHaveBeenCalledWith(
@@ -168,17 +172,21 @@ describe("QueryService", () => {
     });
 
     it("should throw error if connection not active", async () => {
-      const mockConnection = {
-        id: "connection-1",
-        workspaceId: "workspace-1",
-        status: FederationConnectionStatus.PENDING,
-      };
-
-      mockPrisma.federationConnection.findUnique.mockResolvedValue(mockConnection);
+      // Connection should not be found by query because it's not ACTIVE
+      mockPrisma.federationConnection.findUnique.mockResolvedValue(null);
 
       await expect(
         service.sendQuery("workspace-1", "connection-1", "SELECT * FROM tasks")
-      ).rejects.toThrow("Connection is not active");
+      ).rejects.toThrow("Connection not found");
+
+      // Verify that findUnique was called with status: ACTIVE in the query
+      expect(mockPrisma.federationConnection.findUnique).toHaveBeenCalledWith({
+        where: {
+          id: "connection-1",
+          workspaceId: "workspace-1",
+          status: FederationConnectionStatus.ACTIVE,
+        },
+      });
     });
 
     it("should handle network errors gracefully", async () => {
@@ -305,19 +313,21 @@ describe("QueryService", () => {
         signature: "valid-signature",
       };
 
-      const mockConnection = {
-        id: "connection-1",
-        workspaceId: "workspace-1",
-        remoteInstanceId: "remote-instance-1",
-        status: FederationConnectionStatus.PENDING,
-      };
-
-      mockPrisma.federationConnection.findFirst.mockResolvedValue(mockConnection);
+      // Connection should not be found because status filter in query excludes non-ACTIVE
+      mockPrisma.federationConnection.findFirst.mockResolvedValue(null);
       mockSignatureService.validateTimestamp.mockReturnValue(true);
 
       await expect(service.handleIncomingQuery(queryMessage)).rejects.toThrow(
-        "Connection is not active"
+        "No connection found for remote instance"
       );
+
+      // Verify the findFirst was called with status: ACTIVE filter
+      expect(mockPrisma.federationConnection.findFirst).toHaveBeenCalledWith({
+        where: {
+          remoteInstanceId: "remote-instance-1",
+          status: FederationConnectionStatus.ACTIVE,
+        },
+      });
     });
 
     it("should reject query from unknown instance", async () => {

apps/api/src/federation/query.service.ts

@@ -38,19 +38,19 @@ export class QueryService {
     query: string,
     context?: Record<string, unknown>
   ): Promise<QueryMessageDetails> {
-    // Validate connection exists and is active
+    // Validate connection exists and is active (enforced in query)
     const connection = await this.prisma.federationConnection.findUnique({
-      where: { id: connectionId, workspaceId },
+      where: {
+        id: connectionId,
+        workspaceId,
+        status: FederationConnectionStatus.ACTIVE,
+      },
     });
 
     if (!connection) {
       throw new Error("Connection not found");
     }
 
-    if (connection.status !== FederationConnectionStatus.ACTIVE) {
-      throw new Error("Connection is not active");
-    }
-
     // Get local instance identity
     const identity = await this.federationService.getInstanceIdentity();
 
@@ -129,7 +129,7 @@ export class QueryService {
       throw new Error("Query timestamp is outside acceptable range");
     }
 
-    // Find connection for remote instance
+    // Find connection for remote instance (status enforced in query)
     const connection = await this.prisma.federationConnection.findFirst({
       where: {
         remoteInstanceId: queryMessage.instanceId,
@@ -141,11 +141,6 @@ export class QueryService {
       throw new Error("No connection found for remote instance");
     }
 
-    // Validate connection is active
-    if (connection.status !== FederationConnectionStatus.ACTIVE) {
-      throw new Error("Connection is not active");
-    }
-
     // Verify signature
     const { signature, ...messageToVerify } = queryMessage;
     const verificationResult = await this.signatureService.verifyMessage(