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 <noreply@anthropic.com>
* 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 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra
GitHub
parent
091e3bc0e4
commit
933d1739bd
|
|
@ -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 | — |
|
||||
|
|
|
|||
|
|
@ -27,6 +27,16 @@ public static class BpmnErrorCodes
|
|||
/// </summary>
|
||||
public const string ImportBindingInvalid = "bpmn.import.binding-invalid";
|
||||
|
||||
/// <summary>
|
||||
/// <c>bpmn/import</c> and the document <c>PUT</c> 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 <c>BpmnWorkBinder.BindScope</c>, and in <c>Bpmn.Interchange</c>'s own
|
||||
/// <c>BpmnXmlWriter</c> — loop without ever terminating and crash the process outright. Carries
|
||||
/// <c>data.elementIds</c> (the duplicated ids).
|
||||
/// </summary>
|
||||
public const string ImportDuplicateElementId = "bpmn.import.duplicate-element-id";
|
||||
|
||||
/// <summary>
|
||||
/// <c>bpmn/definitions/{id}/export</c> and the document <c>GET</c> 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
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
||||
/// <summary>
|
||||
/// The <see cref="BpmnErrorCodes.ImportDuplicateElementId"/> response for <paramref name="exception"/>, carrying
|
||||
/// the duplicated ids as <c>data.elementIds</c>.
|
||||
/// </summary>
|
||||
internal static BpmnErrorResponse DuplicateElementIdResponseFor(BpmnDuplicateElementIdException exception) =>
|
||||
BpmnErrorResponse.Create(
|
||||
exception.Message,
|
||||
BpmnErrorCodes.ImportDuplicateElementId,
|
||||
StatusCodes.Status422UnprocessableEntity,
|
||||
new { ElementIds = exception.DuplicateElementIds });
|
||||
|
||||
/// <summary>
|
||||
/// The <see cref="BpmnErrorCodes.ImportCapabilityUnsupported"/> response for <paramref name="exception"/>, as its
|
||||
/// own pure, synchronous step so it can be asserted on directly: <c>Bpmn.*</c> 0.2.0 declares every capability
|
||||
|
|
|
|||
|
|
@ -0,0 +1,19 @@
|
|||
namespace Elsa.Bpmn.Interchange.Exceptions;
|
||||
|
||||
/// <summary>
|
||||
/// Thrown when a BPMN document declares the same element id more than once.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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: <c>BpmnInterchangeDocumentService</c>'s
|
||||
/// own capability walk, <c>BpmnWorkBinder.BindScope</c> and <c>Bpmn.Interchange</c>'s own <c>BpmnXmlWriter</c> 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 <see cref="StackOverflowException"/>. This is thrown, and the document refused, before any of that
|
||||
/// recursion runs.
|
||||
/// </remarks>
|
||||
public class BpmnDuplicateElementIdException(string message, IReadOnlyList<string> duplicateElementIds) : Exception(message)
|
||||
{
|
||||
/// <summary>The element ids the document declares more than once.</summary>
|
||||
public IReadOnlyList<string> DuplicateElementIds { get; } = duplicateElementIds;
|
||||
}
|
||||
|
|
@ -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.");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Refuses a document that declares the same element id more than once, naming the duplicated ids.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// BPMN requires every element id to be unique within a document. The library's own reader tolerates a repeat —
|
||||
/// <c>BpmnXmlReader</c> 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 <c>EnsureCapabilitiesSatisfied</c> and <c>BpmnWorkBinder.BindScope</c>
|
||||
/// both find "the nested processes belonging to this scope" by matching <see cref="BpmnWorkBinding.ProcessId"/>
|
||||
/// against the scope's own id, and <c>Bpmn.Interchange</c>'s own <c>BpmnXmlWriter</c> does the same by matching
|
||||
/// <see cref="BpmnWorkBinding.BindingRef"/>. A <see cref="BpmnWorkBinding.NestedProcess"/>'s own
|
||||
/// <see cref="BpmnProcessDefinition.ProcessId"/> 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 <see cref="StackOverflowException"/>. This
|
||||
/// runs before any of them does, so a document like that is refused rather than crashing the server.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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 <see cref="BpmnWorkBinding.ProcessId"/> or
|
||||
/// <see cref="BpmnWorkBinding.BindingRef"/> actually names, so the walk can only ever follow the tree the
|
||||
/// document's own nesting describes.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="processes">The document's own top-level process bodies.</param>
|
||||
/// <param name="bindings">
|
||||
/// Every binding across the same processes, so every subprocess body nested inside them — which is not one of
|
||||
/// <paramref name="processes"/> itself, and carries elements <paramref name="processes"/> does not enumerate —
|
||||
/// is covered too.
|
||||
/// </param>
|
||||
/// <remarks>
|
||||
/// A top-level <see cref="BpmnProcessDefinition"/>'s own <see cref="BpmnProcessDefinition.ProcessId"/> is a scope
|
||||
/// id in exactly the same id-space as every element id below it: <c>EnsureCapabilitiesSatisfied</c>,
|
||||
/// <c>BpmnWorkBinder.BindScope</c> and <c>Bpmn.Interchange</c>'s own <c>BpmnXmlWriter</c> all find "the nested
|
||||
/// processes belonging to this scope" by matching a <see cref="BpmnWorkBinding.NestedProcess"/>'s owner id
|
||||
/// against a <see cref="BpmnProcessDefinition.ProcessId"/> — a top-level process's own <em>id</em>, 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. <c><process id="P"><subProcess id="P"></c>) 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.
|
||||
/// <para>
|
||||
/// A <em>nested</em> process definition's own <see cref="BpmnProcessDefinition.ProcessId"/> needs no equivalent
|
||||
/// addition: it is always exactly the <see cref="BpmnWorkBinding.ElementId"/> 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 <em>owner</em>'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.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <exception cref="BpmnDuplicateElementIdException">An element id, or a top-level process id, is declared more than once.</exception>
|
||||
internal static void EnsureElementIdsUnique(IEnumerable<BpmnProcessDefinition> processes, IReadOnlyList<BpmnWorkBinding> bindings)
|
||||
{
|
||||
var processList = processes as IReadOnlyCollection<BpmnProcessDefinition> ?? processes.ToList();
|
||||
|
||||
var processIds = processList.Select(process => process.ProcessId);
|
||||
|
||||
var elementIds = processList
|
||||
.Concat(bindings.OfType<BpmnWorkBinding.NestedProcess>().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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Refuses the definition, naming the missing capability and the offending element ids, when it or any process
|
||||
/// nested inside it needs a host capability <see cref="DeclaredHostCapabilities"/> does not cover.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,47 @@
|
|||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL"
|
||||
xmlns:vw="https://bpmn.valenceworks.io/schema/bpmn"
|
||||
id="Definitions_nested-subprocess-duplicate-id"
|
||||
targetNamespace="http://bpmn.io/schema/bpmn">
|
||||
<bpmn:process id="nested-subprocess-duplicate-id" name="Nested Subprocess Duplicate Id" isExecutable="true">
|
||||
<bpmn:startEvent id="Start_1">
|
||||
<bpmn:outgoing>Flow_1</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
|
||||
<!-- The outer subprocess and the subprocess nested directly inside it share the same id ("Outer"), which is
|
||||
invalid BPMN (ids must be unique within a document) and, before elsa-core#8074's fix, overflowed the stack
|
||||
of any code that finds a scope's nested processes by matching this id back out of the flat binding list. -->
|
||||
<bpmn:subProcess id="Outer" name="Outer">
|
||||
<bpmn:incoming>Flow_1</bpmn:incoming>
|
||||
<bpmn:outgoing>Flow_2</bpmn:outgoing>
|
||||
<bpmn:startEvent id="Outer_Start">
|
||||
<bpmn:outgoing>Outer_Flow_1</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
|
||||
<bpmn:subProcess id="Outer" name="Inner reusing its parent's id">
|
||||
<bpmn:incoming>Outer_Flow_1</bpmn:incoming>
|
||||
<bpmn:outgoing>Outer_Flow_2</bpmn:outgoing>
|
||||
<bpmn:startEvent id="Inner_Start">
|
||||
<bpmn:outgoing>Inner_Flow_1</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
<bpmn:endEvent id="Inner_End">
|
||||
<bpmn:incoming>Inner_Flow_1</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
<bpmn:sequenceFlow id="Inner_Flow_1" sourceRef="Inner_Start" targetRef="Inner_End" />
|
||||
</bpmn:subProcess>
|
||||
|
||||
<bpmn:endEvent id="Outer_End">
|
||||
<bpmn:incoming>Outer_Flow_2</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
<bpmn:sequenceFlow id="Outer_Flow_1" sourceRef="Outer_Start" targetRef="Outer" />
|
||||
<bpmn:sequenceFlow id="Outer_Flow_2" sourceRef="Outer" targetRef="Outer_End" />
|
||||
</bpmn:subProcess>
|
||||
|
||||
<bpmn:endEvent id="End_1">
|
||||
<bpmn:incoming>Flow_2</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
|
||||
<bpmn:sequenceFlow id="Flow_1" sourceRef="Start_1" targetRef="Outer" />
|
||||
<bpmn:sequenceFlow id="Flow_2" sourceRef="Outer" targetRef="End_1" />
|
||||
</bpmn:process>
|
||||
</bpmn:definitions>
|
||||
|
|
@ -0,0 +1,34 @@
|
|||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL"
|
||||
id="Definitions_subprocess-reuses-parent-process-id"
|
||||
targetNamespace="http://bpmn.io/schema/bpmn">
|
||||
<!-- The top-level process and the subprocess declared directly inside it share the same id ("P"), which is
|
||||
invalid BPMN (ids must be unique within a document) and, before elsa-core#8074's fix, overflowed the stack of
|
||||
any code that finds a scope's nested processes by matching this id back out of the flat binding list: the
|
||||
top-level process's own id was never added to the pool checked for uniqueness (only its elements were), so a
|
||||
subprocess reusing it went undetected. -->
|
||||
<bpmn:process id="P" name="Parent Reused By Its Own Subprocess" isExecutable="true">
|
||||
<bpmn:startEvent id="Start_1">
|
||||
<bpmn:outgoing>Flow_1</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
|
||||
<bpmn:subProcess id="P" name="Subprocess Reusing Its Parent's Id">
|
||||
<bpmn:incoming>Flow_1</bpmn:incoming>
|
||||
<bpmn:outgoing>Flow_2</bpmn:outgoing>
|
||||
<bpmn:startEvent id="Inner_Start">
|
||||
<bpmn:outgoing>Inner_Flow_1</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
<bpmn:endEvent id="Inner_End">
|
||||
<bpmn:incoming>Inner_Flow_1</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
<bpmn:sequenceFlow id="Inner_Flow_1" sourceRef="Inner_Start" targetRef="Inner_End" />
|
||||
</bpmn:subProcess>
|
||||
|
||||
<bpmn:endEvent id="End_1">
|
||||
<bpmn:incoming>Flow_2</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
|
||||
<bpmn:sequenceFlow id="Flow_1" sourceRef="Start_1" targetRef="P" />
|
||||
<bpmn:sequenceFlow id="Flow_2" sourceRef="P" targetRef="End_1" />
|
||||
</bpmn:process>
|
||||
</bpmn:definitions>
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL"
|
||||
id="Definitions_two-process-duplicate-id"
|
||||
targetNamespace="http://bpmn.io/schema/bpmn">
|
||||
<!-- Two top-level processes declaring the same id ("shared"), which is invalid BPMN (ids must be unique within a
|
||||
document): a top-level process's own id was never added to the pool checked for uniqueness (only its
|
||||
elements were), so two processes sharing an id went undetected as long as their own elements did not repeat
|
||||
any id. -->
|
||||
<bpmn:process id="shared" name="First Process" isExecutable="true">
|
||||
<bpmn:startEvent id="Start_1">
|
||||
<bpmn:outgoing>Flow_1</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
<bpmn:endEvent id="End_1">
|
||||
<bpmn:incoming>Flow_1</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
<bpmn:sequenceFlow id="Flow_1" sourceRef="Start_1" targetRef="End_1" />
|
||||
</bpmn:process>
|
||||
<bpmn:process id="shared" name="Second Process" isExecutable="true">
|
||||
<bpmn:startEvent id="Start_2">
|
||||
<bpmn:outgoing>Flow_2</bpmn:outgoing>
|
||||
</bpmn:startEvent>
|
||||
<bpmn:endEvent id="End_2">
|
||||
<bpmn:incoming>Flow_2</bpmn:incoming>
|
||||
</bpmn:endEvent>
|
||||
<bpmn:sequenceFlow id="Flow_2" sourceRef="Start_2" targetRef="End_2" />
|
||||
</bpmn:process>
|
||||
</bpmn:definitions>
|
||||
|
|
@ -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();
|
||||
}
|
||||
|
||||
/// <summary>Renames the document's last top-level element to its first element's id, so the two collide (elsa-core#8074).</summary>
|
||||
private static string WithADuplicatedElementId(string documentJson)
|
||||
{
|
||||
var document = JsonNode.Parse(documentJson)!;
|
||||
var elements = document["processes"]![0]!["elements"]!.AsArray();
|
||||
var firstElementId = elements[0]!["elementId"]!.GetValue<string>();
|
||||
elements[^1]!["elementId"] = firstElementId;
|
||||
return document.ToJsonString();
|
||||
}
|
||||
|
||||
/// <summary>Renames the document's top-level process id to <paramref name="subprocessId"/>, so it collides with a subprocess that already declares that id (elsa-core#8074).</summary>
|
||||
private static string WithTopLevelProcessIdReusingASubprocessId(string documentJson, string subprocessId)
|
||||
{
|
||||
var document = JsonNode.Parse(documentJson)!;
|
||||
document["processes"]![0]!["processId"] = subprocessId;
|
||||
return document.ToJsonString();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Changes the literal text the <c>NotifyWarehouse</c> task's <c>elsa:activityBinding</c> configures its bound
|
||||
/// <see cref="WriteLine"/> with — a real binding change, the kind Elsa Studio's binding UX makes, as opposed to
|
||||
|
|
|
|||
|
|
@ -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<string>)data.ElementIds);
|
||||
}
|
||||
|
||||
[Fact(DisplayName = "A definition-not-found refusal is coded bpmn.document.not-found")]
|
||||
public void NotFoundResponseFor_CarriesTheCode()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Duplicate element id refusal at import: <see cref="BpmnInterchangeDocumentService.EnsureElementIdsUnique"/> is the
|
||||
/// internal seam the Import endpoint and the document <c>PUT</c> 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 <see cref="BpmnInterchangeDocumentService.EnsureCapabilitiesSatisfied"/>, <c>BpmnWorkBinder.BindScope</c>
|
||||
/// and <c>BpmnXmlWriter</c> recurse without terminating (elsa-core#8074).
|
||||
/// </summary>
|
||||
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<BpmnDuplicateElementIdException>(() =>
|
||||
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<BpmnDuplicateElementIdException>(() =>
|
||||
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()
|
||||
{
|
||||
// <process id="P"><subProcess id="P">...</subProcess></process>: 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<BpmnDuplicateElementIdException>(() =>
|
||||
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<BpmnDuplicateElementIdException>(() =>
|
||||
BpmnInterchangeDocumentService.EnsureElementIdsUnique([first, second], []));
|
||||
|
||||
Assert.Equal(["shared"], exception.DuplicateElementIds);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The exact document shape elsa-core#8074 reports: process <c>main</c> declares a subprocess <c>Outer</c>,
|
||||
/// whose body declares another subprocess that reuses the id <c>Outer</c> rather than declaring one of its own.
|
||||
/// </summary>
|
||||
private static (BpmnProcessDefinition Root, IReadOnlyList<BpmnWorkBinding> 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);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue