secret storage #1447

gugu · 2025-11-21T20:59:48Z

No description provided.

…crets

gugu · 2025-11-23T10:35:50Z

Copilot

Pull request overview

This PR implements a secret storage feature that allows users to securely store, manage, and audit sensitive company information like API keys, tokens, and passwords. The feature includes encrypted storage with optional master password protection, comprehensive audit logging, and company-based access control.

Adds complete secret storage backend with entities, service, controller, and DTOs
Implements two-layer encryption (application-level + optional master password)
Adds database migrations for user secrets and audit logs
Includes comprehensive unit and E2E test coverage
Updates Docker configuration for test environment

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 26 comments.

Show a summary per file

File	Description
backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts	Comprehensive E2E tests covering all secret operations including CRUD, authentication, authorization, and master password protection
backend/src/migrations/1763724062000-CreateSecretAccessLogEntity.ts	Database migration for audit log table with proper indexes and foreign keys
backend/src/migrations/1763724061000-CreateUserSecretEntity.ts	Database migration for user secrets table with encryption support and company-level scoping
backend/src/entities/user-secret/user-secrets.service.ts	Core service implementing secret management with encryption, access control, and audit logging
backend/src/entities/user-secret/user-secrets.service.spec.ts	Unit tests for the secrets service covering all major operations and error cases
backend/src/entities/user-secret/user-secret.module.ts	NestJS module definition registering the secret storage components
backend/src/entities/user-secret/user-secret.entity.ts	TypeORM entity for user secrets with relationships and indexes
backend/src/entities/user-secret/user-secret.controller.ts	REST API controller exposing secret management endpoints
backend/src/entities/user-secret/application/dto/update-secret.dto.ts	DTO for updating secret values with validation
backend/src/entities/user-secret/application/dto/secret-list.dto.ts	DTOs for paginated secret list responses
backend/src/entities/user-secret/application/dto/found-secret.dto.ts	DTO for returning secret details with optional value
backend/src/entities/user-secret/application/dto/create-secret.dto.ts	DTO for creating secrets with validation and master password support
backend/src/entities/user-secret/application/dto/audit-log.dto.ts	DTOs for audit log entries and paginated responses
backend/src/entities/secret-access-log/secret-access-log.entity.ts	TypeORM entity for tracking all secret access events
backend/src/decorators/company-id.decorator.ts	Custom decorator for extracting company ID from authenticated requests
backend/src/app.module.ts	Registers the UserSecretModule in the main application module
justfile	Adds --attach flag to focus test output on backend_test container
docker-compose.tst.yml	Updates test environment with explicit DATABASE_URL and simplified volume mounts
Dockerfile	Modifies file ownership configuration for the application directory
SECRET_STORAGE_SPECIFICATION.md	Comprehensive specification document detailing the secret storage feature design

Comments suppressed due to low confidence (1)

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts:71

Unused variable connectionId.

  const connectionId = JSON.parse(createdConnection.text).id;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-23T10:39:44Z

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts

+    .set('Content-Type', 'application/json')
+    .set('Accept', 'application/json');
+
+  const connectionId = JSON.parse(createdConnection.text).id;


The variable connectionId is created but never used in this test or any other tests in the file. Each test creates a connection but doesn't use the connectionId for any assertions or operations. Consider removing these unused variable declarations throughout the test file (lines 71, 107, 138, 170, 202, 228, 254, 277, 312, 334, 361, 392, 426, 455, 490, 516, 550, 573, 600, 623) to improve code maintainability.

Suggested change

const connectionId = JSON.parse(createdConnection.text).id;

// Removed unused connectionId variable

Copilot · 2025-11-23T10:39:45Z

backend/src/entities/user-secret/application/dto/update-secret.dto.ts

+  @ApiProperty({ required: true, type: 'string' })
+  @IsString()
+  @IsNotEmpty()
+  @MinLength(1)
+  @MaxLength(10000)
+  value: string;


The value field is marked as required: true in the @ApiProperty decorator, but for an update operation, the value should be optional to allow partial updates (e.g., only updating expiresAt). The validators @IsNotEmpty(), @MinLength(1) will still enforce that if value is provided, it must be valid. Consider changing to @ApiProperty({ required: false, type: 'string' }) and adding @IsOptional() as the first validator.

Suggested change

@ApiProperty({ required: true, type: 'string' })

@IsString()

@IsNotEmpty()

@MinLength(1)

@MaxLength(10000)

value: string;

@ApiProperty({ required: false, type: 'string' })

@IsOptional()

@IsString()

@IsNotEmpty()

@MinLength(1)

@MaxLength(10000)

value?: string;

Dockerfile

Copilot · 2025-11-23T10:39:46Z

backend/src/entities/user-secret/user-secret.controller.ts

+  Post,
+  Put,
+  Query,
+  UseGuards,


Unused import UseGuards.

Suggested change

UseGuards,

Copilot · 2025-11-23T10:39:46Z

backend/src/entities/user-secret/user-secrets.service.spec.ts

+      jest.spyOn(auditLogRepository, 'save').mockResolvedValue({} as SecretAccessLogEntity);
+
+      const result = await service.updateSecret('user-uuid-1', 'test-secret', updateDto);
+


Unused variable result.

Suggested change

expect(result.slug).toBe('test-secret');

expect(result.encryptedValue).toBe('new-value');

Copilot · 2025-11-23T10:39:51Z

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts

+    .set('Content-Type', 'application/json')
+    .set('Accept', 'application/json');
+
+  const connectionId = JSON.parse(createdConnection.text).id;


Unused variable connectionId.

Suggested change

const connectionId = JSON.parse(createdConnection.text).id;

