From 8af74f4be28a87f519b5d94eea7f649b956569ba Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:22:16 -0300 Subject: [PATCH] refactor(webhooks): rename misleading SecretHash to ProtectedSecret WebhookSubscription.SecretHash stores the per-subscription signing secret encrypted at rest via ASP.NET Data Protection (IWebhookSecretProtector.Protect), then recovered with Unprotect to sign outbound HMAC payloads. The name "SecretHash" wrongly implies a one-way, irreversible hash, misrepresenting the value's security properties and inviting mishandling. - Rename the persisted property/column to ProtectedSecret (matches the existing protectedSecret local in CreateWebhookSubscriptionCommandHandler) with a RenameColumn migration (reversible, no data loss). - Rename IWebhookDeliveryService.DeliverAsync secretHash parameter to signingSecret: there the value is already unprotected plaintext used for signing, matching the signingSecret local in WebhookDispatchJob. No public contract change (the secret is write-only via command). --- ...ookSecretHashToProtectedSecret.Designer.cs | 119 ++++++++++++++++++ ...enameWebhookSecretHashToProtectedSecret.cs | 30 +++++ .../Webhooks/WebhookDbContextModelSnapshot.cs | 4 +- .../Data/WebhookSubscriptionConfiguration.cs | 2 +- .../Domain/WebhookSubscription.cs | 6 +- .../TestWebhookSubscriptionCommandHandler.cs | 2 +- .../Services/IWebhookDeliveryService.cs | 2 +- .../Services/WebhookDeliveryService.cs | 6 +- .../Services/WebhookDispatchJob.cs | 2 +- .../Tests/Webhooks/WebhookDeliveryTests.cs | 2 +- .../WebhookSubscriptionDomainTests.cs | 18 +-- .../Domain/WebhookSubscriptionTests.cs | 10 +- 12 files changed, 176 insertions(+), 27 deletions(-) create mode 100644 src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.Designer.cs create mode 100644 src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.cs diff --git a/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.Designer.cs b/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.Designer.cs new file mode 100644 index 0000000000..1a4e0e6b89 --- /dev/null +++ b/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.Designer.cs @@ -0,0 +1,119 @@ +// +using System; +using FSH.Modules.Webhooks.Data; +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Infrastructure; +using Microsoft.EntityFrameworkCore.Migrations; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; +using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata; + +#nullable disable + +namespace FSH.Starter.Migrations.PostgreSQL.Webhooks +{ + [DbContext(typeof(WebhookDbContext))] + [Migration("20260624152035_RenameWebhookSecretHashToProtectedSecret")] + partial class RenameWebhookSecretHashToProtectedSecret + { + /// + protected override void BuildTargetModel(ModelBuilder modelBuilder) + { +#pragma warning disable 612, 618 + modelBuilder + .HasDefaultSchema("webhooks") + .HasAnnotation("ProductVersion", "10.0.8") + .HasAnnotation("Relational:MaxIdentifierLength", 63); + + NpgsqlModelBuilderExtensions.UseIdentityByDefaultColumns(modelBuilder); + + modelBuilder.Entity("FSH.Modules.Webhooks.Domain.WebhookDelivery", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("AttemptCount") + .HasColumnType("integer"); + + b.Property("AttemptedAtUtc") + .HasColumnType("timestamp with time zone"); + + b.Property("ErrorMessage") + .HasMaxLength(4096) + .HasColumnType("character varying(4096)"); + + b.Property("EventType") + .IsRequired() + .HasMaxLength(256) + .HasColumnType("character varying(256)"); + + b.Property("HttpStatusCode") + .HasColumnType("integer"); + + b.Property("PayloadJson") + .IsRequired() + .HasColumnType("text"); + + b.Property("SubscriptionId") + .HasColumnType("uuid"); + + b.Property("Success") + .HasColumnType("boolean"); + + b.Property("TenantId") + .IsRequired() + .HasColumnType("text"); + + b.HasKey("Id"); + + b.HasIndex("AttemptedAtUtc"); + + b.HasIndex("SubscriptionId"); + + b.ToTable("Deliveries", "webhooks"); + + b.HasAnnotation("Finbuckle:MultiTenant", true); + }); + + modelBuilder.Entity("FSH.Modules.Webhooks.Domain.WebhookSubscription", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("uuid"); + + b.Property("CreatedAtUtc") + .HasColumnType("timestamp with time zone"); + + b.Property("EventsCsv") + .IsRequired() + .HasMaxLength(4096) + .HasColumnType("character varying(4096)"); + + b.Property("IsActive") + .HasColumnType("boolean"); + + b.Property("ProtectedSecret") + .HasMaxLength(512) + .HasColumnType("character varying(512)"); + + b.Property("TenantId") + .IsRequired() + .HasColumnType("text"); + + b.Property("Url") + .IsRequired() + .HasMaxLength(2048) + .HasColumnType("character varying(2048)"); + + b.HasKey("Id"); + + b.HasIndex("IsActive"); + + b.ToTable("Subscriptions", "webhooks"); + + b.HasAnnotation("Finbuckle:MultiTenant", true); + }); +#pragma warning restore 612, 618 + } + } +} diff --git a/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.cs b/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.cs new file mode 100644 index 0000000000..7e6d8840aa --- /dev/null +++ b/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/20260624152035_RenameWebhookSecretHashToProtectedSecret.cs @@ -0,0 +1,30 @@ +using Microsoft.EntityFrameworkCore.Migrations; + +#nullable disable + +namespace FSH.Starter.Migrations.PostgreSQL.Webhooks +{ + /// + public partial class RenameWebhookSecretHashToProtectedSecret : Migration + { + /// + protected override void Up(MigrationBuilder migrationBuilder) + { + migrationBuilder.RenameColumn( + name: "SecretHash", + schema: "webhooks", + table: "Subscriptions", + newName: "ProtectedSecret"); + } + + /// + protected override void Down(MigrationBuilder migrationBuilder) + { + migrationBuilder.RenameColumn( + name: "ProtectedSecret", + schema: "webhooks", + table: "Subscriptions", + newName: "SecretHash"); + } + } +} diff --git a/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/WebhookDbContextModelSnapshot.cs b/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/WebhookDbContextModelSnapshot.cs index 2e7dd140a7..2092316891 100644 --- a/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/WebhookDbContextModelSnapshot.cs +++ b/src/Host/FSH.Starter.Migrations.PostgreSQL/Webhooks/WebhookDbContextModelSnapshot.cs @@ -18,7 +18,7 @@ protected override void BuildModel(ModelBuilder modelBuilder) #pragma warning disable 612, 618 modelBuilder .HasDefaultSchema("webhooks") - .HasAnnotation("ProductVersion", "10.0.5") + .HasAnnotation("ProductVersion", "10.0.8") .HasAnnotation("Relational:MaxIdentifierLength", 63); NpgsqlModelBuilderExtensions.UseIdentityByDefaultColumns(modelBuilder); @@ -89,7 +89,7 @@ protected override void BuildModel(ModelBuilder modelBuilder) b.Property("IsActive") .HasColumnType("boolean"); - b.Property("SecretHash") + b.Property("ProtectedSecret") .HasMaxLength(512) .HasColumnType("character varying(512)"); diff --git a/src/Modules/Webhooks/Modules.Webhooks/Data/WebhookSubscriptionConfiguration.cs b/src/Modules/Webhooks/Modules.Webhooks/Data/WebhookSubscriptionConfiguration.cs index 3a017772e7..9fe89d08fe 100644 --- a/src/Modules/Webhooks/Modules.Webhooks/Data/WebhookSubscriptionConfiguration.cs +++ b/src/Modules/Webhooks/Modules.Webhooks/Data/WebhookSubscriptionConfiguration.cs @@ -15,7 +15,7 @@ public void Configure(EntityTypeBuilder builder) builder.HasKey(x => x.Id); builder.Property(x => x.Url).IsRequired().HasMaxLength(2048); builder.Property(x => x.EventsCsv).IsRequired().HasMaxLength(4096); - builder.Property(x => x.SecretHash).HasMaxLength(512); + builder.Property(x => x.ProtectedSecret).HasMaxLength(512); builder.HasIndex(x => x.IsActive); builder.Ignore(x => x.DomainEvents); } diff --git a/src/Modules/Webhooks/Modules.Webhooks/Domain/WebhookSubscription.cs b/src/Modules/Webhooks/Modules.Webhooks/Domain/WebhookSubscription.cs index 7d52929514..b3d8f485a7 100644 --- a/src/Modules/Webhooks/Modules.Webhooks/Domain/WebhookSubscription.cs +++ b/src/Modules/Webhooks/Modules.Webhooks/Domain/WebhookSubscription.cs @@ -6,13 +6,13 @@ public sealed class WebhookSubscription : BaseEntity { public string Url { get; private set; } = default!; public string EventsCsv { get; private set; } = default!; - public string? SecretHash { get; private set; } + public string? ProtectedSecret { get; private set; } public bool IsActive { get; private set; } = true; public DateTime CreatedAtUtc { get; private set; } private WebhookSubscription() { } - public static WebhookSubscription Create(string url, string[] events, string? secretHash) + public static WebhookSubscription Create(string url, string[] events, string? protectedSecret) { ArgumentNullException.ThrowIfNull(url); ArgumentNullException.ThrowIfNull(events); @@ -22,7 +22,7 @@ public static WebhookSubscription Create(string url, string[] events, string? se Id = Guid.CreateVersion7(), Url = url, EventsCsv = string.Join(',', events), - SecretHash = secretHash, + ProtectedSecret = protectedSecret, IsActive = true, CreatedAtUtc = TimeProvider.System.GetUtcNow().UtcDateTime }; diff --git a/src/Modules/Webhooks/Modules.Webhooks/Features/v1/TestWebhookSubscription/TestWebhookSubscriptionCommandHandler.cs b/src/Modules/Webhooks/Modules.Webhooks/Features/v1/TestWebhookSubscription/TestWebhookSubscriptionCommandHandler.cs index 2be6f674df..92b97bc227 100644 --- a/src/Modules/Webhooks/Modules.Webhooks/Features/v1/TestWebhookSubscription/TestWebhookSubscriptionCommandHandler.cs +++ b/src/Modules/Webhooks/Modules.Webhooks/Features/v1/TestWebhookSubscription/TestWebhookSubscriptionCommandHandler.cs @@ -33,7 +33,7 @@ public async ValueTask Handle(TestWebhookSubscriptionCommand command, Canc await deliveryService.DeliverAsync( subscription.Id, subscription.Url, - secretProtector.Unprotect(subscription.SecretHash), + secretProtector.Unprotect(subscription.ProtectedSecret), "webhook.test", testPayload, cancellationToken).ConfigureAwait(false); diff --git a/src/Modules/Webhooks/Modules.Webhooks/Services/IWebhookDeliveryService.cs b/src/Modules/Webhooks/Modules.Webhooks/Services/IWebhookDeliveryService.cs index 7e896f3433..615d6430ac 100644 --- a/src/Modules/Webhooks/Modules.Webhooks/Services/IWebhookDeliveryService.cs +++ b/src/Modules/Webhooks/Modules.Webhooks/Services/IWebhookDeliveryService.cs @@ -2,5 +2,5 @@ namespace FSH.Modules.Webhooks.Services; public interface IWebhookDeliveryService { - Task DeliverAsync(Guid subscriptionId, string url, string? secretHash, string eventType, string payloadJson, CancellationToken ct = default); + Task DeliverAsync(Guid subscriptionId, string url, string? signingSecret, string eventType, string payloadJson, CancellationToken ct = default); } diff --git a/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDeliveryService.cs b/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDeliveryService.cs index 8b926a96be..ed9c6c8642 100644 --- a/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDeliveryService.cs +++ b/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDeliveryService.cs @@ -14,7 +14,7 @@ public sealed class WebhookDeliveryService( public async Task DeliverAsync( Guid subscriptionId, string url, - string? secretHash, + string? signingSecret, string eventType, string payloadJson, CancellationToken ct = default) @@ -26,9 +26,9 @@ public async Task DeliverAsync( { using var content = new StringContent(payloadJson, Encoding.UTF8, new MediaTypeHeaderValue("application/json")); - if (!string.IsNullOrEmpty(secretHash)) + if (!string.IsNullOrEmpty(signingSecret)) { - var signature = WebhookPayloadSigner.Sign(payloadJson, secretHash); + var signature = WebhookPayloadSigner.Sign(payloadJson, signingSecret); content.Headers.Add("X-Webhook-Signature", signature); } diff --git a/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDispatchJob.cs b/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDispatchJob.cs index 095c16f9aa..1e0d331a02 100644 --- a/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDispatchJob.cs +++ b/src/Modules/Webhooks/Modules.Webhooks/Services/WebhookDispatchJob.cs @@ -103,7 +103,7 @@ public async Task DispatchAsync( try { using var content = new StringContent(payloadJson, Encoding.UTF8, new MediaTypeHeaderValue("application/json")); - var signingSecret = _secretProtector.Unprotect(subscription.SecretHash); + var signingSecret = _secretProtector.Unprotect(subscription.ProtectedSecret); if (!string.IsNullOrEmpty(signingSecret)) { content.Headers.Add("X-Webhook-Signature", WebhookPayloadSigner.Sign(payloadJson, signingSecret)); diff --git a/src/Tests/Integration.Tests/Tests/Webhooks/WebhookDeliveryTests.cs b/src/Tests/Integration.Tests/Tests/Webhooks/WebhookDeliveryTests.cs index 7150a1f792..dc26b83913 100644 --- a/src/Tests/Integration.Tests/Tests/Webhooks/WebhookDeliveryTests.cs +++ b/src/Tests/Integration.Tests/Tests/Webhooks/WebhookDeliveryTests.cs @@ -257,7 +257,7 @@ public async Task DeliverAsync_Should_RecordRowWithoutSignature_When_NoSecretCon await service.DeliverAsync( subscriptionId, "https://no-secret.invalid/hook", - secretHash: null, + signingSecret: null, eventType: "manual.event", payloadJson: "{\"manual\":true}", ct: CancellationToken.None); diff --git a/src/Tests/Integration.Tests/Tests/Webhooks/WebhookSubscriptionDomainTests.cs b/src/Tests/Integration.Tests/Tests/Webhooks/WebhookSubscriptionDomainTests.cs index 391e3ae437..4b41e96fb5 100644 --- a/src/Tests/Integration.Tests/Tests/Webhooks/WebhookSubscriptionDomainTests.cs +++ b/src/Tests/Integration.Tests/Tests/Webhooks/WebhookSubscriptionDomainTests.cs @@ -29,7 +29,7 @@ public void MatchesEvent_Should_ReturnTrue_When_EventIsInList() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "user.created", "user.updated" }, - secretHash: null); + protectedSecret: null); // Act & Assert subscription.MatchesEvent("user.updated").ShouldBeTrue(); @@ -42,7 +42,7 @@ public void GetEvents_Should_ReturnAllConfiguredEvents_When_MultipleSubscribed() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "a.one", "b.two", "c.three" }, - secretHash: null); + protectedSecret: null); // Act var events = subscription.GetEvents(); @@ -62,7 +62,7 @@ public void MatchesEvent_Should_BeCaseInsensitive_When_CasingDiffers() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "User.Created" }, - secretHash: null); + protectedSecret: null); // Act & Assert — exact-name matching ignores case. subscription.MatchesEvent("user.created").ShouldBeTrue(); @@ -76,7 +76,7 @@ public void MatchesEvent_Should_MatchAnything_When_WildcardSubscribed() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "*" }, - secretHash: null); + protectedSecret: null); // Act & Assert subscription.MatchesEvent("any.unconfigured.event").ShouldBeTrue(); @@ -90,7 +90,7 @@ public void MatchesEvent_Should_ReturnFalse_When_EventNotSubscribed() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "user.created" }, - secretHash: null); + protectedSecret: null); // Act & Assert subscription.MatchesEvent("user.deleted").ShouldBeFalse(); @@ -104,7 +104,7 @@ public void GetEvents_Should_TrimAndDropEmptyEntries_When_CsvHasWhitespaceAndBla var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { " spaced.event ", "", " ", "tight.event" }, - secretHash: null); + protectedSecret: null); // Act var events = subscription.GetEvents(); @@ -123,7 +123,7 @@ public void Deactivate_Should_SetIsActiveFalse_When_Called() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "user.created" }, - secretHash: null); + protectedSecret: null); subscription.IsActive.ShouldBeTrue(); // Act @@ -140,12 +140,12 @@ public void Create_Should_PopulateFields_And_GenerateId_When_Valid() var subscription = WebhookSubscription.Create( "https://example.com/hook", new[] { "user.created" }, - secretHash: "hashed-secret"); + protectedSecret: "protected-secret"); // Assert subscription.Id.ShouldNotBe(Guid.Empty); subscription.Url.ShouldBe("https://example.com/hook"); - subscription.SecretHash.ShouldBe("hashed-secret"); + subscription.ProtectedSecret.ShouldBe("protected-secret"); subscription.IsActive.ShouldBeTrue(); subscription.CreatedAtUtc.ShouldNotBe(default); } diff --git a/src/Tests/Webhooks.Tests/Domain/WebhookSubscriptionTests.cs b/src/Tests/Webhooks.Tests/Domain/WebhookSubscriptionTests.cs index 352ca1bffb..183731362b 100644 --- a/src/Tests/Webhooks.Tests/Domain/WebhookSubscriptionTests.cs +++ b/src/Tests/Webhooks.Tests/Domain/WebhookSubscriptionTests.cs @@ -12,22 +12,22 @@ public void Create_Should_Populate_Fields_And_Default_Active_When_Valid() var sub = WebhookSubscription.Create( "https://example.com/hook", ["user.created", "user.deleted"], - "hashed-secret"); + "protected-secret"); sub.Url.ShouldBe("https://example.com/hook"); sub.EventsCsv.ShouldBe("user.created,user.deleted"); - sub.SecretHash.ShouldBe("hashed-secret"); + sub.ProtectedSecret.ShouldBe("protected-secret"); sub.IsActive.ShouldBeTrue(); sub.Id.ShouldNotBe(Guid.Empty); sub.CreatedAtUtc.ShouldNotBe(default); } [Fact] - public void Create_Should_Allow_Null_SecretHash() + public void Create_Should_Allow_Null_ProtectedSecret() { - var sub = WebhookSubscription.Create("https://example.com", ["a"], secretHash: null); + var sub = WebhookSubscription.Create("https://example.com", ["a"], protectedSecret: null); - sub.SecretHash.ShouldBeNull(); + sub.ProtectedSecret.ShouldBeNull(); } [Fact]