# GB5Shared Libraries — Deep Code Analysis

**Date:** 2026-03-02
**Path:** `/Users/venkatv/gb/gb5-dev/GB5Shared/`
**Files Analyzed:** Connection, QueryExecutor, EntityHandler, PubSub/OutBox, Validation, WorkFlowEngine, WorkFlowRunTime, DaprCache, GB5CommonFunction, EventLogPublish, GB5Shared.csproj

---

## 1. CONNECTION LAYER — `ApplicationConnection.cs`

### 1.1 [SECURITY] Hardcoded Database Password — CRITICAL

**Lines 291, 604**

```csharp
// Line 604
"Password=" + "devuser@123"

// Line 291 — in GetConnectionStringAsync
"Password=devuser@123"
```

**Impact:** Database password visible in source code. Anyone with repo access can directly connect to the production database, bypassing all application-level security.

**Fix:**
```csharp
// Load from configuration, never hardcode
var password = _configuration["Database:Password"]
    ?? Environment.GetEnvironmentVariable("DB_PASSWORD")
    ?? throw new InvalidOperationException("Database password not configured");
```

### 1.2 [SECURITY] Connection String by String Concatenation — MEDIUM

**Lines 422–425, 437–445, 473–476, 488–495**

```csharp
$"Server={server};Database={db};User Id={user};Password={password};"
```

**Impact:** If any parameter contains special characters (`;`, `=`), the connection string can be manipulated (connection string injection). Also makes it impossible to validate individual components.

**Fix:** Use dedicated builders:
```csharp
// SQL Server
var csb = new SqlConnectionStringBuilder
{
    DataSource = server,
    InitialCatalog = db,
    UserID = user,
    Password = password,
    Encrypt = true,
    TrustServerCertificate = false
};
```

### 1.3 [BEST-PRACTICE] Empty Finally Blocks — LOW

**Lines 67–70, 92–95**

```csharp
finally
{
    // empty
}
```

**Fix:** Remove empty finally blocks.

### 1.4 [BEST-PRACTICE] Unused JSON Serialization — LOW

**Lines 154, 214**

```csharp
string json = JsonConvert.SerializeObject(result);  // result never used
```

**Fix:** Remove dead code.

---

## 2. QUERY EXECUTOR — `QueryExecutor.cs`

### 2.1 [SECURITY] SQL Injection via String Interpolation — CRITICAL

**Lines 682, 687**

```csharp
string sql = $"UPDATE {tableName} SET STATUS = 1 WHERE ID = @Id";
// tableName comes from workflow engine, not validated
```

**Impact:** If `tableName` is user-influenced, full SQL injection. Classic example: `tableName = "users; DROP TABLE users;--"`

**Fix:**
```csharp
// Option A: Validate against a known whitelist
private static readonly HashSet<string> AllowedTables = new(StringComparer.OrdinalIgnoreCase)
{
    "WorkflowInstance", "WorkflowTask", "OutboxMessage"
};

if (!AllowedTables.Contains(tableName))
    throw new ArgumentException($"Table name '{tableName}' is not allowed");

// Option B: Use schema information tables to validate
var tableExists = await _db.ExecuteScalarAsync<int>(
    "SELECT COUNT(1) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = @TableName",
    new { TableName = tableName });
if (tableExists == 0) throw new ArgumentException("Unknown table");
```

### 2.2 [MEMORY] Missing Transaction Propagation — HIGH

**Line 39**

```csharp
// PublishEventAsync called without transaction, but accepts one
await PublishEventAsync(login, eventData);  // transaction parameter not passed
```

**Impact:** Event published outside database transaction. If the outer transaction rolls back, the event is already published — downstream consumers process an event for data that doesn't exist.

**Fix:** Always pass the transaction:
```csharp
await PublishEventAsync(login, eventData, currentTransaction);
```

### 2.3 [PERFORMANCE] N+1 Queries in WorkFlow Processing — HIGH

**Lines 640–660**

```csharp
foreach (var workflow in workflows)
{
    // Separate DB call for each workflow item
    await ProcessWorkflowItem(workflow, login);
}
```

**Impact:** 50 workflows = 50+ separate database roundtrips. At 10ms per roundtrip = 500ms per batch.

