fix(bpmn): detect designer-edited drafts as stale BPMN source (#8065)

An unpublished draft is saved in place, so a designer save that edits a
bound activity's inputs changes StringData without changing Version,
which the export/document staleness check missed. Import now also
records a content hash of the graph (Bpmn:SourceGraphHash) and
ResolveSourceXml refuses as stale when it no longer matches, falling
back to the version-only check for definitions imported before this
marker existed.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra 2026-09-11 22:46:58 -07:00 committed by GitHub
parent 22c1aea472
commit d0dd7c9ef5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 227 additions and 36 deletions

View file

@ -129,7 +129,8 @@ whole-definition import and resets them from the document, same as it always has
The `document` GET/PUT pair exists for Studio (W21, part of #7909): Studio holds a BPMN process as the library's own
JSON payload, not as `.bpmn` XML, and editing it — binding a task (W11), moving a shape (W14) — has to write that
JSON back through the same path `Import` uses, or the stored BPMN source drifts out of sync with the definition (see
`BpmnInterchangeDocumentService.SourceVersionCustomPropertyKey`) and `Export` starts refusing with `422`.
`BpmnInterchangeDocumentService.SourceVersionCustomPropertyKey` and `SourceGraphHashCustomPropertyKey`) and `Export`
starts refusing with `422`.
The request and response bodies on both routes are the `bpmnDefinitions` document exactly as `Bpmn.Model` serializes
it: property names as `Bpmn.Model`'s own `[JsonPropertyName]` attributes declare them, and any enum as its underlying
@ -142,7 +143,8 @@ imported with — recorded on the workflow definition the first time it is impor
`document` `PUT`, so an edit to a multi-process document does not have to name the process again on every save.
**The document `PUT` edits the BPMN document, not the whole definition.** Only the activity graph the newly bound
document produces and the `Bpmn:*` custom properties (`SourceXml`, `SourceVersion`, `SourceProcessId`) change; the
document produces and the `Bpmn:*` custom properties (`SourceXml`, `SourceVersion`, `SourceProcessId`,
`SourceGraphHash`) change; the
definition's name, description, variables, inputs, outputs, outcomes, options, tool version and every other custom
property are carried forward exactly as they stood before the `PUT`. This is what lets Studio's binding UX (elsa-
studio#1001) save a binding change through this endpoint without silently resetting metadata the author set some
@ -166,19 +168,30 @@ everything the reader retained on import (foreign extension elements, foreign at
DI layout). Instead, it returns exactly the document the most recent successful import stored. This has a real
consequence for one kind of edit: **an edit made through Elsa's own designer, without going back through BPMN, is not
reflected in what `Export` returns** — the graph moves on, but the stored source still describes the document as it
stood before that edit.
stood before that edit. Rather than silently returning that pre-edit document, `Export` and the document `GET` refuse
it as stale — see below.
An edit made through the `document` `PUT` endpoint above is different: it re-imports, so the stored source and the
graph move together, and **`Export` reflects the edit immediately afterward.**
`Export` also refuses outright, with `422 Unprocessable Entity`, rather than silently returning a stale or wrong
document, in two situations:
document, in three situations:
- The definition does not currently carry BPMN source — either it was never imported from BPMN, or a later save
replaced its custom properties wholesale (BPMN source travels on the same `CustomProperties` dictionary a workflow
edit can overwrite).
- The definition has changed — by version — since the source was recorded, meaning the stored BPMN text no longer
corresponds to the current definition.
- The definition's activity graph has changed since the source was recorded, even though its version has not. An
unpublished draft is saved in place (same row, same version), so a designer save that edits a bound activity's
inputs — as Studio's binding UX (elsa-studio#1001) does — moves the graph without moving the version, which the
version check above cannot see. `Import` also records a SHA-256 hash of the graph
(`BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey`, `Bpmn:SourceGraphHash`) at the moment it stores
the source, and `Export`/the document `GET` refuse when the current graph's hash no longer matches it. A definition
imported before this marker existed carries no value for it and falls back to the version-only check, so it is not
refused just for predating the marker. One practical consequence: a document `GET` performed after a designer save
now returns `422` too — Studio has to re-import (or PUT a fresh document) rather than edit a document that no
longer describes the current graph.
A missing `definitionId` returns `404 Not Found`.

View file

@ -1,7 +1,5 @@
using System.Buffers.Binary;
using System.Globalization;
using System.Security.Cryptography;
using System.Text;
using Elsa.Bpmn.Interchange.Services;
using Elsa.Extensions;
using Elsa.Workflows.Management.Entities;
@ -41,29 +39,11 @@ internal static class BpmnDocumentETag
var sourceXml = definition.CustomProperties.TryGetValue<string>(BpmnInterchangeDocumentService.SourceXmlCustomPropertyKey, out var xml) ? xml : null;
using var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA256);
Append(hash, definition.Id);
Append(hash, definition.Version.ToString(CultureInfo.InvariantCulture));
Append(hash, sourceXml);
Append(hash, definition.StringData);
BpmnContentHash.AppendField(hash, definition.Id);
BpmnContentHash.AppendField(hash, definition.Version.ToString(CultureInfo.InvariantCulture));
BpmnContentHash.AppendField(hash, sourceXml);
BpmnContentHash.AppendField(hash, definition.StringData);
return $"\"{Convert.ToHexString(hash.GetHashAndReset())}\"";
}
private static void Append(IncrementalHash hash, string? value)
{
Span<byte> header = stackalloc byte[5];
if (value is null)
{
header[0] = 0;
hash.AppendData(header[..1]);
return;
}
var bytes = Encoding.UTF8.GetBytes(value);
header[0] = 1;
BinaryPrimitives.WriteInt32BigEndian(header[1..], bytes.Length);
hash.AppendData(header);
hash.AppendData(bytes);
}
}

