Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Important Review skippedToo many files! This PR contains 229 files, which is 79 over the limit of 150. 📒 Files selected for processing (229)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [ProducesResponseType(typeof(SendVerificationCodeResult), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status429TooManyRequests)] | ||
| public async Task<ActionResult<SendVerificationCodeResult>> SendVerificationCode( |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix missing CSRF validation on ASP.NET Core MVC controllers that rely on cookie-based auth, you add antiforgery validation to all unsafe HTTP methods (POST, PUT, DELETE, etc.), typically by applying [ValidateAntiForgeryToken] on each action or [AutoValidateAntiforgeryToken] on the controller or globally. This ensures that each POST request must include a valid antiforgery token associated with the current user’s session, blocking cross-site POSTs from an attacker’s site.
For this specific issue, the least intrusive and clearest fix—without changing existing behavior beyond adding CSRF protection—is to decorate the SendVerificationCode action with [ValidateAntiForgeryToken]. ASP.NET Core’s antiforgery attributes live in Microsoft.AspNetCore.Mvc, which is already imported, so no new using directives are required. We only need to insert the attribute above the existing [HttpPost("SendVerificationCode")] attribute, within ContactVerificationController in Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs. This will cause ASP.NET Core to enforce that a valid antiforgery token is provided with the POST request to SendVerificationCode, mitigating CSRF attacks while preserving the existing logic and responses.
| @@ -33,6 +33,7 @@ | ||
| /// <summary> | ||
| /// Generates and sends a verification code to the specified contact method. | ||
| /// </summary> | ||
| [ValidateAntiForgeryToken] | ||
| [HttpPost("SendVerificationCode")] | ||
| [ProducesResponseType(typeof(SendVerificationCodeResult), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] |
| [HttpPost("ConfirmVerificationCode")] | ||
| [ProducesResponseType(typeof(ConfirmVerificationCodeResult), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| public async Task<ActionResult<ConfirmVerificationCodeResult>> ConfirmVerificationCode( |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, POST actions that mutate or confirm security-sensitive state should be protected against CSRF by validating an antiforgery token. In ASP.NET Core MVC, this is typically done with the [ValidateAntiForgeryToken] attribute on each action, or [AutoValidateAntiforgeryToken] globally on the controller or via filters. The client must then send a valid antiforgery token with each POST.
For this specific code, the minimal change that satisfies the CodeQL finding without altering existing logic is to add [ValidateAntiForgeryToken] to the ConfirmVerificationCode action method, alongside the existing [HttpPost("ConfirmVerificationCode")] attribute. This ensures that whenever this endpoint is invoked via POST, the framework will validate the antiforgery token and reject requests without a valid token, mitigating CSRF. No changes to the method body are required. The necessary attribute is available from the already imported Microsoft.AspNetCore.Mvc namespace, so no new imports are needed.
Concretely:
- In
Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs, locate theConfirmVerificationCodeaction (lines 77–99 in the snippet). - Add
[ValidateAntiForgeryToken]directly above the method declaration (line 83 in the snippet), beneath the existingHttpPostandProducesResponseTypeattributes.
| @@ -80,6 +80,7 @@ | ||
| [HttpPost("ConfirmVerificationCode")] | ||
| [ProducesResponseType(typeof(ConfirmVerificationCodeResult), StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<ConfirmVerificationCodeResult>> ConfirmVerificationCode( | ||
| [FromBody] ConfirmVerificationCodeInput model, | ||
| CancellationToken cancellationToken) |
| [HttpPost("Create")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowCredential_Create)] | ||
| public async Task<ActionResult<SaveCredentialResult>> Create([FromBody] WorkflowCredentialInput input, CancellationToken ct) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix missing CSRF validation for ASP.NET Core MVC actions that handle POST requests and rely on cookie-based authentication, you should enable antiforgery token validation. This is usually done by decorating the action (or the entire controller) with [ValidateAntiForgeryToken] or [AutoValidateAntiforgeryToken], and ensuring that clients send the antiforgery token with each state-changing request. For a single problematic method, the minimal and targeted fix is to add the validation attribute directly to that method.
For this specific case, the best fix with minimal impact is to add [ValidateAntiForgeryToken] to the Create action in WorkflowCredentialsController. This maintains existing functionality while adding CSRF protection to the credential-creation endpoint. ASP.NET Core’s antiforgery attributes live in Microsoft.AspNetCore.Mvc, which is already imported in this file, so we do not need new using directives. The concrete change is: in Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs, directly above the Create method declaration (currently annotated with [HttpPost("Create")], [ProducesResponseType(...)], and [Authorize(...)]), insert [ValidateAntiForgeryToken].
| @@ -68,6 +68,7 @@ | ||
| [HttpPost("Create")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowCredential_Create)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<SaveCredentialResult>> Create([FromBody] WorkflowCredentialInput input, CancellationToken ct) | ||
| { | ||
| if (!ModelState.IsValid) |
| [HttpPost("Save")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Workflow_Create)] | ||
| public async Task<ActionResult<WorkflowDetailResult>> Save([FromBody] SaveWorkflowInput input, CancellationToken ct) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the problem, we should ensure that ASP.NET Core’s antiforgery mechanism is applied to this state-changing POST action. The standard way is to add the [ValidateAntiForgeryToken] attribute (or [AutoValidateAntiforgeryToken] at a broader scope) so that incoming POST requests must include a valid antiforgery token. This mitigates CSRF when cookies are used for authentication.
The minimal, targeted fix without altering existing functionality is:
- Import
Microsoft.AspNetCore.Mvcis already present, so we can use[ValidateAntiForgeryToken]directly. - Add the
[ValidateAntiForgeryToken]attribute to theSaveaction method, directly above its method signature, preserving all other attributes ([HttpPost("Save")],[ProducesResponseType],[Authorize]) and logic.
Concretely in Web/Resgrid.Web.Services/Controllers/v4/WorkflowsController.cs:
- Locate the
Savemethod at line 77. - Insert
[ValidateAntiForgeryToken]between[Authorize(Policy = ResgridResources.Workflow_Create)]and the method declaration. - No other method bodies or behaviors are changed.
This ensures that any POST to /Workflows/Save (or the equivalent route) will require and validate an antiforgery token, closing the CSRF gap flagged by CodeQL.
| @@ -74,6 +74,7 @@ | ||
| [HttpPost("Save")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Workflow_Create)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<WorkflowDetailResult>> Save([FromBody] SaveWorkflowInput input, CancellationToken ct) | ||
| { | ||
| if (!ModelState.IsValid) return BadRequest(ModelState); |
| [HttpPost("SaveStep")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Workflow_Create)] | ||
| public async Task<ActionResult<WorkflowStepResult>> SaveStep([FromBody] SaveWorkflowStepInput input, CancellationToken ct) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, POST (and other state-changing) controller actions that can be triggered from a browser should validate an anti-forgery (CSRF) token. In ASP.NET Core MVC, the standard fix is to add the [ValidateAntiForgeryToken] attribute (or rely on a global AutoValidateAntiforgeryToken filter) and ensure that clients include a valid token with each request (e.g., via hidden form fields or an XSRF-TOKEN header).
For this specific method, the minimal fix without changing existing behavior is to decorate SaveStep with [ValidateAntiForgeryToken]. Since the file already imports Microsoft.AspNetCore.Mvc, no new using directive is necessary; the attribute is available from that namespace. We will add the attribute directly above SaveStep, following existing attribute style:
- Before:
SaveStephas[HttpPost("SaveStep")],[ProducesResponseType], and[Authorize]. - After: we insert
[ValidateAntiForgeryToken]between the existing attributes (or at least above the method), e.g.:
[HttpPost("SaveStep")]
[ProducesResponseType(StatusCodes.Status200OK)]
[Authorize(Policy = ResgridResources.Workflow_Create)]
[ValidateAntiForgeryToken]
public async Task<ActionResult<WorkflowStepResult>> SaveStep(...)No other code changes are required within the snippet. This ensures the framework will enforce the presence and validity of the CSRF token for this POST action.
| @@ -131,6 +131,7 @@ | ||
| [HttpPost("SaveStep")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.Workflow_Create)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<WorkflowStepResult>> SaveStep([FromBody] SaveWorkflowStepInput input, CancellationToken ct) | ||
| { | ||
| if (!ModelState.IsValid) return BadRequest(ModelState); |
| [HttpPost("SaveCredential")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowCredential_Create)] | ||
| public async Task<ActionResult<SaveCredentialResult>> SaveCredential([FromBody] SaveCredentialInput input, CancellationToken ct) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix missing CSRF validation on an ASP.NET Core POST action, you add the [ValidateAntiForgeryToken] (or [AutoValidateAntiforgeryToken]) attribute to the action (or controller) and ensure that any browser-based clients send a valid antiforgery token with their requests. This causes the framework to validate the token for each POST and reject forged requests.
For this specific issue, the best targeted fix is to decorate the SaveCredential action with [ValidateAntiForgeryToken] while leaving existing authorization and functionality intact. This ensures that POST requests to SaveCredential must include a valid antiforgery token, but does not change the logic of how credentials are saved. The change is localized to the method declaration region in Web/Resgrid.Web.Services/Controllers/v4/WorkflowsController.cs, around lines 210–214. Because Microsoft.AspNetCore.Mvc is already imported at the top of the file, no additional imports are required to use [ValidateAntiForgeryToken].
| @@ -211,6 +211,7 @@ | ||
| [HttpPost("SaveCredential")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowCredential_Create)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<SaveCredentialResult>> SaveCredential([FromBody] SaveCredentialInput input, CancellationToken ct) | ||
| { | ||
| if (!ModelState.IsValid) return BadRequest(ModelState); |
| [HttpPost("CancelRun/{runId}")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowRun_Delete)] | ||
| public async Task<ActionResult<DeleteWorkflowResult>> CancelRun(string runId, CancellationToken ct) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, the CancelRun POST action should validate an anti‑forgery token on each request. In ASP.NET Core, this is normally done by decorating the action with [ValidateAntiForgeryToken] and ensuring the client sends the token (for example using the standard form helpers or by sending the RequestVerificationToken header). This enforces that the request originates from a page that obtained a valid token from your application, mitigating CSRF.
The minimal, targeted fix is:
- Add the
ValidateAntiForgeryTokenattribute to theCancelRunmethod (line 331) inWeb/Resgrid.Web.Services/Controllers/v4/WorkflowsController.cs. - Ensure that the
Microsoft.AspNetCore.Mvcnamespace is already imported (it is, on line 7), so no extra imports are needed. - No behavior change occurs for correctly formed clients that include the anti‑forgery token; only forged/malicious requests without a valid token will now be rejected.
Concretely, in WorkflowsController.cs, above public async Task<ActionResult<DeleteWorkflowResult>> CancelRun(...), add [ValidateAntiForgeryToken] under the existing attributes. No other changes are required in this file to perform token validation.
| @@ -328,6 +328,7 @@ | ||
| [HttpPost("CancelRun/{runId}")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowRun_Delete)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<DeleteWorkflowResult>> CancelRun(string runId, CancellationToken ct) | ||
| { | ||
| var run = await _workflowService.GetWorkflowRunByIdAsync(runId, ct); |
| [HttpPost("ClearPending")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowRun_Delete)] | ||
| public async Task<ActionResult<DeleteWorkflowResult>> ClearPending(CancellationToken ct) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
In general, to fix missing CSRF protection on ASP.NET Core MVC POST actions that are intended for browser use with cookie-based authentication, you should require and validate an anti-forgery token on those actions. This is typically done by applying a validation attribute like [ValidateAntiForgeryToken] or by using a global filter that enforces validation on all unsafe HTTP methods.
For this specific issue, the best fix that preserves existing functionality is to add the [ValidateAntiForgeryToken] attribute to the ClearPending action so that any POST to /ClearPending must include a valid anti-forgery token. This assumes the rest of the app already uses the standard ASP.NET Core antiforgery infrastructure (e.g., tokens emitted in forms or via JavaScript). No changes are needed to the method body, return type, or route. We only add the attribute and ensure the corresponding namespace is referenced. In ASP.NET Core, ValidateAntiForgeryTokenAttribute lives in Microsoft.AspNetCore.Mvc, which is already imported at the top of the file, so no new using directive is required.
Concretely:
- In
Web/Resgrid.Web.Services/Controllers/v4/WorkflowsController.cs, locate theClearPendingaction (line 346). - Above this method (alongside the existing
[HttpPost],[ProducesResponseType], and[Authorize]attributes), add[ValidateAntiForgeryToken]. - Ensure no other code is modified.
| @@ -343,6 +343,7 @@ | ||
| [HttpPost("ClearPending")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.WorkflowRun_Delete)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<DeleteWorkflowResult>> ClearPending(CancellationToken ct) | ||
| { | ||
| if (!await CanUserViewWorkflowRunsAsync()) return Forbid(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs (1)
56-57:⚠️ Potential issue | 🔴 CriticalCritical:
Unauthorized()call does not stop execution.
Unauthorized()returns anIActionResultbut the result is not returned from the method. Code execution continues, potentially exposing sensitive data to unauthorized users. This is a pre-existing issue but affects new functionality.🐛 Proposed fix
if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) - Unauthorized(); + return Unauthorized();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs` around lines 56 - 57, The authorization check is calling ClaimsAuthorizationHelper.IsUserDepartmentAdmin() and invokes Unauthorized() but does not return it, so execution continues; update the action in SecurityController to return the Unauthorized() result immediately (e.g., change the branch to "return Unauthorized();" when !ClaimsAuthorizationHelper.IsUserDepartmentAdmin()), ensuring the method exits and no further sensitive logic runs.
🟡 Minor comments (27)
Repositories/Resgrid.Repositories.DataRepository/Queries/QueryList.cs-38-53 (1)
38-53:⚠️ Potential issue | 🟡 MinorUnused function parameter
x- code references outer scope variable instead.The lambda parameter
xis declared but never used. The code on line 40 referencestypefrom the outer scope instead. While this works becausegetConstructorParameters(type)is always called with the sametype, this is confusing and error-prone.🐛 Proposed fix
-var getConstructorParameters = new Func<Type, List<object>>(x => +var getConstructorParameters = new Func<Type, List<object>>(targetType => { - var constructorParameters = type.GetTypeInfo() + var constructorParameters = targetType.GetTypeInfo() .DeclaredConstructors .FirstOrDefault(y => y.IsPublic) ?.GetParameters();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/QueryList.cs` around lines 38 - 53, The lambda getConstructorParameters declares a parameter x but uses the outer variable type instead, causing an unused parameter and potential confusion; update the lambda body to reference the parameter x for reflection and constructor resolution (e.g., use x.GetTypeInfo().DeclaredConstructors... and create instances with ServiceLocator.Current.GetInstance(parameter.ParameterType)) so the declared Func<Type, List<object>> actually operates on its Type argument, or alternatively change the delegate to Func<List<object>> and remove the unused parameter if callers always pass the same type; adjust calls to getConstructorParameters accordingly.Core/Resgrid.Model/Repositories/IIdentityUserRepository.cs-37-39 (1)
37-39:⚠️ Potential issue | 🟡 MinorAdd
CancellationTokenparameter toGetTokenAsyncfor consistent cancellation propagation.Line 37 lacks a
CancellationTokenparameter whileSetTokenAsyncandRemoveTokenAsync(lines 38–39) include it. This creates an asymmetry in theIdentityUserStoreadapter, which receives aCancellationTokenfrom ASP.NET Core'sIUserAuthenticationTokenStore<IdentityUser>contract but cannot propagate it to the repository forGetTokenAsync. This breaks cancellation semantics and leaves get operations unable to respond to cancellation requests.Proposed fix
- Task<string> GetTokenAsync(string userId, string loginProvider, string name); + Task<string> GetTokenAsync(string userId, string loginProvider, string name, CancellationToken cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Repositories/IIdentityUserRepository.cs` around lines 37 - 39, The interface method GetTokenAsync is missing a CancellationToken which breaks cancellation propagation compared to SetTokenAsync and RemoveTokenAsync; update the IIdentityUserRepository signature for GetTokenAsync(string userId, string loginProvider, string name, CancellationToken cancellationToken) and then update all implementations (and any callers such as IdentityUserStore and any places implementing IUserAuthenticationTokenStore<IdentityUser>) to accept and pass the CancellationToken through so get operations can be cancelled consistently.Core/Resgrid.Services/EmailService.cs-648-649 (1)
648-649:⚠️ Potential issue | 🟡 MinorValidate
verificationCodebefore sending.The method currently checks only the recipient address. A blank/null code still sends an unusable verification email.
💡 Suggested fix
public async Task<bool> SendEmailVerificationCodeAsync(string toEmailAddress, string firstName, string verificationCode) { if (string.IsNullOrWhiteSpace(toEmailAddress)) return false; + if (string.IsNullOrWhiteSpace(verificationCode)) + return false; try { + var safeFirstName = string.IsNullOrWhiteSpace(firstName) ? "there" : firstName; using var mail = new MailMessage(); mail.To.Add(toEmailAddress); mail.Subject = "Resgrid Verification Code"; mail.From = new MailAddress(Config.OutboundEmailServerConfig.FromMail, "Resgrid"); - mail.Body = $"Hi {firstName},\r\n\r\nYour Resgrid verification code is: {verificationCode}\r\n\r\nThis code expires in {Config.VerificationConfig.VerificationCodeExpiryMinutes} minutes. If you did not request this, please ignore this email.\r\n\r\nThe Resgrid Team"; + mail.Body = $"Hi {safeFirstName},\r\n\r\nYour Resgrid verification code is: {verificationCode}\r\n\r\nThis code expires in {Config.VerificationConfig.VerificationCodeExpiryMinutes} minutes. If you did not request this, please ignore this email.\r\n\r\nThe Resgrid Team"; mail.IsBodyHtml = false;Also applies to: 657-657
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/EmailService.cs` around lines 648 - 649, The method currently only validates toEmailAddress; add a guard that returns false when verificationCode is null/empty/whitespace (use string.IsNullOrWhiteSpace(verificationCode)) before proceeding to send the email so blank codes are never sent; apply the same check at both send sites referencing the verificationCode variable (in the same method in EmailService.cs where toEmailAddress is checked and at the other occurrence around line 657).Providers/Resgrid.Providers.Workflow/Executors/DropboxFileExecutor.cs-43-45 (1)
43-45:⚠️ Potential issue | 🟡 MinorWriteMode "update" not handled correctly.
The UI (Edit.cshtml line 530) provides an "update" option for WriteMode, but this code only checks for "overwrite" and defaults everything else to
WriteMode.Add. The "update" option should map toWriteMode.Update.Instance.🔧 Proposed fix
- WriteMode writeMode = config.WriteMode?.ToLowerInvariant() == "overwrite" - ? WriteMode.Overwrite.Instance - : WriteMode.Add.Instance; + WriteMode writeMode = config.WriteMode?.ToLowerInvariant() switch + { + "overwrite" => WriteMode.Overwrite.Instance, + "update" => WriteMode.Update.Instance, + _ => WriteMode.Add.Instance + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/DropboxFileExecutor.cs` around lines 43 - 45, The WriteMode handling in DropboxFileExecutor.cs currently maps only "overwrite" to WriteMode.Overwrite and treats every other value as WriteMode.Add; update the logic in the code that sets the writeMode (using config.WriteMode) to also detect "update" (case-insensitively) and map it to WriteMode.Update.Instance, e.g. change the conditional/branch that assigns writeMode to include an "update" branch before falling back to WriteMode.Add so "update" from the UI maps correctly to WriteMode.Update.Instance.Providers/Resgrid.Providers.Workflow/Executors/SmtpEmailExecutor.cs-66-71 (1)
66-71:⚠️ Potential issue | 🟡 MinorRecipient cap logic may over-trim To addresses.
When
toAddresses.Count + ccAddresses.Count > maxRecipients, the code first truncatestoAddressestomaxRecipients(line 69), then calculatesremainingForCc. If there were originally more To addresses thanmaxRecipients, the logic is fine. However, the comment says "prioritising To over Cc" but iftoAddresses.Count <= maxRecipients, the To list is unnecessarily truncated on line 69. This line should only trim To if it exceedsmaxRecipients.🔧 Proposed fix
else { // Paid plan: cap total recipients at configured maximum var maxRecipients = WorkflowConfig.MaxEmailRecipients; if (toAddresses.Count + ccAddresses.Count > maxRecipients) { // Trim to fit within cap, prioritising To over Cc - var remainingForCc = Math.Max(0, maxRecipients - toAddresses.Count); - toAddresses = toAddresses.Take(maxRecipients).ToList(); + var toCount = Math.Min(toAddresses.Count, maxRecipients); + var remainingForCc = Math.Max(0, maxRecipients - toCount); + toAddresses = toAddresses.Take(toCount).ToList(); ccAddresses = ccAddresses.Take(remainingForCc).ToList(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/SmtpEmailExecutor.cs` around lines 66 - 71, The recipient-cap trimming currently always truncates toAddresses to maxRecipients then computes remainingForCc, which can over-trim To recipients; update the block in SmtpEmailExecutor (variables: toAddresses, ccAddresses, maxRecipients) so that you only trim toAddresses when toAddresses.Count > maxRecipients, then compute remainingForCc = Math.Max(0, maxRecipients - toAddresses.Count) and trim ccAddresses to remainingForCc; ensure the code path preserves full To list when its count is <= maxRecipients and only limits Cc as needed.Web/Resgrid.Web/Areas/User/Views/Workflows/Edit.cshtml-131-131 (1)
131-131:⚠️ Potential issue | 🟡 MinorXSS risk with Html.Raw and JSON serialization in HTML attribute context.
The
data-stepattribute usesHtml.RawwithSystem.Text.Json.JsonSerializer.Serialize, which produces valid JSON but does not escape for HTML attribute context. SinceOutputTemplateandActionConfiginSaveStepRequesthave no validation restrictions on special characters, a user can save values containing single quotes (e.g.,test'value). This breaks the single-quoted attribute:data-step='{"OutputTemplate":"test'value"}', terminating the attribute prematurely and creating an XSS vector.Remove
Html.Rawto let Razor automatically escape the output, or useHttpUtility.JavaScriptStringEncodeif the raw JSON must be preserved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Workflows/Edit.cshtml` at line 131, The data-step attribute is vulnerable to XSS because it uses Html.Raw(System.Text.Json.JsonSerializer.Serialize(step)), which does not escape JSON for HTML attribute context and allows single quotes in SaveStepRequest fields like OutputTemplate or ActionConfig to break the single-quoted attribute; fix by removing Html.Raw so Razor will HTML-encode the serialized JSON (i.e., use `@System.Text.Json.JsonSerializer.Serialize`(step)) or alternatively serialize then apply a proper encoder such as HttpUtility.JavaScriptStringEncode before inlining into the attribute, ensuring the data-step output is safely escaped for HTML attributes.Web/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtml-23-23 (1)
23-23:⚠️ Potential issue | 🟡 MinorUse localization for new 2FA menu text
Line 23 hardcodes “Two-Factor Authentication”; this should use localized resources like the rest of the nav labels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtml` at line 23, The hardcoded menu label "Two-Factor Authentication" in _Navigation.cshtml (the <a> for asp-controller="TwoFactor" asp-action="Index") should be replaced with the same localization helper used for other nav items in this view; locate the pattern used elsewhere in _Navigation.cshtml (e.g. the existing Localizer/translation helper invocations) and swap the literal string for a resource lookup (e.g. use the same Localizer/T helper and a resource key like "TwoFactorAuthentication" or an existing key) so the menu text is pulled from resources rather than hardcoded.Web/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtml-29-29 (1)
29-29:⚠️ Potential issue | 🟡 MinorLocalize the new “Workflows” label
Line 29 hardcodes English text while adjacent items are localized. This will bypass translations.
Suggested fix
-<li class="@Html.IsSelected(controller: "Workflows", action: "Index")"><a asp-controller="Workflows" asp-action="Index" asp-route-area="User">Workflows</a></li> +<li class="@Html.IsSelected(controller: "Workflows", action: "Index")"><a asp-controller="Workflows" asp-action="Index" asp-route-area="User">@commonLocalizer["Workflows"]</a></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtml` at line 29, The "Workflows" label in _TopNavbar.cshtml is hardcoded; update the anchor text for the <a asp-controller="Workflows" asp-action="Index" ...> element to use the shared localization resource (e.g., use Localizer["Workflows"] or your project's localization helper) instead of the literal string and add the "Workflows" key to the appropriate resource file so translations are available; ensure you modify the view around the anchor with asp-controller="Workflows" asp-action="Index" and verify rendering shows the localized value.Web/Resgrid.Web/Areas/User/Views/TwoFactor/Enable2FA.cshtml-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorParse
enforcedvalue instead of checking only key presence.Line 5 treats
?enforced=falseas enforced. That can incorrectly show enforced-warning behavior.Proposed fix
- bool enforced = Context.Request.Query.ContainsKey("enforced"); + bool enforced = bool.TryParse(Context.Request.Query["enforced"], out var isEnforced) && isEnforced;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/TwoFactor/Enable2FA.cshtml` at line 5, The current line sets enforced by only checking Context.Request.Query.ContainsKey("enforced"), which treats ?enforced=false as true; change it to parse the query value: read the string value via Context.Request.Query["enforced"], then if the value is empty treat as true (to preserve bare ?enforced behavior) otherwise use bool.TryParse(value, out enforced) with a default of false; update the variable `enforced` assignment accordingly so code referencing `enforced` reflects the parsed boolean.Providers/Resgrid.Providers.Workflow/WorkflowActionExecutorFactory.cs-13-16 (1)
13-16:⚠️ Potential issue | 🟡 MinorPotential runtime crash if duplicate
ActionTypeexecutors are registered.
ToDictionarythrowsArgumentExceptionif multiple executors share the sameActionType. This would cause a crash during DI container initialization, which may go unnoticed until deployment.Consider either validating explicitly with a clearer error message, or using a strategy that handles duplicates gracefully (e.g., taking the first or last).
🛡️ Proposed fix for clearer error handling
public WorkflowActionExecutorFactory(IEnumerable<IWorkflowActionExecutor> executors) { - _executors = executors.ToDictionary(e => e.ActionType); + _executors = new Dictionary<WorkflowActionType, IWorkflowActionExecutor>(); + foreach (var executor in executors) + { + if (_executors.ContainsKey(executor.ActionType)) + throw new InvalidOperationException($"Duplicate executor registration for WorkflowActionType '{executor.ActionType}'. Executor types: {_executors[executor.ActionType].GetType().Name} and {executor.GetType().Name}."); + _executors[executor.ActionType] = executor; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/WorkflowActionExecutorFactory.cs` around lines 13 - 16, The constructor for WorkflowActionExecutorFactory currently calls ToDictionary on the incoming IEnumerable<IWorkflowActionExecutor>, which will throw an ambiguous ArgumentException if duplicate ActionType keys exist; update the WorkflowActionExecutorFactory constructor to detect duplicates explicitly by grouping executors by e.ActionType and either (a) throw a clear InvalidOperationException that names the duplicated ActionType(s) and the offending executor types, or (b) choose a deterministic resolution (e.g., use Last() or First() per group) and build _executors from that resolved set so DI initialization won't crash unpredictably.Core/Resgrid.Services/SmsService.cs-350-354 (1)
350-354:⚠️ Potential issue | 🟡 MinorMissing validation for
verificationCodeparameter.The method validates
toPhoneNumberbut notverificationCode. IfverificationCodeis null or empty, the SMS will contain an incomplete/invalid message.🛡️ Proposed fix to add validation
public async Task<bool> SendSmsVerificationCodeAsync(string toPhoneNumber, string verificationCode, string departmentNumber) { - if (string.IsNullOrWhiteSpace(toPhoneNumber)) + if (string.IsNullOrWhiteSpace(toPhoneNumber) || string.IsNullOrWhiteSpace(verificationCode)) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/SmsService.cs` around lines 350 - 354, Add validation for the verificationCode parameter in SendSmsVerificationCodeAsync: check if verificationCode is null or whitespace (similar to the existing toPhoneNumber check) and return false early if invalid. Update the method SendSmsVerificationCodeAsync to validate both toPhoneNumber and verificationCode before proceeding with building/sending the SMS so the message is never sent with an empty code.Web/Resgrid.Web/Areas/User/Views/Workflows/Runs.cshtml-77-80 (1)
77-80:⚠️ Potential issue | 🟡 MinorPaging can expose a false “Next” page.
Line 77 shows Next when
Model.Count >= 50; if the final page has exactly 50 rows, users can land on an empty next page. Prefer a backend-providedHasNextPageflag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Workflows/Runs.cshtml` around lines 77 - 80, The view currently decides whether to render the "Next" button by checking Model.Count >= 50 (the check at Runs.cshtml using Model.Count >= 50), which can show a false Next when the final page has exactly 50 rows; change the view to rely on a backend-provided flag (e.g., Model.HasNextPage or a ViewModel property) instead of counting rows, and update the controller/query that populates the Runs view (the action that provides the model for the "Runs" view) to set HasNextPage correctly (by fetching pageSize+1 or using total count) so the Next link (asp-action="Runs" asp-route-page) is rendered only when HasNextPage is true.Core/Resgrid.Config/VerificationConfig.cs-28-32 (1)
28-32:⚠️ Potential issue | 🟡 MinorXML documentation comment may be inaccurate.
The
RecaptchaMinimumScoredocumentation mentions "department-registration endpoint", but this config file is for contact verification. The comment appears to be copied from elsewhere and should be updated to reflect its actual usage in the contact verification flow.📝 Proposed fix
/// <summary> -/// Minimum reCAPTCHA v3 score (0.0–1.0) required to pass bot-detection on the API -/// department-registration endpoint. Requests scoring below this threshold are rejected. +/// Minimum reCAPTCHA v3 score (0.0–1.0) required to pass bot-detection on contact +/// verification endpoints. Requests scoring below this threshold are rejected. /// </summary> public static decimal RecaptchaMinimumScore = 0.5m;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Config/VerificationConfig.cs` around lines 28 - 32, The XML doc for RecaptchaMinimumScore is inaccurate—update the summary on the RecaptchaMinimumScore field in VerificationConfig (and any surrounding comment) to describe that it is the minimum reCAPTCHA v3 score (0.0–1.0) required to pass bot-detection for the contact verification flow (not the department-registration endpoint), and ensure the comment language matches the file's purpose and usage.Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs-917-929 (1)
917-929:⚠️ Potential issue | 🟡 MinorInconsistent input validation between verification endpoints.
SendContactVerificationCodevalidates thatrequest.Typeis a defined enum value (Line 893), butConfirmContactVerificationCodedoes not perform this check. An invalidContactVerificationTypevalue could cause unexpected behavior in the service layer.🛡️ Proposed fix: Add enum validation
public async Task<IActionResult> ConfirmContactVerificationCode([FromBody] ConfirmContactVerificationCodeRequest request, CancellationToken cancellationToken) { - if (request == null || string.IsNullOrWhiteSpace(request.Code)) + if (request == null || string.IsNullOrWhiteSpace(request.Code) || !Enum.IsDefined(typeof(ContactVerificationType), request.Type)) return BadRequest();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/HomeController.cs` around lines 917 - 929, Add the same enum validation done in SendContactVerificationCode to ConfirmContactVerificationCode: ensure request isn't null and that request.Type is a valid ContactVerificationType (use Enum.IsDefined or TryParse) before calling _contactVerificationService. If the enum value is invalid, return BadRequest() and do not call ConfirmVerificationCodeAsync to prevent invalid values from reaching the service layer.Web/Resgrid.Web/Areas/User/Views/Workflows/Pending.cshtml-62-62 (1)
62-62:⚠️ Potential issue | 🟡 MinorQueued timestamp is rendered with a fixed format and no timezone context.
This can be confusing for users across locales/time zones. Prefer localized display (or explicit UTC labeling/conversion to user zone).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Workflows/Pending.cshtml` at line 62, The timestamp is rendered without timezone or localization using run.QueuedOn.ToString("yyyy-MM-dd HH:mm:ss") in Pending.cshtml; change it to present a localized/explicit-timezone value by converting run.QueuedOn to the user's/local timezone (e.g., call ToLocalTime() or use TimeZoneInfo) and include the offset or zone in the format (use a format that includes zzz or explicit UTC label) so the UI shows the correct zone context for run.QueuedOn.Providers/Resgrid.Providers.Workflow/Executors/FtpFileExecutor.cs-39-39 (1)
39-39:⚠️ Potential issue | 🟡 MinorWhitespace filename is not treated as missing.
??only handles null, so""or" "bypasses fallback and can build an invalid remote path. Usestring.IsNullOrWhiteSpacefor filename fallback.Suggested fix
- var remotePath = $"{config.RemotePath?.TrimEnd('/')}/{config.Filename ?? $"workflow_{DateTime.UtcNow:yyyyMMddHHmmss}.txt"}"; + var filename = string.IsNullOrWhiteSpace(config.Filename) + ? $"workflow_{DateTime.UtcNow:yyyyMMddHHmmss}.txt" + : config.Filename.Trim(); + var remotePath = $"{config.RemotePath?.TrimEnd('/')}/{filename}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/FtpFileExecutor.cs` at line 39, In FtpFileExecutor.cs update the remotePath construction to treat empty or whitespace filenames as missing by replacing the null-coalescing fallback for config.Filename with a check using string.IsNullOrWhiteSpace; i.e., when building remotePath (the line creating remotePath, referencing config.RemotePath and config.Filename in class FtpFileExecutor) use string.IsNullOrWhiteSpace(config.Filename) to decide whether to use config.Filename or the fallback $"workflow_{DateTime.UtcNow:yyyyMMddHHmmss}.txt".Providers/Resgrid.Providers.MigrationsPg/Migrations/M0041_EnsureAspNetUserTokensIndexPg.cs-35-39 (1)
35-39:⚠️ Potential issue | 🟡 Minor
Down()is asymmetric withUp()— table is not dropped.The
Up()method conditionally creates both the table and index, butDown()only removes the index, leaving the table intact. If this is intentional (e.g., to preserve data across rollbacks), consider adding a comment. Otherwise, the table should also be conditionally dropped.Possible fix if table should be dropped
public override void Down() { if (Schema.Table("AspNetUserTokens".ToLower()).Index("IX_AspNetUserTokens_UserId".ToLower()).Exists()) Delete.Index("IX_AspNetUserTokens_UserId".ToLower()).OnTable("AspNetUserTokens".ToLower()); + + if (Schema.Table("AspNetUserTokens".ToLower()).Exists()) + Delete.Table("AspNetUserTokens".ToLower()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0041_EnsureAspNetUserTokensIndexPg.cs` around lines 35 - 39, The Down() migration only deletes the index "IX_AspNetUserTokens_UserId" but does not reverse the conditional table creation performed in Up(), leaving the "AspNetUserTokens" table behind; update Down() in M0041_EnsureAspNetUserTokensIndexPg to either (A) conditionally drop the "AspNetUserTokens" table when it was created (check Schema.Table("AspNetUserTokens".ToLower()).Exists() before calling Delete.Table(...)) or (B) if keeping the table on rollback is intentional, add a clear comment in Down() referencing Up() to document that the table is intentionally preserved while only the index is removed; ensure the logic uses the same existence checks as Up() for symmetry.Providers/Resgrid.Providers.Workflow/Executors/SftpFileExecutor.cs-46-53 (1)
46-53:⚠️ Potential issue | 🟡 MinorRemove the
usingstatement fromkeyStream—PrivateKeyFiletakes ownership and disposes the stream.SSH.NET's
PrivateKeyFileconstructor disposes the stream passed to it during construction. Theusing var keyStreamat line 47 causes the stream to be disposed twice: once byPrivateKeyFileand again when the using block ends. WhileMemoryStream.Dispose()is idempotent and tolerates this, it's a code smell and violates best practices. Remove theusingkeyword and letPrivateKeyFilemanage the stream's lifetime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/SftpFileExecutor.cs` around lines 46 - 53, The code currently wraps the MemoryStream created from cred.PrivateKey in a using (using var keyStream) but then passes it to PrivateKeyFile which takes ownership and disposes the stream; remove the using so the MemoryStream is not disposed twice—change the keyStream declaration to a non-disposing local (e.g., var keyStream = new MemoryStream(...)) and keep constructing PrivateKeyFile as before (the PrivateKeyFile constructor will manage disposal), leaving the rest of the ConnectionInfo/PrivateKeyAuthenticationMethod creation (connectionInfo, PrivateKeyAuthenticationMethod) unchanged.Repositories/Resgrid.Repositories.DataRepository/WorkflowDailyUsageRepository.cs-36-57 (1)
36-57:⚠️ Potential issue | 🟡 MinorPotential
NullReferenceExceptionwhen_unitOfWorkis null.When
_unitOfWork?.Connection == null(line 48), the code creates a fresh connection but still passes_unitOfWork.Transactionat line 43. If_unitOfWorkitself is null, this will throw aNullReferenceException.Consider either:
- Passing
nullfor the transaction when using a standalone connection, or- Using
_unitOfWork?.Transactionto safely handle the null case.🐛 Proposed fix
var selectFunction = new Func<DbConnection, Task<int>>(async x => { var dp = new DynamicParametersExtension(); dp.Add("DepartmentId", departmentId); dp.Add("ActionType", actionType); dp.Add("UsageDate", utcDate.Date); var query = _queryFactory.GetQuery<SelectWorkflowDailyUsageQuery>(); - var record = await x.QueryFirstOrDefaultAsync<WorkflowDailyUsage>(sql: query, param: dp, transaction: _unitOfWork.Transaction); + var record = await x.QueryFirstOrDefaultAsync<WorkflowDailyUsage>(sql: query, param: dp, transaction: _unitOfWork?.Transaction); return record?.SendCount ?? 0; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/WorkflowDailyUsageRepository.cs` around lines 36 - 57, The selectFunction currently always passes _unitOfWork.Transaction into QueryFirstOrDefaultAsync which will throw if _unitOfWork is null; update the call inside selectFunction to use the safe null-aware transaction (_unitOfWork?.Transaction) or accept a transaction parameter and pass null when using the standalone connection created via _connectionProvider.Create(); ensure references to selectFunction, QueryFirstOrDefaultAsync, _unitOfWork.Transaction, conn and _connectionProvider.Create() are updated so the transaction argument is null-safe when _unitOfWork is null.Repositories/Resgrid.Repositories.DataRepository/WorkflowDailyUsageRepository.cs-97-102 (1)
97-102:⚠️ Potential issue | 🟡 MinorSame potential
NullReferenceExceptionand unusedCancellationToken.
- Line 102 has the same issue:
_unitOfWork.Transactionmay throw if_unitOfWorkis null.- The
cancellationTokenparameter is passed toOpenAsyncbut not utilized inExecuteAsync. Dapper supports cancellation viaCommandDefinition.🐛 Proposed fix for both issues
var dp = new DynamicParametersExtension(); dp.Add("Id", newId); dp.Add("DepartmentId", departmentId); dp.Add("ActionType", actionType); dp.Add("UsageDate", usageDate); -await x.ExecuteAsync(sql: upsertSql, param: dp, transaction: _unitOfWork.Transaction); +var cmd = new CommandDefinition(upsertSql, dp, transaction: _unitOfWork?.Transaction, cancellationToken: cancellationToken); +await x.ExecuteAsync(cmd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/WorkflowDailyUsageRepository.cs` around lines 97 - 102, The upsert block in WorkflowDailyUsageRepository can NRE on _unitOfWork.Transaction and ignores the passed cancellationToken; update the method to guard against a null _unitOfWork (throw ArgumentNullException or ensure _unitOfWork is initialized) before accessing _unitOfWork.Transaction and then call Dapper's ExecuteAsync using a CommandDefinition (or equivalent overload) that includes the DynamicParametersExtension dp, the upsertSql, the _unitOfWork.Transaction, and the cancellationToken so the operation can be canceled; reference symbols: WorkflowDailyUsageRepository, _unitOfWork.Transaction, ExecuteAsync, OpenAsync, cancellationToken, DynamicParametersExtension, upsertSql.Providers/Resgrid.Providers.Bus/WorkflowEventProvider.cs-99-99 (1)
99-99:⚠️ Potential issue | 🟡 Minor
async voidmethod risks unobserved exceptions.While the method has a try-catch, any exception in the
catchblock itself (e.g., fromLogging.LogException) would crash the process. Additionally, callers cannot await completion or handle failures. Consider usingasync Taskand letting the event aggregator handle fire-and-forget semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Bus/WorkflowEventProvider.cs` at line 99, The HandleEvent method is declared as async void which can lead to unobserved exceptions and prevents callers from awaiting it; change the signature of HandleEvent(int departmentId, WorkflowTriggerEventType eventType, object eventObj) from async void to async Task, update any places that subscribe/invoke it so they accept a Task-returning handler (or wrap calls with Task.Run/ignore via a deliberate fire-and-forget helper), and ensure the event aggregator or subscription registration uses Task-compatible delegates so exceptions can be observed/handled or logged safely rather than risking process crash from exceptions thrown inside the catch handler (e.g., Logging.LogException).Web/Resgrid.Web/Controllers/AccountController.cs-676-676 (1)
676-676:⚠️ Potential issue | 🟡 MinorTypo in cache key:
$"_${userId}"should be$"_{userId}".The cache key uses
$"_${userId}"which includes a literal$character due to the escaped$. This is likely a typo and should be$"_{userId}"for consistency with howCacheConfig.ApiBearerTokenKeyNameis used elsewhere.🐛 Proposed fix
-await _cacheProvider.SetStringAsync(CacheConfig.ApiBearerTokenKeyName + $"_${userId}", token, new TimeSpan(48, 0, 0)); +await _cacheProvider.SetStringAsync(CacheConfig.ApiBearerTokenKeyName + $"_{userId}", token, new TimeSpan(48, 0, 0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Controllers/AccountController.cs` at line 676, Fix the typo in the cache key used in the call to _cacheProvider.SetStringAsync: replace the malformed interpolated string that contains a literal dollar sign (`$"_${userId}"`) with the correct interpolation (`$"_{userId}"`) so the key is CacheConfig.ApiBearerTokenKeyName + $"_{userId}"; update the call in the AccountController method where token and userId are passed to _cacheProvider.SetStringAsync.Providers/Resgrid.Providers.Workflow/Executors/S3FileExecutor.cs-59-60 (1)
59-60:⚠️ Potential issue | 🟡 Minor
TransferUtilityshould be disposed.
TransferUtilityimplementsIDisposablebut is not wrapped in ausingstatement. While it may not hold significant resources in this simple case, proper disposal is best practice.♻️ Proposed fix
-var transferUtility = new TransferUtility(s3Client); -await transferUtility.UploadAsync(uploadRequest, cancellationToken); +using var transferUtility = new TransferUtility(s3Client); +await transferUtility.UploadAsync(uploadRequest, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/S3FileExecutor.cs` around lines 59 - 60, The TransferUtility instance created in S3FileExecutor (var transferUtility = new TransferUtility(s3Client)) is not disposed; wrap TransferUtility in a using/using-declaration to ensure it is disposed after awaiting transferUtility.UploadAsync(uploadRequest, cancellationToken). Locate the TransferUtility construction and UploadAsync call in S3FileExecutor and replace the direct instantiation with a using (or C# using declaration) so the TransferUtility is disposed even if UploadAsync throws, leaving s3Client, uploadRequest and cancellationToken usage unchanged.Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs-159-165 (1)
159-165:⚠️ Potential issue | 🟡 MinorAdd input validation to
WorkflowCredentialInput.The DTO lacks validation attributes, which could allow invalid data through even when
ModelState.IsValidis checked. At minimum,Nameshould be required.✏️ Proposed fix
public class WorkflowCredentialInput { + [Required] + [StringLength(250)] public string Name { get; set; } public int CredentialType { get; set; } /// <summary>Plaintext JSON blob of credential fields. Never returned in any GET response.</summary> + [Required] public string PlaintextData { get; set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs` around lines 159 - 165, Add data-annotation validation to the WorkflowCredentialInput DTO so invalid payloads are rejected even when ModelState.IsValid is checked; at minimum annotate the Name property with [Required] (and optionally [StringLength] to enforce length) and consider adding a [Range] or enum validation for CredentialType and [Required] or [JsonIgnore]/clear rules for PlaintextData as appropriate; ensure you import System.ComponentModel.DataAnnotations and update any model-binding/tests expecting the previous behavior in the WorkflowCredentialsController that consumes WorkflowCredentialInput.Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs-78-88 (1)
78-88:⚠️ Potential issue | 🟡 MinorHandle null department scenario.
If
GetDepartmentByIdAsyncreturns null,department?.Code ?? string.Emptypasses an empty code toSaveCredentialAsync. This could lead to encryption with an unintended key. Consider returning an error if the department is not found.🛡️ Proposed fix
var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId); +if (department == null) + return NotFound("Department not found."); + var credential = new WorkflowCredential { DepartmentId = DepartmentId, Name = input.Name, CredentialType = input.CredentialType, EncryptedData = input.PlaintextData, // WorkflowService.SaveCredentialAsync will encrypt CreatedByUserId = UserId }; -var saved = await _workflowService.SaveCredentialAsync(credential, department?.Code ?? string.Empty, ct); +var saved = await _workflowService.SaveCredentialAsync(credential, department.Code ?? string.Empty, ct);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs` around lines 78 - 88, When GetDepartmentByIdAsync(DepartmentId) returns null, do not call _workflowService.SaveCredentialAsync with an empty department code; instead detect the null department in the controller action (where department is assigned), and return a proper error response (e.g., NotFound or BadRequest) referencing the DepartmentId. Update the logic around the WorkflowCredential creation/SaveCredentialAsync call to validate department != null before using department?.Code, and include DepartmentId and UserId context in the error response to aid callers.Core/Resgrid.Services/ContactVerificationService.cs-246-256 (1)
246-256:⚠️ Potential issue | 🟡 MinorNormalize phone values before deciding verification reset.
The current comparison uses raw
MobileNumber/HomeNumberstrings. Formatting-only changes (e.g.,555-123-4567vs5551234567) will incorrectly reset verification state.✅ Suggested fix
- if (!string.Equals(existingProfile.MobileNumber ?? string.Empty, updatedProfile.MobileNumber ?? string.Empty, StringComparison.OrdinalIgnoreCase)) + if (!string.Equals(existingProfile.GetPhoneNumber(), updatedProfile.GetPhoneNumber(), StringComparison.Ordinal)) { updatedProfile.MobileNumberVerified = false; updatedProfile.MobileVerificationCode = null; updatedProfile.MobileVerificationCodeExpiry = null; updatedProfile.MobileVerificationAttempts = 0; updatedProfile.MobileVerificationAttemptsResetDate = null; } - if (!string.Equals(existingProfile.HomeNumber ?? string.Empty, updatedProfile.HomeNumber ?? string.Empty, StringComparison.OrdinalIgnoreCase)) + if (!string.Equals(existingProfile.GetHomePhoneNumber(), updatedProfile.GetHomePhoneNumber(), StringComparison.Ordinal)) { updatedProfile.HomeNumberVerified = false; updatedProfile.HomeVerificationCode = null; updatedProfile.HomeVerificationCodeExpiry = null; updatedProfile.HomeVerificationAttempts = 0; updatedProfile.HomeVerificationAttemptsResetDate = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/ContactVerificationService.cs` around lines 246 - 256, The comparison in ContactVerificationService (around the block that checks existingProfile.MobileNumber vs updatedProfile.MobileNumber and existingProfile.HomeNumber vs updatedProfile.HomeNumber) should normalize phone numbers before comparing so formatting-only differences don't reset verification; implement or call a NormalizePhoneNumber helper (e.g., strip all non-digit characters and treat null/empty as empty) and use string.Equals(normalize(existingProfile.MobileNumber), normalize(updatedProfile.MobileNumber), StringComparison.Ordinal) for both MobileNumber and HomeNumber checks, keeping the existing reset logic (MobileNumberVerified, MobileVerificationCode, MobileVerificationCodeExpiry, MobileVerificationAttempts, MobileVerificationAttemptsResetDate) unchanged when the normalized values differ.Core/Resgrid.Services/WorkflowService.cs-329-336 (1)
329-336:⚠️ Potential issue | 🟡 MinorMissing
RecursionLimiton Scriban context.
WorkflowConfig.ScribanRecursionLimit(50) is defined but not applied to theTemplateContext. This could allow malicious templates to cause stack overflows via deep recursion.🔒 Proposed fix
var scribanContext = new Scriban.TemplateContext { LoopLimit = WorkflowConfig.ScribanLoopLimit, + RecursionLimit = WorkflowConfig.ScribanRecursionLimit, StrictVariables = false };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/WorkflowService.cs` around lines 329 - 336, The Scriban TemplateContext in WorkflowService (variable scribanContext) lacks a RecursionLimit; set scribanContext.RecursionLimit = WorkflowConfig.ScribanRecursionLimit when constructing the TemplateContext (the same place where LoopLimit and StrictVariables are set) so the configured recursion limit (WorkflowConfig.ScribanRecursionLimit) is applied and prevents deep-recursion stack issues in TemplateContext.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (226)
Core/Resgrid.Config/ServiceBusConfig.csCore/Resgrid.Config/TwoFactorConfig.csCore/Resgrid.Config/VerificationConfig.csCore/Resgrid.Config/WorkflowConfig.csCore/Resgrid.Localization/Areas/User/Home/EditProfile.en.resxCore/Resgrid.Localization/Areas/User/Home/EditProfile.es.resxCore/Resgrid.Localization/Areas/User/Security/Security.csCore/Resgrid.Localization/Areas/User/Security/Security.en.resxCore/Resgrid.Localization/Areas/User/Security/Security.es.resxCore/Resgrid.Localization/Areas/User/TwoFactor/TwoFactor.csCore/Resgrid.Localization/Areas/User/TwoFactor/TwoFactor.en.resxCore/Resgrid.Localization/Areas/User/TwoFactor/TwoFactor.es.resxCore/Resgrid.Localization/Areas/User/Workflows/Workflows.csCore/Resgrid.Localization/Areas/User/Workflows/Workflows.en.resxCore/Resgrid.Localization/Areas/User/Workflows/Workflows.es.resxCore/Resgrid.Localization/Areas/User/Workflows/Workflows.resxCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/ContactVerificationExtensions.csCore/Resgrid.Model/ContactVerificationStatus.csCore/Resgrid.Model/ContactVerificationType.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Events/CertificationExpiringEvent.csCore/Resgrid.Model/Events/FormSubmittedEvent.csCore/Resgrid.Model/Events/GroupAddedEvent.csCore/Resgrid.Model/Events/GroupUpdatedEvent.csCore/Resgrid.Model/Events/InventoryAdjustedEvent.csCore/Resgrid.Model/Events/MessageSentEvent.csCore/Resgrid.Model/Events/PersonnelRoleChangedEvent.csCore/Resgrid.Model/Events/ResourceOrderAddedEvent.csCore/Resgrid.Model/Events/TrainingAddedEvent.csCore/Resgrid.Model/Events/TrainingUpdatedEvent.csCore/Resgrid.Model/PermissionTypes.csCore/Resgrid.Model/Plan.csCore/Resgrid.Model/Providers/IOutboundQueueProvider.csCore/Resgrid.Model/Providers/IRabbitOutboundQueueProvider.csCore/Resgrid.Model/Providers/IWorkflowActionExecutor.csCore/Resgrid.Model/Providers/IWorkflowActionExecutorFactory.csCore/Resgrid.Model/Providers/IWorkflowEventProvider.csCore/Resgrid.Model/Providers/IWorkflowTemplateContextBuilder.csCore/Resgrid.Model/Providers/WorkflowActionContext.csCore/Resgrid.Model/Providers/WorkflowActionResult.csCore/Resgrid.Model/Queue/WorkflowQueueItem.csCore/Resgrid.Model/Repositories/IIdentityUserRepository.csCore/Resgrid.Model/Repositories/IWorkflowCredentialRepository.csCore/Resgrid.Model/Repositories/IWorkflowDailyUsageRepository.csCore/Resgrid.Model/Repositories/IWorkflowRepository.csCore/Resgrid.Model/Repositories/IWorkflowRunLogRepository.csCore/Resgrid.Model/Repositories/IWorkflowRunRepository.csCore/Resgrid.Model/Repositories/IWorkflowStepRepository.csCore/Resgrid.Model/Services/IAuthorizationService.csCore/Resgrid.Model/Services/IContactVerificationService.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Model/Services/IEmailService.csCore/Resgrid.Model/Services/IEncryptionService.csCore/Resgrid.Model/Services/ISmsService.csCore/Resgrid.Model/Services/IWorkflowService.csCore/Resgrid.Model/SystemAuditTypes.csCore/Resgrid.Model/TemplateVariableDescriptor.csCore/Resgrid.Model/UserProfile.csCore/Resgrid.Model/Workflow.csCore/Resgrid.Model/WorkflowActionType.csCore/Resgrid.Model/WorkflowCredential.csCore/Resgrid.Model/WorkflowCredentialType.csCore/Resgrid.Model/WorkflowDailyUsage.csCore/Resgrid.Model/WorkflowHealthSummary.csCore/Resgrid.Model/WorkflowRun.csCore/Resgrid.Model/WorkflowRunLog.csCore/Resgrid.Model/WorkflowRunStatus.csCore/Resgrid.Model/WorkflowStep.csCore/Resgrid.Model/WorkflowTemplateVariableCatalog.csCore/Resgrid.Model/WorkflowTriggerEventType.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/AuthorizationService.csCore/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/ContactVerificationService.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/EmailService.csCore/Resgrid.Services/EncryptionService.csCore/Resgrid.Services/Resgrid.Services.csprojCore/Resgrid.Services/ServicesModule.csCore/Resgrid.Services/SmsService.csCore/Resgrid.Services/UserProfileService.csCore/Resgrid.Services/WorkflowSampleDataGenerator.csCore/Resgrid.Services/WorkflowService.csCore/Resgrid.Services/WorkflowTemplateContextBuilder.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitConnection.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.csProviders/Resgrid.Providers.Bus.Rabbit/RabbitOutboundQueueProvider.csProviders/Resgrid.Providers.Bus/BusModule.csProviders/Resgrid.Providers.Bus/OutboundQueueProvider.csProviders/Resgrid.Providers.Bus/WorkflowEventProvider.csProviders/Resgrid.Providers.Claims/ClaimsLogic.csProviders/Resgrid.Providers.Claims/ClaimsPrincipalFactory.csProviders/Resgrid.Providers.Claims/JwtTokenProvider.csProviders/Resgrid.Providers.Claims/ResgridClaimTypes.csProviders/Resgrid.Providers.Claims/ResgridResources.csProviders/Resgrid.Providers.Migrations/Migrations/M0037_AddingWorkflows.csProviders/Resgrid.Providers.Migrations/Migrations/M0038_AddingWorkflowAuditColumns.csProviders/Resgrid.Providers.Migrations/Migrations/M0039_AddingWorkflowDailyUsages.csProviders/Resgrid.Providers.Migrations/Migrations/M0040_AddingContactVerification.csProviders/Resgrid.Providers.Migrations/Migrations/M0041_EnsureAspNetUserTokensIndex.csProviders/Resgrid.Providers.Migrations/Migrations/M0042_AddWorkflowDailyUsagesUniqueConstraint.csProviders/Resgrid.Providers.Migrations/Migrations/M0043_FixAspNetUserTokensPrimaryKey.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0037_AddingWorkflowsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0038_AddingWorkflowAuditColumnsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0039_AddingWorkflowDailyUsagesPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0040_AddingContactVerificationPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0041_EnsureAspNetUserTokensIndexPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0042_AddWorkflowDailyUsagesUniqueConstraintPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0043_FixAspNetUserTokensPrimaryKeyPg.csProviders/Resgrid.Providers.Workflow/Executors/AzureBlobExecutor.csProviders/Resgrid.Providers.Workflow/Executors/BoxFileExecutor.csProviders/Resgrid.Providers.Workflow/Executors/DiscordMessageExecutor.csProviders/Resgrid.Providers.Workflow/Executors/DropboxFileExecutor.csProviders/Resgrid.Providers.Workflow/Executors/FtpFileExecutor.csProviders/Resgrid.Providers.Workflow/Executors/HttpApiExecutor.csProviders/Resgrid.Providers.Workflow/Executors/S3FileExecutor.csProviders/Resgrid.Providers.Workflow/Executors/SftpFileExecutor.csProviders/Resgrid.Providers.Workflow/Executors/SlackMessageExecutor.csProviders/Resgrid.Providers.Workflow/Executors/SmtpEmailExecutor.csProviders/Resgrid.Providers.Workflow/Executors/SsrfGuard.csProviders/Resgrid.Providers.Workflow/Executors/TeamsMessageExecutor.csProviders/Resgrid.Providers.Workflow/Executors/TwilioSmsExecutor.csProviders/Resgrid.Providers.Workflow/Executors/WebhookUrlValidator.csProviders/Resgrid.Providers.Workflow/Resgrid.Providers.Workflow.csprojProviders/Resgrid.Providers.Workflow/WorkflowActionExecutorFactory.csProviders/Resgrid.Providers.Workflow/WorkflowProviderModule.csRepositories/Resgrid.Repositories.DataRepository/Extensions/ColumnsBuilderExtensions.csRepositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Queries/QueryList.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectActiveWorkflowsByDeptAndEventTypeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectPendingRunsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectRunsInLastMinuteByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowByDeptAndEventTypeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowCredentialsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowDailyUsageQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowRunLogsByRunIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowRunsByDepartmentIdPagedQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowRunsByWorkflowIdPagedQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowStepsByWorkflowIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Workflows/SelectWorkflowsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Stores/IdentityUserStore.csRepositories/Resgrid.Repositories.DataRepository/WorkflowCredentialRepository.csRepositories/Resgrid.Repositories.DataRepository/WorkflowDailyUsageRepository.csRepositories/Resgrid.Repositories.DataRepository/WorkflowRepository.csRepositories/Resgrid.Repositories.DataRepository/WorkflowRunLogRepository.csRepositories/Resgrid.Repositories.DataRepository/WorkflowRunRepository.csRepositories/Resgrid.Repositories.DataRepository/WorkflowStepRepository.csResgrid.slnTests/Resgrid.Tests/Bootstrapper.csTests/Resgrid.Tests/Helpers/WorkflowHelpers.csTests/Resgrid.Tests/Providers/WorkflowActionExecutorTests.csTests/Resgrid.Tests/Resgrid.Tests.csprojTests/Resgrid.Tests/Services/ContactVerificationServiceTests.csTests/Resgrid.Tests/Services/EncryptionServiceTests.csTests/Resgrid.Tests/Services/WorkflowSampleDataGeneratorTests.csTests/Resgrid.Tests/Services/WorkflowServiceTests.csTests/Resgrid.Tests/Services/WorkflowTemplateContextBuilderTests.csTests/Resgrid.Tests/Services/WorkflowTemplateRenderingTests.csTests/Resgrid.Tests/Services/WorkflowTemplateVariableCatalogTests.csTools/Resgrid.Console/Program.csWeb/Resgrid.Web.Eventing/Startup.csWeb/Resgrid.Web.Mcp/Properties/launchSettings.jsonWeb/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.csWeb/Resgrid.Web.Services/Controllers/v4/SecurityController.csWeb/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.csWeb/Resgrid.Web.Services/Controllers/v4/WorkflowsController.csWeb/Resgrid.Web.Services/Models/v4/ContactVerification/ContactVerificationModels.csWeb/Resgrid.Web.Services/Models/v4/Security/DepartmentRightsResult.csWeb/Resgrid.Web.Services/Models/v4/Workflows/WorkflowModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.csprojWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web.Services/appsettings.jsonWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/SecurityController.csWeb/Resgrid.Web/Areas/User/Controllers/SubscriptionController.csWeb/Resgrid.Web/Areas/User/Controllers/TwoFactorController.csWeb/Resgrid.Web/Areas/User/Controllers/WorkflowsController.csWeb/Resgrid.Web/Areas/User/Models/EditProfileModel.csWeb/Resgrid.Web/Areas/User/Models/Home/ContactVerificationRequests.csWeb/Resgrid.Web/Areas/User/Models/Security/PermissionsView.csWeb/Resgrid.Web/Areas/User/Models/TwoFactor/TwoFactorViewModels.csWeb/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtmlWeb/Resgrid.Web/Areas/User/Views/Security/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtmlWeb/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtmlWeb/Resgrid.Web/Areas/User/Views/TwoFactor/Disable2FA.cshtmlWeb/Resgrid.Web/Areas/User/Views/TwoFactor/Enable2FA.cshtmlWeb/Resgrid.Web/Areas/User/Views/TwoFactor/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/TwoFactor/ShowRecoveryCodes.cshtmlWeb/Resgrid.Web/Areas/User/Views/TwoFactor/Verify2FA.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/CredentialEdit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/CredentialNew.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/Credentials.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/Health.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/Pending.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/Runs.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workflows/_CredentialFields.cshtmlWeb/Resgrid.Web/Areas/User/Views/Workshifts/ViewDay.cshtmlWeb/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.csWeb/Resgrid.Web/Controllers/AccountController.csWeb/Resgrid.Web/Middleware/Require2FAEnrollmentMiddleware.csWeb/Resgrid.Web/Models/AccountViewModels/VerifyCodeViewModel.csWeb/Resgrid.Web/Models/SaveStepRequest.csWeb/Resgrid.Web/Models/WorkflowCredentialViewModel.csWeb/Resgrid.Web/Resgrid.Web.csprojWeb/Resgrid.Web/Startup.csWeb/Resgrid.Web/Views/Account/LoginWith2fa.cshtmlWeb/Resgrid.Web/Views/Account/LoginWithRecoveryCode.cshtmlWeb/Resgrid.Web/WebBootstrapper.csWeb/Resgrid.Web/libman.jsonWeb/Resgrid.Web/wwwroot/js/app/internal/home/resgrid.home.edituserprofile.jsWeb/Resgrid.Web/wwwroot/js/app/internal/security/resgrid.security.permissions.jsWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/QueuesProcessorTask.csWorkers/Resgrid.Workers.Framework/Bootstrapper.csWorkers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.csWorkers/Resgrid.Workers.Framework/Logic/WorkflowQueueLogic.csWorkers/Resgrid.Workers.Framework/Resgrid.Workers.Framework.csproj
💤 Files with no reviewable changes (1)
- Web/Resgrid.Web/Models/AccountViewModels/VerifyCodeViewModel.cs
| private const int KeySize = 32; // 256-bit | ||
| private const int IvSize = 16; // 128-bit block | ||
| private const int Iterations = 10000; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OWASP PBKDF2 recommended iteration count 2024 2025
💡 Result:
OWASP’s current guidance (still the same through 2024–2025 on the OWASP Password Storage Cheat Sheet) is: (cheatsheetseries.owasp.org)
- PBKDF2-HMAC-SHA256: 600,000 iterations (OWASP also summarizes this as “600,000 or more” for FIPS-140 use cases) (cheatsheetseries.owasp.org)
- PBKDF2-HMAC-SHA512: 210,000 iterations (cheatsheetseries.owasp.org)
- PBKDF2-HMAC-SHA1: 1,300,000 iterations (legacy) (cheatsheetseries.owasp.org)
Citations:
- 1: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html
- 2: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html
- 3: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html
- 4: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html
🏁 Script executed:
# Verify the file exists and check the context
find . -type f -name "EncryptionService.cs" | head -5Repository: Resgrid/Core
Length of output: 100
🏁 Script executed:
cat -n ./Core/Resgrid.Services/EncryptionService.cs | head -50Repository: Resgrid/Core
Length of output: 2038
🏁 Script executed:
cat -n ./Core/Resgrid.Services/EncryptionService.cs | sed -n '50,120p'Repository: Resgrid/Core
Length of output: 3113
Update PBKDF2 iteration count to meet OWASP security recommendations.
The current iteration count of 10,000 is significantly below modern security standards. OWASP 2024-2025 guidance recommends a minimum of 600,000 iterations for PBKDF2-HMAC-SHA256 to resist brute-force attacks on derived keys.
💡 Suggested improvement
private const int KeySize = 32; // 256-bit
private const int IvSize = 16; // 128-bit block
-private const int Iterations = 10000;
+private const int Iterations = 600000; // OWASP recommendation for PBKDF2-SHA256📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private const int KeySize = 32; // 256-bit | |
| private const int IvSize = 16; // 128-bit block | |
| private const int Iterations = 10000; | |
| private const int KeySize = 32; // 256-bit | |
| private const int IvSize = 16; // 128-bit block | |
| private const int Iterations = 600000; // OWASP recommendation for PBKDF2-SHA256 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/EncryptionService.cs` around lines 15 - 17, The
Iterations constant in EncryptionService (currently Iterations = 10000) must be
increased to meet OWASP guidance: change Iterations to 600000 (or make it a
configurable setting read from config) and update any related comments to note
PBKDF2-HMAC-SHA256 and the new iteration count; ensure code paths that derive
keys using Iterations (the PBKDF2/derive-key calls) use this updated value and
run unit/integration tests that depend on key derivation to avoid breaking
compatibility or to handle migration if needed.
| .AddColumn("EmailVerificationCode").AsString(16).Nullable() | ||
| .AddColumn("MobileVerificationCode").AsString(16).Nullable() | ||
| .AddColumn("HomeVerificationCode").AsString(16).Nullable() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find verification code length configuration and generation logic
echo "=== Step 1: Verification code length config and generation ==="
rg -n --type cs "Verification.*(CodeLength|Length)|Generate.*Verification|EmailVerificationCode|MobileVerificationCode|HomeVerificationCode" | head -40
echo ""
echo "=== Step 2: Where verification code fields are assigned ==="
# 2) Find where verification code fields are assigned/persisted
rg -n --type cs "(EmailVerificationCode|MobileVerificationCode|HomeVerificationCode)\s*=" | head -40
echo ""
echo "=== Step 3: Hashing/encryption usage ==="
# 3) Check for hashing/encryption
rg -n --type cs "(hash|sha256|hmac|encrypt|IEncryptionService|Decrypt|PBKDF2)" | head -40
echo ""
echo "=== Step 4: SQL Server vs PostgreSQL column definitions ==="
# 4) Check migration files for column definitions
rg -n --type cs "AddColumn\(\"(Email|Mobile|Home)VerificationCode" | head -20Repository: Resgrid/Core
Length of output: 15969
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding code generation in ContactVerificationService ==="
# Get the full context of SendEmailVerificationCodeAsync method
sed -n '37,70p' Core/Resgrid.Services/ContactVerificationService.cs
echo ""
echo "=== Finding where 'code' variable is assigned ==="
rg -n "var code\s*=" Core/Resgrid.Services/ContactVerificationService.cs -A 2 -B 2
echo ""
echo "=== Checking for RandomCodeGenerator or similar ==="
rg -n --type cs "RandomCodeGenerator|GenerateCode|GenerateVerification|Random\.Next" | grep -i verif | head -20Repository: Resgrid/Core
Length of output: 1570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for GenerateCode method definition ==="
rg -n "GenerateCode" --type cs -B 2 -A 5
echo ""
echo "=== Full ContactVerificationService file ==="
wc -l Core/Resgrid.Services/ContactVerificationService.cs
echo ""
echo "=== Reading ContactVerificationService.cs ==="
cat -n Core/Resgrid.Services/ContactVerificationService.cs | head -100Repository: Resgrid/Core
Length of output: 10253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading GenerateCode method and VerificationConfig ==="
sed -n '278,310p' Core/Resgrid.Services/ContactVerificationService.cs
echo ""
echo "=== Finding VerificationConfig definition ==="
find . -name "*.cs" -type f | xargs grep -l "class VerificationConfig\|public.*VerificationCodeLength" | head -5
echo ""
echo "=== Checking Config.VerificationConfig settings ==="
rg -n "VerificationCodeLength|VerificationConfig" --type cs | grep -E "(VerificationCodeLength|public.*int)" | head -20Repository: Resgrid/Core
Length of output: 1667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading VerificationConfig.cs ==="
cat -n Core/Resgrid.Config/VerificationConfig.cs
echo ""
echo "=== Searching for default VerificationCodeLength value ==="
rg -n "VerificationCodeLength" --type cs -B 2 -A 2Repository: Resgrid/Core
Length of output: 2845
Plaintext storage of verification codes is a security concern—fix in the service layer, not the schema.
The length constraint is safe: codes are always 6 digits (from VerificationConfig.VerificationCodeLength = 6) and fit within the 16-char column. However, verification reveals codes are stored and retrieved as plaintext in ContactVerificationService.GenerateCode() without hashing or encryption. Update ContactVerificationService to hash or encrypt verification codes before persistence, and verify comparison logic against stored hashes during validation (around lines 195–228 where codes are compared).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0040_AddingContactVerification.cs`
around lines 14 - 16, The migration adds 16-char columns but the service stores
plaintext codes; update ContactVerificationService (not the schema) so
GenerateCode() hashes the generated 6-digit code before saving into
EmailVerificationCode/MobileVerificationCode/HomeVerificationCode and store only
the hash, and change the validation logic (the code comparison around
ContactVerificationService methods ~195–228) to verify the provided code against
the stored hash using a secure algorithm (e.g., bcrypt or HMAC-SHA256 with a
per-app salt) and a constant-time compare/verify routine; ensure the service
returns or sends the plain code only to the user (not persisted), and update any
save/load calls that reference the plain value to use the hashed field instead.
| -- Step 4: Drop whatever PK currently exists if it is not already the correct composite one | ||
| IF EXISTS ( | ||
| SELECT 1 | ||
| FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc | ||
| WHERE tc.TABLE_NAME = 'AspNetUserTokens' | ||
| AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY' | ||
| AND NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu | ||
| WHERE kcu.CONSTRAINT_NAME = tc.CONSTRAINT_NAME | ||
| AND kcu.TABLE_NAME = tc.TABLE_NAME | ||
| AND kcu.COLUMN_NAME = 'LoginProvider' | ||
| ) | ||
| ) | ||
| BEGIN | ||
| DECLARE @pkName NVARCHAR(256); | ||
| SELECT TOP 1 @pkName = CONSTRAINT_NAME | ||
| FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS | ||
| WHERE TABLE_NAME = 'AspNetUserTokens' | ||
| AND CONSTRAINT_TYPE = 'PRIMARY KEY'; | ||
|
|
||
| EXEC ('ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT [' + @pkName + ']'); | ||
|
|
||
| ALTER TABLE [dbo].[AspNetUserTokens] | ||
| ADD CONSTRAINT [PK_AspNetUserTokens] | ||
| PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); | ||
| END |
There was a problem hiding this comment.
PK correction logic misses valid failure states and can leave the table without the required PK.
Current logic only repairs when the existing PK lacks LoginProvider. If no PK exists, or if PK is still wrong but includes LoginProvider, this migration does nothing. Also, metadata queries should include schema to avoid cross-schema ambiguity.
🐛 Proposed fix (robust PK detection + schema-qualified metadata checks)
- -- Step 4: Drop whatever PK currently exists if it is not already the correct composite one
- IF EXISTS (
- SELECT 1
- FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc
- WHERE tc.TABLE_NAME = 'AspNetUserTokens'
- AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY'
- AND NOT EXISTS (
- SELECT 1
- FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu
- WHERE kcu.CONSTRAINT_NAME = tc.CONSTRAINT_NAME
- AND kcu.TABLE_NAME = tc.TABLE_NAME
- AND kcu.COLUMN_NAME = 'LoginProvider'
- )
- )
- BEGIN
- DECLARE `@pkName` NVARCHAR(256);
- SELECT TOP 1 `@pkName` = CONSTRAINT_NAME
- FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS
- WHERE TABLE_NAME = 'AspNetUserTokens'
- AND CONSTRAINT_TYPE = 'PRIMARY KEY';
-
- EXEC ('ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT [' + `@pkName` + ']');
-
- ALTER TABLE [dbo].[AspNetUserTokens]
- ADD CONSTRAINT [PK_AspNetUserTokens]
- PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]);
- END
+ -- Step 4: Ensure the PK is exactly (UserId, LoginProvider, Name)
+ DECLARE `@pkName` NVARCHAR(256);
+ SELECT TOP 1 `@pkName` = tc.CONSTRAINT_NAME
+ FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc
+ WHERE tc.TABLE_SCHEMA = 'dbo'
+ AND tc.TABLE_NAME = 'AspNetUserTokens'
+ AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY';
+
+ IF `@pkName` IS NULL
+ BEGIN
+ ALTER TABLE [dbo].[AspNetUserTokens]
+ ADD CONSTRAINT [PK_AspNetUserTokens]
+ PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]);
+ END
+ ELSE IF NOT EXISTS (
+ SELECT 1
+ FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu
+ WHERE kcu.TABLE_SCHEMA = 'dbo'
+ AND kcu.TABLE_NAME = 'AspNetUserTokens'
+ AND kcu.CONSTRAINT_NAME = `@pkName`
+ GROUP BY kcu.CONSTRAINT_NAME
+ HAVING COUNT(*) = 3
+ AND SUM(CASE WHEN kcu.COLUMN_NAME IN ('UserId','LoginProvider','Name') THEN 1 ELSE 0 END) = 3
+ )
+ BEGIN
+ EXEC (N'ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT ' + QUOTENAME(`@pkName`));
+ ALTER TABLE [dbo].[AspNetUserTokens]
+ ADD CONSTRAINT [PK_AspNetUserTokens]
+ PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]);
+ END📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- Step 4: Drop whatever PK currently exists if it is not already the correct composite one | |
| IF EXISTS ( | |
| SELECT 1 | |
| FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc | |
| WHERE tc.TABLE_NAME = 'AspNetUserTokens' | |
| AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY' | |
| AND NOT EXISTS ( | |
| SELECT 1 | |
| FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu | |
| WHERE kcu.CONSTRAINT_NAME = tc.CONSTRAINT_NAME | |
| AND kcu.TABLE_NAME = tc.TABLE_NAME | |
| AND kcu.COLUMN_NAME = 'LoginProvider' | |
| ) | |
| ) | |
| BEGIN | |
| DECLARE @pkName NVARCHAR(256); | |
| SELECT TOP 1 @pkName = CONSTRAINT_NAME | |
| FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS | |
| WHERE TABLE_NAME = 'AspNetUserTokens' | |
| AND CONSTRAINT_TYPE = 'PRIMARY KEY'; | |
| EXEC ('ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT [' + @pkName + ']'); | |
| ALTER TABLE [dbo].[AspNetUserTokens] | |
| ADD CONSTRAINT [PK_AspNetUserTokens] | |
| PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); | |
| END | |
| -- Step 4: Ensure the PK is exactly (UserId, LoginProvider, Name) | |
| DECLARE `@pkName` NVARCHAR(256); | |
| SELECT TOP 1 `@pkName` = tc.CONSTRAINT_NAME | |
| FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc | |
| WHERE tc.TABLE_SCHEMA = 'dbo' | |
| AND tc.TABLE_NAME = 'AspNetUserTokens' | |
| AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY'; | |
| IF `@pkName` IS NULL | |
| BEGIN | |
| ALTER TABLE [dbo].[AspNetUserTokens] | |
| ADD CONSTRAINT [PK_AspNetUserTokens] | |
| PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); | |
| END | |
| ELSE IF NOT EXISTS ( | |
| SELECT 1 | |
| FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu | |
| WHERE kcu.TABLE_SCHEMA = 'dbo' | |
| AND kcu.TABLE_NAME = 'AspNetUserTokens' | |
| AND kcu.CONSTRAINT_NAME = `@pkName` | |
| GROUP BY kcu.CONSTRAINT_NAME | |
| HAVING COUNT(*) = 3 | |
| AND SUM(CASE WHEN kcu.COLUMN_NAME IN ('UserId','LoginProvider','Name') THEN 1 ELSE 0 END) = 3 | |
| ) | |
| BEGIN | |
| EXEC (N'ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT ' + QUOTENAME(`@pkName`)); | |
| ALTER TABLE [dbo].[AspNetUserTokens] | |
| ADD CONSTRAINT [PK_AspNetUserTokens] | |
| PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); | |
| END |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0043_FixAspNetUserTokensPrimaryKey.cs`
around lines 62 - 88, The current migration only fixes the PK when the existing
PK lacks LoginProvider and also omits schema-qualified metadata checks, which
can leave AspNetUserTokens without a proper PK; update the logic in
M0043_FixAspNetUserTokensPrimaryKey to (1) use TABLE_SCHEMA in
INFORMATION_SCHEMA queries, (2) detect whether an existing PRIMARY KEY exists
and whether its ordered column set exactly equals the desired composite [UserId,
LoginProvider, Name] (e.g., by grouping KEY_COLUMN_USAGE by CONSTRAINT_NAME and
comparing column list/count), and (3) if no PK exists or the existing PK does
not exactly match that composite, drop the existing PK (if `@pkName` is set) and
then ADD CONSTRAINT [PK_AspNetUserTokens] PRIMARY KEY CLUSTERED
([UserId],[LoginProvider],[Name]); ensure the code safely handles the no-PK case
and always creates the correct PK.
| { | ||
| public class HttpApiExecutor : IWorkflowActionExecutor | ||
| { | ||
| public WorkflowActionType ActionType => WorkflowActionType.CallApiPost; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the WorkflowActionExecutorFactory
find . -type f -name "*.cs" | xargs grep -l "WorkflowActionExecutorFactory" | head -20Repository: Resgrid/Core
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Look at the HttpApiExecutor.cs file to understand its implementation
find . -type f -name "HttpApiExecutor.cs"Repository: Resgrid/Core
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Search for how executors are resolved and registered
rg -n "CallApiGet|CallApiPut|CallApiDelete|CallApiPost" --type=csRepository: Resgrid/Core
Length of output: 1652
🏁 Script executed:
#!/bin/bash
# Read the WorkflowActionExecutorFactory
cat -n ./Providers/Resgrid.Providers.Workflow/WorkflowActionExecutorFactory.csRepository: Resgrid/Core
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Read the HttpApiExecutor to see the full ExecuteAsync implementation
cat -n ./Providers/Resgrid.Providers.Workflow/Executors/HttpApiExecutor.csRepository: Resgrid/Core
Length of output: 6298
🏁 Script executed:
#!/bin/bash
# Check the interface
cat -n ./Core/Resgrid.Model/Providers/IWorkflowActionExecutorFactory.csRepository: Resgrid/Core
Length of output: 276
🏁 Script executed:
#!/bin/bash
# Check if there are other HTTP executors for the different action types
find ./Providers/Resgrid.Providers.Workflow/Executors -type f -name "*.cs" | xargs grep -l "CallApiGet\|CallApiPut\|CallApiDelete" | head -20Repository: Resgrid/Core
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Check the executor registration in the DI container
cat -n ./Providers/Resgrid.Providers.Workflow/WorkflowProviderModule.csRepository: Resgrid/Core
Length of output: 1963
Fix executor registration to support multiple HTTP action types.
The ActionType property returns only CallApiPost, but the factory resolves executors by creating a dictionary keyed on ActionType (WorkflowActionExecutorFactory line 15). This means requests for CallApiGet, CallApiPut, and CallApiDelete will fail at resolution time because no executor dictionary entry exists for those action types—even though HttpApiExecutor.ExecuteAsync contains logic to handle them (lines 64-66).
Either the ActionType property needs to return a value that represents all supported HTTP methods, or the executor registration must be updated to register the same HttpApiExecutor instance for all four HTTP action types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Providers/Resgrid.Providers.Workflow/Executors/HttpApiExecutor.cs` at line
15, The ActionType property on HttpApiExecutor currently returns only
WorkflowActionType.CallApiPost which prevents the factory
(WorkflowActionExecutorFactory) from resolving this executor for
CallApiGet/Put/Delete even though HttpApiExecutor.ExecuteAsync handles them;
update the registration so the factory maps all HTTP-related action types
(WorkflowActionType.CallApiGet, CallApiPost, CallApiPut, CallApiDelete) to the
same HttpApiExecutor instance, or alternatively change the
HttpApiExecutor.ActionType to a value/flag that represents all supported HTTP
methods — modify the factory registration (rather than ExecuteAsync) to register
HttpApiExecutor for each of the four action types to ensure resolution succeeds.
| var selectFunction = new Func<DbConnection, Task<IEnumerable<WorkflowCredential>>>(async x => | ||
| { | ||
| var dp = new DynamicParametersExtension(); | ||
| dp.Add("DepartmentId", departmentId); | ||
| var query = _queryFactory.GetQuery<SelectWorkflowCredentialsByDepartmentIdQuery>(); | ||
| return await x.QueryAsync<WorkflowCredential>(sql: query, param: dp, transaction: _unitOfWork.Transaction); | ||
| }); |
There was a problem hiding this comment.
Null-safe transaction access is required here.
Line 41 uses _unitOfWork.Transaction directly even though Line 45 explicitly handles _unitOfWork as nullable. This can throw a NullReferenceException.
Proposed fix
- return await x.QueryAsync<WorkflowCredential>(sql: query, param: dp, transaction: _unitOfWork.Transaction);
+ return await x.QueryAsync<WorkflowCredential>(sql: query, param: dp, transaction: _unitOfWork?.Transaction);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var selectFunction = new Func<DbConnection, Task<IEnumerable<WorkflowCredential>>>(async x => | |
| { | |
| var dp = new DynamicParametersExtension(); | |
| dp.Add("DepartmentId", departmentId); | |
| var query = _queryFactory.GetQuery<SelectWorkflowCredentialsByDepartmentIdQuery>(); | |
| return await x.QueryAsync<WorkflowCredential>(sql: query, param: dp, transaction: _unitOfWork.Transaction); | |
| }); | |
| var selectFunction = new Func<DbConnection, Task<IEnumerable<WorkflowCredential>>>(async x => | |
| { | |
| var dp = new DynamicParametersExtension(); | |
| dp.Add("DepartmentId", departmentId); | |
| var query = _queryFactory.GetQuery<SelectWorkflowCredentialsByDepartmentIdQuery>(); | |
| return await x.QueryAsync<WorkflowCredential>(sql: query, param: dp, transaction: _unitOfWork?.Transaction); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Repositories/Resgrid.Repositories.DataRepository/WorkflowCredentialRepository.cs`
around lines 36 - 42, The code in the selectFunction closure calls x.QueryAsync
with _unitOfWork.Transaction directly which can throw if _unitOfWork is null;
update the closure (selectFunction) to safely obtain the transaction (e.g., var
tx = _unitOfWork?.Transaction) and pass that tx into QueryAsync (allowing null)
so the call is null-safe; ensure you change the QueryAsync invocation in the
selectFunction to use the safe transaction variable instead of
_unitOfWork.Transaction.
| @if (!string.IsNullOrWhiteSpace(Model.Email) && Model.CanSelfVerify) | ||
| { | ||
| @if (Model.EmailVerified == false) | ||
| { | ||
| <div class="alert alert-warning m-t-xs" id="emailVerifyAlert"> | ||
| <strong><i class="fa fa-exclamation-triangle"></i> @localizer["EmailNotVerifiedWarning"]</strong> | ||
| <div class="m-t-xs" id="emailVerifyWidget"> | ||
| <button type="button" class="btn btn-xs btn-warning rg-send-code" data-contact-type="0" id="btnSendEmailCode">@localizer["VerifyButtonLabel"]</button> | ||
| <span id="emailCodeEntry" style="display:none;"> | ||
| <input type="text" class="form-control input-sm" id="emailVerifyCode" placeholder="@localizer["VerificationCodePlaceholder"]" maxlength="6" style="display:inline-block;width:140px;"> | ||
| <button type="button" class="btn btn-xs btn-primary rg-confirm-code" data-contact-type="0" id="btnConfirmEmailCode">@commonLocalizer["Save"]</button> | ||
| </span> | ||
| <span id="emailVerifyMsg" class="m-l-xs" style="display:none;"></span> | ||
| </div> | ||
| </div> | ||
| } | ||
| else if (Model.EmailVerified == null) | ||
| { | ||
| <div class="alert alert-info m-t-xs" id="emailVerifyAlert"> | ||
| <i class="fa fa-info-circle"></i> @localizer["GrandfatheredEmailWarning"] | ||
| <div class="m-t-xs" id="emailVerifyWidget"> | ||
| <button type="button" class="btn btn-xs btn-info rg-send-code" data-contact-type="0" id="btnSendEmailCode">@localizer["VerifyButtonLabel"]</button> | ||
| <span id="emailCodeEntry" style="display:none;"> | ||
| <input type="text" class="form-control input-sm" id="emailVerifyCode" placeholder="@localizer["VerificationCodePlaceholder"]" maxlength="6" style="display:inline-block;width:140px;"> | ||
| <button type="button" class="btn btn-xs btn-primary rg-confirm-code" data-contact-type="0" id="btnConfirmEmailCode">@commonLocalizer["Save"]</button> | ||
| </span> | ||
| <span id="emailVerifyMsg" class="m-l-xs" style="display:none;"></span> | ||
| </div> | ||
| </div> | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate HTML element IDs will break verification functionality.
The verification widgets for EmailVerified == false and EmailVerified == null cases both define elements with identical IDs (e.g., btnSendEmailCode, emailCodeEntry, emailVerifyCode, btnConfirmEmailCode, emailVerifyMsg). HTML IDs must be unique within a document. When JavaScript queries these IDs, it will only find the first matching element, causing the verification flow to fail for one of the states.
The same issue exists for Home (Lines 198-225) and Mobile (Lines 258-285) verification widgets.
🐛 Proposed fix: Use unique IDs per verification state
Differentiate IDs by appending the state context, for example:
- <button type="button" class="btn btn-xs btn-warning rg-send-code" data-contact-type="0" id="btnSendEmailCode">@localizer["VerifyButtonLabel"]</button>
- <span id="emailCodeEntry" style="display:none;">
- <input type="text" class="form-control input-sm" id="emailVerifyCode" placeholder="@localizer["VerificationCodePlaceholder"]" maxlength="6" style="display:inline-block;width:140px;">
- <button type="button" class="btn btn-xs btn-primary rg-confirm-code" data-contact-type="0" id="btnConfirmEmailCode">@commonLocalizer["Save"]</button>
+ <button type="button" class="btn btn-xs btn-warning rg-send-code" data-contact-type="0" id="btnSendEmailCode_pending">@localizer["VerifyButtonLabel"]</button>
+ <span id="emailCodeEntry_pending" style="display:none;">
+ <input type="text" class="form-control input-sm" id="emailVerifyCode_pending" placeholder="@localizer["VerificationCodePlaceholder"]" maxlength="6" style="display:inline-block;width:140px;">
+ <button type="button" class="btn btn-xs btn-primary rg-confirm-code" data-contact-type="0" id="btnConfirmEmailCode_pending">@commonLocalizer["Save"]</button>Alternatively, since only one block renders at a time (mutually exclusive conditions), you could keep the same IDs but ensure the JavaScript uses class-based selectors or closest-parent scoping rather than direct ID lookups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Home/EditUserProfile.cshtml` around lines
121 - 151, The two mutually-exclusive verification blocks render duplicate
element IDs (btnSendEmailCode, emailCodeEntry, emailVerifyCode,
btnConfirmEmailCode, emailVerifyMsg) which breaks JS lookups; update the
EditUserProfile view so IDs are unique per state (e.g., append a suffix like
_unverified vs _grandfathered) or remove IDs and instead use class-based
selectors/scoped queries in the verification JS; change the IDs in the blocks
that check Model.EmailVerified (false and null) and update any related
client-side code that references those IDs to the new identifiers or to
class/scoped selectors.
| <ol class="breadcrumb"> | ||
| <li> | ||
| <a asp-controller="Home" asp-action="Dashboard" asp-route-area="User">Home</a> | ||
| <a asp-controller="Home" asp-action="Dashboard" asp-route-area="User">@commonLocalizer["HomeModule"]</a> |
There was a problem hiding this comment.
Missing commonLocalizer injection.
The view uses @commonLocalizer["HomeModule"] but commonLocalizer is not injected. This will cause a runtime error.
🐛 Proposed fix: Add the missing injection
`@inject` IStringLocalizer<Resgrid.Localization.Areas.User.Security.Security> localizer
`@inject` IStringLocalizer<Resgrid.Localization.Areas.User.TwoFactor.TwoFactor> twoFactorLocalizer
+@inject IStringLocalizer<Resgrid.Localization.Shared.Common> commonLocalizer📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a asp-controller="Home" asp-action="Dashboard" asp-route-area="User">@commonLocalizer["HomeModule"]</a> | |
| `@inject` IStringLocalizer<Resgrid.Localization.Areas.User.Security.Security> localizer | |
| `@inject` IStringLocalizer<Resgrid.Localization.Areas.User.TwoFactor.TwoFactor> twoFactorLocalizer | |
| `@inject` IStringLocalizer<Resgrid.Localization.Shared.Common> commonLocalizer |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Security/Index.cshtml` at line 15, The view
references commonLocalizer but never injects it; add an injection for the
appropriate localizer (e.g., IViewLocalizer or IStringLocalizer) at the top of
the Index.cshtml view so `@commonLocalizer`["HomeModule"] resolves, e.g., add an
`@inject` statement for commonLocalizer (ensure the injected type matches other
views in the project such as IViewLocalizer commonLocalizer or
IStringLocalizer<SharedResource> commonLocalizer and include the necessary
using/namespace if required).
| <a asp-action="RegenerateRecoveryCodes" class="btn btn-warning">@localizer["RegenerateRecoveryCodesButton"]</a> | ||
| <a asp-action="Disable2FA" class="btn btn-danger">@localizer["Disable2FAButton"]</a> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find TwoFactorController and inspect action method decorators
fd 'TwoFactorController.cs' -t f | head -5Repository: Resgrid/Core
Length of output: 117
🏁 Script executed:
# Once found, inspect the full controller for these specific actions
find . -name '*TwoFactorController.cs' -type f | xargs catRepository: Resgrid/Core
Length of output: 11083
🏁 Script executed:
# Read the view file to see lines 49-50 and 58
sed -n '45,65p' 'Web/Resgrid.Web/Areas/User/Views/TwoFactor/Index.cshtml'Repository: Resgrid/Core
Length of output: 1001
Refactor RegenerateRecoveryCodes to use POST instead of GET for state mutation.
RegenerateRecoveryCodes (line 49) is a [HttpGet] endpoint that calls GenerateNewTwoFactorRecoveryCodesAsync() without [ValidateAntiForgeryToken], making it vulnerable to CSRF attacks via simple link clicks. Convert it to [HttpPost] with antiforgery validation.
Disable2FA (line 50) and Enable2FA (line 58) are correctly designed: the links access GET endpoints that only display forms, while the actual state changes occur via POST endpoints protected with [ValidateAntiForgeryToken].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/TwoFactor/Index.cshtml` around lines 49 -
50, RegenerateRecoveryCodes is currently a GET link that calls
GenerateNewTwoFactorRecoveryCodesAsync, which mutates state and is vulnerable to
CSRF; change the controller action RegenerateRecoveryCodes to be [HttpPost] and
add [ValidateAntiForgeryToken], and update the view to replace the anchor with a
form that posts to asp-action="RegenerateRecoveryCodes" (method="post")
including the antiforgery token and a submit button styled like the existing btn
btn-warning so Disable2FA and Enable2FA remain unchanged.
| @model Resgrid.Model.WorkflowHealthSummary | ||
| @inject IStringLocalizer<Resgrid.Localization.Areas.User.Workflows.Workflows> localizer | ||
| @{ |
There was a problem hiding this comment.
commonLocalizer is not injected but is used on line 13.
Same issue as New.cshtml — the view uses commonLocalizer["HomeModule"] in the breadcrumb without injecting it.
🐛 Proposed fix to add missing injection
`@model` Resgrid.Model.WorkflowHealthSummary
`@inject` IStringLocalizer<Resgrid.Localization.Areas.User.Workflows.Workflows> localizer
+@inject IStringLocalizer<Resgrid.Localization.Shared.Common> commonLocalizer
@{Also applies to: 13-13
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Workflows/Health.cshtml` around lines 1 - 3,
The view references commonLocalizer (used for commonLocalizer["HomeModule"] in
the breadcrumb) but never injects it; add an injection for commonLocalizer
(e.g., inject an IStringLocalizer for the shared/common resource) alongside the
existing localizer injection so the view can resolve commonLocalizer; place the
new `@inject` IStringLocalizer<...> commonLocalizer line near the existing `@inject`
IStringLocalizer<Resgrid.Localization.Areas.User.Workflows.Workflows> localizer
declaration and ensure the generic type matches your shared localization
resource (the symbol to add is commonLocalizer).
| @model Resgrid.Model.Workflow | ||
| @inject IStringLocalizer<Resgrid.Localization.Areas.User.Workflows.Workflows> localizer | ||
| @{ |
There was a problem hiding this comment.
commonLocalizer is not injected but is used on line 19.
The view injects only localizer but references commonLocalizer["HomeModule"] in the breadcrumb. This will cause a compile-time error (if strongly typed) or a runtime NullReferenceException.
🐛 Proposed fix to add missing injection
`@model` Resgrid.Model.Workflow
`@inject` IStringLocalizer<Resgrid.Localization.Areas.User.Workflows.Workflows> localizer
+@inject IStringLocalizer<Resgrid.Localization.Shared.Common> commonLocalizer
@{Note: Verify the correct namespace/type for the common localizer used elsewhere in the project.
Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Workflows/New.cshtml` around lines 1 - 3,
The view uses commonLocalizer (e.g., commonLocalizer["HomeModule"]) but only
injects localizer; add an injection for the common localizer at the top of the
Razor view (similar to the existing `@inject` IStringLocalizer<...> localizer) so
commonLocalizer is initialized; ensure the injected type/namespace matches other
views that use commonLocalizer and name it exactly commonLocalizer to match the
usage in the breadcrumb.
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs (1)
56-57:⚠️ Potential issue | 🟠 Major
Unauthorized()result is not returned.The
Unauthorized()call is made but its result is not returned, so the method continues execution even for non-admin users. This pattern appears elsewhere in this file (lines 387-388, 421-422, 429-430) and should be fixed.🐛 Proposed fix
if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) - Unauthorized(); + return Unauthorized();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs` around lines 56 - 57, The code calls Unauthorized() without returning it, so execution continues for unauthorized users; update each conditional that checks ClaimsAuthorizationHelper.IsUserDepartmentAdmin() to return the Unauthorized() IActionResult (e.g., change "if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) Unauthorized();" to "if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) return Unauthorized();") for the occurrences in SecurityController (the example line and the other occurrences near the checks at the later blocks).
♻️ Duplicate comments (7)
Providers/Resgrid.Providers.Migrations/Migrations/M0043_FixAspNetUserTokensPrimaryKey.cs (1)
62-88:⚠️ Potential issue | 🔴 CriticalPK repair condition is incomplete and can leave an invalid/missing PK.
Line 63 logic only repairs one failure shape (missing
LoginProvider). It skips no-PK cases and can skip other invalid PK definitions. Metadata checks also aren’t schema-qualified, which can mis-resolve constraints in multi-schema DBs.Robust PK check/rebuild pattern
- -- Step 4: Drop whatever PK currently exists if it is not already the correct composite one - IF EXISTS ( - SELECT 1 - FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc - WHERE tc.TABLE_NAME = 'AspNetUserTokens' - AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY' - AND NOT EXISTS ( - SELECT 1 - FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu - WHERE kcu.CONSTRAINT_NAME = tc.CONSTRAINT_NAME - AND kcu.TABLE_NAME = tc.TABLE_NAME - AND kcu.COLUMN_NAME = 'LoginProvider' - ) - ) - BEGIN - DECLARE `@pkName` NVARCHAR(256); - SELECT TOP 1 `@pkName` = CONSTRAINT_NAME - FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS - WHERE TABLE_NAME = 'AspNetUserTokens' - AND CONSTRAINT_TYPE = 'PRIMARY KEY'; - - EXEC ('ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT [' + `@pkName` + ']'); - - ALTER TABLE [dbo].[AspNetUserTokens] - ADD CONSTRAINT [PK_AspNetUserTokens] - PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); - END + -- Step 4: Ensure PK is exactly (UserId, LoginProvider, Name) + DECLARE `@pkName` NVARCHAR(256); + SELECT TOP 1 `@pkName` = tc.CONSTRAINT_NAME + FROM INFORMATION_SCHEMA.TABLE_CONSTRAINTS tc + WHERE tc.TABLE_SCHEMA = 'dbo' + AND tc.TABLE_NAME = 'AspNetUserTokens' + AND tc.CONSTRAINT_TYPE = 'PRIMARY KEY'; + + IF `@pkName` IS NULL + BEGIN + ALTER TABLE [dbo].[AspNetUserTokens] + ADD CONSTRAINT [PK_AspNetUserTokens] + PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); + END + ELSE IF NOT EXISTS ( + SELECT 1 + FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE kcu + WHERE kcu.TABLE_SCHEMA = 'dbo' + AND kcu.TABLE_NAME = 'AspNetUserTokens' + AND kcu.CONSTRAINT_NAME = `@pkName` + GROUP BY kcu.CONSTRAINT_NAME + HAVING COUNT(*) = 3 + AND SUM(CASE WHEN kcu.COLUMN_NAME IN ('UserId','LoginProvider','Name') THEN 1 ELSE 0 END) = 3 + ) + BEGIN + EXEC (N'ALTER TABLE [dbo].[AspNetUserTokens] DROP CONSTRAINT ' + QUOTENAME(`@pkName`)); + ALTER TABLE [dbo].[AspNetUserTokens] + ADD CONSTRAINT [PK_AspNetUserTokens] + PRIMARY KEY CLUSTERED ([UserId], [LoginProvider], [Name]); + END🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0043_FixAspNetUserTokensPrimaryKey.cs` around lines 62 - 88, The current PK-repair only checks for a missing LoginProvider column and can miss cases where the primary key is absent or has the wrong columns; update the conditional that examines INFORMATION_SCHEMA.TABLE_CONSTRAINTS and INFORMATION_SCHEMA.KEY_COLUMN_USAGE to be schema-qualified (check TABLE_SCHEMA = 'dbo') and to explicitly detect either no PRIMARY KEY or a primary key whose ordered set of columns does not exactly match [UserId],[LoginProvider],[Name]; if a mismatched or any PK exists capture its name into `@pkName` (from INFORMATION_SCHEMA.TABLE_CONSTRAINTS where TABLE_NAME='AspNetUserTokens' and TABLE_SCHEMA='dbo' and CONSTRAINT_TYPE='PRIMARY KEY'), DROP that constraint via ALTER TABLE ... DROP CONSTRAINT [`@pkName`] (only if `@pkName` is not null), then always ADD CONSTRAINT [PK_AspNetUserTokens] PRIMARY KEY CLUSTERED ([UserId],[LoginProvider],[Name]) to ensure the correct composite key on AspNetUserTokens.Web/Resgrid.Web/Areas/User/Views/Security/Index.cshtml (1)
15-15:⚠️ Potential issue | 🔴 CriticalMissing
commonLocalizerinjection causes runtime error.The view uses
@commonLocalizer["HomeModule"]butcommonLocalizeris not injected at the top of the file. This will cause aNullReferenceExceptionor compilation error at runtime.🐛 Proposed fix: Add the missing injection
`@inject` IStringLocalizer<Resgrid.Localization.Areas.User.Security.Security> localizer `@inject` IStringLocalizer<Resgrid.Localization.Areas.User.TwoFactor.TwoFactor> twoFactorLocalizer +@inject IStringLocalizer<Resgrid.Localization.Shared.Common> commonLocalizer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Security/Index.cshtml` at line 15, The view references commonLocalizer but it is not injected, causing runtime errors; add an `@inject` for the appropriate IStringLocalizer type (the same resource type used elsewhere, e.g., SharedResource or the view's resource) at the top of the Index.cshtml so the commonLocalizer symbol is initialized before use; ensure the injected name matches commonLocalizer exactly so the existing usages like `@commonLocalizer`["HomeModule"] resolve.Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs (1)
371-374:⚠️ Potential issue | 🟠 MajorMissing
[ValidateAntiForgeryToken]on Cancel POST action.The
Cancelaction is a state-changing POST that cancels subscriptions, but it lacks CSRF protection. While[RequiresRecentTwoFactor]adds authentication verification, it doesn't protect against CSRF attacks.Suggested fix
[HttpPost] [Authorize(Policy = ResgridResources.Department_Update)] +[ValidateAntiForgeryToken] [RequiresRecentTwoFactor] public async Task<IActionResult> Cancel(CancelView model, CancellationToken cancellationToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs` around lines 371 - 374, The POST Cancel action in SubscriptionController (method Cancel) is missing CSRF protection; add the [ValidateAntiForgeryToken] attribute to the Cancel(CancelView model, CancellationToken cancellationToken) action method to enforce antiforgery token validation, and ensure the corresponding view/form that posts to this action includes the antiforgery token (e.g., `@Html.AntiForgeryToken` or equivalent) so the attribute can validate requests.Web/Resgrid.Web/Areas/User/Views/TwoFactor/Index.cshtml (1)
41-50:⚠️ Potential issue | 🔴 CriticalUse POST + antiforgery for recovery-code regeneration action.
RegenerateRecoveryCodesis still invoked via GET links on Lines 41 and 49. If that endpoint regenerates codes (state mutation), this remains CSRF-prone and should be a POST form with antiforgery.Run this to confirm the action attributes and update path before changing the view:
#!/bin/bash set -euo pipefail controller_files=$(fd 'TwoFactorController.cs' -t f) echo "Controller file(s):" printf '%s\n' "$controller_files" echo echo "Inspecting action and HTTP verb attributes:" rg -n -C4 'RegenerateRecoveryCodes|\[HttpGet\]|\[HttpPost\]|ValidateAntiForgeryToken' $controller_filesExpected result:
RegenerateRecoveryCodesshould be[HttpPost]+[ValidateAntiForgeryToken], and the view should post via<form method="post">.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/TwoFactor/Index.cshtml` around lines 41 - 50, Replace the GET links for RegenerateRecoveryCodes in the TwoFactor view with a POST form that submits to the RegenerateRecoveryCodes action and includes the antiforgery token, and ensure the controller action RegenerateRecoveryCodes on TwoFactorController is decorated with [HttpPost] and [ValidateAntiForgeryToken]; specifically, change the anchor elements invoking RegenerateRecoveryCodes into a <form method="post"> that contains an input/button for submission and `@Html.AntiForgeryToken`(), and update or verify the TwoFactorController.RegenerateRecoveryCodes method attributes to [HttpPost] and [ValidateAntiForgeryToken].Web/Resgrid.Web/Areas/User/Models/EditProfileModel.cs (1)
115-116:⚠️ Potential issue | 🟠 Major
CanSelfVerifystill missesSelf-based own-profile flows.At Line 116, gating only on
IsOwnProfilecan incorrectly block self-verification when onlySelfis set.🔧 Proposed fix
- public bool CanSelfVerify => IsOwnProfile; + public bool CanSelfVerify => Self || IsOwnProfile;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Models/EditProfileModel.cs` around lines 115 - 116, The CanSelfVerify getter currently only checks IsOwnProfile and thus blocks self-verification when the model represents a "Self" flow; update the CanSelfVerify expression to allow either IsOwnProfile or the Self flag (e.g., return IsOwnProfile || Self) so self-based own-profile flows are permitted; locate the CanSelfVerify property in EditProfileModel and modify its boolean expression accordingly.Core/Resgrid.Services/EncryptionService.cs (1)
15-17:⚠️ Potential issue | 🟠 MajorPBKDF2 iteration count of 10,000 is below OWASP recommendations.
The current iteration count of 10,000 is significantly below the OWASP-recommended 600,000 iterations for PBKDF2-HMAC-SHA256. This was previously flagged and should be addressed before production use.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/EncryptionService.cs` around lines 15 - 17, The Iterations constant in EncryptionService (Iterations) is set to 10000 which is below OWASP guidance; update the PBKDF2 iteration count used by methods in EncryptionService (referenced by Iterations) to at least 600000 or make it configurable via app settings so production can use the OWASP-recommended 600,000+ iterations, and ensure any callers of the PBKDF2 derivation pick up the new constant/config value; keep KeySize and IvSize as-is unless you have other crypto requirements.Web/Resgrid.Web.Services/Controllers/v4/WorkflowsController.cs (1)
169-179:⚠️ Potential issue | 🔴 CriticalAuthorization bypass:
DeleteStepdoes not verify department ownership.Unlike
Delete(lines 119-120) andDeleteCredential(lines 239-240), this endpoint doesn't verify that the step's parent workflow belongs to the current department before deletion. An authenticated user could delete workflow steps from other departments by enumerating step IDs.🔒 Proposed fix to add ownership verification
[HttpDelete("DeleteStep/{stepId}")] [ProducesResponseType(StatusCodes.Status200OK)] +[ProducesResponseType(StatusCodes.Status404NotFound)] [Authorize(Policy = ResgridResources.Workflow_Delete)] public async Task<ActionResult<DeleteWorkflowResult>> DeleteStep(string stepId, CancellationToken ct) { + // Verify step exists and belongs to a workflow owned by this department + var steps = await _workflowService.GetStepsByWorkflowIdAsync(stepId, ct); + // Note: Need a GetStepByIdAsync method - workaround below uses existing service + var allDeptWorkflows = await _workflowService.GetWorkflowsByDepartmentIdAsync(DepartmentId, ct); + var allDeptStepIds = new HashSet<string>(); + foreach (var wf in allDeptWorkflows) + { + var wfSteps = await _workflowService.GetStepsByWorkflowIdAsync(wf.WorkflowId, ct); + foreach (var s in wfSteps) allDeptStepIds.Add(s.WorkflowStepId); + } + if (!allDeptStepIds.Contains(stepId)) return NotFound(); + if (!await CanUserManageWorkflowsAsync()) return Forbid(); await _workflowService.DeleteWorkflowStepAsync(stepId, ct); return Ok(new DeleteWorkflowResult { Success = true }); }A cleaner solution would be to add
GetStepByIdAsynctoIWorkflowService, then fetch the step, retrieve its parent workflow, and verifyworkflow.DepartmentId == DepartmentId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WorkflowsController.cs` around lines 169 - 179, DeleteStep currently calls DeleteWorkflowStepAsync without verifying the step's workflow belongs to the current department; add ownership verification by first retrieving the step and its parent workflow and comparing department IDs before deletion. Add/Get a method on IWorkflowService like GetStepByIdAsync(stepId) (and if needed GetWorkflowByIdAsync(workflowId)), call GetStepByIdAsync in DeleteStep, use step.WorkflowId to load the workflow, verify workflow.DepartmentId == DepartmentId (or otherwise match the controller's current department identifier), return Forbid() if they differ, and only then call DeleteWorkflowStepAsync(stepId, ct).
🟡 Minor comments (27)
Tests/Resgrid.Tests/Resgrid.Tests.csproj-35-35 (1)
35-35:⚠️ Potential issue | 🟡 MinorConsider updating Scriban to the latest stable version.
Scriban 5.12.1 is compatible with .NET 9.0 via its net7.0 target, but version 6.5.3 is now available and should be preferred for latest features and improvements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Resgrid.Tests.csproj` at line 35, Update the Scriban PackageReference from version 5.12.1 to the latest stable (6.5.3) in the project file where the PackageReference Include="Scriban" entry exists; change the Version attribute to "6.5.3", then run dotnet restore/build to verify compatibility with the project's target frameworks and address any API changes that may require small code adjustments in affected usages of the Scriban library.Repositories/Resgrid.Repositories.DataRepository/Stores/IdentityUserStore.cs-495-499 (1)
495-499:⚠️ Potential issue | 🟡 MinorMissing input validation and cancellation check.
ReplaceCodesAsynclacks thecancellationToken.ThrowIfCancellationRequested()andusernull check present in other methods likeRedeemCodeAsyncandCountCodesAsync.Proposed fix
public async Task ReplaceCodesAsync(IdentityUser user, IEnumerable<string> recoveryCodes, CancellationToken cancellationToken) { + cancellationToken.ThrowIfCancellationRequested(); + if (user == null) throw new ArgumentNullException(nameof(user)); + var mergedCodes = string.Join(";", recoveryCodes); await SetTokenAsync(user, AuthenticatorKeyLoginProvider, RecoveryCodeTokenName, mergedCodes, cancellationToken); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/Stores/IdentityUserStore.cs` around lines 495 - 499, Add the same input validation and cancellation handling to ReplaceCodesAsync as in RedeemCodeAsync/CountCodesAsync: at the start of ReplaceCodesAsync call cancellationToken.ThrowIfCancellationRequested(), check that the user parameter is not null and throw ArgumentNullException if it is, and validate recoveryCodes (e.g., treat null as empty or throw) before joining; then proceed to call SetTokenAsync(AuthenticatorKeyLoginProvider, RecoveryCodeTokenName, mergedCodes, cancellationToken). Ensure you reference ReplaceCodesAsync, RedeemCodeAsync, CountCodesAsync, AuthenticatorKeyLoginProvider, RecoveryCodeTokenName and SetTokenAsync when making the changes.Repositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.cs-1146-1182 (1)
1146-1182:⚠️ Potential issue | 🟡 MinorTransaction handling inconsistent with
SetTokenAsync.
SetTokenAsyncexplicitly bypasses the ambient_unitOfWork.Transaction(lines 1098-1101) because it "may have already been committed earlier in the same request scope." However,RemoveTokenAsyncstill uses_unitOfWork.Transactionat line 1159, which could suffer the same stale-transaction issue.Consider applying the same dedicated-connection pattern here for consistency, or document why the risk is acceptable for deletes but not upserts.
Proposed fix for consistency
public async Task RemoveTokenAsync(string userId, string loginProvider, string name, CancellationToken cancellationToken) { try { - var deleteFunction = new Func<DbConnection, Task>(async x => - { - string deleteSql = Config.DataConfig.DatabaseType == Config.DatabaseTypes.Postgres - ? "DELETE FROM aspnetusertokens WHERE userid = `@UserId` AND loginprovider = `@LoginProvider` AND name = `@Name`" - : "DELETE FROM AspNetUserTokens WHERE UserId = `@UserId` AND LoginProvider = `@LoginProvider` AND Name = `@Name`"; - - await x.ExecuteAsync( - deleteSql, - new { UserId = userId, LoginProvider = loginProvider, Name = name }, - _unitOfWork.Transaction); - }); - - DbConnection conn = null; - if (_unitOfWork?.Connection == null) - { - using (conn = _connectionProvider.Create()) - { - await conn.OpenAsync(cancellationToken); - await deleteFunction(conn); - } - } - else - { - conn = _unitOfWork.CreateOrGetConnection(); - await deleteFunction(conn); - } + string deleteSql = Config.DataConfig.DatabaseType == Config.DatabaseTypes.Postgres + ? "DELETE FROM aspnetusertokens WHERE userid = `@UserId` AND loginprovider = `@LoginProvider` AND name = `@Name`" + : "DELETE FROM AspNetUserTokens WHERE UserId = `@UserId` AND LoginProvider = `@LoginProvider` AND Name = `@Name`"; + + using var conn = _connectionProvider.Create(); + await conn.OpenAsync(cancellationToken); + await conn.ExecuteAsync( + deleteSql, + new { UserId = userId, LoginProvider = loginProvider, Name = name }); } catch (Exception ex) { Logging.LogException(ex); throw; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/IdentityUserRepository.cs` around lines 1146 - 1182, RemoveTokenAsync still passes _unitOfWork.Transaction into ExecuteAsync which can use a stale/committed transaction like SetTokenAsync avoided; update RemoveTokenAsync to mirror SetTokenAsync's dedicated-connection pattern by ensuring the deleteFunction and its ExecuteAsync call do NOT use _unitOfWork.Transaction when creating a fresh connection from _connectionProvider.Create (and only use CreateOrGetConnection when participating in the current unit-of-work but without passing the possibly-committed _unitOfWork.Transaction), i.e., change how deleteFunction is invoked so it uses a standalone connection/command execution like SetTokenAsync to avoid the stale transaction issue.Web/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtml-29-29 (1)
29-29:⚠️ Potential issue | 🟡 MinorLocalize the new menu label.
Line 29 introduces a hardcoded
"Workflows"string while adjacent entries are localized. This will miss translations in non-English UI.Suggested fix
-<li class="@Html.IsSelected(controller: "Workflows", action: "Index")"><a asp-controller="Workflows" asp-action="Index" asp-route-area="User">Workflows</a></li> +<li class="@Html.IsSelected(controller: "Workflows", action: "Index")"><a asp-controller="Workflows" asp-action="Index" asp-route-area="User">@commonLocalizer["Workflows"]</a></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Shared/_TopNavbar.cshtml` at line 29, The menu label for the Workflows entry is hardcoded; replace the visible "Workflows" string in the list item that uses Html.IsSelected(controller: "Workflows", action: "Index") and the anchor asp-controller="Workflows" asp-action="Index" with the localized resource lookup used by adjacent entries (e.g., the same IViewLocalizer/Html helper pattern used elsewhere). Ensure you add the "Workflows" key to the appropriate resource file and use the same localization call used by neighboring menu items so the label is translated in non-English UI.Core/Resgrid.Services/AuditService.cs-66-92 (1)
66-92:⚠️ Potential issue | 🟡 MinorMissing switch cases for TwoFactor audit types.
The
AuditLogTypesenum includesTwoFactorEnabled,TwoFactorDisabled,TwoFactorLoginVerified,TwoFactorRecoveryCodeUsed, andTwoFactorStepUpVerified(lines 81-85 inAuditLogTypes.cs), but these cases are not handled inGetAuditLogTypeString. This will cause them to return an empty string.Proposed fix to add missing TwoFactor mappings
case AuditLogTypes.ContactVerificationFailed: return "Contact Verification Failed"; + case AuditLogTypes.TwoFactorEnabled: + return "Two-Factor Authentication Enabled"; + case AuditLogTypes.TwoFactorDisabled: + return "Two-Factor Authentication Disabled"; + case AuditLogTypes.TwoFactorLoginVerified: + return "Two-Factor Login Verified"; + case AuditLogTypes.TwoFactorRecoveryCodeUsed: + return "Two-Factor Recovery Code Used"; + case AuditLogTypes.TwoFactorStepUpVerified: + return "Two-Factor Step-Up Verified"; } return "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/AuditService.cs` around lines 66 - 92, GetAuditLogTypeString in AuditService.cs is missing handlers for the TwoFactor-related AuditLogTypes, causing those enum values (TwoFactorEnabled, TwoFactorDisabled, TwoFactorLoginVerified, TwoFactorRecoveryCodeUsed, TwoFactorStepUpVerified) to return an empty string; update the switch in GetAuditLogTypeString to add cases for AuditLogTypes.TwoFactorEnabled, TwoFactorDisabled, TwoFactorLoginVerified, TwoFactorRecoveryCodeUsed, and TwoFactorStepUpVerified and return clear descriptive messages (e.g., "Two-Factor Enabled", "Two-Factor Disabled", "Two-Factor Login Verified", "Two-Factor Recovery Code Used", "Two-Factor Step-Up Verified") so each enum value is mapped to a non-empty string.Core/Resgrid.Localization/Areas/User/TwoFactor/TwoFactor.es.resx-50-57 (1)
50-57:⚠️ Potential issue | 🟡 MinorMissing localization keys in Spanish translation.
Comparing with the English resource file, the Spanish version appears to be missing:
RecoveryCodePlaceholder(English line 71:xxxx-xxxx-xxxx)AuthenticatorCodeInputLabel(English line 62)These keys are present in
TwoFactor.en.resxbut not in this file, which could cause fallback to English or runtime issues depending on how the localization framework handles missing keys.🌐 Suggested additions
<data name="RecoveryCodeInputLabel" xml:space="preserve"><value>Código de Recuperación</value></data> + <data name="RecoveryCodePlaceholder" xml:space="preserve"><value>xxxx-xxxx-xxxx</value></data> <data name="LoginWithRecoveryCodeButton" xml:space="preserve"><value>Iniciar Sesión con Código de Recuperación</value></data>And add somewhere in the login 2FA section:
+ <data name="AuthenticatorCodeInputLabel" xml:space="preserve"><value>Código Autenticador</value></data>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Localization/Areas/User/TwoFactor/TwoFactor.es.resx` around lines 50 - 57, Add the missing Spanish localization keys by adding entries for RecoveryCodePlaceholder and AuthenticatorCodeInputLabel to the TwoFactor Spanish resource: include RecoveryCodePlaceholder with a Spanish-appropriate placeholder value (e.g., "xxxx-xxxx-xxxx") and AuthenticatorCodeInputLabel with a Spanish label for the authenticator code (e.g., "Código de la aplicación autenticadora" or "Código del autenticador") so the resource contains RecoveryCodePlaceholder and AuthenticatorCodeInputLabel matching the keys used in TwoFactor.en.resx.Web/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtml-23-23 (1)
23-23:⚠️ Potential issue | 🟡 MinorUse localized string for Two-Factor Authentication label.
The navigation item uses a hard-coded English string
"Two-Factor Authentication"while other menu items use localization via@commonLocalizer["..."]. This breaks l10n consistency.🌐 Proposed fix to use localization
- <li><a asp-controller="TwoFactor" asp-action="Index" asp-route-area="User"><i class="fa fa-shield"></i> Two-Factor Authentication</a></li> + <li><a asp-controller="TwoFactor" asp-action="Index" asp-route-area="User"><i class="fa fa-shield"></i> `@commonLocalizer`["TwoFactorAuthentication"]</a></li>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Shared/_Navigation.cshtml` at line 23, Replace the hard-coded label "Two-Factor Authentication" in the navigation anchor (asp-controller="TwoFactor", asp-action="Index") with the localized string used elsewhere, e.g. use the commonLocalizer resource key for the two-factor label (commonLocalizer["Two-Factor Authentication"] or the appropriate key if different) so the menu item matches the other localized items.Web/Resgrid.Web/Areas/User/Views/TwoFactor/ShowRecoveryCodes.cshtml-32-37 (1)
32-37:⚠️ Potential issue | 🟡 MinorAdd null check before iterating
Model.RecoveryCodes.If
Model.RecoveryCodesis null, theforeachwill throw aNullReferenceException. Consider adding a null-coalescing fallback:Proposed fix
- `@foreach` (var code in Model.RecoveryCodes) + `@foreach` (var code in Model.RecoveryCodes ?? Enumerable.Empty<string>())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/TwoFactor/ShowRecoveryCodes.cshtml` around lines 32 - 37, The foreach over Model.RecoveryCodes can throw when RecoveryCodes is null; update the view (ShowRecoveryCodes.cshtml) to guard the iteration by checking Model.RecoveryCodes for null or by iterating over a safe fallback (e.g. use a null-coalescing empty sequence for Model.RecoveryCodes) before the foreach so the loop only runs when there are codes to render; locate the foreach block referencing Model.RecoveryCodes and replace it with a null-safe iteration or an if (Model.RecoveryCodes != null) guard.Core/Resgrid.Services/SmsService.cs-350-377 (1)
350-377:⚠️ Potential issue | 🟡 MinorMissing validation for
verificationCodeparameter.Similar to
EmailService.SendEmailVerificationCodeAsync, this method validatestoPhoneNumberbut notverificationCode. Sending an SMS with an empty verification code should be prevented.🛡️ Proposed fix to add validation
public async Task<bool> SendSmsVerificationCodeAsync(string toPhoneNumber, string verificationCode, string departmentNumber) { - if (string.IsNullOrWhiteSpace(toPhoneNumber)) + if (string.IsNullOrWhiteSpace(toPhoneNumber) || string.IsNullOrWhiteSpace(verificationCode)) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/SmsService.cs` around lines 350 - 377, The SendSmsVerificationCodeAsync method lacks validation for the verificationCode parameter; add a check at the start of SendSmsVerificationCodeAsync (similar to EmailService.SendEmailVerificationCodeAsync) to return false if verificationCode is null, empty, or whitespace before composing messageBody or calling _textMessageProvider.SendTextMessage, ensuring you reference the verificationCode parameter and preserve the existing toPhoneNumber validation and exception handling.Core/Resgrid.Services/EmailService.cs-646-669 (1)
646-669:⚠️ Potential issue | 🟡 MinorMissing validation for
verificationCodeparameter.The method validates
toEmailAddressbut notverificationCode. Sending an email with an empty/null verification code could confuse users or indicate a bug in the calling code.🛡️ Proposed fix to add validation
public async Task<bool> SendEmailVerificationCodeAsync(string toEmailAddress, string firstName, string verificationCode) { - if (string.IsNullOrWhiteSpace(toEmailAddress)) + if (string.IsNullOrWhiteSpace(toEmailAddress) || string.IsNullOrWhiteSpace(verificationCode)) return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/EmailService.cs` around lines 646 - 669, In SendEmailVerificationCodeAsync validate the verificationCode parameter (in addition to toEmailAddress) before constructing the MailMessage: if verificationCode is null, empty, or whitespace, return false (and optionally log a warning) so we never send an email with a missing code; update the method near the existing toEmailAddress check and ensure behavior remains the same for _emailSender.SendEmail and the mail body that references Config.VerificationConfig.VerificationCodeExpiryMinutes.Web/Resgrid.Web/Areas/User/Views/TwoFactor/Enable2FA.cshtml-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minor
enforcedflag should not be inferred from query-key presence alone.Line 5 treats any
enforcedquery parameter astrue(including?enforced=false). Prefer a server-controlled model flag (or strict boolean parsing) for consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/TwoFactor/Enable2FA.cshtml` at line 5, The code currently sets the local variable `enforced` by checking Context.Request.Query.ContainsKey("enforced"), which treats any presence (including "?enforced=false") as true; change this to either read a server-controlled model property (e.g., a boolean on the view model) or perform strict boolean parsing of the query value (read Context.Request.Query["enforced"] and parse to bool using a safe parse) and assign the result to `enforced` so only explicit true values make it true; update references to the `enforced` variable in the Enable2FA.cshtml view to use the new model flag or parsed boolean.Web/Resgrid.Web/Areas/User/Views/Workflows/New.cshtml-40-40 (1)
40-40:⚠️ Potential issue | 🟡 MinorLocalize hard-coded user-facing text for consistency.
Lines 40 and 63 contain English literals in a localized view. Move these to resource keys to keep translation coverage complete.
Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Workflows/New.cshtml` at line 40, Replace the two hard-coded English strings in the New.cshtml view ("All available trigger event types already have a workflow configured. Delete an existing workflow to free up its event type." and the similar literal at line 63) with resource lookups: add two resource keys (e.g. Workflows_New_AllTriggerTypesConfigured and Workflows_New_DeleteToFreeEventType) to the appropriate .resx and then update New.cshtml to render those keys (e.g. `@Resources.Workflows_New_AllTriggerTypesConfigured` or the project’s localization helper like `@Html.Localize`("Workflows_New_AllTriggerTypesConfigured")). Ensure you remove the literal text and use the same localization pattern used elsewhere in the view so translations are discovered and consistent.Web/Resgrid.Web/Areas/User/Views/Workflows/Pending.cshtml-62-62 (1)
62-62:⚠️ Potential issue | 🟡 MinorUse culture-aware date formatting for queued time.
Line 62 uses a fixed
"yyyy-MM-dd HH:mm:ss"format, which is a localization mismatch in a localized UI. Prefer a culture-aware format (or a localized formatter helper).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Workflows/Pending.cshtml` at line 62, Replace the fixed format on run.QueuedOn with a culture-aware format: change the literal ToString("yyyy-MM-dd HH:mm:ss") to a culture-sensitive call such as run.QueuedOn.ToString("G", System.Globalization.CultureInfo.CurrentCulture) (or use String.Format("{0:G}", run.QueuedOn)) so the queued time respects the current UI culture; add or fully-qualify System.Globalization.CultureInfo if needed in Pending.cshtml.Providers/Resgrid.Providers.Workflow/WorkflowActionExecutorFactory.cs-13-16 (1)
13-16:⚠️ Potential issue | 🟡 MinorHandle duplicate ActionType registrations gracefully.
If two executors are registered with the same
ActionType,ToDictionarythrows a crypticArgumentException. Consider explicit duplicate detection with a clear error message.🛡️ Proposed defensive handling
public WorkflowActionExecutorFactory(IEnumerable<IWorkflowActionExecutor> executors) { - _executors = executors.ToDictionary(e => e.ActionType); + _executors = new Dictionary<WorkflowActionType, IWorkflowActionExecutor>(); + foreach (var executor in executors) + { + if (_executors.ContainsKey(executor.ActionType)) + throw new InvalidOperationException($"Duplicate executor registered for WorkflowActionType '{executor.ActionType}'. Only one executor per action type is allowed."); + _executors[executor.ActionType] = executor; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/WorkflowActionExecutorFactory.cs` around lines 13 - 16, The constructor in WorkflowActionExecutorFactory uses executors.ToDictionary(e => e.ActionType) which will throw a cryptic ArgumentException if duplicate IWorkflowActionExecutor.ActionType values are registered; modify the WorkflowActionExecutorFactory constructor to detect duplicates explicitly by grouping the incoming IEnumerable<IWorkflowActionExecutor> by ActionType, if any group has more than one element throw a clear ArgumentException (or InvalidOperationException) that names the duplicated ActionType and optionally the involved executor types, otherwise build the _executors dictionary from the unique groups and assign it.Web/Resgrid.Web/Areas/User/Views/Workflows/CredentialEdit.cshtml-12-29 (1)
12-29:⚠️ Potential issue | 🟡 MinorCredential type labels are hard-coded and bypass localization.
Lines 14-27 always render English text, which breaks localized UX (e.g., Spanish workflow pages).
🌐 Proposed fix
- WorkflowCredentialType.Smtp => "SMTP (Email)", - WorkflowCredentialType.Twilio => "Twilio (SMS/Voice)", + WorkflowCredentialType.Smtp => localizer["SmtpSettings"], + WorkflowCredentialType.Twilio => localizer["TwilioSettings"], WorkflowCredentialType.Ftp => "FTP", WorkflowCredentialType.Sftp => "SFTP", - WorkflowCredentialType.AwsS3 => "AWS S3", - WorkflowCredentialType.HttpBearer => "HTTP Bearer Token", - WorkflowCredentialType.HttpBasic => "HTTP Basic Auth", - WorkflowCredentialType.HttpApiKey => "HTTP API Key", - WorkflowCredentialType.MicrosoftTeams => "Microsoft Teams Webhook", + WorkflowCredentialType.AwsS3 => localizer["AwsS3Settings"], + WorkflowCredentialType.HttpBearer => localizer["HttpBearerSettings"], + WorkflowCredentialType.HttpBasic => localizer["HttpBasicSettings"], + WorkflowCredentialType.HttpApiKey => localizer["HttpApiKeySettings"], + WorkflowCredentialType.MicrosoftTeams => localizer["TeamsSettings"], WorkflowCredentialType.Slack => "Slack", WorkflowCredentialType.Discord => "Discord", - WorkflowCredentialType.AzureBlobStorage => "Azure Blob Storage", + WorkflowCredentialType.AzureBlobStorage => localizer["AzureBlobSettings"], WorkflowCredentialType.Box => "Box", WorkflowCredentialType.Dropbox => "Dropbox",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Workflows/CredentialEdit.cshtml` around lines 12 - 29, The switch on WorkflowCredentialType in CredentialEdit.cshtml is returning hard-coded English labels; change it to use localization by replacing each literal (e.g., "SMTP (Email)", "Twilio (SMS/Voice)", "AWS S3", etc.) with localized resource lookups (for example Localizer["WorkflowCredentialType_Smtp"], Localizer["WorkflowCredentialType_Twilio"], Localizer["WorkflowCredentialType_AwsS3"], etc.) and ensure the view has an IViewLocalizer/IStringLocalizer available (or have the controller populate a localized DisplayName dictionary for the view model). Add corresponding resource keys/translations to your .resx files for each WorkflowCredentialType enum value (fallback to e.ToString() if missing) so all enum labels render via the localization system instead of hard-coded English.Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs-135-143 (1)
135-143:⚠️ Potential issue | 🟡 MinorPotential null reference when department is null.
Line 142 calls
department.IsUserAnAdmin(UserId)butGetDepartmentByIdAsynccan return null. This would throw aNullReferenceException.🛡️ Proposed fix
private async Task<bool> CanUserManageWorkflowCredentialsAsync() { var permission = await _permissionsService.GetPermissionByDepartmentTypeAsync(DepartmentId, PermissionTypes.ManageWorkflowCredentials); var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId); + if (department == null) return false; var group = await _departmentGroupsService.GetGroupForUserAsync(UserId, DepartmentId); var roles = await _personnelRolesService.GetRolesForUserAsync(UserId, DepartmentId); bool isGroupAdmin = group != null && group.IsUserGroupAdmin(UserId); return _permissionsService.IsUserAllowed(permission, department.IsUserAnAdmin(UserId), isGroupAdmin, roles); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WorkflowCredentialsController.cs` around lines 135 - 143, In CanUserManageWorkflowCredentialsAsync, avoid a NullReferenceException from department.IsUserAnAdmin(UserId) by first checking whether department is null; replace the direct call with a safe boolean (e.g., bool isDeptAdmin = department != null && department.IsUserAnAdmin(UserId)) and pass isDeptAdmin into _permissionsService.IsUserAllowed; ensure handling of a null department follows existing permission semantics (treat as not admin or return false).Repositories/Resgrid.Repositories.DataRepository/WorkflowRunLogRepository.cs-36-42 (1)
36-42:⚠️ Potential issue | 🟡 MinorNull-safe transaction access required.
Line 41 accesses
_unitOfWork.Transactiondirectly, but line 45 explicitly handles_unitOfWorkbeing null. This can cause aNullReferenceExceptionwhen_unitOfWorkis null butselectFunctionis invoked.🛡️ Proposed fix
- return await x.QueryAsync<WorkflowRunLog>(sql: query, param: dp, transaction: _unitOfWork.Transaction); + return await x.QueryAsync<WorkflowRunLog>(sql: query, param: dp, transaction: _unitOfWork?.Transaction);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/WorkflowRunLogRepository.cs` around lines 36 - 42, The lambda selectFunction captures _unitOfWork.Transaction directly which can throw if _unitOfWork is null; fix by resolving the transaction into a local variable (e.g., var tx = _unitOfWork?.Transaction) before creating selectFunction or by using a null-safe access inside the lambda (e.g., _unitOfWork?.Transaction) so that QueryAsync is called with a possibly null transaction; update the code that builds selectFunction (and any references to _unitOfWork.Transaction) to use that null-safe local variable or conditional access.Providers/Resgrid.Providers.Bus/WorkflowEventProvider.cs-163-168 (1)
163-168:⚠️ Potential issue | 🟡 MinorDaily count incremented per-workflow but limit checked once.
When multiple workflows match the same event,
IncrementDailyCountis called for each workflow (line 167), butIsWithinDailyLimitwas only checked once at the start (line 114). This means if a department has 99 of 100 daily runs consumed and an event triggers 5 workflows, all 5 will be enqueued (consuming 104 total), exceeding the limit.Consider checking the daily limit inside the loop or reserving capacity upfront.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Bus/WorkflowEventProvider.cs` around lines 163 - 168, The current logic calls IncrementDailyCount(departmentId) for each matched workflow after enqueueing but only calls IsWithinDailyLimit(departmentId) once before the loop, allowing multiple enqueues to exceed the allowed daily quota; update the loop around _outboundQueueProvider.EnqueueWorkflow(queueItem) so you either (a) call IsWithinDailyLimit(departmentId) inside the per-workflow loop and skip/enforce stopping when false, or (b) reserve capacity up-front by calling a new method like ReserveDailyCapacity(departmentId, count) before iterating and only enqueue as many workflows as the reserved capacity allows, ensuring IncrementDailyCount is invoked exactly once per actual enqueue. Ensure changes reference the existing methods IncrementDailyCount, IsWithinDailyLimit, and the enqueue call on _outboundQueueProvider.EnqueueWorkflow so the check and count update remain consistent.Providers/Resgrid.Providers.Workflow/Executors/SmtpEmailExecutor.cs-75-80 (1)
75-80:⚠️ Potential issue | 🟡 MinorInvalid email addresses will throw unhandled
FormatException.
MailboxAddress.Parse()throwsFormatExceptionfor malformed email addresses. While the outer catch will handle it, the error message won't be user-friendly. Consider validating email format before parsing or catching this specific exception.🛡️ Proposed fix with validation
+// Validate email addresses before parsing +try +{ var message = new MimeMessage(); message.From.Add(MailboxAddress.Parse(cred.FromAddress)); foreach (var to in toAddresses) message.To.Add(MailboxAddress.Parse(to)); foreach (var cc in ccAddresses) message.Cc.Add(MailboxAddress.Parse(cc)); +} +catch (FormatException ex) +{ + return WorkflowActionResult.Failed("SMTP send failed.", $"Invalid email address format: {ex.Message}"); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/SmtpEmailExecutor.cs` around lines 75 - 80, The code currently calls MailboxAddress.Parse on cred.FromAddress, entries in toAddresses and ccAddresses (creating message in SmtpEmailExecutor.cs) which will throw FormatException for malformed emails; update the construction to validate addresses first (use MailboxAddress.TryParse where available or pre-validate with a simple regex/EmailAddressAttribute) and handle invalid entries by logging or returning a clear user-friendly error instead of letting FormatException bubble up; ensure you reference the variables message, cred.FromAddress, toAddresses and ccAddresses and either skip/log invalid addresses or throw a controlled exception with a helpful message so the outer catch no longer surfaces raw FormatException details.Providers/Resgrid.Providers.Workflow/Executors/S3FileExecutor.cs-59-60 (1)
59-60:⚠️ Potential issue | 🟡 Minor
TransferUtilityshould be disposed.
TransferUtilityimplementsIDisposable. While it may not hold significant resources in this case, it's good practice to dispose it properly.🛠️ Proposed fix
-var transferUtility = new TransferUtility(s3Client); -await transferUtility.UploadAsync(uploadRequest, cancellationToken); +using var transferUtility = new TransferUtility(s3Client); +await transferUtility.UploadAsync(uploadRequest, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/S3FileExecutor.cs` around lines 59 - 60, The TransferUtility instance created in S3FileExecutor.cs (the transferUtility variable used before calling UploadAsync) implements IDisposable and must be disposed; update the code in the method that constructs new TransferUtility(s3Client) (where UploadAsync(uploadRequest, cancellationToken) is called) to ensure TransferUtility is disposed—e.g. wrap creation and the await transferUtility.UploadAsync call in a using statement/using declaration or try/finally and call transferUtility.Dispose() so the resource is released properly.Providers/Resgrid.Providers.Workflow/Executors/SftpFileExecutor.cs-40-41 (1)
40-41:⚠️ Potential issue | 🟡 MinorPotential path construction issue with null
RemotePath.If
config.RemotePathis null (despite the default in class definition), the path becomes"null/filename". The null-conditional?.TrimEnd('/')returns null when the input is null.🛠️ Proposed fix
var filename = config.Filename ?? $"workflow_{DateTime.UtcNow:yyyyMMddHHmmss}.txt"; -var remotePath = $"{config.RemotePath?.TrimEnd('/')}/{filename}"; +var remotePath = $"{(config.RemotePath ?? "/").TrimEnd('/')}/{filename}";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/SftpFileExecutor.cs` around lines 40 - 41, The remotePath construction can yield "null/filename" when config.RemotePath is null because of the null-conditional TrimEnd; update the logic in SftpFileExecutor (around the filename and remotePath variables) to normalize/configure the directory first (e.g., var dir = (config.RemotePath ?? string.Empty).TrimEnd('/')) and then build remotePath by conditionally joining dir and filename (if dir is empty use filename, otherwise $"{dir}/{filename}") so you never include the literal "null" or an extra slash.Providers/Resgrid.Providers.Workflow/Executors/AzureBlobExecutor.cs-51-51 (1)
51-51:⚠️ Potential issue | 🟡 MinorBlob uploads with second-precision paths risk silent data loss.
BlobClient.UploadAsyncwithBlobUploadOptionsoverwrites existing blobs by default. The auto-generated path uses only seconds precision (HHmmss), so multiple calls within the same second will silently overwrite prior uploads. Consider using higher-precision timestamps (e.g.,HHmmssffffor milliseconds) or implementing collision detection if this executor may be called multiple times per second.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/AzureBlobExecutor.cs` at line 51, The upload currently uses BlobClient.UploadAsync in AzureBlobExecutor with an auto-generated path using seconds precision (HHmmss), which can cause silent overwrites; update the path generation used before calling BlobClient.UploadAsync (e.g., the blob name/variable built in AzureBlobExecutor) to include higher-precision timestamps like HHmmssfff or append a collision-safe suffix (GUID or incremental counter) so each upload is unique, or implement a check for existing blob and retry with a new name before calling UploadAsync; ensure BlobUploadOptions usage remains unchanged and only the generated blob path/identifier is modified.Providers/Resgrid.Providers.Workflow/Executors/DiscordMessageExecutor.cs-30-31 (1)
30-31:⚠️ Potential issue | 🟡 MinorWebhook URL leaked in error message.
The full webhook URL is included in the error message, which could expose sensitive tokens embedded in Discord webhook URLs when errors are logged or displayed to users.
🔒 Proposed fix to sanitize the URL
if (!Uri.TryCreate(cred.WebhookUrl, UriKind.Absolute, out _)) - return WorkflowActionResult.Failed("Discord message failed.", $"The configured WebhookUrl '{cred.WebhookUrl}' is not a valid absolute URI."); + return WorkflowActionResult.Failed("Discord message failed.", "The configured WebhookUrl is not a valid absolute URI.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Workflow/Executors/DiscordMessageExecutor.cs` around lines 30 - 31, The error currently returns the full webhook URL (cred.WebhookUrl) which leaks secret tokens; change the Uri.TryCreate to capture the parsed Uri (e.g., out var uri) instead of out _ and build a sanitized message that does not include the full URL (for example use uri.Scheme + "://" + uri.Host + "/..." or simply "<redacted webhook url>") when returning WorkflowActionResult.Failed from the DiscordMessageExecutor validation block, replacing the direct usage of cred.WebhookUrl in the error string.Web/Resgrid.Web/Controllers/AccountController.cs-676-676 (1)
676-676:⚠️ Potential issue | 🟡 MinorTypo in cache key:
_$should likely be_.The cache key
CacheConfig.ApiBearerTokenKeyName + $"_${userId}"contains_$which appears to be a typo. This could cause cache key mismatches when retrieving the token elsewhere.🐛 Proposed fix
- await _cacheProvider.SetStringAsync(CacheConfig.ApiBearerTokenKeyName + $"_${userId}", token, new TimeSpan(48, 0, 0)); + await _cacheProvider.SetStringAsync(CacheConfig.ApiBearerTokenKeyName + $"_{userId}", token, new TimeSpan(48, 0, 0));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Controllers/AccountController.cs` at line 676, The cache key concatenation in AccountController when calling _cacheProvider.SetStringAsync uses a typo $"_${userId}" which introduces an extra dollar sign and will break lookups; update the key construction to use the correct separator (e.g., CacheConfig.ApiBearerTokenKeyName + $"_{userId}" or string.Concat(CacheConfig.ApiBearerTokenKeyName, "_", userId)) so the stored token (variable token) is keyed consistently with retrieval logic.Web/Resgrid.Web/Controllers/AccountController.cs-292-293 (1)
292-293:⚠️ Potential issue | 🟡 MinorFix fallback user lookup logic to use session-stored username.
The fallback
await _userManager.FindByNameAsync(model.Provider ?? string.Empty)is incorrect. TheProviderfield inVerifyCodeViewModelis never populated in the form and is unused throughout the flow. WhenGetTwoFactorAuthenticationUserAsync()returns null, the fallback attempts to find a user by an empty string, which will fail.The correct fallback should retrieve the username from the session values that were stored during login:
HttpContext.Session.GetString("Resgrid2FAPendingUsername").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Controllers/AccountController.cs` around lines 292 - 293, The fallback user lookup should use the session-stored username instead of model.Provider; replace the current fallback call to _userManager.FindByNameAsync(model.Provider ?? string.Empty) with retrieving HttpContext.Session.GetString("Resgrid2FAPendingUsername") and passing that to _userManager.FindByNameAsync (guarding for null/empty before calling). Update the logic around GetTwoFactorAuthenticationUserAsync() so it falls back to _userManager.FindByNameAsync(sessionUsername) and handle the case where sessionUsername is missing by logging/returning an appropriate error.Web/Resgrid.Web/Areas/User/Controllers/TwoFactorController.cs-148-149 (1)
148-149:⚠️ Potential issue | 🟡 MinorUse localized strings for all user-facing 2FA messages.
These status/error messages are hardcoded while this controller already uses
_localizer; this creates mixed-language UX in localized views.Also applies to: 191-191, 235-235, 264-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TwoFactorController.cs` around lines 148 - 149, Replace hardcoded user-facing 2FA messages in TwoFactorController by using the existing _localizer instead of literal strings: locate the TempData["StatusMessage"] assignments (e.g., the message "Your authenticator app has been verified. Save your recovery codes below." and the other occurrences noted around the same method/flow at the other lines) and change them to use _localizer with resource keys (for example _localizer["AuthenticatorVerified_SaveRecovery"] or similar descriptive keys) so all status/error messages come from localization resources; add/update the corresponding entries in the resource file for those keys.Web/Resgrid.Web/Areas/User/Controllers/TwoFactorController.cs-291-303 (1)
291-303:⚠️ Potential issue | 🟡 MinorGuard
FormatKeyagainst null keys.If key retrieval returns null/empty,
FormatKeythrows before the view can recover gracefully.🩹 Proposed null-safe guard
private static string FormatKey(string unformattedKey) { + if (string.IsNullOrWhiteSpace(unformattedKey)) + return string.Empty; + var result = new StringBuilder(); int currentPosition = 0; while (currentPosition + 4 < unformattedKey.Length)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TwoFactorController.cs` around lines 291 - 303, FormatKey currently assumes unformattedKey is non-null and accesses Length, causing exceptions when the key is null/empty; update FormatKey to guard at the top (e.g., if string.IsNullOrEmpty/unformattedKey is null or whitespace) and return String.Empty (or an empty lower-case string) early, otherwise proceed with the existing splitting logic and final ToLowerInvariant call to preserve behavior; reference the FormatKey method to locate and patch the guard.
| var department = await _departmentsService.GetDepartmentByIdAsync(departmentId); | ||
|
|
||
| if (group != null) | ||
| isGroupAdmin = group.IsUserGroupAdmin(userId); | ||
|
|
||
| return _permissionsService.IsUserAllowed(permission, department.IsUserAnAdmin(userId), isGroupAdmin, roles); |
There was a problem hiding this comment.
Guard department before dereferencing.
These methods call department.IsUserAnAdmin(userId) without checking whether GetDepartmentByIdAsync returned null.
🛡️ Proposed fix
var department = await _departmentsService.GetDepartmentByIdAsync(departmentId);
+if (department == null)
+ return false;
if (group != null)
isGroupAdmin = group.IsUserGroupAdmin(userId);Also applies to: 1460-1465, 1475-1480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/AuthorizationService.cs` around lines 1445 - 1450, The
code calls _departmentsService.GetDepartmentByIdAsync(departmentId) then
dereferences department.IsUserAnAdmin(userId) without a null check; update the
AuthorizationService methods that use GetDepartmentByIdAsync (including the
blocks around department.IsUserAnAdmin shown and the similar blocks at the other
locations) to guard against department == null — e.g., check the returned
department and treat a missing department as not an admin (pass false into
IsUserAnAdmin/IUserAllowed) or short-circuit to the appropriate permission
outcome before calling department.IsUserAnAdmin, ensuring all uses of
department.IsUserAnAdmin and the final call to _permissionsService.IsUserAllowed
handle department being null.
| if (WorkflowQueueReceived != null) | ||
| { | ||
| var workflowQueueConsumer = new AsyncEventingBasicConsumer(_channel); | ||
| workflowQueueConsumer.ReceivedAsync += async (model, ea) => | ||
| { | ||
| if (ea != null && ea.Body.Length > 0) | ||
| { | ||
| Resgrid.Model.Queue.WorkflowQueueItem workflowItem = null; | ||
| try | ||
| { | ||
| var body = ea.Body; | ||
| var message = Encoding.UTF8.GetString(body.ToArray()); | ||
| workflowItem = ObjectSerialization.Deserialize<Resgrid.Model.Queue.WorkflowQueueItem>(message); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex, Encoding.UTF8.GetString(ea.Body.ToArray())); | ||
| } | ||
|
|
||
| try | ||
| { | ||
| if (workflowItem != null && WorkflowQueueReceived != null) | ||
| await WorkflowQueueReceived.Invoke(workflowItem); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logging.LogException(ex); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| await _channel.BasicConsumeAsync( | ||
| queue: RabbitConnection.SetQueueNameForEnv(ServiceBusConfig.WorkflowQueueName), | ||
| autoAck: true, | ||
| consumer: workflowQueueConsumer); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the workflow consumer implementation and other consumers for ack strategy comparison
rg -n "BasicConsumeAsync|autoAck|BasicAckAsync|BasicNackAsync" Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs -A 2 -B 2Repository: Resgrid/Core
Length of output: 8788
🏁 Script executed:
#!/bin/bash
# Get full context of lines around 564-599
sed -n '560,605p' Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs | cat -nRepository: Resgrid/Core
Length of output: 1635
🏁 Script executed:
#!/bin/bash
# Search for RetryQueueItem method to verify it exists
rg -n "RetryQueueItem|async.*Retry" Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs -A 5Repository: Resgrid/Core
Length of output: 2101
🏁 Script executed:
#!/bin/bash
# Check for WorkflowQueueItem imports and usage
rg -n "WorkflowQueueItem|using.*Queue" Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs | head -20Repository: Resgrid/Core
Length of output: 347
Use manual acknowledgements for workflow queue messages to match the established pattern used for critical queue consumers.
At line 597, autoAck: true acknowledges messages before processing completes. If deserialization (lines 571–578) or handler execution (lines 579–587) fails, the message is lost with no retry path. This is inconsistent with all other business-critical queue consumers in this file (CallQueue, MessageQueue, Notification, Events), which use autoAck: false with explicit retry logic via RetryQueueItem.
🔧 Proposed fix
if (WorkflowQueueReceived != null)
{
var workflowQueueConsumer = new AsyncEventingBasicConsumer(_channel);
workflowQueueConsumer.ReceivedAsync += async (model, ea) =>
{
if (ea != null && ea.Body.Length > 0)
{
- Resgrid.Model.Queue.WorkflowQueueItem workflowItem = null;
+ WorkflowQueueItem workflowItem = null;
try
{
var body = ea.Body;
var message = Encoding.UTF8.GetString(body.ToArray());
- workflowItem = ObjectSerialization.Deserialize<Resgrid.Model.Queue.WorkflowQueueItem>(message);
+ workflowItem = ObjectSerialization.Deserialize<WorkflowQueueItem>(message);
}
catch (Exception ex)
{
+ await _channel.BasicNackAsync(ea.DeliveryTag, false, false);
Logging.LogException(ex, Encoding.UTF8.GetString(ea.Body.ToArray()));
+ return;
}
try
{
if (workflowItem != null && WorkflowQueueReceived != null)
- await WorkflowQueueReceived.Invoke(workflowItem);
+ {
+ await WorkflowQueueReceived.Invoke(workflowItem);
+ await _channel.BasicAckAsync(ea.DeliveryTag, false);
+ }
}
catch (Exception ex)
{
Logging.LogException(ex);
+ if (await RetryQueueItem(ea, ex))
+ await _channel.BasicAckAsync(ea.DeliveryTag, false);
+ else
+ await _channel.BasicNackAsync(ea.DeliveryTag, false, true);
}
}
};
await _channel.BasicConsumeAsync(
queue: RabbitConnection.SetQueueNameForEnv(ServiceBusConfig.WorkflowQueueName),
- autoAck: true,
+ autoAck: false,
consumer: workflowQueueConsumer);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Providers/Resgrid.Providers.Bus.Rabbit/RabbitInboundQueueProvider.cs` around
lines 564 - 599, The WorkflowQueue consumer currently sets autoAck: true which
ACKs messages before processing; change the BasicConsumeAsync call to use
autoAck: false and implement manual acknowledgements on the
AsyncEventingBasicConsumer. Inside the ReceivedAsync handler for
workflowQueueConsumer, after successful deserialization and await
WorkflowQueueReceived.Invoke(workflowItem) call, call the channel's BasicAck for
ea.DeliveryTag; on failures (deserialization or handler exceptions) call the
same retry path used by other consumers—invoke RetryQueueItem with the
WorkflowQueueItem (or construct a retry wrapper when workflowItem is null) and
then BasicNack (or BasicReject) with requeue=false as appropriate. Use the
existing _channel, WorkflowQueueReceived, WorkflowQueueItem, RetryQueueItem, and
BasicConsumeAsync symbols to mirror the error/retry pattern used by
CallQueue/MessageQueue/Notification consumers.
| TwilioClient.Init(cred.AccountSid, cred.AuthToken); | ||
|
|
||
| string lastSid = null; | ||
| foreach (var recipient in recipients) | ||
| { | ||
| var message = await MessageResource.CreateAsync( | ||
| body: context.RenderedContent, | ||
| from: new Twilio.Types.PhoneNumber(cred.FromNumber), | ||
| to: new Twilio.Types.PhoneNumber(recipient)); | ||
| lastSid = message.Sid; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Twilio .NET SDK 7.5.1 TwilioClient.Init global state thread safety multi-tenant
💡 Result:
In Twilio .NET SDK 7.5.1, TwilioClient.Init(...) configures a static/global TwilioClient that internally holds a singleton TwilioRestClient; this is fine if your app uses one Twilio account, but it’s the wrong model for multi-tenant (multiple Twilio accounts/credentials in the same process). Twilio explicitly calls out that you can’t support a multi-tenant “multiple accounts” scenario with the static TwilioClient; you should instead create your own TwilioRestClient instances and pass them per call. [1]
What this means for thread-safety / multi-tenancy
- Thread-safety for normal API calls: once initialized, using the static client for requests is generally “okay” for concurrent calls as long as you never change the global configuration.
- Not safe for multi-tenant credential switching: calling
TwilioClient.Init(...)(or other static setters like region/edge/log level) per request/tenant is effectively mutating shared global state; concurrent requests can race and send requests with the wrong tenant’s credentials/config. This is the core multi-tenant hazard. [1] - HTTP client behavior: Twilio notes the static
TwilioClientholds onto a singletonTwilioRestClient(and thus a singleHttpClient), reinforcing that the static model is process-wide, not tenant-scoped. [1]
Recommended multi-tenant pattern
Create a tenant-scoped TwilioRestClient (per tenant/account) and pass it into resource methods via the client: parameter, e.g. MessageResource.Create(..., client: twilioClient)—this is the documented alternative to relying on global static state. [1][2]
If you’re on ASP.NET Core, Twilio also provides DI-oriented patterns around TwilioRestClient (instead of the static TwilioClient) to avoid the static/singleton limitations. [1]
Sources: Twilio SDK guidance on static TwilioClient vs instance TwilioRestClient (including explicit multi-tenant limitation and singleton behavior) [1], plus an example of instantiating TwilioRestClient and passing it per call [2].
🌐 Web query:
Twilio .NET SDK TwilioRestClient per-request credentials MessageResource.CreateAsync
💡 Result:
To use per-request credentials with the Twilio .NET SDK, don’t call TwilioClient.Init(...). Instead, create a new TwilioRestClient with the credentials you want for that specific call, and pass it via the optional client: parameter on MessageResource.CreateAsync(...). [1][2]
using Twilio.Clients;
using Twilio.Rest.Api.V2010.Account;
using Twilio.Types;
async Task SendWithPerRequestCreds(
string accountSid, // AC... (the Twilio Account SID whose /Accounts/{AccountSid} path you want)
string apiKeySid, // SK...
string apiKeySecret, // key secret
string fromE164,
string toE164,
string body)
{
var client = new TwilioRestClient(
username: apiKeySid,
password: apiKeySecret,
accountSid: accountSid // important when username != AccountSid (e.g., API Key auth)
); // [1]
var msg = await MessageResource.CreateAsync(
from: new PhoneNumber(fromE164),
to: new PhoneNumber(toE164),
body: body,
client: client
); // [2]
}Notes:
TwilioRestClientsupportsaccountSidas an explicit parameter; if you omit it, it defaults tousername. [1]MessageResource.CreateAsyncaccepts aclient:parameter specifically for this “use a different client/credentials for this call” scenario. [2]
Sources: TwilioRestClient constructor and accountSid defaulting behavior [1]; Twilio example showing MessageResource.CreateAsync(..., client: twilioClient) [2].
[1] https://raw.githubusercontent.com/twilio/twilio-csharp/main/src/Twilio/Clients/TwilioRestClient.cs
[2] https://www.twilio.com/en-us/blog/whats-new-in-twilio-helper-library-for-aspnet-v6
🏁 Script executed:
# Find and examine the TwilioSmsExecutor.cs file
find . -name "TwilioSmsExecutor.cs" -type fRepository: Resgrid/Core
Length of output: 125
🏁 Script executed:
# Get context around the problematic lines and understand the class structure
fd TwilioSmsExecutor.cs --exec cat -n {} \;Repository: Resgrid/Core
Length of output: 4091
Avoid process-global Twilio client initialization in this multitenant executor.
Line 56 uses TwilioClient.Init(...) which configures a static process-level singleton. Concurrent workflow executions with different credentials will race to re-initialize this global state, causing messages to send using the wrong account context.
Instead, create a per-request client instance and pass it explicitly:
Fix: Use per-request TwilioRestClient
- TwilioClient.Init(cred.AccountSid, cred.AuthToken);
string lastSid = null;
foreach (var recipient in recipients)
{
+ var twilioClient = new Twilio.Clients.TwilioRestClient(cred.AccountSid, cred.AuthToken);
var message = await MessageResource.CreateAsync(
body: context.RenderedContent,
from: new Twilio.Types.PhoneNumber(cred.FromNumber),
- to: new Twilio.Types.PhoneNumber(recipient));
+ to: new Twilio.Types.PhoneNumber(recipient),
+ client: twilioClient);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TwilioClient.Init(cred.AccountSid, cred.AuthToken); | |
| string lastSid = null; | |
| foreach (var recipient in recipients) | |
| { | |
| var message = await MessageResource.CreateAsync( | |
| body: context.RenderedContent, | |
| from: new Twilio.Types.PhoneNumber(cred.FromNumber), | |
| to: new Twilio.Types.PhoneNumber(recipient)); | |
| lastSid = message.Sid; | |
| string lastSid = null; | |
| foreach (var recipient in recipients) | |
| { | |
| var twilioClient = new Twilio.Clients.TwilioRestClient(cred.AccountSid, cred.AuthToken); | |
| var message = await MessageResource.CreateAsync( | |
| body: context.RenderedContent, | |
| from: new Twilio.Types.PhoneNumber(cred.FromNumber), | |
| to: new Twilio.Types.PhoneNumber(recipient), | |
| client: twilioClient); | |
| lastSid = message.Sid; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Providers/Resgrid.Providers.Workflow/Executors/TwilioSmsExecutor.cs` around
lines 56 - 65, TwilioClient.Init is setting process-global credentials which can
leak between tenants; replace that with a per-request TwilioRestClient created
from the current cred (use new
Twilio.Rest.Clients.TwilioRestClient(cred.AccountSid, cred.AuthToken)) and
remove the TwilioClient.Init call, then call MessageResource.CreateAsync(...,
client: twilioClient) for each recipient so each message uses the per-request
client; update the method that currently uses lastSid and
MessageResource.CreateAsync to accept/use the local twilioClient instance
instead of relying on static TwilioClient state.
| <a asp-action="Runs" asp-route-workflowId="@wf.WorkflowId" class="btn btn-xs btn-default"><i class="fa fa-list"></i> @localizer["Runs"]</a> | ||
| <a asp-action="Health" asp-route-workflowId="@wf.WorkflowId" class="btn btn-xs btn-info"><i class="fa fa-heartbeat"></i> @localizer["Health"]</a> | ||
| <form asp-action="Delete" method="post" style="display:inline;" | ||
| onsubmit="return confirm('@Html.Raw(string.Format(localizer["DeleteWorkflowConfirm"].Value, wf.Name))');"> |
There was a problem hiding this comment.
Do not inject raw workflow names into inline JavaScript confirm text.
Line 88 uses Html.Raw with wf.Name inside a JS string. A crafted name containing quotes can break out of the confirm string and trigger script injection.
🛡️ Safer confirm message encoding
+@using System.Text.Encodings.Web
...
-<form asp-action="Delete" method="post" style="display:inline;"
- onsubmit="return confirm('@Html.Raw(string.Format(localizer["DeleteWorkflowConfirm"].Value, wf.Name))');">
+@{
+ var deleteConfirmMessage = string.Format(localizer["DeleteWorkflowConfirm"].Value, wf.Name);
+}
+<form asp-action="Delete" method="post" style="display:inline;"
+ onsubmit="return confirm('@JavaScriptEncoder.Default.Encode(deleteConfirmMessage)');">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Workflows/Index.cshtml` at line 88, The
inline confirm currently injects raw wf.Name via Html.Raw into JavaScript;
instead build the formatted message using
localizer["DeleteWorkflowConfirm"].Value and wf.Name, then JavaScript-encode
that resulting string before placing it inside the onsubmit confirm call (e.g.,
use HttpUtility.JavaScriptStringEncode or Json.Encode/Html.Raw(Json.Encode(...))
on the formatted text) so quotes/characters in wf.Name cannot break out and
cause script injection; update the onsubmit usage that references wf.Name and
localizer["DeleteWorkflowConfirm"].Value to use the encoded string.
| private const string StepUpSessionKey = "Resgrid2FAVerifiedAt"; | ||
|
|
||
| public override void OnActionExecuting(ActionExecutingContext context) | ||
| { | ||
| var user = context.HttpContext.User; | ||
|
|
||
| // Not authenticated — let the framework handle it | ||
| if (user?.Identity == null || !user.Identity.IsAuthenticated) | ||
| { | ||
| base.OnActionExecuting(context); | ||
| return; | ||
| } | ||
|
|
||
| // Check if user has 2FA enabled via the claim set by Identity | ||
| // TwoFactorEnabled is persisted on the IdentityUser; we check via UserManager | ||
| // but that requires async — use session flag set during login/step-up instead. | ||
| // If session key is absent the user must re-verify. | ||
| var session = context.HttpContext.Session; | ||
| var verifiedAtRaw = session.GetString(StepUpSessionKey); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where the step-up session key is set/read.
rg -nP 'Resgrid2FAVerifiedAt|StepUpSessionKey' --type=cs
# Find logout/signout/session-clear paths.
rg -nP 'SignOutAsync|LogOff|Session\.(Clear|Remove)\(' --type=csRepository: Resgrid/Core
Length of output: 3334
🏁 Script executed:
#!/bin/bash
# Get the full implementation of LogOff method
sed -n '441,460p' Web/Resgrid.Web/Controllers/AccountController.cs
# Check if Session.Clear() is used anywhere in AccountController
rg -n 'Session\.Clear\(' Web/Resgrid.Web/Controllers/AccountController.cs
# Also check the context around LogOff to see if there's more code
sed -n '440,475p' Web/Resgrid.Web/Controllers/AccountController.csRepository: Resgrid/Core
Length of output: 1634
🏁 Script executed:
#!/bin/bash
# Find the RemoveCookies method implementation
rg -n -A 15 'void RemoveCookies\(\)|private void RemoveCookies' Web/Resgrid.Web/Controllers/AccountController.cs
# Also check if Session.Clear() appears anywhere in the entire account controller
rg -n 'Session\.Clear\(\)' Web/Resgrid.Web/Controllers/Repository: Resgrid/Core
Length of output: 642
🏁 Script executed:
#!/bin/bash
# Check if Session.Clear() is called anywhere
rg -n 'Session\.Clear\(\)' Web/Resgrid.Web/ --type=cs
# Also check if there's middleware that clears session
rg -n 'session.Clear|Session.Clear' Web/Resgrid.Web/ --type=csRepository: Resgrid/Core
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Read the full RequiresRecentTwoFactorAttribute.cs file
cat -n Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.csRepository: Resgrid/Core
Length of output: 2895
Bind step-up proof to the current user, not only the session timestamp.
At lines 37–44, the filter validates only that a timestamp exists and is recent—it never checks the user identity. If session state persists across sign-out or account switch (e.g., distributed session store), a different user can inherit the previous user's recent step-up state and bypass 2FA re-verification for sensitive operations.
Required fix: Include the current user's identity (e.g., UserId) in the session key or value to ensure each user's step-up proof is isolated. Additionally, clear the session key during LogOff (currently not cleared, unlike other sensitive session keys in the same controller).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Attributes/RequiresRecentTwoFactorAttribute.cs` around lines
19 - 37, The current RequiresRecentTwoFactorAttribute.OnActionExecuting uses
only StepUpSessionKey ("Resgrid2FAVerifiedAt") and a timestamp from session,
which allows a different user to inherit another user's step-up proof; change
the logic to bind the step-up proof to the current user by incorporating the
current user's unique id (e.g., user.GetUserId() or claim "sub"/NameIdentifier)
into the session key or by storing a structured value that contains both UserId
and verified timestamp (update references to StepUpSessionKey usage in
OnActionExecuting accordingly), validate that the stored UserId matches
context.HttpContext.User before accepting the timestamp, and ensure the same
session key/value is explicitly cleared during the LogOff method in your
account/controller to remove any residual step-up proof on sign-out.
|
|
||
| try | ||
| { | ||
| var department = await departmentsService.GetDepartmentByUserIdAsync(user.UserName); |
There was a problem hiding this comment.
Use the user ID for department lookup.
Line 79 passes user.UserName to GetDepartmentByUserIdAsync, which can return null for valid users and silently disable enforcement.
🔧 Proposed fix
- var department = await departmentsService.GetDepartmentByUserIdAsync(user.UserName);
+ var department = await departmentsService.GetDepartmentByUserIdAsync(user.Id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var department = await departmentsService.GetDepartmentByUserIdAsync(user.UserName); | |
| var department = await departmentsService.GetDepartmentByUserIdAsync(user.Id); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Middleware/Require2FAEnrollmentMiddleware.cs` at line 79, The
department lookup is using user.UserName when calling
departmentsService.GetDepartmentByUserIdAsync which can return null; update the
lookup to use the canonical user ID instead (pass the user's Id or obtain it via
UserManager.GetUserId(user) if you only have a ClaimsPrincipal) so
Require2FAEnrollmentMiddleware calls
departmentsService.GetDepartmentByUserIdAsync with the actual user identifier
rather than UserName.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit