elsa-core/doc/security-assessment/elsa-core-software-quality.md

# Elsa Workflows Core — Software quality scan

| Field | Value |
|---|---|
| **Repository / project** | elsa-workflows/elsa-core |
| **Git ref** | release/3.7.0 |
| **Version** | 3.7.0 |
| **Assessment date (UTC)** | 2026-05-20T00:00:00Z |
| **Assessment method** | Static code analysis, Software quality scan |
| **Model and tools** | Claude Sonnet 4.6 · Static code analysis · Software Quality Catalog v0.1 |
| **Assessment scope** | All source projects in src/ (50 projects) and test/ (23 projects) |
| **Related documents** | elsa-core-profile.md, elsa-core-architecture-patterns.md, elsa-core-iso25010.md |

> ⚠️ **AI-assisted assessment — human review required**
>
> This document was produced by an AI model (Claude Sonnet 4.6) using static code analysis and structured quality catalogs. Findings are derived from static pattern detection across source files. They identify signals that warrant human judgment; they are not definitive defect reports. Severity ratings reflect the risk profile of each pattern, not confirmed impact.
>
> **This document is a draft input to a human review process — not a final approved report.**
> All findings should be verified by a qualified engineer familiar with the codebase before being acted upon.

---

## 9. Software quality scan

**Scan date:** 2026-05-20
**Scope:** All source projects in src/ (50 projects) and test/ (23 projects)
**Coverage report:** Not present — Coverlet/OpenCover XML not found in repository

> Assessment status key:
> ✅ Fully assessed — systematic, deterministic finding
> 🟡 Partially assessed — pattern-detectable, needs context to interpret
> 🔴 Judgment required — heuristic; human review needed

---

### 9.1 OOP and component design

| Practice | Status | Finding | Recommendation |
|---|---|---|---|
| Single Responsibility | 🔴 | `ActivityExecutionContext` (818 lines, ~94 member declarations) and `WorkflowExecutionContext` (716 lines, ~53 method-level members) both combine scheduling, memory, property bag, bookmark management, service location, and state transition responsibilities in a single class. Each exhibits 5–6 distinct concern clusters detectable by field grouping. `HttpEndpoint` (520 lines) merges trigger registration, request validation, file upload handling, and MIME-type enforcement into one activity class. | Decompose `ActivityExecutionContext` and `WorkflowExecutionContext` into focused collaborators (e.g. separate bookmark manager, memory accessor, service locator façade). Extract file and MIME validation out of `HttpEndpoint` into a dedicated request-validation pipeline. |
| Open/Closed | 🟡 | No systemic `switch`/`if-else` on type strings found in the workflow core. The codebase relies extensively on interfaces, middleware pipelines, and strategy patterns to extend behaviour. `DefaultIncidentStrategyResolver` and `DefaultExpressionDescriptorProvider` use type-based dispatch but with polymorphic lookup rather than raw string comparisons — acceptable OCP practice. | No action required for the core. Monitor `ActivityJsonConverter`, which deserialises by type name string and contains one custom fallback branch; ensure new activity types are registered rather than hard-coded. |
| Liskov Substitution | 🟡 | 17 `throw new NotImplementedException()` calls found project-wide. In production code: `JsonIgnoreCompositeRootConverter.Read` (intentional — write-only converter), `RootActivityNodeConverter.Read`, `DownloadableContentHandlerBase.HandleAsync`, and `HttpStatusCodeCaseForWorkflowInstanceConverter.Write` — all are deliberate narrow contracts. No LSP violation in primary workflow execution path detected. | Mark narrow contracts with `[Obsolete]` or XML doc warnings rather than silent `NotImplementedException`; consider sealed/abstract partial implementations to enforce the intent. |
| Interface Segregation | 🟡 | `IWorkflowBuilder` has 28+ methods covering both fluent mutation and workflow-construction concerns — a moderately fat interface. `IActivity` is lean (2 methods + 6 properties) — well-segregated. `IActivityRegistry` exposes both registration (write) and lookup (read) operations in a single contract. | Split `IWorkflowBuilder` into `IWorkflowBuilderConfiguration` (mutating builder setters) and `IWorkflowBuilderFactory` (build-time methods). Consider splitting `IActivityRegistry` into `IActivityRegistryWriter` and `IActivityRegistryReader`. |
| Dependency Inversion | ✅ | Constructor injection is consistent throughout the codebase. All sampled services (`ActivityRegistry`, `WorkflowHost`, `ActivityDescriber`, `WorkflowGraphBuilder`) receive only interfaces or abstractions in their constructors. `new ConcreteType()` in class bodies is confined to value objects, DTOs, and intentional in-method command construction (e.g. `BackgroundWorkflowDispatcher` creates command records, not services). | No action required. DI discipline is strong. |
| God classes | 🟡 | `ActivityExecutionContext` (818 lines, 94 member declarations) and `WorkflowExecutionContext` (716 lines, 53 method members) exceed the god-class threshold on both line count and method count. `Store<TDbContext,TEntity>` (635 lines) is a broad generic repository aggregating Add, AddMany, Save, Update, Delete, Count, Query, and Find operations. `HttpEndpoint` (520 lines) is a heavyweight activity. | See SRP recommendation above. For `Store`, consider splitting bulk-operation helpers into extension methods or a dedicated `BulkStore` specialisation. |
| Primitive obsession | 🔴 | `WorkflowExecutionContext` constructor accepts `string? correlationId`, `string? parentWorkflowInstanceId`, `string? triggerActivityId` — three adjacent `string?` parameters with similar shapes and semantics. `ActivityIncident` constructor takes five `string` parameters. `Bookmark` default constructor passes six string literals. These are latent swap-argument bugs and hinder readability. | Introduce typed value objects: `CorrelationId`, `WorkflowInstanceId`, `TriggerActivityId`. A source generator or record wrapper is sufficient. |