View file

@ -0,0 +1,47 @@
using System.Buffers.Binary;
using System.Security.Cryptography;
using System.Text;
namespace Elsa.Bpmn.Interchange.Services;
/// <summary>
/// The one place a workflow definition's serialized activity graph
/// (<see cref="Elsa.Workflows.Management.Entities.WorkflowDefinition.StringData"/>) is turned into a content hash,
/// shared by <see cref="BpmnInterchangeDocumentService"/> — which records the hash at import time under
/// <see cref="BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey"/> to tell a designer-edited graph
/// apart from the one BPMN source was imported against — and
/// <see cref="Elsa.Bpmn.Interchange.Endpoints.Bpmn.Document.BpmnDocumentETag"/>, so the two never disagree about how
/// the same field hashes.
/// </summary>
internal static class BpmnContentHash
{
/// <summary>The hex-encoded SHA-256 of a workflow definition's serialized activity graph.</summary>
public static string OfGraph(string? stringData)
{
using var hash = IncrementalHash.CreateHash(HashAlgorithmName.SHA256);
AppendField(hash, stringData);
return Convert.ToHexString(hash.GetHashAndReset());
}
/// <summary>
/// Appends a length-prefixed UTF-8 encoding of <paramref name="value"/> to <paramref name="hash"/>, marking a
/// <c>null</c> value distinctly from an empty one so no two different inputs feed the hash the same bytes.
/// </summary>
public static void AppendField(IncrementalHash hash, string? value)
{
Span<byte> header = stackalloc byte[5];
if (value is null)
{
header[0] = 0;
hash.AppendData(header[..1]);
return;
}
var bytes = Encoding.UTF8.GetBytes(value);
header[0] = 1;
BinaryPrimitives.WriteInt32BigEndian(header[1..], bytes.Length);
hash.AppendData(header);
hash.AppendData(bytes);
}
}

View file

