diff --git a/code/backend/Cleanuparr.Api.Tests/CustomWebApplicationFactory.cs b/code/backend/Cleanuparr.Api.Tests/CustomWebApplicationFactory.cs index a336e421..5e8e0f84 100644 --- a/code/backend/Cleanuparr.Api.Tests/CustomWebApplicationFactory.cs +++ b/code/backend/Cleanuparr.Api.Tests/CustomWebApplicationFactory.cs @@ -1,20 +1,17 @@ -using Cleanuparr.Persistence; +using Cleanuparr.Shared.Helpers; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Mvc.Testing; -using Microsoft.EntityFrameworkCore; -using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Xunit; -// Integration tests share file-system state (config-dir users.db used by SetupGuardMiddleware), -// so we must run them sequentially to avoid interference between factories. +// Integration tests share file-system state (config-dir used by SetupGuardMiddleware), +// so they must be run sequentially to avoid interference between factories. [assembly: CollectionBehavior(DisableTestParallelization = true)] namespace Cleanuparr.Api.Tests; /// -/// Custom WebApplicationFactory that uses an isolated SQLite database for each test fixture. -/// The database file is created in a temp directory so both DI and static contexts share the same data. +/// Custom WebApplicationFactory that redirects all database contexts to an isolated temp directory /// public class CustomWebApplicationFactory : WebApplicationFactory { @@ -24,6 +21,8 @@ public class CustomWebApplicationFactory : WebApplicationFactory { _tempDir = Path.Combine(Path.GetTempPath(), $"cleanuparr-test-{Guid.NewGuid():N}"); Directory.CreateDirectory(_tempDir); + + ConfigurationPathProvider.SetConfigPath(_tempDir); } protected override void ConfigureWebHost(IWebHostBuilder builder) @@ -32,35 +31,12 @@ public class CustomWebApplicationFactory : WebApplicationFactory builder.ConfigureServices(services => { - // Remove the existing UsersContext registration - var descriptor = services.SingleOrDefault(d => d.ServiceType == typeof(DbContextOptions)); - if (descriptor != null) services.Remove(descriptor); - - // Also remove the DbContext registration itself - var contextDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(UsersContext)); - if (contextDescriptor != null) services.Remove(contextDescriptor); - - var dbPath = Path.Combine(_tempDir, "users.db"); - - services.AddDbContext(options => - { - options.UseSqlite($"Data Source={dbPath}"); - }); - // Remove all hosted services (Quartz scheduler, BackgroundJobManager) to prevent // Quartz.Logging.LogProvider.ResolvedLogProvider (a cached Lazy) from being accessed - // with a disposed ILoggerFactory from the previous factory lifecycle. - // Auth tests don't depend on background job scheduling, so this is safe. foreach (var hostedService in services.Where(d => d.ServiceType == typeof(IHostedService)).ToList()) + { services.Remove(hostedService); - - // Ensure DB is created using a minimal isolated context (not the full app DI container) - // to avoid any residual static state contamination. - using var db = new UsersContext( - new DbContextOptionsBuilder() - .UseSqlite($"Data Source={dbPath}") - .Options); - db.Database.EnsureCreated(); + } }); } diff --git a/code/backend/Cleanuparr.Api.Tests/Features/Auth/AccountControllerOidcTests.cs b/code/backend/Cleanuparr.Api.Tests/Features/Auth/AccountControllerOidcTests.cs index f5b02a9c..a943c98f 100644 --- a/code/backend/Cleanuparr.Api.Tests/Features/Auth/AccountControllerOidcTests.cs +++ b/code/backend/Cleanuparr.Api.Tests/Features/Auth/AccountControllerOidcTests.cs @@ -10,7 +10,6 @@ using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Hosting; using Shouldly; namespace Cleanuparr.Api.Tests.Features.Auth; @@ -281,76 +280,18 @@ public class AccountControllerOidcTests : IClassFixture + public class OidcLinkWebApplicationFactory : CustomWebApplicationFactory { - private readonly string _configDir; - - public OidcLinkWebApplicationFactory() - { - _configDir = Cleanuparr.Shared.Helpers.ConfigurationPathProvider.GetConfigPath(); - - // Delete both databases (and their WAL sidecar files) so each test run starts clean. - // users.db must be at the config path so CreateStaticInstance() (used by - // SetupGuardMiddleware) reads the same file as the DI-injected UsersContext. - // ClearAllPools() releases any SQLite connections held by the previous test factory's - // connection pool, which would otherwise prevent file deletion on Windows or cause - // SQLite to reconstruct a partial database from stale WAL files on other platforms. - Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools(); - foreach (var name in new[] { - "users.db", "users.db-shm", "users.db-wal", - "cleanuparr.db", "cleanuparr.db-shm", "cleanuparr.db-wal" }) - { - var path = Path.Combine(_configDir, name); - if (File.Exists(path)) - try { File.Delete(path); } catch { /* best effort */ } - } - } - protected override void ConfigureWebHost(IWebHostBuilder builder) { - builder.UseEnvironment("Testing"); + base.ConfigureWebHost(builder); builder.ConfigureServices(services => { - var descriptor = services.SingleOrDefault(d => d.ServiceType == typeof(DbContextOptions)); - if (descriptor != null) services.Remove(descriptor); - - var contextDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(UsersContext)); - if (contextDescriptor != null) services.Remove(contextDescriptor); - - // Use the config-path db so SetupGuardMiddleware (CreateStaticInstance) sees - // the same data as the DI-injected context. Apply the same naming conventions - // as CreateStaticInstance() so the schemas match. - var dbPath = Path.Combine(_configDir, "users.db"); - services.AddDbContext(options => - { - options - .UseSqlite($"Data Source={dbPath}") - .UseLowerCaseNamingConvention() - .UseSnakeCaseNamingConvention(); - }); - var oidcDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(IOidcAuthService)); if (oidcDescriptor != null) services.Remove(oidcDescriptor); services.AddSingleton(); - - // Remove all hosted services (Quartz scheduler, BackgroundJobManager) to prevent - // Quartz.Logging.LogProvider.ResolvedLogProvider (a cached Lazy) from being accessed - // with a disposed ILoggerFactory from the previous factory lifecycle. - // Auth tests don't depend on background job scheduling, so this is safe. - foreach (var hostedService in services.Where(d => d.ServiceType == typeof(IHostedService)).ToList()) - services.Remove(hostedService); - - // Ensure DB is created using a minimal isolated context (not the full app DI container) - // to avoid any residual static state contamination. - using var db = new UsersContext( - new DbContextOptionsBuilder() - .UseSqlite($"Data Source={dbPath}") - .UseLowerCaseNamingConvention() - .UseSnakeCaseNamingConvention() - .Options); - db.Database.EnsureCreated(); }); } @@ -410,22 +351,6 @@ public class AccountControllerOidcTests : IClassFixture +/// Tests that the login endpoint always runs BCrypt verification regardless of +/// username validity, preventing timing-based username enumeration. +/// +[Collection("Login Timing Tests")] +[TestCaseOrderer("Cleanuparr.Api.Tests.PriorityOrderer", "Cleanuparr.Api.Tests")] +public class LoginTimingTests : IClassFixture +{ + private readonly HttpClient _client; + private readonly TimingTestWebApplicationFactory _factory; + + public LoginTimingTests(TimingTestWebApplicationFactory factory) + { + _factory = factory; + _client = factory.CreateClient(); + } + + [Fact, TestPriority(0)] + public async Task Login_NoUserExists_StillCallsPasswordVerification() + { + _factory.TrackingPasswordService.Reset(); + + var response = await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "nouser", + password = "SomePassword123!" + }); + + response.StatusCode.ShouldBe(HttpStatusCode.Unauthorized); + _factory.TrackingPasswordService.VerifyPasswordCallCount.ShouldBeGreaterThanOrEqualTo(1); + } + + [Fact, TestPriority(1)] + public async Task Setup_CreateAccountAndComplete() + { + var createResponse = await _client.PostAsJsonAsync("/api/auth/setup/account", new + { + username = "timingtest", + password = "TimingTestPassword123!" + }); + createResponse.StatusCode.ShouldBe(HttpStatusCode.Created); + + var completeResponse = await _client.PostAsJsonAsync("/api/auth/setup/complete", new { }); + completeResponse.StatusCode.ShouldBe(HttpStatusCode.OK); + } + + [Fact, TestPriority(2)] + public async Task Login_ValidUsername_CallsPasswordVerification() + { + _factory.TrackingPasswordService.Reset(); + + await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "timingtest", + password = "TimingTestPassword123!" + }); + + _factory.TrackingPasswordService.VerifyPasswordCallCount.ShouldBeGreaterThanOrEqualTo(1); + } + + [Fact, TestPriority(3)] + public async Task Login_NonexistentUsername_StillCallsPasswordVerification() + { + _factory.TrackingPasswordService.Reset(); + + var response = await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "doesnotexist", + password = "SomePassword123!" + }); + + response.StatusCode.ShouldBe(HttpStatusCode.Unauthorized); + _factory.TrackingPasswordService.VerifyPasswordCallCount.ShouldBeGreaterThanOrEqualTo(1); + } + + [Fact, TestPriority(4)] + public async Task Login_LockedOutUser_StillCallsPasswordVerification() + { + // Trigger lockout by making several failed login attempts + for (var i = 0; i < 5; i++) + { + await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "timingtest", + password = "WrongPassword!" + }); + } + + _factory.TrackingPasswordService.Reset(); + + var response = await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "timingtest", + password = "WrongPassword!" + }); + + response.StatusCode.ShouldBe(HttpStatusCode.TooManyRequests); + _factory.TrackingPasswordService.VerifyPasswordCallCount.ShouldBeGreaterThanOrEqualTo(1); + } + + [Fact, TestPriority(5)] + public async Task Login_TimingConsistency_InvalidAndValidUsernamesTakeSimilarTime() + { + const int iterations = 10; + + // Warm up the server and BCrypt static init + await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "warmup", + password = "WarmupPassword123!" + }); + + var invalidTimings = new List(iterations); + var validTimings = new List(iterations); + + for (var i = 0; i < iterations; i++) + { + // Alternate to avoid ordering bias + var invalidSw = Stopwatch.StartNew(); + await _client.PostAsJsonAsync("/api/auth/login", new + { + username = $"nonexistent_{i}", + password = "SomePassword123!" + }); + invalidSw.Stop(); + invalidTimings.Add(invalidSw.ElapsedMilliseconds); + + var validSw = Stopwatch.StartNew(); + await _client.PostAsJsonAsync("/api/auth/login", new + { + username = "timingtest", + password = "WrongPasswordForTiming!" + }); + validSw.Stop(); + validTimings.Add(validSw.ElapsedMilliseconds); + } + + var invalidMedian = Median(invalidTimings); + var validMedian = Median(validTimings); + + // The invalid-username path must not be suspiciously fast + invalidMedian.ShouldBeGreaterThan(50, + $"Non-existent username median too fast ({invalidMedian}ms) — BCrypt may have been skipped"); + + // Medians should be in the same ballpark + var ratio = invalidMedian > validMedian + ? (double)invalidMedian / validMedian + : (double)validMedian / invalidMedian; + + ratio.ShouldBeLessThan(3.0, + $"Timing difference too large: invalid median={invalidMedian}ms, valid median={validMedian}ms (ratio={ratio:F1}x)"); + } + + private static long Median(List values) + { + values.Sort(); + var mid = values.Count / 2; + return values.Count % 2 == 0 + ? (values[mid - 1] + values[mid]) / 2 + : values[mid]; + } +} diff --git a/code/backend/Cleanuparr.Api.Tests/Features/Auth/OidcAuthControllerTests.cs b/code/backend/Cleanuparr.Api.Tests/Features/Auth/OidcAuthControllerTests.cs index 5a599fae..7522b0db 100644 --- a/code/backend/Cleanuparr.Api.Tests/Features/Auth/OidcAuthControllerTests.cs +++ b/code/backend/Cleanuparr.Api.Tests/Features/Auth/OidcAuthControllerTests.cs @@ -4,6 +4,7 @@ using System.Text.Json; using Cleanuparr.Infrastructure.Features.Auth; using Cleanuparr.Persistence; using Cleanuparr.Persistence.Models.Auth; +using Cleanuparr.Shared.Helpers; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.EntityFrameworkCore; @@ -451,16 +452,8 @@ public class OidcAuthControllerTests : IClassFixture { - // Remove existing UsersContext registration - var descriptor = services.SingleOrDefault(d => d.ServiceType == typeof(DbContextOptions)); - if (descriptor != null) services.Remove(descriptor); - - var contextDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(UsersContext)); - if (contextDescriptor != null) services.Remove(contextDescriptor); - - var dbPath = Path.Combine(_tempDir, "users.db"); - services.AddDbContext(options => - { - options.UseSqlite($"Data Source={dbPath}"); - }); - // Replace IOidcAuthService with mock var oidcDescriptor = services.SingleOrDefault(d => d.ServiceType == typeof(IOidcAuthService)); if (oidcDescriptor != null) services.Remove(oidcDescriptor); @@ -494,14 +474,6 @@ public class OidcAuthControllerTests : IClassFixture d.ServiceType == typeof(IHostedService)).ToList()) services.Remove(hostedService); - - // Ensure DB is created using a minimal isolated context (not the full app DI container) - // to avoid any residual static state contamination. - using var db = new UsersContext( - new DbContextOptionsBuilder() - .UseSqlite($"Data Source={dbPath}") - .Options); - db.Database.EnsureCreated(); }); } diff --git a/code/backend/Cleanuparr.Api.Tests/Features/Auth/TimingTestWebApplicationFactory.cs b/code/backend/Cleanuparr.Api.Tests/Features/Auth/TimingTestWebApplicationFactory.cs new file mode 100644 index 00000000..3fdba3b4 --- /dev/null +++ b/code/backend/Cleanuparr.Api.Tests/Features/Auth/TimingTestWebApplicationFactory.cs @@ -0,0 +1,29 @@ +using Cleanuparr.Infrastructure.Features.Auth; +using Microsoft.AspNetCore.Hosting; +using Microsoft.Extensions.DependencyInjection; + +namespace Cleanuparr.Api.Tests.Features.Auth; + +/// +/// Factory variant that replaces with a +/// spy so tests can assert that +/// password verification is always called regardless of username validity. +/// +public class TimingTestWebApplicationFactory : CustomWebApplicationFactory +{ + public TrackingPasswordService TrackingPasswordService { get; } = new(); + + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + base.ConfigureWebHost(builder); + + builder.ConfigureServices(services => + { + // Replace IPasswordService with our tracking spy + var descriptor = services.SingleOrDefault(d => d.ServiceType == typeof(IPasswordService)); + if (descriptor != null) services.Remove(descriptor); + + services.AddSingleton(TrackingPasswordService); + }); + } +} diff --git a/code/backend/Cleanuparr.Api.Tests/Features/Auth/TrackingPasswordService.cs b/code/backend/Cleanuparr.Api.Tests/Features/Auth/TrackingPasswordService.cs new file mode 100644 index 00000000..9497dbe8 --- /dev/null +++ b/code/backend/Cleanuparr.Api.Tests/Features/Auth/TrackingPasswordService.cs @@ -0,0 +1,33 @@ +using Cleanuparr.Infrastructure.Features.Auth; + +namespace Cleanuparr.Api.Tests.Features.Auth; + +/// +/// Spy wrapper around that tracks calls to +/// for behavioral assertions in timing tests. +/// +public sealed class TrackingPasswordService : IPasswordService +{ + private readonly PasswordService _inner = new(); + private int _verifyPasswordCallCount; + + public int VerifyPasswordCallCount => _verifyPasswordCallCount; + + public string DummyHash => _inner.DummyHash; + + public string HashPassword(string password) + { + return _inner.HashPassword(password); + } + + public bool VerifyPassword(string password, string hash) + { + Interlocked.Increment(ref _verifyPasswordCallCount); + return _inner.VerifyPassword(password, hash); + } + + public void Reset() + { + Interlocked.Exchange(ref _verifyPasswordCallCount, 0); + } +} diff --git a/code/backend/Cleanuparr.Api/Features/Auth/Controllers/AuthController.cs b/code/backend/Cleanuparr.Api/Features/Auth/Controllers/AuthController.cs index fd50395d..c1db932a 100644 --- a/code/backend/Cleanuparr.Api/Features/Auth/Controllers/AuthController.cs +++ b/code/backend/Cleanuparr.Api/Features/Auth/Controllers/AuthController.cs @@ -264,6 +264,11 @@ public sealed class AuthController : ControllerBase var user = await _usersContext.Users.AsNoTracking().FirstOrDefaultAsync(); + // Always verify the submitted password to prevent timing-based username enumeration + var userHasPassword = user?.PasswordHash is not null; + var passwordHash = user?.PasswordHash ?? _passwordService.DummyHash; + var passwordValid = _passwordService.VerifyPassword(request.Password, passwordHash) && userHasPassword; + if (user is null || !user.SetupCompleted) { return Unauthorized(new { error = "Invalid credentials" }); @@ -272,12 +277,11 @@ public sealed class AuthController : ControllerBase // Check lockout if (user.LockoutEnd.HasValue && user.LockoutEnd.Value > DateTime.UtcNow) { - var remaining = (int)(user.LockoutEnd.Value - DateTime.UtcNow).TotalSeconds; + var remaining = (int)Math.Ceiling((user.LockoutEnd.Value - DateTime.UtcNow).TotalSeconds); return StatusCode(429, new { error = "Account is locked", retryAfterSeconds = remaining }); } - if (!_passwordService.VerifyPassword(request.Password, user.PasswordHash) || - !string.Equals(user.Username, request.Username, StringComparison.OrdinalIgnoreCase)) + if (!passwordValid || !string.Equals(user.Username, request.Username, StringComparison.OrdinalIgnoreCase)) { var retryAfterSeconds = await IncrementFailedAttempts(user.Id); return Unauthorized(new { error = "Invalid credentials", retryAfterSeconds }); diff --git a/code/backend/Cleanuparr.Infrastructure/Features/Auth/IPasswordService.cs b/code/backend/Cleanuparr.Infrastructure/Features/Auth/IPasswordService.cs index af81de61..2d393a5e 100644 --- a/code/backend/Cleanuparr.Infrastructure/Features/Auth/IPasswordService.cs +++ b/code/backend/Cleanuparr.Infrastructure/Features/Auth/IPasswordService.cs @@ -2,6 +2,9 @@ namespace Cleanuparr.Infrastructure.Features.Auth; public interface IPasswordService { + string DummyHash { get; } + string HashPassword(string password); + bool VerifyPassword(string password, string hash); } diff --git a/code/backend/Cleanuparr.Infrastructure/Features/Auth/PasswordService.cs b/code/backend/Cleanuparr.Infrastructure/Features/Auth/PasswordService.cs index 98ce6634..d024c2ad 100644 --- a/code/backend/Cleanuparr.Infrastructure/Features/Auth/PasswordService.cs +++ b/code/backend/Cleanuparr.Infrastructure/Features/Auth/PasswordService.cs @@ -4,6 +4,11 @@ public sealed class PasswordService : IPasswordService { private const int WorkFactor = 12; + /// + /// Pre-computed BCrypt hash with a work factor of 12 used as a fallback when no user exists + /// + public string DummyHash => "$2a$12$tQw4MgGGq7WTFro3Me4mQOekctJ0mIOYmFMn.XEmEbyZhBq0i4qKy"; + public string HashPassword(string password) { return BCrypt.Net.BCrypt.HashPassword(password, WorkFactor); diff --git a/code/backend/Cleanuparr.Shared/Helpers/ConfigurationPathProvider.cs b/code/backend/Cleanuparr.Shared/Helpers/ConfigurationPathProvider.cs index 2a6aad46..b9e1595b 100644 --- a/code/backend/Cleanuparr.Shared/Helpers/ConfigurationPathProvider.cs +++ b/code/backend/Cleanuparr.Shared/Helpers/ConfigurationPathProvider.cs @@ -48,4 +48,19 @@ public static class ConfigurationPathProvider { return _configPath ?? InitializeConfigPath(); } + + public static void SetConfigPath(string path) + { + if (string.IsNullOrWhiteSpace(path)) + { + throw new ArgumentException("Value cannot be null or whitespace.", nameof(path)); + } + + if (!Directory.Exists(path)) + { + Directory.CreateDirectory(path); + } + + _configPath = path; + } }