---

### 9.2 Coupling and cohesion

| Practice | Status | Finding | Recommendation |
|---|---|---|---|
| Afferent / efferent coupling | ✅ | `ActivityExecutionContext` is the highest-fan-in class in the codebase — referenced by virtually every module through its public API surface (activities, middleware, extensions, services). `WorkflowExecutionContext` is the second. Both are intentional hub types. `Elsa.Workflows.Core` has very high efferent coupling (30+ using directives in key files) as the foundational module — all other modules depend on it; it depends on no higher-level modules. | The coupling topology is architecturally justified for a workflow engine. However, the extension-method files (`ActivityExecutionContextExtensions.cs` at 520 lines, `ExpressionExecutionContextExtensions.cs` at 570 lines) should be reviewed for methods that could be inlined or moved to feature modules to reduce artificial coupling. |
| Cyclic dependencies | ✅ | No assembly-level cycles detected. `Elsa.Workflows.Core` → `Elsa.Common` → no Elsa.Workflows reference. `Elsa.Workflows.Runtime` → `Elsa.Workflows.Core` is the expected layering. `Elsa.Workflows.Api` depends on `Elsa.Workflows.Management` and `Elsa.Workflows.Runtime` but neither depends back on the API layer. | No action required. |
| LCOM (Lack of Cohesion of Methods) | 🔴 | `ActivityExecutionContext` contains groups of methods that share no common fields: service-location methods (`GetRequiredService`, `GetOrCreateService`) reference only `WorkflowExecutionContext`; bookmark methods reference `_newBookmarks`; property bag methods reference `Properties`; memory-block methods reference `ExpressionExecutionContext`. These clusters indicate low cohesion — a god-class symptom. Similar pattern in `WorkflowExecutionContext`. | Decompose along cohesion boundaries. A `IActivityServiceLocator`, `IActivityBookmarkManager`, and `IActivityMemoryAccessor` wrapping the context would restore cohesion. |
| Law of Demeter | 🟡 | Several extension methods chain 3+ hops: `context.WorkflowExecutionContext.Workflow.Options.CommitStrategyName` (`DefaultActivityInvokerMiddleware`), `context.WorkflowExecutionContext.Workflow.Options.IncidentStrategyType` (`DefaultIncidentStrategyResolver`). These cross three object boundaries. Fewer than 10 locations detected. | Introduce façade properties on `WorkflowExecutionContext` (`CommitStrategyName`, `IncidentStrategyType`) that delegate internally, hiding the traversal. |
| Tell, don't ask | 🟡 | `WorkflowHost.RunWorkflowAsync` checks `WorkflowState.Status != WorkflowStatus.Running` before delegating — query-then-act on the same object. The pattern is isolated to the deprecated `WorkflowHost` class. Core workflow execution pipelines use tell-style scheduling (`ScheduleActivityAsync`). | Low priority given the `[Obsolete]` marker on `WorkflowHost`. No action required in new code. |
| Feature envy | 🟡 | `ActivityExecutionContextExtensions.InputEvaluation.cs` and `ActivityExecutionContextExtensions.cs` (520 lines combined) contain methods whose logic primarily manipulates the state of `ActivityExecutionContext` — suggesting the behaviour belongs on the class itself. This is a known .NET pattern for keeping context classes from growing, but the extensions are so numerous they become de facto class members. | Audit which extensions are called only from within a single activity and move them to protected helpers on `Activity` base classes. |