**Fix:**
```csharp
// Batch load all related data in one query
var workflowIds = workflows.Select(w => w.Id).ToArray();
var relatedData = await _db.QueryAsync<WorkflowRelatedDTO>(
    "SELECT * FROM WorkflowRelated WHERE WorkflowId = ANY(@Ids)",
    new { Ids = workflowIds });

var relatedByWorkflow = relatedData.GroupBy(r => r.WorkflowId)
    .ToDictionary(g => g.Key, g => g.ToList());

foreach (var workflow in workflows)
{
    var related = relatedByWorkflow.GetValueOrDefault(workflow.Id, new List<WorkflowRelatedDTO>());
    await ProcessWorkflowItemBatch(workflow, related, login);
}
```

### 2.4 [PERFORMANCE] Hardcoded CommandTimeout — MEDIUM

**Line 307**

```csharp
commandTimeout: 400  // 400 seconds hardcoded
```

**Impact:** Long-running queries block thread for 400 seconds. Different operations need different timeouts (OLTP 30s, reports 300s). Cannot tune without code change.

**Fix:**
```csharp
// Inject from configuration
private readonly int _defaultCommandTimeout;
public QueryExecutor(IConfiguration config)
{
    _defaultCommandTimeout = config.GetValue<int>("Database:CommandTimeoutSeconds", 30);
}
```

### 2.5 [BEST-PRACTICE] SessionQueryAsync Uses HttpContext Items for State — MEDIUM

**Lines 522–527, 578–583**

```csharp
var connection = HttpContext.Items["DbConnection"] as DbConnection;
var transaction = HttpContext.Items["DbTransaction"] as DbTransaction;
```

**Impact:** Not suitable for Dapr actor/background scenarios where there is no HttpContext. State is lost when requests span async boundaries or background workers.

**Fix:** Use explicit parameter passing instead of ambient context:
```csharp
public async Task<IEnumerable<T>> QueryAsync<T>(
    LoginDTO login, string sql, object? param = null,
    DbConnection? connection = null, DbTransaction? transaction = null)
```

---

## 3. OUTBOX / PUBSUB — `OutBox.cs`

### 3.1 [BUG] Transaction Parameter Ignored — CRITICAL

**Line 30**

```csharp
// Method signature accepts transaction
public async Task PublishEventAsync(LoginDTO login, EventDTO eventData, DbTransaction Trans)
{
    // ... but Trans is NEVER used
    await _queryExecutor.ExecuteAsync(login, insertSql, parameters);
    // Should be:
    // await _queryExecutor.ExecuteAsync(login, insertSql, parameters, Trans);
}
```

**Impact:** Events are inserted into the outbox table OUTSIDE the caller's transaction. If the caller rolls back, the event still exists in the outbox and will be published — creating phantom events for data that was never committed.

**Fix:**
```csharp
public async Task PublishEventAsync(LoginDTO login, EventDTO eventData, DbTransaction Trans)
{
    var parameters = new { /* ... */ };
    await _queryExecutor.ExecuteAsync(login, insertSql, parameters, Trans); // Pass Trans!
}
```

### 3.2 [BEST-PRACTICE] Silent Exception Swallowing — HIGH

**Lines 98–107**

```csharp
catch (Exception)
{
    // Mark as FAILED silently — no logging
    await _queryExecutor.ExecuteAsync(login, markFailedSql, new { Id = msg.Id });
}
```

**Impact:** Event publishing failures completely invisible. No way to know events are failing, no alert, no metrics.

**Fix:**
```csharp
catch (Exception ex)
{
    _logger.LogError(ex, "Failed to publish outbox event {EventId} of type {EventType}",
        msg.Id, msg.EventType);
    await _queryExecutor.ExecuteAsync(login, markFailedSql, new { Id = msg.Id });
    // Increment failure counter for alerting
    _metrics.OutboxPublishFailures.Add(1);
}
```

### 3.3 [BEST-PRACTICE] No Retry Logic for Failed Events — HIGH

**Lines 98–107**

Events go from PENDING → FAILED with no retry attempts.

