chore: adjust unscoped finds and coverage

code quality

Author: Bulletdev

app/controllers/api/v1/scouting/players_controller.rb

@@ -28,17 +28,20 @@ def index
       targets = targets.where('summoner_name ILIKE ? OR real_name ILIKE ?', search_term, search_term)
     end
 
-    # Sorting
-    sort_by = params[:sort_by] || 'created_at'
-    sort_order = params[:sort_order] || 'desc'
-
-    # Map 'rank' to actual column names
-    if sort_by == 'rank'
-      targets = targets.order("current_lp #{sort_order} NULLS LAST")
-    elsif sort_by == 'winrate'
-      targets = targets.order("performance_trend #{sort_order} NULLS LAST")
+    # Whitelist for sort parameters to prevent SQL injection
+    allowed_sort_fields = %w[created_at updated_at summoner_name current_tier priority status role region age]
+    allowed_sort_orders = %w[asc desc]
+
+    sort_by = allowed_sort_fields.include?(params[:sort_by]) ? params[:sort_by] : 'created_at'
+    sort_order = allowed_sort_orders.include?(params[:sort_order]&.downcase) ? params[:sort_order].downcase : 'desc'
+
+    # Handle special sort fields
+    if params[:sort_by] == 'rank'
+      targets = targets.order(Arel.sql("current_lp #{sort_order} NULLS LAST"))
+    elsif params[:sort_by] == 'winrate'
+      targets = targets.order(Arel.sql("performance_trend #{sort_order} NULLS LAST"))
     else
-      targets = targets.order("#{sort_by} #{sort_order}")
+      targets = targets.order(sort_by => sort_order)
     end
 
     # Pagination

app/modules/authentication/controllers/auth_controller.rb

@@ -64,7 +64,7 @@ def register
         end
       rescue ActiveRecord::RecordInvalid => e
         render_validation_errors(e)
-      rescue => e
+      rescue StandardError => _e
         render_error(message: 'Registration failed', code: 'REGISTRATION_ERROR')
       end
 

app/modules/players/services/riot_sync_service.rb

@@ -33,14 +33,38 @@ class RiotSyncService
       require 'net/http'
       require 'json'
 
+      # Whitelist of valid Riot API regions to prevent host injection
+      VALID_REGIONS = %w[
+        br1 eun1 euw1 jp1 kr la1 la2 na1 oc1 tr1 ru ph2 sg2 th2 tw2 vn2
+      ].freeze
+
       attr_reader :player, :region, :api_key
 
       def initialize(player, region: nil, api_key: nil)
         @player = player
-        @region = region || player.region.presence&.downcase || 'br1'
+        @region = sanitize_region(region || player&.region || 'br1')
         @api_key = api_key || ENV['RIOT_API_KEY']
       end
 
+      private
+
+      # Sanitizes and validates region to prevent host injection
+      #
+      # @param region [String] Region code to sanitize
+      # @return [String] Sanitized region code
+      # @raise [ArgumentError] if region is invalid
+      def sanitize_region(region)
+        normalized = region.to_s.downcase.strip
+
+        unless VALID_REGIONS.include?(normalized)
+          raise ArgumentError, "Invalid region: #{region}. Must be one of: #{VALID_REGIONS.join(', ')}"
+        end
+
+        normalized
+      end
+
+      public
+
       def self.import(summoner_name:, role:, region:, organization:, api_key: nil)
         new(nil, region: region, api_key: api_key)
           .import_player(summoner_name, role, organization)

app/modules/schedules/controllers/schedules_controller.rb

@@ -27,8 +27,10 @@ def index
       schedules = schedules.where(start_time: Time.current.beginning_of_week..Time.current.end_of_week)
       end
 
-      sort_order = params[:sort_order] || 'asc'
-      schedules = schedules.order("start_time #{sort_order}")
+      # Whitelist for sort parameters to prevent SQL injection
+      allowed_sort_orders = %w[asc desc]
+      sort_order = allowed_sort_orders.include?(params[:sort_order]&.downcase) ? params[:sort_order].downcase : 'asc'
+      schedules = schedules.order(start_time: sort_order)
 
       result = paginate(schedules)
 

