From aabf97fe4e84da20491aec98dc2f70689428d58e Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 3 Feb 2026 21:32:47 -0600 Subject: [PATCH 1/4] fix(#283): Enforce connection status validation in queries Move status validation from post-retrieval checks into Prisma WHERE clauses. This prevents TOCTOU issues and ensures only ACTIVE connections are retrieved. Removed redundant status checks after retrieval in both query and command services. Security improvement: Enforces status=ACTIVE in database query rather than checking after retrieval, preventing race conditions. Fixes #283 Co-Authored-By: Claude Sonnet 4.5 --- .../src/federation/command.service.spec.ts | 31 +++++++++---- apps/api/src/federation/command.service.ts | 19 +++----- apps/api/src/federation/query.service.spec.ts | 46 +++++++++++-------- apps/api/src/federation/query.service.ts | 19 +++----- 4 files changed, 63 insertions(+), 52 deletions(-) diff --git a/apps/api/src/federation/command.service.spec.ts b/apps/api/src/federation/command.service.spec.ts index 42e5dd7..a6ea75f 100644 --- a/apps/api/src/federation/command.service.spec.ts +++ b/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 () => { diff --git a/apps/api/src/federation/command.service.ts b/apps/api/src/federation/command.service.ts index 626c1af..e094a59 100644 --- a/apps/api/src/federation/command.service.ts +++ b/apps/api/src/federation/command.service.ts @@ -41,19 +41,19 @@ export class CommandService { commandType: string, payload: Record ): Promise { - // 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( diff --git a/apps/api/src/federation/query.service.spec.ts b/apps/api/src/federation/query.service.spec.ts index 8b1b59f..6d8b431 100644 --- a/apps/api/src/federation/query.service.spec.ts +++ b/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 () => { diff --git a/apps/api/src/federation/query.service.ts b/apps/api/src/federation/query.service.ts index 6a2458b..a56c9e0 100644 --- a/apps/api/src/federation/query.service.ts +++ b/apps/api/src/federation/query.service.ts @@ -38,19 +38,19 @@ export class QueryService { query: string, context?: Record ): Promise { - // 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( -- 2.49.1 From ecb33a17feae2457246a9ea00141f7942d2646e2 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 3 Feb 2026 21:33:57 -0600 Subject: [PATCH 2/4] fix(#288): Upgrade RSA key size to 4096 bits Changed modulusLength from 2048 to 4096 in generateKeypair() method following NIST recommendations for long-term security. Added test to verify generated keys meet the minimum size requirement. Security improvement: RSA-4096 provides better protection against future cryptographic attacks as computational power increases. Fixes #288 Co-Authored-By: Claude Sonnet 4.5 --- apps/api/src/federation/federation.service.spec.ts | 12 ++++++++++++ apps/api/src/federation/federation.service.ts | 3 ++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/apps/api/src/federation/federation.service.spec.ts b/apps/api/src/federation/federation.service.spec.ts index fb85ea8..3914270 100644 --- a/apps/api/src/federation/federation.service.spec.ts +++ b/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", () => { diff --git a/apps/api/src/federation/federation.service.ts b/apps/api/src/federation/federation.service.ts index 390aec3..1e7e03a 100644 --- a/apps/api/src/federation/federation.service.ts +++ b/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", -- 2.49.1 From 77d1d14e08b5075537937ea2e3454555ccf04287 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 3 Feb 2026 21:35:15 -0600 Subject: [PATCH 3/4] fix(#289): Prevent private key decryption error data leaks Modified decrypt() error handling to only log error type without stack traces, error details, or encrypted content. Added test to verify sensitive data is not exposed in logs. Security improvement: Prevents leakage of encrypted data or partial decryption results through error logs. Fixes #289 Co-Authored-By: Claude Sonnet 4.5 --- .../api/src/federation/crypto.service.spec.ts | 28 ++++++++++++++++++- apps/api/src/federation/crypto.service.ts | 5 +++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/apps/api/src/federation/crypto.service.spec.ts b/apps/api/src/federation/crypto.service.spec.ts index ce0d76b..d1b1d7c 100644 --- a/apps/api/src/federation/crypto.service.spec.ts +++ b/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", () => { diff --git a/apps/api/src/federation/crypto.service.ts b/apps/api/src/federation/crypto.service.ts index 56140d6..8acee35 100644 --- a/apps/api/src/federation/crypto.service.ts +++ b/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"); } } -- 2.49.1 From 1390da2e74168a73209c37fc65542c98e5a11323 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Tue, 3 Feb 2026 21:36:31 -0600 Subject: [PATCH 4/4] fix(#290): Secure identity verification endpoint Added @UseGuards(AuthGuard) and rate limiting (@Throttle) to /api/v1/federation/identity/verify endpoint. Configured strict rate limit (10 req/min) to prevent abuse of this previously public endpoint. Added test to verify guards are applied. Security improvement: Prevents unauthorized access and rate limit abuse of identity verification endpoint. Fixes #290 Co-Authored-By: Claude Sonnet 4.5 --- .../src/federation/identity-linking.controller.spec.ts | 9 +++++++++ apps/api/src/federation/identity-linking.controller.ts | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/apps/api/src/federation/identity-linking.controller.spec.ts b/apps/api/src/federation/identity-linking.controller.spec.ts index 33b8510..56349e3 100644 --- a/apps/api/src/federation/identity-linking.controller.spec.ts +++ b/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", diff --git a/apps/api/src/federation/identity-linking.controller.ts b/apps/api/src/federation/identity-linking.controller.ts index a1b45ab..08c69f5 100644 --- a/apps/api/src/federation/identity-linking.controller.ts +++ b/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 { return this.identityLinkingService.verifyIdentity(dto); } -- 2.49.1