From a4d1e9512e85059b81d5356113cc8346cb0487d1 Mon Sep 17 00:00:00 2001 From: santidev21 Date: Wed, 30 Sep 2026 20:09:57 -0500 Subject: [PATCH] fix(codeql): triage CodeQL alerts and fix password-recovery reveal flow - codeql.yml: switch C# analysis to build-mode none so source-generator output under obj/ (OpenApiXmlCommentSupport) is excluded from analysis; paths-ignore does not apply to compiled languages. - recoveries/{id}/reveal: persist the hash of the code actually shown and stop resolving the request on reveal, so /auth/reset can match it (the previously stored hash belonged to a code that was never disclosed, which made password recovery unusable). - player.component: drop a null check that is always true after the guard above (js/comparison-between-incompatible-types). - AdminAuthHandler: use a using-scope instead of manual try/finally dispose (cs/missed-using-statement). - MarkIdempotentAsync: catch DbUpdateException instead of all exceptions (cs/catch-of-all-exceptions). - Settings loop and slug filter: use explicit LINQ Where (cs/linq/missed-where). - MatchHistoryTests: assert the closed round instead of leaving the result unused (cs/useless-assignment-to-local). --- .github/workflows/codeql.yml | 10 ++-- .../Auth/AdminAuthHandler.cs | 49 ++++++++----------- .../Endpoints/BilliardEndpoints.cs | 18 +++++-- .../Entities/RecoveryRequest.cs | 9 ++++ .../BilliardSystem.Domain/Entities/Tenant.cs | 7 +-- .../BilliardSystem.Tests/MatchHistoryTests.cs | 1 + .../app/features/player/player.component.ts | 6 +-- 7 files changed, 54 insertions(+), 46 deletions(-) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 375069b..4894abf 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -31,15 +31,19 @@ jobs: - name: Checkout uses: actions/checkout@v4 + # build-mode: none extracts tracked source files only — no build, no + # compilation. This deliberately excludes compiler/source-generator output + # under obj/ (e.g. Microsoft.AspNetCore.OpenApi's OpenApiXmlCommentSupport + # generated at obj/Debug/net10.0/generated/...), which otherwise floods the + # results with alerts in code nobody edits. `paths-ignore` in the CodeQL + # config does NOT apply to compiled languages, so the exclusion happens here. - name: Initialize CodeQL uses: github/codeql-action/init@v3 with: languages: ${{ matrix.language }} + build-mode: none config-file: ./.github/codeql/codeql-config.yml - - name: Autobuild - uses: github/codeql-action/autobuild@v3 - - name: Perform CodeQL analysis uses: github/codeql-action/analyze@v3 with: diff --git a/backend/src/BilliardSystem.API/Auth/AdminAuthHandler.cs b/backend/src/BilliardSystem.API/Auth/AdminAuthHandler.cs index fe89844..112532d 100644 --- a/backend/src/BilliardSystem.API/Auth/AdminAuthHandler.cs +++ b/backend/src/BilliardSystem.API/Auth/AdminAuthHandler.cs @@ -46,38 +46,31 @@ protected override async Task HandleAuthenticateAsync() var tokenHash = Convert.ToHexString(SHA256.HashData(Encoding.UTF8.GetBytes(token))); - var scope = Context.RequestServices.CreateScope(); - try - { - var dbContext = scope.ServiceProvider.GetRequiredService(); - - var session = await dbContext.Sessions - .FirstOrDefaultAsync(s => s.TokenHash == tokenHash); + using var scope = Context.RequestServices.CreateScope(); + var dbContext = scope.ServiceProvider.GetRequiredService(); - if (session is null || !session.IsValid()) - { - return AuthenticateResult.Fail("Sesión inválida o expirada."); - } + var session = await dbContext.Sessions + .FirstOrDefaultAsync(s => s.TokenHash == tokenHash); - if (DateTimeOffset.UtcNow - session.LastUsedAt > TimeSpan.FromHours(RefreshThresholdHours)) - { - session.Touch(); - await dbContext.SaveChangesAsync(); - } - - var claims = new[] - { - new Claim(ClaimTypes.Name, "Admin"), - new Claim("session_id", session.Id.ToString()) - }; - var identity = new ClaimsIdentity(claims, Scheme.Name); - var principal = new ClaimsPrincipal(identity); - var ticket = new AuthenticationTicket(principal, Scheme.Name); - return AuthenticateResult.Success(ticket); + if (session is null || !session.IsValid()) + { + return AuthenticateResult.Fail("Sesión inválida o expirada."); } - finally + + if (DateTimeOffset.UtcNow - session.LastUsedAt > TimeSpan.FromHours(RefreshThresholdHours)) { - (scope as IDisposable)?.Dispose(); + session.Touch(); + await dbContext.SaveChangesAsync(); } + + var claims = new[] + { + new Claim(ClaimTypes.Name, "Admin"), + new Claim("session_id", session.Id.ToString()) + }; + var identity = new ClaimsIdentity(claims, Scheme.Name); + var principal = new ClaimsPrincipal(identity); + var ticket = new AuthenticationTicket(principal, Scheme.Name); + return AuthenticateResult.Success(ticket); } } diff --git a/backend/src/BilliardSystem.API/Endpoints/BilliardEndpoints.cs b/backend/src/BilliardSystem.API/Endpoints/BilliardEndpoints.cs index bd7b76f..ca6f19f 100644 --- a/backend/src/BilliardSystem.API/Endpoints/BilliardEndpoints.cs +++ b/backend/src/BilliardSystem.API/Endpoints/BilliardEndpoints.cs @@ -318,9 +318,11 @@ public static IEndpointRouteBuilder MapBilliardEndpoints(this IEndpointRouteBuil .FirstOrDefaultAsync(r => r.Id == id && !r.IsResolved, ct); if (request is null) return Results.NotFound(); + // Persist the hash of the code being shown, otherwise /auth/reset can + // never match it (the hash from request creation belongs to a code that + // was never disclosed). Resolution happens on successful reset only. var code = GenerateRecoveryCode(); - var codeHash = HashToken(code); - request.Resolve(); + request.ReplaceCode(HashToken(code)); await dbContext.SaveChangesAsync(ct); return Results.Ok(new { code, userName = request.User?.UserName }); @@ -612,9 +614,8 @@ await WriteAuditAsync(dbContext, AuditActionType.SettingsChanged, user.GetUserId { var tenantId = user.GetTenantId(); if (tenantId is null) return Results.Forbid(); - foreach (var pair in values) + foreach (var pair in values.Where(pair => AllowedSettingKeys.Contains(pair.Key))) { - if (!AllowedSettingKeys.Contains(pair.Key)) continue; var setting = await dbContext.Settings.FirstOrDefaultAsync(s => s.TenantId == tenantId && s.Key == pair.Key, ct); if (setting is null) dbContext.Settings.Add(new AppSetting(pair.Key, pair.Value, tenantId)); else setting.Update(pair.Value); @@ -1121,7 +1122,14 @@ private static async Task MarkIdempotentAsync(BilliardDbContext dbContext, Guid? if (transactionId is null) return; if (await dbContext.IdempotencyKeys.AnyAsync(k => k.TransactionId == transactionId, ct)) return; dbContext.IdempotencyKeys.Add(new IdempotencyKey(transactionId.Value)); - try { await dbContext.SaveChangesAsync(ct); } catch { /* duplicate */ } + try + { + await dbContext.SaveChangesAsync(ct); + } + catch (DbUpdateException) + { + // Duplicate key: a concurrent request already recorded this transaction. + } } private static async Task WriteAuditAsync(BilliardDbContext dbContext, AuditActionType actionType, diff --git a/backend/src/BilliardSystem.Domain/Entities/RecoveryRequest.cs b/backend/src/BilliardSystem.Domain/Entities/RecoveryRequest.cs index 06604ee..1a00d54 100644 --- a/backend/src/BilliardSystem.Domain/Entities/RecoveryRequest.cs +++ b/backend/src/BilliardSystem.Domain/Entities/RecoveryRequest.cs @@ -29,6 +29,15 @@ public RecoveryRequest(Guid tenantId, Guid userId, string codeHash, DateTimeOffs public bool IsExpired() => DateTimeOffset.UtcNow >= ExpiresAt; + /// + /// Replaces the stored code hash with the hash of a newly revealed code, + /// so the last revealed code is the one /auth/reset will match. + /// + public void ReplaceCode(string codeHash) + { + CodeHash = codeHash; + } + public void Resolve() { ResolvedAt = DateTimeOffset.UtcNow; diff --git a/backend/src/BilliardSystem.Domain/Entities/Tenant.cs b/backend/src/BilliardSystem.Domain/Entities/Tenant.cs index 2f2bf54..0e5afc8 100644 --- a/backend/src/BilliardSystem.Domain/Entities/Tenant.cs +++ b/backend/src/BilliardSystem.Domain/Entities/Tenant.cs @@ -50,12 +50,9 @@ private static string GenerateSlug(string name) .Replace('_', '-'); var result = new StringBuilder(slug.Length); - foreach (var c in slug) + foreach (var c in slug.Where(c => char.IsLetterOrDigit(c) || c == '-')) { - if (char.IsLetterOrDigit(c) || c == '-') - { - result.Append(c); - } + result.Append(c); } var finalSlug = result.ToString().Trim('-'); diff --git a/backend/tests/BilliardSystem.Tests/MatchHistoryTests.cs b/backend/tests/BilliardSystem.Tests/MatchHistoryTests.cs index c1b2fad..7043b94 100644 --- a/backend/tests/BilliardSystem.Tests/MatchHistoryTests.cs +++ b/backend/tests/BilliardSystem.Tests/MatchHistoryTests.cs @@ -154,6 +154,7 @@ public void TryCloseFinalRound_AfterCloseRoundUsesLastEndAsStart() var t1 = t0.AddSeconds(20); var t2 = t0.AddSeconds(50); var r1 = match.CloseRound(t1); + r1.Should().NotBeNull(); match.AddScore("yellow", 3, null); var final = match.TryCloseFinalRound(t2); final.Should().NotBeNull(); diff --git a/frontend/src/app/features/player/player.component.ts b/frontend/src/app/features/player/player.component.ts index 0b78da8..680e6ac 100644 --- a/frontend/src/app/features/player/player.component.ts +++ b/frontend/src/app/features/player/player.component.ts @@ -437,11 +437,7 @@ export class PlayerComponent implements OnInit, OnDestroy { // monotonic guard: don't let a stale poll (old StartedAt) overwrite a just-started session (00:00 -> old time bug in FreeMode) if (currentStart === null || serverStart > currentStart || m.roundNumber > this.roundNumber()) { this.startedAt.set(serverStart); - } else if ( - serverStart !== currentStart && - currentStart !== null && - Math.abs(serverStart - currentStart) < 5000 - ) { + } else if (serverStart !== currentStart && Math.abs(serverStart - currentStart) < 5000) { // small clock skew (server vs client Date.now) — sync to server this.startedAt.set(serverStart); }