**Fix:** Implement retry with attempt count:
```csharp
// Outbox table needs RetryCount column
const int MaxRetries = 3;

if (msg.RetryCount < MaxRetries)
{
    // Schedule for retry with exponential backoff
    var nextRetry = DateTime.UtcNow.AddSeconds(Math.Pow(2, msg.RetryCount) * 5);
    await _queryExecutor.ExecuteAsync(login,
        "UPDATE OutboxEvents SET RetryCount += 1, NextRetryAt = @NextRetry WHERE Id = @Id",
        new { Id = msg.Id, NextRetry = nextRetry });
}
else
{
    // Move to dead letter after max retries
    await _queryExecutor.ExecuteAsync(login,
        "UPDATE OutboxEvents SET Status = 'DEAD_LETTER' WHERE Id = @Id",
        new { Id = msg.Id });
    _logger.LogError("Outbox event {EventId} moved to dead letter after {MaxRetries} retries",
        msg.Id, MaxRetries);
}
```

### 3.4 [BEST-PRACTICE] No Idempotency Protection — MEDIUM

**Lines 84–88**

Same event can be published to Dapr twice if the status update fails after publish succeeds.

**Fix:** Use idempotency key or check for published state before publishing:
```csharp
// Before publishing, mark as IN_PROGRESS atomically
var updated = await _queryExecutor.ExecuteAsync(login,
    "UPDATE OutboxEvents SET Status = 'IN_PROGRESS', ProcessingStartedAt = @Now " +
    "WHERE Id = @Id AND Status = 'PENDING'",  // Atomic compare-and-swap
    new { Id = msg.Id, Now = DateTime.UtcNow });

if (updated == 0) return; // Another process picked it up

// Now publish safely
await _daprClient.PublishEventAsync(pubSubName, topicName, msg.Payload);
```

---

## 4. VALIDATION — `Validation.cs`

### 4.1 [PERFORMANCE] Separate DB Call Per Validated Field — HIGH

**Lines 115–131**

```csharp
public async Task<bool> MaxLength(string fieldName, string value, LoginDTO login)
{
    // Opens new database connection for EVERY field validation
    var maxLen = await GetColumnMaxLengthAsync(fieldName, login);
    return value.Length <= maxLen;
}
```

**Impact:** If a form has 20 fields to validate, this makes 20 separate database connections just for column metadata. Column metadata is static — it never changes at runtime.

**Fix:** Cache column metadata at startup:
```csharp
private readonly IMemoryCache _cache;

public async Task<bool> MaxLength(string fieldName, string value, LoginDTO login)
{
    var cacheKey = $"colmeta:{login.DatabaseName}:{fieldName}";
    var maxLen = await _cache.GetOrCreateAsync(cacheKey, async entry =>
    {
        entry.AbsoluteExpirationRelativeToNow = TimeSpan.FromHours(24);
        return await GetColumnMaxLengthAsync(fieldName, login);
    });
    return value.Length <= maxLen;
}
```

### 4.2 [PERFORMANCE] Async Anti-Pattern — Task.Run on Sync Code — MEDIUM

**Lines 40–52, 56–77, 134–142**

```csharp
public async Task<bool> NotNull(string value)
{
    return await Task.Run(() => !string.IsNullOrEmpty(value));
}
```

**Impact:** `Task.Run` queues work on ThreadPool thread — costs a context switch for a trivially fast string check. This is a well-known .NET async anti-pattern.

**Fix:** Make synchronous (no async needed):
```csharp
public bool NotNull(string? value) => !string.IsNullOrEmpty(value);
public bool NotEmpty(string? value) => !string.IsNullOrWhiteSpace(value);
```

---

## 5. WORKFLOW ENGINE — `WorkFlowEngine.cs`

### 5.1 [SECURITY] SQL Injection via Table Name — CRITICAL

**Lines 682, 687**

```csharp
$"SELECT COUNT(1) FROM {tableName} WHERE STATUS = @Status"
$"UPDATE {tableName} SET STATUS = 1 WHERE ID = @Id"
```

**Impact:** Direct string interpolation of table name — classic SQL injection vector.

**Fix:** See QueryExecutor Issue 2.1 for whitelist validation pattern.

### 5.2 [PERFORMANCE] N+1 Queries in AutoTransitionAsync — HIGH

**Lines 604–635**

```csharp
foreach (var transition in def.Transitions)
{
    // def.Steps.First() called in loop — O(n*m) scan
    var step = def.Steps.First(s => s.WorkflowDetailId == transition.FromStepId);
}
```

