# Attachment / File Upload — Deep Code Analysis **Date:** 2026-03-03 **Scope:** All Attachment and File upload/download code in GB5Framework **Modules analysed:** - `FrameworkBLL/Attachment/` — AttachmentBLL, IAttachmentBLL - `FrameworkBLL/File/` — FileBLL, IFileBLL - `FrameworkDAL/CustomCode/Attachment/` — AttachmentDAL, IAttachmentDAL - `FrameworkDAL/CustomCode/File/` — FileDAL, IFileDAL - `FrameworkDAL/Query/Attachment/` — AttachmentQB - `FrameworkDAL/Query/File/` — FileQB - `FrameworkDAL/DTO/` — FileDTO, AttachmentDTO, MobileAttachmentDTO, FileUploadUsingBase64DTO, AlfrescoAttachmentDTO, BlobImportDTO, FilePathDTO - `FrameworkSL/Controllers/FileUpload/` — UploadService (MVC), AribaFileReceive (MVC) - `FrameworkSL/Endpoints/Attachment/` — GetAttachment, SaveAttachment, DeleteAttachment, GetSelectListAttachment - `GB5Shared/FileUpload/FileStorage.cs` --- ## Executive Summary The Attachment subsystem is in a **critically incomplete and insecure state**. The designed Attachment CRUD layer (BLL, DAL, QB, all four FastEndpoints) consists entirely of **empty stub classes** — the feature is unimplemented. The actual file handling uses a parallel, ad-hoc implementation split across two MVC Controllers (`UploadService`, `AribaFileReceive`) and a `FileStorage` utility class. This ad-hoc path has **no authentication**, **no file type validation**, **no size limits**, **hardcoded internal server credentials**, **SQL injection–adjacent syntax errors**, **path traversal vulnerabilities**, and **cross-tenant data leakage**. Additionally, the `IAttachmentBLL` and `IAttachmentDAL` files are declared as `internal class` instead of `interface`, making them structurally broken as DI contracts. **Total issues found:** 35 - Critical: 8 - High: 10 - Medium: 10 - Low: 7 --- ## Issue Index | # | Severity | Category | Title | |---|----------|----------|-------| | 1 | CRITICAL | VAPT | Path traversal via client-supplied filename | | 2 | CRITICAL | VAPT | No authentication on file upload/download endpoints | | 3 | CRITICAL | VAPT | No file type or MIME validation | | 4 | CRITICAL | VAPT | Hardcoded internal IP and UNC path in source code | | 5 | CRITICAL | VAPT | No file size limits — DoS vector | | 6 | CRITICAL | VAPT | Cross-tenant file metadata access — no tenant filter in SQL | | 7 | CRITICAL | Architecture | `IAttachmentBLL` and `IAttachmentDAL` declared as `class`, not `interface` | | 8 | CRITICAL | Architecture | Entire Attachment CRUD layer is empty stub code — feature unimplemented | | 9 | HIGH | VAPT | Glob injection in `GetPdf` via `fileId` wildcard | | 10 | HIGH | VAPT | `AribaFileReceive` stores unvalidated XML from external client to disk | | 11 | HIGH | Performance | `File.ReadAllBytes` for PDF download — OOM on large files | | 12 | HIGH | Memory | `FileUploadUsingBase64DTO.FileBody` unbounded base64 string | | 13 | HIGH | Architecture | MVC Controllers violating FastEndpoints-only policy | | 14 | HIGH | Bug | SQL syntax error in `GET_SELECTLIST_FILE` — `FROMMFILE` | | 15 | HIGH | Architecture | No linkage between physical file storage and DB metadata record | | 16 | HIGH | Best Practice | `ValidationException` wrapped in `Exception` in FileBLL | | 17 | HIGH | Best Practice | `IValidation` injected into DAL — belongs in BLL | | 18 | HIGH | Bug | Literal string `{FileDTO.FileId}` returned instead of interpolated value | | 19 | MEDIUM | VAPT | `MobileAttachmentDTO.FileExtension` not validated — client can claim any extension | | 20 | MEDIUM | VAPT | No malware/virus scanning of uploaded files | | 21 | MEDIUM | Reliability | `Directory.CreateDirectory` on UNC share called in constructor — startup failure | | 22 | MEDIUM | Performance | Serial bulk upload — `foreach` with no parallelism | | 23 | MEDIUM | Memory | `BlobImportDTO.PictureData` as `byte[]` — large in-memory image allocation | | 24 | MEDIUM | Best Practice | `FileDTO` uses 50+ Java-era private backing fields | | 25 | MEDIUM | Best Practice | `finally { Json = null; }` is a no-op in FileDAL | | 26 | MEDIUM | Best Practice | `null!` passed to `QueryAsync` in `GetSelectListFile` | | 27 | MEDIUM | Best Practice | No logging or audit trail for file upload/download/delete | | 28 | MEDIUM | Best Practice | `AttachmentDTO.AttachmentTag` typed as `object` — unsafe deserialization | | 29 | LOW | Architecture | Alfresco / `AlfrescoAttachmentDTO` — no apparent active usage | | 30 | LOW | Best Practice | `FilePathDTO` wraps single string with Java-era backing field | | 31 | LOW | Best Practice | Unused `StackExchange.Redis` import in FileQB.cs | | 32 | LOW | Architecture | `FileStorage` is registered nowhere in DI — consumed ad-hoc | | 33 | LOW | Best Practice | All 4 Attachment FastEndpoints are empty stub classes | | 34 | LOW | Best Practice | `GET_SELECTLIST_FILE` returns all files with `STATUS = 1` — no WHERE on entity | | 35 | LOW | Performance | No cache key or response caching on file metadata endpoints | --- ## Detailed Findings --- ### CRITICAL-1 — Path Traversal via Client-Supplied Filename **File:** `GB5Shared/FileUpload/FileStorage.cs` **Lines:** 13–21 ```csharp public async Task SaveAsync(Stream stream, string fileName, string folder) { var id = Guid.NewGuid().ToString(); var dir = Path.Combine(_root, folder); Directory.CreateDirectory(dir); var fullPath = Path.Combine(dir, $"{id}_{fileName}"); // ← fileName from client using var fs = new FileStream(fullPath, FileMode.Create, ...); await stream.CopyToAsync(fs); return id; } ``` `fileName` is passed directly from `IFormFile.FileName` (client-supplied) into `Path.Combine`. A crafted filename such as `../../appsettings.json` or `..\..\..\windows\system32\config\sam` can escape the intended storage directory. **Fix:** ```csharp // Sanitize: strip directory components and limit to safe characters var safeName = Path.GetFileName(fileName); // strips path components safeName = Regex.Replace(safeName, @"[^\w\-.]", "_"); // allow only safe chars if (string.IsNullOrEmpty(safeName)) safeName = "file"; var fullPath = Path.Combine(dir, $"{id}_{safeName}"); ``` Additionally, validate that the resolved full path still starts with `_root` (canonical path check): ```csharp var canonical = Path.GetFullPath(fullPath); if (!canonical.StartsWith(Path.GetFullPath(_root), StringComparison.OrdinalIgnoreCase)) throw new SecurityException("Illegal file path."); ``` --- ### CRITICAL-2 — No Authentication on File Upload/Download Endpoints **File:** `FrameworkSL/Controllers/FileUpload/UploadService.cs` ```csharp [ApiController] [Route("[controller]")] public class UploadService : ControllerBase { [HttpPost("UploadExcel")] public async Task UploadExcel(IFormFile file) { ... } [HttpPost("UploadCsv")] public async Task UploadCsv(IFormFile file) { ... } [HttpPost("UploadPdf")] public async Task UploadPdf(IFormFile file) { ... } [HttpGet("GetPdf/{fileId}")] public IActionResult GetPdf(string fileId) { ... } // No [Authorize] anywhere } ``` There is no `[Authorize]` attribute at class or method level, and no JWT/OIDC middleware validation. Any anonymous caller can upload arbitrary files to the server or download any file by guessing a GUID. **Fix:** Add `[Authorize]` at the controller level, validate `LoginDTO` from headers on each request, and enforce tenant scoping on download (confirm the requested file belongs to the caller's tenant). --- ### CRITICAL-3 — No File Type or MIME Validation **File:** `FrameworkSL/Controllers/FileUpload/UploadService.cs` Despite routes named `UploadExcel`, `UploadCsv`, and `UploadPdf`, none of the upload handlers validate the file's MIME type, content signature (magic bytes), or extension. An attacker can: - Upload a `.aspx` WebShell through the "Excel" endpoint and gain Remote Code Execution if the storage root is web-accessible. - Upload executable malware that is later executed by other system processes. - Upload a crafted file that exploits document parser vulnerabilities in downstream processing. **Fix:** ```csharp private static readonly HashSet AllowedExcelExtensions = new(StringComparer.OrdinalIgnoreCase) { ".xlsx", ".xls" }; private static readonly byte[] XlsxMagic = { 0x50, 0x4B, 0x03, 0x04 }; private async Task IsValidExcelAsync(IFormFile file) { if (!AllowedExcelExtensions.Contains(Path.GetExtension(file.FileName))) return false; using var ms = new MemoryStream(4); await file.OpenReadStream().CopyToAsync(ms); var header = ms.ToArray(); return header.Length >= 4 && header[..4].SequenceEqual(XlsxMagic); } ``` --- ### CRITICAL-4 — Hardcoded Internal IP and UNC Path in Source Code **File:** `GB5Shared/FileUpload/FileStorage.cs`, line 7 ```csharp _root = @"\\172.16.200.39\E$\Unisoft\GB5Index"; ``` Problems: 1. **Credential exposure**: UNC share `E$` is an administrative share requiring domain/local admin credentials — embedding this in source code reveals internal network topology. 2. **SSRF risk**: If an attacker gains control of what folder names or paths are passed, UNC paths could be redirected. 3. **Configuration management**: This hardcoded value cannot differ between development, staging, and production environments. 4. **Version control exposure**: This internal IP is now in git history permanently. **Fix:** Move the storage root to configuration (`appsettings.json` / Vault): ```csharp public FileStorage(IConfiguration config) { _root = config["FileStorage:Root"] ?? throw new InvalidOperationException("FileStorage:Root not configured."); } ``` Store the actual value in HashiCorp Vault (as used elsewhere in the project), not in `appsettings.json`. --- ### CRITICAL-5 — No File Size Limits — DoS Vector **File:** `FrameworkSL/Controllers/FileUpload/UploadService.cs` None of the upload methods check `file.Length` before processing. An attacker can send a multi-gigabyte file to: - Exhaust server disk space. - Cause OOM on the server (especially combined with CRITICAL-3 and the base64 paths). - Slow down the server for all other users (DoS). **Fix:** ```csharp private const long MaxExcelSize = 10 * 1024 * 1024; // 10 MB if (file.Length > MaxExcelSize) return BadRequest($"File exceeds maximum allowed size of {MaxExcelSize / 1024 / 1024} MB."); ``` Also configure ASP.NET Core's `RequestSizeLimitAttribute` and Kestrel's `MaxRequestBodySize`: ```csharp [RequestSizeLimit(10 * 1024 * 1024)] [HttpPost("UploadExcel")] public async Task UploadExcel(IFormFile file) { ... } ``` --- ### CRITICAL-6 — Cross-Tenant File Metadata Access — No Tenant Filter in SQL **File:** `FrameworkDAL/Query/File/FileQB.cs` ```csharp public const string GET_FILE = @" SELECT F.FILEID, F.FILENAME, ... FROM MFILE F WHERE F.FILEID = @fileid"; // ← no ClientId / DatabaseName filter public const string GET_SELECTLIST_FILE = @" SELECT F.FILEID, F.FILENAME, ... FROMMFILE F // ← also syntax error WHERE F.STATUS = 1"; // ← no tenant filter at all public const string DELETE_FILE = @" UPDATE MFILE SET STATUS = 0 WHERE FILEID = @fileid"; // ← no tenant filter ``` Any authenticated tenant can query, list, or soft-delete files belonging to any other tenant by supplying a known `FILEID`. This is a direct IDOR (Insecure Direct Object Reference) vulnerability. **Fix:** Add mandatory tenant filter to every query: ```csharp public const string GET_FILE = @" SELECT F.FILEID, F.FILENAME, ... FROM MFILE F WHERE F.FILEID = @FileId AND F.DATABASENAME = @DatabaseName"; public const string DELETE_FILE = @" UPDATE MFILE SET STATUS = 0 WHERE FILEID = @FileId AND DATABASENAME = @DatabaseName"; ``` --- ### CRITICAL-7 — `IAttachmentBLL` and `IAttachmentDAL` Declared as `class`, Not `interface` **Files:** - `FrameworkBLL/Attachment/IAttachmentBLL.cs` - `FrameworkDAL/CustomCode/Attachment/IAttachmentDAL.cs` ```csharp // IAttachmentBLL.cs internal class IAttachmentBLL { } // ← should be "interface IAttachmentBLL" // IAttachmentDAL.cs internal class IAttachmentDAL { } // ← should be "interface IAttachmentDAL" ``` These are placeholder classes masquerading as interfaces. They cannot be used as DI contracts, cannot be implemented, and break the entire 3-tier contract pattern. Additionally, `internal` visibility prevents cross-assembly usage (SL → BLL, BLL → DAL). **Fix:** ```csharp // IAttachmentBLL.cs public interface IAttachmentBLL { Task GetAttachment(int attachmentId, LoginDTO login, CancellationToken ct); Task SaveAttachment(AttachmentDTO dto, LoginDTO login, CancellationToken ct); Task DeleteAttachment(int attachmentId, LoginDTO login, CancellationToken ct); Task GetSelectListAttachment(LoginDTO login, CancellationToken ct); } ``` --- ### CRITICAL-8 — Entire Attachment CRUD Layer Is Empty Stub Code **Files affected:** | File | Content | |------|---------| | `AttachmentBLL.cs` | `internal class AttachmentBLL { }` | | `AttachmentDAL.cs` | `internal class AttachmentDAL { }` | | `AttachmentQB.cs` | `internal class AttachmentQB { }` | | `GetAttachment.cs` | `public class GetAttachment { }` | | `SaveAttachment.cs` | `public class SaveAttachment { }` | | `DeleteAttachment.cs` | `public class DeleteAttachment { }` | | `GetSelectListAttachment.cs` | `public class GetSelectListAttachment { }` | All seven files contain empty class bodies. The Attachment feature is entirely unimplemented at the designed layer. The codebase falls back to the ad-hoc `UploadService` MVC controller path, which has all the critical issues documented above. **Impact:** Any frontend call to `/Attachment/*` endpoints will fail or return no response. Attachment functionality used across modules (PO, SO, invoices, HR documents) has no working backend. --- ### HIGH-9 — Glob Injection in `GetPdf` via `fileId` **File:** `FrameworkSL/Controllers/FileUpload/UploadService.cs` ```csharp [HttpGet("GetPdf/{fileId}")] public IActionResult GetPdf(string fileId) { var root = @"\\172.16.200.39\E$\Unisoft\GB5Index"; var files = Directory.GetFiles(root, $"{fileId}_*"); // ← fileId from URL if (files.Length == 0) return NotFound(); var bytes = File.ReadAllBytes(files[0]); return File(bytes, "application/pdf"); } ``` `fileId` from the URL is used directly in a `Directory.GetFiles` glob pattern. A malicious `fileId` of `*` would match all files in the root. A fileId of `../secret` allows traversal. This also returns the first matching file, potentially leaking another tenant's file if GUID collision occurs. **Fix:** Validate `fileId` is a valid GUID before use, then perform an exact filename match: ```csharp if (!Guid.TryParse(fileId, out var guid)) return BadRequest("Invalid file identifier."); // Use exact pattern only — no wildcards in user input var files = Directory.GetFiles(dir, "*", SearchOption.TopDirectoryOnly) .Where(f => Path.GetFileName(f).StartsWith(guid.ToString() + "_", StringComparison.Ordinal)) .ToArray(); ``` --- ### HIGH-10 — `AribaFileReceive` Stores Unvalidated External XML to Disk **File:** `FrameworkSL/Controllers/FileUpload/AribaFileReceive.cs` The Ariba cXML integration receives files from an external system and saves them without: - Authentication verification of the Ariba sender. - XML schema validation. - File size limits. - Content sanitization. - Malware scanning. An attacker spoofing the Ariba sender can store arbitrary files (including malicious payloads) into the application's file system. **Fix:** - Validate sender via shared secret / HMAC header. - Validate XML against Ariba cXML schema (XSD). - Apply file size limit. - Store to an isolated quarantine directory, validate, then move. --- ### HIGH-11 — `File.ReadAllBytes` for PDF Download — OOM on Large Files **File:** `FrameworkSL/Controllers/FileUpload/UploadService.cs` ```csharp var bytes = File.ReadAllBytes(files[0]); // loads entire file into byte array return File(bytes, "application/pdf"); ``` For a 500MB PDF, this allocates 500MB on the LOH (Large Object Heap). Concurrent downloads will multiply this. The LOH is not compacted by default, causing long-term heap fragmentation and GC pressure. **Fix:** Stream the file directly: ```csharp var stream = new FileStream(files[0], FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 81920, useAsync: true); return File(stream, "application/pdf", enableRangeProcessing: true); // ASP.NET Core disposes the stream after the response is sent ``` --- ### HIGH-12 — `FileUploadUsingBase64DTO.FileBody` Unbounded Base64 String **File:** `FrameworkDAL/DTO/File/FileUploadUsingBase64DTO.cs` ```csharp public class FileUploadUsingBase64DTO { public string FileBody { get; set; } // base64-encoded file content public string FileName { get; set; } public string FileType { get; set; } } ``` Base64 encoding has ~33% overhead. A 100MB file becomes a 133MB string. This string is: - Allocated in full on the heap (not streamed). - Cannot be collected until the GC runs. - Duplicated when deserialized from JSON. For mobile clients uploading photos or documents, this will cause repeated OOM or GC pauses. **Fix:** Use chunked/multipart upload instead of base64. If base64 is required for mobile compatibility, enforce a strict size limit: ```csharp // In BLL validation if (dto.FileBody?.Length > 10 * 1024 * 1024) // 10MB base64 ≈ 7.5MB file throw new ValidationException("File too large for base64 upload. Use multipart."); ``` --- ### HIGH-13 — MVC Controllers Violating FastEndpoints-Only Policy **Files:** - `FrameworkSL/Controllers/FileUpload/UploadService.cs` (`ControllerBase`) - `FrameworkSL/Controllers/FileUpload/AribaFileReceive.cs` (`ControllerBase`) CLAUDE.md states: *"No traditional MVC controllers. All HTTP endpoints use FastEndpoints."* Both controllers use `[ApiController]`, `ControllerBase`, `[HttpPost]` attributes — the prohibited pattern. This creates inconsistency in middleware pipeline, error handling, authentication, and response formatting. **Fix:** Migrate to FastEndpoints: ```csharp public class UploadExcel : BaseEndpoint> { public override void Configure() { Post("/File/UploadExcel"); // AllowFileUploads(); } protected override async Task> ExecuteAsync( UploadExcelParameters req, LoginDTO login, CancellationToken ct) { // validate, save, return } } ``` --- ### HIGH-14 — SQL Syntax Error in `GET_SELECTLIST_FILE` **File:** `FrameworkDAL/Query/File/FileQB.cs` ```csharp public const string GET_SELECTLIST_FILE = @" SELECT F.FILEID FILEID, F.FILENAME DESCRIPTION, F.STATUS STATUS FROMMFILE F -- ← missing space: should be FROM MFILE WHERE F.STATUS = 1"; ``` `FROMMFILE` is not valid SQL. This query will throw a runtime SQL exception on every call to `GetSelectListFile`. The bug has been present since the file was created and would cause compilation-time or startup-time failures if query validation were in place. **Fix:** ```csharp public const string GET_SELECTLIST_FILE = @" SELECT F.FILEID FILEID, F.FILENAME DESCRIPTION, F.STATUS STATUS FROM MFILE F WHERE F.STATUS = 1 AND F.DATABASENAME = @DatabaseName"; ``` --- ### HIGH-15 — No Linkage Between Physical File Storage and DB Metadata **Design flaw spanning multiple files** `FileStorage.SaveAsync` returns a GUID string: ```csharp public async Task SaveAsync(Stream stream, string fileName, string folder) { var id = Guid.NewGuid().ToString(); // ... stores file as {id}_{fileName} ... return id; } ``` `FileBLL.SaveFile` saves metadata to `MFILE` table via `FileDAL.SaveFile`, but does not call `FileStorage.SaveAsync`. The two paths are entirely disconnected: - **UploadService** calls `FileStorage.SaveAsync` → returns GUID → no DB record. - **FileBLL.SaveFile** calls `FileDAL.SaveFile` → inserts into MFILE → no physical file. Result: Physical files are orphaned (no metadata), or metadata records are orphaned (no physical file), making it impossible to reliably retrieve uploaded files by their database ID. **Fix:** Create a unified `AttachmentService` that atomically: 1. Validates the file. 2. Saves physical file via `FileStorage` and captures the path/GUID. 3. Saves metadata to `MFILE` with the physical path reference. 4. Returns the `FILEID` to the caller. Use a DB transaction to ensure atomicity; delete the physical file on DB failure. --- ### HIGH-16 — `ValidationException` Wrapped in `Exception` in FileBLL **File:** `FrameworkBLL/File/FileBLL.cs` ```csharp catch (ValidationException ex) { throw new Exception(ex.Message); // ← loses type and stack trace } ``` Wrapping in `Exception` loses the type (callers cannot `catch (ValidationException)`) and the original stack trace. The SL layer's exception handler cannot distinguish validation errors from system errors, returning HTTP 500 for what should be HTTP 400. **Fix:** ```csharp catch (ValidationException) { throw; // let caller handle validation exceptions by type } ``` --- ### HIGH-17 — `IValidation` Injected into DAL — Belongs in BLL **File:** `FrameworkDAL/CustomCode/File/FileDAL.cs` ```csharp public class FileDAL : IFileDAL { private readonly IQueryExecutor _queryExecutor; private readonly IValidation _validation; // ← validation in DAL public FileDAL(IQueryExecutor queryExecutor, IValidation validation) { _queryExecutor = queryExecutor; _validation = validation; } } ``` CLAUDE.md states: *"DAL must never contain business logic; only data retrieval/mutation."* Validation is business logic. Its presence in DAL means validation runs after the BLL layer, bypassing the intended separation. Validation exceptions from DAL are harder to handle gracefully. **Fix:** Remove `IValidation` from `FileDAL`. Move all validation calls to `FileBLL` before the DAL call. --- ### HIGH-18 — Literal String `{FileDTO.FileId}` Returned Instead of Interpolated Value **File:** `FrameworkBLL/File/FileBLL.cs` ```csharp return "Details updated successfully with File Id {FileDTO.FileId}."; ``` Missing `$` prefix — this returns the literal string `"Details updated successfully with File Id {FileDTO.FileId}."` instead of the actual FileId value. Every update response will return the same unhelpful string. **Fix:** ```csharp return $"Details updated successfully with File Id {FileDTO.FileId}."; ``` --- ### MEDIUM-19 — `MobileAttachmentDTO.FileExtension` Not Validated **File:** `FrameworkDAL/DTO/Attachment/MobileAttachmentDTO.cs` ```csharp public class MobileAttachmentDTO { public string FileBody { get; set; } // base64 public string FileExtension { get; set; } // e.g. ".pdf", ".jpg" — no validation public string FileName { get; set; } } ``` Mobile clients can claim any file extension. A `.apk`, `.exe`, or `.aspx` file submitted as `.jpg` will bypass any extension-based filtering in downstream processing. **Fix:** Define an allowlist and validate in BLL before processing: ```csharp private static readonly HashSet AllowedExtensions = new(StringComparer.OrdinalIgnoreCase) { ".pdf", ".jpg", ".jpeg", ".png", ".xlsx", ".docx" }; if (!AllowedExtensions.Contains(dto.FileExtension)) throw new ValidationException($"File type '{dto.FileExtension}' is not permitted."); ``` --- ### MEDIUM-20 — No Malware/Virus Scanning of Uploaded Files All upload paths (`UploadService`, `AribaFileReceive`, `FileBLL`) store files to disk without any antivirus or malware scanning. Uploaded files that are later opened by other users or processed by document parsers can deliver malware to internal systems. **Fix:** Integrate ClamAV (open-source) or a cloud AV API (Azure Defender for Storage, AWS Malware Protection) to scan files before moving them from quarantine to permanent storage. --- ### MEDIUM-21 — `Directory.CreateDirectory` on UNC Share Called in Constructor — Startup Failure **File:** `GB5Shared/FileUpload/FileStorage.cs` ```csharp public FileStorage() { _root = @"\\172.16.200.39\E$\Unisoft\GB5Index"; Directory.CreateDirectory(_root); // ← called at DI construction time } ``` If the network share is unavailable (server down, network partition, permissions changed), `Directory.CreateDirectory` throws an `IOException` during DI resolution, crashing the entire application at startup before it can serve any requests — including health checks. **Fix:** Use lazy initialization or a startup health check: ```csharp public async Task EnsureStorageReadyAsync(CancellationToken ct) { await Task.Run(() => Directory.CreateDirectory(_root), ct); } ``` Register as a `IHostedService` startup check, not in the constructor. --- ### MEDIUM-22 — Serial Bulk Upload — No Parallelism **File:** `FrameworkSL/Controllers/FileUpload/UploadService.cs` ```csharp public async Task UploadBulk(List files) { var results = new List(); foreach (var file in files) // ← serial { var id = await _fileStorage.SaveAsync(file.OpenReadStream(), file.FileName, "bulk"); results.Add(id); } return Ok(results); } ``` Files are saved one at a time. For 50 files, each with ~50ms I/O, total time is 2.5 seconds minimum. Network latency to the UNC share multiplies this. **Fix:** ```csharp var tasks = files.Select(f => _fileStorage.SaveAsync(f.OpenReadStream(), f.FileName, "bulk")); var results = await Task.WhenAll(tasks); return Ok(results); ``` Apply a `SemaphoreSlim` to cap concurrency and avoid overwhelming the UNC share. --- ### MEDIUM-23 — `BlobImportDTO.PictureData` as `byte[]` — Large In-Memory Image **File:** `FrameworkDAL/DTO/Alfresco/BlobImportDTO.cs` ```csharp public class BlobImportDTO { public byte[] PictureData { get; set; } // full image in memory public string FileName { get; set; } } ``` For bulk image imports (e.g., employee photos, product images), each DTO holds the entire image as a `byte[]`. Processing 1,000 × 1MB images allocates 1GB on the heap simultaneously if not carefully paged. **Fix:** Accept `Stream` or a file path reference instead. Process images one at a time with `using` blocks to release memory promptly. --- ### MEDIUM-24 — `FileDTO` Uses 50+ Java-Era Private Backing Fields **File:** `FrameworkDAL/DTO/File/FileDTO.cs` The DTO declares private fields with manual getters/setters, a pattern from Java-era C# (pre-auto-properties): ```csharp private int _fileId; private string _fileName; // ... 50+ more ... public int FileId { get { return _fileId; } set { _fileId = value; } } public string FileName { get { return _fileName; } set { _fileName = value; } } ``` This is ~200 lines of code that could be 20 lines with auto-properties. The verbosity obscures the actual DTO shape and increases maintenance cost. **Fix:** Replace with auto-properties: ```csharp public class FileDTO { public int FileId { get; set; } public string FileName { get; set; } // ... } ``` --- ### MEDIUM-25 — `finally { Json = null; }` Is a No-Op in FileDAL **File:** `FrameworkDAL/CustomCode/File/FileDAL.cs` ```csharp string? Json = null; try { Json = JsonSerializer.Serialize(dto); // ... } finally { Json = null; // ← sets local variable to null; no effect on GC } ``` Setting a local `string` variable to `null` in a `finally` block does not help GC — the variable goes out of scope at the end of the method anyway. This is misleading cargo-cult code. **Fix:** Remove the `finally` block entirely. --- ### MEDIUM-26 — `null!` Passed to `QueryAsync` in `GetSelectListFile` **File:** `FrameworkDAL/CustomCode/File/FileDAL.cs` ```csharp return (await _queryExecutor.QueryAsync(login, FileQB.GET_SELECTLIST_FILE, null!, ct)).ToList(); ``` `null!` suppresses the null-safety warning but passes `null` as the query parameters object. `IQueryExecutor.QueryAsync` presumably requires a non-null parameters object. This will fail at runtime with a `NullReferenceException` inside Dapper when it tries to enumerate the parameters. **Fix:** ```csharp return (await _queryExecutor.QueryAsync( login, FileQB.GET_SELECTLIST_FILE, new { login.DatabaseName }, ct)).ToList(); ``` --- ### MEDIUM-27 — No Logging or Audit Trail for File Operations Neither `UploadService`, `FileStorage`, nor `FileBLL` log file uploads, downloads, or deletions. File operations are security-sensitive events that should be audited: - Who uploaded what file, when, from which IP. - Who downloaded a file. - Who deleted a file. **Fix:** ```csharp _logger.LogInformation("File uploaded: {FileName} {FileId} by user {UserId} from {IP}", fileName, id, login.UserId, login.MachineIP); ``` Also publish to the Dapr event log for audit trail: ```csharp await _eventLog.PublishAsync(new EventLogDTO { EventText = "File Uploaded", UserId = login.UserId, Data = JsonSerializer.Serialize(new { FileName = fileName, FileId = id }), EventTypeId = EventTypes.CREATE }, login); ``` --- ### MEDIUM-28 — `AttachmentDTO.AttachmentTag` Typed as `object` **File:** `FrameworkDAL/DTO/Attachment/AttachmentDTO.cs` ```csharp public class AttachmentDTO { public int AttachmentId { get; set; } public object AttachmentTag { get; set; } // ← untyped // ... } ``` `object` as a DTO property is unsafe for JSON deserialization — `System.Text.Json` will deserialize it as `JsonElement`, which requires explicit casting. Callers using `Newtonsoft.Json` will get a `JObject`. This causes runtime type errors. **Fix:** Define a concrete type or use `JsonElement`: ```csharp public JsonElement? AttachmentTag { get; set; } // explicit, safe ``` --- ### LOW-29 — Alfresco / `AlfrescoAttachmentDTO` — No Apparent Active Usage **File:** `FrameworkDAL/DTO/Alfresco/AlfrescoAttachmentDTO.cs` The Alfresco ECM integration DTO exists with no corresponding BLL, DAL, or endpoint code. If Alfresco is not being used, these DTOs are dead code that should be removed to reduce maintenance burden. **Recommendation:** Confirm with the team if Alfresco integration is planned. If not, delete the DTO and `BlobImportDTO`. --- ### LOW-30 — `FilePathDTO` Wraps Single String with Java-Era Backing Field **File:** `FrameworkDAL/DTO/File/FilePathDTO.cs` ```csharp public class FilePathDTO { private string _filePath; public string FilePath { get { return _filePath; } set { _filePath = value; } } } ``` A DTO with a single string property and manual getter/setter. This entire class could be replaced with: ```csharp public record FilePathDTO(string FilePath); ``` --- ### LOW-31 — Unused `StackExchange.Redis` Import in FileQB.cs **File:** `FrameworkDAL/Query/File/FileQB.cs` ```csharp using StackExchange.Redis; // ← unused in a SQL query builder ``` A Redis namespace in a SQL query builder class indicates either a copy-paste error or a removed feature that wasn't fully cleaned up. **Fix:** Remove the unused using statement. --- ### LOW-32 — `FileStorage` Is Not Registered in DI Container **File:** `GB5Shared/FileUpload/FileStorage.cs` `FileStorage` is instantiated ad-hoc inside `UploadService` (either `new FileStorage()` or resolved manually), not registered in the DI container. This means: - It cannot have its dependencies injected (`IConfiguration` for the storage root, `ILogger`). - It cannot be mocked in tests. - Its lifecycle is unmanaged. **Fix:** Register in `Program.cs`: ```csharp builder.Services.AddSingleton(); ``` Define an `IFileStorage` interface for testability. --- ### LOW-33 — All 4 Attachment FastEndpoints Are Empty Stub Classes **Files:** `GetAttachment.cs`, `SaveAttachment.cs`, `DeleteAttachment.cs`, `GetSelectListAttachment.cs` These endpoint class bodies are completely empty — they don't even inherit from `BaseEndpoint`. As a result: - They are not recognized as FastEndpoints and not registered in the routing table. - All frontend requests to `/Attachment/*` return 404. - The empty classes waste compile-time with no warning. These should either be implemented or removed until implementation is ready. --- ### LOW-34 — `GET_SELECTLIST_FILE` Returns All Files Globally **File:** `FrameworkDAL/Query/File/FileQB.cs` ```csharp public const string GET_SELECTLIST_FILE = @" SELECT F.FILEID, F.FILENAME, F.STATUS FROMMFILE F WHERE F.STATUS = 1"; ``` Beyond the syntax error (CRITICAL-6), this query has no WHERE clause filtering by entity, context, or folder. Calling this endpoint would return every active file in the database — potentially tens of thousands of records — as a dropdown list. **Fix:** Add contextual filters (at minimum `DatabaseName`, and ideally entity type/ID for context-sensitive attachment lists). --- ### LOW-35 — No Cache Key or Response Caching on File Metadata Endpoints File metadata (file name, type, description) is stable reference data that changes rarely. Currently all file metadata queries go to the database on every request without caching. **Fix:** Add `GetCacheKey()` override in file metadata read endpoints: ```csharp protected override string? GetCacheKey(GetFileParameters req, LoginDTO login) => $"File:{login.ClientId}:{req.FileId}"; ``` Invalidate on file update or delete: ```csharp await _keyInvalidate.InvalidateAsync("File", login.ClientId, CacheKeyLevel.CLIENT_LEVEL); ``` --- ## Architectural Findings Summary ### Dual File Handling Paths (No Unified Strategy) ``` Client → UploadService (MVC Controller) ↓ FileStorage.SaveAsync() ↓ Stores physical file at \\172.16.200.39\...\{guid}_{filename} ↓ Returns GUID — NO DB RECORD CREATED Client → FileBLL.SaveFile() ↓ FileDAL.SaveFile() ↓ INSERT INTO MFILE — NO PHYSICAL FILE STORED ``` These two paths are completely disconnected. There is no code that calls both, meaning it is impossible to: - Look up a file by its MFILE ID and retrieve the physical bytes. - Clean up orphaned physical files when a DB record is deleted. - Know which tenant owns a physical file from the filesystem alone. ### Attachment Feature Status Matrix | Component | Status | Severity | |-----------|--------|----------| | AttachmentBLL | Empty stub | CRITICAL | | AttachmentDAL | Empty stub | CRITICAL | | AttachmentQB | Empty stub | CRITICAL | | IAttachmentBLL | Wrong: `internal class` | CRITICAL | | IAttachmentDAL | Wrong: `internal class` | CRITICAL | | GET /Attachment/GetAttachment | Empty stub, no route | CRITICAL | | POST /Attachment/SaveAttachment | Empty stub, no route | CRITICAL | | DELETE /Attachment/DeleteAttachment | Empty stub, no route | CRITICAL | | GET /Attachment/GetSelectListAttachment | Empty stub, no route | CRITICAL | | FileBLL | Implemented, with issues | HIGH | | FileDAL | Implemented, with issues | HIGH | | FileQB | Implemented, syntax error | HIGH | | UploadService (MVC) | Implemented, many issues | CRITICAL | | FileStorage | Implemented, many issues | CRITICAL | --- ## Priority Remediation Plan ### Immediate (before next release) 1. **CRITICAL-1**: Sanitize `fileName` in `FileStorage.SaveAsync` — strip path components, validate canonical path. 2. **CRITICAL-2**: Add `[Authorize]` + `LoginDTO` validation to all upload/download endpoints. 3. **CRITICAL-3**: Add file type allowlist and magic-byte validation to all upload handlers. 4. **CRITICAL-5**: Add file size limits at both the ASP.NET Core middleware level and per-endpoint. 5. **CRITICAL-6**: Add `DatabaseName` tenant filter to all file SQL queries. 6. **HIGH-14**: Fix `FROMMFILE` syntax error in `GET_SELECTLIST_FILE`. 7. **HIGH-18**: Add `$` prefix to fix the literal string interpolation bug in `FileBLL`. ### Short-term (next sprint) 8. **CRITICAL-4**: Move hardcoded UNC path to HashiCorp Vault configuration. 9. **CRITICAL-7**: Fix `IAttachmentBLL` and `IAttachmentDAL` from `class` to `interface`. 10. **CRITICAL-8**: Implement the Attachment CRUD layer (BLL → DAL → QB → FastEndpoints). 11. **HIGH-9**: Validate `fileId` as GUID before using in `Directory.GetFiles`. 12. **HIGH-11**: Replace `File.ReadAllBytes` with `FileStream` streaming in `GetPdf`. 13. **HIGH-13**: Migrate `UploadService` and `AribaFileReceive` from MVC to FastEndpoints. 14. **HIGH-15**: Design and implement unified file handling (physical + metadata atomically). 15. **HIGH-16, HIGH-17**: Fix `ValidationException` wrapping; move `IValidation` from DAL to BLL. ### Medium-term (next quarter) 16. **CRITICAL-8**: Unify the dual file handling paths into a single `AttachmentService`. 17. **MEDIUM-19**: Validate `FileExtension` from mobile clients against an allowlist. 18. **MEDIUM-20**: Integrate antivirus scanning (ClamAV or cloud AV). 19. **MEDIUM-21**: Lazy-initialize `FileStorage`; add a startup health check for the UNC share. 20. **MEDIUM-27**: Add structured logging and Dapr event log publishing for all file operations. 21. **HIGH-12**: Enforce base64 file size limits or migrate mobile upload to multipart. --- ## Conclusion The Attachment module represents one of the highest-risk areas in the GB5 codebase: - **All planned FastEndpoints are empty stubs** — the feature is not functional through the designed API. - **The fallback MVC Controller path has critical VAPT vulnerabilities** — path traversal, no auth, no type validation, no size limits, glob injection. - **SQL has a syntax error and no tenant filters** — data is inaccessible and cross-tenant leakage is possible. - **Physical files and DB metadata are completely disconnected** — creating a data integrity problem that makes reliable file retrieval impossible. The priority is to fix the critical VAPT issues immediately (auth, size limits, type validation, path traversal) to eliminate the attack vectors, then systematically implement the designed Attachment CRUD layer to replace the ad-hoc MVC Controller approach.