@ -57,7 +57,9 @@ namespace Elsa.Bpmn.Interchange.Services;
/// while the stored BPMN text still describes the pre-edit document. <see cref="SourceVersionCustomPropertyKey"/>
/// records the definition's own version at the moment of import for exactly this: if the current version no longer
/// matches, the stored source is stale, and exporting it would silently hand back a document that is not what the
/// caller has, which is worse than refusing.
/// caller has, which is worse than refusing. A version bump alone misses one case, though: an unpublished draft is
/// saved in place — same row, same version — so a designer save of the draft moves the graph without moving the
/// version. <see cref="SourceGraphHashCustomPropertyKey"/> closes that gap with a content hash of the graph itself.
/// </para>
/// <para>
/// <b>A whole-definition import and a document edit disagree about what else gets replaced.</b> <see cref="ImportAsync"/>
@ -66,9 +68,10 @@ namespace Elsa.Bpmn.Interchange.Services;
/// inputs, outputs, outcomes, options, tool version, read-only flag and custom properties with what that model carries.
/// <see cref="ImportDocumentAsync"/> is different: it edits the BPMN document of an <em>existing</em> definition, so
/// it passes that definition to the shared import logic's <c>preserveMetadataFrom</c> parameter, which carries all
/// of the above onto the result unchanged. Either way, only the bound activity graph and the three custom properties
/// of the above onto the result unchanged. Either way, only the bound activity graph and the four custom properties
/// this service owns (<see cref="SourceXmlCustomPropertyKey"/>, <see cref="SourceVersionCustomPropertyKey"/>,
/// <see cref="SourceProcessIdCustomPropertyKey"/>) come from the import itself.
/// <see cref="SourceProcessIdCustomPropertyKey"/>, <see cref="SourceGraphHashCustomPropertyKey"/>) come from the
/// import itself.
/// </para>
/// <para>
/// <b>Capability refusal happens here, at import, not at <c>BpmnGraph.Build</c>.</b> <see cref="BpmnCapabilityRequirements.Analyze"/>
@ -100,8 +103,9 @@ public sealed class BpmnInterchangeDocumentService(
/// <see cref="Export(WorkflowDefinition)"/> compares this against the definition's current version to tell a
/// still-current source from a stale one. The version number is what <see cref="WorkflowDefinition"/> itself
/// already exposes for "has this definition changed", so this reuses it rather than inventing a second notion of
/// change (a content hash, a timestamp) that could disagree with the versioning the rest of the system already
/// uses.
/// change for the case a version bump alone catches: a publish, or any other save that assigns a new version.
/// It does not, on its own, catch an unpublished draft saved in place — same row, same version — which is what
/// <see cref="SourceGraphHashCustomPropertyKey"/> is for.
/// </remarks>
public const string SourceVersionCustomPropertyKey = "Bpmn:SourceVersion";
@ -118,6 +122,28 @@ public sealed class BpmnInterchangeDocumentService(
/// </remarks>
public const string SourceProcessIdCustomPropertyKey = "Bpmn:SourceProcessId";
/// <summary>
/// The workflow definition custom property <see cref="ImportAsync"/> records a content hash of the definition's
/// serialized activity graph (<see cref="WorkflowDefinition.StringData"/>) under, at the moment it stores
/// <see cref="SourceXmlCustomPropertyKey"/>.
/// </summary>
/// <remarks>
/// An unpublished draft is saved in place — same row, same version — so a designer save of the draft that edits
/// a bound activity's inputs changes <see cref="WorkflowDefinition.StringData"/> without changing
/// <see cref="WorkflowDefinition"/>'s own <c>Version</c>, which <see cref="SourceVersionCustomPropertyKey"/> alone
/// cannot tell apart from no change at all. This marker closes that gap: <see cref="Export(WorkflowDefinition)"/> and
/// <see cref="ReadDocument"/> also treat the stored source as stale when the current graph's hash differs from
/// the one recorded here, in addition to the existing version check. Computed with the same hashing
/// <see cref="Elsa.Bpmn.Interchange.Endpoints.Bpmn.Document.BpmnDocumentETag"/> uses for the graph field, via
/// <see cref="BpmnContentHash"/>, so the two never disagree about what "the graph changed" means.
/// <para>
/// A definition imported before this marker existed carries no value for this key; <see cref="ResolveSourceXml"/>
/// falls back to the version-only check for it rather than refusing every definition imported under the older
/// behaviour.
/// </para>
/// </remarks>
public const string SourceGraphHashCustomPropertyKey = "Bpmn:SourceGraphHash";
/// <summary>
/// The host capabilities this deployment's BPMN runtime declares.
/// </summary>
@ -177,8 +203,9 @@ public sealed class BpmnInterchangeDocumentService(
/// When set, the definition this import must otherwise leave untouched: its name, description, variables,
/// inputs, outputs, outcomes, options, tool version, read-only flag and custom properties are carried onto the
/// imported definition as-is, and only the bound activity graph and the <see cref="SourceXmlCustomPropertyKey"/>,
/// <see cref="SourceVersionCustomPropertyKey"/> and <see cref="SourceProcessIdCustomPropertyKey"/> custom
/// properties this method owns change. This is what <see cref="ImportDocumentAsync"/> passes so the document PUT
/// <see cref="SourceVersionCustomPropertyKey"/>, <see cref="SourceProcessIdCustomPropertyKey"/> and
/// <see cref="SourceGraphHashCustomPropertyKey"/> custom properties this method owns change. This is what
/// <see cref="ImportDocumentAsync"/> passes so the document PUT
/// edits the BPMN document without silently resetting the rest of the definition; left <c>null</c> for a
/// whole-definition import, where the model built from the document alone is the intended contract.
/// </param>
@ -240,6 +267,7 @@ public sealed class BpmnInterchangeDocumentService(
persisted.CustomProperties[SourceXmlCustomPropertyKey] = xml;
persisted.CustomProperties[SourceVersionCustomPropertyKey] = persisted.Version;
persisted.CustomProperties[SourceProcessIdCustomPropertyKey] = rootDefinition.ProcessId;
persisted.CustomProperties[SourceGraphHashCustomPropertyKey] = BpmnContentHash.OfGraph(persisted.StringData);
await store.SaveAsync(persisted, cancellationToken);
}
@ -307,7 +335,7 @@ public sealed class BpmnInterchangeDocumentService(
/// properties other than the ones this service owns — is carried onto the result unchanged; see the shared
/// import logic's <c>preserveMetadataFrom</c> parameter, which this passes the existing definition to. Only the activity graph
/// and the <see cref="SourceXmlCustomPropertyKey"/>/<see cref="SourceVersionCustomPropertyKey"/>/
/// <see cref="SourceProcessIdCustomPropertyKey"/> custom properties move.
/// <see cref="SourceProcessIdCustomPropertyKey"/>/<see cref="SourceGraphHashCustomPropertyKey"/> custom properties move.
/// </remarks>
/// <param name="document">The edited document, deserialized through the library's own JSON converters.</param>
/// <param name="definitionId">The workflow definition to update.</param>
@ -383,6 +411,22 @@ public sealed class BpmnInterchangeDocumentService(
+ "would silently return a document that is not what this definition currently is.");
}
// The version check above catches a publish or any other save that assigns a new version, but an unpublished
// draft is saved in place — same row, same version — so a designer save of the draft (e.g. an edit to a bound
// activity's inputs) moves the graph without moving the version. SourceGraphHashCustomPropertyKey catches that:
// a definition imported before this marker existed carries no value for it, so it falls back to the
// version-only check above rather than refusing every definition imported under the older behaviour.
if (definition.CustomProperties.TryGetValue<string>(SourceGraphHashCustomPropertyKey, out var sourceGraphHash)
&& !string.IsNullOrEmpty(sourceGraphHash)
&& sourceGraphHash != BpmnContentHash.OfGraph(definition.StringData))
{
throw new BpmnExportUnavailableException(
$"Workflow definition '{definition.DefinitionId}' has changed since it was imported from BPMN: its activity graph no longer matches "
+ "the graph the stored source was imported against, even though its version has not changed (an unpublished draft is saved in place). "
+ "The BPMN source stored on it no longer corresponds to this definition, so exporting it would silently return a document that is "
+ "not what this definition currently is.");
}
return xml;
}

View file

@ -1,4 +1,5 @@
using Bpmn.Interchange;
using Elsa.Bpmn.Activities;
using Elsa.Bpmn.Interchange.Binding;
using Elsa.Bpmn.Interchange.Exceptions;
using Elsa.Bpmn.Interchange.IntegrationTests.Support;
@ -6,6 +7,7 @@ using Elsa.Bpmn.Interchange.Services;
using Elsa.Common.Models;
using Elsa.Extensions;
using Elsa.Testing.Shared;
using Elsa.Workflows.Activities;
using Elsa.Workflows.Management;
using Elsa.Workflows.Management.Entities;
using Elsa.Workflows.Management.Filters;
@ -51,6 +53,79 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B
Assert.Equal(stored.Version, sourceVersion);
}
[Fact(DisplayName = "Exporting a definition whose unpublished draft was saved from the designer with a changed graph is refused as stale, even though its version has not changed")]
public async Task Export_OfADefinitionSavedFromTheDesignerWithAChangedGraph_IsRefusedAsStale()
{
var xml = ReadAsset("camunda-order-process.bpmn");
var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None);
Assert.True(imported.ImportResult.Succeeded);
var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId;
// Captured as plain values, not the live entity: the in-memory store this test runs against hands back the
// same object reference on every find, so holding onto the entity itself would see the designer's edit
// through both "before" and "after" variables.
var storedBefore = await FindLatestAsync(definitionId);
var (versionBeforeSave, stringDataBeforeSave) = (storedBefore.Version, storedBefore.StringData);
await SaveDraftFromTheDesignerAsync(definitionId);
var storedAfterSave = await FindLatestAsync(definitionId);
// Saved in place: same version, only the graph moved.
Assert.Equal(versionBeforeSave, storedAfterSave.Version);
Assert.NotEqual(stringDataBeforeSave, storedAfterSave.StringData);
var exception = Assert.Throws<BpmnExportUnavailableException>(() => DocumentService.Export(storedAfterSave));
Assert.Contains("has changed since it was imported", exception.Message);
var documentException = Assert.Throws<BpmnExportUnavailableException>(() => DocumentService.ReadDocument(storedAfterSave));
Assert.Contains("has changed since it was imported", documentException.Message);
}
[Fact(DisplayName = "Exporting a definition whose unpublished draft was saved from the designer with no graph change still succeeds")]
public async Task Export_OfADefinitionSavedFromTheDesignerWithNoGraphChange_StillSucceeds()
{
var xml = ReadAsset("camunda-order-process.bpmn");
var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None);
Assert.True(imported.ImportResult.Succeeded);
var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId;
// Round-trips the draft through the designer's save path unedited, so the graph hash still matches.
var draft = await DefinitionPublisher.GetDraftAsync(definitionId, VersionOptions.Latest);
Assert.NotNull(draft);
await DefinitionPublisher.SaveDraftAsync(draft!);
var stored = await FindLatestAsync(definitionId);
var bytes = DocumentService.Export(stored);
Assert.NotEmpty(bytes);
}
[Fact(DisplayName = "A definition imported before the graph-hash marker existed still exports after a designer save, on version alone")]
public async Task Export_OfADefinitionWithoutAGraphHashMarker_FallsBackToVersionOnlyAfterADesignerSave()
{
var xml = ReadAsset("camunda-order-process.bpmn");
var imported = await DocumentService.ImportAsync(xml, definitionId: null, name: null, processId: null, CancellationToken.None);
Assert.True(imported.ImportResult.Succeeded);
var definitionId = imported.ImportResult.WorkflowDefinition.DefinitionId;
// Simulates a definition imported before this marker existed.
var stored = await FindLatestAsync(definitionId);
stored.CustomProperties.Remove(BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey);
await DefinitionStore.SaveAsync(stored);
await SaveDraftFromTheDesignerAsync(definitionId);
var storedAfterSave = await FindLatestAsync(definitionId);
Assert.False(storedAfterSave.CustomProperties.ContainsKey(BpmnInterchangeDocumentService.SourceGraphHashCustomPropertyKey));
var bytes = DocumentService.Export(storedAfterSave);
Assert.NotEmpty(bytes);
}
[Fact(DisplayName = "Exporting a definition whose custom properties no longer carry the BPMN source is refused, naming that as the reason")]
public async Task Export_OfADefinitionWithoutStoredSource_IsRefused()
{
@ -171,6 +246,24 @@ public class BpmnExportAvailabilityTests(ITestOutputHelper testOutputHelper) : B
return definition!;
}
/// <summary>
/// Saves the latest draft of <paramref name="definitionId"/> the way the workflow-definition save endpoint
/// Studio's designer calls does — <see cref="IWorkflowDefinitionPublisher.GetDraftAsync(string, VersionOptions, CancellationToken)"/>
/// then <see cref="IWorkflowDefinitionPublisher.SaveDraftAsync"/> — with the bound activity's text edited, same
/// as <c>BpmnInterchangeEndpointTests.SaveDraftFromTheDesignerAsync</c>.
/// </summary>
private async Task SaveDraftFromTheDesignerAsync(string definitionId)
{
var draft = await DefinitionPublisher.GetDraftAsync(definitionId, VersionOptions.Latest);
Assert.NotNull(draft);
var root = Assert.IsType<BpmnProcess>(ActivitySerializer.Deserialize(draft!.StringData!));
Assert.Single(root.Activities.OfType<WriteLine>()).Text = new("Notifying the warehouse, edited in the designer");
draft.StringData = ActivitySerializer.Serialize(root);
await DefinitionPublisher.SaveDraftAsync(draft);
}
/// <summary>
/// Mirrors the one line of the real <see cref="Elsa.Workflows.Management.Services.WorkflowDefinitionImporter"/>
/// this test cares about — <c>draft.CustomProperties = model.CustomProperties ?? new Dictionary&lt;...&gt;()</c> —

