diff --git a/Services/AuthApi/Planora.Auth.Domain/Entities/User.cs b/Services/AuthApi/Planora.Auth.Domain/Entities/User.cs index a0efa84a..6b42e10a 100644 --- a/Services/AuthApi/Planora.Auth.Domain/Entities/User.cs +++ b/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)); diff --git a/Services/AuthApi/Planora.Auth.Infrastructure/Persistence/Repositories/UserRepository.cs b/Services/AuthApi/Planora.Auth.Infrastructure/Persistence/Repositories/UserRepository.cs index e3a0b7c6..5266a183 100644 --- a/Services/AuthApi/Planora.Auth.Infrastructure/Persistence/Repositories/UserRepository.cs +++ b/Services/AuthApi/Planora.Auth.Infrastructure/Persistence/Repositories/UserRepository.cs @@ -1,5 +1,6 @@ using Planora.Auth.Domain.Entities; using Planora.Auth.Domain.Events; +using Planora.BuildingBlocks.Infrastructure.Configuration; namespace Planora.Auth.Infrastructure.Persistence.Repositories { @@ -84,9 +85,9 @@ public async Task HandleFailedLoginAsync(Guid userId, CancellationToken cancella 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); diff --git a/Services/CategoryApi/Planora.Category.Api/Program.cs b/Services/CategoryApi/Planora.Category.Api/Program.cs index a1f9db96..16ff621e 100644 --- a/Services/CategoryApi/Planora.Category.Api/Program.cs +++ b/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"); diff --git a/Services/CollaborationApi/Planora.Collaboration.Api/Program.cs b/Services/CollaborationApi/Planora.Collaboration.Api/Program.cs index 4832a956..9f9ec749 100644 --- a/Services/CollaborationApi/Planora.Collaboration.Api/Program.cs +++ b/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"); diff --git a/Services/CollaborationApi/Planora.Collaboration.Application/Features/Comments/Commands/UpdateComment/UpdateCommentCommandHandler.cs b/Services/CollaborationApi/Planora.Collaboration.Application/Features/Comments/Commands/UpdateComment/UpdateCommentCommandHandler.cs index 11d3bfee..b7a6771a 100644 --- a/Services/CollaborationApi/Planora.Collaboration.Application/Features/Comments/Commands/UpdateComment/UpdateCommentCommandHandler.cs +++ b/Services/CollaborationApi/Planora.Collaboration.Application/Features/Comments/Commands/UpdateComment/UpdateCommentCommandHandler.cs @@ -14,17 +14,20 @@ public sealed class UpdateCommentCommandHandler : IRequestHandler> Handle(UpdateCommentCommand request, CancellationToken cancellationToken) @@ -37,6 +40,15 @@ public async Task> 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); diff --git a/Services/MessagingApi/Planora.Messaging.Api/Program.cs b/Services/MessagingApi/Planora.Messaging.Api/Program.cs index 83b0006f..1e829cf1 100644 --- a/Services/MessagingApi/Planora.Messaging.Api/Program.cs +++ b/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"); diff --git a/Services/RealtimeApi/Planora.Realtime.Api/Hubs/PresenceHub.cs b/Services/RealtimeApi/Planora.Realtime.Api/Hubs/PresenceHub.cs index 271f7ea5..4580f185 100644 --- a/Services/RealtimeApi/Planora.Realtime.Api/Hubs/PresenceHub.cs +++ b/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 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); } } } diff --git a/Services/TodoApi/Planora.Todo.Api/Program.cs b/Services/TodoApi/Planora.Todo.Api/Program.cs index cd869c08..94571d85 100644 --- a/Services/TodoApi/Planora.Todo.Api/Program.cs +++ b/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"); diff --git a/docs/security-audit-2026-06.md b/docs/security-audit-2026-06.md new file mode 100644 index 00000000..0ea69fa4 --- /dev/null +++ b/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. diff --git a/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/AuthLifecycleHandlerTests.cs b/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/AuthLifecycleHandlerTests.cs index b3cec1c5..6822e982 100644 --- a/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/AuthLifecycleHandlerTests.cs +++ b/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())) @@ -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())).ReturnsAsync(lockedUser); var locked = await handler.Handle(new ValidateTokenQuery { Token = "locked" }, CancellationToken.None); diff --git a/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/LoginCommandHandlerTests.cs b/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/LoginCommandHandlerTests.cs index 4179ba55..7ffc5b2b 100644 --- a/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/LoginCommandHandlerTests.cs +++ b/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/LoginCommandHandlerTests.cs @@ -41,7 +41,7 @@ await Assert.ThrowsAsync(() => 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())).ReturnsAsync(lockedUser); locked.Users.Setup(x => x.GetWithRefreshTokensAsync(lockedUser.Id, It.IsAny())).ReturnsAsync(lockedUser); diff --git a/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/RequestPasswordResetCommandHandlerTests.cs b/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/RequestPasswordResetCommandHandlerTests.cs index 3844390f..a6083b5b 100644 --- a/tests/Planora.UnitTests/Services/AuthApi/Authentication/Handlers/RequestPasswordResetCommandHandlerTests.cs +++ b/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())) diff --git a/tests/Planora.UnitTests/Services/AuthApi/Domain/UserDomainTests.cs b/tests/Planora.UnitTests/Services/AuthApi/Domain/UserDomainTests.cs index 14913fb5..62e22463 100644 --- a/tests/Planora.UnitTests/Services/AuthApi/Domain/UserDomainTests.cs +++ b/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); diff --git a/tests/Planora.UnitTests/Services/CollaborationApi/Handlers/CommentCommandHandlerTests.cs b/tests/Planora.UnitTests/Services/CollaborationApi/Handlers/CommentCommandHandlerTests.cs index 7fccd9b0..3221a97e 100644 --- a/tests/Planora.UnitTests/Services/CollaborationApi/Handlers/CommentCommandHandlerTests.cs +++ b/tests/Planora.UnitTests/Services/CollaborationApi/Handlers/CommentCommandHandlerTests.cs @@ -117,6 +117,7 @@ await Assert.ThrowsAsync(() => 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())).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())).ReturnsAsync(comment); + + await Assert.ThrowsAsync(() => + fixture.UpdateHandler().Handle( + new UpdateCommentCommand(fixture.TaskId, comment.Id, "x"), CancellationToken.None)); + + fixture.Comments.Verify(x => x.Update(It.IsAny()), 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); } }