# AddOnField — Deep Code Analysis **Date:** 2026-03-03 **Scope:** All AddOnField and related dynamic-field code in GB5Framework and cross-module consumers **Modules analysed:** - `FrameworkBLL/AddOnField/` — AddOnFieldBLL, IAddOnFieldBLL - `FrameworkDAL/CustomCode/AddOnField/` — AddOnFieldDAL, IAddOnFieldDAL - `FrameworkDAL/Query/AddOnField/` — AddOnFieldQB - `FrameworkDAL/DTO/addonfield/` — AddOnFieldDTO, AddOnFieldPickListDTO, AddonFieldLoginTimeDTO - `FrameworkSL/Endpoints/AddOnField/` — GetAddOnField, SaveAddOnField, DeleteAddOnField, GetSelectListAddOnField - `CostingDAL/DTO/CostAddOnField/` — CostAddOnFieldsDTO, CostAddOnFieldsPicklistDTO - `MMDAL/DTO/Item/` — ItemWithDynamicAddonDTO, ItemWithDynamicAddonFieldDTO - `PayRollDAL/Query/Employee/EmployeeQB.cs` — dynamic `:addonfields` SQL - `PayRollDAL/CustomeCode/PayProcess/PayProcessDAL.cs` — runtime addon field query construction --- ## Executive Summary The AddOnField module is a **framework-level feature** that allows runtime addition of custom fields to ERP entities without schema changes. It is used across PayRoll, Costing, MM, and ICE Map modules. However, the framework AddOnField CRUD layer is **structurally incomplete**: only the `GetAddOnField` endpoint is functional; Save, Delete, and GetSelectList are all empty stubs. The one working endpoint has **no authentication**, **no tenant filter in SQL** (IDOR vulnerability), and **exposes internal error details**. Cross-module consumers (PayRoll) use a **runtime dynamic SQL construction** pattern (`:addonfields` placeholder) that is fragile and hard to audit for injection risk. Multiple DTOs use the outdated Java-era backing-field pattern. **Total issues found:** 32 - Critical: 5 - High: 9 - Medium: 10 - Low: 8 --- ## Issue Index | # | Severity | Category | Title | |---|----------|----------|-------| | 1 | CRITICAL | VAPT | No tenant filter in `GET_ADDONFIELD` SQL — IDOR | | 2 | CRITICAL | VAPT | `AllowAnonymous` on the only working endpoint | | 3 | CRITICAL | VAPT | `ex.Message` leaked in error response | | 4 | CRITICAL | Architecture | 3 of 4 SL endpoints are empty stubs — feature incomplete | | 5 | CRITICAL | VAPT | Dynamic SQL column injection risk in PayRoll `:addonfields` placeholder | | 6 | HIGH | Best Practice | No `CancellationToken` in any interface, BLL, or DAL method | | 7 | HIGH | Performance | Needless `JsonConvert.SerializeObject` in DAL — result never used | | 8 | HIGH | Memory | `finally { json = null; }` is a no-op — misleading cleanup | | 9 | HIGH | Best Practice | Empty `try/catch { throw; }` in BLL, DAL, and `GetCacheKey` | | 10 | HIGH | Best Practice | `AddOnFieldPickListDTO` and `CostAddOnFieldsPicklistDTO` are mutable `struct` | | 11 | HIGH | Caching | Wrong cache level: `USER_LEVEL` for entity-schema configuration data | | 12 | HIGH | Performance | `ArrayList` (non-generic) used for addon field names in PayProcessDAL | | 13 | HIGH | Best Practice | `null!` passed to `QueryAsync` in PayProcessDAL for `ALL_PAYPROCSS_FIELDS` | | 14 | HIGH | Architecture | `ItemWithDynamicAddonFieldDTO.FieldValue` typed as `string` — loses type fidelity | | 15 | MEDIUM | Best Practice | Java-era backing fields in `AddOnFieldDTO` (~250 lines) | | 16 | MEDIUM | Best Practice | Java-era backing fields in `CostAddOnFieldsDTO` (~200 lines) | | 17 | MEDIUM | Best Practice | Dead SQL query embedded in comments inside `AddonFieldLoginTimeDTO` | | 18 | MEDIUM | Architecture | `PayProcessNewDTO` has 3 hardcoded `AddonFieldValue` fields instead of using the dynamic system | | 19 | MEDIUM | Best Practice | Trailing semicolon in `GET_ADDONFIELD` SQL constant | | 20 | MEDIUM | Best Practice | `Newtonsoft.Json` used in DAL (should be `System.Text.Json`) | | 21 | MEDIUM | Architecture | BLL has no validation before calling DAL | | 22 | MEDIUM | Best Practice | No logging anywhere in the AddOnField BLL | | 23 | MEDIUM | Architecture | `AddonFieldLoginTimeDTO` appears to be dead code | | 24 | MEDIUM | Performance | Dynamic SQL in PayProcessDAL uses `.Replace()` with integer `.ToString()` instead of parameterized values | | 25 | LOW | Best Practice | DTO folder name is lowercase `addonfield` instead of PascalCase | | 26 | LOW | Best Practice | `AddOnFieldPickListDTO` is missing `Code` property that `CostAddOnFieldsPicklistDTO` has | | 27 | LOW | Architecture | `IceMapDetailsDTO.IceMapDetailsAddonFields` stored as plain `string` (field name only, no type reference) | | 28 | LOW | Best Practice | `EntityConstant.ADDONFIELD = -1399999982` — large negative magic number, non-intuitive | | 29 | LOW | Best Practice | No event log publishing for Save/Delete (when eventually implemented) | | 30 | LOW | Architecture | `CriteriaDAL` — all related methods are commented out (dead code) | | 31 | LOW | Best Practice | `AddonFieldLoginTimeDTO.EntityDefinedIn` holds an assembly-relative path with no validation | | 32 | LOW | Architecture | `PayProcessEditDTO.AddonFields` typed as `IList` — inconsistent with system design | --- ## Detailed Findings --- ### CRITICAL-1 — No Tenant Filter in `GET_ADDONFIELD` SQL — IDOR **File:** `FrameworkDAL/Query/AddOnField/AddOnFieldQB.cs`, lines 11–38 ```sql SELECT AF.ADDONFIELDID AS AddOnFieldId, AF.ENTITYID AS EntityId, AF.FIELDNAME AS AddOnFieldFieldName, ... FROM MADDONFIELDS AF WHERE AF.ADDONFIELDID = @addonfieldid; -- ← no DATABASENAME / ClientId filter ``` The query has no tenant filter. An authenticated user from tenant A can query the addon field definitions of any other tenant by supplying a known `ADDONFIELDID`. This is a direct IDOR (Insecure Direct Object Reference) vulnerability. Addon field definitions include `FIELDNAME`, `DATATYPE`, `DATASIZE`, `TYPEOFINFO`, `LOVTYPEID`, and the `CRITERIAATTRIBUTEID` mapping — information that reveals tenant-specific schema configuration. **Fix:** Add `DATABASENAME` filter to all queries: ```sql FROM MADDONFIELDS AF WHERE AF.ADDONFIELDID = @AddOnFieldId AND AF.DATABASENAME = @DatabaseName ``` Pass `loginDTO.DatabaseName` as a parameter in DAL: ```csharp var parameters = new { AddOnFieldId = addOnFieldId, loginDTO.DatabaseName }; ``` --- ### CRITICAL-2 — `AllowAnonymous` on the Only Working Endpoint **File:** `FrameworkSL/Endpoints/AddOnField/GetAddOnField.cs`, line 22 ```csharp public override void Configure() { Get("/AddOnField/GetAddOnField"); AllowAnonymous(); // ← unauthenticated access to field configuration } ``` Addon field definitions are tenant-specific configuration data. Allowing anonymous access means any external caller can enumerate the field schema for all tenants (especially combined with CRITICAL-1's missing tenant filter). **Fix:** Remove `AllowAnonymous()`. Authenticate via the JWT/OIDC middleware configured in `Program.cs`. The `Login` header is already received via `[FromHeader]`, so the `LoginDTO` is available — just remove the bypass: ```csharp public override void Configure() { Get("/AddOnField/GetAddOnField"); // AllowAnonymous removed — use Roles() or just remove to require auth } ``` --- ### CRITICAL-3 — `ex.Message` Leaked in Error Response **File:** `FrameworkSL/Endpoints/AddOnField/GetAddOnField.cs`, lines 49–52 ```csharp catch (Exception ex) { return await GB5Shared.ResponseStandard.Response.CreateExceptionError( ex, CacheKeyLevel.USER_LEVEL, loginDTO, ex.Message, 500); // ↑ leaks internal error text } ``` `ex.Message` is passed as the public-facing error message. SQL errors from Dapper (e.g., "Invalid column name 'DATABASENAME'") reveal schema internals. Connection errors reveal server names. Stack traces can be included in some exception messages. **Fix:** Return a generic error message to the client; log the full exception server-side: ```csharp catch (Exception ex) { _logger.LogError(ex, "GetAddOnField failed for AddOnFieldId {Id}", parameters.AddOnFieldId); return await GB5Shared.ResponseStandard.Response.CreateExceptionError( ex, CacheKeyLevel.USER_LEVEL, loginDTO, "An error occurred. Please contact support.", 500); } ``` --- ### CRITICAL-4 — 3 of 4 SL Endpoints Are Empty Stubs **Files:** | File | Content | |------|---------| | `SaveAddOnField.cs` | `public class SaveAddOnField { }` | | `DeleteAddOnField.cs` | `public class DeleteAddOnField { }` | | `GetSelectListAddOnField.cs` | `public class GetSelectListAddOnField { }` | None of these classes: - Inherit from `BaseEndpoint`. - Configure a route. - Have any method. As a result: - `/AddOnField/SaveAddOnField` returns 404. - `/AddOnField/DeleteAddOnField` returns 404. - `/AddOnField/GetSelectListAddOnField` returns 404. The AddOnField configuration UI cannot save new fields, delete existing ones, or populate field-selection dropdowns. **Impact:** The AddOnField CRUD UI is non-functional. Any admin attempting to configure custom fields will silently fail. The `IAddOnFieldBLL` and `IAddOnFieldDAL` interfaces also only declare `GetAddOnField`, so Save and Delete are missing at every layer (SL, BLL, DAL, QB). **Recommendation:** Implement or remove these stub classes. If scheduled for later, add a clear `// TODO:` and a thrown `NotImplementedException` so it fails loudly rather than silently 404ing. --- ### CRITICAL-5 — Dynamic SQL Column Injection Risk in PayRoll `:addonfields` Placeholder **File:** `PayRollDAL/Query/Employee/EmployeeQB.cs`, lines 1271–1284 ```csharp public const string GET_EMPLOYEE_ADDON = @" SELECT EMPLOYEEID AS EmployeeId :addonfields -- ← runtime column list injected here FROM MEMPLOYEEADDON WHERE EMPLOYEEID IN (SELECT value FROM STRING_SPLIT(@Ids, ','));"; ``` The `:addonfields` placeholder is replaced at runtime with a comma-separated list of column names sourced from the `ALL_PAYPROCSS_FIELDS` query (DB column names from the system catalogue). In `PayProcessDAL.cs`: ```csharp IList DBObjectFieldsDTOs = (await _queryExecutor.QueryAsync( LoginDTO, AllAddonQuery, null!)).ToList(); // Fields string is built by concatenating DBObjectFieldsName values Fields = Fields + DBObjectFieldsDTOs[i].DBObjectFieldsName + ","; // ... HqlTOSQL = HqlTOSQL.Replace(":addonfields", "," + Fields); ``` While field names come from a system catalogue query (lower trust than user input), risks remain: 1. **If the schema inspection query or `MADDONFIELDS` is compromised** (e.g., by a data injection earlier in the pipeline), a field name containing `; DROP TABLE` or `UNION SELECT` gets injected verbatim into the SQL. 2. **No allowlist validation** on the column names before they are interpolated into the SQL. 3. **No quoting/escaping** of column names — a column named `[col]; SELECT 1--` would be injected directly. **Fix:** Validate all column names against an alphanumeric + underscore pattern before string substitution: ```csharp private static readonly Regex SafeColumnName = new(@"^[A-Za-z_][A-Za-z0-9_]*$", RegexOptions.Compiled); foreach (var field in DBObjectFieldsDTOs) { if (!SafeColumnName.IsMatch(field.DBObjectFieldsName)) throw new InvalidOperationException($"Unsafe column name detected: {field.DBObjectFieldsName}"); AllAddonFields.Add(field.DBObjectFieldsName); } ``` Additionally, wrap column names in square brackets (SQL Server) or double quotes (PostgreSQL) in the generated SELECT list: ```csharp Fields = string.Join(",", DBObjectFieldsDTOs.Select(f => $"[{f.DBObjectFieldsName}]")); ``` --- ### HIGH-6 — No `CancellationToken` in Any Interface, BLL, or DAL Method **Files:** - `IAddOnFieldBLL.cs`: `Task> GetAddOnField(int addOnFieldId, LoginDTO loginDTO)` - `IAddOnFieldDAL.cs`: `Task> GetAddOnField(int addOnFieldId, LoginDTO loginDTO)` - `AddOnFieldBLL.cs`: Same signature - `AddOnFieldDAL.cs`: Same signature, and `_queryExecutor.QueryAsync` called without CT `CancellationToken` propagation is mandated by CLAUDE.md. Without it: - A client disconnection does not cancel in-flight DB queries. - The DB connection remains occupied until the query completes. - Under high concurrency, this causes connection pool exhaustion. **Fix:** Add `CancellationToken ct = default` to every interface and implementation: ```csharp // IAddOnFieldBLL.cs Task> GetAddOnField(int addOnFieldId, LoginDTO loginDTO, CancellationToken ct = default); // AddOnFieldDAL.cs IEnumerable result = await _queryExecutor.QueryAsync( loginDTO, sql, parameters, cancellationToken: ct); ``` --- ### HIGH-7 — Needless `JsonConvert.SerializeObject` in DAL — Result Never Used **File:** `FrameworkDAL/CustomCode/AddOnField/AddOnFieldDAL.cs`, lines 20–36 ```csharp public async Task> GetAddOnField(int addOnFieldId, LoginDTO loginDTO) { string json = ""; try { string sql = AddOnFieldQB.GET_ADDONFIELD; var parameters = new { addonfieldid = addOnFieldId }; IEnumerable result = await _queryExecutor.QueryAsync(loginDTO, sql, parameters); json = JsonConvert.SerializeObject(result); // ← serialised but never returned/used return result; } catch (Exception) { throw; } finally { json = null; // ← sets local to null, does nothing } } ``` The `JsonConvert.SerializeObject(result)` call: - Forces full materialisation of the `IEnumerable` into a JSON string. - Allocates a potentially large string on the heap. - That string is immediately assigned to `json`, which is then set to `null` in `finally` and goes out of scope. - The serialisation result is never returned, logged, or used in any way. This is pure CPU and memory waste on every call. It also requires `using Newtonsoft.Json` unnecessarily. **Fix:** Remove the `json` variable and serialisation call entirely: ```csharp public async Task> GetAddOnField( int addOnFieldId, LoginDTO loginDTO, CancellationToken ct = default) { return await _queryExecutor.QueryAsync( loginDTO, AddOnFieldQB.GET_ADDONFIELD, new { AddOnFieldId = addOnFieldId, loginDTO.DatabaseName }, cancellationToken: ct); } ``` --- ### HIGH-8 — `finally { json = null; }` Is a No-Op — Misleading Cleanup **File:** `FrameworkDAL/CustomCode/AddOnField/AddOnFieldDAL.cs`, line 35 ```csharp finally { json = null; // ← local variable goes out of scope at method end anyway } ``` Setting a local `string` variable to `null` in a `finally` block does not help GC. The variable goes out of scope at method exit regardless. This is cargo-cult code that misleads readers into thinking some cleanup is being performed. This same pattern appears in `FileDAL.cs` (noted in the Attachment analysis). It is a systemic pattern to be cleaned up project-wide. **Fix:** Remove the `finally` block entirely. If the intent was to log or release a resource, implement the actual logic. --- ### HIGH-9 — Empty `try/catch { throw; }` in BLL, DAL, and `GetCacheKey` Three separate locations contain structurally identical no-op exception handling: **File 1:** `AddOnFieldBLL.cs`, lines 19–26 ```csharp try { return await _addOnFieldDAL.GetAddOnField(addOnFieldId, loginDTO); } catch (Exception) { throw; // ← catches and immediately rethrows — no logging, no transformation } ``` **File 2:** `AddOnFieldDAL.cs` — same pattern. **File 3:** `GetAddOnField.cs`, `GetCacheKey` override, lines 32–39 ```csharp protected override string? GetCacheKey(GetAddOnFieldParameters parameters, LoginDTO loginDTO) { try { return KeyGenerator.KeyGeneration(...); } catch (Exception) { throw; } } ``` These blocks: - Add an unnecessary stack frame on every call. - Mislead code readers into expecting some exception handling logic. - Prevent the compiler from optimising tail calls. **Fix:** Remove all three `try/catch { throw; }` blocks. Let exceptions propagate naturally. If logging is desired, add it: ```csharp public async Task> GetAddOnField(int addOnFieldId, LoginDTO loginDTO, CancellationToken ct) => await _addOnFieldDAL.GetAddOnField(addOnFieldId, loginDTO, ct); ``` --- ### HIGH-10 — `AddOnFieldPickListDTO` and `CostAddOnFieldsPicklistDTO` Are Mutable `struct` **Files:** - `FrameworkDAL/DTO/addonfield/AddOnFieldPickListDTO.cs` — `public struct AddOnFieldPicklistDTO` - `CostingDAL/DTO/CostAddOnField/CostAddOnFieldPicklistDTO.cs` — `public struct CostAddOnFieldsPicklistDTO` Mutable structs (structs with settable properties) are a well-documented .NET anti-pattern: 1. **Copy semantics cause silent data loss**: `list[0].FieldName = "X"` modifies a copy, not the list element. 2. **Boxing overhead**: When stored in `ArrayList`, `IList`, or `object` references, structs are boxed, allocating heap objects anyway. 3. **Default initialisation**: `FieldName` defaults to `null` for `string` fields, potentially causing NREs. 4. **Dapper mapping**: Dapper maps to structs differently from classes; some projections may fail silently. **Fix:** Convert to `record` for clean value semantics, or to `class` for reference semantics: ```csharp // Option A — record (preferred for DTOs) public record AddOnFieldPicklistDTO(int Id, string FieldName); // Option B — class with auto-properties public class AddOnFieldPicklistDTO { public int Id { get; set; } public string FieldName { get; set; } = string.Empty; } ``` --- ### HIGH-11 — Wrong Cache Level: `USER_LEVEL` for Entity-Schema Configuration Data **File:** `FrameworkSL/Endpoints/AddOnField/GetAddOnField.cs`, lines 33–34 and 47 ```csharp return KeyGenerator.KeyGeneration( parameters.AddOnFieldId, EntityConstant.ADDONFIELD, CacheKeyLevel.USER_LEVEL, loginDTO); // ← USER_LEVEL // ... return await GB5Shared.ResponseStandard.Response.CreateSuccessResponse( result, CacheKeyLevel.USER_LEVEL, loginDTO); ``` Addon field definitions are **entity schema configuration** — they describe what custom fields exist for a given entity. This data: - Is the same for all users within a client/tenant. - Changes only when an administrator modifies the addon field setup. - Does NOT differ per user. `USER_LEVEL` caching means every user gets a separate cache entry for the same data, multiplying cache memory usage by the number of users. **Fix:** Use `CLIENT_LEVEL` (per-tenant) caching: ```csharp return KeyGenerator.KeyGeneration( parameters.AddOnFieldId, EntityConstant.ADDONFIELD, CacheKeyLevel.CLIENT_LEVEL, loginDTO); // ... return await GB5Shared.ResponseStandard.Response.CreateSuccessResponse( result, CacheKeyLevel.CLIENT_LEVEL, loginDTO); ``` Invalidate at `CLIENT_LEVEL` when Save or Delete is implemented. --- ### HIGH-12 — `ArrayList` (Non-Generic) Used for Addon Field Names in PayProcessDAL **File:** `PayRollDAL/CustomeCode/PayProcess/PayProcessDAL.cs`, lines 1163, 1176 ```csharp ArrayList AllAddonFields = new ArrayList(); // ← non-generic, pre-LINQ // ... AllAddonFields.Add(DBObjectFieldsDTOs[i].DBObjectFieldsName); // ... foreach (string field in AllAddonFields) // ← unboxing cast on each iteration ``` `ArrayList` is a .NET 1.x collection that stores `object`. Every element is: - Boxed when added (`string` → `object`). - Cast when retrieved (`object` → `string`). The cast `foreach (string field in AllAddonFields)` will throw `InvalidCastException` if any element is not a `string`. Modern C# should use `List`: ```csharp var allAddonFields = new List(); // ... allAddonFields.Add(DBObjectFieldsDTOs[i].DBObjectFieldsName); ``` This also aligns with the CLAUDE.md prohibition on legacy patterns. --- ### HIGH-13 — `null!` Passed to `QueryAsync` for `ALL_PAYPROCSS_FIELDS` **File:** `PayRollDAL/CustomeCode/PayProcess/PayProcessDAL.cs`, line 1173 ```csharp IList DBObjectFieldsDTOs = (await _queryExecutor.QueryAsync( LoginDTO, AllAddonQuery, null!)).ToList(); ``` `null!` is used to suppress a compiler null-safety warning. This passes an actual `null` as the query parameters argument to `IQueryExecutor.QueryAsync`. Dapper will attempt to enumerate parameters from `null`, which throws a `NullReferenceException` at runtime unless `IQueryExecutor` has special null-parameter handling. **Fix:** Pass an empty anonymous object if no parameters are needed: ```csharp IList DBObjectFieldsDTOs = (await _queryExecutor.QueryAsync( LoginDTO, AllAddonQuery, new { })).ToList(); ``` --- ### HIGH-14 — `ItemWithDynamicAddonFieldDTO.FieldValue` Typed as `string` — Loses Type Fidelity **File:** `MMDAL/DTO/Item/ItemWithDynamicAddonDTO.cs`, lines 43–56 ```csharp public class ItemWithDynamicAddonFieldDTO { private string fieldcode; private string fieldvalue; // ← all dynamic values stored as string public string FieldCode { get { return fieldcode; } set { fieldcode = value; } } public string FieldValue { get { return fieldvalue; } set { fieldvalue = value; } } } ``` Addon fields can have `DATATYPE` values of numeric, date, boolean, or LOV (list-of-values). Storing all values as `string` means: - Numeric values must be parsed on every use — error-prone. - Date values lose timezone and format information. - Boolean values become `"1"` or `"0"` — fragile. - Sorting and comparison require type-aware parsing. **Fix:** Use a typed discriminated union or `JsonElement?` for dynamic values: ```csharp public record ItemWithDynamicAddonFieldDTO( string FieldCode, string? TextValue, decimal? NumericValue, DateTime? DateValue, int? LovId, byte DataType // original DATATYPE from MADDONFIELDS ); ``` Alternatively, use `object?` with a documented type contract, or store raw `JsonElement` for API serialisation. --- ### MEDIUM-15 — Java-Era Backing Fields in `AddOnFieldDTO` (~250 Lines) **File:** `FrameworkDAL/DTO/addonfield/AddOnFieldDTO.cs` The DTO declares ~25 private fields with manual getter/setter pairs, resulting in ~250 lines for what should be a ~30-line record: ```csharp private int addonfieldid; public int AddOnFieldId { get { return addonfieldid; } set { addonfieldid = value; } } // ... repeated ~25 times ``` This is Java 1.4-era code ported to C#. Modern C# auto-properties are cleaner and equivalent: ```csharp public class AddOnFieldDTO { public int AddOnFieldId { get; set; } public int EntityId { get; set; } public string EntityCode { get; set; } = string.Empty; public string EntityName { get; set; } = string.Empty; public short AddOnFieldSlNo { get; set; } public string AddOnFieldFieldName { get; set; } = string.Empty; public byte AddOnFieldDataType { get; set; } public double AddOnFieldDataSize { get; set; } public byte AddOnFieldTypeOfInfo { get; set; } public int LovTypeId { get; set; } public string LovTypeCode { get; set; } = string.Empty; public string LovTypeName { get; set; } = string.Empty; public string AddOnFieldRemarks { get; set; } = string.Empty; public string AddOnFieldFieldDescription { get; set; } = string.Empty; public int CriteriaAttributeId { get; set; } = -1; public string CriteriaAttributeCode { get; set; } = string.Empty; public string CriteriaAttributeName { get; set; } = string.Empty; public int AddOnFieldCreatedById { get; set; } public DateTime AddOnFieldCreatedOn { get; set; } public int AddOnFieldModifiedById { get; set; } public DateTime AddOnFieldModifiedOn { get; set; } public short AddOnFieldSortOrder { get; set; } public byte AddOnFieldStatus { get; set; } public short AddOnFieldVersion { get; set; } public byte AddOnFieldSourceType { get; set; } public byte AddOnFieldIsMandatory { get; set; } = 1; public byte AddOnFieldIsMultiselect { get; set; } = 1; public byte OperationType { get; set; } } ``` The same pattern applies to `CostAddOnFieldsDTO` (MEDIUM-16). --- ### MEDIUM-16 — Java-Era Backing Fields in `CostAddOnFieldsDTO` (~200 Lines) **File:** `CostingDAL/DTO/CostAddOnField/CostAddOnFieldDTO.cs` Same anti-pattern as MEDIUM-15. The DTO has ~25 private fields with manual getters/setters, totalling ~200 lines. Convert to auto-properties. --- ### MEDIUM-17 — Dead SQL Query Embedded in Comments Inside `AddonFieldLoginTimeDTO` **File:** `FrameworkDAL/DTO/addonfield/AddonFieldLoginTimeDTO.cs`, lines 10–15 ```csharp public class AddonFieldLoginTimeDTO { //"SELECT b.ENTITYCODE as EntityCode," + "\r\n" + // " DataType as DataType," + "\r\n" + // " FieldName as FieldName," + "\r\n" + // " b.ENTITYDEFINEDIN as EntityDefinedIn," + "\r\n" + // " c.ASSEMBLYNAME as AssemblyName," + "\r\n" + // " d.DBOBJECTNAME as DbObjectnName" + "\r\n" + ``` A SQL query fragment is commented out directly inside a DTO class body. This violates the Query Builder pattern (SQL belongs in `*QB.cs` files). It reveals historical implementation details and creates noise. The fact that this SQL uses Java-era string concatenation (`"\r\n" +`) suggests it was ported from a legacy system. **Fix:** Remove the commented-out SQL entirely. If the query is needed, place it in `AddOnFieldQB.cs`. --- ### MEDIUM-18 — `PayProcessNewDTO` Has 3 Hardcoded `AddonFieldValue` Fields **File:** `PayRollDAL/DTO/PayProcess/PayProcessNewDTO.cs`, lines 730–754 ```csharp public decimal AddonFieldValue { get { return addonFieldValue; } ... } public decimal AddonFieldValue2 { get { return addonFieldValue2; } ... } public decimal AddonFieldValue1 { get { return addonFieldValue1; } ... } ``` Three hardcoded addon field value slots exist in the main PayProcess DTO. This defeats the purpose of the dynamic addon field system: - If a tenant has 4 addon fields, the 4th cannot be returned via this DTO. - The numbering (`Value`, `Value1`, `Value2`) is non-sequential and confusing. - These fields are not linked to field names or the `MADDONFIELDS` definitions. **Fix:** Remove the hardcoded fields. Use the `PayProcessEditDTO.AddonFields` list pattern (`IList`) consistently for all addon field values. --- ### MEDIUM-19 — Trailing Semicolon in `GET_ADDONFIELD` SQL Constant **File:** `FrameworkDAL/Query/AddOnField/AddOnFieldQB.cs`, line 37 ```sql WHERE AF.ADDONFIELDID = @addonfieldid; "; ``` The SQL has a trailing semicolon inside the constant string. While Dapper typically handles this gracefully, it is: - Non-standard (Dapper doesn't require a semicolon). - Inconsistent with queries in other QB files. - Potentially problematic if the query is used in a batch or multi-statement context. **Fix:** Remove the trailing semicolon from the SQL constant. --- ### MEDIUM-20 — `Newtonsoft.Json` Used in DAL (Should Be `System.Text.Json`) **File:** `FrameworkDAL/CustomCode/AddOnField/AddOnFieldDAL.cs`, line 5 ```csharp using Newtonsoft.Json; ``` CLAUDE.md states: *"Serialization: System.Text.Json (primary) + Newtonsoft.Json (legacy)"*. New DAL code should use `System.Text.Json`. Since the serialisation call is being removed entirely (HIGH-7), the `using` statement becomes dead. **Fix:** Remove the `using Newtonsoft.Json;` statement from `AddOnFieldDAL.cs` once the serialisation call is removed. --- ### MEDIUM-21 — BLL Has No Validation Before Calling DAL **File:** `FrameworkBLL/AddOnField/AddOnFieldBLL.cs` ```csharp public async Task> GetAddOnField(int addOnFieldId, LoginDTO loginDTO) { return await _addOnFieldDAL.GetAddOnField(addOnFieldId, loginDTO); } ``` There is no validation of `addOnFieldId` (e.g., checking it is > 0) or `loginDTO` (checking `DatabaseName` is not empty). Invalid inputs go directly to the database. If `addOnFieldId = 0` is passed, the SQL `WHERE AF.ADDONFIELDID = 0` returns empty results — silently returning an empty list rather than a meaningful error. **Fix:** ```csharp if (addOnFieldId <= 0) throw new ValidationException("AddOnFieldId must be a positive integer."); if (string.IsNullOrWhiteSpace(loginDTO?.DatabaseName)) throw new ValidationException("LoginDTO.DatabaseName is required."); ``` --- ### MEDIUM-22 — No Logging Anywhere in AddOnField BLL **File:** `FrameworkBLL/AddOnField/AddOnFieldBLL.cs` The BLL has no `ILogger` injection and logs nothing. Errors are silently re-thrown. Cache hits/misses cannot be diagnosed. There is no audit trail for field configuration reads. **Fix:** ```csharp public class AddOnFieldBLL : IAddOnFieldBLL { private readonly IAddOnFieldDAL _addOnFieldDAL; private readonly ILogger _logger; public AddOnFieldBLL(IAddOnFieldDAL addOnFieldDAL, ILogger logger) { _addOnFieldDAL = addOnFieldDAL; _logger = logger; } public async Task> GetAddOnField( int addOnFieldId, LoginDTO loginDTO, CancellationToken ct = default) { _logger.LogDebug("GetAddOnField: AddOnFieldId={Id} Tenant={Tenant}", addOnFieldId, loginDTO.DatabaseName); return await _addOnFieldDAL.GetAddOnField(addOnFieldId, loginDTO, ct); } } ``` --- ### MEDIUM-23 — `AddonFieldLoginTimeDTO` Appears to Be Dead Code **File:** `FrameworkDAL/DTO/addonfield/AddonFieldLoginTimeDTO.cs` This DTO defines 6 fields (`EntityCode`, `DataType`, `FieldName`, `EntityDefinedIn`, `AssemblyName`, `AddonDbObjectName`) that describe the runtime reflection-based addon field system from the legacy GB4 architecture (where addon fields were loaded by assembly name and reflection). No call site in the GB5 codebase uses `AddonFieldLoginTimeDTO`. The related SQL that populated it is commented out (MEDIUM-17). The `AssemblyName` and `EntityDefinedIn` fields suggest a reflection-based loading pattern not present in the GB5 microservices architecture. **Recommendation:** Confirm with the team if this DTO is still needed. If not, delete it. If needed for a future migration, add a `// TODO: used by ` comment. --- ### MEDIUM-24 — Dynamic SQL in PayProcessDAL Uses `.Replace()` with `.ToString()` Instead of Parameterised Values **File:** `PayRollDAL/CustomeCode/PayProcess/PayProcessDAL.cs`, lines 1210–1213 ```csharp HqlTOSQL = HqlTOSQL.Replace(":payprocessId", PaySlipReportDTOs[i].PayprocessReport.PayProcessId.ToString()); HqlTOSQL = HqlTOSQL.Replace(":slno", i.ToString()); HqlTOSQL = HqlTOSQL.Replace(":payslipstatus", PaySlipReportDTOs[i].PayprocessReport.PaySlipStatus!.ToString()); await _queryExecutor.ExecuteAsync(LoginDTO, HqlTOSQL, null!, trans); ``` Integer values are being converted to strings and concatenated directly into SQL via `.Replace()`. While these are integers (not user-supplied strings), this practice: - Normalises the string-concatenation SQL pattern, making it easy to accidentally apply to user strings. - Cannot benefit from query plan caching (every invocation is a unique SQL string). - Passes `null!` for parameters (HIGH-13 pattern). **Fix:** Use parameterised queries: ```csharp await _queryExecutor.ExecuteAsync(LoginDTO, PayProcessQB.INSERT_INTO_TABLE, new { PayProcessId = PaySlipReportDTOs[i].PayprocessReport.PayProcessId, SlNo = i, PaySlipStatus = PaySlipReportDTOs[i].PayprocessReport.PaySlipStatus }, trans, ct); ``` Move the SQL to the QB file with proper `@PayProcessId`, `@SlNo`, `@PaySlipStatus` parameters. --- ### LOW-25 — DTO Folder Name Is Lowercase `addonfield` Instead of PascalCase **Path:** `FrameworkDAL/DTO/addonfield/` (lowercase) All other DTO folders in the project use PascalCase (e.g., `FrameworkDAL/DTO/Attachment/`, `FrameworkDAL/DTO/File/`, `FrameworkDAL/DTO/Ice/`). The lowercase folder name is inconsistent and can cause issues on case-sensitive filesystems (Linux). **Fix:** Rename to `FrameworkDAL/DTO/AddOnField/` and update all namespace references. --- ### LOW-26 — `AddOnFieldPickListDTO` Missing `Code` Property **Files:** - `AddOnFieldPickListDTO`: has `Id` and `FieldName`. - `CostAddOnFieldsPicklistDTO`: has `Id`, `Code`, and `Name`. The framework-level picklist DTO is missing the `Code` property present in the costing module's equivalent. This inconsistency means frontend dropdowns using the AddOnField picklist cannot display a code alongside the name, while costing dropdowns can. **Fix:** Align the picklist DTOs: ```csharp public record AddOnFieldPicklistDTO(int Id, string Code, string FieldName); ``` --- ### LOW-27 — `IceMapDetailsDTO.IceMapDetailsAddonFields` Stored as Plain `string` **File:** `FrameworkDAL/DTO/Ice/IceMapDetailsDTO.cs` ```csharp private string icemapdetailsaddonfields; ``` The ICE Map system uses addon field **names** (not IDs) as string references. There is no FK or validation that the referenced addon field name actually exists in `MADDONFIELDS`. A typo or renamed field will cause silent failures during ICE Map import/export without any error. **Recommendation:** Store `ADDONFIELDID` (integer FK) alongside or instead of the name string, and validate at save time. --- ### LOW-28 — `EntityConstant.ADDONFIELD = -1399999982` — Large Negative Magic Number **File:** `GB5Shared/GB5Constant/Constant.cs`, line 1546 ```csharp public static int ADDONFIELD = -1399999982; ``` This constant is used as an object type identifier in cache key generation. Large negative integer constants are non-intuitive, hard to remember, and error-prone if typed manually. They suggest the constants were auto-generated from some legacy ID scheme rather than designed. **Recommendation:** Document the origin and meaning of this value. Consider using an enum or a named constant group: ```csharp // EntityConstants — these IDs correspond to MMASTER.MASTERID values in the database public static class EntityConstant { public const int ADDONFIELD = -1399999982; // MADDONFIELDS master entity ID // ... } ``` --- ### LOW-29 — No Event Log Publishing for Save/Delete (When Implemented) When Save and Delete are eventually implemented, they must publish to the Dapr event log for audit trail, per the observability requirements in CLAUDE.md: ```csharp await _eventLog.PublishAsync(new EventLogDTO { EventText = "AddOnField Saved", UserId = login.UserId, Data = JsonSerializer.Serialize(dto), EventTypeId = EventTypes.CREATE }, login); ``` --- ### LOW-30 — `CriteriaDAL` — All Related Criteria Methods Are Commented Out **File:** `FrameworkDAL/CustomCode/Criteria/CriteriaDAL.cs` The `CriteriaDAL` class has its entire `GetListObjectCriteria` and `GetGenricCriteriaPagination` methods commented out (80+ lines). The `CriteriaAttributeId` field in `AddOnFieldDTO` (`criteriaattributeid = -1` default) is intended to link addon fields to criteria attributes, but the criteria system is non-functional. This means: - Addon field filtering via criteria is not implemented. - The `CRITERIAATTRIBUTEID` column in `MADDONFIELDS` serves no purpose in the current system. **Recommendation:** Either implement the criteria-addon linkage or document that `CriteriaAttributeId` is reserved for future use. --- ### LOW-31 — `AddonFieldLoginTimeDTO.EntityDefinedIn` Holds Assembly Path Without Validation **File:** `FrameworkDAL/DTO/addonfield/AddonFieldLoginTimeDTO.cs` ```csharp public string EntityDefinedIn { get { return entitydefinedin; } set { entitydefinedin = value; } } ``` `EntityDefinedIn` held relative assembly paths in the legacy system. If this DTO is ever re-activated, unvalidated assembly paths passed to `Assembly.Load` or `Type.GetType` enable **assembly injection / arbitrary code loading**. **Recommendation:** If the DTO is reactivated, validate `EntityDefinedIn` against an allowlist of known assembly names. --- ### LOW-32 — `PayProcessEditDTO.AddonFields` Typed as `IList` — Inconsistent Pattern **File:** `PayRollDAL/DTO/PayProcess/PayProcessEditDTO.cs`, line 19 ```csharp private IList addonfields; ``` The DTO uses `IList` interface type for the backing field, while most DTOs in the system use concrete `List`. `IList` as a backing field requires casting when `Add()` or `Count` are needed, and `null` initialisation risks NRE. Should initialise with `new List()` for consistency. --- ## Architectural Summary ### AddOnField Feature Completeness Matrix | Component | Status | Severity | |-----------|--------|----------| | `IAddOnFieldBLL` (interface) | Only GetAddOnField declared | CRITICAL | | `IAddOnFieldDAL` (interface) | Only GetAddOnField declared | CRITICAL | | `AddOnFieldBLL` | Only GetAddOnField implemented | CRITICAL | | `AddOnFieldDAL` | Only GetAddOnField implemented | CRITICAL | | `AddOnFieldQB` | Only GET_ADDONFIELD constant | CRITICAL | | `GET /AddOnField/GetAddOnField` | Working (with issues) | HIGH | | `POST /AddOnField/SaveAddOnField` | Empty stub — 404 | CRITICAL | | `DELETE /AddOnField/DeleteAddOnField` | Empty stub — 404 | CRITICAL | | `GET /AddOnField/GetSelectListAddOnField` | Empty stub — 404 | CRITICAL | | SQL tenant filter | Missing | CRITICAL | | CancellationToken propagation | Missing | HIGH | | Cache level | Wrong (USER vs CLIENT) | HIGH | ### Dynamic Addon Field Usage Pattern (Cross-Module) The PayRoll module uses a runtime SQL assembly pattern to dynamically query addon fields: ``` 1. Query ALL_PAYPROCSS_FIELDS → get column names from DB schema 2. Build SELECT list by string concatenation with :addonfields 3. Replace :addonfields in template SQL → execute dynamic query 4. Merge results into main DTO via JObject ``` This pattern has several weaknesses: - **No column name validation** (CRITICAL-5). - **No parameterisation** of IDs passed to INSERT statements (MEDIUM-24). - **`ArrayList` for column names** (HIGH-12). - **`null!` for empty parameters** (HIGH-13). - **No caching** of the field list — `ALL_PAYPROCSS_FIELDS` is queried on every pay slip report generation. --- ## Priority Remediation Plan ### Immediate (before next release) 1. **CRITICAL-1**: Add `AND AF.DATABASENAME = @DatabaseName` to `GET_ADDONFIELD` and all future QB queries. 2. **CRITICAL-2**: Remove `AllowAnonymous()` from `GetAddOnField` endpoint. 3. **CRITICAL-3**: Replace `ex.Message` with a generic error message; add `ILogger` and log the real exception. 4. **CRITICAL-5**: Add column name validation (alphanumeric regex) before `:addonfields` SQL substitution in PayProcessDAL. Quote column names with `[]`. ### Short-Term (next sprint) 5. **HIGH-6**: Add `CancellationToken` to `IAddOnFieldBLL`, `IAddOnFieldDAL`, and their implementations. Pass it through to `QueryAsync`. 6. **HIGH-7 + HIGH-8**: Remove the `JsonConvert.SerializeObject(result)` call and the `finally { json = null; }` block in `AddOnFieldDAL`. 7. **HIGH-9**: Remove all `try/catch { throw; }` no-op blocks in BLL, DAL, and `GetCacheKey`. 8. **HIGH-10**: Convert `AddOnFieldPickListDTO` and `CostAddOnFieldsPicklistDTO` from `struct` to `record`. 9. **HIGH-11**: Change cache level from `USER_LEVEL` to `CLIENT_LEVEL`. 10. **HIGH-12 + HIGH-13**: Replace `ArrayList` with `List`; replace `null!` with `new { }` in PayProcessDAL. 11. **MEDIUM-21**: Add input validation to `AddOnFieldBLL.GetAddOnField`. ### Medium-Term (next quarter) 12. **CRITICAL-4**: Implement the Save, Delete, and GetSelectList stack (QB → DAL → BLL → SL) for the AddOnField module. 13. **MEDIUM-15 + MEDIUM-16**: Convert `AddOnFieldDTO` and `CostAddOnFieldsDTO` from Java-era backing fields to auto-properties. 14. **HIGH-14**: Replace `ItemWithDynamicAddonFieldDTO.FieldValue` (string) with a typed value holder. 15. **MEDIUM-24**: Move PayProcess inline SQL to parameterised QB constants. 16. **MEDIUM-19**: Remove trailing semicolon from `GET_ADDONFIELD` SQL. 17. **LOW-25**: Rename DTO folder to `AddOnField` (PascalCase). 18. **MEDIUM-17 + MEDIUM-23**: Remove dead comments and evaluate `AddonFieldLoginTimeDTO` for deletion. --- ## Conclusion The AddOnField module serves as a foundational extensibility mechanism for the entire ERP system, allowing custom fields to be added to any entity at runtime. Despite its critical importance, the framework-level CRUD layer is only ~25% complete (Get only; Save/Delete/SelectList are stubs). The one working endpoint lacks authentication and tenant filtering, creating a direct IDOR vulnerability. The dynamic SQL pattern used by PayRoll for runtime column assembly has injection risk that must be mitigated immediately. The module-wide absence of `CancellationToken` and the no-op exception handling patterns are systemic issues that affect both correctness and performance. Prioritise the VAPT fixes (CRITICAL-1 through 5) immediately, then implement the missing CRUD operations.