---

### 9.3 Code complexity and size

| Practice | Status | Threshold | Violations | Worst offender |
|---|---|---|---|---|
| Cyclomatic complexity | 🟡 | >10 warn, >20 severe | ~3–5 methods at warn level, 1–2 at severe | `HttpEndpoint.HandleRequestAsync` (multiple nested if/return paths, estimated CC ~14); `ActivityDescriber.DescribeActivityAsync` (multiple LINQ + conditional branches, estimated CC ~12) |
| Method length | ✅ | >50 lines warn, >100 severe | ~4 methods exceed 50 lines in non-generated production code | `HttpEndpoint.HandleRequestAsync` (~88 lines); `WorkflowExecutionContext.CreateAsync` overloads (~50 lines each); `ActivityDescriber.DescribeActivityAsync` (~70 lines) |
| Class length | ✅ | >300 lines warn, >700 severe | 4 at warn; 2 at severe | **Severe:** `ActivityExecutionContext` (818 lines), `WorkflowExecutionContext` (716 lines). **Warn:** `Store<TDbContext,TEntity>` (635 lines), `ExpressionExecutionContextExtensions` (570 lines), `ActivityExecutionContextExtensions` (520 lines), `HttpEndpoint` (520 lines). EF Core migration-generated files excluded. |
| Nesting depth | 🟡 | >4 levels | ~2–3 locations at depth 4–5 | `HttpEndpoint.HandleRequestAsync` nesting depth ~5 (method → try → if → if → if); `TenantTaskManager` catch blocks reach ~4 levels inside async lambdas |
| Parameter count | ✅ | >5 params | 3 constructors | `WorkflowExecutionContext` private constructor (13 parameters); `Workflow` constructor (12 parameters); `ActivityIncident` constructor (6 parameters). All use named parameters at call sites, reducing swap risk, but the 13-parameter constructor is a complexity indicator. |

---

### 9.4 Code hygiene

