Skip to content

RE1-T112 fixes #326

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ucswift merged 1 commit into from
Apr 11, 2026
Merged

RE1-T112 fixes #326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions Core/Resgrid.Services/WeatherAlertService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,18 +363,19 @@ public async Task SendPendingNotificationsAsync(CancellationToken ct = default)
SentOn = DateTime.UtcNow,
SystemGenerated = true,
IsBroadcast = true,
Type = 0,
Recipients = string.Join(",", members.Select(m => m.UserId))
Type = 0
};

// Use SendMessageAsync which saves AND enqueues for push/SMS/email delivery
var sent = await _messageService.SendMessageAsync(
message, "Weather Alert System", departmentId, true, ct);

if (sent)
foreach (var member in members)
{
alert.SystemMessageId = message.MessageId;
if (member.UserId != senderId)
message.AddRecipient(member.UserId);
}

var savedMessage = await _messageService.SaveMessageAsync(message, ct);
await _messageService.SendMessageAsync(savedMessage, "Weather Alert System", departmentId, false, ct);
Comment on lines +369 to +376
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't persist one shared Message for all recipients.

The previous broadcastSingle: true path created one Message row per recipient. This version saves a single row with multiple MessageRecipients, but ReceivingUserId, ReadOn, and IsDeleted still live on Message itself. That changes inbox semantics for weather alerts from recipient-scoped state to shared state.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 369 - 376, The
code currently reuses a single Message instance (variable message) for all
recipients, then calls _messageService.SaveMessageAsync and
_messageService.SendMessageAsync, which incorrectly shares recipient-scoped
state (ReceivingUserId, ReadOn, IsDeleted) across users; instead, iterate the
members and for each member (in the loop where you call
message.AddRecipient(member.UserId)) create a new Message instance or clone the
original, set the recipient-specific fields (ReceivingUserId etc.) for that
user, then call _messageService.SaveMessageAsync(newMessage, ct) and
_messageService.SendMessageAsync(savedMessage, "Weather Alert System",
departmentId, false, ct) per recipient so each user gets their own Message row
as the previous broadcastSingle:true path did.


alert.SystemMessageId = savedMessage.MessageId;
}
}
catch (Exception ex)
Expand Down
16 changes: 4 additions & 12 deletions Web/Resgrid.Web/Filters/RequireActivePlanFilter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,22 +19,14 @@ public class RequireActivePlanFilter : IAsyncActionFilter
// Actions that are always reachable regardless of plan status
private static readonly string[] ExemptControllers = new[]
{
"account"
"account",
"subscription"
Comment on lines +22 to +23
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Controller-level subscription exemption is too broad and bypasses plan enforcement.

On Line 23, exempting the entire Subscription controller skips this filter for all subscription endpoints, including sensitive mutation flows (e.g., cancel/addon management). That weakens the intended “active plan required” gate and can allow no-plan departments into operations that should stay restricted.

Suggested fix (narrow exemption to explicit routes) 
-		private static readonly string[] ExemptControllers = new[]
-		{
-			"account",
-			"subscription"
-		};
+		private static readonly string[] ExemptControllers = new[]
+		{
+			"account"
+		};
+
+		private static readonly (string Controller, string Action)[] ExemptRoutes = new[]
+		{
+			("subscription", "selectregistrationplan"),
+			("subscription", "logstriperesponse"),
+			("subscription", "validatecoupon")
+		};
@@
-			// Skip exempt actions (payment flow, plan selection itself)
+			// Skip explicitly exempt controller/action routes
+			foreach (var route in ExemptRoutes)
+			{
+				if (string.Equals(controller, route.Controller, StringComparison.OrdinalIgnoreCase) &&
+					string.Equals(action, route.Action, StringComparison.OrdinalIgnoreCase))
+				{
+					await next();
+					return;
+				}
+			}
+
+			// Skip exempt actions
 			var action = routeData.Values["action"]?.ToString() ?? string.Empty;
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Web/Resgrid.Web/Filters/RequireActivePlanFilter.cs` around lines 22 - 23, The
exemption list in RequireActivePlanFilter currently whitelists the entire
"subscription" controller which is too broad and bypasses plan enforcement;
remove "subscription" from the controller-level exemptions in the
ExemptControllers/whitelist array inside RequireActivePlanFilter and instead add
explicit route/action-level checks: keep controller-level "account" if needed
but implement granular exemptions by matching controller+action names (e.g.,
only allow specific read-only or webhook actions) in the filter's
OnActionExecuting (or equivalent) logic so only those explicit actions are
skipped while all other SubscriptionController endpoints still enforce an active
plan.

};

private static readonly string[] ExemptActions = new[]
{
"selectregistrationplan",
"getstripesession",
"getstripeupdate",
"stripeprocessing",
"paymentcomplete",
"paymentpending",
"paymentfailed",
"logstriperesponse",
"stripebillinginfoupdatesuccess",
"getpaddlecheckout",
"paddleprocessing"
"topiconsarea",
"getsearchresults"
};

public async Task OnActionExecutionAsync(ActionExecutingContext context, ActionExecutionDelegate next)
Expand Down
Loading