dohertj2/scadalink-design
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(security): resolve Security-009,010,011 — LDAP connection timeout, design-doc correction, security-path test coverage; Security-008 deferred

Browse Source
This commit is contained in:
Joseph Doherty
2026-05-16 22:24:03 -04:00
parent a9bd017c88
commit 84a696b0e4
5 changed files with 160 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
code-reviews/Security/findings.md
View File

@@ -8,7 +8,7 @@
| Last reviewed | 2026-05-16 |
| Reviewer | claude-agent |
| Commit reviewed | `9c60592` |
| Open findings | 4 |
| Open findings | 0 (1 deferred — Security-008) |
## Summary
@@ -315,7 +315,7 @@ Security-side defect — the reset-on-refresh bug — is fully fixed here. Regre
|--|--|
| Severity | Low |
| Category | Performance & resource management |
| Status | Open |
| Status | Deferred |
| Location | `src/ScadaLink.Security/RoleMapper.cs:25-48` |
**Description**
@@ -333,7 +333,18 @@ Add a repository method that loads scope rules for a set of mapping IDs in one q
**Resolution**
_Unresolved._
Deferred 2026-05-16 (commit `pending commit`). Finding confirmed accurate: `RoleMapper.MapGroupsToRolesAsync`
issues one `GetScopeRulesForMappingAsync` round-trip per matched Deployment mapping — a
genuine N+1 on the login / 15-minute-refresh path. However, the only correct fix
(a batch `GetScopeRulesForMappingsAsync(IEnumerable<int>)` repository method, or an
eager-load navigation property) requires changes to `ISecurityRepository`
(`src/ScadaLink.Commons`) and `SecurityRepository` (`src/ScadaLink.ConfigurationDatabase`).
Both are outside the Security module's permitted edit scope for this review pass, and the
existing `ISecurityRepository` surface offers no per-set scope-rule query, so the N+1
cannot be removed from within `RoleMapper.cs` alone. Severity is Low (bounded by the
number of site-scoped Deployment groups, typically small). Deferred to a cross-module
change that adds the batch repository method; `RoleMapper` should then resolve all scope
rules in a single call.
### Security-009 — CancellationToken not honored inside `Task.Run` LDAP calls
@@ -341,7 +352,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `src/ScadaLink.Security/LdapAuthService.cs:42`, `:46`, `:51`, `:56-57`, `:67-73`, `:135`, `:139-145` |
**Description**
@@ -361,7 +372,17 @@ work-item scheduling, or implement a timeout-with-disconnect fallback.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending commit`). Confirmed: the synchronous Novell LDAP
calls were wrapped in `Task.Run(..., ct)` where `ct` only prevents the work item from
starting and cannot interrupt an in-progress blocking `Connect`/`Bind`/`Search`, and no
network/operation timeout was configured on the `LdapConnection`. Added a configurable
`SecurityOptions.LdapConnectionTimeoutMs` (default 10s) and a new `ApplyConnectionTimeout`
helper that sets both `LdapConnection.ConnectionTimeout` (socket connect) and
`LdapConstraints.TimeLimit` (per-operation limit) before connecting, so a hung LDAP
server can no longer pin a thread-pool thread indefinitely. The `ct`-only-guards-scheduling
limitation is now documented in the option's XML doc and an inline comment. Regression
tests `SecurityOptions_LdapConnectionTimeout_HasSaneDefault` and
`AuthenticateAsync_UnreachableHost_FailsWithinConfiguredTimeout`.
### Security-010 — Design doc contradicts itself on Windows Integrated Authentication
@@ -369,7 +390,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Design-document adherence |
| Status | Open |
| Status | Resolved |
| Location | `docs/requirements/Component-Security.md:13` (vs. `:23`) |
**Description**
@@ -388,7 +409,14 @@ to match the implemented behavior and the rest of the document.
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending commit`). Confirmed: the Responsibilities bullet at
line 13 said "using Windows Integrated Authentication", directly contradicting the
Authentication section's "No Windows Integrated Authentication ... authenticates directly
against LDAP/AD, not via Kerberos/NTLM", CLAUDE.md, and the implementation (`LdapAuthService`
performs a direct username/password bind). Documentation-only finding — no regression test
is meaningful. Reworded the Responsibilities bullet to "Authenticate users against
LDAP/Active Directory using a direct LDAP/AD bind (username/password)", matching the rest
of the document and the code.
### Security-011 — Missing tests for security-critical paths
@@ -396,7 +424,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Testing coverage |
| Status | Open |
| Status | Resolved |
| Location | `tests/ScadaLink.Security.Tests/UnitTest1.cs` |
**Description**
@@ -417,4 +445,14 @@ DN-escaping of hostile usernames, and idle-timeout behavior across a refresh. Re
**Resolution**
_Unresolved._
Resolved 2026-05-16 (commit `pending commit`). Re-triage: most of the listed gaps were
already closed by the Security-001..007 fixes — the regression classes
`SecurityReviewRegressionTests` (StartTLS path, JWT empty/short-key rejection) and
`SecurityReviewRegressionTests2` (DN-injection / hostile-username escaping, `uid`/`cn`
fallback consistency, idle-timeout preserved across refresh) cover them. The finding's
reference to a `SecurityHardeningTests` class is stale — no such class exists in the
suite. Remaining gaps closed here: renamed the test file `UnitTest1.cs` →
`SecurityTests.cs`, and added a new `SecurityReviewRegressionTests3` class with the
LDAP-timeout coverage (Security-009) plus extra no-service-account / DN-path edge cases
(`BuildFallbackUserDn_NoSearchBase_ReturnsBareRdn`, `EscapeLdapDn_LeadingHash_IsEscaped`,
`EscapeLdapDn_NullOrEmpty_ReturnedUnchanged`). Full module suite: 54 tests, all green.