# CommonReport — Deep Code Analysis Report **Date:** 2026-03-02 **Scope:** CommonReport service — all layers (SL, BLL, DAL, QB, PdfRenderer, CSVExport, ReportCallingDTO) **Files Reviewed:** - `FrameworkSL/Endpoints/CommonReport/Report.cs` - `FrameworkBLL/CommonReport/CommonReportBLL.cs` / `ICommonReportBLL.cs` - `FrameworkDAL/CustomCode/CommonReport/CommonReportDAL.cs` / `ICommonReportDAL.cs` - `FrameworkDAL/Query/Report/ReportQB.cs` - `GB5Shared/Export/HybridReport/PdfRenderer.cs` - `GB5Shared/Export/CSVExport/CSVExport.cs` - `GB5Shared/DTO/Report/ReportCallingDTO.cs` --- ## Executive Summary CommonReport is the single export gateway for all report formats (Excel, CSV, PDF, JSON) across the entire ERP. It processes potentially hundreds of thousands of rows per request and drives a headless Chromium browser for PDF rendering. A total of **34 issues** were found. The most critical are a **Server-Side Request Forgery (SSRF)** vulnerability where the server calls any URL supplied by the client, **hardcoded Windows file paths** that overwrite the same file on every export causing race conditions and data corruption, and a **Chromium re-download** triggered on every PDF request. Three `AllowAnonymous` endpoints and leaked exception messages compound the security exposure. --- ## 1. VAPT / Security Issues ### CRIT-01 — Server-Side Request Forgery (SSRF) via Client-Supplied `ReportUri` **Severity:** Critical **Files:** `ReportCallingDTO.cs:48`, `CSVExport.cs:103`, `CommonReportBLL.cs:79,117,134` ```csharp // ReportCallingDTO — fully client-controlled public string ReportUri { get { return reporturi; } set { reporturi = value; } } // CSVExport.cs — called for CSV, Excel, JSON: using var response = await httpClient.PostAsync(reportCallingDTO.ReportUri, content, cancellationToken); ``` The `ReportUri` field in `ReportCallingDTO` is accepted from the POST body (unauthenticated, see CRIT-02) and forwarded directly to `HttpClient.PostAsync` with no validation, whitelist check, or hostname restriction. An attacker can supply any URL — including internal microservice URLs, cloud metadata endpoints, or database management interfaces: ```json { "ReportCallingDTO": { "ReportUri": "http://169.254.169.254/latest/meta-data/", ... } } ``` The server will faithfully call the target and return the response body (parsed as a JSON report). This allows: - **Cloud metadata exfiltration** (AWS/Azure/GCP IMDS credentials) - **Internal service enumeration and exploitation** (Dapr sidecar, Redis, Vault HTTP APIs) - **Internal port scanning** via response timing differences **Recommendation:** Maintain a server-side allowlist of valid report base URIs loaded from configuration. Reject any `ReportUri` not matching the allowlist before making the HTTP call. --- ### CRIT-02 — `AllowAnonymous` on the Report Export Endpoint **Severity:** Critical **File:** `Report.cs:26` ```csharp public override void Configure() { Post("/CommonReport/Report"); AllowAnonymous(); } ``` The single entry point for all report exports (Excel, CSV, PDF, JSON) is unauthenticated. Combined with CRIT-01 (SSRF), an unauthenticated attacker can direct the server to call arbitrary internal URLs. Even without SSRF, an unauthenticated caller can: - Export any report's data by supplying a valid `ReportViewId` - Trigger Chromium PDF rendering on the server (resource exhaustion) - Enumerate organisation/company metadata via standard-fields queries --- ### CRIT-03 — Hardcoded Windows File Paths Overwrite Same File on Concurrent Exports **Severity:** Critical **Files:** `CommonReportBLL.cs:82-83`, `CommonReportBLL.cs:121-122` ```csharp // Excel export: string path = @"C:\Users\GoodBooks\Documents\Reports\Reports.xlsx"; await System.IO.File.WriteAllBytesAsync(path, ms.ToArray(), cancellationToken); // CSV export: string path = @"C:\Users\GoodBooks\Documents\Reports\Reportscsv.csv"; await System.IO.File.WriteAllBytesAsync(path, ms.ToArray(), cancellationToken); ``` Three critical problems: 1. **Race condition / data corruption**: Every Excel export overwrites the identical file path. Two concurrent Excel exports will corrupt each other's output — User A downloads User B's report. 2. **Platform failure**: These paths are Windows-only. The service will throw `DirectoryNotFoundException` on any Linux/Docker deployment (which is the standard for .NET 9 microservices). 3. **Disk fill / DoS**: No limit on how many exports can accumulate; combined with PDF chunk files written to `OutputPath`, disk can fill rapidly. The `MemoryStream` result `ms.ToArray()` is already available in memory — the disk write is a side effect with no evident purpose beyond debugging. --- ### HIGH-01 — `LoginDTO` Serialised to Plain JSON in HTTP Header for Inter-Service Calls **Severity:** High **Files:** `CSVExport.cs:121-126` ```csharp private static void LoginHeader(HttpClient httpClient, LoginDTO? loginDTO) { var loginJson = JsonSerializer.Serialize(loginDTO); httpClient.DefaultRequestHeaders.Add("Login", loginJson); } ``` The complete `LoginDTO` (containing `UserId`, `RoleId`, `ClientId`, `DatabaseName`, connection info, `BaseUri`, etc.) is serialised to JSON and transmitted as a plain HTTP header to the report data source URI. If the report service is reachable via HTTP (not HTTPS), or if the internal network is untrusted, this header is plaintext-readable. It also bypasses the `LoginEncryption` that the framework mandates for `LoginDTO` transport. --- ### HIGH-02 — Raw `ex.Message` Leaked in Response **Severity:** High **Files:** `Report.cs:71`, `CommonReportBLL.cs:343` ```csharp // Report.cs: return await Response.CreateExceptionError(ex, CacheKeyLevel.USER_LEVEL, LoginDTO, ex.Message, 500); // CommonReportBLL.cs: throw new Exception($"PDF Export failed: {ex.Message}", ex); ``` Internal exception messages — file paths, SQL fragments, Chromium launch errors, template parse errors — are surfaced to the HTTP client. File-not-found messages expose full server directory paths; Chromium errors expose OS/environment details. --- ### HIGH-03 — ReportQB Queries Have No Tenant Filter **Severity:** High **Files:** `ReportQB.cs:39,69` ```sql -- GETREPORTFORMAT: no ClientId filter WHERE REPORTFORMATID = @ReportFormatId; -- GETREPORT: no ClientId filter WHERE REPORTID = @ReportId; ``` Any user can fetch any report format or report configuration across all tenants by supplying any integer ID. The `GETREPORTFORMAT` result includes `REPPATH` (file system path), `DATASOURCEFILENAME`, and `REPORTFILE` — exposing internal file system layout. --- ### HIGH-04 — `Timeout = 0` on Puppeteer Navigation (Infinite Hang Risk) **Severity:** High **File:** `CommonReportBLL.cs:412-413` ```csharp await page.SetContentAsync(html, new PuppeteerSharp.NavigationOptions { WaitUntil = new[] { WaitUntilNavigation.Networkidle0 }, Timeout = 0 // infinite timeout }); ``` `WaitUntil = Networkidle0` combined with `Timeout = 0` means if any network activity in the rendered HTML does not settle (e.g., an external CDN reference, a broken image tag, a script referencing an unreachable URL), the page will hang indefinitely. This blocks the semaphore-protected `GlobalBrowser` which is shared across all concurrent PDF requests, causing a full service freeze. --- ### MED-01 — `new HttpClient()` Directly Instantiated — Socket Exhaustion **Severity:** Medium **Files:** `CSVExport.cs:96` ```csharp using var httpClient = new HttpClient(); ``` `HttpClient` is created per-call. On high-throughput reporting, this exhausts OS socket handles (TIME_WAIT state). The project's anti-patterns table explicitly prohibits this: `"new HttpClient() — Socket exhaustion; use IHttpClientFactory"`. --- ### MED-02 — Chromium Launched with `--no-sandbox` **Severity:** Medium **Files:** `CommonReportBLL.cs:667`, `PdfRenderer.cs:29` ```csharp Args = new[] { "--no-sandbox", "--disable-setuid-sandbox" } // and in PdfRenderer: Args = new[] { "--no-sandbox", "--disable-setuid-sandbox" } ``` `--no-sandbox` disables Chromium's security sandbox. If any HTML injected into the PDF template contains malicious scripts (e.g., via XSS in a data field), the script executes in the same OS process with no sandbox containment. Combined with client-controlled template content, this is a potential Remote Code Execution path. --- ### MED-03 — Temp PDF Chunk Files Not Cleaned on Exception **Severity:** Medium **File:** `CommonReportBLL.cs:336-344` ```csharp catch (Exception ex) { throw new Exception($"PDF Export failed: {ex.Message}", ex); } ``` The `tempPdfPaths` list is built in the `try` block. If an exception occurs mid-stream (e.g., after 10 of 25 chunks are written), the already-written temp PDF files are never deleted — no `finally` block cleans them up. Over time these accumulate in `OutputPath` causing disk fill. --- ## 2. Performance Issues ### PERF-01 — Chromium Downloaded on Every PdfRenderer Call **Severity:** Critical **File:** `PdfRenderer.cs:17-20` ```csharp var browserFetcher = new BrowserFetcher(new BrowserFetcherOptions { Path = chromePath }); var revision = Chrome.DefaultBuildId; await browserFetcher.DownloadAsync(revision); // called every RenderToPdfBytesAsync invocation ``` `browserFetcher.DownloadAsync()` is called unconditionally on every `RenderToPdfBytesAsync` invocation. While it may be a no-op if the binary already exists, it still performs a network/filesystem check on every call. The `PdfRenderer` class also launches a **new Chromium instance** per call (not using `GlobalBrowser`), meaning both the `PdfRenderer` path and the BLL path each launch separate browser processes. For a 200K-row report this can result in 25+ browser instances launched serially. --- ### PERF-02 — `Task.Delay(500ms)` Hardcoded in Excel and CSV Hot Path **Severity:** High **Files:** `CommonReportBLL.cs:81`, `CommonReportBLL.cs:120` ```csharp await IExcelExport.Export(..., ms, "Report", cancellationToken); await Task.Delay(500, cancellationToken); // ← 500ms sleep after every export string path = @"C:\Users\GoodBooks\..."; ``` A hardcoded 500-millisecond `Task.Delay` is inserted after every Excel and CSV export. At 100 concurrent report requests this burns 50 thread-seconds per second doing nothing. There is no comment explaining its purpose — it appears to be a debug/timing workaround left in production code. --- ### PERF-03 — Handlebars Environment Created and Helpers Re-Registered Per Chunk **Severity:** Critical **File:** `CommonReportBLL.cs:486-637` ```csharp private async Task RenderTemplateFromFileAsync(string templatePath, object context) { var hb = Handlebars.Create(); // new environment every call hb.RegisterHelper("formatDate", ...); // re-register all helpers hb.RegisterHelper("formatCurrency",...); hb.RegisterHelper("formatAuto", ...); hb.RegisterHelper("add", ...); hb.RegisterHelper("multiply", ...); hb.RegisterHelper("eq", ...); hb.RegisterHelper("gt", ...); hb.RegisterHelper("json", ...); hb.RegisterHelper("renderSubRows", ...); // 9 helpers registered var template = hb.Compile(source); // full AST compile from file source return template(context); } ``` `RenderTemplateFromFileAsync` is called **once per chunk** (for body, header, and footer = 3× per chunk). For a 200K-row report at 8K rows/chunk = 25 chunks × 3 calls = **75 full Handlebars environment constructions, 675 helper registrations, and 75 template compilations** per report. This is the dominant CPU cost for PDF generation. The same three-call pattern exists in `TemplateEngine.cs` (see TemplateEngine analysis) — the fix is the same: cache compiled delegates keyed by template path. --- ### PERF-04 — Template File Read from Disk on Every Chunk Render **Severity:** High **File:** `CommonReportBLL.cs:484` ```csharp var source = await System.IO.File.ReadAllTextAsync(templatePath); ``` The template `.hbs` file is read from disk on **every single chunk render call**. For a 25-chunk report this is 75 synchronous I/O reads of the same three template files. Template files change infrequently — they should be read once and cached in memory. --- ### PERF-05 — `GC.Collect()` Explicitly Called in Chunk Loop **Severity:** Medium **File:** `CommonReportBLL.cs:296` ```csharp if (chunkRows.Count >= rowsPerChunk) { // ... render chunk ... chunkRows.Clear(); GC.Collect(); // ← manual GC after every chunk } ``` `GC.Collect()` is called after every chunk flush in the streaming loop. Explicit GC calls: - Disrupt the GC's generation-based scheduling, forcing full Gen2 collection - Cause stop-the-world pauses affecting all concurrent requests on the server - Rarely help in practice — `chunkRows.Clear()` already removes references; the GC will reclaim them at the appropriate time **Recommendation:** Remove `GC.Collect()`. If memory pressure is genuine, use `GC.Collect(0, GCCollectionMode.Optimized)` at most, or investigate object pooling for the chunk rows. --- ### PERF-06 — `firstOrDefault` in Inner Loop Over `reportViewFields` **Severity:** Medium **File:** `CommonReportBLL.cs:239` ```csharp await foreach (var record in IExcelExport.ReportURICalling(...)) { foreach (var header in pdfColumnHeaders) { var fieldMeta = reportViewFields.FirstOrDefault(f => f.ReportViewFieldsFieldTitle == header); // ^^^^ O(N) scan per column per row ``` `reportViewFields.FirstOrDefault(...)` is called for every column of every row. If a report has 30 columns and 200K rows = **6 million O(N) list scans**. `reportViewFields` should be pre-indexed as a `Dictionary` before the streaming loop (the `fieldMap` dictionary already exists but is not used for `fieldMeta` lookup). --- ### PERF-07 — `FillReportStandardDTO` Makes 3 Sequential Database Round-Trips **Severity:** Medium **File:** `CommonReportBLL.cs:710-758` ```csharp ReportFormatDTO ReportFormatDTO = await CommonReportDAL.GetReportFormat(ReportFormatId, LoginDTO); // ... ReportDTO = await CommonReportDAL.GetReport(ReportId, LoginDTO); // ... List = await CommonReportDAL.GetReportView(ReportViewId, LoginDTO); ``` Three independent database calls are made sequentially. All three are independent and can be parallelised with `Task.WhenAll`. For a report with all three IDs set, this wastes 2× the latency of the slowest query. --- ### PERF-08 — Temp Chunk Files Written and Then Fully Read Back for Merge **Severity:** Medium **File:** `CommonReportBLL.cs:327`, `MergePdfFilesAndCleanup` ```csharp byte[] finalPdf = MergePdfFilesAndCleanup(tempPdfPaths); // reads all chunk files back from disk ``` Each chunk is written to a temp file (disk write), then all chunks are read back for merging (disk read), producing `finalPdf` in memory, then written to disk again as the final PDF. For a 200K-row report this is 3 full disk I/O passes of the entire PDF data. An in-memory `PipeWriter`/`PipeReader` pipeline or streaming merge would eliminate the intermediate disk passes. --- ### PERF-09 — `subToParentMap` Built with Nested OrderBy Scan Per Sub-Field **Severity:** Low **File:** `CommonReportBLL.cs:184-188` ```csharp foreach (var sub in subFields) { var parent = mainFields .Where(m => m.ReportViewFieldsDisplaySlNo < sub.ReportViewFieldsDisplaySlNo) .OrderByDescending(m => m.ReportViewFieldsDisplaySlNo) .FirstOrDefault(); ``` For each sub-field, all main fields are filtered and sorted — O(M log M) per sub-field. If there are many sub-fields this becomes O(S × M log M). A simple single-pass scan through sorted `mainFields` would suffice. --- ## 3. Memory Issues ### MEM-01 — `GlobalBrowser` Is Never Disposed — Chromium Process Leaked **Severity:** Critical **File:** `CommonReportBLL.cs:646-678` ```csharp private static class GlobalBrowser { private static IBrowser? _browser; private static readonly SemaphoreSlim _sem = new(1, 1); public static async Task GetAsync() { if (_browser != null) return _browser; // ... launch browser ... _browser = await Puppeteer.LaunchAsync(...); // Never disposed } } ``` `GlobalBrowser` holds a static `IBrowser` that is: - **Never disposed** on application shutdown — the Chromium OS process is orphaned - **Not registered for `IHostApplicationLifetime.ApplicationStopping`** cleanup - **Not recoverable** — if Chromium crashes, `_browser` holds a dead reference and all subsequent PDF requests fail with no retry logic - Shared globally across DI scopes — the `BrowserFetcher` is re-created every `GetAsync` call despite the browser being reused **Recommendation:** Register `GlobalBrowser` as a proper `IHostedService` or `IAsyncDisposable` singleton, with startup/shutdown lifecycle hooks and crash recovery via `browser.Disconnected` event. --- ### MEM-02 — Entire Final PDF Held in Memory Before Return **Severity:** High **File:** `CommonReportBLL.cs:327,334` ```csharp byte[] finalPdf = MergePdfFilesAndCleanup(tempPdfPaths); // ... return finalPdf; // returns entire PDF as byte[] to SL ``` For a 200K-row report the final merged PDF can be tens to hundreds of MB. The entire byte array is held in memory simultaneously with: - The in-memory `PdfDocument` object during merge (PdfSharpCore keeps all pages in memory) - The serialised response wrapper from `CreateSuccessResponse` This can cause Gen2 heap bloat and OutOfMemoryException under concurrent load. **Recommendation:** Stream the PDF directly to the HTTP response using `IResult.File(stream)` or `SendStreamAsync`, rather than materialising the full byte array. --- ### MEM-03 — Duplicate `MergePdfParts` Method Is Dead Code **Severity:** Low **File:** `CommonReportBLL.cs:462-475` ```csharp private static byte[] MergePdfParts(List pdfParts) // never called { using var output = new PdfDocument(); ... } ``` `MergePdfParts(List)` is an unused overload alongside `MergePdfFilesAndCleanup(List)`. Dead code increases binary size and maintenance surface. --- ### MEM-04 — `ReportCallingDTO` Uses Old-Style Backing Fields Throughout **Severity:** Low **File:** `ReportCallingDTO.cs` (entire file) `ReportCallingDTO` defines ~18 private backing fields with corresponding explicit property accessors — identical to the DTO pattern flagged in the TemplateEngine analysis. This is Java-era style that adds symbol bloat. Modern C# auto-properties or `record` types reduce code by ~60% with identical runtime behaviour. --- ## 4. Best Practice Issues ### BP-01 — Wrong Namespace Imports in BLL **Severity:** Medium **File:** `CommonReportBLL.cs:4-6,18` ```csharp using DocumentFormat.OpenXml.Drawing.Diagrams; // OpenXml diagram shapes — unused using DocumentFormat.OpenXml.Spreadsheet; // OpenXml spreadsheet cells — unused using Microsoft.AspNetCore.Components; // Blazor server-side components — wrong layer using System.Management; // WMI (Windows only) — unused using Google.Protobuf; // gRPC serialisation — unused (also in Report.cs) ``` These imports pull in: - Blazor/ASP.NET Components in a BLL class — a clear architectural violation (BLL must not reference ASP.NET types) - WMI (`System.Management`) which is Windows-only and will fail on Linux deployments - gRPC protobuf in a reporting endpoint with no gRPC usage --- ### BP-02 — `CancellationToken` Parameter Shadows Its Own Type Name **Severity:** Low **File:** `Report.cs:34` ```csharp protected override async Task> ExecuteAsync( ReportStandardAsyncReq ReportReq, LoginDTO LoginDTO, CancellationToken CancellationToken) ``` The parameter is named `CancellationToken` (same as the type). While it compiles, this is confusing to read and violates standard C# naming conventions. It should be named `ct` or `cancellationToken`. --- ### BP-03 — `JsonReport` Does Not Await Async Enumeration — Returns IAsyncEnumerable Serialised as Object **Severity:** High **File:** `CommonReportBLL.cs:127-141` ```csharp public async Task JsonReport( ReportCallingDTO ReportCallingDTO, LoginDTO LoginDTO, CancellationToken cancellationToken) { var Result = IExcelExport.ReportURICalling(ReportCallingDTO, LoginDTO, cancellationToken); return Newtonsoft.Json.JsonConvert.SerializeObject(Result); } ``` `IExcelExport.ReportURICalling` returns `IAsyncEnumerable>`. Calling `JsonConvert.SerializeObject` on an `IAsyncEnumerable` **does not enumerate it** — it serialises the enumerator object itself (yielding metadata, not data). The JSON response for any JSON report will be `{}` or the type descriptor, not the actual report rows. --- ### BP-04 — `GetStandardFields` Takes `OuId` as `string` (Should Be `int`) **Severity:** Medium **Files:** `ICommonReportDAL.cs:18`, `CommonReportDAL.cs:105` ```csharp public async Task> GetStandardFields(string OuId, LoginDTO LoginDTO) { var parameters = new Dictionary { { "@ouid", OuId } }; ``` `OuId` maps to `MOrganizationUnit.OUID` which is an integer primary key. Accepting it as `string` bypasses type safety, risks type mismatch in Dapper, and opens injection concerns (though parameterised). The interface and all callers should use `int`. --- ### BP-05 — Dictionary-Based Parameters Instead of Anonymous Objects **Severity:** Low **File:** `CommonReportDAL.cs:49-53,69-72,89-92,109-112,129-132` ```csharp var parameters = new Dictionary { { "@ReportFormatId", ReportFormatId } }; ``` All five DAL methods use `Dictionary` for Dapper parameters. The project standard (and CLAUDE.md) specifies anonymous objects: `new { ReportFormatId }`. Dictionaries allocate a heap `Dictionary` object unnecessarily; anonymous objects are allocated on the stack and are more readable. --- ### BP-06 — `IsPrintLogo` Logic Is Inverted (0 = true) **Severity:** Medium **File:** `CommonReportBLL.cs:725-731` ```csharp if (ReportFormatDTO.IsPrintLogo == 0) { ReportStandardDTO.IsPrintLogo = true; // 0 means TRUE? } else { ReportStandardDTO.IsPrintLogo = false; } ``` `IsPrintLogo == 0` sets `IsPrintLogo = true`. This inverted logic (zero = enabled) is non-intuitive and likely a historic encoding artifact. It is a latent bug if any new code assumes standard boolean semantics. --- ### BP-07 — Temp Chunk File Names Can Collide Under Concurrent Load **Severity:** Medium **File:** `CommonReportBLL.cs:396` ```csharp string tempPath = Path.Combine(outputDir, $"tmp_part_{DateTime.UtcNow:yyyyMMddHHmmss}_{chunkIndex}.pdf"); ``` Two concurrent report exports processed at the same second will produce identical file names for the same `chunkIndex`. The second export's `WriteAllBytesAsync` will overwrite the first's chunk, corrupting both reports. `Guid.NewGuid()` or a request-scoped prefix should be used. --- ### BP-08 — `ReportStandardAsyncReq` Route Name Does Not Match Convention **Severity:** Low **File:** `Report.cs:29` ```csharp public record ReportStandardAsyncReq(...) ``` The endpoint request record is named `ReportStandardAsyncReq` but the endpoint class is `Report`. All other endpoints in the codebase use `{EndpointName}Parameters` (e.g., `GetMenuForUserModuleParameters`). Inconsistent naming makes auto-discovery and tooling harder. --- ### BP-09 — `GlobalBrowser._browser` Is Not Thread-Safe (Double-Check Lock Without `volatile`) **Severity:** Medium **File:** `CommonReportBLL.cs:648,651` ```csharp private static IBrowser? _browser; public static async Task GetAsync() { if (_browser != null) return _browser; // first check — no lock, not volatile await _sem.WaitAsync(); try { if (_browser == null) // second check — inside lock ``` The double-check lock pattern requires the shared field to be `volatile` to prevent CPU/compiler instruction reordering. Without `volatile`, the first `if (_browser != null)` check may read a stale cached value from a CPU register, causing incorrect behaviour on multi-core systems. `private static volatile IBrowser? _browser;` is required. --- ### BP-10 — `renderSubRows` Handlebars Helper Emits Raw HTML Without Full XSS Protection **Severity:** Medium **File:** `CommonReportBLL.cs:618-623` ```csharp string combined = string.Join("
", subFields.Select(sf => System.Net.WebUtility.HtmlEncode(sf.Value))); writer.WriteSafeString($@" {combined} "); ``` `combined` is HTML-encoded but is then embedded inside an interpolated string used with `WriteSafeString`. The `style` attribute value `{padding}px` is an integer so safe here, but the pattern of building raw HTML strings inside `WriteSafeString` bypasses Handlebars' auto-escaping. Any future change that introduces a string variable into this interpolation could introduce XSS. --- ## 5. Architecture Compliance Issues ### ARCH-01 — BLL Directly Reads Files from the File System **Severity:** Medium **Files:** `CommonReportBLL.cs:169-173,484` ```csharp // Logo loading: var bytes = await System.IO.File.ReadAllBytesAsync(logoPath, cancellationToken); // Template loading: var source = await System.IO.File.ReadAllTextAsync(templatePath); ``` Direct `System.IO.File` access in a BLL ties the business logic to the file system layout and makes unit testing impossible without actual files on disk. An `ITemplateProvider` or `IFileProvider` abstraction should be injected. --- ### ARCH-02 — BLL Injects `IConfiguration` Directly **Severity:** Medium **File:** `CommonReportBLL.cs:34,167` ```csharp private readonly IConfiguration _configuration; // ... var pdfConfig = _configuration.GetSection("PDFEXPORT"); var logoPath = pdfConfig.GetValue("LogoPath"); var rowsPerChunk = pdfConfig.GetValue("RowsPerChunk") ?? 8000; ``` BLL directly reads raw `IConfiguration` with untyped `GetValue`. This should be bound to a strongly-typed options class (`PdfExportOptions`) via `IOptions` to benefit from validation at startup and clean dependency injection. --- ### ARCH-03 — `IExcelExport` Is Injected Into Both BLL and DAL **Severity:** Medium **Files:** `CommonReportBLL.cs:32,37`, `CommonReportDAL.cs:34,37` `IExcelExport` (which includes `ReportURICalling`, the HTTP caller) is injected into both BLL and DAL. The HTTP call to the report data source is the "data access" operation and belongs exclusively in DAL. Having BLL also hold a reference to `IExcelExport` creates confusion about which layer owns the data fetch, and means the HTTP caller can be invoked from two different layers. --- ### ARCH-04 — Two Separate Chromium Launch Paths (`PdfRenderer` vs `GlobalBrowser`) **Severity:** Medium **Files:** `PdfRenderer.cs`, `CommonReportBLL.cs:646` `PdfRenderer.RenderToPdfBytesAsync` creates a fresh `IBrowser` per call. `CommonReportBLL.RenderHtmlToPdfBytesUsingSharedBrowserAsync` uses `GlobalBrowser.GetAsync()` which maintains one shared instance. These two paths coexist in the same codebase with no coordination, leading to: - Inconsistent PDF rendering behaviour depending on which path is taken - Double Chromium downloads (each fetcher has its own `Path`) - No governance over total browser instances --- ## 6. Summary Table | ID | Category | Severity | Issue | |----|----------|----------|-------| | CRIT-01 | Security/VAPT | **Critical** | SSRF — client supplies `ReportUri` which server calls directly | | CRIT-02 | Security/VAPT | **Critical** | `AllowAnonymous` on the report export entry point | | CRIT-03 | Security/VAPT | **Critical** | Hardcoded Windows path, file overwritten on every export → race condition + Linux failure | | HIGH-01 | Security/VAPT | High | `LoginDTO` serialised to plain JSON in HTTP header for inter-service calls | | HIGH-02 | Security/VAPT | High | Raw `ex.Message` exposed in HTTP response | | HIGH-03 | Security/VAPT | High | ReportQB queries have no tenant/ClientId filter | | HIGH-04 | Security/VAPT | High | Puppeteer `Timeout=0` — infinite hang risk blocks all PDF requests | | MED-01 | Security | Medium | `new HttpClient()` directly instantiated — socket exhaustion | | MED-02 | Security | Medium | Chromium launched with `--no-sandbox` — XSS → RCE escalation risk | | MED-03 | Security | Medium | Temp PDF chunk files not cleaned on exception — disk fill | | PERF-01 | Performance | **Critical** | Chromium downloaded on every `PdfRenderer` call | | PERF-02 | Performance | High | `Task.Delay(500ms)` hardcoded in Excel and CSV export hot path | | PERF-03 | Performance | **Critical** | Handlebars env + 9 helpers + template re-compiled per chunk (75× per large report) | | PERF-04 | Performance | High | Template file read from disk on every chunk render (75 reads for 25-chunk report) | | PERF-05 | Performance | Medium | `GC.Collect()` called after every chunk — disrupts GC, causes stop-the-world pauses | | PERF-06 | Performance | Medium | `FirstOrDefault` O(N) scan per column per row in streaming loop (6M+ scans) | | PERF-07 | Performance | Medium | 3 sequential DB round-trips in `FillReportStandardDTO` — should be parallelised | | PERF-08 | Performance | Medium | Chunk files written → read back → merged → written again (3 full disk passes) | | PERF-09 | Performance | Low | Nested `OrderByDescending` scan to build `subToParentMap` | | MEM-01 | Memory | **Critical** | `GlobalBrowser` never disposed — Chromium process orphaned, no crash recovery | | MEM-02 | Memory | High | Full merged PDF byte array held in memory — OOM risk for large reports | | MEM-03 | Memory | Low | `MergePdfParts(List)` is dead code — unused overload | | MEM-04 | Memory | Low | `ReportCallingDTO` uses old-style backing fields (18 fields) | | BP-01 | Best Practice | Medium | Wrong namespace imports in BLL (Blazor, WMI, gRPC) | | BP-02 | Best Practice | Low | Parameter named same as its type (`CancellationToken CancellationToken`) | | BP-03 | Best Practice | High | `JsonReport` serialises `IAsyncEnumerable` as object — returns no data | | BP-04 | Best Practice | Medium | `GetStandardFields` takes `OuId` as `string` not `int` | | BP-05 | Best Practice | Low | Dictionary-based Dapper parameters instead of anonymous objects | | BP-06 | Best Practice | Medium | `IsPrintLogo == 0` sets `IsPrintLogo = true` — inverted logic | | BP-07 | Best Practice | Medium | Temp chunk file names collide under concurrent load (second-resolution timestamp) | | BP-08 | Best Practice | Low | Request record named `ReportStandardAsyncReq` not `ReportParameters` | | BP-09 | Best Practice | Medium | `GlobalBrowser._browser` not `volatile` — double-check lock unsafe on multi-core | | BP-10 | Best Practice | Medium | `renderSubRows` helper builds raw HTML with `WriteSafeString` — XSS risk in future edits | | ARCH-01 | Architecture | Medium | BLL reads files directly from file system — untestable, filesystem-coupled | | ARCH-02 | Architecture | Medium | BLL uses raw `IConfiguration` instead of `IOptions` | | ARCH-03 | Architecture | Medium | `IExcelExport` injected into both BLL and DAL — layer ownership ambiguous | | ARCH-04 | Architecture | Medium | Two separate Chromium launch paths with no coordination | --- ## 7. Recommended Fix Priority ### Immediate (this sprint — production risk) 1. **CRIT-01** — Add server-side URI allowlist; reject any `ReportUri` not in the approved list. 2. **CRIT-02** — Add authentication to `/CommonReport/Report`. 3. **CRIT-03** — Remove hardcoded file paths; use `Path.GetTempFileName()` with unique names per request, or return bytes directly without disk write. 4. **BP-03** — Fix `JsonReport`: materialise the `IAsyncEnumerable` with `ToListAsync` before serialising. 5. **MED-01** — Replace `new HttpClient()` in `CSVExport` with injected `IHttpClientFactory`. 6. **HIGH-04** — Set a bounded `Timeout` on Puppeteer navigation (e.g., 60 seconds). 7. **MEM-01** — Register `GlobalBrowser` as a proper `IAsyncDisposable` singleton with application lifetime hooks. ### Short-term (next sprint) 8. **HIGH-03** — Add `ClientId`/tenant filter to `GETREPORTFORMAT` and `GETREPORT` queries. 9. **HIGH-01** — Use the framework's `LoginEncryption` for inter-service `LoginDTO` transport. 10. **PERF-02** — Remove `Task.Delay(500)` from Excel and CSV export paths. 11. **PERF-03 + PERF-04** — Cache compiled Handlebars delegates and template source; read each template file once per process lifetime. 12. **BP-07** — Use `Guid.NewGuid()` in temp chunk file names to prevent collisions. 13. **MED-03** — Add a `finally` block to clean up temp chunk files on exception. ### Medium-term (tech debt) 14. **PERF-05** — Remove `GC.Collect()` from the chunk loop. 15. **PERF-06** — Pre-index `reportViewFields` into a `Dictionary` before the streaming loop. 16. **PERF-07** — Parallelise `FillReportStandardDTO` DB calls with `Task.WhenAll`. 17. **MEM-02** — Stream final PDF directly to HTTP response instead of materialising full byte array. 18. **ARCH-01 + ARCH-02** — Abstract file access behind `ITemplateProvider`; bind config to `IOptions`. 19. **BP-06** — Clarify and fix the inverted `IsPrintLogo` logic with an explanatory comment or enum. 20. **ARCH-04** — Consolidate `PdfRenderer` and `GlobalBrowser` into a single `IPdfRenderService` abstraction.