From 933d1739bdb177c70ae055498a2d84596b59191a Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sat, 12 Sep 2026 11:26:07 -0700 Subject: [PATCH] fix(bpmn): refuse documents with duplicate element ids before recursion (#8080) * fix(bpmn): refuse documents with duplicate element ids before recursion A subProcess nested inside another subProcess with the same id made EnsureCapabilitiesSatisfied, BpmnWorkBinder.BindScope and the interchange library's own BpmnXmlWriter recurse without terminating, overflowing the stack and killing the process (.NET cannot catch StackOverflowException). Refuse such a document up front, coded bpmn.import.duplicate-element-id (422), on POST bpmn/import and PUT bpmn/definitions/{id}/document, listing the duplicated ids. Co-Authored-By: Claude Opus 5 * fix(bpmn): include process ids in the duplicate-id check EnsureElementIdsUnique only pooled element ids, never a process definition's own ProcessId. A top-level process's id is never one of its own elements, so a subprocess reusing its parent's id (or two top-level processes sharing an id) went undetected and still overflowed the stack in EnsureCapabilitiesSatisfied, BpmnWorkBinder and BpmnXmlWriter the same way a repeated element id does. Add every top-level process's own id to the pool; a nested process definition's own id needs no equivalent addition since it is always exactly the element id that opens it, already counted once via its owner's elements. Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- doc/wiki/bpmn-workflows.md | 12 ++ .../Elsa.Bpmn.Interchange/BpmnErrorCodes.cs | 10 ++ .../Bpmn/BpmnImportErrorResponses.cs | 16 +++ .../BpmnDuplicateElementIdException.cs | 19 +++ .../BpmnInterchangeDocumentService.cs | 94 ++++++++++++- .../nested-subprocess-duplicate-id.bpmn | 47 +++++++ .../subprocess-reuses-parent-process-id.bpmn | 34 +++++ .../Assets/two-process-duplicate-id.bpmn | 27 ++++ .../Endpoints/BpmnInterchangeEndpointTests.cs | 123 ++++++++++++++++++ .../BpmnErrorResponseMappingTests.cs | 15 +++ ...eDocumentServiceDuplicateElementIdTests.cs | 107 +++++++++++++++ 11 files changed, 503 insertions(+), 1 deletion(-) create mode 100644 src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn create mode 100644 test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn create mode 100644 test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs diff --git a/doc/wiki/bpmn-workflows.md b/doc/wiki/bpmn-workflows.md index d96673c9e..38600b97f 100644 --- a/doc/wiki/bpmn-workflows.md +++ b/doc/wiki/bpmn-workflows.md @@ -215,6 +215,17 @@ capabilities and the offending element ids, rather than persisting a definition runs. `Analyze` never performs this check, since it does not persist; a document that `Analyze` reports cleanly can still be refused by `Import` on capability grounds. +### Duplicate element id refusal + +BPMN requires every element id to be unique within a document. `Import` and the document `PUT` both refuse, with +`422 Unprocessable Entity`, a document that repeats one — most often a subprocess nested inside another subprocess +that reuses its parent's id. This is not just an ordinary validation rule: reading or writing such a document walks +into a nested process by matching the repeated id back out of a flat binding list, in three different places (this +service's own capability walk, the work binder, and the interchange library's own writer), and each of those walks +would otherwise recurse without ever terminating and crash the process outright — .NET cannot catch a +`StackOverflowException`. Both endpoints check this before any of that recursion runs. `Analyze` never performs this +check, since the plain read it does never walks a nested process this way to begin with. + ### Export's limitation `Export` does not reconstruct a `.bpmn` document from the Elsa activity graph a definition runs — that would discard @@ -290,6 +301,7 @@ element that has a stored body but no `bindingRef`, are uncoded. | --- | --- | --- | --- | | `bpmn.import.capability-unsupported` | `POST bpmn/import`, document `PUT` | 422 | `capabilities: string[]` (missing capability names), `elementIds: string[]` (offending element ids, combined across every missing capability) | | `bpmn.import.binding-invalid` | `POST bpmn/import`, document `PUT` | 422 | — | +| `bpmn.import.duplicate-element-id` | `POST bpmn/import`, document `PUT` | 422 | `elementIds: string[]` (the duplicated ids) | | `bpmn.export.not-imported` | `GET .../export`, document `GET` | 422 | — | | `bpmn.export.source-stale` | `GET .../export`, document `GET` | 422 | — | | `bpmn.export.source-version-unknown` | `GET .../export`, document `GET` | 422 | — | diff --git a/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs b/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs index 98c38b31a..ae25cc2e1 100644 --- a/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs +++ b/src/modules/Elsa.Bpmn.Interchange/BpmnErrorCodes.cs @@ -27,6 +27,16 @@ public static class BpmnErrorCodes /// public const string ImportBindingInvalid = "bpmn.import.binding-invalid"; + /// + /// bpmn/import and the document PUT refuse a document that declares the same element id more than + /// once — most often a subprocess nested inside another subprocess that reuses its parent's id. Refused before + /// any recursion into nested processes runs, since a repeated id would otherwise make that recursion — in this + /// type's own capability walk, in BpmnWorkBinder.BindScope, and in Bpmn.Interchange's own + /// BpmnXmlWriter — loop without ever terminating and crash the process outright. Carries + /// data.elementIds (the duplicated ids). + /// + public const string ImportDuplicateElementId = "bpmn.import.duplicate-element-id"; + /// /// bpmn/definitions/{id}/export and the document GET refuse a workflow definition that does not /// currently carry BPMN source — either it was never imported from BPMN, or a later save replaced its custom diff --git a/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs b/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs index 6229e84f8..094e259a1 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Endpoints/Bpmn/BpmnImportErrorResponses.cs @@ -50,6 +50,11 @@ internal static class BpmnImportErrorResponses await BpmnErrorResponse.SendAsync(httpResponse, BindingInvalidResponseFor(exception), cancellationToken); return null; } + catch (BpmnDuplicateElementIdException exception) + { + await BpmnErrorResponse.SendAsync(httpResponse, DuplicateElementIdResponseFor(exception), cancellationToken); + return null; + } catch (BpmnCapabilityException exception) { await BpmnErrorResponse.SendAsync(httpResponse, CapabilityResponseFor(exception), cancellationToken); @@ -83,6 +88,17 @@ internal static class BpmnImportErrorResponses internal static BpmnErrorResponse BindingInvalidResponseFor(BpmnBindingException exception) => BpmnErrorResponse.Create(exception.Message, BpmnErrorCodes.ImportBindingInvalid, StatusCodes.Status422UnprocessableEntity); + /// + /// The response for , carrying + /// the duplicated ids as data.elementIds. + /// + internal static BpmnErrorResponse DuplicateElementIdResponseFor(BpmnDuplicateElementIdException exception) => + BpmnErrorResponse.Create( + exception.Message, + BpmnErrorCodes.ImportDuplicateElementId, + StatusCodes.Status422UnprocessableEntity, + new { ElementIds = exception.DuplicateElementIds }); + /// /// The response for , as its /// own pure, synchronous step so it can be asserted on directly: Bpmn.* 0.2.0 declares every capability diff --git a/src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs b/src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs new file mode 100644 index 000000000..d1f2f0fe7 --- /dev/null +++ b/src/modules/Elsa.Bpmn.Interchange/Exceptions/BpmnDuplicateElementIdException.cs @@ -0,0 +1,19 @@ +namespace Elsa.Bpmn.Interchange.Exceptions; + +/// +/// Thrown when a BPMN document declares the same element id more than once. +/// +/// +/// BPMN requires every element id to be unique within a document. A repeat — most often a subprocess nested inside +/// another subprocess that reuses its parent's id — is not merely invalid input: BpmnInterchangeDocumentService's +/// own capability walk, BpmnWorkBinder.BindScope and Bpmn.Interchange's own BpmnXmlWriter all read +/// "the nested processes belonging to this scope" back out of a flat binding list by matching on the repeated id, so a +/// document like this makes each of them recurse without ever terminating and crash the process outright — .NET +/// cannot catch a . This is thrown, and the document refused, before any of that +/// recursion runs. +/// +public class BpmnDuplicateElementIdException(string message, IReadOnlyList duplicateElementIds) : Exception(message) +{ + /// The element ids the document declares more than once. + public IReadOnlyList DuplicateElementIds { get; } = duplicateElementIds; +} diff --git a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs index 5b07c609b..02bad7e81 100644 --- a/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs +++ b/src/modules/Elsa.Bpmn.Interchange/Services/BpmnInterchangeDocumentService.cs @@ -226,6 +226,13 @@ public sealed class BpmnInterchangeDocumentService( CancellationToken cancellationToken) { var result = reader.Read(xml, new BpmnImportOptions { ProcessId = processId }); + + // Before anything below walks into a nested process by matching an id, refuse a document that repeats one: + // see EnsureElementIdsUnique's remarks for why that walk is otherwise not provably finite. reader.Read itself + // never recurses this way — it walks the XML's own element tree, not an id lookup — so it is safe to call + // first and check its result. + EnsureElementIdsUnique(result.Definitions.Processes, result.Bindings); + var rootDefinition = ResolveRootDefinition(result.Definitions, processId); EnsureCapabilitiesSatisfied(rootDefinition, result.Bindings); @@ -384,7 +391,15 @@ public sealed class BpmnInterchangeDocumentService( $"Workflow definition '{definitionId}' does not exist, so its BPMN document cannot be edited."); } - var xml = writer.Write(document, StoredNestedScopesStillDeclaredBy(document, existingDefinition)); + var storedNestedScopes = StoredNestedScopesStillDeclaredBy(document, existingDefinition); + + // Unlike ImportAsync's xml, writer.Write itself is one of the sites that walks nested processes by matching + // an id (see EnsureElementIdsUnique's remarks), and it runs before ImportCoreAsync — and the same check + // inside it — ever sees this document. So it is checked here too, against exactly the inputs writer.Write is + // about to receive, before that call rather than after it. + EnsureElementIdsUnique(document.Processes, storedNestedScopes); + + var xml = writer.Write(document, storedNestedScopes); return await ImportCoreAsync(xml, definitionId, name: null, processId, preserveMetadataFrom: existingDefinition, cancellationToken); } @@ -595,6 +610,83 @@ public sealed class BpmnInterchangeDocumentService( $"The document declares {definitions.Processes.Count} processes ({declared}); specify which one to import."); } + /// + /// Refuses a document that declares the same element id more than once, naming the duplicated ids. + /// + /// + /// + /// BPMN requires every element id to be unique within a document. The library's own reader tolerates a repeat — + /// BpmnXmlReader walks the XML's own element tree, so it terminates regardless of what any id says — but + /// nothing downstream of it does: this type's own EnsureCapabilitiesSatisfied and BpmnWorkBinder.BindScope + /// both find "the nested processes belonging to this scope" by matching + /// against the scope's own id, and Bpmn.Interchange's own BpmnXmlWriter does the same by matching + /// . A 's own + /// is always the element id of the subprocess element that opens + /// it, so a subprocess nested inside another subprocess that reuses its parent's id makes that lookup find its + /// own parent — or itself — again on every step down. Each of those three walks then recurses without ever + /// terminating and crashes the process outright: .NET cannot catch a . This + /// runs before any of them does, so a document like that is refused rather than crashing the server. + /// + /// + /// Once every element id is unique, that recursion is provably finite without a separate depth guard: a scope's + /// nested processes can then only ever be the ones its own or + /// actually names, so the walk can only ever follow the tree the + /// document's own nesting describes. + /// + /// + /// The document's own top-level process bodies. + /// + /// Every binding across the same processes, so every subprocess body nested inside them — which is not one of + /// itself, and carries elements does not enumerate — + /// is covered too. + /// + /// + /// A top-level 's own is a scope + /// id in exactly the same id-space as every element id below it: EnsureCapabilitiesSatisfied, + /// BpmnWorkBinder.BindScope and Bpmn.Interchange's own BpmnXmlWriter all find "the nested + /// processes belonging to this scope" by matching a 's owner id + /// against a — a top-level process's own id, not one of + /// its declared elements, so nothing below ever puts it in the pool checked for uniqueness on its own. A + /// subprocess reusing that id (e.g. <process id="P"><subProcess id="P">) makes that lookup + /// find the top-level scope again instead of terminating — the same class of infinite recursion a repeated + /// element id causes — so it is added here explicitly, once per top-level process. + /// + /// A nested process definition's own needs no equivalent + /// addition: it is always exactly the of the subprocess + /// element that opens it, by construction of the library's own reader, and that element id is already in the + /// pool below as one of its owner's elements. Adding it a second time would flag every ordinary + /// subprocess as a duplicate of itself; the legitimate pairing is counted once by not adding it again here. + /// + /// + /// An element id, or a top-level process id, is declared more than once. + internal static void EnsureElementIdsUnique(IEnumerable processes, IReadOnlyList bindings) + { + var processList = processes as IReadOnlyCollection ?? processes.ToList(); + + var processIds = processList.Select(process => process.ProcessId); + + var elementIds = processList + .Concat(bindings.OfType().Select(nested => nested.Definition)) + .SelectMany(process => process.Elements) + .Select(element => element.ElementId); + + var duplicateIds = processIds + .Concat(elementIds) + .GroupBy(id => id, StringComparer.Ordinal) + .Where(group => group.Count() > 1) + .Select(group => group.Key) + .ToList(); + + if (duplicateIds.Count == 0) + return; + + throw new BpmnDuplicateElementIdException( + $"The document declares the same element id more than once, which BPMN requires to be unique: {string.Join(", ", duplicateIds)}. " + + "This is most often a subprocess nested inside another subprocess that reuses its parent's id. Reading or writing such a document " + + "cannot be done safely, so it is refused rather than attempted.", + duplicateIds); + } + /// /// Refuses the definition, naming the missing capability and the offending element ids, when it or any process /// nested inside it needs a host capability does not cover. diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn new file mode 100644 index 000000000..7c5460fe7 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/nested-subprocess-duplicate-id.bpmn @@ -0,0 +1,47 @@ + + + + + Flow_1 + + + + + Flow_1 + Flow_2 + + Outer_Flow_1 + + + + Outer_Flow_1 + Outer_Flow_2 + + Inner_Flow_1 + + + Inner_Flow_1 + + + + + + Outer_Flow_2 + + + + + + + Flow_2 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn new file mode 100644 index 000000000..a172caf90 --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/subprocess-reuses-parent-process-id.bpmn @@ -0,0 +1,34 @@ + + + + + + Flow_1 + + + + Flow_1 + Flow_2 + + Inner_Flow_1 + + + Inner_Flow_1 + + + + + + Flow_2 + + + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn new file mode 100644 index 000000000..1cbd45a4a --- /dev/null +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Assets/two-process-duplicate-id.bpmn @@ -0,0 +1,27 @@ + + + + + + Flow_1 + + + Flow_1 + + + + + + Flow_2 + + + Flow_2 + + + + diff --git a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs index 3a09e565a..f4a231ab1 100644 --- a/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs +++ b/test/integration/Elsa.Bpmn.Interchange.IntegrationTests/Endpoints/BpmnInterchangeEndpointTests.cs @@ -148,6 +148,67 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : Assert.Contains("nothing binds it to an Elsa activity", body); } + [Fact] + public async Task Import_OfADocumentWithASubprocessNestedInsideASubprocessThatReusesItsParentsId_ReturnsUnprocessableEntityAndTheServerStaysAlive() + { + using var content = new MultipartFormDataContent(); + AddBpmnFile(content, ReadAsset("nested-subprocess-duplicate-id.bpmn"), "file"); + + var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("Outer", body); + + // elsa-core#8074: before the fix, reading this document overflowed the stack and killed the process, which + // .NET cannot catch — there would be no HTTP response to assert on at all. Reaching the assertions above already + // proves the process survived; a further successful request proves the host is still serving requests, too. + using var followUpContent = new MultipartFormDataContent(); + AddBpmnFile(followUpContent, ReadAsset("camunda-order-process.bpmn"), "file"); + var followUpResponse = await PostAuthenticatedAsync("bpmn/analyze", followUpContent, "workflows/definitions:view"); + Assert.Equal(HttpStatusCode.OK, followUpResponse.StatusCode); + } + + [Fact] + public async Task Import_OfADocumentWithASubprocessReusingItsParentTopLevelProcessesOwnId_ReturnsUnprocessableEntityAndTheServerStaysAlive() + { + using var content = new MultipartFormDataContent(); + AddBpmnFile(content, ReadAsset("subprocess-reuses-parent-process-id.bpmn"), "file"); + + var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("P", body); + + // elsa-core#8074: a top-level process's own id was never in the pool checked for uniqueness (only its + // elements were), so a subprocess declared directly inside it that reuses that same id went undetected and + // overflowed the stack the same way a subprocess nested inside another subprocess does. See the equivalent + // nested-subprocess test's remarks: reaching the assertions above already proves the process survived; a + // further successful request proves the host is still serving requests, too. + using var followUpContent = new MultipartFormDataContent(); + AddBpmnFile(followUpContent, ReadAsset("camunda-order-process.bpmn"), "file"); + var followUpResponse = await PostAuthenticatedAsync("bpmn/analyze", followUpContent, "workflows/definitions:view"); + Assert.Equal(HttpStatusCode.OK, followUpResponse.StatusCode); + } + + [Fact] + public async Task Import_OfADocumentWithTwoTopLevelProcessesSharingAnId_ReturnsUnprocessableEntity() + { + using var content = new MultipartFormDataContent(); + AddBpmnFile(content, ReadAsset("two-process-duplicate-id.bpmn"), "file"); + content.Add(new StringContent("shared"), "ProcessId"); + + var response = await PostAuthenticatedAsync("bpmn/import", content, "workflows/definitions:write"); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, response.StatusCode); + var body = await response.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("shared", body); + } + [Fact] public async Task Import_OfAValidDocument_ReturnsOkAndPersistsADefinition() { @@ -491,6 +552,50 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : Assert.Equal(versionBeforePut, await LatestVersionOfAsync(definitionId)); } + [Fact] + public async Task DocumentPut_WithARepeatedElementId_ReturnsUnprocessableEntityAndPersistsNoNewDraftAndTheServerStaysAlive() + { + var definitionId = await ImportCamundaOrderProcessAsync(); + var versionBeforePut = await LatestVersionOfAsync(definitionId); + + var (etag, documentJson) = await GetDocumentAsync(definitionId); + var putResponse = await PutDocumentAsync(definitionId, WithADuplicatedElementId(documentJson), etag); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, putResponse.StatusCode); + var body = await putResponse.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Equal(versionBeforePut, await LatestVersionOfAsync(definitionId)); + + // See the equivalent Import test's remarks: reaching the assertions above already proves the process + // survived reading this document; a further successful request proves the host is still serving requests. + Assert.Equal(HttpStatusCode.OK, (await GetAuthenticatedAsync($"bpmn/definitions/{definitionId}/document", "workflows/definitions:view")).StatusCode); + } + + [Fact] + public async Task DocumentPut_WithATopLevelProcessIdReusingAnExistingSubprocessId_ReturnsUnprocessableEntityAndPersistsNoNewDraftAndTheServerStaysAlive() + { + // nested-subprocesses.bpmn already declares a subprocess with id "Outer"; renaming the top-level process's + // own id to "Outer" reproduces elsa-core#8074's collision through the document PUT, where the nested scope + // ("Outer"'s stored body) comes not from the edited document but from the definition's already-stored + // source (see ImportDocumentAsync's remarks on storedNestedScopes). + var definitionId = await ImportWrittenBackAsync("nested-subprocesses.bpmn"); + var versionBeforePut = await LatestVersionOfAsync(definitionId); + + var (etag, documentJson) = await GetDocumentAsync(definitionId); + var putResponse = await PutDocumentAsync(definitionId, WithTopLevelProcessIdReusingASubprocessId(documentJson, "Outer"), etag); + + Assert.Equal(HttpStatusCode.UnprocessableEntity, putResponse.StatusCode); + var body = await putResponse.Content.ReadAsStringAsync(); + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, CodeOf(body)); + Assert.Contains("Outer", body); + Assert.Equal(versionBeforePut, await LatestVersionOfAsync(definitionId)); + + // See the equivalent repeated-element-id test's remarks: reaching the assertions above already proves the + // process survived reading this document; a further successful request proves the host is still serving + // requests, too. + Assert.Equal(HttpStatusCode.OK, (await GetAuthenticatedAsync($"bpmn/definitions/{definitionId}/document", "workflows/definitions:view")).StatusCode); + } + [Fact] public async Task DocumentPut_UnchangedDocument_ReturnsOkAndTheSameFindingsAsImport() { @@ -784,6 +889,24 @@ public class BpmnInterchangeEndpointTests(ITestOutputHelper testOutputHelper) : return document.ToJsonString(); } + /// Renames the document's last top-level element to its first element's id, so the two collide (elsa-core#8074). + private static string WithADuplicatedElementId(string documentJson) + { + var document = JsonNode.Parse(documentJson)!; + var elements = document["processes"]![0]!["elements"]!.AsArray(); + var firstElementId = elements[0]!["elementId"]!.GetValue(); + elements[^1]!["elementId"] = firstElementId; + return document.ToJsonString(); + } + + /// Renames the document's top-level process id to , so it collides with a subprocess that already declares that id (elsa-core#8074). + private static string WithTopLevelProcessIdReusingASubprocessId(string documentJson, string subprocessId) + { + var document = JsonNode.Parse(documentJson)!; + document["processes"]![0]!["processId"] = subprocessId; + return document.ToJsonString(); + } + /// /// Changes the literal text the NotifyWarehouse task's elsa:activityBinding configures its bound /// with — a real binding change, the kind Elsa Studio's binding UX makes, as opposed to diff --git a/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs index 87dd4d5c8..80eb7be5d 100644 --- a/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs +++ b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnErrorResponseMappingTests.cs @@ -56,6 +56,21 @@ public class BpmnErrorResponseMappingTests Assert.Null(response.Data); } + [Fact(DisplayName = "A duplicate-element-id refusal is coded bpmn.import.duplicate-element-id, carrying the duplicated ids as data")] + public void DuplicateElementIdResponseFor_CarriesTheCodeAndTheStructuredData() + { + var exception = new BpmnDuplicateElementIdException("The document declares the same element id more than once: Outer.", ["Outer"]); + + var response = BpmnImportErrorResponses.DuplicateElementIdResponseFor(exception); + + Assert.Equal(BpmnErrorCodes.ImportDuplicateElementId, response.Code); + Assert.Equal(StatusCodes.Status422UnprocessableEntity, response.StatusCode); + Assert.Equal(exception.Message, Assert.Single(response.Errors["generalErrors"])); + + dynamic data = response.Data!; + Assert.Equal(new[] { "Outer" }, (IReadOnlyList)data.ElementIds); + } + [Fact(DisplayName = "A definition-not-found refusal is coded bpmn.document.not-found")] public void NotFoundResponseFor_CarriesTheCode() { diff --git a/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs new file mode 100644 index 000000000..d341948b0 --- /dev/null +++ b/test/unit/Elsa.Bpmn.Interchange.UnitTests/BpmnInterchangeDocumentServiceDuplicateElementIdTests.cs @@ -0,0 +1,107 @@ +using Bpmn.Interchange; +using Bpmn.Model; +using Elsa.Bpmn.Interchange.Exceptions; +using Elsa.Bpmn.Interchange.Services; + +namespace Elsa.Bpmn.Interchange.UnitTests; + +/// +/// Duplicate element id refusal at import: is the +/// internal seam the Import endpoint and the document PUT both call into, exercised here directly with the +/// exact shape a subprocess nested inside another subprocess that reuses its parent's id produces — the shape that +/// otherwise makes , BpmnWorkBinder.BindScope +/// and BpmnXmlWriter recurse without terminating (elsa-core#8074). +/// +public class BpmnInterchangeDocumentServiceDuplicateElementIdTests +{ + [Fact(DisplayName = "A document whose element ids are all unique is accepted")] + public void EnsureElementIdsUnique_AcceptsADocumentWithNoRepeatedIds() + { + var task = new BpmnElement("task-1", BpmnElementTypes.ServiceTask, bindingRef: "node-task-1"); + var root = new BpmnProcessDefinition("main", Elements: [task]); + + // No exception is the assertion: every element id in the document is unique. + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], []); + } + + [Fact(DisplayName = "A document declaring the same element id twice at the top level is refused, naming the id")] + public void EnsureElementIdsUnique_RefusesARepeatedTopLevelElementId() + { + var first = new BpmnElement("dup", BpmnElementTypes.ServiceTask, bindingRef: "node-dup-1"); + var second = new BpmnElement("dup", BpmnElementTypes.ServiceTask, bindingRef: "node-dup-2"); + var root = new BpmnProcessDefinition("main", Elements: [first, second]); + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], [])); + + Assert.Equal(["dup"], exception.DuplicateElementIds); + Assert.Contains("dup", exception.Message); + } + + [Fact(DisplayName = "A subprocess nested inside another subprocess that reuses its parent's id is refused, naming that id")] + public void EnsureElementIdsUnique_RefusesASubprocessNestedInsideASubprocessThatReusesItsParentsId() + { + var (root, bindings) = NestedSubprocessReusingItsOwnId(); + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], bindings)); + + Assert.Equal(["Outer"], exception.DuplicateElementIds); + } + + [Fact(DisplayName = "A subprocess reusing its parent top-level process's own id is refused, naming that id")] + public void EnsureElementIdsUnique_RefusesASubprocessReusingItsParentTopLevelProcessesOwnId() + { + // ...: the subprocess element id equals the + // top-level process's own ProcessId. Before the fix, a top-level process's own id was never added to the + // pool checked for uniqueness (only its Elements were), so "P" appeared only once — as the subprocess + // element inside root.Elements — and this collision went undetected. + var subProcessElement = new BpmnElement("P", BpmnElementTypes.SubProcess, bindingRef: "node-p"); + var root = new BpmnProcessDefinition("P", Elements: [subProcessElement]); + var subProcessBody = new BpmnProcessDefinition("P"); + + BpmnWorkBinding[] bindings = [new BpmnWorkBinding.NestedProcess("P", "P", "node-p", BpmnBindingSlot.Primary, subProcessBody)]; + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([root], bindings)); + + Assert.Equal(["P"], exception.DuplicateElementIds); + } + + [Fact(DisplayName = "Two top-level processes sharing the same id are refused, naming that id")] + public void EnsureElementIdsUnique_RefusesTwoTopLevelProcessesSharingAnId() + { + var first = new BpmnProcessDefinition("shared", Elements: [new BpmnElement("task-1", BpmnElementTypes.ServiceTask, bindingRef: "node-task-1")]); + var second = new BpmnProcessDefinition("shared", Elements: [new BpmnElement("task-2", BpmnElementTypes.ServiceTask, bindingRef: "node-task-2")]); + + var exception = Assert.Throws(() => + BpmnInterchangeDocumentService.EnsureElementIdsUnique([first, second], [])); + + Assert.Equal(["shared"], exception.DuplicateElementIds); + } + + /// + /// The exact document shape elsa-core#8074 reports: process main declares a subprocess Outer, + /// whose body declares another subprocess that reuses the id Outer rather than declaring one of its own. + /// + private static (BpmnProcessDefinition Root, IReadOnlyList Bindings) NestedSubprocessReusingItsOwnId() + { + var outerElement = new BpmnElement("Outer", BpmnElementTypes.SubProcess, bindingRef: "node-outer"); + var root = new BpmnProcessDefinition("main", Elements: [outerElement]); + + // The inner subprocess element, declared inside Outer's own body, reuses "Outer" as its id instead of + // declaring its own — the innermost NestedProcess.Definition's own ProcessId is "Outer" too, for the same + // reason (Bpmn.Interchange's reader gives a subprocess body the element id that opens it as its ProcessId). + var innerElement = new BpmnElement("Outer", BpmnElementTypes.SubProcess, bindingRef: "node-outer-inner"); + var outerBody = new BpmnProcessDefinition("Outer", Elements: [innerElement]); + var innerBody = new BpmnProcessDefinition("Outer"); + + BpmnWorkBinding[] bindings = + [ + new BpmnWorkBinding.NestedProcess("main", "Outer", "node-outer", BpmnBindingSlot.Primary, outerBody), + new BpmnWorkBinding.NestedProcess("Outer", "Outer", "node-outer-inner", BpmnBindingSlot.Primary, innerBody) + ]; + + return (root, bindings); + } +}