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

fix(configuration): resolve High code-review findings (Configuration-001, Configuration-008)

Browse Source 
Configuration-001: wrap the EXEC dbo.sp_ValidateDraft call in
sp_PublishGeneration in a BEGIN TRY/CATCH ROLLBACK; THROW block so a
validation RAISERROR aborts the publish instead of being ignored.

Configuration-008: route caller-supplied strings interpolated into
ConfigAuditLog.DetailsJson through STRING_ESCAPE(@x, 'json') and emit
sp_RollbackToGeneration's @TargetGenerationId as a bare JSON number,
closing the JSON-injection / denial-of-operation vector.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 06:12:00 -04:00
parent adf794f791
commit ee51878c08
3 changed files with 115 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
code-reviews/Configuration/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 11 |
| Open findings | 9 |
## Checklist coverage
@@ -33,13 +33,13 @@
| Severity | High |
| Category | Correctness & logic bugs |
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs:282` |
| Status | Open |
| Status | Resolved |
**Description:** `sp_PublishGeneration` invokes `EXEC dbo.sp_ValidateDraft @DraftGenerationId = @DraftGenerationId;` and then continues unconditionally to the reservation MERGE and the `Status='Published'` update. `sp_ValidateDraft` signals every failure with `RAISERROR(..., 16, 1)` followed by `RETURN`. A severity-16 `RAISERROR` is not a batch-aborting error and `SET XACT_ABORT ON` does not abort the transaction for it, so control returns to `sp_PublishGeneration`, which publishes the draft even though validation rejected it (cross-cluster namespace binding, dangling tag FKs, duplicate external identifiers, EquipmentUuid immutability all pass through). Pre-publish validation is effectively bypassed.
**Recommendation:** Wrap the `EXEC dbo.sp_ValidateDraft` in `BEGIN TRY ... END TRY BEGIN CATCH ROLLBACK; THROW; END CATCH` so the validation `RAISERROR` propagates and aborts the publish, or have `sp_ValidateDraft` return a result-set/output parameter that `sp_PublishGeneration` inspects and explicitly rolls back on. Add a regression test that publishes a draft with a known violation and asserts it stays `Draft`.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — wrapped the `EXEC dbo.sp_ValidateDraft` call in `sp_PublishGeneration` in a `BEGIN TRY ... BEGIN CATCH ROLLBACK; THROW; END CATCH` block so a validation `RAISERROR` rolls back the publish transaction and re-raises instead of being silently ignored; added DB-backed regression test `Publish_aborts_when_ValidateDraft_rejects_the_draft`.
### Configuration-002
@@ -138,13 +138,13 @@
| Severity | High |
| Category | Security |
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs:150`, `:373`, `:468` |
| Status | Open |
| Status | Resolved |
**Description:** Three stored procedures build `ConfigAuditLog.DetailsJson` by raw string concatenation of caller-supplied `nvarchar` parameters: `sp_RegisterNodeGenerationApplied` (`@Status`), `sp_RollbackToGeneration` (`@TargetGenerationId`), `sp_ReleaseExternalIdReservation` (`@Kind`, `@Value`). A value with a double-quote or backslash produces malformed JSON; combined with the `CK_ConfigAuditLog_DetailsJson_IsJson` check constraint, the `INSERT` fails the constraint and aborts the surrounding publish/rollback transaction (denial of operation). It is also a JSON-injection vector that can silently rewrite the audit record's shape.
**Recommendation:** Build the JSON with a safe constructor (`FOR JSON PATH, WITHOUT_ARRAY_WRAPPER` or `JSON_OBJECT(...)` on SQL Server 2022+) so values are properly escaped, or run each interpolated value through `STRING_ESCAPE(@Value, 'json')`. Add tests with quote/backslash-containing inputs.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — routed every caller-supplied string interpolated into `DetailsJson` through `STRING_ESCAPE(@x, 'json')` (`@Status` in `sp_RegisterNodeGenerationApplied`; `@Kind`/`@Value` in `sp_ReleaseExternalIdReservation`) and emitted `sp_RollbackToGeneration`'s `@TargetGenerationId` as a bare JSON number via explicit `CONVERT(nvarchar(20), CONVERT(bigint, ...))`; added DB-backed regression tests `RegisterNodeGenerationApplied_escapes_quotes_in_audit_DetailsJson` and `ReleaseReservation_escapes_quotes_in_audit_DetailsJson` that round-trip quote/backslash inputs through `ISJSON`/`JSON_VALUE`.
### Configuration-009

