stwaddle/budget
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Security hardening

Browse Source 
- Remove OwnerUserId from BudgetDto: OIDC sub of the budget owner was
  being returned to all collaborators (including View-only users)
- Remove SharedWithUserId from ShareDto: other users' internal OIDC subs
  were visible to anyone with read access to a budget
- Delete MeController: scaffolding endpoint that returned sub to the
  browser; no legitimate frontend use case
- Restrict /healthz to require authorization: prevents unauthenticated
  probing of database connectivity
- Add input validation annotations to all request DTOs: [Required],
  [MaxLength], [Range(0,0.9999)] on EffectiveTaxRate, [EmailAddress] on
  share email — [ApiController] now returns 400 instead of 500 for
  invalid input hitting DB constraints
- Replace User.FindFirst("sub")!.Value with GetUserId() extension across
  all controllers: returns 401 instead of NullReferenceException (500)
  if a token lacks a sub claim

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Spencer Twaddle
2026-04-25 09:00:33 -05:00
parent a8cf6957b5
commit 6d1bc2ce2c
14 changed files with 131 additions and 77 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Budget.Api/Controllers/SharesController.cs
+18 -8
View File
@@ -13,15 +13,22 @@ namespace Budget.Api.Controllers;
[Authorize]
public class SharesController(AppDbContext db, BudgetAuthorizationService authz) : ControllerBase
{
private string UserId => User.FindFirst("sub")!.Value;
private IActionResult? TryGetUserId(out string userId)
{
var id = User.GetUserId();
if (id is null) { userId = string.Empty; return Unauthorized(); }
userId = id;
return null;
}
[HttpGet]
public async Task<IActionResult> List(Guid budgetId)
{
if (!await authz.CanReadAsync(budgetId, UserId)) return Forbid();
if (TryGetUserId(out var userId) is { } err) return err;
if (!await authz.CanReadAsync(budgetId, userId)) return Forbid();
var shares = await db.BudgetShares
.Where(s => s.BudgetId == budgetId)
.Select(s => new ShareDto(s.Id, s.SharedWithUserId, s.SharedWithEmail, s.Permission, s.IsPending, s.CreatedAt))
.Select(s => new ShareDto(s.Id, s.SharedWithEmail, s.Permission, s.IsPending, s.CreatedAt))
.ToListAsync();
return Ok(shares);
}
@@ -29,7 +36,8 @@ public class SharesController(AppDbContext db, BudgetAuthorizationService authz)
[HttpPost]
public async Task<IActionResult> Add(Guid budgetId, [FromBody] CreateShareRequest req)
{
if (!await authz.IsOwnerAsync(budgetId, UserId)) return Forbid();
if (TryGetUserId(out var userId) is { } err) return err;
if (!await authz.IsOwnerAsync(budgetId, userId)) return Forbid();
var existing = await db.BudgetShares
.FirstOrDefaultAsync(s => s.BudgetId == budgetId && s.SharedWithEmail == req.Email);
@@ -48,24 +56,26 @@ public class SharesController(AppDbContext db, BudgetAuthorizationService authz)
};
db.BudgetShares.Add(share);
await db.SaveChangesAsync();
return Ok(new ShareDto(share.Id, share.SharedWithUserId, share.SharedWithEmail, share.Permission, share.IsPending, share.CreatedAt));
return Ok(new ShareDto(share.Id, share.SharedWithEmail, share.Permission, share.IsPending, share.CreatedAt));
}
[HttpPut("{shareId:guid}")]
public async Task<IActionResult> Update(Guid budgetId, Guid shareId, [FromBody] UpdateShareRequest req)
{
if (!await authz.IsOwnerAsync(budgetId, UserId)) return Forbid();
if (TryGetUserId(out var userId) is { } err) return err;
if (!await authz.IsOwnerAsync(budgetId, userId)) return Forbid();
var share = await db.BudgetShares.FirstOrDefaultAsync(s => s.Id == shareId && s.BudgetId == budgetId);
if (share is null) return NotFound();
share.Permission = req.Permission;
await db.SaveChangesAsync();
return Ok(new ShareDto(share.Id, share.SharedWithUserId, share.SharedWithEmail, share.Permission, share.IsPending, share.CreatedAt));
return Ok(new ShareDto(share.Id, share.SharedWithEmail, share.Permission, share.IsPending, share.CreatedAt));
}
[HttpDelete("{shareId:guid}")]
public async Task<IActionResult> Revoke(Guid budgetId, Guid shareId)
{
if (!await authz.IsOwnerAsync(budgetId, UserId)) return Forbid();
if (TryGetUserId(out var userId) is { } err) return err;
if (!await authz.IsOwnerAsync(budgetId, userId)) return Forbid();
var share = await db.BudgetShares.FirstOrDefaultAsync(s => s.Id == shareId && s.BudgetId == budgetId);
if (share is null) return NotFound();
db.BudgetShares.Remove(share);