diff --git a/code-reviews/Configuration/findings.md b/code-reviews/Configuration/findings.md index 4dc9db6..ac2ece9 100644 --- a/code-reviews/Configuration/findings.md +++ b/code-reviews/Configuration/findings.md @@ -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 diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs index 33f3571..8646767 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Migrations/20260417215224_StoredProcedures.cs @@ -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 - EXEC dbo.sp_ValidateDraft @DraftGenerationId = @DraftGenerationId; + -- 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 "; } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/StoredProceduresTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/StoredProceduresTests.cs index 3942088..a3ed771 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/StoredProceduresTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/StoredProceduresTests.cs @@ -156,6 +156,93 @@ public sealed class StoredProceduresTests ex.Message.ShouldContain("ReleaseReason is required"); } + /// + /// 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. + /// + [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(() => + Exec(conn, "EXEC dbo.sp_PublishGeneration @ClusterId=@c, @DraftGenerationId=@g", + ("c", clusterId), ("g", draftId))); + ex.Message.ShouldContain("unresolved DriverInstanceId"); + + var status = Scalar(conn, + "SELECT Status FROM dbo.ConfigGeneration WHERE GenerationId = @g", ("g", draftId)); + status.ShouldBe("Draft", "an invalid draft must remain Draft after a rejected publish"); + } + + /// + /// 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. + /// + [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(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(conn, "SELECT ISJSON(@j)", ("j", detailsJson)); + isValidJson.ShouldBe(1, "DetailsJson must be well-formed JSON"); + var recovered = Scalar(conn, "SELECT JSON_VALUE(@j, '$.status')", ("j", detailsJson)); + recovered.ShouldBe(hostileStatus, "the escaped value must round-trip unchanged"); + } + + /// + /// Regression for Configuration-008 covering sp_ReleaseExternalIdReservation's @Kind/@Value. + /// + [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(conn, + @"SELECT TOP 1 DetailsJson FROM dbo.ConfigAuditLog + WHERE EventType = 'ExternalIdReleased' ORDER BY AuditId DESC"); + var isValidJson = Scalar(conn, "SELECT ISJSON(@j)", ("j", detailsJson)); + isValidJson.ShouldBe(1, "DetailsJson must be well-formed JSON"); + var recovered = Scalar(conn, "SELECT JSON_VALUE(@j, '$.value')", ("j", detailsJson)); + recovered.ShouldBe(hostileValue, "the escaped value must round-trip unchanged"); + } + // ---- helpers ---- /// Creates a cluster, one node, one credential bound to the current SUSER_SNAME(), and an empty Draft.