dohertj2/scadaproj
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Mark all baseline code-review findings resolved

Browse Source 
All 35 findings fixed in 544a6dd and marked Status: Resolved with resolution
notes. README regenerated: 0 pending / 35 total across 6 libraries.
This commit is contained in:
Joseph Doherty
2026-06-01 11:22:37 -04:00
parent 544a6ddb77
commit ae0ccc9a3a
7 changed files with 112 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Auth/findings.md
+13 -13
View File
@@ -10,7 +10,7 @@
| Last reviewed | 2026-06-01 |
| Reviewer | Claude (automated baseline) |
| Commit reviewed | `5f75cd4` |
| Open findings | 6 |
| Open findings | 0 |
## Summary
@@ -52,7 +52,7 @@ hashing algorithm and the AspNetCore surface. The library is **not yet adopted**
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.AspNetCore/ServiceCollectionExtensions.cs:40` |
**Description**
@@ -89,7 +89,7 @@ already run.) Add a test asserting that building + starting the host fails for a
**Resolution**
_Unresolved._
Resolved in `544a6dd`, 2026-06-01 — `AddZbLdapAuth` now binds via `AddOptions<LdapOptions>().Bind(...).ValidateOnStart()` so the validator runs at host start, not on first login (test: `ServiceCollectionExtensionsTests.AddZbLdapAuth_StartingHost_FailsForInsecureConfig`, plus a `...SucceedsForSecureConfig` companion).
### Auth-002 — A failed `MarkUsedAsync` write turns a valid API key into a thrown exception
@@ -97,7 +97,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Error handling & resilience |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs:66` |
**Description**
@@ -121,7 +121,7 @@ throws and assert the result is still `Succeeded == true`.
**Resolution**
_Unresolved._
Resolved in `544a6dd`, 2026-06-01 — `VerifyAsync` now wraps the best-effort `MarkUsedAsync` write in try/catch, re-throwing only `OperationCanceledException` and swallowing all other failures so a valid key still authenticates (tests: `ApiKeyVerifierTests.VerifyAsync_ValidKey_MarkUsedThrows_StillSucceeds` and `..._MarkUsedThrowsOperationCanceled_Propagates`).
### Auth-003 — Corrupt `scopes`/`constraints` column throws `JsonException` through the verifier
@@ -129,7 +129,7 @@ _Unresolved._
|--|--|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/ScopeSerializer.cs:31`, `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Sqlite/SqliteApiKeyStore.cs:71` |
**Description**
@@ -153,7 +153,7 @@ documented exception) rather than an unhandled `JsonException`.
**Resolution**
_Unresolved._
Resolved in `544a6dd`, 2026-06-01 — `ScopeSerializer.Deserialize` now catches `JsonException` and fails closed to an empty scope set (policy (a)), so a corrupt `scopes`/`constraints` column degrades to a zero-scope identity instead of throwing through the store and verifier (tests: `SqliteApiKeyStoreTests.ScopeSerializer_DeserializeMalformed_ReturnsEmptySet_DoesNotThrow` (Theory) and `..._FindByKeyId_CorruptScopesColumn_ReturnsRecordWithEmptyScopes_DoesNotThrow`).
### Auth-004 — README misstates the hashing algorithm and the AspNetCore public surface
@@ -161,7 +161,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Documentation & XML docs |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/README.md:13`, `ZB.MOM.WW.Auth/README.md:14` |
**Description**
@@ -190,7 +190,7 @@ DI-method names with the shipped names.
**Resolution**
_Unresolved._
Resolved in `544a6dd`, 2026-06-01 — README package table corrected: "PBKDF2" → "pepper-keyed HMAC-SHA256" and the AspNetCore row rewritten to the real surface (`AddZbLdapAuth`, `ZbCookieDefaults.Apply`, `ZbClaimTypes`, LDAP-only, no API-key/cookie-middleware wiring), with the stale `ApiKeys` dependency dropped from that row to match the `.csproj` (test: doc-only).
### Auth-005 — `CreateKeyAsync` persists `KeyPrefix` as `prefix_keyId`, inconsistent with the read path
@@ -198,7 +198,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Correctness & logic bugs |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/Admin/ApiKeyAdminCommands.cs:104` |
**Description**
@@ -220,7 +220,7 @@ column, or document precisely what `KeyPrefix` is meant to contain on the `ApiKe
**Resolution**
_Unresolved._
Resolved in `544a6dd`, 2026-06-01 — `CreateKeyAsync` now persists `KeyPrefix = _options.TokenPrefix` (the bare prefix) instead of `prefix_keyId`, aligning the write path with the read/test paths (test: `ApiKeyAdminCommandsTests.CreateKey_PersistsBareTokenPrefix_NotPrefixUnderscoreKeyId`).
### Auth-006 — LDAP connection ignores `allowInsecure` and offers no TLS certificate-validation hook
@@ -228,7 +228,7 @@ _Unresolved._
|--|--|
| Severity | Low |
| Category | Security & secret handling |
| Status | Open |
| Status | Resolved |
| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs:16` |
**Description**
@@ -251,4 +251,4 @@ beyond the OS default, and document the default validation behavior.
**Resolution**
_Unresolved._
Resolved in `544a6dd`, 2026-06-01 — added an optional `LdapOptions.ServerCertificateValidationCallback` (BCL `RemoteCertificateValidationCallback`, no new dependency) threaded through the `ILdapConnection.Connect` seam; `NovellLdapConnection` now builds `LdapConnectionOptions.ConfigureRemoteCertificateValidationCallback(...)` (a supplied callback wins; otherwise the previously-dead `allowInsecure` gates a dev-only accept-any path; otherwise OS-trust-store default), and the default validation behaviour is documented in XML/README (tests: `LdapAuthServiceTests.Connect_ReceivesConfiguredCertValidationCallback`, `..._Connect_ReceivesAllowInsecureFlag_FromOptions`, `..._Connect_NoCertCallbackConfigured_PassesNull`).