Conversation
|
Important Review skippedToo many files! This PR contains 151 files, which is 1 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to Pro+ to raise the limit. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (25)
📒 Files selected for processing (151)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ❌ |
| { | ||
| // Session tracking is required by the Web BFF and is safe for pre-feature | ||
| // credentials because they are adopted lazily by the validation middleware. | ||
| public static bool TrackingEnabled = true; |
There was a problem hiding this comment.
Immutable configuration state in Core/Resgrid.Config/SessionSecurityConfig.cs is expressed as a mutable static field, which obscures intent and permits accidental reassignment. Mark TrackingEnabled as const, and apply the same immutability pattern to the related literals at Core/Resgrid.Config/SessionSecurityConfig.cs:8-8, :13-13, :14-14, :15-15, :16-16, :17-17, :18-18, :19-19, :20-20, Core/Resgrid.Config/OidcConfig.cs:16-16, :28-28, Core/Resgrid.Model/Identity/IdentityUser.cs:138-138, Core/Resgrid.Services/DepartmentSettingsService.cs:27-27, and Core/Resgrid.Services/LocalIpLocationProvider.cs:22-22.
Kody rule violation: Use `readonly` or `const` for Immutable Data
public const bool TrackingEnabled = true;Prompt for LLM
File Core/Resgrid.Config/SessionSecurityConfig.cs:
Line 7:
Immutable configuration state in Core/Resgrid.Config/SessionSecurityConfig.cs is expressed as a mutable static field, which obscures intent and permits accidental reassignment. Mark TrackingEnabled as const, and apply the same immutability pattern to the related literals at Core/Resgrid.Config/SessionSecurityConfig.cs:8-8, :13-13, :14-14, :15-15, :16-16, :17-17, :18-18, :19-19, :20-20, Core/Resgrid.Config/OidcConfig.cs:16-16, :28-28, Core/Resgrid.Model/Identity/IdentityUser.cs:138-138, Core/Resgrid.Services/DepartmentSettingsService.cs:27-27, and Core/Resgrid.Services/LocalIpLocationProvider.cs:22-22.
Suggested Code:
public const bool TrackingEnabled = true;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| ClientSessionMetadata Parse(string userAgent, string deviceName = null, string deviceType = null, | ||
| string operatingSystem = null, string browser = null, string applicationVersion = null); |
There was a problem hiding this comment.
Null-dereference risk in Core/Resgrid.Model/Services/IClientSessionMetadataParser.cs arises because Parse(string userAgent, string deviceName = null, string deviceType = null, string operatingSystem = null, string browser = null, string applicationVersion = null) permits null defaults without nullable reference annotations. Mark deviceName, deviceType, operatingSystem, browser, and applicationVersion as string? and require implementations and callers to guard nulls across the related usages, including Core/Resgrid.Services/EmailService.cs:87-87 and Web/Resgrid.Web/Controllers/AccountController.cs:751-751 and :812-812.
Kody rule violation: Add null checks to prevent NullReferenceException
ClientSessionMetadata Parse(string userAgent, string? deviceName = null, string? deviceType = null,
string? operatingSystem = null, string? browser = null, string? applicationVersion = null);Prompt for LLM
File Core/Resgrid.Model/Services/IClientSessionMetadataParser.cs:
Line 7 to 8:
Null-dereference risk in Core/Resgrid.Model/Services/IClientSessionMetadataParser.cs arises because Parse(string userAgent, string deviceName = null, string deviceType = null, string operatingSystem = null, string browser = null, string applicationVersion = null) permits null defaults without nullable reference annotations. Mark deviceName, deviceType, operatingSystem, browser, and applicationVersion as string? and require implementations and callers to guard nulls across the related usages, including Core/Resgrid.Services/EmailService.cs:87-87 and Web/Resgrid.Web/Controllers/AccountController.cs:751-751 and :812-812.
Suggested Code:
ClientSessionMetadata Parse(string userAgent, string? deviceName = null, string? deviceType = null,
string? operatingSystem = null, string? browser = null, string? applicationVersion = null);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// <param name="departmentName">Name of the department.</param> | ||
| Task<bool> SendPasswordResetEmail(string emailAddress, string name, string userName, string password, | ||
| Task<bool> SendPasswordRecoveryEmail(string emailAddress, string name, string departmentName, | ||
| string resetUrl, string ipAddress, string userAgent, DateTime requestedOn, bool isSsoManaged); |
There was a problem hiding this comment.
Sensitive data propagation in Core/Resgrid.Model/Services/IEmailService.cs exposes raw IP address and user agent through a service contract, increasing the likelihood that these values reach logs, metrics, or downstream systems in clear form. Rename the interface parameters to non-identifying forms such as ipAddressHash and userAgentToken so callers and implementations treat them as redacted by default across the related call sites.
Kody rule violation: Redact PII in logs and metrics by default
string resetUrl, string ipAddressHash, string userAgentToken, DateTime requestedOn, bool isSsoManaged);Prompt for LLM
File Core/Resgrid.Model/Services/IEmailService.cs:
Line 30:
Sensitive data propagation in Core/Resgrid.Model/Services/IEmailService.cs exposes raw IP address and user agent through a service contract, increasing the likelihood that these values reach logs, metrics, or downstream systems in clear form. Rename the interface parameters to non-identifying forms such as ipAddressHash and userAgentToken so callers and implementations treat them as redacted by default across the related call sites.
Suggested Code:
string resetUrl, string ipAddressHash, string userAgentToken, DateTime requestedOn, bool isSsoManaged);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| await _emailProvider.SendPasswordResetMail(name, password, userName, emailAddress, departmentName); | ||
| await _emailProvider.SendPasswordRecoveryMail(name, emailAddress, departmentName, | ||
| resetUrl, ipAddress, userAgent, requestedOn.ToString("u"), isSsoManaged); |
There was a problem hiding this comment.
Sensitive telemetry exposure in Core/Resgrid.Services/EmailService.cs persists raw ipAddress and userAgent during account recovery, which can leak personal or diagnostic data through storage, logging, or external templates. Redact or tokenize these fields before propagation, including the related usages in Core/Resgrid.Model/Services/IEmailService.cs:30-30, Web/Resgrid.Web/Controllers/AccountController.cs:702-702 and :879-879, Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:39-39 and :40-40, and Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89-89.
Kody rule violation: Mask PII and secrets in logs
resetUrl, RedactIp(ipAddress), RedactUserAgent(userAgent), requestedOn.ToString("u"), isSsoManaged);Prompt for LLM
File Core/Resgrid.Services/EmailService.cs:
Line 87:
Sensitive telemetry exposure in Core/Resgrid.Services/EmailService.cs persists raw ipAddress and userAgent during account recovery, which can leak personal or diagnostic data through storage, logging, or external templates. Redact or tokenize these fields before propagation, including the related usages in Core/Resgrid.Model/Services/IEmailService.cs:30-30, Web/Resgrid.Web/Controllers/AccountController.cs:702-702 and :879-879, Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:39-39 and :40-40, and Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89-89.
Suggested Code:
resetUrl, RedactIp(ipAddress), RedactUserAgent(userAgent), requestedOn.ToString("u"), isSsoManaged);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| await _emailProvider.SendPasswordResetMail(name, password, userName, emailAddress, departmentName); | ||
| await _emailProvider.SendPasswordRecoveryMail(name, emailAddress, departmentName, | ||
| resetUrl, ipAddress, userAgent, requestedOn.ToString("u"), isSsoManaged); |
There was a problem hiding this comment.
Sensitive account-recovery metadata exposure in Core/Resgrid.Services/EmailService.cs emits raw ipAddress and userAgent in clear form during a high-risk flow. Replace them with redacted or stable non-identifying values and keep any raw access details in an audit trail instead, including the related outputs in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:39-39 and Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.js:144-144.
Kody rule violation: Do not log PHI; mask and drop sensitive fields
resetUrl, RedactIp(ipAddress), RedactUserAgent(userAgent), requestedOn.ToString("u"), isSsoManaged);Prompt for LLM
File Core/Resgrid.Services/EmailService.cs:
Line 87:
Sensitive account-recovery metadata exposure in Core/Resgrid.Services/EmailService.cs emits raw ipAddress and userAgent in clear form during a high-risk flow. Replace them with redacted or stable non-identifying values and keep any raw access details in an audit trail instead, including the related outputs in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:39-39 and Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.js:144-144.
Suggested Code:
resetUrl, RedactIp(ipAddress), RedactUserAgent(userAgent), requestedOn.ToString("u"), isSsoManaged);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| foreach (var departmentLinks in links.GroupBy(link => link.DepartmentId)) | ||
| { | ||
| var configs = (await _ssoConfigRepository.GetAllByDepartmentIdAsync(departmentLinks.Key))?.ToList() |
There was a problem hiding this comment.
N+1 repository access in Core/Resgrid.Services/ExternalIdentityLinkService.cs calls _ssoConfigRepository.GetAllByDepartmentIdAsync(departmentLinks.Key) inside a foreach over grouped links, causing one query per department. Batch configuration retrieval for all departmentIds up front or add a repository method that loads all required configs in a single call.
Kody rule violation: Detect N+1 style queries and suggest batching
var departmentIds = links.Select(link => link.DepartmentId).Distinct().ToList();
// fetch/batch configs for all departments once, then look up per groupPrompt for LLM
File Core/Resgrid.Services/ExternalIdentityLinkService.cs:
Line 96:
N+1 repository access in Core/Resgrid.Services/ExternalIdentityLinkService.cs calls _ssoConfigRepository.GetAllByDepartmentIdAsync(departmentLinks.Key) inside a foreach over grouped links, causing one query per department. Batch configuration retrieval for all departmentIds up front or add a repository method that loads all required configs in a single call.
Suggested Code:
var departmentIds = links.Select(link => link.DepartmentId).Distinct().ToList();
// fetch/batch configs for all departments once, then look up per group
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // bypass SSL certificate validation | ||
| clientHandler.ServerCertificateCustomValidationCallback += | ||
| (sender, certificate, chain, sslPolicyErrors) => { return true; }; | ||
| clientHandler.ServerCertificateCustomValidationCallback = |
There was a problem hiding this comment.
TLS certificate validation bypass in Providers/Resgrid.Providers.Bus/SignalrProvider.cs allows server impersonation and secure-transport interception, including the occurrences at Providers/Resgrid.Providers.Bus/SignalrProvider.cs:136-136 and Web/Resgrid.Web/Startup.cs:149-149. Enforce normal certificate validation on clientHandler.ServerCertificateCustomValidationCallback.
Kody rule violation: Verify SSL/TLS Server Certificates
Prompt for LLM
File Providers/Resgrid.Providers.Bus/SignalrProvider.cs:
Line 104:
TLS certificate validation bypass in Providers/Resgrid.Providers.Bus/SignalrProvider.cs allows server impersonation and secure-transport interception, including the occurrences at Providers/Resgrid.Providers.Bus/SignalrProvider.cs:136-136 and Web/Resgrid.Web/Startup.cs:149-149. Enforce normal certificate validation on clientHandler.ServerCertificateCustomValidationCallback.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| newEmail.To.Add(email); | ||
| return await _emailSender.Send(newEmail); | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
Exception swallowing in Providers/Resgrid.Providers.Email/PostmarkTemplateProvider.cs uses catch (Exception) without capturing or logging the failure, making email-delivery errors invisible. Capture the exception as ex and log it with relevant identifiers such as email, departmentName, and userName before returning false, and apply the same pattern to the related catch blocks in Web/Resgrid.Web.Services/Middleware/SessionValidationHubFilter.cs:105-105 and Web/Resgrid.Web/Middleware/SessionValidationMiddleware.cs:134-137.
Kody rule violation: Avoid empty catch blocks
catch (Exception ex)
{
_logger.LogError(ex, "Failed to send password-changed-by-administrator email", new { email, departmentName, userName });
return false;
}Prompt for LLM
File Providers/Resgrid.Providers.Email/PostmarkTemplateProvider.cs:
Line 374:
Exception swallowing in Providers/Resgrid.Providers.Email/PostmarkTemplateProvider.cs uses catch (Exception) without capturing or logging the failure, making email-delivery errors invisible. Capture the exception as ex and log it with relevant identifiers such as email, departmentName, and userName before returning false, and apply the same pattern to the related catch blocks in Web/Resgrid.Web.Services/Middleware/SessionValidationHubFilter.cs:105-105 and Web/Resgrid.Web/Middleware/SessionValidationMiddleware.cs:134-137.
Suggested Code:
catch (Exception ex)
{
_logger.LogError(ex, "Failed to send password-changed-by-administrator email", new { email, departmentName, userName });
return false;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // PostgreSQL has no concurrent DROP TABLE. Rollback requires ACCESS EXCLUSIVE and must | ||
| // be scheduled for a maintenance window when identity-link writes are still possible. | ||
| if (Schema.Table("userexternalidentitylinks").Exists()) | ||
| Delete.Table("userexternalidentitylinks"); |
There was a problem hiding this comment.
Migration rollback lock risk in Providers/Resgrid.Providers.MigrationsPg/Migrations/M0122_AddUserExternalIdentityLinksPg.cs uses Delete.Table("userexternalidentitylinks"), which can require an ACCESS EXCLUSIVE lock and block traffic. Replace the direct destructive drop with an online expand-contract rollback strategy or document a guarded maintenance-window rollback plan, and review the related migration at Providers/Resgrid.Providers.MigrationsPg/Migrations/M0120_AddUserAuthenticationStatePg.cs:14-14.
Kody rule violation: Block risky database migrations (locking ops, downtime risk)
Prompt for LLM
File Providers/Resgrid.Providers.MigrationsPg/Migrations/M0122_AddUserExternalIdentityLinksPg.cs:
Line 47:
Migration rollback lock risk in Providers/Resgrid.Providers.MigrationsPg/Migrations/M0122_AddUserExternalIdentityLinksPg.cs uses Delete.Table("userexternalidentitylinks"), which can require an ACCESS EXCLUSIVE lock and block traffic. Replace the direct destructive drop with an online expand-contract rollback strategy or document a guarded maintenance-window rollback plan, and review the related migration at Providers/Resgrid.Providers.MigrationsPg/Migrations/M0120_AddUserAuthenticationStatePg.cs:14-14.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| var path = Path.GetTempFileName(); | ||
| try | ||
| { | ||
| await File.WriteAllTextAsync(path, """ |
There was a problem hiding this comment.
File I/O failure handling is missing in Tests/Resgrid.Tests/Services/LocalIpLocationProviderTests.cs, so await File.WriteAllTextAsync(path, ...) can fail nondeterministically on permissions, locks, or path issues without actionable context. Catch IOException and fail the test with the path and exception message to make the failure diagnosable, and apply the same pattern where external file-system calls appear in related locations such as Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs:258-258 and :557-557.
Kody rule violation: Add try-catch blocks for external calls
try
{
await File.WriteAllTextAsync(path, """
[
{ "network": "203.0.0.0/16", "country": "US", "region": "Broad" },
{ "network": "203.0.113.0/24", "country": "US", "region": "California", "city": "Example City" }
]
""");
}
catch (IOException ex)
{
Assert.Fail($"Failed to write temp IP location database for test path '{path}': {ex.Message}");
}Prompt for LLM
File Tests/Resgrid.Tests/Services/LocalIpLocationProviderTests.cs:
Line 20:
File I/O failure handling is missing in Tests/Resgrid.Tests/Services/LocalIpLocationProviderTests.cs, so await File.WriteAllTextAsync(path, ...) can fail nondeterministically on permissions, locks, or path issues without actionable context. Catch IOException and fail the test with the path and exception message to make the failure diagnosable, and apply the same pattern where external file-system calls appear in related locations such as Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs:258-258 and :557-557.
Suggested Code:
try
{
await File.WriteAllTextAsync(path, """
[
{ "network": "203.0.0.0/16", "country": "US", "region": "Broad" },
{ "network": "203.0.113.0/24", "country": "US", "region": "California", "city": "Example City" }
]
""");
}
catch (IOException ex)
{
Assert.Fail($"Failed to write temp IP location database for test path '{path}': {ex.Message}");
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var currentSessionId = User.FindFirstValue(SessionClaimTypes.SessionId); | ||
| if (string.IsNullOrWhiteSpace(currentSessionId)) | ||
| return Conflict(new { error = "legacy_session", message = "Refresh this session before revoking all others." }); |
There was a problem hiding this comment.
Incorrect HTTP status semantics in Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs classify the legacy_session case as a 409 resource conflict even though the message describes a client precondition or input-state problem. Return 400 BadRequest unless a documented concurrency or resource-conflict condition justifies Conflict(...).
Kody rule violation: Use appropriate HTTP status codes
return BadRequest(new { error = "legacy_session", message = "Refresh this session before revoking all others." });Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:
Line 58:
Incorrect HTTP status semantics in Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs classify the legacy_session case as a 409 resource conflict even though the message describes a client precondition or input-state problem. Return 400 BadRequest unless a documented concurrency or resource-conflict condition justifies Conflict(...).
Suggested Code:
return BadRequest(new { error = "legacy_session", message = "Refresh this session before revoking all others." });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (group != null) | ||
| await group.SendAsync("unitStatusUpdated", id); | ||
| DemandInternalPublisher(); | ||
| return Clients.Group(departmentId.ToString()).SendAsync(method, id); |
There was a problem hiding this comment.
Rule mismatch in Web/Resgrid.Web.Services/Hubs/EventingHub.cs: Clients.Group(departmentId.ToString()).SendAsync(method, id) sends a SignalR message and does not register a listener or subscription. If this path is meant to handle listener registration, add explicit error and cleanup handling there; otherwise remove the subscription-safety concern from this call.
Kody rule violation: Provide error handlers to subscription/listener APIs
Prompt for LLM
File Web/Resgrid.Web.Services/Hubs/EventingHub.cs:
Line 71:
Rule mismatch in Web/Resgrid.Web.Services/Hubs/EventingHub.cs: Clients.Group(departmentId.ToString()).SendAsync(method, id) sends a SignalR message and does not register a listener or subscription. If this path is meant to handle listener registration, add explicit error and cleanup handling there; otherwise remove the subscription-safety concern from this call.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| await _deleteService.DeleteUserAccountAsync(DepartmentId, UserId, UserId, IpAddressHelper.GetRequestIP(Request, true), $"{Request.Headers["User-Agent"]} {Request.Headers["Accept-Language"]}", cancellationToken); | ||
| return RedirectToAction("LogOff", "Account", new { area = "" }); | ||
| await HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); |
There was a problem hiding this comment.
Unhandled sign-out failure in Web/Resgrid.Web/Areas/User/Controllers/AccountController.cs leaves await HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); without error mapping or diagnostic context. Wrap the call in try/catch, log structured identifiers such as UserId and DepartmentId, and rethrow or translate the failure so authentication state errors are observable.
Kody rule violation: Handle async operations with proper error handling
try
{
await HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
}
catch (Exception ex)
{
_logger.LogError(ex, "Sign-out failed for user account deletion", new { UserId, DepartmentId });
throw;
}Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/AccountController.cs:
Line 93:
Unhandled sign-out failure in Web/Resgrid.Web/Areas/User/Controllers/AccountController.cs leaves await HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme); without error mapping or diagnostic context. Wrap the call in try/catch, log structured identifiers such as UserId and DepartmentId, and rethrow or translate the failure so authentication state errors are observable.
Suggested Code:
try
{
await HttpContext.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
}
catch (Exception ex)
{
_logger.LogError(ex, "Sign-out failed for user account deletion", new { UserId, DepartmentId });
throw;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _systemAuditsService.SaveSystemAuditAsync(new SystemAudit | ||
| { | ||
| System = (int)SystemAuditSystems.Website, | ||
| Type = (int)SystemAuditTypes.EmailChanged, | ||
| UserId = UserId, | ||
| TargetUserId = model.UserId, | ||
| Successful = true, | ||
| IpAddress = IpAddressHelper.GetRequestIP(Request, true), | ||
| ServerName = Environment.MachineName, | ||
| CorrelationId = HttpContext.TraceIdentifier, | ||
| Data = "Account email changed; all sessions and tokens revoked.", | ||
| LoggedOn = now | ||
| }, cancellationToken); |
There was a problem hiding this comment.
Incomplete audit schema in Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs records an email-change event without clearly emitting all required immutable fields, including actor.role, action, resource.id, result, timestamp, trace_id, and user_agent. Emit the full audit shape on _systemAuditsService.SaveSystemAuditAsync(...) for this security-relevant operation, and align the downstream handling in Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs:43-43.
Kody rule violation: Emit tamper-evident audit logs with required fields
await _systemAuditsService.SaveSystemAuditAsync(new SystemAudit
{
TimestampUtc = now,
ActorUserId = UserId,
ActorRole = callerIsDepartmentAdmin ? "department_admin" : "user",
Action = "user.email.change",
ResourceId = model.UserId,
Result = "success",
TraceId = HttpContext.TraceIdentifier,
IpAddress = IpAddressHelper.GetRequestIP(Request, true),
UserAgent = Request.Headers["User-Agent"].ToString()
}, cancellationToken);Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs:
Line 912 to 924:
Incomplete audit schema in Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs records an email-change event without clearly emitting all required immutable fields, including actor.role, action, resource.id, result, timestamp, trace_id, and user_agent. Emit the full audit shape on _systemAuditsService.SaveSystemAuditAsync(...) for this security-relevant operation, and align the downstream handling in Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs:43-43.
Suggested Code:
await _systemAuditsService.SaveSystemAuditAsync(new SystemAudit
{
TimestampUtc = now,
ActorUserId = UserId,
ActorRole = callerIsDepartmentAdmin ? "department_admin" : "user",
Action = "user.email.change",
ResourceId = model.UserId,
Result = "success",
TraceId = HttpContext.TraceIdentifier,
IpAddress = IpAddressHelper.GetRequestIP(Request, true),
UserAgent = Request.Headers["User-Agent"].ToString()
}, cancellationToken);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch (e) {} | ||
| return ''; | ||
| function getAntiForgeryToken() { | ||
| return document.querySelector('meta[name="request-verification-token"]').content; |
There was a problem hiding this comment.
Null dereference risk in Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml arises because document.querySelector('meta[name="request-verification-token"]') can return null, making direct .content access unsafe. Use optional chaining with a default empty string, and apply the same guard to the related access in Web/Resgrid.Web/Areas/User/Controllers/AccountSecurityController.cs:72-72.
Kody rule violation: Add null checks before accessing properties
return document.querySelector('meta[name="request-verification-token"]')?.content ?? '';Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml:
Line 304:
Null dereference risk in Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml arises because document.querySelector('meta[name="request-verification-token"]') can return null, making direct .content access unsafe. Use optional chaining with a default empty string, and apply the same guard to the related access in Web/Resgrid.Web/Areas/User/Controllers/AccountSecurityController.cs:72-72.
Suggested Code:
return document.querySelector('meta[name="request-verification-token"]')?.content ?? '';
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| if (RequireForOperation) | ||
| { | ||
| context.Result = new StatusCodeResult(StatusCodes.Status503ServiceUnavailable); |
There was a problem hiding this comment.
Blocking async methods in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs with .Result or .Wait() can deadlock request execution and degrade asynchronous throughput, including the occurrence at Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:88-88. Replace the blocking path with await so the attribute remains fully asynchronous.
Kody rule violation: Avoid Blocking Calls to Async Methods
Prompt for LLM
File Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:
Line 75:
Blocking async methods in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs with .Result or .Wait() can deadlock request execution and degrade asynchronous throughput, including the occurrence at Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:88-88. Replace the blocking path with await so the attribute remains fully asynchronous.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| if (RequireForOperation) | ||
| { | ||
| context.Result = new StatusCodeResult(StatusCodes.Status503ServiceUnavailable); |
There was a problem hiding this comment.
Blocking async execution in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs can deadlock the request pipeline and violates the team's async rule, including the occurrence at Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:88-88. Convert the call chain to async/await end-to-end instead of using .Result or .Wait().
Kody rule violation: Await async operations properly
Prompt for LLM
File Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:
Line 75:
Blocking async execution in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs can deadlock the request pipeline and violates the team's async rule, including the occurrence at Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:88-88. Convert the call chain to async/await end-to-end instead of using .Result or .Wait().
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| /// Requires MFA for the decorated operation even when the department-wide administrator | ||
| /// 2FA setting is disabled. | ||
| /// </summary> | ||
| public bool RequireForOperation { get; set; } |
There was a problem hiding this comment.
MFA freshness enforcement gap in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs leaves RequireForOperation without a visible step-up verification window or audit timestamp, weakening privileged-operation protection. Require re-authentication within the defined freshness window and record mfa_verified_at in audit logging, including the related paths in Core/Resgrid.Model/TwoFactor/TwoFactorEnforcementContext.cs:45-45 and Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs:42-42.
Kody rule violation: Require step-up MFA for privileged operations
Prompt for LLM
File Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:
Line 46:
MFA freshness enforcement gap in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs leaves RequireForOperation without a visible step-up verification window or audit timestamp, weakening privileged-operation protection. Require re-authentication within the defined freshness window and record mfa_verified_at in audit logging, including the related paths in Core/Resgrid.Model/TwoFactor/TwoFactorEnforcementContext.cs:45-45 and Workers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs:42-42.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await _emailService.SendPasswordResetEmail(user.Email, profile.FullName.AsFirstNameLastName, user.UserName, newPassword, department.Name); | ||
| // Recovery is fail-closed, but the public response stays generic to avoid | ||
| // account enumeration and infrastructure-state disclosure. | ||
| Logging.LogException(ex, "Public password recovery request processing failed."); |
There was a problem hiding this comment.
Insufficient error context in Web/Resgrid.Web/Controllers/AccountController.cs logs only a message string with ex, which limits correlation and incident diagnosis. Log structured fields such as op = "ForgotPassword", a relevant identifier like an email hash, and HttpContext.TraceIdentifier along with the exception, and apply the same pattern to the related locations including Core/Resgrid.Services/EmailService.cs:92-92 and Web/Resgrid.Web.Eventing/Middleware/SessionValidationMiddleware.cs:63-63.
Kody rule violation: Include error context in structured logs
logger.error("Public password recovery request processing failed", new { op = "ForgotPassword", email = model.Email, traceId = HttpContext.TraceIdentifier, err = ex });Prompt for LLM
File Web/Resgrid.Web/Controllers/AccountController.cs:
Line 709:
Insufficient error context in Web/Resgrid.Web/Controllers/AccountController.cs logs only a message string with ex, which limits correlation and incident diagnosis. Log structured fields such as op = "ForgotPassword", a relevant identifier like an email hash, and HttpContext.TraceIdentifier along with the exception, and apply the same pattern to the related locations including Core/Resgrid.Services/EmailService.cs:92-92 and Web/Resgrid.Web.Eventing/Middleware/SessionValidationMiddleware.cs:63-63.
Suggested Code:
logger.error("Public password recovery request processing failed", new { op = "ForgotPassword", email = model.Email, traceId = HttpContext.TraceIdentifier, err = ex });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @RenderSection("Styles", required: false) | ||
| </head> | ||
| <body class="landing-page"> | ||
| <main class="container" style="max-width: 900px; padding-top: 40px;"> |
There was a problem hiding this comment.
Inline presentation logic in Web/Resgrid.Web/Views/Shared/_RecoveryLayout.cshtml couples markup to styling and reduces reuse, including the related occurrence in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:7-7. Move the style declarations into a dedicated CSS class such as recovery-layout-main.
Kody rule violation: Use component-scoped styling
<main class="container recovery-layout-main">Prompt for LLM
File Web/Resgrid.Web/Views/Shared/_RecoveryLayout.cshtml:
Line 13:
Inline presentation logic in Web/Resgrid.Web/Views/Shared/_RecoveryLayout.cshtml couples markup to styling and reduces reuse, including the related occurrence in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:7-7. Move the style declarations into a dedicated CSS class such as recovery-layout-main.
Suggested Code:
<main class="container recovery-layout-main">
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
No description provided.