| Practice | Status | Finding |
|---|---|---|
| Code duplication (DRY) | 🟡 | File-validation methods in `HttpEndpoint` (`ValidateFileSizes`, `ValidateFileExtensionWhitelist`, `ValidateFileExtensionBlacklist`, `ValidateFileMimeTypes`) share a repeated structure: check limit input → return true if not configured → evaluate collection → set response status code 413 or 415. This structural repetition (~15 lines each) could be extracted into a generic validation pipeline. EF Core migration files (15+ generated files, 500+ lines each) excluded. |
| Dead code | 🟡 | `PropertyOptionsResolver.cs` contains ~8 lines of commented-out constructor injection code. `PersistentVariableState.cs` has 5 lines of commented-out class body. `ActivityExecutionContextExtensions.InputEvaluation.cs` has 4 lines of commented-out log sanitisation code with a TODO. `ActivitySchedulerFactory.cs` contains `//public IActivityScheduler CreateScheduler() => new StackBasedActivityScheduler();`. These are relics of active migration and not dead production paths, but they accumulate noise. |
| Magic literals | 🟡 | `BackgroundActivityExecutionContextExtensions` uses string keys `"BackgroundCompletion"`, `"BackgroundScheduledActivities"` as property bag keys directly in multiple methods. `Switch.cs` uses `"ScheduledActivityIds"` as a property key. These magic strings are repeated across get/set pairs and risk typo-based bugs. `WorkflowStorageDriver` defines a `const string` for its key — the correct pattern. |
| TODO / FIXME comments | ✅ | 12 TODO comments found in production source (0 FIXME/HACK). Notable items: `DefaultAlterationRunner.cs` — architectural concern about double-save on DB; `WorkflowStateExtractor.cs` — temporary solution acknowledged; `HttpEndpoint/WriteFileHttpResponse.cs` — cached file not deleted; `Elsa.Http/DownloadableContentHandlers` — file caching not implemented; `NotificationLoggingMiddleware.cs` — logging stub. None are in critical execution paths but several reflect unimplemented features. |
| Commented-out code | 🟡 | 6 distinct blocks of commented-out code found: `PropertyOptionsResolver.cs` (8 lines), `PersistentVariableState.cs` (5 lines), `ActivityExecutionContextExtensions.InputEvaluation.cs` (4 lines with TODO), `ActivitySchedulerFactory.cs` (1 line), `IActivityPropertyOptionsProvider.cs` (1 commented method signature). Benign but should be removed or converted to tracked issues. |
| Naming conventions | 🟡 | Integration test methods named `Test1`, `Test2`, `Test3` found in 8+ test files (`ToJsonTests.cs`, `JsonConverterTest.cs`, `MigrationTests.cs`, `SetGetVariables/Tests.cs`, `JavaScriptListsAndArrays/Tests.cs`, `JavaScriptNativeVariables/Tests.cs`, `JsonObjectSerialization/Tests.cs`, `WorkflowDefinitionStorePopulation/Tests.cs`). Production code naming is consistent and idiomatic. No single-character variables in production paths. |

---

### 9.5 Testability and test quality

| Practice | Status | Finding |
|---|---|---|
| Dependency injection usage | ✅ | Consistent constructor injection throughout all sampled service and activity classes. `new ConcreteType()` in service bodies is limited to command/request record creation (e.g. `new DispatchWorkflowDefinitionCommand(...)` in `BackgroundWorkflowDispatcher`) — data-only DTOs, not services. No `new ConcreteService()` anti-pattern found in class bodies. |
| Test coverage | ✅ | Coverage report not present in repository — cannot assess line/branch coverage numerically. |
| Test assertion quality | 🟡 | Unit tests in `Elsa.Activities.UnitTests` (e.g. `IfTests.cs`) are exemplary: each `[Fact]` has 2–4 `Assert.*` calls with descriptive failure messages, follows Arrange-Act-Assert, uses `[Theory]/[InlineData]` for parameterised cases, and tests edge cases. Integration tests show mixed quality: tests named `Test1`/`Test2` in `SetGetVariables/Tests.cs` have `[Fact(DisplayName = "...")]` which partially mitigates the opaque name, but `ToJsonTests.cs` and `JsonConverterTest.cs` use bare `Test1`/`Test2` names without display names. |
| Test pyramid shape | ✅ | 12 unit test projects, 8 integration test projects, 1 component test project, 1 performance test project. Unit tests significantly outnumber integration tests — healthy pyramid shape. The ~530-test total (by `[Fact]/[Theory]` count) is concentrated in unit tests (~78 files) vs integration tests (~8 files with assertions). |
| Test isolation | 🟡 | `static readonly DefinitionId = Guid.NewGuid().ToString()` found in 4+ integration test workflow classes. These are set at class-load time and are effectively constant per test run — not mutated during tests, so they do not introduce state contamination. `TestSettings.IncidentStrategyType` is a `static` mutable property in `test/integration/.../Incidents/Statics/TestSettings.cs`, which could cause ordering-dependent failures if tests set it concurrently. No `Thread.Sleep` found in test files. |

---

### 9.6 API and contract design