Copilot · 2025-11-23T10:39:51Z

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts

+    .set('Content-Type', 'application/json')
+    .set('Accept', 'application/json');
+
+  const connectionId = JSON.parse(createdConnection.text).id;


Unused variable connectionId.

Copilot · 2025-11-23T10:39:51Z

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts

+    .set('Content-Type', 'application/json')
+    .set('Accept', 'application/json');
+
+  const connectionId = JSON.parse(createdConnection.text).id;


Unused variable connectionId.

Copilot · 2025-11-23T10:39:52Z

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts

+    .set('Content-Type', 'application/json')
+    .set('Accept', 'application/json');
+
+  const connectionId = JSON.parse(createdConnection.text).id;


Unused variable connectionId.

Copilot · 2025-11-23T10:39:52Z

backend/test/ava-tests/non-saas-tests/non-saas-secrets-e2e.test.ts

+    .set('Content-Type', 'application/json')
+    .set('Accept', 'application/json');
+
+  const connectionId = JSON.parse(createdConnection.text).id;


Unused variable connectionId.

Suggested change

const connectionId = JSON.parse(createdConnection.text).id;

Artuomka

The implementation does not align with the overall project architecture.
New entities have been added that are related to existing ones, but the corresponding changes were not made to the existing entities.

Artuomka · 2025-11-23T15:06:01Z

backend/src/entities/secret-access-log/secret-access-log.entity.ts

+  id: string;
+
+  @ManyToOne(() => UserSecretEntity, (secret) => secret.accessLogs, { onDelete: 'CASCADE' })
+  @JoinColumn()


It's better to explicitly set the foreign key name and the separate field with this name to the entity

Artuomka · 2025-11-23T15:07:09Z

backend/src/decorators/company-id.decorator.ts

+  }
+
+  // Company ID should be retrieved from the user entity via a repository lookup
+  // This is a simplified version - in practice, you'd inject a UserRepository


UserRepository can not be injected in the decorator

Artuomka · 2025-11-23T15:08:21Z

backend/src/entities/secret-access-log/secret-access-log.entity.ts

+
+  @ManyToOne(() => UserEntity, { onDelete: 'CASCADE' })
+  @JoinColumn()
+  user: Relation<UserEntity>;


It's better to explicitly set the foreign key name and the separate field with this name to the entity

Artuomka · 2025-11-23T15:10:23Z

backend/src/entities/user-secret/application/dto/audit-log.dto.ts

+  user: {
+    id: string;
+    email: string;
+  };


I'm not sure that the user object will be correctly displayed in Swagger this way; it's better to create a separate object describing it with Swagger decorators

Artuomka · 2025-11-23T15:11:08Z

backend/src/entities/user-secret/application/dto/audit-log.dto.ts

+
+export class AuditLogResponseDto {
+  @ApiProperty({ type: [AuditLogEntryDto] })
+  data: AuditLogEntryDto[];


({ type: AuditLogEntryDto, isArray: true })

Artuomka · 2025-11-23T16:00:59Z

backend/src/entities/user-secret/user-secrets.service.ts

+export class UserSecretsService {
+  constructor(
+    @InjectRepository(UserSecretEntity)
+    private readonly secretRepository: Repository<UserSecretEntity>,


A global database context should be used

Artuomka · 2025-11-23T16:04:01Z

backend/src/entities/user-secret/user-secrets.service.ts

+      throw new ForbiddenException('User not found or not associated with a company');
+    }
+
+    return user;


Private methods are usually placed at the end of the class, after the public ones

Artuomka · 2025-11-23T16:04:23Z

backend/src/entities/user-secret/user-secrets.service.ts

+    });
+
+    await this.auditLogRepository.save(log);
+  }


Private methods are usually placed at the end of the class, after the public ones

Artuomka · 2025-11-23T16:07:24Z

backend/src/entities/user-secret/user-secrets.service.ts

+    });
+
+    if (existing) {
+      throw new ConflictException('Secret with this slug already exists in your company');


We must avoid magic strings

Artuomka · 2025-11-23T16:10:58Z

docker-compose.tst.yml

  test-dynamodb-e2e-testing:
    image: amazon/dynamodb-local
-    ports:
-      - 8000:8000


why port was removed?

gugu added 4 commits November 21, 2025 20:59

secret storage

0ed44cb

Merge branch 'main' into secrets

a516bc4

secret encryption

cf9822c

Merge branch 'secrets' of github.com:rocket-admin/rocketadmin into se…

6a2d675

…crets

gugu requested a review from Artuomka November 23, 2025 10:34

gugu marked this pull request as ready for review November 23, 2025 10:34

Copilot AI review requested due to automatic review settings November 23, 2025 10:34

Copilot started reviewing on behalf of gugu November 23, 2025 10:35 View session

Copilot finished reviewing on behalf of gugu November 23, 2025 10:36

Copilot AI reviewed Nov 23, 2025

View reviewed changes

gugu marked this pull request as draft November 23, 2025 10:53

Artuomka requested changes Nov 23, 2025

View reviewed changes

	const connectionId = JSON.parse(createdConnection.text).id;
	// Removed unused connectionId variable

		jest.spyOn(auditLogRepository, 'save').mockResolvedValue({} as SecretAccessLogEntity);

		const result = await service.updateSecret('user-uuid-1', 'test-secret', updateDto);



	expect(result.slug).toBe('test-secret');
	expect(result.encryptedValue).toBe('new-value');

Uh oh!

secret storage #1447

Are you sure you want to change the base?

secret storage #1447

Uh oh!

Conversation

gugu commented Nov 21, 2025

Uh oh!

gugu commented Nov 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

Uh oh!

Artuomka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants