proposal-system/api/tests/ProposalSystem.Tests/Middleware/InternalApiKeyMiddlewareTests.cs
Adam Moussa 9e579f84e2
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-08-20 12:38:29 -04:00

244 lines
9.6 KiB
C#

using System.Security.Claims;
using FluentAssertions;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Logging;
using NSubstitute;
using ProposalSystem.Api.Middleware;
using ProposalSystem.Tests.Helpers;
using Xunit;
namespace ProposalSystem.Tests.Middleware;
/// <summary>
/// QA-C4: InternalApiKeyMiddleware tests.
/// Validates that internal API key authentication works correctly:
/// - Valid key on any path sets system claims and calls next (current behavior)
/// - Invalid key logs warning but still calls next (passes through to JWT)
/// - Missing key header passes through to next middleware (JWT auth)
/// - Empty key in config disables the middleware entirely
///
/// Note: API-C1 audit finding identified that this middleware applies globally
/// and bypasses JWT on ANY route when a valid key is provided. These tests
/// document the current behavior; the fix should scope keys to /internal/ paths.
/// </summary>
public class InternalApiKeyMiddlewareTests
{
private const string ValidApiKey = "test-internal-api-key-secret-value";
[Fact(DisplayName = "QA-C4: Valid key sets system claims and calls next")]
public async Task ValidKey_SetsClaimsAndCallsNext()
{
// Arrange
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
context.Request.Headers["X-Internal-Api-Key"] = ValidApiKey;
context.Request.Path = "/api/proposals/123";
// Act
await middleware.InvokeAsync(context);
// Assert
nextCalled().Should().BeTrue("next middleware should be called");
context.User.Identity!.IsAuthenticated.Should().BeTrue();
context.User.Identity!.AuthenticationType.Should().Be("InternalApiKey");
context.User.FindFirst(ClaimTypes.NameIdentifier)!.Value.Should().Be("system");
context.User.FindFirst("sub")!.Value.Should().Be("system-lambda-caller");
context.User.FindFirst(ClaimTypes.Role)!.Value.Should().Be("admins");
context.User.FindFirst("cognito:groups")!.Value.Should().Be("admins");
context.User.FindFirst(ClaimTypes.Email)!.Value.Should().Be("system@proposal-system.internal");
}
[Fact(DisplayName = "QA-C4/API-H1: Invalid key returns 401 immediately")]
public async Task InvalidKey_Returns401()
{
// Arrange
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
context.Request.Headers["X-Internal-Api-Key"] = "wrong-key";
context.Request.Path = "/api/proposals/123";
// Act
await middleware.InvokeAsync(context);
// Assert — API-H1 fix: invalid key short-circuits with 401
nextCalled().Should().BeFalse("invalid key should not fall through");
context.Response.StatusCode.Should().Be(401);
}
[Fact(DisplayName = "QA-C4: Missing key header passes through to next middleware")]
public async Task MissingKeyHeader_PassesThrough()
{
// Arrange
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
// No X-Internal-Api-Key header set
context.Request.Path = "/api/proposals";
// Act
await middleware.InvokeAsync(context);
// Assert
nextCalled().Should().BeTrue("request should pass through to next middleware");
context.User.Identity!.IsAuthenticated.Should().BeFalse();
}
[Fact(DisplayName = "QA-C4: Empty key in config disables API key check entirely")]
public async Task EmptyKeyInConfig_DisablesMiddleware()
{
// Arrange - empty config key means middleware is effectively disabled
var (middleware, context, nextCalled) = CreateMiddleware("");
context.Request.Headers["X-Internal-Api-Key"] = "any-key-value";
context.Request.Path = "/api/proposals/123";
// Act
await middleware.InvokeAsync(context);
// Assert
nextCalled().Should().BeTrue("next middleware should be called");
context.User.Identity!.IsAuthenticated.Should().BeFalse(
"middleware is disabled when config key is empty");
}
[Fact(DisplayName = "QA-C4: Null config key disables API key check")]
public async Task NullConfigKey_DisablesMiddleware()
{
// Arrange - null config key (INTERNAL_API_KEY not set)
var (middleware, context, nextCalled) = CreateMiddleware(null);
context.Request.Headers["X-Internal-Api-Key"] = "any-key-value";
// Act
await middleware.InvokeAsync(context);
// Assert
nextCalled().Should().BeTrue();
context.User.Identity!.IsAuthenticated.Should().BeFalse();
}
[Fact(DisplayName = "QA-C4: Empty header value passes through")]
public async Task EmptyHeaderValue_PassesThrough()
{
// Arrange
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
context.Request.Headers["X-Internal-Api-Key"] = "";
// Act
await middleware.InvokeAsync(context);
// Assert
nextCalled().Should().BeTrue();
context.User.Identity!.IsAuthenticated.Should().BeFalse();
}
[Fact(DisplayName = "QA-C4: Timing-safe comparison used (key differs by one char)")]
public async Task SimilarKey_Returns401()
{
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
context.Request.Headers["X-Internal-Api-Key"] = ValidApiKey + "x";
context.Request.Path = "/api/proposals/123";
// Act
await middleware.InvokeAsync(context);
// Assert — API-H1 fix: invalid key short-circuits with 401
nextCalled().Should().BeFalse("similar but wrong key should not fall through");
context.Response.StatusCode.Should().Be(401);
}
[Fact(DisplayName = "QA-C4/API-C1: Valid key on allowed path authenticates")]
public async Task ValidKey_AllowedPath_Authenticates()
{
var allowedPaths = new[] { "/api/proposals", "/api/vendor-proposals/1", "/api/generated-pdfs", "/api/files/upload" };
foreach (var path in allowedPaths)
{
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
context.Request.Headers["X-Internal-Api-Key"] = ValidApiKey;
context.Request.Path = path;
await middleware.InvokeAsync(context);
context.User.Identity!.IsAuthenticated.Should().BeTrue(
$"valid key should authenticate on allowed path {path}");
nextCalled().Should().BeTrue();
}
}
[Fact(DisplayName = "QA-C4/API-C1: Valid key on disallowed path returns 403")]
public async Task ValidKey_DisallowedPath_Returns403()
{
var disallowedPaths = new[] { "/api/admin/dashboard", "/api/users", "/health" };
foreach (var path in disallowedPaths)
{
var (middleware, context, nextCalled) = CreateMiddleware(ValidApiKey);
context.Request.Headers["X-Internal-Api-Key"] = ValidApiKey;
context.Request.Path = path;
await middleware.InvokeAsync(context);
nextCalled().Should().BeFalse($"valid key on disallowed path {path} should not call next");
context.Response.StatusCode.Should().Be(403);
}
}
[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 = _ =>
{
wasNextCalled = true;
return Task.CompletedTask;
};
var configData = new Dictionary<string, string?>();
if (configuredKey != null)
{
configData["INTERNAL_API_KEY"] = configuredKey;
}
var configuration = new ConfigurationBuilder()
.AddInMemoryCollection(configData)
.Build();
logger ??= Substitute.For<ILogger<InternalApiKeyMiddleware>>();
var middleware = new InternalApiKeyMiddleware(next, configuration, logger);
var context = new DefaultHttpContext();
return (middleware, context, () => wasNextCalled);
}
}