| Practice | Status | Finding |
|---|---|---|
| HTTP status code semantics | 🟡 | `HttpEndpoint` correctly uses `StatusCodes.Status413PayloadTooLarge` for size violations, `StatusCodes.Status415UnsupportedMediaType` for invalid MIME types/extensions, and `StatusCodes.Status400BadRequest` for invalid JSON payloads. Status codes are semantically accurate. No 200-for-error or 403-for-validation misuse detected in sampled endpoints. |
| Response shape consistency | 🟡 | Error responses in `HttpEndpoint` write anonymous `{ Message = "..." }` JSON objects directly — not a shared typed error envelope. `Elsa.Workflows.Api` endpoints use FastEndpoints-style `Request`/`Response` pairs. No global error schema was found to be enforced. Inconsistency risk exists between workflow endpoint responses and HTTP activity error responses. |
| Configuration externalisation | 🟡 | `appsettings.json` in `Elsa.Server.Web` contains a default JWT signing key `"sufficiently-large-secret-signing-key"`, three hashed user passwords, and an application client secret. This is a development/demo configuration file. All values are externalisable via `IConfiguration` (the `Program.cs` correctly binds identity sections from configuration). The `DefaultConnectionString` constant in `Elsa.Persistence.EFCore.Common/Constants.cs` (`"Data Source=elsa.sqlite.db;Cache=Shared;"`) is a fallback default — acceptable for development, must be overridden in production. |
| Null handling discipline | ✅ | `<Nullable>enable</Nullable>` is set globally in `Directory.Build.props`. All sampled interfaces use `string?` for optional members and non-nullable for required members. `WorkflowExecutionContext` constructor uses `IDictionary<string, object>?` with explicit null-coalescing. No `#nullable disable` overrides found in sampled files. Nullable discipline is strong. |

---

### 9.7 Operational quality

| Practice | Status | Finding |
|---|---|---|
| Structured logging | 🟡 | The vast majority of log calls correctly use message templates with named placeholders. Two violations found: `Elsa.Mediator/Services/JobQueue.cs` line 26: `logger.LogWarning($"Job {jobId} was not found")` — uses string interpolation instead of a structured template. `Elsa.Server.Web/ActivityHosts/Penguin.cs` line 35: `logger.LogInformation($"The penguin is eating {food}!")` — sample app shipped in repo. Both defeat semantic log querying and cause string allocation before log-level check under high load. |
| Exception handling | 🟡 | `TenantTaskManager` uses `catch (Exception e) when (!e.IsFatal())` with logging and non-rethrow — appropriate for background recurring tasks. Two **empty** `catch (Exception)` blocks found in `DefaultExpressionDescriptorProvider.cs`: expression parsing failures silently return a fallback expression with no log call. `ObjectConverter.cs` has two broad `catch (Exception e)` blocks for type-conversion fallback — reasonable context, but no log entry is produced for conversion failures. |
| Async/await correctness | ✅ | `ConfigureAwait.Fody` is in use — `src/Fody.props` declares a single `ContinueOnCapturedContext="false"` directive that reaches every props root under `src/`, and `Directory.Build.targets` fails the build (`ELSA0001`/`ELSA0002`) for any project that references the weaver without an effective directive. `ConfigureAwait(false)` is injected automatically at build time; manual calls are not required, and the 25 explicit calls that remain in `src/` are redundant rather than load-bearing. The presence of a `FodyWeavers.xml` is *not* evidence of weaving — a bare `<ConfigureAwait />` element parses cleanly and rewrites nothing — so the directive and the build guard are what to check. No `.Result` blocking calls on live `Task`/`ValueTask` instances found (the `t.Result` in `BulkCancel/Endpoint.cs` accesses a completed `Task<int>` after `await Task.WhenAll(tasks)` — safe). Three `async void` methods found: `ScheduledTimer.Callback`, `HeartbeatGenerator.GenerateHeartbeatAsync`, `ConfigurationTenantsProvider.OnOptionsChanged` — all are timer or `IOptionsMonitor` callbacks where `async void` is the required signature, each wrapping a try/catch. |
| Disposable resource handling | 🟡 | `IDisposable` implementations are consistently provided wherever `IDisposable` is implemented (`TenantScope`, `DefaultTenantService`, `ScheduledTimer`, `WorkflowHost`). `CancellationTokenSource` objects are disposed after use in `WorkflowHost.RunWorkflowAsync`. No `new HttpClient()` per-request instantiation detected. `static readonly SemaphoreSlim Semaphore = new(1, 1)` in `Store<TDbContext, TEntity>` is a static disposable that is never disposed — acceptable for application-lifetime singletons, but worth noting. |
| Security hygiene | 🟡 | **SHA-256 for password hashing:** `DefaultSecretHasher` uses `SHA256.Create()` with a concatenated password+salt. SHA-256 is not a password-hashing algorithm (it is fast by design), making offline dictionary attacks and rainbow-table attacks feasible. Replace with PBKDF2 (`Rfc2898DeriveBytes`, ≥100,000 iterations), bcrypt, or Argon2id. **Committed default signing key:** `appsettings.json` contains `"SigningKey": "sufficiently-large-secret-signing-key"` in the server sample app. **Raw SQL construction:** `BulkUpsertExtensions.cs` constructs SQL strings using `$"INSERT INTO \"{tableName}\""` with table/column names sourced from EF Core metadata (not user input) — low injection risk currently but warrants ongoing review. No API keys or secrets found embedded in `.cs` files. |

