S3 widget secure paths upload fix #1532

gugu · 2026-01-23T21:35:55Z

No description provided.

- Replace timestamp-based file keys with cryptographically secure nanoid - Return previewUrl from upload endpoint for immediate preview - Fix "File key not found in row" error when uploading to new records Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Replace Observable-based API with Promise-based async/await pattern for better integration with Angular signals. Update all S3 components and tests accordingly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot

Pull request overview

This pull request refactors the S3 widget implementation to use secure, randomized file paths and modernizes the frontend architecture with Angular signals.

Changes:

Backend file key generation changed from timestamp-based to nanoid-based randomized keys for enhanced security
Frontend S3 service converted from Observable-based to Promise-based async/await pattern
S3 edit component migrated from class-based inputs to Angular signal-based reactive architecture
Added previewUrl to upload response for immediate preview access after file upload

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
backend/src/entities/s3-widget/s3-helper.service.ts	Implements nanoid-based file key generation with extension validation, replacing timestamp-based naming
backend/src/entities/s3-widget/use-cases/get-s3-upload-url.use.case.ts	Adds previewUrl generation during upload URL creation
backend/src/entities/s3-widget/application/data-structures/s3-operation.ds.ts	Extends S3UploadUrlResponseDs with previewUrl field
frontend/src/app/services/s3.service.ts	Converts from Observable pattern to async/await with Promises, adds combined uploadFile method
frontend/src/app/services/s3.service.spec.ts	Updates tests for async/await pattern using fakeAsync and flush
frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.ts	Migrates to signal-based inputs/outputs, removes BaseEditFieldComponent inheritance
frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.spec.ts	Comprehensive test updates for signal-based component architecture
frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.html	Template updates to use signal getters with function call syntax
frontend/src/app/components/ui-components/table-display-fields/s3/s3.component.ts	Converts to async/await pattern for getFileUrl calls
frontend/src/app/components/ui-components/record-view-fields/s3/s3.component.ts	Converts to async/await pattern for getFileUrl calls

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

Copilot · 2026-01-23T21:41:03Z

frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.ts

+		// Initialize internal value from input
+		const initialValue = this.value();
+		if (initialValue) {
+			this.internalValue.set(initialValue);
 			this._loadPreview();
 		}


In ngOnInit, _loadPreview() is called directly (line 112) if initialValue exists. Since this is also an async function, this call is not awaited, which means the component initialization continues before the preview loads. While this might be intentional for performance, if _loadPreview() throws an error, it will become an unhandled promise rejection. Consider adding error handling or using async ngOnInit pattern if synchronous initialization is needed.

Copilot · 2026-01-23T21:41:04Z

backend/src/entities/s3-widget/application/data-structures/s3-operation.ds.ts

 	uploadUrl: string;
 	key: string;
 	expiresIn: number;
+	previewUrl: string;


The S3UploadUrlResponseDs now includes a previewUrl field (line 30), but the API documentation in the controller (lines 123-139) doesn't document this new response field. API consumers won't know about this field without updated documentation. Consider adding API response schema documentation using @apiresponse decorator with proper schema definition to document the complete response structure including the new previewUrl field.

Copilot · 2026-01-23T21:41:04Z

backend/src/entities/s3-widget/s3-helper.service.ts

+	public generateFileKey(prefix: string | undefined, filename: string): string {
+		const id = nanoid(12);
+		const extension = this._extractFileExtension(filename);
+		const key = extension ? `${id}${extension}` : id;
+
+		if (prefix) {
+			const normalizedPrefix = prefix.replace(/\/$/, '');
+			return `${normalizedPrefix}/${key}`;
+		}
+		return key;
+	}


The generateFileKey method now generates file keys using nanoid(12) instead of timestamp-based names. This is a breaking change that could affect existing S3 file paths and references. Files uploaded before this change will have keys like "1234567890_filename.pdf" while new files will have keys like "abc123xyz456.pdf". This inconsistency could cause issues with file organization, searching, and debugging. Consider the impact on existing data and whether a migration or versioning strategy is needed.