**Impact:** For 20 transitions and 50 steps, this is 1,000 iterations for every auto-transition check.

**Fix:**
```csharp
// Build lookup dictionary ONCE before loop
var stepById = def.Steps.ToDictionary(s => s.WorkflowDetailId);

foreach (var transition in def.Transitions)
{
    if (stepById.TryGetValue(transition.FromStepId, out var step))
    {
        // O(1) lookup instead of O(n) scan
    }
}
```

### 5.3 [BEST-PRACTICE] No Concurrency Protection for Workflow Actions — HIGH

**Lines 427–553**

```csharp
// HandleSingleAsync — no locking
var task = await GetWorkflowTask(taskId, login);
task.Status = "COMPLETED";
await UpdateWorkflowTask(task, login);
```

**Impact:** Two users clicking "Approve" simultaneously both read the task as PENDING, both mark it COMPLETED — double approval. For financial workflows this is dangerous.

**Fix:** Use optimistic locking or database-level locking:
```csharp
// Option A: Optimistic locking with row version
var updated = await _db.ExecuteAsync(
    "UPDATE WorkflowTasks SET Status = @NewStatus, ModifiedAt = @Now " +
    "WHERE Id = @Id AND Status = @ExpectedStatus AND RowVersion = @Version",
    new { Id = taskId, NewStatus = "COMPLETED", ExpectedStatus = "PENDING",
          Now = DateTime.UtcNow, Version = task.RowVersion });

if (updated == 0)
    throw new ConcurrencyException("Task was already actioned by another user");
```

### 5.4 [MEMORY] MemoryCache for Workflow Definitions Never Invalidated — MEDIUM

**Lines 29–30**

```csharp
private readonly IMemoryCache _workflowCache;
// 10-minute TTL, but never manually invalidated
```

**Impact:** If a workflow definition is modified, all running engine instances continue using stale cached version for up to 10 minutes. In a microservices deployment with multiple instances, some may have stale cache while others don't.

**Fix:** Add explicit cache invalidation on workflow save:
```csharp
// In workflow definition save handler
_workflowCache.Remove($"workflow:{workflowId}");
// Or: Use distributed cache (Redis) and publish invalidation event via Dapr
```

---

## 6. WORKFLOW RUNTIME — `WorkFlowRunTime.cs`

### 6.1 [PERFORMANCE] 3-Roundtrip Load for Each Workflow Definition — HIGH

**Lines 354–403**

```csharp
// Query 1: Load workflow header
var workflow = await _db.QuerySingleAsync<WorkflowDTO>(sql1, params1);
// Query 2: Load steps separately
var steps = await _db.QueryAsync<WorkflowStepDTO>(sql2, new { WorkflowId = workflow.Id });
// Query 3: Load transitions separately
var transitions = await _db.QueryAsync<WorkflowTransitionDTO>(sql3, new { WorkflowId = workflow.Id });
// ... then additional queries for groups/rules
```

**Impact:** 3-5 roundtrips per workflow definition load. At 10ms per roundtrip = 30–50ms minimum latency just for definition loading, before any actual work.

**Fix:** Load all in a single query with JOIN or use Dapper's multi-mapping:
```csharp
using var multi = await _db.QueryMultipleAsync(
    @"SELECT * FROM Workflows WHERE Id = @Id;
      SELECT * FROM WorkflowSteps WHERE WorkflowId = @Id;
      SELECT * FROM WorkflowTransitions WHERE WorkflowId = @Id;
      SELECT * FROM WorkflowRuleGroups WHERE WorkflowId = @Id;
      SELECT * FROM WorkflowRules WHERE WorkflowId = @Id;",
    new { Id = workflowId });

var workflow = await multi.ReadSingleAsync<WorkflowDTO>();
var steps = (await multi.ReadAsync<WorkflowStepDTO>()).ToList();
var transitions = (await multi.ReadAsync<WorkflowTransitionDTO>()).ToList();
// etc.
```

---

## 7. DAPR CACHE — `CacheKeyGeneration.cs`

### 7.1 [PERFORMANCE] SHA256 Hash of Full CriteriaDTO — MEDIUM

**Lines 89–100**

```csharp
string json = JsonConvert.SerializeObject(criteriaDTO);
using var sha256 = SHA256.Create();
byte[] hash = sha256.ComputeHash(Encoding.UTF8.GetBytes(json));
```

