diff --git a/api/src/ProposalSystem.Api/Controllers/AuthController.cs b/api/src/ProposalSystem.Api/Controllers/AuthController.cs index afdd9b8..2cc0c44 100644 --- a/api/src/ProposalSystem.Api/Controllers/AuthController.cs +++ b/api/src/ProposalSystem.Api/Controllers/AuthController.cs @@ -95,42 +95,7 @@ public class AuthController : ControllerBase : groups.Contains("admins") ? UserRole.Admin : UserRole.Dispatcher; - var user = await _db.Users.FirstOrDefaultAsync(u => u.CognitoSub == sub, ct); - if (user == null) - { - user = new User - { - Id = Guid.NewGuid(), - CognitoSub = sub, - Email = email, - DisplayName = name, - Role = role, - IsActive = true, - CreatedAt = DateTime.UtcNow, - UpdatedAt = DateTime.UtcNow, - }; - _db.Users.Add(user); - await _db.SaveChangesAsync(ct); - } - else - { - var changed = false; - if (user.Email != email) { user.Email = email; changed = true; } - if (user.DisplayName != name) { user.DisplayName = name; changed = true; } - if (user.Role != role) - { - // Fix: API-M13 — log previous role on Cognito-synced role changes - _logger.LogInformation("User {Email} role changed from {OldRole} to {NewRole} via Cognito sync", - user.Email, user.Role, role); - user.Role = role; - changed = true; - } - if (changed) - { - user.UpdatedAt = DateTime.UtcNow; - await _db.SaveChangesAsync(ct); - } - } + var user = await SyncCognitoUserAsync(sub, email, name, role, ct); return Ok(new AuthResponse( user.Id.ToString(), @@ -214,6 +179,48 @@ public class AuthController : ControllerBase )); } + // Tested via InternalsVisibleTo: role-sync must log user.Id, not email (CodeQL alert 1). + internal async Task SyncCognitoUserAsync(string sub, string email, string name, UserRole role, CancellationToken ct) + { + var user = await _db.Users.FirstOrDefaultAsync(u => u.CognitoSub == sub, ct); + if (user == null) + { + user = new User + { + Id = Guid.NewGuid(), + CognitoSub = sub, + Email = email, + DisplayName = name, + Role = role, + IsActive = true, + CreatedAt = DateTime.UtcNow, + UpdatedAt = DateTime.UtcNow, + }; + _db.Users.Add(user); + await _db.SaveChangesAsync(ct); + return user; + } + + var changed = false; + if (user.Email != email) { user.Email = email; changed = true; } + if (user.DisplayName != name) { user.DisplayName = name; changed = true; } + if (user.Role != role) + { + // Fix: API-M13 — log previous role on Cognito-synced role changes + _logger.LogInformation("User {UserId} role changed from {OldRole} to {NewRole} via Cognito sync", + user.Id, user.Role, role); + user.Role = role; + changed = true; + } + if (changed) + { + user.UpdatedAt = DateTime.UtcNow; + await _db.SaveChangesAsync(ct); + } + + return user; + } + private async Task ExchangeCodeAsync( string domain, string clientId, string code, string redirectUri, CancellationToken ct) { diff --git a/api/src/ProposalSystem.Api/Middleware/InternalApiKeyMiddleware.cs b/api/src/ProposalSystem.Api/Middleware/InternalApiKeyMiddleware.cs index c5b599d..c19c458 100644 --- a/api/src/ProposalSystem.Api/Middleware/InternalApiKeyMiddleware.cs +++ b/api/src/ProposalSystem.Api/Middleware/InternalApiKeyMiddleware.cs @@ -14,6 +14,27 @@ public class InternalApiKeyMiddleware "/api/files", ]; + private static string SanitizeForLog(string value) + { + if (string.IsNullOrEmpty(value)) + { + return string.Empty; + } + + var sanitized = value.Replace("\r", "").Replace("\n", ""); + var builder = new StringBuilder(sanitized.Length); + + foreach (var c in sanitized) + { + if (!char.IsControl(c)) + { + builder.Append(c); + } + } + + return builder.ToString(); + } + private readonly RequestDelegate _next; private readonly byte[] _apiKeyBytes; private readonly ILogger _logger; @@ -32,20 +53,22 @@ public class InternalApiKeyMiddleware context.Request.Headers.TryGetValue("X-Internal-Api-Key", out var providedKey) && !string.IsNullOrEmpty(providedKey.ToString())) { + var path = context.Request.Path.Value ?? ""; + var sanitizedPath = SanitizeForLog(path); + var providedBytes = Encoding.UTF8.GetBytes(providedKey.ToString()); if (!CryptographicOperations.FixedTimeEquals(providedBytes, _apiKeyBytes)) { _logger.LogWarning("Invalid internal API key from {RemoteIp} on {Path}", - context.Connection.RemoteIpAddress, context.Request.Path); + context.Connection.RemoteIpAddress, sanitizedPath); context.Response.StatusCode = 401; return; } - var path = context.Request.Path.Value ?? ""; if (!AllowedPathPrefixes.Any(prefix => path.StartsWith(prefix, StringComparison.OrdinalIgnoreCase))) { _logger.LogWarning("Internal API key used on disallowed path {Path} from {RemoteIp}", - context.Request.Path, context.Connection.RemoteIpAddress); + sanitizedPath, context.Connection.RemoteIpAddress); context.Response.StatusCode = 403; return; } diff --git a/api/src/ProposalSystem.Api/ProposalSystem.Api.csproj b/api/src/ProposalSystem.Api/ProposalSystem.Api.csproj index ed95c7c..1bf58ed 100644 --- a/api/src/ProposalSystem.Api/ProposalSystem.Api.csproj +++ b/api/src/ProposalSystem.Api/ProposalSystem.Api.csproj @@ -14,6 +14,10 @@ + + + + diff --git a/api/tests/ProposalSystem.Tests/Controllers/AuthControllerRoleSyncTests.cs b/api/tests/ProposalSystem.Tests/Controllers/AuthControllerRoleSyncTests.cs new file mode 100644 index 0000000..1442e77 --- /dev/null +++ b/api/tests/ProposalSystem.Tests/Controllers/AuthControllerRoleSyncTests.cs @@ -0,0 +1,85 @@ +using FluentAssertions; +using Microsoft.Extensions.Configuration; +using NSubstitute; +using ProposalSystem.Api.Controllers; +using ProposalSystem.Domain.Entities; +using ProposalSystem.Tests.Helpers; +using Xunit; + +namespace ProposalSystem.Tests.Controllers; + +public class AuthControllerRoleSyncTests +{ + [Fact(DisplayName = "SEC-29: Cognito role sync logs user id, not email")] + public async Task RoleChange_LogsUserIdNotEmail() + { + var email = "pii@example.com"; + var user = new User + { + Id = Guid.NewGuid(), + CognitoSub = "sub-role-sync", + Email = email, + DisplayName = "Test User", + Role = UserRole.Dispatcher, + IsActive = true, + CreatedAt = DateTime.UtcNow, + UpdatedAt = DateTime.UtcNow, + }; + + var (controller, db, logger) = CreateController(user); + + var synced = await controller.SyncCognitoUserAsync( + user.CognitoSub, email, user.DisplayName, UserRole.Admin, CancellationToken.None); + + synced.Id.Should().Be(user.Id); + synced.Email.Should().Be(email); + synced.Role.Should().Be(UserRole.Admin); + logger.Messages.Should().ContainSingle(); + logger.Messages[0].Should().Contain(user.Id.ToString()); + logger.Messages[0].Should().NotContain(email); + logger.Messages[0].Should().Contain(UserRole.Dispatcher.ToString()); + logger.Messages[0].Should().Contain(UserRole.Admin.ToString()); + db.Dispose(); + } + + [Fact(DisplayName = "SEC-29: Cognito role sync does not log when role is unchanged")] + public async Task UnchangedRole_DoesNotLog() + { + var user = new User + { + Id = Guid.NewGuid(), + CognitoSub = "sub-unchanged", + Email = "pii@example.com", + DisplayName = "Test User", + Role = UserRole.Admin, + IsActive = true, + CreatedAt = DateTime.UtcNow, + UpdatedAt = DateTime.UtcNow, + }; + + var (controller, db, logger) = CreateController(user); + + await controller.SyncCognitoUserAsync( + user.CognitoSub, user.Email, user.DisplayName, UserRole.Admin, CancellationToken.None); + + logger.Messages.Should().BeEmpty(); + db.Dispose(); + } + + private static (AuthController Controller, Infrastructure.Data.ProposalDbContext Db, CapturingLogger Logger) + CreateController(User seed) + { + var db = DbContextFactory.Create(); + db.Users.Add(seed); + db.SaveChanges(); + + var logger = new CapturingLogger(); + var controller = new AuthController( + db, + Substitute.For(), + new ConfigurationBuilder().Build(), + logger); + + return (controller, db, logger); + } +} diff --git a/api/tests/ProposalSystem.Tests/Helpers/CapturingLogger.cs b/api/tests/ProposalSystem.Tests/Helpers/CapturingLogger.cs new file mode 100644 index 0000000..03c3dab --- /dev/null +++ b/api/tests/ProposalSystem.Tests/Helpers/CapturingLogger.cs @@ -0,0 +1,22 @@ +using Microsoft.Extensions.Logging; + +namespace ProposalSystem.Tests.Helpers; + +internal sealed class CapturingLogger : ILogger +{ + public List Messages { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + Messages.Add(formatter(state, exception)); + } +} diff --git a/api/tests/ProposalSystem.Tests/Middleware/InternalApiKeyMiddlewareTests.cs b/api/tests/ProposalSystem.Tests/Middleware/InternalApiKeyMiddlewareTests.cs index 134bae1..f285d68 100644 --- a/api/tests/ProposalSystem.Tests/Middleware/InternalApiKeyMiddlewareTests.cs +++ b/api/tests/ProposalSystem.Tests/Middleware/InternalApiKeyMiddlewareTests.cs @@ -5,6 +5,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.Logging; using NSubstitute; using ProposalSystem.Api.Middleware; +using ProposalSystem.Tests.Helpers; using Xunit; namespace ProposalSystem.Tests.Middleware; @@ -178,7 +179,43 @@ public class InternalApiKeyMiddlewareTests } } - private static (InternalApiKeyMiddleware middleware, HttpContext context, Func nextCalled) CreateMiddleware(string? configuredKey) + [Fact(DisplayName = "SEC-29: Invalid-key log strips CR/LF from request path")] + public async Task InvalidKey_PathWithNewlines_LogsSanitizedPath() + { + var logger = new CapturingLogger(); + var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey, logger); + context.Request.Headers["X-Internal-Api-Key"] = "wrong-key"; + context.Request.Path = "/api/proposals/\nINFO forged"; + + await middleware.InvokeAsync(context); + + nextCalled().Should().BeFalse(); + context.Response.StatusCode.Should().Be(401); + logger.Messages.Should().ContainSingle(); + logger.Messages[0].Should().NotContain("\n").And.NotContain("\r"); + logger.Messages[0].Should().Contain("/api/proposals/INFO forged"); + } + + [Fact(DisplayName = "SEC-29: Disallowed-path log strips CR/LF from request path")] + public async Task ValidKey_DisallowedPathWithNewlines_LogsSanitizedPath() + { + var logger = new CapturingLogger(); + var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey, logger); + context.Request.Headers["X-Internal-Api-Key"] = ValidApiKey; + context.Request.Path = "/api/admin\r\nINFO forged"; + + await middleware.InvokeAsync(context); + + nextCalled().Should().BeFalse(); + context.Response.StatusCode.Should().Be(403); + logger.Messages.Should().ContainSingle(); + logger.Messages[0].Should().NotContain("\n").And.NotContain("\r"); + logger.Messages[0].Should().Contain("/api/adminINFO forged"); + } + + private static (InternalApiKeyMiddleware middleware, HttpContext context, Func nextCalled) CreateMiddleware( + string? configuredKey, + ILogger? logger = null) { var wasNextCalled = false; RequestDelegate next = _ => @@ -197,7 +234,7 @@ public class InternalApiKeyMiddlewareTests .AddInMemoryCollection(configData) .Build(); - var logger = Substitute.For>(); + logger ??= Substitute.For>(); var middleware = new InternalApiKeyMiddleware(next, configuration, logger); var context = new DefaultHttpContext(); diff --git a/web/package-lock.json b/web/package-lock.json index 5c4cd88..de36804 100644 --- a/web/package-lock.json +++ b/web/package-lock.json @@ -3115,9 +3115,9 @@ "license": "MIT" }, "node_modules/nanoid": { - "version": "3.3.16", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", - "integrity": "sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { @@ -3288,9 +3288,9 @@ } }, "node_modules/postcss": { - "version": "8.5.25", - "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.25.tgz", - "integrity": "sha512-DTPx3RWSSnWyzLxQnlH0rJP+EW5ekl16ZU4/psbIhA0e53kJfdgaN5vKM+xP7yJtXVu+nfdVFmlgFDEKAe4Pyw==", + "version": "8.5.26", + "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.5.26.tgz", + "integrity": "sha512-u82N74LFzG8ca+dD8puPnplTXoGH4fTPpVGuIbt36G3qvNlkvfD0lEAZSxaly3KX8TS/L1A1gsCEmvKmBcVbkQ==", "dev": true, "funding": [ { @@ -3308,7 +3308,7 @@ ], "license": "MIT", "dependencies": { - "nanoid": "^3.3.16", + "nanoid": "^3.3.17", "picocolors": "^1.1.1", "source-map-js": "^1.2.1" },