View file

@ -2,6 +2,7 @@ using Elsa.Bpmn.Interchange.IntegrationTests.Support;
using Elsa.Bpmn.Interchange.Services;
using Elsa.Extensions;
using Elsa.Testing.Shared;
using Elsa.Workflows;
using Elsa.Workflows.Management;
using Microsoft.Extensions.DependencyInjection;
using Xunit.Abstractions;
@ -23,12 +24,25 @@ public abstract class BpmnInterchangeTestBase : IAsyncLifetime
DocumentService = _services.GetRequiredService<BpmnInterchangeDocumentService>();
DefinitionStore = _services.GetRequiredService<IWorkflowDefinitionStore>();
DefinitionPublisher = _services.GetRequiredService<IWorkflowDefinitionPublisher>();
ActivitySerializer = _services.GetRequiredService<IActivitySerializer>();
}
protected BpmnInterchangeDocumentService DocumentService { get; }
protected IWorkflowDefinitionStore DefinitionStore { get; }
/// <summary>
/// Resolves the same <see cref="IWorkflowDefinitionPublisher"/> the workflow-definition save endpoint the
/// designer uses calls, so a test can save a draft the way the designer does:
/// <see cref="IWorkflowDefinitionPublisher.GetDraftAsync(string, Elsa.Common.Models.VersionOptions, CancellationToken)"/>
/// then <see cref="IWorkflowDefinitionPublisher.SaveDraftAsync"/>.
/// </summary>
protected IWorkflowDefinitionPublisher DefinitionPublisher { get; }
/// <summary>Resolves activities out of a draft's <c>StringData</c> so a test can edit one before saving it back.</summary>
protected IActivitySerializer ActivitySerializer { get; }
public Task InitializeAsync() => _services.PopulateRegistriesAsync();
public Task DisposeAsync() => Task.CompletedTask;