Skip to content

S3 widget secure paths upload fix #1532

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gugu wants to merge 3 commits into
base: main
Choose a base branch
from
Open

S3 widget secure paths upload fix #1532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91d7afc
Use nanoid for S3 file keys and fix new record upload preview
gugu Jan 23, 2026
4e4ba28
Refactor S3 service to use async/await instead of RxJS
gugu Jan 23, 2026
d861d1c
Merge branch 'main' into s3-widget-secure-paths-upload-fix
gugu Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/src/entities/s3-widget/application/data-structures/s3-operation.ds.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,5 @@ export class S3UploadUrlResponseDs {
uploadUrl: string;
key: string;
expiresIn: number;
previewUrl: string;
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
99 changes: 57 additions & 42 deletions backend/src/entities/s3-widget/s3-helper.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,51 +1,66 @@
import { S3Client, GetObjectCommand, PutObjectCommand } from '@aws-sdk/client-s3';
import { GetObjectCommand, PutObjectCommand, S3Client } from '@aws-sdk/client-s3';
import { getSignedUrl } from '@aws-sdk/s3-request-presigner';
import { Injectable } from '@nestjs/common';
import { nanoid } from 'nanoid';

@Injectable()
export class S3HelperService {
public createS3Client(accessKeyId: string, secretAccessKey: string, region: string = 'us-east-1'): S3Client {
return new S3Client({
region,
credentials: {
accessKeyId,
secretAccessKey,
},
});
}
public createS3Client(accessKeyId: string, secretAccessKey: string, region: string = 'us-east-1'): S3Client {
return new S3Client({
region,
credentials: {
accessKeyId,
secretAccessKey,
},
});
}

public async getSignedGetUrl(
client: S3Client,
bucket: string,
key: string,
expiresIn: number = 3600,
): Promise<string> {
const command = new GetObjectCommand({ Bucket: bucket, Key: key });
return getSignedUrl(client, command, { expiresIn });
}
public async getSignedGetUrl(
client: S3Client,
bucket: string,
key: string,
expiresIn: number = 3600,
): Promise<string> {
const command = new GetObjectCommand({ Bucket: bucket, Key: key });
return getSignedUrl(client, command, { expiresIn });
}

public async getSignedPutUrl(
client: S3Client,
bucket: string,
key: string,
contentType: string,
expiresIn: number = 3600,
): Promise<string> {
const command = new PutObjectCommand({
Bucket: bucket,
Key: key,
ContentType: contentType,
});
return getSignedUrl(client, command, { expiresIn });
}
public async getSignedPutUrl(
client: S3Client,
bucket: string,
key: string,
contentType: string,
expiresIn: number = 3600,
): Promise<string> {
const command = new PutObjectCommand({
Bucket: bucket,
Key: key,
ContentType: contentType,
});
return getSignedUrl(client, command, { expiresIn });
}

public generateFileKey(prefix: string | undefined, filename: string): string {
const timestamp = Date.now();
const sanitizedFilename = filename.replace(/[^a-zA-Z0-9._-]/g, '_');
if (prefix) {
const normalizedPrefix = prefix.replace(/\/$/, '');
return `${normalizedPrefix}/${timestamp}_${sanitizedFilename}`;
}
return `${timestamp}_${sanitizedFilename}`;
}
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;
}
Comment on lines +43 to +53
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment on lines +43 to +53
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment on lines +43 to +53
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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 '';
}
Comment on lines +43 to +65
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
Comment on lines +55 to +65
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
99 changes: 50 additions & 49 deletions backend/src/entities/s3-widget/use-cases/get-s3-upload-url.use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,78 +1,79 @@
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';
Comment on lines 1 to 13
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
import { WidgetTypeEnum } from '../../../enums/index.js';
import JSON5 from 'json5';