**Impact:** Serializing large `CriteriaDTO` objects to JSON on every cache key generation is expensive. This runs on every cacheable request.

**Fix:** Hash only the key fields, not the whole object:
```csharp
public string GenerateKey(CriteriaDTO criteria)
{
    // Only hash fields that affect query results
    var keyData = $"{criteria.EntityName}|{criteria.PageSize}|{criteria.PageNumber}|{criteria.SortBy}|{criteria.FilterHash}";
    return $"v1:{ComputeHash(keyData)}";  // v1: prefix for versioning
}
```

### 7.2 [BEST-PRACTICE] Hardcoded State Store Name — MEDIUM

**Lines 29, 34, 47 in `KeyInvalidate.cs`**

```csharp
await _daprClient.DeleteStateAsync("statestore", key);  // hardcoded
```

**Fix:**
```csharp
private readonly string _stateStoreName;
public KeyInvalidate(IConfiguration config)
{
    _stateStoreName = config["Dapr:StateStoreName"] ?? "statestore";
}
```

---

## 8. COMMON FUNCTIONS — `GB5CommonFunction.cs`

### 8.1 [BEST-PRACTICE] Fragile SQL Parsing via String Operations — HIGH

**Lines 64–118**

```csharp
// Parses SQL by splitting on newlines and searching for keywords
string[] lines = query.Split(Environment.NewLine.ToCharArray());
int groupByIndex = query.ToLower().IndexOf("group by");
int orderByIndex = query.ToLower().IndexOf("order by");
```

**Impact:**
- Case-sensitive after `.ToLower()` — inconsistent
- Breaks if keywords appear in string literals (`WHERE Name = 'group by'`)
- Doesn't handle subqueries containing GROUP BY
- Doesn't handle multi-line keywords

**Fix:** Use a proper SQL parser library (`SqlParser.Net`) or wrap in stored procedure that handles paging server-side.

### 8.2 [PERFORMANCE] Task.Run Wrapping Synchronous Code — MEDIUM

**Lines 120–130**

```csharp
public async Task<int> CountArrayAsync<T>(IEnumerable<T>? array)
{
    return await Task.Run(() => array?.Count() ?? 0);
}
```

**Impact:** Wastes a thread pool thread for a synchronous count operation.

**Fix:**
```csharp
public int CountArray<T>(IEnumerable<T>? array) => array?.Count() ?? 0;
```

---

## 9. EVENT LOG PUBLISH — `EventLogPublish.cs`

### 9.1 [PERFORMANCE] Reflection in Publish Hot Path — HIGH

**Lines 124–128**

```csharp
// Called on every event publish
var props = eventData.GetType().GetProperties();
foreach (var prop in props)
{
    dict[prop.Name] = prop.GetValue(eventData)?.ToString();
}
```

**Impact:** `Type.GetProperties()` is expensive — typically 10-50x slower than direct property access. When called on every event publish, this significantly impacts throughput.

**Fix:** Cache the property info:
```csharp
private static readonly ConcurrentDictionary<Type, PropertyInfo[]> _propCache = new();

private static PropertyInfo[] GetProperties(Type type)
    => _propCache.GetOrAdd(type, t => t.GetProperties());

// Usage:
var props = GetProperties(eventData.GetType());
```

### 9.2 [BEST-PRACTICE] No Retry Policy on Dapr Publish — HIGH

**Line 142**

```csharp
await _daprClient.PublishEventAsync(pubSubName, topicName, payload);
// No retry on transient failure
```

**Fix:** Use Polly:
```csharp
private static readonly AsyncRetryPolicy _retryPolicy = Policy
    .Handle<DaprException>()
    .WaitAndRetryAsync(3,
        retryAttempt => TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)));

await _retryPolicy.ExecuteAsync(() =>
    _daprClient.PublishEventAsync(pubSubName, topicName, payload));
```

### 9.3 [SECURITY] Full LoginDTO Serialized into Messages — MEDIUM

**Lines 66, 88, 136**

```csharp
// Entire LoginDTO included in Dapr event payload
await _daprClient.PublishEventAsync(pubSubName, topicName, new { Login = loginDTO, Data = data });
```

