Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/gitleaks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,7 @@ jobs:
env:
# GITLEAKS_LICENSE only required for private/org repos; public OSS is free.
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
# No project-level gitleaks config. Default ruleset is sufficient; fake
# credentials in integration test factories are suppressed by inline
# `// gitleaks:allow` markers on the literal lines. See CLAUDE.md
# "Testing → fake credentials in test fixtures."
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ These are always-on. Deeper guidance (modern EF features, transactions, caching
- **Test structure — AAA with narrative comments.** Every test is structured as **Arrange → Act → Assert** with `// ARRANGE`, `// ACT`, `// ASSERT` markers (all caps, em-dash explanation on the same line). Each phase carries a *story comment*: explain what's being set up and *why it matters*, what's being called, and what each assertion is verifying. A junior dev should be able to read a single test top-to-bottom and understand the contract being checked + the failure mode being guarded against — without having to read the SUT first. When the ASSERT phase verifies multiple invariants, number them and explain why each matters (especially for security boundaries, idempotency guards, and ordering-sensitive operations like cache-after-save). Trivial happy-path tests can be shorter; security/concurrency/idempotency tests get the full story. Reference templates: [ProductAuthorizationTests.cs](tests/CatalogService.Tests.Integration/ProductAuthorizationTests.cs) (IDOR-prevention + rejected-write invariants), [PaymentFailedHandlerTests.cs](tests/OrderService.Tests.Unit/Application/PaymentFailedHandlerTests.cs) (idempotency under at-least-once delivery), [GetShipmentByOrderHandlerTests.cs](tests/ShippingService.Tests.Unit/Application/GetShipmentByOrderHandlerTests.cs) (IDOR-prevention pattern, unit-tier).
- Integration tests with Testcontainers for infrastructure — `tests/{Service}.Tests.Integration`, booting the real API via `WebApplicationFactory<Program>`. **CatalogService** slice (Postgres + Redis: caching + concurrency token) and **OrderService** slice (SQL Server + Wolverine stubbed transport: outbox, saga handlers, `RowVersion` token) exist; pattern documented in each project's README.
- **Integration tests need Docker.** On macOS, Docker Desktop's socket is at `~/.docker/run/docker.sock`, not `/var/run/docker.sock` — Testcontainers fails fast with `DockerUnavailableException` unless `DOCKER_HOST` points there (or Docker Desktop's "default Docker socket" toggle is on). CI runners have it at the standard path.
- **Fake credentials in test fixtures are suppressed with inline `// gitleaks:allow` markers — there is no project-level gitleaks config.** Some factory fakes have to be protocol-syntax-valid: the fake Azure Service Bus connection string in `OrderApiFactory.cs` / `PaymentApiFactory.cs` / `ShippingApiFactory.cs` is required because `Program.cs` parses `UseAzureServiceBus(GetConnectionString("messaging"))` eagerly at registration time, *before* `DisableAllExternalWolverineTransports()` stubs the transport. The convention: keep the high-entropy base64-encoded **self-labeling** literal (the project's fake `SharedAccessKey` base64-decodes to `fake-shared-key-for-testing-only`) and add `// gitleaks:allow` to the end of the line containing the literal. Gitleaks' default `generic-api-key` rule fires on the entropy of `key=value` strings regardless of value content, so realistic-looking fakes get flagged at PR-scan time. The fix is the **inline marker**, not lowering the fake's entropy below scanner thresholds. **Why no `.gitleaks.toml` lives at repo root:** a path+regex `[[allowlists]]` block was tried and removed — global `[[allowlists]]` requires gitleaks ≥ 8.25.0 (the action ships 8.24.x) and `MatchCondition` defaults to OR not AND, so the block both failed to fire on the pinned scanner version AND would have over-matched if it did. The inline marker is reliable across all 8.x versions, scoped to the exact line, and self-documenting at the call site. Pasting a real key into the same file still trips because the marker only suppresses THAT specific finding line, and pasting the fake literal anywhere without the marker still trips. Do NOT also reproduce the high-entropy literal in CLAUDE.md or any other prose — the scanner walks every diff, including documentation. **The diff-range residue trap:** gitleaks scans every commit in the PR range, not just HEAD, so a fix-in-a-later-commit doesn't suppress findings introduced un-marked in an earlier commit — squash the branch if you hit this. Reference: any of the three factory files for the marker pattern; [.github/workflows/gitleaks.yml](.github/workflows/gitleaks.yml) for the CI wiring.
- **IDOR test is required for every new endpoint that returns or mutates a scoped entity.** Add an integration test that authenticates as buyer X, requests a resource owned by buyer Y, and asserts the response is 404 (NOT 200, NOT 403 — see Security Requirements). The absence of such a test is exactly how the original `GET /api/v1/orders/{id}` IDOR survived undetected for the lifetime of the codebase. A `dotnet build` clean and unit tests passing aren't sufficient — *authorization behavior is only proven by an authorization-failure test*.
- **Outbox-in-non-handler test.** Code paths that publish events from outside a Wolverine handler (BackgroundService sweepers, recovery jobs) need an integration test that asserts a row appears in `wolverine.outgoing_envelopes` in the same transaction as the entity write. See the outbox-outside-handler trap in Observability → Transactional Outbox. The PaymentRecoveryJob outbox bug survived because no test asserted that the staged envelope actually persisted.
- **Handlers resolved directly in tests must be DI-registered.** If an integration test does `scope.ServiceProvider.GetRequiredService<MyHandler>()`, the handler must be `AddScoped<MyHandler>()`'d in `AddXInfrastructure`. Wolverine's handler-discovery does not populate `IServiceCollection` — it builds its own internal map. See "Communication Patterns → Wolverine handler discovery is NOT DI registration" for the full mechanism. Failure mode: `InvalidOperationException: No service for type 'X.Handler' has been registered` at first test run. Catch this at PR review time, not in CI — `/check-rules` audit pattern for the test diff: every `GetRequiredService<*Handler>()` line needs a matching `AddScoped<*Handler>()` in DI.
Expand Down
2 changes: 2 additions & 0 deletions NextAurora.slnx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
<Project Path="tests/NotificationService.Tests.Unit/NotificationService.Tests.Unit.csproj" />
<Project Path="tests/CatalogService.Tests.Integration/CatalogService.Tests.Integration.csproj" />
<Project Path="tests/OrderService.Tests.Integration/OrderService.Tests.Integration.csproj" />
<Project Path="tests/PaymentService.Tests.Integration/PaymentService.Tests.Integration.csproj" />
<Project Path="tests/ShippingService.Tests.Integration/ShippingService.Tests.Integration.csproj" />
</Folder>
<Folder Name="/Benchmarks/">
<Project Path="benchmarks/NextAurora.Benchmarks/NextAurora.Benchmarks.csproj" />
Expand Down
14 changes: 13 additions & 1 deletion PaymentService/Endpoints/PaymentEndpoints.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Security.Claims;
using PaymentService.Features;
using Wolverine;

Expand Down Expand Up @@ -31,8 +32,19 @@ public static void MapPaymentEndpoints(this WebApplication app)
// call lives in PaymentProcessingRequestedHandler, which runs on a Wolverine worker.
// The Location header points at /api/v1/payments/{id} for the caller to poll status.
// See CLAUDE.md "Performance Rules → Long-running work belongs on the message bus".
group.MapPost("/process", async (ProcessPaymentCommand command, IMessageBus bus, CancellationToken ct) =>
//
// BuyerId is derived from the JWT NameIdentifier claim, NOT the request body.
// The HTTP body is ProcessPaymentRequest (no BuyerId field); the internal
// ProcessPaymentCommand sets BuyerId from the trusted claim. Without this, an
// authenticated buyer could submit any BuyerId in the body and attribute payments
// to other buyers. See CLAUDE.md "Security Requirements → Server-controlled fields".
group.MapPost("/process", async (ProcessPaymentRequest body, HttpContext httpContext, IMessageBus bus, CancellationToken ct) =>
{
var sub = httpContext.User.FindFirst(ClaimTypes.NameIdentifier)?.Value;
if (sub is null || !Guid.TryParse(sub, out var buyerId))
return Results.Unauthorized();

var command = new ProcessPaymentCommand(body.OrderId, body.Amount, body.Currency, buyerId);
var paymentId = await bus.InvokeAsync<Guid>(command, ct);
return Results.Accepted($"/api/v1/payments/{paymentId}", new { Id = paymentId });
}).RequireRateLimiting("payments").RequireAuthorization();
Expand Down
15 changes: 15 additions & 0 deletions PaymentService/Features/ProcessPayment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,21 @@ namespace PaymentService.Features;
/// <see cref="IPaymentGateway"/> XML doc for provider semantics.</item>
/// </list></para>
/// </summary>
/// <summary>
/// HTTP request body for <c>POST /api/v1/payments/process</c>. <b>Does not include BuyerId</b> —
/// the endpoint reads the authenticated buyer from the JWT <c>NameIdentifier</c> claim and
/// constructs the internal <see cref="ProcessPaymentCommand"/>. Trusting <c>BuyerId</c> from the
/// request body would let any authenticated buyer mint payments attributed to other buyers
/// (CWE-639 / mass-assignment). See CLAUDE.md "Security Requirements → Server-controlled fields
/// are computed server-side."
/// </summary>
public record ProcessPaymentRequest(Guid OrderId, decimal Amount, string Currency);

/// <summary>
/// Internal command. <c>BuyerId</c> is set by the HTTP endpoint from the JWT claim, or by the
/// saga (<see cref="OrderPlacedHandler"/>) from the trusted <c>OrderPlacedEvent</c> — never
/// from a client-controlled HTTP body.
/// </summary>
public record ProcessPaymentCommand(Guid OrderId, decimal Amount, string Currency, Guid BuyerId);

public class ProcessPaymentCommandValidator : AbstractValidator<ProcessPaymentCommand>
Expand Down
8 changes: 6 additions & 2 deletions tests/OrderService.Tests.Integration/OrderApiFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,14 @@ protected override void ConfigureWebHost(IWebHostBuilder builder)

// Syntactically-valid Azure Service Bus connection string. Never used over the wire —
// DisableAllExternalWolverineTransports below routes outgoing messages to local stubs —
// but Wolverine's UseAzureServiceBus(...) registration parses it eagerly.
// but Wolverine's UseAzureServiceBus(...) registration parses it eagerly. SharedAccessKey
// base64-decodes to "fake-shared-key-for-testing-only". The inline `gitleaks:allow`
// marker on the literal line is the suppressor. There is no project-level gitleaks
// config (global [[allowlists]] needs gitleaks 8.25+, runner ships 8.24.x); the inline
// marker is the load-bearing mechanism. See CLAUDE.md.
builder.UseSetting(
"ConnectionStrings:messaging",
"Endpoint=sb://fake.servicebus.windows.net/;SharedAccessKeyName=fake;SharedAccessKey=ZmFrZS1zaGFyZWQta2V5LWZvci10ZXN0aW5nLW9ubHk=");
"Endpoint=sb://fake.servicebus.windows.net/;SharedAccessKeyName=fake;SharedAccessKey=ZmFrZS1zaGFyZWQta2V5LWZvci10ZXN0aW5nLW9ubHk="); // gitleaks:allow

builder.ConfigureTestServices(services =>
{
Expand Down
119 changes: 119 additions & 0 deletions tests/PaymentService.Tests.Integration/PaymentApiFactory.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
using Microsoft.AspNetCore.Authentication;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc.Testing;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using NSubstitute;
using PaymentService.Domain;
using Testcontainers.MsSql;
using Wolverine;
using Xunit;

namespace PaymentService.Tests.Integration;

/// <summary>
/// Boots the real PaymentService API in-process against a throwaway SQL Server container, with
/// Wolverine's external transports stubbed so the Acceptor + Gateway handlers and the
/// transactional outbox run locally against a real DB.
///
/// <para>
/// <b>What this exercises that unit tests can't:</b> the split <c>ProcessPaymentHandler</c>
/// (Acceptor) → <c>PaymentProcessingRequested</c> → <c>PaymentProcessingRequestedHandler</c>
/// (Gateway) flow running over Wolverine's local queue + middleware chain (FluentValidation,
/// AutoApplyTransactions, ContextPropagation), the outbox staging the terminal event in the same
/// EF transaction as the entity write, EF migrations applying to real SQL Server, the
/// <c>RowVersion</c> concurrency token, and the OrderId-uniqueness idempotency guard.
/// </para>
/// <para>
/// <b>Why stub the transport instead of the ASB emulator:</b> the outbox-staging guarantee and
/// the handler logic are what unit tests can't reach. The internal <c>PaymentProcessingRequested</c>
/// message has no external routing, so it stays on the in-process local queue and is fully
/// exercised here; only the outbound <c>PaymentCompletedEvent</c>/<c>PaymentFailedEvent</c> wire
/// hop is stubbed. See <c>docs/STATUS.md</c>.
/// </para>
/// <para>
/// <b>Why a fake "messaging" connection string:</b> <c>Program.cs</c> calls
/// <c>UseAzureServiceBus(GetConnectionString("messaging")!)</c>, parsed eagerly at registration
/// even with external transports disabled — so it must be syntactically valid ASB.
/// </para>
/// <para>
/// <b>Why stub <c>IPaymentGateway</c>:</b> the Gateway handler calls the Stripe gateway. Tests
/// configure the stub to return success or failure to drive the Completed / Failed branches
/// without touching a real payment provider.
/// </para>
/// </summary>
public sealed class PaymentApiFactory : WebApplicationFactory<Program>, IAsyncLifetime
{
private readonly MsSqlContainer _sqlServer =
new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-latest").Build();

/// <summary>The stub injected in place of <c>StripePaymentGateway</c>. Tests configure it.</summary>
public IPaymentGateway Gateway { get; } = Substitute.For<IPaymentGateway>();

public async Task InitializeAsync()
{
await _sqlServer.StartAsync();
}

Task IAsyncLifetime.DisposeAsync() => DisposeAsync().AsTask();

public override async ValueTask DisposeAsync()
{
// Dispose the host BEFORE the SQL container so Wolverine's durable background agents
// (outbox dispatcher, recovery sweeper) stop polling the wolverine.* tables before the
// DB is torn down — otherwise their heartbeats hit "connection refused" and crash the
// test host AFTER all tests passed. Swallow the cancellation that a slow shutdown can
// surface (a delayed background-service stop isn't a correctness signal). See the
// matching note in OrderApiFactory.
try
{
await base.DisposeAsync();
}
catch (OperationCanceledException)
{
// Intentional swallow — TaskCanceledException derives from OperationCanceledException.
}

await _sqlServer.DisposeAsync();
}

protected override void ConfigureWebHost(IWebHostBuilder builder)
{
ArgumentNullException.ThrowIfNull(builder);

// Disable Wolverine AutoProvision — against the fake ASB string it would hang trying to
// provision topics/subscriptions at startup. DisableAllExternalWolverineTransports below
// handles routing; this handles the broker-provisioning that runs earlier.
builder.UseSetting("Wolverine:AutoProvision", "false");

// Real SQL Server for the payments DB + Wolverine outbox tables.
builder.UseSetting("ConnectionStrings:payments-db", _sqlServer.GetConnectionString());

// Syntactically-valid ASB connection string — parsed eagerly, never used over the wire.
// SharedAccessKey base64-decodes to "fake-shared-key-for-testing-only". The inline
// `gitleaks:allow` marker on the literal line is the suppressor. There is no project-level
// gitleaks config (global [[allowlists]] needs gitleaks 8.25+, runner ships 8.24.x); the
// inline marker is the load-bearing mechanism. See CLAUDE.md.
builder.UseSetting(
"ConnectionStrings:messaging",
"Endpoint=sb://fake.servicebus.windows.net/;SharedAccessKeyName=fake;SharedAccessKey=ZmFrZS1zaGFyZWQta2V5LWZvci10ZXN0aW5nLW9ubHk="); // gitleaks:allow

builder.ConfigureTestServices(services =>
{
// Always-succeeds auth so .RequireAuthorization() on /payments/process passes.
services.AddAuthentication(TestAuthHandler.SchemeName)
.AddScheme<AuthenticationSchemeOptions, TestAuthHandler>(TestAuthHandler.SchemeName, _ => { });

// Stub the Stripe gateway — tests drive Completed/Failed via the configured result.
services.AddSingleton<IPaymentGateway>(Gateway);

// Disable external ASB listeners/senders. Outgoing events route to in-process stubs
// (still through the outbox + middleware chain); the internal PaymentProcessingRequested
// message stays on the local queue and runs end-to-end.
services.DisableAllExternalWolverineTransports();
});
}

/// <summary>Opens a DI scope — callers resolve <c>PaymentDbContext</c> for direct DB setup/assertions.</summary>
public AsyncServiceScope CreateDbScope() => Services.CreateAsyncScope();
}
Loading
Loading