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 |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ❌ |
| /// Comma-separated, registered client IDs allowed to receive the longer mobile | ||
| /// refresh-token lifetime. Anonymous requests and caller-supplied scopes never qualify. | ||
| /// </summary> | ||
| public static string TrustedLongLivedClientIds = ""; |
There was a problem hiding this comment.
Mutable configuration state identified in Core/Resgrid.Config/OidcConfig.cs and the related fields in Core/Resgrid.Config/SessionSecurityConfig.cs:7-20, Core/Resgrid.Services/DepartmentSettingsService.cs:27, and Tests/Resgrid.Tests/Services/UserSessionServiceTests.cs:19-24. Mark TrustedLongLivedClientIds as readonly to communicate immutable configuration intent and prevent accidental reassignment.
Kody rule violation: Use `readonly` or `const` for Immutable Data
public static readonly string TrustedLongLivedClientIds = string.Empty;Prompt for LLM
File Core/Resgrid.Config/OidcConfig.cs:
Line 28:
Mutable configuration state identified in Core/Resgrid.Config/OidcConfig.cs and the related fields in Core/Resgrid.Config/SessionSecurityConfig.cs:7-20, Core/Resgrid.Services/DepartmentSettingsService.cs:27, and Tests/Resgrid.Tests/Services/UserSessionServiceTests.cs:19-24. Mark TrustedLongLivedClientIds as readonly to communicate immutable configuration intent and prevent accidental reassignment.
Suggested Code:
public static readonly string TrustedLongLivedClientIds = string.Empty;
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 diagnostics exposure identified in Core/Resgrid.Model/Services/IEmailService.cs through ipAddress and userAgent, with related usage in Core/Resgrid.Model/UserSession.cs:44, Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89, Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:40, and the listed JavaScript files. Ensure implementations redact or hash ipAddress and userAgent before logging, and avoid passing raw values unless strictly necessary.
Kody rule violation: Mask PII and secrets in logs
Prompt for LLM
File Core/Resgrid.Model/Services/IEmailService.cs:
Line 30:
Sensitive diagnostics exposure identified in Core/Resgrid.Model/Services/IEmailService.cs through ipAddress and userAgent, with related usage in Core/Resgrid.Model/UserSession.cs:44, Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89, Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:40, and the listed JavaScript files. Ensure implementations redact or hash ipAddress and userAgent before logging, and avoid passing raw values unless strictly necessary.
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.
Privacy-sensitive telemetry exposure identified in Core/Resgrid.Model/Services/IEmailService.cs through ipAddress and userAgent, with related usage in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:39-40, Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89, and the listed JavaScript files. Minimize downstream logging and metrics of raw PII and include purpose or lawful-basis context wherever diagnostics are emitted.
Kody rule violation: Redact PII in logs and metrics by default
Prompt for LLM
File Core/Resgrid.Model/Services/IEmailService.cs:
Line 30:
Privacy-sensitive telemetry exposure identified in Core/Resgrid.Model/Services/IEmailService.cs through ipAddress and userAgent, with related usage in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:39-40, Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89, and the listed JavaScript files. Minimize downstream logging and metrics of raw PII and include purpose or lawful-basis context wherever diagnostics are emitted.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return string.Equals(storedToken, bearerToken, StringComparison.Ordinal); | ||
| return FixedTimeSecretEquals(storedToken, bearerToken); | ||
| } | ||
| catch (Exception ex) |
There was a problem hiding this comment.
Undiagnosable exception path identified in Core/Resgrid.Services/DepartmentSsoService.cs and the related catch sites. Log the failure with structured context in catch (Exception ex), including departmentId and departmentCode, so SCIM bearer token validation failures remain traceable.
Kody rule violation: Include error context in structured logs
catch (Exception ex)
{
_logger.LogError(ex, "SCIM bearer token validation failed", new { departmentId, departmentCode });
return false;
}Prompt for LLM
File Core/Resgrid.Services/DepartmentSsoService.cs:
Line 354:
Undiagnosable exception path identified in Core/Resgrid.Services/DepartmentSsoService.cs and the related catch sites. Log the failure with structured context in catch (Exception ex), including departmentId and departmentCode, so SCIM bearer token validation failures remain traceable.
Suggested Code:
catch (Exception ex)
{
_logger.LogError(ex, "SCIM bearer token validation failed", new { departmentId, departmentCode });
return false;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| if (!IPAddress.TryParse(ipAddress, out var address) || | ||
| string.IsNullOrWhiteSpace(SessionSecurityConfig.IpLocationDatabasePath)) | ||
| return Task.FromResult<IpLocationResult>(null); |
There was a problem hiding this comment.
Nullable Task result identified in Core/Resgrid.Services/LocalIpLocationProvider.cs. Returning Task.FromResult(null) from a Task-returning method can produce unexpected null dereferences after await, so use an explicitly handled non-null task result pattern or redesign the contract.
Kody rule violation: Avoid Returning Null in Non-Async Task Methods
return Task.FromResult<IpLocationResult>(default(IpLocationResult));Prompt for LLM
File Core/Resgrid.Services/LocalIpLocationProvider.cs:
Line 33:
Nullable Task result identified in Core/Resgrid.Services/LocalIpLocationProvider.cs. Returning Task.FromResult<IpLocationResult>(null) from a Task-returning method can produce unexpected null dereferences after await, so use an explicitly handled non-null task result pattern or redesign the contract.
Suggested Code:
return Task.FromResult<IpLocationResult>(default(IpLocationResult));
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 identified in Providers/Resgrid.Providers.Bus/SignalrProvider.cs:136 and Web/Resgrid.Web/Startup.cs:149 via clientHandler.ServerCertificateCustomValidationCallback. Skipping server certificate validation allows impersonation and man-in-the-middle interception of secure communications.
Kody rule violation: Verify SSL/TLS Server Certificates
Prompt for LLM
File Providers/Resgrid.Providers.Bus/SignalrProvider.cs:
Line 104:
TLS certificate validation bypass identified in Providers/Resgrid.Providers.Bus/SignalrProvider.cs:136 and Web/Resgrid.Web/Startup.cs:149 via clientHandler.ServerCertificateCustomValidationCallback. Skipping server certificate validation allows impersonation and man-in-the-middle interception of secure communications.
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.
Swallowed exception identified in Providers/Resgrid.Providers.Email/PostmarkTemplateProvider.cs and Web/Resgrid.Web.Eventing/Middleware/SessionValidationHubFilter.cs:109. Capture the exception in catch (Exception ex), log it with relevant identifiers such as email, departmentName, and userName, and then return false or rethrow so failures remain diagnosable.
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:
Swallowed exception identified in Providers/Resgrid.Providers.Email/PostmarkTemplateProvider.cs and Web/Resgrid.Web.Eventing/Middleware/SessionValidationHubFilter.cs:109. Capture the exception in catch (Exception ex), log it with relevant identifiers such as email, departmentName, and userName, and then return false or rethrow so failures remain diagnosable.
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.
| <div class="details"> | ||
| <strong>Request details</strong><br> | ||
| Time (UTC): {{requested_on}}<br> | ||
| IP address: {{ip_address}}<br> |
There was a problem hiding this comment.
Sensitive request metadata exposure identified in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html and the related sites in Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89 and the listed JavaScript files. Avoid displaying or logging raw IP address values like {{ip_address}}, or replace them with redacted or tokenized data if operationally required.
Kody rule violation: Do not log PHI; mask and drop sensitive fields
Prompt for LLM
File Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html:
Line 39:
Sensitive request metadata exposure identified in Providers/Resgrid.Providers.Email/Template/PasswordRecovery.html and the related sites in Web/Resgrid.Web.Services/Controllers/v4/SessionsController.cs:89 and the listed JavaScript files. Avoid displaying or logging raw IP address values like {{ip_address}}, or replace them with redacted or tokenized data if operationally required.
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.
Unannotated file-system failure path identified in Tests/Resgrid.Tests/Services/LocalIpLocationProviderTests.cs:38 and the related I/O call sites. Wrap File.WriteAllTextAsync(path, ...) in try/catch for IOException or UnauthorizedAccessException and include the operation and temp file path in the failure message so test failures remain diagnosable.
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 '{nameof(longest_matching_local_cidr_provides_only_coarse_location)}' at path '{path}': {ex.Message}");
}Prompt for LLM
File Tests/Resgrid.Tests/Services/LocalIpLocationProviderTests.cs:
Line 20:
Unannotated file-system failure path identified in Tests/Resgrid.Tests/Services/LocalIpLocationProviderTests.cs:38 and the related I/O call sites. Wrap File.WriteAllTextAsync(path, ...) in try/catch for IOException or UnauthorizedAccessException and include the operation and temp file path in the failure message so test failures remain diagnosable.
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 '{nameof(longest_matching_local_cidr_provides_only_coarse_location)}' at path '{path}': {ex.Message}");
}
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 identified in Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml and the related querySelector call sites, including Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Zones.cshtml:166, Providers/Resgrid.Providers.Claims/ClaimsPrincipalFactory.cs:67, Web/Resgrid.Web/wwwroot/js/app/internal/profile/resgrid.profile.yourdepartments.js:51, Tests/Resgrid.Tests/Web/User/ProfileControllerPasswordResetSecurityTests.cs:51 and :61, and Web/Resgrid.Web/wwwroot/js/app/public/resgrid.password-recovery.js:11-12. querySelector(...) can return null, so access .content with optional chaining and a fallback.
Kody rule violation: Add null checks to prevent NullReferenceException
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 identified in Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml and the related querySelector call sites, including Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Zones.cshtml:166, Providers/Resgrid.Providers.Claims/ClaimsPrincipalFactory.cs:67, Web/Resgrid.Web/wwwroot/js/app/internal/profile/resgrid.profile.yourdepartments.js:51, Tests/Resgrid.Tests/Web/User/ProfileControllerPasswordResetSecurityTests.cs:51 and :61, and Web/Resgrid.Web/wwwroot/js/app/public/resgrid.password-recovery.js:11-12. querySelector(...) can return null, so access .content with optional chaining and a fallback.
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.
| } catch (e) {} | ||
| return ''; | ||
| function getAntiForgeryToken() { | ||
| return document.querySelector('meta[name="request-verification-token"]').content; |
There was a problem hiding this comment.
Null dereference risk identified in Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml and Tests/Resgrid.Tests/Web/User/ProfileControllerPasswordResetSecurityTests.cs:51. document.querySelector('meta[name="request-verification-token"]') can return null, so access .content with optional chaining and a default value.
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 identified in Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml and Tests/Resgrid.Tests/Web/User/ProfileControllerPasswordResetSecurityTests.cs:51. document.querySelector('meta[name="request-verification-token"]') can return null, so access .content with optional chaining and a default value.
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 call identified in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs, including line 88. Using .Result or .Wait() can deadlock the request pipeline and degrades asynchronous execution; replace it with await.
Kody rule violation: Avoid Blocking Calls to Async Methods
Prompt for LLM
File Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:
Line 75:
Blocking async call identified in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs, including line 88. Using .Result or .Wait() can deadlock the request pipeline and degrades asynchronous execution; replace it with await.
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.
Async blocking identified in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs, including line 88. Blocking with .Result or .Wait() can deadlock the request path and breaks the team's async rule; use async/await end-to-end and configure awaits appropriately.
Kody rule violation: Await async operations properly
Prompt for LLM
File Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs:
Line 75:
Async blocking identified in Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs, including line 88. Blocking with .Result or .Wait() can deadlock the request path and breaks the team's async rule; use async/await end-to-end and configure awaits appropriately.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }), | ||
| contentType: 'application/json', | ||
| contentType: 'application/json', | ||
| headers: { 'RequestVerificationToken': $('input[name="__RequestVerificationToken"]').first().val() }, |
There was a problem hiding this comment.
Missing antiforgery token guard identified in Web/Resgrid.Web/wwwroot/js/app/internal/profile/resgrid.profile.yourdepartments.js and the related request setup sites. Validate $('input[name="__RequestVerificationToken"]').first().val() before issuing the async POST so the operation fails deterministically instead of sending an invalid request.
Kody rule violation: Handle async operations with proper error handling
const token = $('input[name="__RequestVerificationToken"]').first().val();
if (!token) {
console.error('switchActiveDepartment missing antiforgery token', { op: 'switchActiveDepartment', departmentId });
return;
}
headers: { RequestVerificationToken: token },Prompt for LLM
File Web/Resgrid.Web/wwwroot/js/app/internal/profile/resgrid.profile.yourdepartments.js:
Line 51:
Missing antiforgery token guard identified in Web/Resgrid.Web/wwwroot/js/app/internal/profile/resgrid.profile.yourdepartments.js and the related request setup sites. Validate $('input[name="__RequestVerificationToken"]').first().val() before issuing the async POST so the operation fails deterministically instead of sending an invalid request.
Suggested Code:
const token = $('input[name="__RequestVerificationToken"]').first().val();
if (!token) {
console.error('switchActiveDepartment missing antiforgery token', { op: 'switchActiveDepartment', departmentId });
return;
}
headers: { RequestVerificationToken: token },
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
Summary
This PR introduces a broad authentication and account-security upgrade across the platform, centered on session tracking, safer password reset flows, stronger SSO enforcement, and more secure web/API/eventing integration.
What changed
Added first-class user session tracking and revocation
Hardened password reset and credential-change flows
Added stronger MFA protection for privileged operations
Improved SSO and external identity handling
Secured SignalR/eventing and web-to-API access
Expanded auditing
Added department-level password reset policy
Other fixes and security improvements
Functional impact
From a user and administrator perspective, this PR delivers:
It also lays the data and service foundation for session-aware authentication across web, API, eventing, SCIM, and console tooling.