25
src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs
View File

@@ -145,9 +145,12 @@ BEGIN
(NodeId, CurrentGenerationId, LastAppliedAt, LastAppliedStatus, LastAppliedError, LastSeenAt)
VALUES (@NodeId, @GenerationId, SYSUTCDATETIME(), @Status, @Error, SYSUTCDATETIME());
-- Build DetailsJson via STRING_ESCAPE so a @Status containing a double-quote/backslash cannot
-- produce malformed JSON (which would fail CK_ConfigAuditLog_DetailsJson_IsJson and abort the
-- transaction) or inject extra JSON structure into the audit record.
INSERT dbo.ConfigAuditLog (Principal, EventType, NodeId, GenerationId, DetailsJson)
VALUES (@Caller, 'NodeApplied', @NodeId, @GenerationId,
CONCAT('{""status"":""', @Status, '""}'));
CONCAT('{""status"":""', STRING_ESCAPE(@Status, 'json'), '""}'));
END
";
@@ -279,7 +282,18 @@ BEGIN
RETURN;
END
-- sp_ValidateDraft signals every rejection with RAISERROR(..., 16, 1) — a severity-16 error is
-- NOT batch-aborting and SET XACT_ABORT ON does not abort the transaction for it, so without a
-- TRY/CATCH control would return here and the draft would publish despite failed validation.
-- Catch the validation error, roll back the publish transaction, and re-raise so the caller sees
-- the real validation failure.
BEGIN TRY
EXEC dbo.sp_ValidateDraft @DraftGenerationId = @DraftGenerationId;
END TRY
BEGIN CATCH
IF @@TRANCOUNT > 0 ROLLBACK;
THROW;
END CATCH
MERGE dbo.ExternalIdReservation AS tgt
USING (
@@ -369,9 +383,11 @@ BEGIN
EXEC dbo.sp_PublishGeneration @ClusterId = @ClusterId, @DraftGenerationId = @NewGenId, @Notes = @Notes;
-- @TargetGenerationId is a bigint, but build the JSON value via an explicit numeric CONVERT so
-- the emitted token is always a bare JSON number — never reliant on implicit string coercion.
INSERT dbo.ConfigAuditLog (Principal, EventType, ClusterId, GenerationId, DetailsJson)
VALUES (SUSER_SNAME(), 'RolledBack', @ClusterId, @NewGenId,
CONCAT('{""rolledBackTo"":', @TargetGenerationId, '}'));
CONCAT('{""rolledBackTo"":', CONVERT(nvarchar(20), CONVERT(bigint, @TargetGenerationId)), '}'));
COMMIT;
END
@@ -464,9 +480,12 @@ BEGIN
RETURN;
END
-- Escape both caller-supplied values via STRING_ESCAPE so quotes/backslashes cannot break the
-- JSON document or inject additional structure into the audit record.
INSERT dbo.ConfigAuditLog (Principal, EventType, DetailsJson)
VALUES (SUSER_SNAME(), 'ExternalIdReleased',
CONCAT('{""kind"":""', @Kind, '"",""value"":""', @Value, '""}'));
CONCAT('{""kind"":""', STRING_ESCAPE(@Kind, 'json'),
'"",""value"":""', STRING_ESCAPE(@Value, 'json'), '""}'));
END
";
}

87
tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/StoredProceduresTests.cs
View File