**Impact:** LoginDTO likely contains session tokens, connection strings, or sensitive claims. These get stored in RabbitMQ message queues, visible in Dapr dashboard, and logged by message brokers.

**Fix:**
```csharp
// Only include necessary identity context
var context = new EventContext
{
    UserId = loginDTO.UserId,
    RoleId = loginDTO.RoleId,
    TenantId = loginDTO.TenantId,
    // NOT: password, connection string, token
};
await _daprClient.PublishEventAsync(pubSubName, topicName, new { Context = context, Data = data });
```

---

## 10. DEPENDENCY ANALYSIS — `GB5Shared.csproj`

### 10.1 Potentially Misplaced Dependencies — MEDIUM

```xml
<PackageReference Include="PuppeteerSharp" Version="..." />   <!-- Browser automation -->
<PackageReference Include="itext7" Version="..." />           <!-- PDF generation -->
<PackageReference Include="QuestPDF" Version="..." />         <!-- PDF generation (duplicate?) -->
```

**Impact:** `PuppeteerSharp` (browser automation) and dual PDF libraries bloat the shared library, increasing container image size and dependency attack surface for every service.

**Recommendation:** Move to feature-specific projects that actually use them.

### 10.2 No FluentValidation Reference — MEDIUM

**Impact:** `Validation.cs` likely depends on FluentValidation but it's not in the csproj — implicit transitive dependency. Can break on package updates.

**Fix:** Add explicit `<PackageReference Include="FluentValidation" />`.

---

## Summary Table

| # | Category | Issue | File | Lines | Priority |
|---|----------|-------|------|-------|----------|
| 1 | SECURITY | Hardcoded DB password | ApplicationConnection.cs | 291, 604 | 🔴 P0 |
| 2 | SECURITY | Connection string concatenation | ApplicationConnection.cs | 422–495 | 🟡 P2 |
| 3 | BUG | OutBox ignores transaction parameter | OutBox.cs | 30 | 🔴 P0 |
| 4 | SECURITY | SQL injection via table name | WorkFlowEngine.cs | 682, 687 | 🔴 P0 |
| 5 | BEST-PRACTICE | Silent exception swallowing in OutBox | OutBox.cs | 98–107 | 🟠 P1 |
| 6 | BEST-PRACTICE | No retry logic for failed events | OutBox.cs | 98–107 | 🟠 P1 |
| 7 | BEST-PRACTICE | No idempotency check | OutBox.cs | 84–88 | 🟡 P2 |
| 8 | PERFORMANCE | Separate DB call per validated field | Validation.cs | 115–131 | 🟠 P1 |
| 9 | PERFORMANCE | Task.Run on sync code | Validation.cs, CommonFunction.cs | 40–52, 120–130 | 🟡 P2 |
| 10 | PERFORMANCE | N+1 queries in WorkFlow | WorkFlowEngine.cs | 604–635 | 🟠 P1 |
| 11 | PERFORMANCE | 3-roundtrip workflow definition load | WorkFlowRunTime.cs | 354–403 | 🟠 P1 |
| 12 | BEST-PRACTICE | No concurrency lock for workflow actions | WorkFlowEngine.cs | 427–553 | 🟠 P1 |
| 13 | MEMORY | MemoryCache never invalidated | WorkFlowEngine.cs | 29–30 | 🟡 P2 |
| 14 | PERFORMANCE | Reflection in event publish hot path | EventLogPublish.cs | 124–128 | 🟠 P1 |
| 15 | BEST-PRACTICE | No retry on Dapr publish | EventLogPublish.cs | 142 | 🟠 P1 |
| 16 | SECURITY | Full LoginDTO in event payloads | EventLogPublish.cs | 66, 88, 136 | 🟡 P2 |
| 17 | PERFORMANCE | SHA256 of full CriteriaDTO | CacheKeyGeneration.cs | 89–100 | 🟡 P2 |
| 18 | BEST-PRACTICE | Hardcoded state store name | KeyInvalidate.cs | 29, 34, 47 | 🟡 P2 |
| 19 | BEST-PRACTICE | Fragile SQL string parsing | GB5CommonFunction.cs | 64–118 | 🟡 P2 |
| 20 | PERF/MEMORY | N+1 transaction propagation missing | QueryExecutor.cs | 39 | 🟠 P1 |