From ad995f5a7c4c7867b1498c5bc99acd6bd5d98fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 5 Mar 2025 22:54:01 +0100 Subject: [PATCH] Check for valid segments before returning recipients We were getting a warning by CodeQL regarding a possible code injection in the `send(segment)` code. In practice, this wasn't a big deal because the `self.recipients` method is only called in the admin section, meaning only admin users could try to take advantage of the code injection, and because this code is rarely called with an invalid segment due to several conditions in the code checking that the user segment is valid, with the only exception being the `generate_csv` action in the `Admin::EmailsDownloadController`. In any case, now we're checking that the segment is valid before calling the `send` method. Since now we're making sure that the segment is valid in the `recipients` method itself, we can remove this check from methods calling it. --- app/lib/user_segments.rb | 2 +- app/models/admin_notification.rb | 2 +- spec/lib/user_segments_spec.rb | 18 ++++++++++++++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/lib/user_segments.rb b/app/lib/user_segments.rb index 55fab74be..79af67ed1 100644 --- a/app/lib/user_segments.rb +++ b/app/lib/user_segments.rb @@ -64,7 +64,7 @@ class UserSegments def self.recipients(segment) if geozones[segment.to_s] all_users.where(geozone: geozones[segment.to_s]) - else + elsif valid_segment?(segment) send(segment) end end diff --git a/app/models/admin_notification.rb b/app/models/admin_notification.rb index b2357c22d..abf5bd12c 100644 --- a/app/models/admin_notification.rb +++ b/app/models/admin_notification.rb @@ -13,7 +13,7 @@ class AdminNotification < ApplicationRecord before_validation :complete_link_url def list_of_recipients - UserSegments.recipients(segment_recipient) if valid_segment_recipient? + UserSegments.recipients(segment_recipient) end def valid_segment_recipient? diff --git a/spec/lib/user_segments_spec.rb b/spec/lib/user_segments_spec.rb index 79b6d176f..90ec6d621 100644 --- a/spec/lib/user_segments_spec.rb +++ b/spec/lib/user_segments_spec.rb @@ -301,6 +301,24 @@ describe UserSegments do end end + describe ".recipients" do + it "does not return any recipients when given an invalid segment" do + create(:user, email: "first@email.com") + + recipients = UserSegments.recipients(:invalid_segment) + + expect(recipients).to be nil + end + + it "does not return any recipients when given a method name as a segment" do + create(:user, email: "first@email.com") + + recipients = UserSegments.recipients(:ancestors) + + expect(recipients).to be nil + end + end + describe ".user_segment_emails" do it "returns list of emails sorted by user creation date" do create(:user, email: "first@email.com", created_at: 1.day.ago)