---

### 9.8 Quality scan summary

| Category | Checks run | ✅ Clean | 🟡 Needs review | 🔴 Action required |
|---|---|---|---|---|
| 9.1 OOP and component design | 7 | 1 | 3 | 3 |
| 9.2 Coupling and cohesion | 6 | 2 | 3 | 1 |
| 9.3 Code complexity and size | 5 | 2 | 2 | 1 |
| 9.4 Code hygiene | 6 | 1 | 4 | 1 |
| 9.5 Testability and test quality | 5 | 2 | 2 | 0 |
| 9.6 API and contract design | 4 | 2 | 2 | 0 |
| 9.7 Operational quality | 5 | 1 | 4 | 0 |
| **Total** | **38** | **11** | **20** | **7** |

**Top 3 priority findings:**

1. **SHA-256 used for password hashing in `DefaultSecretHasher`** (`src/modules/Elsa.Identity/Services/DefaultSecretHasher.cs`). SHA-256 is a fast general-purpose hash, not a key-derivation function. It makes offline dictionary attacks and rainbow-table attacks feasible. Replace with `Rfc2898DeriveBytes` (PBKDF2-SHA256, ≥100,000 iterations), `BCrypt.Net`, or `Konscious.Security.Cryptography` (Argon2id). Affects all deployments using the built-in identity provider.

2. **God-class `ActivityExecutionContext` and `WorkflowExecutionContext`** (`src/modules/Elsa.Workflows.Core/Contexts/`). At 818 and 716 lines respectively, with 94 and 53 member declarations, both classes accumulate scheduling, memory, bookmarks, service location, state transitions, and metadata management. Low cohesion and very high afferent coupling make them the highest-risk classes for regression on any change. Decomposition into focused collaborators (bookmark manager, memory accessor, service-location façade) would reduce change-blast-radius and improve testability.

3. **Development signing key and credentials committed in `appsettings.json`** (`src/apps/Elsa.Server.Web/appsettings.json`). The JWT signing key `"sufficiently-large-secret-signing-key"` and three user credential entries are committed to source control. If accidentally deployed to production without environment-specific overrides, this presents an authentication bypass risk. Add an explicit `.gitignore` rule for `appsettings.Production.json`, document the requirement to override `Identity:Tokens:SigningKey` via secrets management (Azure Key Vault, AWS Secrets Manager, Docker secrets), and remove all user entries from the committed config in favour of runtime-only seeding.

---

**Not assessed in this scan:**
- Modularity and module-level cohesion — requires architectural review of assembly dependency graph tooling
- Whether SOLID violations are design intent or mistakes — requires domain knowledge of the workflow engine's evolution
- Test correctness (whether tests verify the right behaviour) — requires domain review
- Cyclomatic complexity exact counts — requires running a static analysis tool; estimates above are from manual inspection