app/modules/scouting/controllers/players_controller.rb

@@ -24,16 +24,26 @@ def index
       targets = targets.where('summoner_name ILIKE ? OR real_name ILIKE ?', search_term, search_term)
     end
 
-    sort_by = params[:sort_by] || 'created_at'
-    sort_order = params[:sort_order] || 'desc'
-
-    # Map 'rank' to actual column names
-    if sort_by == 'rank'
-      targets = targets.order("current_lp #{sort_order} NULLS LAST")
-    elsif sort_by == 'winrate'
-      targets = targets.order("performance_trend #{sort_order} NULLS LAST")
+    # Whitelist for sort parameters to prevent SQL injection
+    allowed_sort_fields = %w[created_at updated_at summoner_name current_tier priority status role region age]
+    allowed_sort_orders = %w[asc desc]
+
+    sort_by = allowed_sort_fields.include?(params[:sort_by]) ? params[:sort_by] : 'created_at'
+    sort_order = allowed_sort_orders.include?(params[:sort_order]&.downcase) ? params[:sort_order].downcase : 'desc'
+
+    # Handle special sort fields with NULLS LAST
+    if params[:sort_by] == 'rank'
+      # Use Arel for safe SQL generation with NULLS LAST
+      column = ScoutingTarget.arel_table[:current_lp]
+      order_clause = sort_order == 'asc' ? column.asc.nulls_last : column.desc.nulls_last
+      targets = targets.order(order_clause)
+    elsif params[:sort_by] == 'winrate'
+      # Use Arel for safe SQL generation with NULLS LAST
+      column = ScoutingTarget.arel_table[:performance_trend]
+      order_clause = sort_order == 'asc' ? column.asc.nulls_last : column.desc.nulls_last
+      targets = targets.order(order_clause)
     else
-      targets = targets.order("#{sort_by} #{sort_order}")
+      targets = targets.order(sort_by => sort_order)
     end
 
     result = paginate(targets)

app/modules/scrims/controllers/opponent_teams_controller.rb

@@ -1,8 +1,15 @@
 module Scrims
+  # OpponentTeams Controller
+  #
+  # Manages opponent team records which are shared across organizations.
+  # Security note: Update and delete operations are restricted to organizations
+  # that have used this opponent team in scrims.
+  #
   class OpponentTeamsController < ApplicationController
     include TierAuthorization
 
     before_action :set_opponent_team, only: [:show, :update, :destroy, :scrim_history]
+    before_action :verify_team_usage!, only: [:update, :destroy]
 
     # GET /api/v1/scrims/opponent_teams
     def index
@@ -75,18 +82,43 @@ def update
 
     # DELETE /api/v1/scrims/opponent_teams/:id
     def destroy
+      # Check if team has scrims from other organizations before deleting
+      other_org_scrims = @opponent_team.scrims.where.not(organization_id: current_organization.id).exists?
+
+      if other_org_scrims
+        return render json: {
+          error: 'Cannot delete opponent team that is used by other organizations'
+        }, status: :unprocessable_entity
+      end
+
       @opponent_team.destroy
       head :no_content
     end
 
     private
 
+    # Finds opponent team by ID
+    # Security Note: OpponentTeam is a shared resource across organizations.
+    # Deletion is restricted to teams without cross-org usage (see destroy action).
+    # Consider adding organization_id in future for proper multi-tenancy.
     def set_opponent_team
       @opponent_team = OpponentTeam.find(params[:id])
     rescue ActiveRecord::RecordNotFound
       render json: { error: 'Opponent team not found' }, status: :not_found
     end
 