@Injectable()
export class GetS3UploadUrlUseCase
extends AbstractUseCase<S3GetUploadUrlDs, S3UploadUrlResponseDs>
implements IGetS3UploadUrl
extends AbstractUseCase<S3GetUploadUrlDs, S3UploadUrlResponseDs>
implements IGetS3UploadUrl
{
constructor(
@Inject(BaseType.GLOBAL_DB_CONTEXT)
protected _dbContext: IGlobalDatabaseContext,
private readonly s3Helper: S3HelperService,
) {
super();
}
constructor(
@Inject(BaseType.GLOBAL_DB_CONTEXT)
protected _dbContext: IGlobalDatabaseContext,
private readonly s3Helper: S3HelperService,
) {
super();
}

protected async implementation(inputData: S3GetUploadUrlDs): Promise<S3UploadUrlResponseDs> {
const { connectionId, tableName, fieldName, userId, masterPwd, filename, contentType } = inputData;
protected async implementation(inputData: S3GetUploadUrlDs): Promise<S3UploadUrlResponseDs> {
const { connectionId, tableName, fieldName, userId, masterPwd, filename, contentType } = inputData;

const user = await this._dbContext.userRepository.findOneUserByIdWithCompany(userId);
if (!user || !user.company) {
throw new HttpException({ message: Messages.USER_NOT_FOUND_OR_NOT_IN_COMPANY }, HttpStatus.NOT_FOUND);
}
const user = await this._dbContext.userRepository.findOneUserByIdWithCompany(userId);
if (!user || !user.company) {
throw new HttpException({ message: Messages.USER_NOT_FOUND_OR_NOT_IN_COMPANY }, HttpStatus.NOT_FOUND);
}

const foundTableWidgets = await this._dbContext.tableWidgetsRepository.findTableWidgets(connectionId, tableName);
const widget = foundTableWidgets.find((w) => w.field_name === fieldName);
const foundTableWidgets = await this._dbContext.tableWidgetsRepository.findTableWidgets(connectionId, tableName);
const widget = foundTableWidgets.find((w) => w.field_name === fieldName);

if (!widget || widget.widget_type !== WidgetTypeEnum.S3) {
throw new HttpException({ message: 'S3 widget not configured for this field' }, HttpStatus.BAD_REQUEST);
}
if (!widget || widget.widget_type !== WidgetTypeEnum.S3) {
throw new HttpException({ message: 'S3 widget not configured for this field' }, HttpStatus.BAD_REQUEST);
}

const params: S3WidgetParams =
typeof widget.widget_params === 'string' ? JSON5.parse(widget.widget_params) : widget.widget_params;
const params: S3WidgetParams =
typeof widget.widget_params === 'string' ? JSON5.parse(widget.widget_params) : widget.widget_params;

const accessKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
params.aws_access_key_id_secret_name,
user.company.id,
);
const accessKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
params.aws_access_key_id_secret_name,
user.company.id,
);

const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
params.aws_secret_access_key_secret_name,
user.company.id,
);
const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
params.aws_secret_access_key_secret_name,
user.company.id,
);

if (!accessKeySecret || !secretKeySecret) {
throw new HttpException({ message: 'AWS credentials secrets not found' }, HttpStatus.NOT_FOUND);
}
if (!accessKeySecret || !secretKeySecret) {
throw new HttpException({ message: 'AWS credentials secrets not found' }, HttpStatus.NOT_FOUND);
}

let accessKeyId = Encryptor.decryptData(accessKeySecret.encryptedValue);
let secretAccessKey = Encryptor.decryptData(secretKeySecret.encryptedValue);
let accessKeyId = Encryptor.decryptData(accessKeySecret.encryptedValue);
let secretAccessKey = Encryptor.decryptData(secretKeySecret.encryptedValue);

