# TemplateEngine — Deep Code Analysis Report **Date:** 2026-03-02 **Scope:** All template-related layers across GB5Framework (TemplateEngine core, Template, MailTemplate, LoadTemplate, TemplateApproval, WhatsAppTemplate) **Files Reviewed:** - `GB5Shared/Export/HybridReport/TemplateEngine.cs` - `FrameworkSL/Endpoints/Template/` (GetTemplate, SaveTemplate, DeleteTemplate) - `FrameworkSL/Endpoints/MailTemplate/` (Get, Save, Delete, GetSelectList) - `FrameworkSL/Endpoints/LoadTemplate/` (Get, Save, Delete, GetSelectList) - `FrameworkSL/Endpoints/MessageHubService/TemplateApproval/UpdateTemplateApproval.cs` - `FrameworkBLL/Template/TemplateBLL.cs` - `FrameworkBLL/MailTemplate/MailTemplateBLL.cs` - `FrameworkBLL/LoadTemplate/LoadTemplateBLL.cs` - `FrameworkBLL/WhatsAppTemplate/WhatsAppTemplateBLL.cs` - `FrameworkBLL/MessageHubService/TemplateApproval/TemplateApprovalBLL.cs` - `FrameworkDAL/CustomCode/Template/TemplateDAL.cs` - `FrameworkDAL/CustomCode/MailTemplate/MailTemplateDAL.cs` - `FrameworkDAL/CustomCode/LoadTemplate/LoadTemplateDAL.cs` - `FrameworkDAL/CustomCode/MessageHubService/TemplateApproval/TemplateApprovalDAL.cs` - `FrameworkDAL/Query/Template/TemplateQB.cs` - `FrameworkDAL/Query/MailTemplate/MailTemplateQB.cs` - `FrameworkDAL/Query/LoadTemplate/LoadTemplateQB.cs` - `FrameworkDAL/Query/MessageHubService/TemplateApproval/TemplateApprovalQB.cs` - `FrameworkDAL/DTO/Menu/TemplateDTO.cs` - `FrameworkDAL/DTO/MailTemplate/MailTemplateDTO.cs` --- ## Executive Summary The template engine is a rendering and persistence service that spans six sub-modules. A total of **29 issues** were found. The most severe are in the core `TemplateEngine.cs` class: an unauthenticated path traversal vulnerability that can read arbitrary server files, a missing compiled-template cache that causes full recompilation on every render, and a Redis `ConnectionMultiplexer` created per instance causing socket exhaustion. Three production-breaking stubs also exist in `TemplateApprovalQB.cs`. All approval workflow SQL is incomplete placeholder code. --- ## 1. VAPT / Security Issues ### CRIT-01 — Path Traversal via Unsanitised `templateName` **Severity:** Critical **File:** `TemplateEngine.cs:28-33` ```csharp var path = Path.Combine("reports", "templates", templateName + ".hbs"); if (!File.Exists(path)) throw new FileNotFoundException($"Template file not found: {path}"); templateSource = await File.ReadAllTextAsync(path); ``` `templateName` is caller-supplied and is passed directly into `Path.Combine` without any sanitisation or whitelist check. On Linux/macOS, `Path.Combine` with a segment containing `../` traverses upward: ``` templateName = "../../appsettings.json%00" → reads application config templateName = "../../../etc/passwd" → reads OS files ``` `%00` null-byte truncation works on some runtimes. Even without it, `"../../../appsettings"` with the `.hbs` suffix appended still traverses and, if the target file does not exist, exposes the full resolved path in the `FileNotFoundException` message — leaking the server directory structure. **Recommendation:** Validate `templateName` against a strict whitelist (alphanumeric + dash/underscore only) before building the path, and verify the resolved path stays within the intended directory: ```csharp var safeName = Path.GetFileName(templateName); // strips directory components var basePath = Path.GetFullPath(Path.Combine("reports", "templates")); var fullPath = Path.GetFullPath(Path.Combine(basePath, safeName + ".hbs")); if (!fullPath.StartsWith(basePath, StringComparison.OrdinalIgnoreCase)) throw new UnauthorizedAccessException("Invalid template name."); ``` --- ### CRIT-02 — Template Cache Key Not Tenant-Scoped (Cross-Tenant Template Bleed) **Severity:** Critical **File:** `TemplateEngine.cs:21` ```csharp var cacheKey = $"tpl:{templateName}:v{version}"; ``` The Redis cache key is `tpl:{name}:v{version}` — there is no `tenantId` component. If Tenant A customises a template named `invoice` and it is written to Redis, Tenant B calling `RenderTemplateAsync("invoice", ..., tenantBId)` will receive Tenant A's cached template body. This is a cross-tenant data leakage risk in any multi-tenant deployment. **Recommendation:** Include `tenantId` in the template cache key: ```csharp var cacheKey = $"tpl:{tenantId}:{templateName}:v{version}"; ``` --- ### CRIT-03 — All Template, MailTemplate, LoadTemplate Queries Lack Tenant Filter **Severity:** Critical **Files:** `TemplateQB.cs`, `MailTemplateQB.cs`, `LoadTemplateQB.cs` Every `GET_*`, `UPDATE_*`, and `DELETE_*` query filters only by the entity's own primary key, with no `ClientId` or `DatabaseName` filter: ```sql -- TemplateQB.GET_TEMPLATE WHERE T.TEMPLATEID = @templateid -- MailTemplateQB.GET_MAILTEMPLATE WHERE MT.MAILTEMPLATEID = @mailtemplateid -- LoadTemplateQB.GET_LOADTEMPLATE WHERE LT.LOADTEMPLATEID = @loadtemplateid -- TemplateQB.DELETE_TEMPLATE WHERE TEMPLATEID = @templateid -- deletes any tenant's record ``` An authenticated user from Tenant A who knows or guesses the integer primary key of another tenant's template record can read, modify, or delete it. **Recommendation:** Add mandatory tenant filters to all queries: ```sql WHERE T.TEMPLATEID = @templateid AND T.CLIENTID = @clientid ``` --- ### HIGH-01 — AllowAnonymous on All Template Endpoints **Severity:** High **Files:** `GetTemplate.cs:21`, `SaveTemplate.cs:22`, `DeleteTemplate.cs:21`, all MailTemplate/LoadTemplate endpoints, `UpdateTemplateApproval.cs:24` Every template endpoint — including `SaveTemplate`, `DeleteTemplate`, and `UpdateTemplateApproval` — is decorated with `AllowAnonymous()`. Write and delete operations on template data, and approval workflow state changes, require no authentication whatsoever. Combined with CRIT-03 (no tenant filter), an unauthenticated attacker can delete any template by ID from any tenant. **Recommendation:** Apply role-based or policy-based authentication. At minimum: `Roles("User")` for read endpoints, `Roles("Admin")` for write/delete/approval endpoints. --- ### HIGH-02 — `version` Is a Client-Supplied Parameter in TemplateEngine **Severity:** High **File:** `TemplateEngine.cs:19` ```csharp public async Task RenderTemplateAsync(string templateName, object context, string tenantId, int version = 1) { var cacheKey = $"tpl:{templateName}:v{version}"; ``` `version` is an externally controllable parameter. A caller can request any arbitrary version number, causing cache misses and unbounded Redis key creation (one key per distinct version number requested). This can be used to: - Exhaust Redis memory via cache key spray (`v=1`, `v=2`, ..., `v=99999`) - Evict a legitimate cached template by storing a different version under a guessed key **Recommendation:** Version should be resolved server-side from the database record, not accepted as a client parameter. --- ### HIGH-03 — Raw `ex.Message` Exposed in Every Endpoint Response **Severity:** High **Files:** All SL endpoint files ```csharp catch (Exception ex) { return await Response.CreateExceptionError(ex, ..., ex.Message, 500); } ``` `ex.Message` in `TemplateApprovalDAL.cs` contains hardcoded internal strings like `"Error while saving Region Type"` — a copy-paste from another module — which leaks internal implementation context. In general, SQL errors, table names, and column names from Dapper exceptions will surface directly to the HTTP client. **Recommendation:** Log exception server-side with full detail; return only a generic error message to the client. --- ### MED-01 — TemplateApproval Approval Workflow Is Fully Broken (Stub SQL in Production) **Severity:** Medium (Functional — but also a security concern as approvals silently succeed) **File:** `TemplateApprovalQB.cs:11-13` ```csharp public const string ACCEPT_TEMPLATE_APPROVAL = @"SELECT"; public const string REJECT_TEMPLATE_APPROVAL = @"SELECT"; public const string FORWARD_TEMPLATE = @"SELECT"; ``` All three SQL constants are bare `SELECT` — incomplete placeholder statements. When `AcceptTemplateApproval`, `RejectTemplateApproval`, or `ForwardTemplate` are called, the database executes a syntax-error SQL statement. The DAL catches the exception and re-throws, so the BLL will throw, the endpoint returns a 500, but the approval state in the database is **never actually changed**. The endpoint returns HTTP 500 for every approval action in production. No template is ever genuinely approved or rejected. --- ### MED-02 — CancellationToken Never Propagated **Severity:** Medium **Files:** All BLL and DAL interface methods None of the BLL or DAL interfaces (`ITemplateBLL`, `IMailTemplateBLL`, `ILoadTemplateBLL`, `ITemplateApprovalBLL`, `ITemplateDAL`, `IMailTemplateDAL`, `ILoadTemplateDAL`) include `CancellationToken` parameters. The SL endpoints receive `ct` but it is discarded at the BLL boundary. Long-running DB calls cannot be cancelled on client disconnect. --- ### MED-03 — GetSelectListMailTemplate Has No Tenant Filter **Severity:** Medium **File:** `MailTemplateQB.cs:105-122` ```sql FROM MMAILTEMPLATE MT WHERE MT.STATUS = 1 ``` The `GET_SELECTLIST_MAILTEMPLATE` query returns all active mail templates from all tenants. With `AllowAnonymous` also active, any unauthenticated caller can enumerate the full mail template catalogue across all clients. --- ## 2. Performance Issues ### PERF-01 — Handlebars Template Recompiled on Every Render (Critical Hotspot) **Severity:** Critical **File:** `TemplateEngine.cs:36` ```csharp var templateSource = await _cache.StringGetAsync(cacheKey); // cached source string // ... var template = _hb.Compile(templateSource); // recompiled EVERY call — not cached ``` The template *source text* is cached in Redis, but the compiled Handlebars delegate (`HandlebarsTemplate`) is **recompiled from source on every single `RenderTemplateAsync` call**. Handlebars compilation parses the template AST and generates a delegate — this is a CPU-intensive operation, roughly equivalent to a small JIT compilation. For a high-throughput report/notification service this becomes the dominant cost. **Recommendation:** Cache the compiled delegate in a `ConcurrentDictionary>` keyed by `cacheKey`. Invalidate when the Redis source changes (version increment or expiry). ```csharp private readonly ConcurrentDictionary> _compiledCache = new(); // In RenderTemplateAsync: var compiled = _compiledCache.GetOrAdd(cacheKey, _ => _hb.Compile(templateSource!)); return compiled(finalContext); ``` --- ### PERF-02 — `ConnectionMultiplexer` Created Per Instance — Socket Exhaustion **Severity:** Critical **File:** `TemplateEngine.cs:11-16` ```csharp public TemplateEngine(string redisConnection) { _hb = Handlebars.Create(); RegisterHelpers(); var mux = ConnectionMultiplexer.Connect(redisConnection); // blocking + new multiplexer _cache = mux.GetDatabase(); } ``` `ConnectionMultiplexer.Connect()` is: 1. **Synchronous blocking I/O** called in a constructor — blocks the calling thread while negotiating the TCP/TLS connection to Redis. 2. Creates a **new multiplexer** (and thus new socket pool) per `TemplateEngine` instance. If `TemplateEngine` is registered as `Scoped` in DI (new instance per request), every request opens a new socket to Redis. At 100 RPS this exhausts OS ephemeral ports within minutes. Even with `Singleton`, the blocking constructor is a deadlock risk in async startup contexts. **Recommendation:** - Inject `IConnectionMultiplexer` (or `IDatabase`) from DI — let the application's shared `ConnectionMultiplexer.ConnectAsync()` manage the single Redis connection pool. - Register `TemplateEngine` as `Singleton`. - Expose a factory or `InitializeAsync()` for the Handlebars setup. --- ### PERF-03 — Redis Round-Trip for Tenant Metadata on Every Render **Severity:** Medium **File:** `TemplateEngine.cs:39-45` ```csharp var tenantJson = await _cache.StringGetAsync($"tenant:meta:{tenantId}"); object? tenantObj = null; if (!tenantJson.IsNullOrEmpty) tenantObj = Newtonsoft.Json.JsonConvert.DeserializeObject(tenantJson!); ``` Every template render makes a separate Redis call to fetch tenant metadata. For high-frequency rendering (e.g., bulk notification dispatch), this doubles the Redis round-trips per render. Tenant metadata changes infrequently and should be held in a local `IMemoryCache` with a sliding expiry rather than fetching from Redis each time. --- ### PERF-04 — `sum` Helper Walks Dictionary on Every Element **Severity:** Low **File:** `TemplateEngine.cs:74-90` ```csharp var dict = it as IDictionary; if (dict != null && dict.ContainsKey(fld!)) total += Convert.ToDecimal(dict[fld!]); ``` `ContainsKey` followed immediately by `[]` is a double lookup. Prefer `TryGetValue` for a single lookup. Minor, but called per element in report line items. --- ## 3. Memory Issues ### MEM-01 — `finally { Json = null; }` Pattern Is Meaningless and Misleading **Severity:** Low **Files:** `TemplateDAL.cs:44-47`, `MailTemplateDAL.cs:42-45`, `LoadTemplateDAL.cs:42-45` ```csharp string Json = ""; try { // ... use Json return result; } catch (Exception) { throw; } finally { Json = null; // ← has no effect } ``` Setting a local variable to `null` in a `finally` block does nothing — the variable is already going out of scope when the method returns. The GC does not need this hint. This pattern is repeated across all three DAL files and gives a false impression of resource management. --- ### MEM-02 — Each `TemplateEngine` Instance Holds a Full Handlebars Environment **Severity:** Medium **File:** `TemplateEngine.cs:8,13-14` ```csharp private readonly IHandlebars _hb; // ... _hb = Handlebars.Create(); RegisterHelpers(); ``` `Handlebars.Create()` creates an isolated environment with its own helper registry and partial registry. If `TemplateEngine` is registered as `Scoped`, a new `IHandlebars` environment (including all registered helpers) is allocated per request. This is wasted allocation given helpers are static. The global `Handlebars` singleton with thread-safe helper registration should be preferred, or the instance should be `Singleton`. --- ### MEM-03 — DTO Classes Use Old-Style Private Backing Fields Throughout **Severity:** Low **Files:** `TemplateDTO.cs`, `MailTemplateDTO.cs` Both DTOs declare private backing fields for every property (Java-era pattern) instead of auto-properties: ```csharp private int templateid; public int TemplateId { get { return templateid; } set { templateid = value; } } ``` This doubles the symbol count per property (one field + one property), increases binary size, and reduces readability. Modern C# auto-properties compile to identical IL. `TemplateDTO` has 15 such pairs; `MailTemplateDTO` has 18. Should be `record` or auto-property class. --- ## 4. Best Practice Issues ### BP-01 — `ValidationException` Wrapped and Downgraded to `Exception` **Severity:** Medium **Files:** `TemplateBLL.cs:73-75`, `MailTemplateBLL.cs:73-75`, `LoadTemplateBLL.cs:73-75` ```csharp catch (ValidationException vex) { throw new Exception(vex.Message); // type information destroyed } ``` Catching a typed `ValidationException` and rethrowing as a base `Exception` destroys the type information. Any upstream handler that catches `ValidationException` specifically will miss this error, and the status code mapping in the framework may classify it as a server error (500) rather than a validation error (400). **Recommendation:** Either rethrow the original `ValidationException` with bare `throw`, or throw a domain-specific `TemplateValidationException : ValidationException` so type-based handling works correctly. --- ### BP-02 — `MailTemplateBLL`: Dead `count` Variable Calculation **Severity:** Low **File:** `MailTemplateBLL.cs:55-56` ```csharp int initialId = MailTemplateDTO.MailTemplateId; int count = 1 + initialId; // dead: always computed but never used // ... if (initialId == 0) { AutoNumberDTO AutoNumberDTO = await _AutoNumber.GetNumberAsync(1, "MAILTEMPLATE", LoginDTO); // ^^^^ hardcoded 1, not count ``` `count` is calculated as `1 + initialId` but `GetNumberAsync` is called with hardcoded `1`. The `count` variable is never referenced. This is dead code that adds confusion. The same pattern appears in `LoadTemplateBLL.cs:56` — but there it is actually passed to `GetNumberAsync(count, ...)` making it actively wrong: when creating a new `LoadTemplate` record (`InitialId == 0`), `count = 1 + 0 = 1` which is correct, but the variable name and calculation suggest something else was intended. --- ### BP-03 — `TemplateApprovalBLL` Injects `AutoNumber` But Never Uses It **Severity:** Low **File:** `TemplateApprovalBLL.cs:18` ```csharp public TemplateApprovalBLL(ITemplateApprovalDAL ITemplateApprovalDAL, AutoNumber autoNumber) { _TemplateApprovalDAL = ITemplateApprovalDAL; // autoNumber is accepted but never stored or used } ``` `AutoNumber` is constructor-injected but not stored in a field. It is an unnecessary dependency that forces DI to resolve and inject an unused service on every request. --- ### BP-04 — `TemplateApprovalDAL` Error Messages Are Copy-Pasted from Wrong Module **Severity:** Low **File:** `TemplateApprovalDAL.cs:31,44,57` ```csharp throw new Exception("Error while saving Region Type", ex); // in all 3 methods ``` All three exception messages say `"Error while saving Region Type"` — clearly copied from a `RegionTypeDAL`. This makes production log triage impossible: "Region Type" errors in a template approval context are deeply confusing. --- ### BP-05 — `WhatsAppTemplateBLL` Is an Empty Internal Stub **Severity:** Medium **File:** `FrameworkBLL/WhatsAppTemplate/WhatsAppTemplateBLL.cs` ```csharp internal class WhatsAppTemplateBLL { } ``` The class is `internal` (will not be resolved by the DI scanner looking for `public` classes), implements no interface, and has no members. Combined with the separate `IWhatsAppTemplateBLL` interface that exists, this means WhatsApp template functionality is wired at the interface level but completely unimplemented. Any call to this feature at runtime silently fails due to unresolved DI. --- ### BP-06 — Bare `catch (Exception) { throw; }` on Trivial Methods **Severity:** Low **Files:** `TemplateBLL.cs:37-40`, `MailTemplateBLL.cs:36-40`, `LoadTemplateBLL.cs:34-40`, and others ```csharp public async Task GetTemplate(int TemplateId, LoginDTO LoginDTO) { try { return await _TemplateDAL.GetTemplate(TemplateId, LoginDTO); } catch (Exception) { throw; } } ``` Try/catch with no handler and bare `throw` in single-delegation methods adds stack-unwinding overhead with zero diagnostic or recovery value. These should be direct one-liners without a try block. --- ### BP-07 — `MailTemplateQB` Is Not a `static` Class **Severity:** Low **File:** `MailTemplateQB.cs:9`, `LoadTemplateQB.cs:9` ```csharp public class MailTemplateQB { ... } // should be static public class LoadTemplateQB { ... } // should be static ``` `TemplateQB` and `TemplateApprovalQB` are correctly declared `static`. `MailTemplateQB` and `LoadTemplateQB` are not. Non-static query builder classes can be instantiated unnecessarily and are inconsistent with the rest of the codebase. --- ### BP-08 — `GetTemplate` Cache Uses `USER_LEVEL` but Returns the Same Record for All Users **Severity:** Medium **File:** `GetTemplate.cs:33,46` ```csharp // Cache key generation: KeyGenerator.KeyGeneration(Parameters.TemplateId!, EntityConstant.OBJECTTEMPLATE, CacheKeyLevel.USER_LEVEL, LoginDTO); // Response: return await Response.CreateSuccessResponse(Result, CacheKeyLevel.USER_LEVEL, LoginDTO); ``` A template record (code, name, remarks, status) is the same for all users — it is a configuration record, not user-specific data. Using `USER_LEVEL` caching means one cache entry per user per template ID. If 1,000 users request `TemplateId=5`, there are 1,000 separate cache entries with identical content. `ROLE_LEVEL` or `CLIENT_LEVEL` is appropriate. --- ### BP-09 — `ITemplateEngine` Interface Does Not Exist **Severity:** Medium **File:** `TemplateEngine.cs` `TemplateEngine` is a concrete class with no interface abstraction. This means: - It cannot be mocked in tests - It cannot be swapped for a different rendering backend - It cannot be registered with the DI container as an interface type **Recommendation:** Extract `ITemplateEngine` with `RenderTemplateAsync` signature and register the concrete class against the interface. --- ### BP-10 — `GetSelectListMailTemplate` `IsCount=true` Branch Returns Hardcoded Zero **Severity:** Medium **File:** `MailTemplateDAL.cs:53-64` ```csharp if (IsCount == false) { // real query } else { int Count = 0; Json = Count.ToString(); // always returns "0" — no actual COUNT query } ``` When `IsCount == true`, no database call is made — the count is hardcoded to `0`. Any paging component that requests the total count before rendering will always receive `0`, breaking pagination. --- ## 5. Architecture Compliance Issues ### ARCH-01 — `TemplateEngine` Violates DI Lifetime Rules (Owns Its Own Redis Connection) **Severity:** High **File:** `TemplateEngine.cs:11-16` The `TemplateEngine` class creates and owns a `ConnectionMultiplexer` internally. Per project rules and StackExchange.Redis best practices, `ConnectionMultiplexer` must be a `Singleton` managed by the DI container. Internal ownership: - Prevents connection pool sharing with other services - Bypasses centralised connection configuration (Vault secrets) - Makes testing impossible without a live Redis instance --- ### ARCH-02 — `IValidation` Injected Into DAL Layer **Severity:** Low **Files:** `TemplateDAL.cs:21`, `MailTemplateDAL.cs:19`, `LoadTemplateDAL.cs:20` Validation is a BLL responsibility per the architecture spec. DAL methods call `_Validation.HandleException(ex, ...)` to format error messages. This is not data-access logic and creates an unnecessary coupling between the DAL and the validation framework. --- ### ARCH-03 — `TemplateDTO` Lives in `FrameworkDAL/DTO/Menu/` Namespace **Severity:** Low **File:** `FrameworkDAL/DTO/Menu/TemplateDTO.cs` `TemplateDTO` (used by `TemplateBLL` and `TemplateDAL`) is located under the `Menu` DTO folder and namespace (`FrameworkDAL.DTO.Menu`). This is a wrong namespace — template DTOs should reside in `FrameworkDAL/DTO/Template/`. --- ## 6. Summary Table | ID | Category | Severity | Issue | |----|----------|----------|-------| | CRIT-01 | Security/VAPT | **Critical** | Path traversal via unsanitised `templateName` → arbitrary file read | | CRIT-02 | Security/VAPT | **Critical** | Template cache key not tenant-scoped → cross-tenant template bleed | | CRIT-03 | Security/VAPT | **Critical** | All GET/UPDATE/DELETE queries lack tenant filter → cross-tenant CRUD | | HIGH-01 | Security/VAPT | High | AllowAnonymous on all template endpoints including write/delete/approval | | HIGH-02 | Security/VAPT | High | `version` is client-supplied → Redis key spray / cache poisoning vector | | HIGH-03 | Security/VAPT | High | Raw `ex.Message` exposed in HTTP response body | | MED-01 | Functional/Security | Medium | All TemplateApproval SQL is stub `"SELECT"` → approvals always fail silently | | MED-02 | Security | Medium | CancellationToken never propagated through BLL/DAL | | MED-03 | Security/VAPT | Medium | GetSelectListMailTemplate has no tenant filter | | PERF-01 | Performance | **Critical** | Handlebars template recompiled on every render — no compiled-delegate cache | | PERF-02 | Performance | **Critical** | `ConnectionMultiplexer.Connect()` in constructor → blocking + socket exhaustion | | PERF-03 | Performance | Medium | Redis round-trip for tenant metadata on every render | | PERF-04 | Performance | Low | `sum` helper double-lookup via ContainsKey + `[]` | | MEM-01 | Memory | Low | `finally { Json = null; }` in 3 DAL files — no-op, misleading | | MEM-02 | Memory | Medium | New `IHandlebars` environment created per instance if Scoped | | MEM-03 | Memory | Low | DTOs use private backing fields instead of auto-properties (TemplateDTO, MailTemplateDTO) | | BP-01 | Best Practice | Medium | `ValidationException` wrapped into base `Exception` — type info lost | | BP-02 | Best Practice | Low | Dead `count` variable in MailTemplateBLL (computed but not used) | | BP-03 | Best Practice | Low | `AutoNumber` injected into TemplateApprovalBLL but never stored or used | | BP-04 | Best Practice | Low | All TemplateApprovalDAL errors say "Error while saving Region Type" (copy-paste) | | BP-05 | Best Practice | Medium | `WhatsAppTemplateBLL` is empty + `internal` — feature unimplemented, DI silently fails | | BP-06 | Best Practice | Low | Empty `catch (Exception) { throw; }` blocks across all BLL passthrough methods | | BP-07 | Best Practice | Low | `MailTemplateQB` and `LoadTemplateQB` are not `static` classes (inconsistent) | | BP-08 | Best Practice | Medium | Template cached at `USER_LEVEL` for data that is role/client-scoped — wasteful | | BP-09 | Best Practice | Medium | No `ITemplateEngine` interface — untestable, not mockable, not swappable | | BP-10 | Best Practice | Medium | `IsCount=true` branch always returns hardcoded `0` — pagination broken | | ARCH-01 | Architecture | High | TemplateEngine owns its own Redis connection — violates DI lifetime rules | | ARCH-02 | Architecture | Low | `IValidation` injected into DAL — validation is BLL responsibility | | ARCH-03 | Architecture | Low | `TemplateDTO` lives in `FrameworkDAL/DTO/Menu/` wrong namespace | --- ## 7. Recommended Fix Priority ### Immediate (this sprint — production risk) 1. **CRIT-01** — Sanitise `templateName` and enforce path containment in `TemplateEngine`. 2. **CRIT-02** — Add `tenantId` to template Redis cache key. 3. **CRIT-03** — Add `ClientId`/`DatabaseName` tenant filter to all template QB queries. 4. **MED-01** — Implement actual SQL for `ACCEPT_TEMPLATE_APPROVAL`, `REJECT_TEMPLATE_APPROVAL`, `FORWARD_TEMPLATE` — or mark the feature as disabled and return HTTP 501. 5. **PERF-02** — Remove `ConnectionMultiplexer` creation from constructor; inject `IDatabase` via DI. ### Short-term (next sprint) 6. **HIGH-01** — Add authentication to all template endpoints. 7. **HIGH-02** — Remove `version` as a client parameter; resolve from DB server-side. 8. **HIGH-03** — Stop passing `ex.Message` to HTTP response; log internally only. 9. **PERF-01** — Cache compiled Handlebars delegate in `ConcurrentDictionary` keyed by cache key. 10. **ARCH-01** — Register `TemplateEngine` as Singleton; inject shared `IConnectionMultiplexer`. 11. **BP-09** — Extract `ITemplateEngine` interface and register via DI. ### Medium-term (tech debt) 12. **BP-05** — Implement `WhatsAppTemplateBLL` properly or remove the stub. 13. **MED-02** — Add `CancellationToken` to all BLL/DAL interfaces. 14. **BP-01** — Rethrow `ValidationException` directly; do not wrap in base `Exception`. 15. **BP-10** — Implement real `COUNT` query in `GetSelectListMailTemplate`. 16. **PERF-03** — Cache tenant metadata in `IMemoryCache` instead of re-fetching from Redis per render. 17. **MEM-02** — Register `TemplateEngine` as `Singleton` to share single Handlebars environment. 18. **BP-08** — Change template `GetCacheKey` from `USER_LEVEL` to `CLIENT_LEVEL`. 19. **ARCH-03** — Move `TemplateDTO` to `FrameworkDAL/DTO/Template/` namespace. 20. **MEM-01**, **BP-06**, **BP-07** — Clean up dead `finally` null-assigns, empty try/catch, and non-static QB classes.