@@ -156,6 +156,93 @@ public sealed class StoredProceduresTests
ex.Message.ShouldContain("ReleaseReason is required");
}
/// <summary>
/// Regression for Configuration-001: a draft that fails sp_ValidateDraft must NOT be published.
/// Before the fix, sp_PublishGeneration ran sp_ValidateDraft, ignored its severity-16 RAISERROR,
/// and published the invalid draft anyway.
/// </summary>
[Fact]
public void Publish_aborts_when_ValidateDraft_rejects_the_draft()
{
using var conn = _fixture.OpenConnection();
var (clusterId, _, _, draftId) = SeedClusterWithDraft(conn, suffix: "valbypass");
// Orphan tag — references a DriverInstanceId that does not exist in the generation.
Exec(conn, @"INSERT dbo.Tag (GenerationId, TagId, DriverInstanceId, Name, DataType, AccessLevel, WriteIdempotent, TagConfig)
VALUES (@g, 'tag-1', 'missing-driver', 'X', 'Int32', 'Read', 0, '{}')",
("g", draftId));
var ex = Should.Throw<SqlException>(() =>
Exec(conn, "EXEC dbo.sp_PublishGeneration @ClusterId=@c, @DraftGenerationId=@g",
("c", clusterId), ("g", draftId)));
ex.Message.ShouldContain("unresolved DriverInstanceId");
var status = Scalar<string>(conn,
"SELECT Status FROM dbo.ConfigGeneration WHERE GenerationId = @g", ("g", draftId));
status.ShouldBe("Draft", "an invalid draft must remain Draft after a rejected publish");
}
/// <summary>
/// Regression for Configuration-008: a quote/backslash-bearing value passed to a proc that
/// records DetailsJson must produce well-formed JSON (STRING_ESCAPE), not malformed JSON that
/// fails the CK_ConfigAuditLog_DetailsJson_IsJson check constraint.
/// </summary>
[Fact]
public void RegisterNodeGenerationApplied_escapes_quotes_in_audit_DetailsJson()
{
using var conn = _fixture.OpenConnection();
var (clusterId, nodeId, _, draftId) = SeedClusterWithDraft(conn, suffix: "jsonesc");
Exec(conn, "EXEC dbo.sp_PublishGeneration @ClusterId=@c, @DraftGenerationId=@g",
("c", clusterId), ("g", draftId));
// A status string containing a double-quote and a backslash — would break naive concatenation.
const string hostileStatus = "Applied\"; \\evil";
Should.NotThrow(() =>
Exec(conn, "EXEC dbo.sp_RegisterNodeGenerationApplied @NodeId=@n, @GenerationId=@g, @Status=@s",
("n", nodeId), ("g", draftId), ("s", hostileStatus)));
var detailsJson = Scalar<string>(conn,
@"SELECT TOP 1 DetailsJson FROM dbo.ConfigAuditLog
WHERE EventType = 'NodeApplied' AND NodeId = @n ORDER BY AuditId DESC",
("n", nodeId));
detailsJson.ShouldNotBeNull();
// Round-trip: ISJSON must accept it, and JSON_VALUE must recover the exact original string.
var isValidJson = Scalar<int>(conn, "SELECT ISJSON(@j)", ("j", detailsJson));
isValidJson.ShouldBe(1, "DetailsJson must be well-formed JSON");
var recovered = Scalar<string>(conn, "SELECT JSON_VALUE(@j, '$.status')", ("j", detailsJson));
recovered.ShouldBe(hostileStatus, "the escaped value must round-trip unchanged");
}
/// <summary>
/// Regression for Configuration-008 covering sp_ReleaseExternalIdReservation's @Kind/@Value.
/// </summary>
[Fact]
public void ReleaseReservation_escapes_quotes_in_audit_DetailsJson()
{
using var conn = _fixture.OpenConnection();
// Seed an active reservation with a hostile Value, then release it.
const string hostileValue = "Z\"100\\";
Exec(conn,
@"INSERT dbo.ExternalIdReservation (Kind, Value, EquipmentUuid, ClusterId, FirstPublishedBy, LastPublishedAt)
VALUES ('ZTag', @v, NEWID(), 'cluster-resv-esc', SUSER_SNAME(), SYSUTCDATETIME());",
("v", hostileValue));
Should.NotThrow(() =>
Exec(conn, "EXEC dbo.sp_ReleaseExternalIdReservation @Kind='ZTag', @Value=@v, @ReleaseReason='cleanup'",
("v", hostileValue)));
var detailsJson = Scalar<string>(conn,
@"SELECT TOP 1 DetailsJson FROM dbo.ConfigAuditLog
WHERE EventType = 'ExternalIdReleased' ORDER BY AuditId DESC");
var isValidJson = Scalar<int>(conn, "SELECT ISJSON(@j)", ("j", detailsJson));
isValidJson.ShouldBe(1, "DetailsJson must be well-formed JSON");
var recovered = Scalar<string>(conn, "SELECT JSON_VALUE(@j, '$.value')", ("j", detailsJson));
recovered.ShouldBe(hostileValue, "the escaped value must round-trip unchanged");
}
// ---- helpers ----
/// <summary>Creates a cluster, one node, one credential bound to the current SUSER_SNAME(), and an empty Draft.</summary>