if (accessKeySecret.masterEncryption && masterPwd) {
accessKeyId = Encryptor.decryptDataMasterPwd(accessKeyId, masterPwd);
}
if (secretKeySecret.masterEncryption && masterPwd) {
secretAccessKey = Encryptor.decryptDataMasterPwd(secretAccessKey, masterPwd);
}
if (accessKeySecret.masterEncryption && masterPwd) {
accessKeyId = Encryptor.decryptDataMasterPwd(accessKeyId, masterPwd);
}
if (secretKeySecret.masterEncryption && masterPwd) {
secretAccessKey = Encryptor.decryptDataMasterPwd(secretAccessKey, masterPwd);
}

const client = this.s3Helper.createS3Client(accessKeyId, secretAccessKey, params.region || 'us-east-1');
const client = this.s3Helper.createS3Client(accessKeyId, secretAccessKey, params.region || 'us-east-1');

const key = this.s3Helper.generateFileKey(params.prefix, filename);
const expiresIn = 3600;
const uploadUrl = await this.s3Helper.getSignedPutUrl(client, params.bucket, key, contentType, expiresIn);
const key = this.s3Helper.generateFileKey(params.prefix, filename);
const expiresIn = 3600;
const uploadUrl = await this.s3Helper.getSignedPutUrl(client, params.bucket, key, contentType, expiresIn);
const previewUrl = await this.s3Helper.getSignedGetUrl(client, params.bucket, key, expiresIn);

return { uploadUrl, key, expiresIn };
}
return { uploadUrl, key, expiresIn, previewUrl };
}
}
28 changes: 14 additions & 14 deletions frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
<div class="s3-widget">
<mat-form-field class="s3-widget__key" appearance="outline">
<mat-label>{{normalizedLabel}}</mat-label>
<input matInput type="text" name="{{label}}-{{key}}"
[required]="required" [disabled]="disabled" [readonly]="readonly"
attr.data-testid="record-{{label}}-s3"
[(ngModel)]="value" (ngModelChange)="onFieldChange.emit($event)">
<mat-label>{{normalizedLabel()}}</mat-label>
<input matInput type="text" name="{{label()}}-{{key()}}"
[required]="required()" [disabled]="disabled()" [readonly]="readonly()"
attr.data-testid="record-{{label()}}-s3"
[ngModel]="internalValue() || value()" (ngModelChange)="onValueChange($event)">
</mat-form-field>

<div class="s3-widget__actions">
<input type="file" #fileInput hidden [accept]="fileAccept" (change)="onFileSelected($event)">
<input type="file" #fileInput hidden [accept]="fileAccept()" (change)="onFileSelected($event)">
<button mat-stroked-button type="button"
[disabled]="disabled || readonly || isLoading"
[disabled]="disabled() || readonly() || isLoading()"
(click)="fileInput.click()">
<mat-icon>upload</mat-icon>
Upload
</button>
<button mat-stroked-button type="button"
*ngIf="previewUrl"
*ngIf="previewUrl()"
(click)="openFile()">
<mat-icon>open_in_new</mat-icon>
Open
</button>
</div>

<div class="s3-widget__preview" *ngIf="value">
<mat-spinner *ngIf="isLoading" diameter="40"></mat-spinner>
<img *ngIf="isImage && previewUrl && !isLoading"
[src]="previewUrl"
<div class="s3-widget__preview" *ngIf="internalValue() || value()">
<mat-spinner *ngIf="isLoading()" diameter="40"></mat-spinner>
<img *ngIf="isImage() && previewUrl() && !isLoading()"
[src]="previewUrl()"
class="s3-widget__thumbnail"
alt="Preview">
<div *ngIf="!isImage && previewUrl && !isLoading" class="s3-widget__file-icon">
<div *ngIf="!isImage() && previewUrl() && !isLoading()" class="s3-widget__file-icon">
<mat-icon>insert_drive_file</mat-icon>
<span class="s3-widget__filename">{{value | slice:-30}}</span>
<span class="s3-widget__filename">{{(internalValue() || value()) | slice:-30}}</span>
</div>
</div>
</div>
Loading
Loading