Copilot · 2026-01-23T21:41:05Z

backend/src/entities/s3-widget/s3-helper.service.ts

+	public generateFileKey(prefix: string | undefined, filename: string): string {
+		const id = nanoid(12);
+		const extension = this._extractFileExtension(filename);
+		const key = extension ? `${id}${extension}` : id;
+
+		if (prefix) {
+			const normalizedPrefix = prefix.replace(/\/$/, '');
+			return `${normalizedPrefix}/${key}`;
+		}
+		return key;
+	}


The new file key generation removes the original filename from the S3 key (except for the extension). This means stored keys like "abc123xyz456.pdf" provide no human-readable information about the file contents, making it harder to identify files when browsing S3 directly, during debugging, or when reviewing logs. The previous timestamp-based approach with sanitized filenames (e.g., "1234567890_my_document.pdf") was more traceable. Consider preserving some portion of the original filename for debugging and operational purposes.

Copilot · 2026-01-23T21:41:05Z

backend/src/entities/s3-widget/s3-helper.service.ts

+	public generateFileKey(prefix: string | undefined, filename: string): string {
+		const id = nanoid(12);
+		const extension = this._extractFileExtension(filename);
+		const key = extension ? `${id}${extension}` : id;
+
+		if (prefix) {
+			const normalizedPrefix = prefix.replace(/\/$/, '');
+			return `${normalizedPrefix}/${key}`;
+		}
+		return key;
+	}
+
+	private _extractFileExtension(filename: string): string {
+		const lastDotIndex = filename.lastIndexOf('.');
+		if (lastDotIndex === -1 || lastDotIndex === 0) {
+			return '';
+		}
+		const extension = filename.slice(lastDotIndex).toLowerCase();
+		if (/^\.[a-z0-9]+$/i.test(extension)) {
+			return extension;
+		}
+		return '';
+	}


The generateFileKey and _extractFileExtension methods in S3HelperService lack test coverage. Given that this is critical functionality that affects how files are stored in S3 and includes security-related filename validation, comprehensive tests should be added to verify correct behavior for various filename patterns, edge cases, and security scenarios (e.g., path traversal attempts, invalid characters, etc.).

Copilot · 2026-01-23T21:41:06Z

frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.ts

 	ngOnInit(): void {
-		super.ngOnInit();
 		this.connectionId = this.connectionsService.currentConnectionID;
 		this.tableName = this.tablesService.currentTableName;
-		this._parseWidgetParams();
-		if (this.value) {
-			this._loadPreview();
-		}
-	}

