-
Notifications
You must be signed in to change notification settings - Fork 5
Security: Fix authorization bypass and data exposure in UserProfile/Eventing APIs #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rf_registrant2user
Are you sure you want to change the base?
Changes from all commits
de01e4a
24c5dcc
f3a21ea
b629d3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -250,10 +250,31 @@ | |||||
| } | ||||||
| }).RequireAuthorization(); | ||||||
|
|
||||||
| // Get all users endpoint | ||||||
| app.MapGet("/api/users", async Task<IEnumerable<User>> (UserDB db) => | ||||||
| // Get all users endpoint - restricted to caller's profile only | ||||||
| app.MapGet("/api/users", async Task<Results<Ok<User>, NotFound, UnauthorizedHttpResult>> ( | ||||||
| UserDB db, | ||||||
| HttpContext httpContext, | ||||||
| ILogger<Program> logger) => | ||||||
| { | ||||||
| return await db.Users.ToListAsync(); | ||||||
| // Extract authenticated user's sub claim | ||||||
| var auth0Sub = httpContext.User.FindFirst("sub")?.Value; | ||||||
| if (string.IsNullOrEmpty(auth0Sub)) | ||||||
| { | ||||||
| logger.LogWarning("GET /api/users: Authentication required"); | ||||||
| return TypedResults.Unauthorized(); | ||||||
| } | ||||||
|
|
||||||
| // Return only the caller's profile | ||||||
| var user = await db.Users | ||||||
| .FirstOrDefaultAsync(u => u.Auth0Subject == auth0Sub); | ||||||
|
|
||||||
| if (user == null) | ||||||
| { | ||||||
| logger.LogWarning("GET /api/users: User not found for Auth0Subject {Auth0Subject}", auth0Sub); | ||||||
| return TypedResults.NotFound(); | ||||||
| } | ||||||
|
|
||||||
| return TypedResults.Ok(user); | ||||||
| }).RequireAuthorization(); | ||||||
|
|
||||||
| // Event registration endpoint | ||||||
|
|
@@ -322,20 +343,52 @@ | |||||
| }).RequireAuthorization(); | ||||||
|
|
||||||
| // Get user's event registrations | ||||||
| app.MapGet("/api/users/{userId}/registrations", async Task<IEnumerable<EventRegistration>> ( | ||||||
| app.MapGet("/api/users/{userId}/registrations", async Task<Results<Ok<IEnumerable<EventRegistration>>, UnauthorizedHttpResult, ForbidHttpResult, BadRequest<string>>> ( | ||||||
| string userId, | ||||||
| UserDB db, | ||||||
| HttpContext httpContext, | ||||||
| ILogger<Program> logger) => | ||||||
| { | ||||||
| if (!StrictId.Id<User>.TryParse(userId, out var parsedUserId)) | ||||||
| { | ||||||
| logger.LogWarning("Invalid userId format: {UserId}", userId); | ||||||
| return []; | ||||||
| return TypedResults.BadRequest("Invalid userId format"); | ||||||
| } | ||||||
|
|
||||||
| // Extract authenticated user's sub claim | ||||||
| var auth0Sub = httpContext.User.FindFirst("sub")?.Value; | ||||||
| if (string.IsNullOrEmpty(auth0Sub)) | ||||||
| { | ||||||
| logger.LogWarning("GET /api/users/{userId}/registrations: Authentication required", userId); | ||||||
| return TypedResults.Unauthorized(); | ||||||
| } | ||||||
|
|
||||||
| // Get the authenticated user's ID | ||||||
| var authenticatedUser = await db.Users | ||||||
| .FirstOrDefaultAsync(u => u.Auth0Subject == auth0Sub); | ||||||
|
|
||||||
| if (authenticatedUser == null) | ||||||
| { | ||||||
| logger.LogWarning("GET /api/users/{userId}/registrations: Authenticated user not found for Auth0Subject {Auth0Subject}", userId, auth0Sub); | ||||||
| return TypedResults.Unauthorized(); | ||||||
| } | ||||||
|
|
||||||
| // Check if the caller is the owner or an admin | ||||||
| bool isOwner = authenticatedUser.Id == parsedUserId; | ||||||
| bool isAdmin = httpContext.User.IsInRole("Admin"); | ||||||
|
||||||
| bool isAdmin = httpContext.User.IsInRole("Admin"); | |
| bool isAdmin = httpContext.User.IsInRole("VisageAdmin"); |
Copilot
AI
Jan 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The removed Auth0Subject mismatch check was necessary to prevent ownership bypass. Since the lookup now uses Auth0Subject directly, an existing user record will always belong to the authenticated user, making the removed check redundant. However, the redundant assignment on line 430 (inputUser.Auth0Subject = auth0Subject) is unnecessary since inputUser.Auth0Subject was already set on line 411 and won't be used when existing is not null. Consider removing line 430 for code clarity.
| inputUser.Auth0Subject = auth0Subject; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -475,7 +475,7 @@ static async Task<Results<BadRequest<string>, UnauthorizedHttpResult, ForbidHttp | |
| "Checked out successfully")); | ||
| } | ||
|
|
||
| static async Task<Results<BadRequest<string>, NotFound, Ok<EventRegistration>>> LookupByPin( | ||
| static async Task<Results<UnauthorizedHttpResult, NotFound, Ok<RegistrationDto>>> LookupByPin( | ||
| string pin, | ||
| EventDB db, | ||
| HttpContext http) | ||
|
|
@@ -484,7 +484,7 @@ static async Task<Results<BadRequest<string>, NotFound, Ok<EventRegistration>>> | |
| var auth0Sub = http.User.FindFirst("sub")?.Value; | ||
| if (string.IsNullOrEmpty(auth0Sub)) | ||
| { | ||
| return TypedResults.BadRequest("Authentication required"); | ||
| return TypedResults.Unauthorized(); | ||
| } | ||
|
|
||
| // Fast lookup using CheckInPin index | ||
|
|
@@ -497,7 +497,17 @@ static async Task<Results<BadRequest<string>, NotFound, Ok<EventRegistration>>> | |
| return TypedResults.NotFound(); | ||
| } | ||
|
|
||
| return TypedResults.Ok(registration); | ||
| // Map to DTO to avoid exposing sensitive fields | ||
| var dto = new RegistrationDto( | ||
| registration.Id, | ||
| registration.EventId, | ||
| registration.Auth0Subject, | ||
| registration.RegisteredAt, | ||
| registration.Status, | ||
| registration.CheckInPin | ||
| ); | ||
|
|
||
| return TypedResults.Ok(dto); | ||
| } | ||
|
|
||
| // Helper methods | ||
|
|
@@ -514,6 +524,17 @@ static string GenerateCheckInPin() | |
| // Use shared contracts so frontend and tests can reuse models. | ||
| // (Defined in `Visage.Shared.Models.CheckInDtos`) | ||
|
|
||
| /// <summary> | ||
| /// Limited DTO for registration lookup to avoid exposing sensitive fields | ||
| /// </summary> | ||
| record RegistrationDto( | ||
| StrictId.Id<EventRegistration> Id, | ||
| StrictId.Id<Event> EventId, | ||
| string Auth0Subject, | ||
| DateTime RegisteredAt, | ||
| RegistrationStatus Status, | ||
| string? CheckInPin | ||
| ); | ||
|
Comment on lines
+530
to
+537
|
||
|
|
||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The security fixes in this PR (authorization checks on /api/users, /api/users/{userId}/registrations, /register endpoints, and LookupByPin) lack test coverage. Given the critical security nature of these changes, consider adding integration tests to verify: 1) users can only access their own profile data, 2) unauthorized access to other users' registrations returns 403, 3) admin role grants appropriate access, and 4) LookupByPin properly restricts access. Reference existing test patterns in tests/Visage.Test.Aspire/ for examples.