fix(web,api): pin nanoid and sanitize auth logs (SEC-29) (#312)

* fix: bump nanoid to 3.3.16 and postcss to 8.5.26 Bump nanoid from 3.3.16 to 3.3.18 in web/ Bump postcss from 8.5.25 to 8.5.26 in web/ Closes [Dependabot 47] (https://github.com/Sea-Haven-Industries/proposal-system/security/dependabot/47) * fix(api): sanitize request path in internal API key logs (SEC-29) Strip CR/LF from Request.Path before logging invalid-key and disallowed-path warnings so CodeQL alerts 4 and 5 close without changing 401/403 behavior. * fix(api): log user id instead of email on cognito role sync (SEC-29) Keep AuthResponse.Email unchanged so CodeQL alert 1 closes without altering the callback payload. * fix(api): use sanitized path on both internal key logs (SEC-29) The 401 branch referenced an out-of-scope identifier and the 403 branch skipped SanitizeForLog. Cover newline-in-path logs and Cognito role-sync user-id logging with tests.
2026-09-30 03:03:13 +00:00 · 2026-08-20 12:38:29 -04:00 · 2026-08-20 12:38:29 -04:00 · 9e579f84e2
commit 9e579f84e2
parent 9cdb1ac406
7 changed files with 226 additions and 48 deletions
--- 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<User> 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<CognitoTokenResponse?> ExchangeCodeAsync(
        string domain, string clientId, string code, string redirectUri, CancellationToken ct)
    {
--- 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<InternalApiKeyMiddleware> _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;
            }
--- a/api/src/ProposalSystem.Api/ProposalSystem.Api.csproj
+++ b/api/src/ProposalSystem.Api/ProposalSystem.Api.csproj
@ -14,6 +14,10 @@
    <Content Update="Data\verified-sites.json" CopyToOutputDirectory="PreserveNewest" />
  </ItemGroup>

+  <ItemGroup>
+    <InternalsVisibleTo Include="ProposalSystem.Tests" />
+  </ItemGroup>
+
  <ItemGroup>
    <ProjectReference Include="..\ProposalSystem.Application\ProposalSystem.Application.csproj" />
    <ProjectReference Include="..\ProposalSystem.Infrastructure\ProposalSystem.Infrastructure.csproj" />
--- a/api/tests/ProposalSystem.Tests/Controllers/AuthControllerRoleSyncTests.cs
+++ 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<AuthController> Logger)
+        CreateController(User seed)
+    {
+        var db = DbContextFactory.Create();
+        db.Users.Add(seed);
+        db.SaveChanges();
+
+        var logger = new CapturingLogger<AuthController>();
+        var controller = new AuthController(
+            db,
+            Substitute.For<IHttpClientFactory>(),
+            new ConfigurationBuilder().Build(),
+            logger);
+
+        return (controller, db, logger);
+    }
+}
--- a/api/tests/ProposalSystem.Tests/Helpers/CapturingLogger.cs
+++ b/api/tests/ProposalSystem.Tests/Helpers/CapturingLogger.cs
@ -0,0 +1,22 @@
+using Microsoft.Extensions.Logging;
+
+namespace ProposalSystem.Tests.Helpers;
+
+internal sealed class CapturingLogger<T> : ILogger<T>
+{
+    public List<string> Messages { get; } = [];
+
+    public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+
+    public bool IsEnabled(LogLevel logLevel) => true;
+
+    public void Log<TState>(
+        LogLevel logLevel,
+        EventId eventId,
+        TState state,
+        Exception? exception,
+        Func<TState, Exception?, string> formatter)
+    {
+        Messages.Add(formatter(state, exception));
+    }
+}
--- 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<bool> 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<InternalApiKeyMiddleware>();
+        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<InternalApiKeyMiddleware>();
+        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<bool> nextCalled) CreateMiddleware(
+        string? configuredKey,
+        ILogger<InternalApiKeyMiddleware>? logger = null)
    {
        var wasNextCalled = false;
        RequestDelegate next = _ =>
@ -197,7 +234,7 @@ public class InternalApiKeyMiddlewareTests
            .AddInMemoryCollection(configData)
            .Build();

-        var logger = Substitute.For<ILogger<InternalApiKeyMiddleware>>();
+        logger ??= Substitute.For<ILogger<InternalApiKeyMiddleware>>();

        var middleware = new InternalApiKeyMiddleware(next, configuration, logger);
        var context = new DefaultHttpContext();
--- 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"
      },