-	ngOnChanges(): void {
-		this._parseWidgetParams();
-		if (this.value && !this.previewUrl && !this.isLoading) {
+		// Initialize internal value from input
+		const initialValue = this.value();
+		if (initialValue) {
+			this.internalValue.set(initialValue);
 			this._loadPreview();
 		}


Both the effect (lines 95-101) and ngOnInit (lines 108-113) can trigger _loadPreview() for the same initial value. If a component is initialized with a value and rowPrimaryKey, ngOnInit calls _loadPreview() at line 112. The effect will also trigger because value() and rowPrimaryKey() are set, calling _loadPreview() again. This results in two concurrent HTTP requests for the same preview URL, wasting resources and potentially causing race conditions. Consider either removing the _loadPreview() call from ngOnInit or adding a flag to prevent duplicate requests.

Copilot · 2026-01-23T21:41:06Z

backend/src/entities/s3-widget/use-cases/get-s3-upload-url.use.case.ts

 import { HttpStatus, Inject, Injectable } from '@nestjs/common';
 import { HttpException } from '@nestjs/common/exceptions/http.exception.js';
+import JSON5 from 'json5';
 import AbstractUseCase from '../../../common/abstract-use.case.js';
 import { IGlobalDatabaseContext } from '../../../common/application/global-database-context.interface.js';
 import { BaseType } from '../../../common/data-injection.tokens.js';
+import { WidgetTypeEnum } from '../../../enums/index.js';
 import { Messages } from '../../../exceptions/text/messages.js';
 import { Encryptor } from '../../../helpers/encryption/encryptor.js';
 import { S3GetUploadUrlDs, S3UploadUrlResponseDs } from '../application/data-structures/s3-operation.ds.js';
 import { S3WidgetParams } from '../application/data-structures/s3-widget-params.ds.js';
 import { S3HelperService } from '../s3-helper.service.js';
 import { IGetS3UploadUrl } from './s3-use-cases.interface.js';


The import statements for WidgetTypeEnum and JSON5 have been reordered (moved from lines 14-15 to lines 3 and 7). While this doesn't affect functionality, it breaks the apparent import organization convention where external library imports (JSON5) come before internal imports. This makes the import section less readable.

Copilot · 2026-01-23T21:41:06Z

frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.ts

@@ -1,23 +1,24 @@
 import { CommonModule } from '@angular/common';
-import { Component, Input, OnInit } from '@angular/core';
+import { Component, computed, effect, inject, input, OnInit, output, signal } from '@angular/core';


The S3EditComponent has been changed from extending BaseEditFieldComponent to implementing its own signal-based inputs. However, all other edit field components in the codebase still extend BaseEditFieldComponent (see frontend/src/app/components/ui-components/record-edit-fields/*/component.ts files). This creates an inconsistency in the codebase architecture. Consider either keeping the BaseEditFieldComponent inheritance or establishing a clear pattern for when to use signals vs class-based inputs across all edit field components.

Copilot · 2026-01-23T21:41:06Z

backend/src/entities/s3-widget/s3-helper.service.ts

+	private _extractFileExtension(filename: string): string {
+		const lastDotIndex = filename.lastIndexOf('.');
+		if (lastDotIndex === -1 || lastDotIndex === 0) {
+			return '';
+		}
+		const extension = filename.slice(lastDotIndex).toLowerCase();
+		if (/^\.[a-z0-9]+$/i.test(extension)) {
+			return extension;
+		}
+		return '';
+	}


The _extractFileExtension method only validates extensions matching /^.[a-z0-9]+$/i pattern. This will reject valid file extensions containing multiple dots (e.g., ".tar.gz", ".min.js") or other characters (e.g., ".c++"). While these might be edge cases, rejecting them silently by returning an empty string could cause confusion when users upload such files and they lose their extension.

Copilot · 2026-01-23T21:41:07Z

backend/src/entities/s3-widget/s3-helper.service.ts

+	public generateFileKey(prefix: string | undefined, filename: string): string {
+		const id = nanoid(12);
+		const extension = this._extractFileExtension(filename);
+		const key = extension ? `${id}${extension}` : id;
+
+		if (prefix) {
+			const normalizedPrefix = prefix.replace(/\/$/, '');
+			return `${normalizedPrefix}/${key}`;
+		}
+		return key;
+	}


The generateFileKey method doesn't sanitize or validate the prefix parameter before using it in the S3 key. A malicious or misconfigured prefix containing path traversal sequences (e.g., "../") or special characters could lead to files being written to unexpected S3 locations. Consider validating the prefix to ensure it doesn't contain path traversal patterns or other potentially dangerous characters.

gugu and others added 2 commits January 23, 2026 13:06

Refactor S3 service to use async/await instead of RxJS

4e4ba28

Replace Observable-based API with Promise-based async/await pattern for better integration with Angular signals. Update all S3 components and tests accordingly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings January 23, 2026 21:35

Merge branch 'main' into s3-widget-secure-paths-upload-fix

d861d1c

gugu requested a review from lyubov-voloshko January 23, 2026 21:36

Copilot started reviewing on behalf of gugu January 23, 2026 21:36 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

S3 widget secure paths upload fix #1532

S3 widget secure paths upload fix #1532

Uh oh!

gugu commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

S3 widget secure paths upload fix #1532

Are you sure you want to change the base?

S3 widget secure paths upload fix #1532

Uh oh!

Conversation

gugu commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants