# Address Module — Deep Code Analysis **Date:** 2026-03-03 **Scope:** All Address, TRM Address, Contact-Address, and AddressContactSearch code in GB5Framework and PayRoll **Modules analysed:** - `FrameworkBLL/Address/` — AddressBLL, IAddressBLL - `FrameworkBLL/TRMAddress/` — TRMAddressBLL, ITRMAddressBLL - `FrameworkDAL/CustomCode/Address/` — AddressDAL, IAddressDAL - `FrameworkDAL/CustomCode/TRMAddress/` — TRMAddressDAL, ITRMAddressDAL - `FrameworkDAL/Query/Address/` — AddressQB - `FrameworkDAL/Query/TRMAddress/` — TRMAddressQB - `FrameworkDAL/DTO/Address/` — AddressDTO, AddressFlatDTO, AddressPicklistDTO, AddressPicklistNewDTO, AddressReportDTO, AddressContactDumpUpdateDTO - `FrameworkSL/Endpoints/Address/` — GetAddress, GetAddressDetail, GetSelectListAddress, GetSelectListAddressNew, SaveAddress, DeleteAddress - `FrameworkSL/Endpoints/TRMAddress/` — GetTRMAddress, SaveTRMAddress, DeleteTRMAddress - `PayRollBLL/Address/` — AddressBLL, IAddressBLL - `PayRollDAL/CustomeCode/Address/` — AddressDAL, IAddressDAL - `PayRollDAL/Query/Address/` — AddressQB - `PayRollSL/EndPoints/Address/` — SaveAddress (commented out) - `AdminBLL/AddressContactSearch/` — AddressContactSearchBLL (empty stub) - `AdminDAL/CustomCode/AddressContactSearch/` — AddressContactSearchDAL (empty stub) - `AdminDAL/Query/AddressContactSearch/` — AddressContactSearchQB (empty stub) - `AdminSL/EndPoints/AddressContactSearch/` — GetAddressDetail (empty stub) --- ## Executive Summary The Address module is a **critical framework-level service** used by virtually every module in the ERP (PayRoll, Accounts, MM, Logistics, CRM, Marketing). It stores physical addresses, contact details, and statutory tax identifiers (PAN, TAN, GSTIN, VAT, Excise) for parties, branches, and employees. Despite its critical importance, **every single endpoint uses `AllowAnonymous()`** — there is no authentication on any read, write, or delete operation. All SQL queries **lack tenant filters**, making every address record cross-tenant accessible and deletable. A `WITH (NOLOCK)` hint on the select-list query can return dirty data for financial tax identifiers. A query for employee addresses has **no object-ID filter** and returns every employee's address in the system — a mass data leak. The module has five near-duplicate DTOs, a systemic `finally { json = null; }` no-op pattern throughout the DAL, hardcoded magic numbers, mutable structs, and the Admin AddressContactSearch module is entirely empty stubs. **Total issues found:** 35 - Critical: 7 - High: 10 - Medium: 11 - Low: 7 --- ## Issue Index | # | Severity | Category | Title | |---|----------|----------|-------| | 1 | CRITICAL | VAPT | No tenant filter on any SELECT query — cross-tenant IDOR | | 2 | CRITICAL | VAPT | `AllowAnonymous` on every endpoint — no authentication at all | | 3 | CRITICAL | VAPT | Hard `DELETE` with no tenant filter — any caller can delete any record | | 4 | CRITICAL | VAPT | `WITH (NOLOCK)` on `GET_SELECTLIST_ADDRESS_NEW` — dirty reads of tax identifiers | | 5 | CRITICAL | VAPT | `GET_ADDRESS_DETAIL_FOR_FULL_EMPLOYEE` has no `ObjectId` filter — returns ALL employees' addresses | | 6 | CRITICAL | VAPT | `ex.Message` leaked in all error responses | | 7 | CRITICAL | Architecture | `DeleteAddress` BLL deletes TRM first then main record — non-atomic, no transaction | | 8 | HIGH | Best Practice | No `CancellationToken` in any interface, BLL, or DAL method | | 9 | HIGH | Bug | `GetSelectListAddressNew` passes `null!` to `QueryAsync` — runtime NullReferenceException | | 10 | HIGH | Bug | `GetSelectListAddress` ignores `CriteriaDTO` entirely — criteria filtering silently dropped | | 11 | HIGH | Bug | `GetSelectListAddress` `IsCount == true` branch always returns hardcoded `0` — pagination broken | | 12 | HIGH | Bug | `GetAddressDetail` uses `QuerySingleAsync` but query can return multiple rows — InvalidOperationException | | 13 | HIGH | Performance | `TRMAddressDAL.GetTRMAddress` queries `QueryAsync` into full `AddressDTO` for translation-only data | | 14 | HIGH | Architecture | `GET_ADDRESS_DETAIL` SQL duplicated verbatim in both Framework and PayRoll QB files | | 15 | HIGH | Bug | `GET_ADDRESS_DETAIL_FOR_FULL_EMPLOYEE` uses `INNER JOIN MREGION` — employees without region excluded | | 16 | HIGH | Best Practice | `PayRollBLL.AddressBLL` uses `DateTime.Now` instead of `DateTime.UtcNow` | | 17 | HIGH | Architecture | `PayRollSL/SaveAddress.cs` is entirely commented out — no active endpoint | | 18 | MEDIUM | Best Practice | Java-era backing fields in `AddressDTO` (~550 lines, 50+ fields) | | 19 | MEDIUM | Architecture | `AddressFlatDTO` nearly duplicates `AddressDTO` with minor type differences | | 20 | MEDIUM | Architecture | `AddressReportDTO` is a third partial duplicate of `AddressDTO` | | 21 | MEDIUM | Best Practice | `AddressPicklistDTO` is a mutable `struct` | | 22 | MEDIUM | Caching | `GetAddressDetail` inconsistency: cache key uses `ROLE_LEVEL`, response uses `USER_LEVEL` | | 23 | MEDIUM | Caching | `GetTRMAddress` computes cache key at `USER_LEVEL` but response uses `NOT_REQUIRED` | | 24 | MEDIUM | Performance | `AddressDAL.SaveAddress/UpdateAddress` passes entire `AddressDTO` as Dapper parameter | | 25 | MEDIUM | Best Practice | `Google.Apis.Json` imported in `AddressDAL.cs` — unused, inappropriate dependency | | 26 | MEDIUM | Best Practice | `finally { json = null; }` no-op in `AddressDAL.GetAddress` and `TRMAddressDAL.GetTRMAddress` | | 27 | MEDIUM | Architecture | `AddressContactSearchBLL/DAL/QB/SL` are all empty stubs — feature entirely unimplemented | | 28 | MEDIUM | Bug | `TRMAddressDAL.DeleteTRMAddress` returns `Failure` when 0 rows deleted — blocks parent address delete for EN-only records | | 29 | MEDIUM | VAPT | `AddressDTO` default string values are `"NONE"` — inserted verbatim into DB | | 30 | LOW | Architecture | `GetSelectListAddress` uses HTTP `POST` for a data retrieval endpoint | | 31 | LOW | Architecture | `GetSelectListAddressNew` also uses HTTP `POST` for retrieval | | 32 | LOW | Best Practice | `TRMAddressQB.SAVE_TRMADDRESS` has `ADDRESSTRANSLATIONID` commented out with no documentation | | 33 | LOW | Best Practice | `AddressFlatDTO.AddressAddressType` is `int` but `AddressDTO.AddressAddressType` is `byte` — type inconsistency | | 34 | LOW | Best Practice | `GET_ADDRESS` accepts `@languageid` parameter but never uses it for filtering or translation | | 35 | LOW | Performance | `AddressBLL.GetAddress` string-comparison branch check for `"EN"` vs TRM — no input validation on `LanguageId` | --- ## Detailed Findings --- ### CRITICAL-1 — No Tenant Filter on Any SELECT Query — Cross-Tenant IDOR **Files:** `FrameworkDAL/Query/Address/AddressQB.cs`, `PayRollDAL/Query/Address/AddressQB.cs` ```sql -- GET_ADDRESS WHERE A.ADDRESSID = @addressid; -- ← no DATABASENAME filter -- GET_ADDRESS_DETAIL WHERE A.OBJECTTYPEID = @ObjectTypeId AND A.OBJECTID = @ObjectId; -- ← no DATABASENAME filter -- GET_SELECTLIST_ADDRESS FROM MADDRESS A -- no WHERE on tenant at all -- GET_SELECTLIST_ADDRESS_NEW FROM MADDRESS A WITH (NOLOCK); -- no WHERE at all ``` None of the four SELECT queries filter by `DATABASENAME` or any tenant identifier. An authenticated user from Tenant A can: - Query any party's PAN, TAN, GSTIN, VAT numbers from Tenant B by supplying a known `ADDRESSID`. - List all addresses in the database (including all tenants) via the select-list endpoint. - Retrieve employee/party/branch addresses across tenant boundaries. This is a direct IDOR (Insecure Direct Object Reference) and multi-tenancy breach. Address records contain highly sensitive statutory data (PAN, TAN, GSTIN, VAT/CST, Excise numbers, mobile numbers, email). **Fix:** Add mandatory tenant filter to all queries: ```sql -- GET_ADDRESS WHERE A.ADDRESSID = @AddressId AND A.DATABASENAME = @DatabaseName -- GET_SELECTLIST_ADDRESS FROM MADDRESS A WHERE A.DATABASENAME = @DatabaseName AND ...pagination... ``` Pass `loginDTO.DatabaseName` as a parameter in every DAL call: ```csharp var parameters = new { AddressId = addressId, loginDTO.DatabaseName }; ``` --- ### CRITICAL-2 — `AllowAnonymous` on Every Endpoint — No Authentication At All **Files:** All 9 endpoint files in `FrameworkSL/Endpoints/Address/` and `FrameworkSL/Endpoints/TRMAddress/` ```csharp // GetAddress.cs public override void Configure() { Get("/Address/GetAddress"); AllowAnonymous(); // ← no auth on read } // SaveAddress.cs public override void Configure() { Post("/Address/SaveAddress"); AllowAnonymous(); // ← no auth on write } // DeleteAddress.cs public override void Configure() { Delete("/Address/DeleteAddress"); AllowAnonymous(); // ← no auth on delete } // GetTRMAddress, SaveTRMAddress, DeleteTRMAddress — same ``` Every endpoint that reads, writes, and deletes address records (including PAN, TAN, GSTIN, VAT numbers) is completely unauthenticated. Combined with CRITICAL-1 (no tenant filter), any external caller can: - Read all tax registration numbers for all parties across all tenants. - Create or modify address records for any party. - Delete address records without being logged in. **Fix:** Remove all `AllowAnonymous()` calls. Require authentication (JWT/OIDC via the middleware chain). The `Login` header is already received and parsed as `LoginDTO` by the base class — just remove the bypass. --- ### CRITICAL-3 — Hard `DELETE` With No Tenant Filter — Any Caller Can Delete Any Record **File:** `FrameworkDAL/Query/Address/AddressQB.cs`, lines 327–328 ```sql public const string DELETE_ADDRESS = @" DELETE FROM MADDRESS WHERE ADDRESSID = @addressid;"; ``` ```sql public const string DELETE_TRMADDRESS = @" DELETE FROM TR_MADDRESS WHERE ADDRESSID = @addressid;"; ``` Both DELETE statements are hard deletes (physically removing rows) with no tenant filter. Combined with `AllowAnonymous` on the endpoint, any caller knowing a valid `ADDRESSID` can permanently delete: - Party billing/shipping addresses. - Employee residential addresses. - All stored PAN/TAN/GSTIN records linked to the deleted address. This is an irreversible destructive operation with no access control. **Fix:** ```sql DELETE FROM MADDRESS WHERE ADDRESSID = @AddressId AND DATABASENAME = @DatabaseName; ``` Consider soft delete (`UPDATE MADDRESS SET STATUS = 0`) to allow recovery. Add audit trail (Dapr event log) for all deletes. --- ### CRITICAL-4 — `WITH (NOLOCK)` on `GET_SELECTLIST_ADDRESS_NEW` — Dirty Reads of Tax Identifiers **File:** `FrameworkDAL/Query/Address/AddressQB.cs`, lines 177–187 ```sql public const string GET_SELECTLIST_ADDRESS_NEW = @"SELECT A.ADDRESSID AS Id, A.OBJECTID AS ObjectId, A.OBJECTTYPEID AS ObjectTypeId, A.ADDRESSLINE1 AS Line1, A.ADDRESSLINE2 AS Line2, A.ADDRESSLINE3 AS Line3, A.MOBILE AS Mobile, A.FAX AS Fax FROM MADDRESS A WITH (NOLOCK); -- ← dirty read hint ``` `WITH (NOLOCK)` (READ UNCOMMITTED) means this query can return: - **Uncommitted rows** — data that is being inserted/updated by another transaction and may be rolled back. - **Partially written rows** — a row where some columns are written and others are not yet (phantom row). - **Rows that don't exist** — rows being deleted in the same transaction. For address data used in party selection (PO/SO/Invoice), this can return addresses with stale or invalid mobile numbers, partial data, or rows that no longer exist. This is particularly dangerous if the address list is used to generate invoices. **Fix:** Remove `WITH (NOLOCK)`. If read performance is a concern, use appropriate indexing and `READ COMMITTED SNAPSHOT ISOLATION` (RCSI) at the database level instead: ```sql FROM MADDRESS A -- no NOLOCK hint ``` --- ### CRITICAL-5 — `GET_ADDRESS_DETAIL_FOR_FULL_EMPLOYEE` Has No `ObjectId` Filter — Mass Data Leak **File:** `PayRollDAL/Query/Address/AddressQB.cs`, lines 157–200 ```sql public const string GET_ADDRESS_DETAIL_FOR_FULL_EMPLOYEE = @" SELECT addr.ADDRESSID, addr.OBJECTTYPEID, addr.OBJECTID, ... FROM MADDRESS addr INNER JOIN MREGION region ON region.regionID = addr.STANDARDREGIONID INNER JOIN mCITY city ON city.CITYID = addr.CITYID INNER JOIN MCOUNTRY country ON country.COUNTRYID = addr.COUNTRYID WHERE addr.OBJECTTYPEID = -1399999905"; -- ← only filters by entity type, no employee ID ``` This query returns **every employee's address** in the entire database. There is: - No `OBJECTID = @employeeId` filter. - No `DATABASENAME` tenant filter. - No pagination. Calling this query returns a full table scan of all employee addresses across all tenants. Each address row contains residential addresses, phone numbers, and email addresses of all employees. **Fix:** Add both tenant and object filters: ```sql WHERE addr.OBJECTTYPEID = -1399999905 -- Employee entity type AND addr.OBJECTID = @EmployeeId -- specific employee AND addr.DATABASENAME = @DatabaseName -- tenant isolation ``` --- ### CRITICAL-6 — `ex.Message` Leaked in All Error Responses **Files:** All 9 endpoints in Address and TRMAddress folders ```csharp catch (Exception ex) { return await GB5Shared.ResponseStandard.Response.CreateExceptionError( ex, CacheKeyLevel.USER_LEVEL, LoginDTO, ex.Message, 500); // ↑ exposes internal details } ``` Dapper SQL errors include the full SQL statement, table names, column names, and constraint names. Connection errors expose server names. These are returned verbatim to the API caller. **Fix:** Log the full exception server-side; return a generic message to the client: ```csharp catch (Exception ex) { _logger.LogError(ex, "GetAddress failed for AddressId {Id}", req.AddressId); return await GB5Shared.ResponseStandard.Response.CreateExceptionError( ex, CacheKeyLevel.USER_LEVEL, LoginDTO, "An error occurred. Please contact support.", 500); } ``` --- ### CRITICAL-7 — `DeleteAddress` BLL Deletes TRM First Then Main Record — Non-Atomic, No Transaction **File:** `FrameworkBLL/Address/AddressBLL.cs`, lines 127–151 ```csharp public async Task> DeleteAddress(int AddressId, LoginDTO LoginDTO) { var trmResult = await _trmAddressDAL.DeleteTRMAddress(AddressId, LoginDTO); if (!trmResult.IsSuccess) return Result.Failure("Failed to delete TRM Address. Parent not deleted."); var addrResult = await _addressDAL.DeleteAddress(AddressId, LoginDTO); // ← second operation if (!addrResult.IsSuccess) return Result.Failure("TRM Address deleted, but failed to delete Address."); // ↑ TRM is already gone, parent delete failed — orphaned state ``` The two delete operations are executed sequentially without a database transaction: 1. If `DeleteTRMAddress` succeeds but `DeleteAddress` fails — TRM translations are deleted but the parent address remains. Inconsistent state. 2. If the process crashes between the two calls — same orphaned state. 3. `DeleteAddress` itself wraps a single-statement list in `ExecuteInTransactionAsync` — redundant for a single DELETE. Additionally, `TRMAddressDAL.DeleteTRMAddress` returns `Failure` when 0 rows are affected (address has no translations). This means addresses that were never translated (English-only) **cannot be deleted** — `DeleteAddress` always returns `Failure` for them. **Fix:** Execute both deletes in a single transaction. Accept 0 affected rows from TRM delete as a success (translations may not exist): ```csharp await using var tx = await _queryExecutor.BeginTransactionAsync(loginDTO); try { await _queryExecutor.ExecuteAsync(loginDTO, TRMAddressQB.DELETE_TRMADDRESS, new { AddressId }, tx); await _queryExecutor.ExecuteAsync(loginDTO, AddressQB.DELETE_ADDRESS, new { AddressId, loginDTO.DatabaseName }, tx); await tx.CommitAsync(); return Result.Success("Address deleted successfully."); } catch { await tx.RollbackAsync(); throw; } ``` --- ### HIGH-8 — No `CancellationToken` in Any Interface, BLL, or DAL Method **Files:** `IAddressBLL.cs`, `IAddressDAL.cs`, `ITRMAddressBLL.cs`, `ITRMAddressDAL.cs` and all implementations ```csharp // IAddressBLL.cs Task GetAddress(int AddressId, string LanguageId, LoginDTO LoginDTO); Task SaveAddress(AddressDTO AddressDTO, string LanguageId, LoginDTO LoginDTO); // ... no CancellationToken anywhere ``` CLAUDE.md mandates `CancellationToken` propagation in every async method chain. Without it, client disconnects do not cancel in-flight database queries. Address queries with multiple JOINs (GET_ADDRESS_DETAIL joins 7 tables) hold connection pool slots open until query completion regardless of whether the client is still connected. **Fix:** Add `CancellationToken ct = default` to all interface methods and propagate through to `_queryExecutor` calls: ```csharp // Interface Task GetAddress(int addressId, string languageId, LoginDTO login, CancellationToken ct = default); // DAL implementation public async Task GetAddress(int addressId, string languageId, LoginDTO login, CancellationToken ct = default) { var result = await _queryExecutor.QuerySingleAsync( login, AddressQB.GET_ADDRESS, new { AddressId = addressId, login.DatabaseName }, cancellationToken: ct); return JsonSerializer.Serialize(result); } ``` --- ### HIGH-9 — `GetSelectListAddressNew` Passes `null!` to `QueryAsync` **File:** `FrameworkDAL/CustomCode/Address/AddressDAL.cs`, line 96 ```csharp var Result = await _QueryExecutor.QueryAsync(LoginDTO, SQL, null!); ``` `null!` suppresses the nullable warning but passes an actual `null` as Dapper's parameter object. If `IQueryExecutor.QueryAsync` does not handle `null` parameters explicitly, Dapper will throw a `NullReferenceException` at runtime. **Fix:** ```csharp var Result = await _QueryExecutor.QueryAsync( LoginDTO, SQL, new { LoginDTO.DatabaseName }); ``` --- ### HIGH-10 — `GetSelectListAddress` Ignores `CriteriaDTO` Entirely **File:** `FrameworkDAL/CustomCode/Address/AddressDAL.cs`, lines 66–89 ```csharp public async Task GetSelectListAddress(int FirstNumber, int MaxResult, CriteriaDTO CriteriaDTO, LoginDTO LoginDTO, bool IsCount = false) { // CriteriaDTO is accepted but never read or used string SQL = AddressQB.GET_SELECTLIST_ADDRESS; var parameter = new { firstnumber = FirstNumber, maxresult = MaxResult }; var Result = await _QueryExecutor.QueryAsync(LoginDTO, SQL, parameter); ``` The `CriteriaDTO` parameter — which may contain search filters (address line text, city, zip code) sent by the frontend — is silently ignored. All addresses are returned unfiltered. This means: - Address search UI shows all records instead of filtered results. - No filtering by active/inactive status. - No search by address line or city. **Fix:** Either implement criteria filtering in the SQL query (passing filter parameters), or remove the `CriteriaDTO` parameter from the interface if it is not intended to be used. --- ### HIGH-11 — `GetSelectListAddress` Count Branch Always Returns Hardcoded `0` **File:** `FrameworkDAL/CustomCode/Address/AddressDAL.cs`, lines 79–82 ```csharp else // IsCount == true { int Count = 0; Json = Count.ToString(); // ← always returns "0" } ``` When called for pagination count (`IsCount == true`), the method returns `"0"` without executing any SQL. Any pagination UI that uses this count will always show "Total: 0" or "Page 1 of 0", breaking all pagination controls for address selection lists. **Fix:** Execute a `COUNT(*)` query when `IsCount == true`: ```csharp int Count = await _QueryExecutor.ExecuteScalarAsync( LoginDTO, AddressQB.COUNT_SELECTLIST_ADDRESS, new { LoginDTO.DatabaseName }); Json = Count.ToString(); ``` Add `COUNT_SELECTLIST_ADDRESS` to `AddressQB`. --- ### HIGH-12 — `GetAddressDetail` Uses `QuerySingleAsync` But Query Can Return Multiple Rows **File:** `FrameworkDAL/CustomCode/Address/AddressDAL.cs`, lines 51–63 ```csharp AddressDTO AddressDTO = await _QueryExecutor.QuerySingleAsync(LoginDTO, Sql, Parameters); ``` The SQL: ```sql WHERE A.OBJECTTYPEID = @ObjectTypeId AND A.OBJECTID = @ObjectId ``` A party branch (`OBJECTID`) typically has **multiple addresses** (billing address, shipping address, delivery address) distinguished by `ADDRESSTYPE`. `QuerySingleAsync` throws `InvalidOperationException: Sequence contains more than one element` when more than one address row is returned. This is a runtime exception waiting to happen for any party that has both billing and shipping addresses. **Fix:** Use `QueryAsync` and return `IEnumerable`. Update the interface and BLL accordingly: ```csharp IEnumerable results = await _QueryExecutor.QueryAsync(LoginDTO, Sql, Parameters); return JsonSerializer.Serialize(results); ``` --- ### HIGH-13 — `TRMAddressDAL.GetTRMAddress` Queries Full `AddressDTO` for Translation-Only Data **File:** `FrameworkDAL/CustomCode/TRMAddress/TRMAddressDAL.cs`, lines 22–42 ```csharp IEnumerable addressDTOs = await _queryExecutor.QueryAsync(loginDTO, sql, parameters); ``` The `TRMAddressQB.GET_TRMADDRESS` query returns ~35 columns but the TRM (translation) table only has translated `ADDRESSLINE1–5`. The other 30 columns are fetched from `MADDRESS` (the base record) but are not the purpose of a translation query. Mapping 35+ fields into the full `AddressDTO` (which has 50+ properties) per row is wasteful. A dedicated `AddressTranslationDTO` with only the translated fields would reduce memory allocation and clarify intent. **Fix:** Create a lean `AddressTranslationDTO`: ```csharp public record AddressTranslationDTO( int AddressId, string LanguageId, int AddressTranslationId, string AddressLine1, string AddressLine2, string AddressLine3, string AddressLine4, string AddressLine5 ); ``` --- ### HIGH-14 — `GET_ADDRESS_DETAIL` SQL Duplicated Verbatim in Framework and PayRoll QB Files **Files:** - `FrameworkDAL/Query/Address/AddressQB.cs` — `GET_ADDRESS_DETAIL` (lines 82–158) - `PayRollDAL/Query/Address/AddressQB.cs` — `GET_ADDRESS_DETAIL` (lines 79–155) The same 80-line SQL with identical column list and JOIN structure is copy-pasted into two separate query builder files. Any future change (adding a column, fixing a JOIN) must be made in two places. The two copies have already diverged slightly in formatting. **Fix:** Define `GET_ADDRESS_DETAIL` in one location (Framework) and reference it from PayRoll. PayRoll can inherit or compose: ```csharp // PayRollDAL/Query/Address/AddressQB.cs public static class AddressQB { // Delegate to framework — single source of truth public static string GET_ADDRESS_DETAIL => FrameworkDAL.Query.Address.AddressQB.GET_ADDRESS_DETAIL; } ``` --- ### HIGH-15 — `GET_ADDRESS_DETAIL_FOR_FULL_EMPLOYEE` Uses `INNER JOIN MREGION` — Employees Without Region Excluded **File:** `PayRollDAL/Query/Address/AddressQB.cs`, lines 193–195 and 235–240 ```sql INNER JOIN MREGION region ON region.regionID = addr.STANDARDREGIONID ``` Employees who have no `STANDARDREGIONID` set (nullable FK) will have their addresses excluded from this query. The INNER JOIN silently filters them out, producing an incomplete result without any warning. **Fix:** Change to `LEFT JOIN`: ```sql LEFT JOIN MREGION region ON region.regionID = addr.STANDARDREGIONID ``` --- ### HIGH-16 — `PayRollBLL.AddressBLL` Uses `DateTime.Now` Instead of `DateTime.UtcNow` **File:** `PayRollBLL/Address/AddressBLL.cs`, lines 43–44 and 81–82 ```csharp PresentAddress.AddressCreatedOn = DateTime.Now; // ← local time PresentAddress.AddressModifiedOn = DateTime.Now; // ← local time // ... same for PermanentAddress ``` `DateTime.Now` stores server-local time. In multi-region deployments or cloud environments, different server instances may record different local times for concurrent operations. Audit fields (`CreatedOn`, `ModifiedOn`) must use `DateTime.UtcNow` for consistency. **Fix:** ```csharp PresentAddress.AddressCreatedOn = DateTime.UtcNow; PresentAddress.AddressModifiedOn = DateTime.UtcNow; ``` --- ### HIGH-17 — `PayRollSL/SaveAddress.cs` Is Entirely Commented Out — No Active Endpoint **File:** `PayRollSL/EndPoints/Address/SaveAddress.cs` The entire file (47 lines) is commented out: ```csharp //namespace PayRollSL.EndPoints.Address //{ // public class SaveAddress : BaseEndpoint<...> // { // // ... entire implementation commented out // } //} ``` The PayRoll module has no active address save endpoint. Address saves in PayRoll go through the Framework endpoint at `/Address/SaveAddress`. However, `PayRollBLL.AddressBLL.SaveOrUpdatePresentAddress` and `SaveOrUpdatePermanentAddress` are only callable internally from the Employee save flow, not as standalone endpoints. **Fix:** Either implement the endpoint properly or remove the dead commented file. --- ### MEDIUM-18 — Java-Era Backing Fields in `AddressDTO` (~550 Lines, 50+ Fields) **File:** `FrameworkDAL/DTO/Address/AddressDTO.cs` ```csharp private int addressid; public int AddressId { get { return addressid; } set { addressid = value; } } // ... repeated 50+ times (~550 lines total) ``` This is the most verbose DTO in the codebase. It could be expressed as: ```csharp public class AddressDTO { public int AddressId { get; set; } public int AddressObjectTypeId { get; set; } public int AddressObjectId { get; set; } public byte AddressAddressType { get; set; } public byte AddressAddressNature { get; set; } public string AddressLine1 { get; set; } = string.Empty; // ... ~50 auto-properties } ``` This would reduce the file from 556 lines to ~60 lines with identical functionality. --- ### MEDIUM-19 — `AddressFlatDTO` Nearly Duplicates `AddressDTO` With Minor Type Differences **File:** `FrameworkDAL/DTO/Address/AddressFlatDTO.cs` `AddressFlatDTO` has the same 50+ fields as `AddressDTO` with one difference: numeric status fields use `int` instead of `byte`/`Int16`. The file is ~499 lines of near-identical code. This creates two DTOs to maintain for the same data shape. When a new field is added to the address table, it must be added to both DTOs. **Fix:** Evaluate whether the `int` vs `byte` difference is needed. If not, remove `AddressFlatDTO` and use `AddressDTO` everywhere. If the type difference is genuinely needed (e.g., for a different API contract), add `[JsonIgnore]` or projection attributes rather than duplicating the entire class. --- ### MEDIUM-20 — `AddressReportDTO` Is a Third Partial Duplicate of `AddressDTO` **File:** `FrameworkDAL/DTO/Address/AddressReportDTO.cs` `AddressReportDTO` is yet another ~236-line class with 30 of the same fields as `AddressDTO`, using the same Java-era backing field pattern. Three near-identical DTOs exist for the same address data. **Fix:** Consolidate into one `AddressDTO` with `[JsonIgnore]` on report-specific fields, or use projection/inheritance. The proliferation of near-duplicate DTOs indicates absent DTO governance. --- ### MEDIUM-21 — `AddressPicklistDTO` Is a Mutable `struct` **File:** `FrameworkDAL/DTO/Address/AddressPicklistDTO.cs` ```csharp public struct AddressPicklistDTO // ← mutable struct { private int id; private string line1; // ... public int Id { get { return id; } set { id = value; } } ``` Mutable structs cause silent copy-on-modify bugs when stored in `List` or `IEnumerable`. Each element accessed is a copy; modifying `list[0].Line1 = "X"` modifies the copy, not the list element. Dapper maps into structs differently and may fail silently on some projections. **Fix:** Convert to a `record`: ```csharp public record AddressPicklistDTO(int Id, string Line1, string Line2, string Line3, string Line4, string Mobile); ``` --- ### MEDIUM-22 — `GetAddressDetail` Inconsistency: Cache Key Uses `ROLE_LEVEL`, Response Uses `USER_LEVEL` **File:** `FrameworkSL/Endpoints/Address/GetAddressDetail.cs` ```csharp protected override string? GetCacheKey(GetAddressDetailParameters Parameters, LoginDTO LoginDTO) { return KeyGenerator.KeyGeneration(..., CacheKeyLevel.ROLE_LEVEL, LoginDTO); // ← ROLE_LEVEL key } protected override async Task> ExecuteAsync(...) { return await GB5Shared.ResponseStandard.Response.CreateSuccessResponse( Result, CacheKeyLevel.USER_LEVEL, LoginDTO); // ← USER_LEVEL response } ``` The cache key is generated at `ROLE_LEVEL` (shared across all users of the same role) but the response is stored at `USER_LEVEL` (per user). This mismatch means: - Cache miss: `ROLE_LEVEL` key computed, but stored as `USER_LEVEL` — key never matches on subsequent requests. - Or: different users with the same role share cached address data that may be user-specific. Either way, the cache is either never hit or incorrectly shared. **Fix:** Use consistent levels: ```csharp // Address detail is shared across users of the same client protected override string? GetCacheKey(...) => KeyGenerator.KeyGeneration(..., CacheKeyLevel.CLIENT_LEVEL, LoginDTO); protected override async Task> ExecuteAsync(...) => CreateSuccessResponse(result, CacheKeyLevel.CLIENT_LEVEL, LoginDTO); ``` --- ### MEDIUM-23 — `GetTRMAddress` Computes Cache Key But Response Uses `NOT_REQUIRED` **File:** `FrameworkSL/Endpoints/TRMAddress/GetTRMAddress.cs` ```csharp protected override string? GetCacheKey(...) { return KeyGenerator.KeyGeneration(parameters.AddressId!, EntityConstant.OBJECTADDRESS, CacheKeyLevel.USER_LEVEL, loginDTO); // ← computes cache key } protected override async Task> ExecuteAsync(...) { return await CreateSuccessResponse(result, CacheKeyLevel.NOT_REQUIRED, loginDTO); // ← never cached } ``` `GetCacheKey` does CPU work to generate a cache key string, then `CreateSuccessResponse` is called with `NOT_REQUIRED` — so the key is never stored or used. Wasted computation on every request. **Fix:** Either return `null` from `GetCacheKey` (to disable caching) or align both to use the same level (e.g., `USER_LEVEL` since TRM content varies per language and user session). --- ### MEDIUM-24 — `AddressDAL.SaveAddress/UpdateAddress` Passes Entire `AddressDTO` as Dapper Parameter **File:** `FrameworkDAL/CustomCode/Address/AddressDAL.cs`, lines 106–117 ```csharp public async Task SaveAddress(AddressDTO addressDTO, string LanguageId, LoginDTO loginDTO) { string sql = AddressQB.SAVE_ADDRESS; return await _QueryExecutor.ExecuteAsync(loginDTO, sql, addressDTO); // ← full DTO as param bag } ``` `AddressDTO` has 50+ properties. Dapper sends all matching properties as parameters. The INSERT only needs ~47 of them. Extra properties (`PartyCode`, `PartyName`, `CityCode`, `CityName`, `CountryCode`, etc.) are sent as unused parameters. While Dapper ignores unmatched properties, this: - Increases the size of the parameter bag sent over the wire. - Means refactoring `AddressDTO` (renaming a property) could silently break the SQL mapping. **Fix:** Use an explicit anonymous object for the parameters matching SQL column bindings, or use a dedicated save-specific DTO. --- ### MEDIUM-25 — `Google.Apis.Json` Imported in `AddressDAL.cs` — Unused Dependency **File:** `FrameworkDAL/CustomCode/Address/AddressDAL.cs`, line 11 ```csharp using Google.Apis.Json; ``` The Google APIs client library's JSON namespace has no business being in a Dapper-based Address DAL. This suggests a copy-paste error or abandoned Google Sheets integration attempt. The `using` is unused and adds an unnecessary transitive dependency on the Google API client package. **Fix:** Remove the `using Google.Apis.Json;` statement. Check if the `Google.Apis` package can be removed from the project entirely. --- ### MEDIUM-26 — `finally { json = null; }` No-Op in `AddressDAL.GetAddress` and `TRMAddressDAL.GetTRMAddress` **Files:** - `AddressDAL.cs`, line 47: `finally { Json = null; }` - `TRMAddressDAL.cs`, line 40: `finally { json = null; }` Setting a local `string` variable to `null` in a `finally` block does nothing — the variable goes out of scope at method exit regardless. This is a systemic no-op pattern (also seen in `AddOnFieldDAL`, `FileDAL`). It adds noise without any benefit. **Fix:** Remove both `finally` blocks. --- ### MEDIUM-27 — `AddressContactSearchBLL/DAL/QB/SL` Are All Empty Stubs — Feature Unimplemented **Files:** - `AdminBLL/AddressContactSearch/AddressContactSearchBLL.cs`: `internal class AddressContactSearchBLL { }` - `AdminDAL/CustomCode/AddressContactSearch/AddressContactSearchDAL.cs`: `internal class AddressContactSearchDAL { }` - `AdminDAL/Query/AddressContactSearch/AddressContactSearchQB.cs`: `internal class AddressContactSearchQB { }` - `AdminSL/EndPoints/AddressContactSearch/GetAddressDetail.cs`: `public class GetAddressDetail { }` All four files are empty. The AddressContactSearch feature (unified address + contact search for admin) is entirely unimplemented. The endpoint class doesn't even inherit `BaseEndpoint`, so it registers no route. All four classes use `internal` visibility, making them inaccessible from cross-assembly calls even if implemented. **Fix:** Either implement the feature or remove the skeleton files. If planned for future, add `// TODO:` with a feature tracker reference and throw `NotImplementedException` in methods. --- ### MEDIUM-28 — `TRMAddressDAL.DeleteTRMAddress` Returns `Failure` When 0 Rows Deleted — Blocks Parent Delete **File:** `FrameworkDAL/CustomCode/TRMAddress/TRMAddressDAL.cs`, lines 77–84 ```csharp if (result > 0) { return Result.Success("TRM Address deleted successfully."); } else { return Result.Failure("TRM Address not found or could not be deleted."); // ← failure when no TRM exists } ``` In `AddressBLL.DeleteAddress`: ```csharp var trmResult = await _trmAddressDAL.DeleteTRMAddress(AddressId, LoginDTO); if (!trmResult.IsSuccess) return Result.Failure("Failed to delete TRM Address. Parent not deleted."); ``` For any address that was only ever used in English (`LanguageId = "EN"`) — the most common case — no `TR_MADDRESS` record exists. The TRM delete returns 0 rows affected → `Failure` → the BLL returns early without deleting the parent address. **Result: English-only addresses cannot be deleted.** **Fix:** Treat 0 rows as success (no translation existed, nothing to delete): ```csharp // 0 rows is fine — no translation existed return Result.Success("TRM Address deletion processed."); ``` --- ### MEDIUM-29 — `AddressDTO` Default String Values Are `"NONE"` — Inserted Verbatim Into DB **File:** `FrameworkDAL/DTO/Address/AddressDTO.cs`, lines 19–23 ```csharp private string addressline1 = "NONE"; private string addressline2 = "NONE"; private string addressline3 = "NONE"; private string addressline4 = "NONE"; private string addressline5 = "NONE"; private string addressphone = "NONE"; private string addressmobile = "NONE"; // ... many more "NONE" defaults ``` If the caller does not set these fields, the literal string `"NONE"` is inserted into the database. This: - Pollutes address data with `"NONE"` in `ADDRESSLINE1`, `PHONE`, `MOBILE`, etc. - Makes NULL-checks impossible (`WHERE ADDRESSLINE1 IS NULL` returns nothing; actual NULLs are replaced by `"NONE"`). - Causes issues in address formatting/display — `"NONE"` appears in printed invoices and PO documents. **Fix:** Use `null` or `string.Empty` defaults, and let the database store `NULL` for absent optional fields: ```csharp public string AddressLine1 { get; set; } // null by default (optional field) public string AddressPhone { get; set; } // null by default ``` --- ### LOW-30 — `GetSelectListAddress` Uses HTTP `POST` for a Data Retrieval Endpoint **File:** `FrameworkSL/Endpoints/Address/GetSelectListAddress.cs`, line 21 ```csharp public override void Configure() { Post("/Address/GetSelectListAddress"); // ← POST for read ``` CLAUDE.md route convention: `GET /Entity/GetSelectListEntity`. Using POST for reads: - Prevents response caching by HTTP clients and proxies (POST is not cacheable by RFC). - Violates REST conventions. - Breaks any client using a HTTP cache layer. **Fix:** Change to `Get("/Address/GetSelectListAddress")`. Move the `CriteriaDTO` body to query parameters or a separate filter endpoint. --- ### LOW-31 — `GetSelectListAddressNew` Also Uses HTTP `POST` for Retrieval **File:** `FrameworkSL/Endpoints/Address/GetSelectListAddressNew.cs`, line 21 Same issue as LOW-30. `Post("/Address/GetSelectListAddressNew")` — a read endpoint using POST. --- ### LOW-32 — `TRMAddressQB.SAVE_TRMADDRESS` Has `ADDRESSTRANSLATIONID` Commented Out **File:** `FrameworkDAL/Query/TRMAddress/TRMAddressQB.cs`, lines 71–93 ```sql INSERT INTO TR_MADDRESS ( --ADDRESSTRANSLATIONID, ← commented out LANGUAGEID, ADDRESSID, ... ``` The primary key `ADDRESSTRANSLATIONID` is commented out with no explanation. This works only if `ADDRESSTRANSLATIONID` is an `IDENTITY` column. If the column is not an identity (e.g., it's a GUID or application-generated ID), inserts will fail with a null constraint violation. This creates a silent runtime dependency on the database schema that is not documented anywhere in the code. **Fix:** Add a comment explaining the DB schema dependency: ```sql -- ADDRESSTRANSLATIONID is omitted — DB column is IDENTITY(1,1), value auto-generated ``` --- ### LOW-33 — `AddressFlatDTO.AddressAddressType` Is `int` But `AddressDTO.AddressAddressType` Is `byte` **Files:** - `AddressDTO.cs`: `private byte addressaddresstype;` - `AddressFlatDTO.cs`: `private int addressaddresstype;` The same field (`ADDRESSTYPE`) is mapped to `byte` in one DTO and `int` in another. Depending on which DTO is used, the value is treated differently. If `ADDRESSTYPE` has a value > 255, one DTO would overflow. This inconsistency means code cannot safely switch between the two DTOs. --- ### LOW-34 — `GET_ADDRESS` Accepts `@languageid` Parameter But Never Uses It **File:** `FrameworkDAL/Query/Address/AddressQB.cs`, lines 11–79 ```sql FROM MADDRESS A ... WHERE A.ADDRESSID = @addressid; -- @languageid parameter is passed but not used in the WHERE clause ``` `AddressDAL.GetAddress` passes `languageid` as a parameter: ```csharp var parameters = new { addressid = AddressId, languageid = LanguageId }; ``` But `GET_ADDRESS` never uses `@languageid`. Dapper sends it as an unnecessary parameter. The actual language-based lookup is handled by `TRMAddressQB.GET_TRMADDRESS` via a separate path in `AddressBLL`. **Fix:** Remove `languageid` from the parameter bag in `AddressDAL.GetAddress`: ```csharp var parameters = new { AddressId, loginDTO.DatabaseName }; ``` --- ### LOW-35 — `AddressBLL.GetAddress` Has No Validation on `LanguageId` **File:** `FrameworkBLL/Address/AddressBLL.cs`, lines 31–38 ```csharp if (LanguageId == "EN") { return await _addressDAL.GetAddress(AddressId, LanguageId, LoginDTO); } else { return await _trmAddressDAL.GetTRMAddress(AddressId, LanguageId, LoginDTO); } ``` If `LanguageId` is `null`, empty, or a non-existent language code like `"XX"`, the method falls into the `else` branch and calls `GetTRMAddress`. For `null` or empty, this passes `null` to the SQL parameter `@languageId`, which in `TRMAddressQB.GET_TRMADDRESS` would produce an `INNER JOIN` match on `taddr.LANGUAGEID = NULL` — which always returns 0 rows in SQL (NULL ≠ NULL). The result is silently empty rather than an error. **Fix:** Validate `LanguageId` before branching: ```csharp if (string.IsNullOrWhiteSpace(LanguageId)) throw new ValidationException("LanguageId is required."); if (AddressId <= 0) throw new ValidationException("AddressId must be positive."); ``` --- ## Architectural Summary ### Cross-Module Impact of Identified Issues The Address module is consumed by 120+ files across 8 modules. Issues found here have cascading impact: | Issue | Modules Impacted | |-------|-----------------| | No tenant filter on queries | All modules (Accounts, MM, PayRoll, Logistics, CRM, Marketing) | | `AllowAnonymous` | All modules | | Hard DELETE no tenant filter | Accounts (party addresses), MM (supplier addresses), PayRoll (employee addresses) | | `"NONE"` default strings | All modules that print addresses (invoices, POs, payslips) | | `QuerySingleAsync` on multi-row result | Accounts (PartyBranch with billing + shipping), MM (vendor multi-site) | | `DateTime.Now` in audit fields | PayRoll (employee address audit trail) | ### Admin AddressContactSearch — Implementation Status All four components of the Admin AddressContactSearch feature are empty: | Component | Status | Access | |-----------|--------|--------| | `AddressContactSearchBLL` | Empty stub | `internal` — inaccessible | | `AddressContactSearchDAL` | Empty stub | `internal` — inaccessible | | `AddressContactSearchQB` | Empty stub | `internal` — inaccessible | | `AdminSL.GetAddressDetail` | Empty stub | No route registered | --- ## Priority Remediation Plan ### Immediate (before next release) 1. **CRITICAL-1**: Add `AND A.DATABASENAME = @DatabaseName` to ALL address SELECT queries. Pass `loginDTO.DatabaseName` as a parameter. 2. **CRITICAL-2**: Remove `AllowAnonymous()` from all 9 address/TRM endpoints. 3. **CRITICAL-3**: Add `AND DATABASENAME = @DatabaseName` to DELETE_ADDRESS and DELETE_TRMADDRESS. Consider switching to soft delete. 4. **CRITICAL-4**: Remove `WITH (NOLOCK)` from `GET_SELECTLIST_ADDRESS_NEW`. 5. **CRITICAL-5**: Add `AND addr.OBJECTID = @EmployeeId AND addr.DATABASENAME = @DatabaseName` to `GET_ADDRESS_DETAIL_FOR_FULL_EMPLOYEE`. 6. **CRITICAL-6**: Replace `ex.Message` with generic error; add `ILogger` to all BLL classes. 7. **CRITICAL-7**: Wrap the TRM + main address delete in a single DB transaction. Fix `TRMAddressDAL.DeleteTRMAddress` to treat 0 rows as success. ### Short-Term (next sprint) 8. **HIGH-8**: Add `CancellationToken` to all interfaces and propagate to `QueryAsync`. 9. **HIGH-9**: Replace `null!` with `new { LoginDTO.DatabaseName }` in `GetSelectListAddressNew`. 10. **HIGH-11**: Implement the count query branch in `GetSelectListAddress`. 11. **HIGH-12**: Change `QuerySingleAsync` to `QueryAsync` in `GetAddressDetail`. Return a list. 12. **HIGH-15**: Change `INNER JOIN MREGION` to `LEFT JOIN MREGION` in employee address query. 13. **HIGH-16**: Replace `DateTime.Now` with `DateTime.UtcNow` in `PayRollBLL.AddressBLL`. 14. **MEDIUM-28**: Fix `TRMAddressDAL.DeleteTRMAddress` to treat 0 affected rows as success. 15. **MEDIUM-29**: Replace `"NONE"` default strings in `AddressDTO` with `null` or `string.Empty`. ### Medium-Term (next quarter) 16. **MEDIUM-22**: Align `GetAddressDetail` cache key level and response level to `CLIENT_LEVEL`. 17. **MEDIUM-23**: Fix `GetTRMAddress` to either use the computed cache key or return `null` from `GetCacheKey`. 18. **HIGH-10**: Implement `CriteriaDTO` filtering in `GetSelectListAddress`. 19. **MEDIUM-18/19/20**: Consolidate `AddressDTO`, `AddressFlatDTO`, `AddressReportDTO` — use auto-properties. 20. **HIGH-13**: Create `AddressTranslationDTO` for TRM queries instead of mapping into the full `AddressDTO`. 21. **MEDIUM-21**: Convert `AddressPicklistDTO` from `struct` to `record`. 22. **HIGH-14**: Consolidate duplicated QB SQL into a single framework QB. 23. **LOW-30/31**: Change GetSelectList endpoints from `Post` to `Get`. 24. **MEDIUM-25**: Remove `using Google.Apis.Json` from `AddressDAL.cs`. 25. **MEDIUM-26**: Remove `finally { json = null; }` no-op blocks. --- ## Conclusion The Address module is one of the most security-critical components in the system — it stores statutory tax numbers, financial addresses, and personal contact data for every party, branch, and employee. The **complete absence of authentication and tenant filtering** across all endpoints is the most severe vulnerability in the entire codebase reviewed so far. A single unauthenticated HTTP call can enumerate, modify, or hard-delete any party's PAN, GSTIN, TAN, and residential address data across all tenants. The broken English-only delete path (MEDIUM-28 → CRITICAL-7), the mass employee address data leak (CRITICAL-5), and the hardcoded `"NONE"` values in printed documents (MEDIUM-29) are operational defects that need fixing alongside the security issues. Prioritise the 7 Critical fixes immediately, particularly CRITICAL-1 through CRITICAL-4 which together form the core multi-tenancy and authentication breach.