fix(security): close comment IDOR, unify lockout policy, extend CSRF …

Services/AuthApi/Planora.Auth.Domain/Entities/User.cs

@@ -132,9 +132,12 @@ public void ClearEmailVerificationToken(Guid changedBy)
         MarkAsModified(changedBy);
     }
 
-    public void LockAccount()
+    public void LockAccount(int lockoutMinutes)
     {
-        LockedUntil = DateTime.UtcNow.AddMinutes(15);
+        if (lockoutMinutes <= 0)
+            throw new AuthDomainException("Lockout duration must be positive");
+
+        LockedUntil = DateTime.UtcNow.AddMinutes(lockoutMinutes);
         LockoutEndDate = LockedUntil;
         Status = UserStatus.Locked;
         AddDomainEvent(new UserLockedEvent(Id, Email.Value, LockedUntil.Value));

Services/AuthApi/Planora.Auth.Infrastructure/Persistence/Repositories/UserRepository.cs

@@ -1,9 +1,10 @@
 using Planora.Auth.Domain.Entities;
 using Planora.Auth.Domain.Events;
+using Planora.BuildingBlocks.Infrastructure.Configuration;
 
 namespace Planora.Auth.Infrastructure.Persistence.Repositories
 {
     public sealed class UserRepository : BaseRepository<User>, IUserRepository
     {
         public UserRepository(AuthDbContext context) : base(context)
         {
@@ -84,9 +85,9 @@
             if (user == null) return;
 
             user.IncrementFailedLoginAttempts();
-            if (user.FailedLoginAttempts >= 5)
+            if (user.FailedLoginAttempts >= SecurityConstants.SecurityPolicies.MaxFailedLoginAttempts)
             {
-                user.LockAccount();
+                user.LockAccount(SecurityConstants.SecurityPolicies.AccountLockoutMinutes);
             }
 
             await _context.SaveChangesAsync(cancellationToken);

Services/CategoryApi/Planora.Category.Api/Program.cs

@@ -194,6 +194,10 @@ await DatabaseStartup.EnsureReadyAsync(
                 app.UseAuthentication();
                 app.UseAuthorization();
 
+                // Double-submit CSRF check for browser cookie flows. Internal gRPC
+                // (application/grpc over HTTP/2) is exempt inside the middleware.
+                app.UseCsrfProtection();
+
                 // Swagger UI in Development / Staging only (production never exposes it)
                 app.UsePlanoraSwagger(app.Environment, documentTitle: "Planora Category API");
 

Services/CollaborationApi/Planora.Collaboration.Api/Program.cs

@@ -196,6 +196,10 @@ await DatabaseStartup.EnsureReadyAsync(
                 app.UseAuthentication();
                 app.UseAuthorization();
 
+                // Double-submit CSRF check for browser cookie flows. Internal gRPC
+                // (application/grpc over HTTP/2) is exempt inside the middleware.
+                app.UseCsrfProtection();
+
                 // Swagger UI in Development / Staging only
                 app.UsePlanoraSwagger(app.Environment, documentTitle: "Planora Collaboration API");
 

Services/CollaborationApi/Planora.Collaboration.Application/Features/Comments/Commands/UpdateComment/UpdateCommentCommandHandler.cs

@@ -14,17 +14,20 @@ public sealed class UpdateCommentCommandHandler : IRequestHandler<UpdateCommentC
         private readonly IUnitOfWork _unitOfWork;
         private readonly ICurrentUserContext _currentUserContext;
         private readonly IUserService _userService;
+        private readonly ITaskAccessService _taskAccessService;
 
         public UpdateCommentCommandHandler(
             ICommentRepository commentRepository,
             IUnitOfWork unitOfWork,
             ICurrentUserContext currentUserContext,
-            IUserService userService)
+            IUserService userService,
+            ITaskAccessService taskAccessService)
         {
             _commentRepository = commentRepository;
             _unitOfWork = unitOfWork;
             _currentUserContext = currentUserContext;
             _userService = userService;
+            _taskAccessService = taskAccessService;
         }
 
         public async Task<Result<CommentDto>> Handle(UpdateCommentCommand request, CancellationToken cancellationToken)
@@ -37,6 +40,15 @@ public async Task<Result<CommentDto>> Handle(UpdateCommentCommand request, Cance
             if (comment.TaskId != request.TaskId)
                 throw new EntityNotFoundException("Comment", request.CommentId);
 
+            // Authorise against the live task access rules owned by TodoApi (sharing/friendship),
+            // mirroring Add/Delete/Get handlers. Without this a user who authored a comment but
+            // has since lost access to the task could still edit it (IDOR/BOLA).
+            var access = await _taskAccessService.CheckCommentAccessAsync(request.TaskId, userId, cancellationToken);
+            if (!access.Exists)
+                throw new EntityNotFoundException("Task", request.TaskId);
+            if (!access.HasAccess)
+                throw new ForbiddenException("You do not have access to this task");
+
             // The task description ("Author's Note") is no longer a stored comment — it is edited
             // on the task itself (PUT /todos), so this handler only edits real user comments.
             comment.UpdateContent(request.Content, userId);

Services/MessagingApi/Planora.Messaging.Api/Program.cs

@@ -4,6 +4,7 @@
 using Planora.BuildingBlocks.Infrastructure.Filters;
 using Planora.BuildingBlocks.Infrastructure.Grpc;
 using Planora.BuildingBlocks.Infrastructure.Logging;
+using Planora.BuildingBlocks.Infrastructure.Middleware;
 using Planora.BuildingBlocks.Infrastructure.Persistence;
 using Planora.BuildingBlocks.Infrastructure.Resilience;
 using Planora.BuildingBlocks.Infrastructure.Security;
@@ -215,6 +216,10 @@ await DatabaseStartup.EnsureReadyAsync(
                 app.UseAuthentication();
                 app.UseAuthorization();
 
+                // Double-submit CSRF check for browser cookie flows. Internal gRPC
+                // (application/grpc over HTTP/2) is exempt inside the middleware.
+                app.UseCsrfProtection();
+
                 // Swagger UI in Development / Staging only
                 app.UsePlanoraSwagger(app.Environment, documentTitle: "Planora Messaging API");
 

Services/RealtimeApi/Planora.Realtime.Api/Hubs/PresenceHub.cs

@@ -34,14 +34,22 @@ public override async Task OnDisconnectedAsync(Exception? exception)
             await base.OnDisconnectedAsync(exception);
         }
 
+        // Allowed presence states. The client cannot broadcast arbitrary strings — an
+        // unbounded value would let one client push large payloads to every other client.
+        private static readonly HashSet<string> AllowedStatuses =
+            new(StringComparer.OrdinalIgnoreCase) { "online", "away", "busy", "offline" };
+
         public async Task UpdateStatus(string status)
         {
             var userId = Context.User?.FindFirst("sub")?.Value;
-            if (!string.IsNullOrEmpty(userId))
-            {
-                await Clients.Others.SendAsync("UserStatusChanged", userId, status);
-                _logger.LogInformation("User {UserId} status changed to {Status}", userId, status);
-            }
+            if (string.IsNullOrEmpty(userId))
+                return;
+
+            if (string.IsNullOrWhiteSpace(status) || !AllowedStatuses.Contains(status))
+                throw new HubException("Invalid status");
+
+            await Clients.Others.SendAsync("UserStatusChanged", userId, status);
+            _logger.LogInformation("User {UserId} status changed to {Status}", userId, status);
         }
     }
 }

Services/TodoApi/Planora.Todo.Api/Program.cs

@@ -231,6 +231,10 @@ AND character_maximum_length < 1500
                 app.UseAuthentication();
                 app.UseAuthorization();
 
+                // Double-submit CSRF check for browser cookie flows. Internal gRPC
+                // (application/grpc over HTTP/2) is exempt inside the middleware.
+                app.UseCsrfProtection();
+
                 // Swagger UI in Development / Staging only
                 app.UsePlanoraSwagger(app.Environment, documentTitle: "Planora Todo API");
 

docs/security-audit-2026-06.md

@@ -0,0 +1,108 @@
+# Security Audit — June 2026
+
+Full-project security audit of Planora: threat surface, architecture, and code.
+This document records every confirmed finding, the fixes applied in this pass, and
+the items that were investigated and deliberately left unchanged (with rationale),
+so reviewers do not "re-fix" accepted trade-offs.
+
+## Scope
+
+- Auth service (auth, sessions, tokens, 2FA, friendships)
+- Business services (Todo, Category, Messaging, Collaboration, Realtime) and gRPC contracts
+- API Gateway (Ocelot) and shared BuildingBlocks
+- Next.js frontend
+- Infrastructure and CI/CD (Docker, Fly, GitHub Actions)
+
+Methodology: five parallel domain audits, then per-finding manual verification of the
+code before any change. Findings that turned out to be false positives are listed so the
+reasoning is preserved.
+
+## Overall posture
+
+The codebase is mature. Strong, correctly-implemented controls already in place include:
+
+- PBKDF2-SHA512 password hashing (100k iterations, per-password salt, constant-time verify).
+- Refresh-token rotation with reuse detection and per-user security stamp revocation.
+- TOTP 2FA with Redis replay-prevention and recovery codes.
+- JWT validation everywhere (issuer/audience/lifetime/signing key, 30s clock skew, HS256 pinned).
+- CSRF double-submit with timing-safe comparison; access token kept in frontend memory,
+  refresh token in a path-scoped `HttpOnly; Secure; SameSite=Strict` cookie.
+- Per-user/per-IP rate limiting that is not bypassable via spoofed `X-Forwarded-For`
+  (forwarded headers are opt-in and gated on configured known proxies).
+- gRPC service-to-service auth with a shared key compared in constant time.
+- Strict security headers, log sanitization (CR/LF/control stripping), and generic 5xx errors.
+- Non-root containers, SHA-pinned actions, gitleaks + CodeQL + Trivy, SBOM with Sigstore
+  attestation, internal-only Fly services.
+
+## Findings fixed in this pass
+
+### 1. Comment update missing task-access authorization (IDOR/BOLA) — High
+`UpdateCommentCommandHandler` checked only comment authorship (inside `Comment.UpdateContent`)
+and, unlike the Add/Delete/Get handlers, never called `ITaskAccessService.CheckCommentAccessAsync`.
+A user who authored a comment but later lost access to the parent task (e.g. a share was revoked)
+could still edit that comment.
+
+Fix: the handler now resolves live task access via gRPC to TodoApi and returns
+`404` when the task is not visible / `403` when access is denied, before editing. Added a
+unit test (`UpdateComment_AuthorWithoutTaskAccess_ThrowsForbidden`).
+
+### 2. CSRF enforcement only on AuthApi — Medium (defense-in-depth)
+The documented security model requires a double-submit CSRF token on browser state-changing
+requests, but `UseCsrfProtection()` was wired only into AuthApi. Business services authenticate
+with a bearer token (not an ambient cookie), so they were not classically CSRF-exploitable, but
+the inconsistency diverged from the stated model.
+
+Fix: `UseCsrfProtection()` added after authentication in Todo, Category, Messaging, and
+Collaboration. The frontend already sends `X-CSRF-Token` on every POST/PUT/DELETE/PATCH through
+the gateway, and the middleware exempts internal gRPC (`application/grpc` over HTTP/2), so no
+legitimate traffic breaks. Realtime is intentionally excluded — its only mutating surface is the
+SignalR negotiate POST, which does not carry the CSRF header and is bearer-authenticated.
+
+### 3. Account-lockout values hardcoded and inconsistent — Medium
+`User.LockAccount()` hardcoded a 15-minute lockout while `SecurityConstants.SecurityPolicies`
+defined `AccountLockoutMinutes = 30` and `MaxFailedLoginAttempts = 5`; `UserRepository` separately
+hardcoded the `>= 5` threshold. The documented policy was not the one actually enforced.
+
+Fix: `LockAccount(int lockoutMinutes)` is now parameterized (and rejects non-positive values),
+and `UserRepository` drives both the threshold and the duration from
+`SecurityConstants.SecurityPolicies`. Single source of truth restored.
+
+### 4. PresenceHub broadcast of unvalidated client string — Low
+`PresenceHub.UpdateStatus(string status)` rebroadcast an arbitrary, unbounded client-supplied
+string to all other connected clients (an amplification vector). The hub is not currently mapped,
+but the method was hardened anyway.
+
+Fix: `status` is validated against an allow-list (`online`/`away`/`busy`/`offline`) and rejected
+with `HubException` otherwise.
+
+## Investigated and intentionally left unchanged
+
+- **Messaging conversation "IDOR"** — reported as readable cross-user conversations. Not a
+  vulnerability: the controller binds the first participant to `ICurrentUserContext.UserId` from
+  the JWT, so a caller can only read conversations they participate in. No change.
+- **Unencrypted HTTP/2 (h2c) for gRPC** (`Http2UnencryptedSupport` switch) — required for the
+  cleartext-h2c gRPC clients, which run over Fly's WireGuard-encrypted private mesh. Disabling it
+  would break production service-to-service calls without adding real confidentiality. No change.
+- **PBKDF2 iteration count (100k)** — acceptable for PBKDF2-SHA512. Cannot be raised safely without
+  a rehash-on-login migration, since the stored hash encodes no iteration count; bumping the
+  constant would invalidate every existing password. Left for a coordinated migration.
+- **Registration returns 409 on duplicate email** — enables limited account enumeration, but the
+  endpoint is rate-limited (3/min) and hiding it materially degrades signup UX. Accepted trade-off.
+- **Access-token lifetime 60m in Docker vs 15m in dev** — within industry norms; revocation is
+  handled by refresh rotation + security stamp. Configuration choice, not a flaw.
+- **Dev/local default DB credentials in committed `appsettings*.json` and `launchSettings.json`** —
+  localhost-only convenience values; real environments inject secrets via env vars / Fly secrets,
+  and docker-compose overrides the connection string entirely. Low risk; left to avoid breaking
+  local onboarding. Operators must continue to supply strong secrets in every non-dev environment.
+- **k6 GPG key installed via `curl | gpg` in perf-smoke CI / test `postgres/postgres` in CI** —
+  ephemeral, localhost-only CI containers; low real risk. Candidate for future hardening
+  (pin the key fingerprint, generate random CI passwords) but not a deployed exposure.
+- **SignalR token via query string / dev-only `unsafe-eval` CSP / analytics bearer to own backend** —
+  reviewed; these are framework requirements or first-party authenticated calls, not leaks.
+
+## Recommended follow-ups (not code changes)
+
+- Pin the k6 signing-key fingerprint and generate random CI database passwords.
+- Add a CSP `report-to`/`report-uri` endpoint to gain visibility into violations.
+- Document the production requirement to set `ForwardedHeaders:KnownProxies` to the edge range.
+- Plan a PBKDF2 rehash-on-login path so the iteration count can be raised over time.

tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/AuthLifecycleHandlerTests.cs

@@ -411,7 +411,7 @@ public async Task RefreshToken_ShouldReturnExpectedFailures()
         Assert.Equal("INVALID_REFRESH_TOKEN", inactiveResult.Error!.Code);
 
         var lockedUser = CreateUser();
-        lockedUser.LockAccount();
+        lockedUser.LockAccount(30);
         var lockedToken = new RefreshTokenEntity(lockedUser.Id, "locked", "127.0.0.1", DateTime.UtcNow.AddDays(1));
         AddRefreshTokenToUser(lockedUser, lockedToken);
         fixture.Users.Setup(x => x.GetByRefreshTokenAsync("locked", It.IsAny<CancellationToken>()))
@@ -498,7 +498,7 @@ public async Task ValidateToken_ShouldReturnInvalidForBadTokenMissingUserAndLock
         Assert.Equal(userId, missingUser.Value.UserId);
 
         var lockedUser = CreateUser();
-        lockedUser.LockAccount();
+        lockedUser.LockAccount(30);
         fixture.TokenService.Setup(x => x.ValidateAccessToken("locked")).Returns(lockedUser.Id);
         fixture.Users.Setup(x => x.GetByIdAsync(lockedUser.Id, It.IsAny<CancellationToken>())).ReturnsAsync(lockedUser);
         var locked = await handler.Handle(new ValidateTokenQuery { Token = "locked" }, CancellationToken.None);

tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/LoginCommandHandlerTests.cs

@@ -41,7 +41,7 @@ await Assert.ThrowsAsync<UnauthorizedAccessException>(() =>
                 CancellationToken.None));
 
         var lockedUser = CreateUser("locked-login@example.com");
-        lockedUser.LockAccount();
+        lockedUser.LockAccount(30);
         var locked = CreateFixture();
         locked.Users.Setup(x => x.GetByEmailAsync(lockedUser.Email, It.IsAny<CancellationToken>())).ReturnsAsync(lockedUser);
         locked.Users.Setup(x => x.GetWithRefreshTokensAsync(lockedUser.Id, It.IsAny<CancellationToken>())).ReturnsAsync(lockedUser);

tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/RequestPasswordResetCommandHandlerTests.cs

@@ -52,7 +52,7 @@ public async Task Handle_ShouldReturnSuccessWithoutToken_WhenUserDoesNotExistOrI
         missing.TokenService.Verify(x => x.GenerateToken(), Times.Never);
 
         var lockedUser = CreateUser("locked@example.com");
-        lockedUser.LockAccount();
+        lockedUser.LockAccount(30);
         var locked = CreateFixture();
         locked.Users
             .Setup(x => x.GetByEmailAsync(lockedUser.Email, It.IsAny<CancellationToken>()))

tests/Planora.UnitTests/Services/AuthApi/Domain/UserDomainTests.cs

@@ -205,7 +205,7 @@ public void AccountStateMethods_ShouldLockUnlockDeactivateAndActivate()
         user.VerifyEmail();
         var token = user.AddRefreshToken("refresh-token", "127.0.0.1", DateTime.UtcNow.AddDays(1));
 
-        user.LockAccount();
+        user.LockAccount(30);
 
         Assert.True(user.IsLocked());
         Assert.Equal(UserStatus.Locked, user.Status);

tests/Planora.UnitTests/Services/CollaborationApi/Handlers/CommentCommandHandlerTests.cs

@@ -117,6 +117,7 @@ await Assert.ThrowsAsync<ForbiddenException>(() =>
     public async Task UpdateComment_ByAuthor_Updates()
     {
         var fixture = new Fixture();
+        fixture.GrantAccess(fixture.UserId);
         var comment = Comment.Create(fixture.TaskId, fixture.UserId, "Me", "old");
         fixture.Comments.Setup(x => x.GetByIdAsync(comment.Id, It.IsAny<CancellationToken>())).ReturnsAsync(comment);
         fixture.Users
@@ -131,6 +132,22 @@ public async Task UpdateComment_ByAuthor_Updates()
         fixture.Comments.Verify(x => x.Update(comment), Times.Once);
     }
 
+    [Fact]
+    [Trait("TestType", "Security")]
+    public async Task UpdateComment_AuthorWithoutTaskAccess_ThrowsForbidden()
+    {
+        var fixture = new Fixture();
+        fixture.DenyAccess();
+        var comment = Comment.Create(fixture.TaskId, fixture.UserId, "Me", "old");
+        fixture.Comments.Setup(x => x.GetByIdAsync(comment.Id, It.IsAny<CancellationToken>())).ReturnsAsync(comment);
+
+        await Assert.ThrowsAsync<ForbiddenException>(() =>
+            fixture.UpdateHandler().Handle(
+                new UpdateCommentCommand(fixture.TaskId, comment.Id, "x"), CancellationToken.None));
+
+        fixture.Comments.Verify(x => x.Update(It.IsAny<Comment>()), Times.Never);
+    }
+
     [Fact]
     [Trait("TestType", "Security")]
     public async Task UpdateComment_WrongTask_ThrowsNotFound()
@@ -189,6 +206,6 @@ public DeleteCommentCommandHandler DeleteHandler() =>
             new(Comments.Object, UnitOfWork.Object, CurrentUser.Object, Access.Object);
 
         public UpdateCommentCommandHandler UpdateHandler() =>
-            new(Comments.Object, UnitOfWork.Object, CurrentUser.Object, Users.Object);
+            new(Comments.Object, UnitOfWork.Object, CurrentUser.Object, Users.Object, Access.Object);
     }
 }