+    # Verifies that current organization has used this opponent team
+    # Prevents organizations from modifying/deleting teams they haven't interacted with
+    def verify_team_usage!
+      has_scrims = current_organization.scrims.exists?(opponent_team_id: @opponent_team.id)
+
+      unless has_scrims
+        render json: {
+          error: 'You cannot modify this opponent team. Your organization has not played against them.'
+        }, status: :forbidden
+      end
+    end
+
     def opponent_team_params
       params.require(:opponent_team).permit(
         :name,

app/modules/team_goals/controllers/team_goals_controller.rb

@@ -16,9 +16,13 @@ def index
 
     goals = goals.where(assigned_to_id: params[:assigned_to_id]) if params[:assigned_to_id].present?
 
-    sort_by = params[:sort_by] || 'created_at'
-    sort_order = params[:sort_order] || 'desc'
-    goals = goals.order("#{sort_by} #{sort_order}")
+    # Whitelist for sort parameters to prevent SQL injection
+    allowed_sort_fields = %w[created_at updated_at title status category start_date end_date progress]
+    allowed_sort_orders = %w[asc desc]
+
+    sort_by = allowed_sort_fields.include?(params[:sort_by]) ? params[:sort_by] : 'created_at'
+    sort_order = allowed_sort_orders.include?(params[:sort_order]&.downcase) ? params[:sort_order].downcase : 'desc'
+    goals = goals.order(sort_by => sort_order)
 
     result = paginate(goals)
 

app/modules/vod_reviews/controllers/vod_reviews_controller.rb

@@ -20,9 +20,13 @@ def index
       vod_reviews = vod_reviews.where('title ILIKE ?', search_term)
       end
 
-      sort_by = params[:sort_by] || 'created_at'
-      sort_order = params[:sort_order] || 'desc'
-      vod_reviews = vod_reviews.order("#{sort_by} #{sort_order}")
+      # Whitelist for sort parameters to prevent SQL injection
+      allowed_sort_fields = %w[created_at updated_at title status reviewed_at]
+      allowed_sort_orders = %w[asc desc]
+
+      sort_by = allowed_sort_fields.include?(params[:sort_by]) ? params[:sort_by] : 'created_at'
+      sort_order = allowed_sort_orders.include?(params[:sort_order]&.downcase) ? params[:sort_order].downcase : 'desc'
+      vod_reviews = vod_reviews.order(sort_by => sort_order)
 
       result = paginate(vod_reviews)
 

app/modules/vod_reviews/controllers/vod_timestamps_controller.rb

@@ -96,9 +96,17 @@ def set_vod_review
   end
 
   def set_vod_timestamp
-    @timestamp = VodTimestamp.joins(:vod_review)
-                             .where(vod_reviews: { organization: current_organization })
-                             .find(params[:id])
+    # Scope to organization through vod_review to prevent cross-org access
+    @timestamp = VodTimestamp
+                   .joins(:vod_review)
+                   .where(vod_reviews: { organization_id: current_organization.id })
+                   .find_by!(id: params[:id])
+  rescue ActiveRecord::RecordNotFound
+    render_error(
+      message: 'Timestamp not found',
+      code: 'NOT_FOUND',
+      status: :not_found
+    )
   end
 
   def vod_timestamp_params

spec/models/vod_timestamp_spec.rb

@@ -45,9 +45,9 @@
 
     describe '.chronological' do
       it 'orders by timestamp_seconds' do
-        ts1 = create(:vod_timestamp, vod_review: vod_review, timestamp_seconds: 100)
-        ts2 = create(:vod_timestamp, vod_review: vod_review, timestamp_seconds: 50)
-        ts3 = create(:vod_timestamp, vod_review: vod_review, timestamp_seconds: 200)
+        create(:vod_timestamp, vod_review: vod_review, timestamp_seconds: 100)
+        create(:vod_timestamp, vod_review: vod_review, timestamp_seconds: 50)
+        create(:vod_timestamp, vod_review: vod_review, timestamp_seconds: 200)
 
         expect(VodTimestamp.chronological.pluck(:timestamp_seconds)).to eq([50, 100, 200])
       end