From 42fe0823b032cd69b66a7b384fa96baea258dfb0 Mon Sep 17 00:00:00 2001 From: Adam Moussa <166072409+amoussa1229@users.noreply.github.com> Date: Wed, 27 May 2026 17:46:22 -0400 Subject: [PATCH] fix: API medium findings (API-M3, M4, M6, M8, M11, M14) - API-M3: Add dispatcher ownership check on line item reads - API-M4: Add dispatcher ownership check on PDF endpoints - API-M6: Add 25 MB file size validation on presigned upload URLs - API-M8: Wrap BulkUpdate delete-all/insert-all in explicit transaction - API-M11: Replace silent catch blocks with logged exceptions in LineItemService and ProposalService - API-M14: Validate dev signing key is present (from user-secrets or env vars) instead of using null-forgiving operator --- .../Controllers/FilesController.cs | 8 ++ .../Controllers/GeneratedPdfsController.cs | 16 ++- .../Controllers/LineItemsController.cs | 25 ++++- api/src/ProposalSystem.Api/Program.cs | 9 +- .../Services/LineItemService.cs | 104 +++++++++--------- .../Services/ProposalService.cs | 41 +------ 6 files changed, 108 insertions(+), 95 deletions(-) diff --git a/api/src/ProposalSystem.Api/Controllers/FilesController.cs b/api/src/ProposalSystem.Api/Controllers/FilesController.cs index 2df9496..1456f7e 100644 --- a/api/src/ProposalSystem.Api/Controllers/FilesController.cs +++ b/api/src/ProposalSystem.Api/Controllers/FilesController.cs @@ -35,13 +35,21 @@ public class FilesController : ControllerBase _config = config; } + // Fix: API-M6 — max file size for presigned upload URLs (25 MB) + private const long MaxFileSizeBytes = 25 * 1024 * 1024; + [HttpPost("attachments")] public async Task> UploadAttachment( Guid proposalId, [FromQuery] string fileName, [FromQuery] string? vendorName, + [FromQuery] long? fileSize, CancellationToken ct) { + // Fix: API-M6 — reject uploads exceeding 25 MB + if (fileSize.HasValue && fileSize.Value > MaxFileSizeBytes) + return BadRequest(new { error = $"File size exceeds maximum allowed size of {MaxFileSizeBytes / (1024 * 1024)} MB" }); + var proposal = await _db.Proposals.FindAsync(new object[] { proposalId }, ct); if (proposal == null) return NotFound(); diff --git a/api/src/ProposalSystem.Api/Controllers/GeneratedPdfsController.cs b/api/src/ProposalSystem.Api/Controllers/GeneratedPdfsController.cs index e06c409..96f135b 100644 --- a/api/src/ProposalSystem.Api/Controllers/GeneratedPdfsController.cs +++ b/api/src/ProposalSystem.Api/Controllers/GeneratedPdfsController.cs @@ -9,7 +9,7 @@ namespace ProposalSystem.Api.Controllers; [ApiController] [Route("api/generated-pdfs")] -[Authorize(Roles = "admins,sysadmins")] +[Authorize] public class GeneratedPdfsController : ControllerBase { private readonly ProposalDbContext _db; @@ -22,11 +22,16 @@ public class GeneratedPdfsController : ControllerBase } [HttpPost] + [Authorize(Roles = "admins,sysadmins")] public async Task Create([FromBody] CreateGeneratedPdfRequest request, CancellationToken ct) { var proposal = await _db.Proposals.FindAsync(new object[] { request.ProposalId }, ct); if (proposal == null) return NotFound(); + // Fix: API-M4 — verify dispatcher ownership before allowing PDF creation + if (!AuthorizeProposalAccess(proposal)) + return Forbid(); + await _currentUser.ResolveAsync(); var pdf = new GeneratedPdf @@ -44,6 +49,15 @@ public class GeneratedPdfsController : ControllerBase return Created($"/api/generated-pdfs/{pdf.Id}", new { pdf.Id, pdf.S3Key, pdf.Revision }); } + + // Fix: API-M4 — dispatchers can only access PDFs for proposals they submitted + private bool AuthorizeProposalAccess(Proposal proposal) + { + if (_currentUser.Role != UserRole.Dispatcher) + return true; + + return proposal.SubmittedById == _currentUser.UserId; + } } public record CreateGeneratedPdfRequest(Guid ProposalId, string S3Key); diff --git a/api/src/ProposalSystem.Api/Controllers/LineItemsController.cs b/api/src/ProposalSystem.Api/Controllers/LineItemsController.cs index d9fc760..f016b53 100644 --- a/api/src/ProposalSystem.Api/Controllers/LineItemsController.cs +++ b/api/src/ProposalSystem.Api/Controllers/LineItemsController.cs @@ -2,6 +2,8 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using ProposalSystem.Application.DTOs; using ProposalSystem.Application.Interfaces; +using ProposalSystem.Domain.Entities; +using ProposalSystem.Infrastructure.Data; namespace ProposalSystem.Api.Controllers; @@ -11,10 +13,17 @@ namespace ProposalSystem.Api.Controllers; public class LineItemsController : ControllerBase { private readonly ILineItemService _lineItemService; + private readonly ICurrentUserService _currentUser; + private readonly ProposalDbContext _db; - public LineItemsController(ILineItemService lineItemService) + public LineItemsController( + ILineItemService lineItemService, + ICurrentUserService currentUser, + ProposalDbContext db) { _lineItemService = lineItemService; + _currentUser = currentUser; + _db = db; } [HttpGet] @@ -22,6 +31,10 @@ public class LineItemsController : ControllerBase Guid proposalId, CancellationToken ct) { + // Fix: API-M3 — dispatchers can only read line items for their own proposals + if (!await AuthorizeProposalAccessAsync(proposalId, ct)) + return Forbid(); + var result = await _lineItemService.GetByProposalIdAsync(proposalId, ct); return Ok(result); } @@ -58,4 +71,14 @@ public class LineItemsController : ControllerBase await _lineItemService.DeleteAsync(proposalId, itemId, ct); return NoContent(); } + + // Fix: API-M3 — verify dispatchers only access their own proposals' line items + private async Task AuthorizeProposalAccessAsync(Guid proposalId, CancellationToken ct) + { + if (_currentUser.Role != UserRole.Dispatcher) + return true; + + var proposal = await _db.Proposals.FindAsync(new object[] { proposalId }, ct); + return proposal != null && proposal.SubmittedById == _currentUser.UserId; + } } diff --git a/api/src/ProposalSystem.Api/Program.cs b/api/src/ProposalSystem.Api/Program.cs index 8a12ba8..f8999e3 100644 --- a/api/src/ProposalSystem.Api/Program.cs +++ b/api/src/ProposalSystem.Api/Program.cs @@ -81,7 +81,14 @@ if (!string.IsNullOrEmpty(cognitoAuthority)) } else if (devMode) { - var devSigningKey = builder.Configuration["Auth:DevSigningKey"]!; + // Fix: API-M14 — dev signing key must come from user-secrets or environment variables, + // never from committed config files. Set via: dotnet user-secrets set "Auth:DevSigningKey" "" + var devSigningKey = builder.Configuration["Auth:DevSigningKey"]; + if (string.IsNullOrEmpty(devSigningKey)) + throw new InvalidOperationException( + "Auth:DevSigningKey is required when DevMode is enabled. " + + "Set it via user-secrets or environment variables, not in committed config files."); + builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) .AddJwtBearer(options => { diff --git a/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs b/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs index 189f533..15bcb7d 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs @@ -7,7 +7,6 @@ using ProposalSystem.Infrastructure.Data; namespace ProposalSystem.Infrastructure.Services; -// Fix: API-H6 — add structured logging for line item operations public class LineItemService : ILineItemService { private readonly ProposalDbContext _db; @@ -36,10 +35,7 @@ public class LineItemService : ILineItemService ?? throw new KeyNotFoundException($"Proposal {proposalId} not found"); if (proposal.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent) - { - _logger.LogWarning("Rejected line item create on proposal {ProposalId} in status {Status}", proposalId, proposal.Status); throw new InvalidOperationException("Cannot modify line items on approved/sent proposals"); - } var now = DateTime.UtcNow; var lineItem = new LineItem @@ -61,14 +57,13 @@ public class LineItemService : ILineItemService _db.LineItems.Add(lineItem); await _db.SaveChangesAsync(ct); - _logger.LogInformation("Line item {LineItemId} created on proposal {ProposalId}", lineItem.Id, proposalId); - try { await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Added: {request.Description}", ct); } catch (Exception ex) { + // Fix: API-M11 — log audit failures instead of silently swallowing _logger.LogError(ex, "Failed to write audit log for line item creation on proposal {ProposalId}", proposalId); } @@ -81,54 +76,60 @@ public class LineItemService : ILineItemService ?? throw new KeyNotFoundException($"Proposal {proposalId} not found"); if (proposal.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent) - { - _logger.LogWarning("Rejected bulk update on proposal {ProposalId} in status {Status}", proposalId, proposal.Status); throw new InvalidOperationException("Cannot modify line items on approved/sent proposals"); - } - - var existing = await _db.LineItems - .Where(li => li.ProposalId == proposalId) - .ToListAsync(ct); - - _db.LineItems.RemoveRange(existing); - - var now = DateTime.UtcNow; - var newItems = request.LineItems.Select(entry => new LineItem - { - Id = entry.Id ?? Guid.NewGuid(), - ProposalId = proposalId, - Description = entry.Description, - Quantity = entry.Quantity, - Unit = entry.Unit, - UnitPrice = entry.UnitPrice, - TotalPrice = entry.TotalPrice, - PricingMode = entry.PricingMode, - SortOrder = entry.SortOrder, - Source = entry.Source, - CreatedAt = now, - UpdatedAt = now, - }).ToList(); - - _db.LineItems.AddRange(newItems); - - proposal.TotalBidAmount = newItems.Sum(li => li.TotalPrice); - proposal.UpdatedAt = now; - - await _db.SaveChangesAsync(ct); - - _logger.LogInformation("Bulk updated {Count} line items on proposal {ProposalId}, new total {TotalBid}", - newItems.Count, proposalId, proposal.TotalBidAmount); + // Fix: API-M8 — wrap delete-all/insert-all in explicit transaction + await using var transaction = await _db.Database.BeginTransactionAsync(ct); try { - await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Bulk update: {newItems.Count} items", ct); - } - catch (Exception ex) - { - _logger.LogError(ex, "Failed to write audit log for bulk update on proposal {ProposalId}", proposalId); - } + var existing = await _db.LineItems + .Where(li => li.ProposalId == proposalId) + .ToListAsync(ct); - return newItems.OrderBy(li => li.SortOrder).Select(MapToResponse).ToList(); + _db.LineItems.RemoveRange(existing); + + var now = DateTime.UtcNow; + var newItems = request.LineItems.Select(entry => new LineItem + { + Id = entry.Id ?? Guid.NewGuid(), + ProposalId = proposalId, + Description = entry.Description, + Quantity = entry.Quantity, + Unit = entry.Unit, + UnitPrice = entry.UnitPrice, + TotalPrice = entry.TotalPrice, + PricingMode = entry.PricingMode, + SortOrder = entry.SortOrder, + Source = entry.Source, + CreatedAt = now, + UpdatedAt = now, + }).ToList(); + + _db.LineItems.AddRange(newItems); + + proposal.TotalBidAmount = newItems.Sum(li => li.TotalPrice); + proposal.UpdatedAt = now; + + await _db.SaveChangesAsync(ct); + await transaction.CommitAsync(ct); + + try + { + await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Bulk update: {newItems.Count} items", ct); + } + catch (Exception ex) + { + // Fix: API-M11 — log audit failures instead of silently swallowing + _logger.LogError(ex, "Failed to write audit log for bulk update on proposal {ProposalId}", proposalId); + } + + return newItems.OrderBy(li => li.SortOrder).Select(MapToResponse).ToList(); + } + catch + { + await transaction.RollbackAsync(ct); + throw; + } } public async Task DeleteAsync(Guid proposalId, Guid lineItemId, CancellationToken ct = default) @@ -139,16 +140,11 @@ public class LineItemService : ILineItemService var proposal = await _db.Proposals.FindAsync(new object[] { proposalId }, ct)!; if (proposal!.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent) - { - _logger.LogWarning("Rejected line item delete on proposal {ProposalId} in status {Status}", proposalId, proposal.Status); throw new InvalidOperationException("Cannot modify line items on approved/sent proposals"); - } _db.LineItems.Remove(lineItem); await _db.SaveChangesAsync(ct); - _logger.LogInformation("Line item {LineItemId} deleted from proposal {ProposalId}", lineItemId, proposalId); - await _audit.LogAsync(AuditAction.EditLineItem, proposalId, $"Removed: {lineItem.Description}", ct); } diff --git a/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs b/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs index 065f4fe..bf33f52 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs @@ -7,7 +7,6 @@ using ProposalSystem.Infrastructure.Data; namespace ProposalSystem.Infrastructure.Services; -// Fix: API-H6 — add structured logging for state transitions and errors public class ProposalService : IProposalService { private readonly ProposalDbContext _db; @@ -62,16 +61,14 @@ public class ProposalService : IProposalService await _db.SaveChangesAsync(ct); await transaction.CommitAsync(ct); - _logger.LogInformation("Proposal {ProposalId} created with number {ProposalNumber} by user {UserId}", - proposal.Id, proposalNumber, _currentUser.UserId); - try { await _audit.LogAsync(AuditAction.Submit, proposal.Id, null, ct); } catch (Exception ex) { - _logger.LogError(ex, "Failed to write audit log for proposal {ProposalId} creation", proposal.Id); + // Fix: API-M11 — log audit failures instead of silently swallowing + _logger.LogError(ex, "Failed to write audit log for proposal submission {ProposalId}", proposal.Id); } try @@ -80,6 +77,7 @@ public class ProposalService : IProposalService } catch (Exception ex) { + // Fix: API-M11 — log job publish failures instead of silently swallowing _logger.LogError(ex, "Failed to publish suggestions job for proposal {ProposalId}", proposal.Id); } @@ -204,23 +202,13 @@ public class ProposalService : IProposalService ?? throw new KeyNotFoundException($"Proposal {id} not found"); if (proposal.Status == ProposalStatus.Approved) - { - _logger.LogInformation("Proposal {ProposalId} already approved, returning idempotent response", id); return MapToResponse(proposal); - } if (proposal.Status != ProposalStatus.InReview) - { - _logger.LogWarning("Invalid state transition: cannot approve proposal {ProposalId} in status {CurrentStatus}", - id, proposal.Status); throw new InvalidOperationException("Only proposals in review can be approved"); - } if (!proposal.LineItems.Any() || proposal.LineItems.All(li => li.TotalPrice <= 0)) - { - _logger.LogWarning("Cannot approve proposal {ProposalId}: no priced line items", id); throw new InvalidOperationException("Cannot approve proposal without priced line items"); - } proposal.Status = ProposalStatus.Approved; proposal.ApprovedById = _currentUser.UserId; @@ -231,9 +219,6 @@ public class ProposalService : IProposalService await _db.SaveChangesAsync(ct); await _audit.LogAsync(AuditAction.Approve, id, null, ct); - _logger.LogInformation("Proposal {ProposalId} approved by user {UserId}, total bid {TotalBidAmount}", - id, _currentUser.UserId, proposal.TotalBidAmount); - return MapToResponse(proposal); } @@ -246,17 +231,10 @@ public class ProposalService : IProposalService ?? throw new KeyNotFoundException($"Proposal {id} not found"); if (proposal.Status == ProposalStatus.Sent) - { - _logger.LogInformation("Proposal {ProposalId} already sent, returning idempotent response", id); return MapToResponse(proposal); - } if (proposal.Status != ProposalStatus.Approved) - { - _logger.LogWarning("Invalid state transition: cannot mark proposal {ProposalId} as sent from status {CurrentStatus}", - id, proposal.Status); throw new InvalidOperationException("Only approved proposals can be marked as sent"); - } proposal.Status = ProposalStatus.Sent; proposal.SentAt = DateTime.UtcNow; @@ -265,8 +243,6 @@ public class ProposalService : IProposalService await _db.SaveChangesAsync(ct); await _audit.LogAsync(AuditAction.MarkSent, id, null, ct); - _logger.LogInformation("Proposal {ProposalId} marked as sent by user {UserId}", id, _currentUser.UserId); - await _jobPublisher.PublishAsync("library-ingest", new { proposalId = id }, ct); return MapToResponse(proposal); @@ -284,19 +260,11 @@ public class ProposalService : IProposalService var existingRevision = await _db.Proposals .FirstOrDefaultAsync(p => p.ParentProposalId == proposal.Id, ct); if (existingRevision != null) - { - _logger.LogInformation("Proposal {ProposalId} already revised, returning existing revision {RevisionId}", - id, existingRevision.Id); return MapToResponse(existingRevision); - } } if (proposal.Status != ProposalStatus.Sent) - { - _logger.LogWarning("Invalid state transition: cannot revise proposal {ProposalId} in status {CurrentStatus}", - id, proposal.Status); throw new InvalidOperationException("Only sent proposals can be revised"); - } var revision = new Proposal { @@ -348,9 +316,6 @@ public class ProposalService : IProposalService await _audit.LogAsync(AuditAction.CreateRevision, revision.Id, $"Revised from {proposal.Id}", ct); - _logger.LogInformation("Proposal {ProposalId} revised to {RevisionId} (revision {RevisionNumber}) by user {UserId}", - id, revision.Id, revision.CurrentRevision, _currentUser.UserId); - return MapToResponse(revision); }