diff --git a/.cursorrules b/.cursorrules index 0a1ade25..9f757862 100644 --- a/.cursorrules +++ b/.cursorrules @@ -57,6 +57,7 @@ - Implement proper authentication and authorization (e.g., Devise, Pundit). - Use strong parameters in controllers. - Protect against common web vulnerabilities (XSS, CSRF, SQL injection). + - Use Brakeman to scan for security vulnerabilities. Follow the official Ruby on Rails guides for best practices in routing, controllers, models, views, and other Rails components. diff --git a/.github/workflows/brakeman.yml b/.github/workflows/brakeman.yml new file mode 100644 index 00000000..1603f0e1 --- /dev/null +++ b/.github/workflows/brakeman.yml @@ -0,0 +1,30 @@ +name: Brakeman Security Scan + +on: + push: + branches: [ main, master ] + pull_request: + branches: [ main, master ] + schedule: + - cron: '0 0 * * 0' # Run weekly on Sunday + +jobs: + security: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.3.4' + bundler-cache: true + + - name: Run Brakeman + run: bundle exec brakeman -A -q -o tmp/brakeman-report.json + + - name: Upload Brakeman report + uses: actions/upload-artifact@v3 + with: + name: brakeman-report + path: tmp/brakeman-report.json diff --git a/.gitignore b/.gitignore index 630d3d42..25c14911 100644 --- a/.gitignore +++ b/.gitignore @@ -49,3 +49,6 @@ uploads/ # Ignore working_files directory and all its contents /working_files/ /working_files/**/* + +# Ignore Brakeman reports +/tmp/brakeman-report.* diff --git a/Gemfile b/Gemfile index 2cba4b62..e9cf98b0 100644 --- a/Gemfile +++ b/Gemfile @@ -38,6 +38,7 @@ group :development do gem 'annotate' gem 'better_errors' gem 'binding_of_caller' + gem 'brakeman' gem 'capistrano', '~> 3.17', require: false gem 'capistrano-rails', '~> 1.6', '>= 1.6.1', require: false gem 'capistrano-asdf', require: false diff --git a/Gemfile.lock b/Gemfile.lock index c88c0ad5..de8fa8eb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -93,6 +93,8 @@ GEM debug_inspector (>= 1.2.0) bootsnap (1.18.4) msgpack (~> 1.2) + brakeman (7.0.2) + racc builder (3.3.0) byebug (11.1.3) capistrano (3.19.1) @@ -258,7 +260,7 @@ GEM mysql2 (0.5.6) net-http (0.4.1) uri - net-imap (0.4.19) + net-imap (0.4.20) date net-protocol net-pop (0.1.2) @@ -551,6 +553,7 @@ DEPENDENCIES better_errors binding_of_caller bootsnap + brakeman capistrano (~> 3.17) capistrano-asdf capistrano-rails (~> 1.6, >= 1.6.1) diff --git a/app/controllers/addresses_controller.rb b/app/controllers/addresses_controller.rb index af200654..71650280 100644 --- a/app/controllers/addresses_controller.rb +++ b/app/controllers/addresses_controller.rb @@ -60,7 +60,7 @@ def destroy private def set_address - @address = Address.find(params[:id]) + @address = policy_scope(Address).find(params[:id]) end def address_params diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1432f245..b16ed049 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -43,7 +43,7 @@ def handle_exceptions user_not_authorized(exception) rescue ActiveRecord::RecordNotFound => exception Rails.logger.info('!!!!!!! Handling ActiveRecord::RecordNotFound in ApplicationController') - redirect_to not_found_path + redirect_to root_path, alert: '!!! Not authorized !!!' end # Private method for handling Pundit not authorized errors diff --git a/app/controllers/assignments_controller.rb b/app/controllers/assignments_controller.rb index 7d01c1c1..0d164b0a 100644 --- a/app/controllers/assignments_controller.rb +++ b/app/controllers/assignments_controller.rb @@ -52,7 +52,7 @@ def destroy private def set_container - @container = Container.find(params[:container_id]) + @container = policy_scope(Container).find(params[:container_id]) end def assignment_params diff --git a/app/controllers/bulk_contest_instances_controller.rb b/app/controllers/bulk_contest_instances_controller.rb index 7b4bd037..279ca270 100644 --- a/app/controllers/bulk_contest_instances_controller.rb +++ b/app/controllers/bulk_contest_instances_controller.rb @@ -38,7 +38,7 @@ def create private def set_container - @container = Container.find(params[:container_id]) + @container = policy_scope(Container).find(params[:container_id]) end def authorize_container_access diff --git a/app/controllers/containers_controller.rb b/app/controllers/containers_controller.rb index 43044466..2c7178e3 100644 --- a/app/controllers/containers_controller.rb +++ b/app/controllers/containers_controller.rb @@ -1,9 +1,12 @@ # app/controllers/containers_controller.rb class ContainersController < ApplicationController include ContestDescriptionsHelper + after_action :verify_authorized, except: :lookup_user before_action :set_container, only: %i[show edit update destroy description active_applicants_report] - before_action :authorize_container, only: %i[edit show update destroy description] - before_action :authorize_index, only: [ :index ] + before_action :authorize_container, only: %i[show edit destroy] + before_action :authorize_new_container, only: :new + before_action :authorize_admin_content, only: :admin_content + before_action :authorize_index, only: :index def index @containers = policy_scope(Container) @@ -143,20 +146,28 @@ def active_applicants_report private + def set_container + @container = policy_scope(Container).find(params[:id]) + end + + def container_params + params.require(:container).permit(:name, :description, :notes, :contact_email, :department_id, :visibility_id, + assignments_attributes: %i[id user_id role_id _destroy]) + end + def authorize_container authorize @container end - def set_container - @container = Container.find(params[:id]) + def authorize_new_container + authorize Container, :new? end - def authorize_index - authorize Container + def authorize_admin_content + authorize Container, :admin_content? end - def container_params - params.require(:container).permit(:name, :description, :notes, :contact_email, :department_id, :visibility_id, - assignments_attributes: %i[id user_id role_id _destroy]) + def authorize_index + authorize Container end end diff --git a/app/controllers/contest_descriptions_controller.rb b/app/controllers/contest_descriptions_controller.rb index 65fc4dd9..5abf1f1b 100644 --- a/app/controllers/contest_descriptions_controller.rb +++ b/app/controllers/contest_descriptions_controller.rb @@ -79,11 +79,11 @@ def handle_save(success, action) end def set_container - @container = Container.find(params[:container_id]) + @container = policy_scope(Container).find(params[:container_id]) end def set_contest_description - @contest_description = ContestDescription.find(params[:id]) + @contest_description = policy_scope(ContestDescription).find(params[:id]) end def contest_description_params diff --git a/app/controllers/contest_instances_controller.rb b/app/controllers/contest_instances_controller.rb index f9fbcd2f..dbffb64e 100644 --- a/app/controllers/contest_instances_controller.rb +++ b/app/controllers/contest_instances_controller.rb @@ -79,89 +79,101 @@ def destroy end def email_preferences - set_contest_instance - authorize @contest_instance, :send_round_results? + begin + set_contest_instance + authorize @contest_instance, :send_round_results? - round_id = params[:round_id] - @judging_round = @contest_instance.judging_rounds.find_by(id: round_id) + round_id = params[:round_id] + @judging_round = @contest_instance.judging_rounds.find_by(id: round_id) - if @judging_round.nil? - redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), - alert: 'Judging round not found.' - return - end + if @judging_round.nil? + redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), + alert: 'Judging round not found.' + return + end - if !@judging_round.completed? - redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), - alert: 'Cannot send results for an incomplete judging round.' - nil + if !@judging_round.completed? + redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), + alert: 'Cannot send results for an incomplete judging round.' + nil + end + rescue Pundit::NotAuthorizedError + flash[:alert] = 'Not authorized to perform this action' + redirect_to root_path end end def send_round_results - authorize @contest_instance, :send_round_results? + begin + authorize @contest_instance, :send_round_results? - round_id = params[:round_id] - judging_round = @contest_instance.judging_rounds.find_by(id: round_id) + round_id = params[:round_id] + judging_round = @contest_instance.judging_rounds.find_by(id: round_id) - if judging_round.nil? - redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), - alert: 'Judging round not found.' - return - end + if judging_round.nil? + redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), + alert: 'Judging round not found.' + return + end - if !judging_round.completed? - redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), - alert: 'Cannot send results for an incomplete judging round.' - return - end + if !judging_round.completed? + redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), + alert: 'Cannot send results for an incomplete judging round.' + return + end - # Update email preferences if they were provided - if params[:include_average_ranking].present? || params[:include_advancement_status].present? - judging_round.update( - include_average_ranking: params[:include_average_ranking] == '1', - include_advancement_status: params[:include_advancement_status] == '1' - ) - end + # Update email preferences if they were provided + if params[:include_average_ranking].present? || params[:include_advancement_status].present? + judging_round.update( + include_average_ranking: params[:include_average_ranking] == '1', + include_advancement_status: params[:include_advancement_status] == '1' + ) + end - # Get all entries for this round - entries = judging_round.entries.uniq + # Get all entries for this round + entries = judging_round.entries.uniq - email_count = 0 + email_count = 0 - # Send an email for each entry - entries.each do |entry| - mail = ResultsMailer.entry_evaluation_notification(entry, judging_round) - mail.deliver_later - email_count += 1 - end + # Send an email for each entry + entries.each do |entry| + mail = ResultsMailer.entry_evaluation_notification(entry, judging_round) + mail.deliver_later + email_count += 1 + end - # Increment the emails sent counter for this round - judging_round.increment!(:emails_sent_count) + # Increment the emails sent counter for this round + judging_round.increment!(:emails_sent_count) - redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), - notice: "Successfully queued #{email_count} evaluation result emails for round #{judging_round.round_number}. This is email batch ##{judging_round.emails_sent_count}." + redirect_to container_contest_description_contest_instance_path(@container, @contest_description, @contest_instance), + notice: "Successfully queued #{email_count} evaluation result emails for round #{judging_round.round_number}. This is email batch ##{judging_round.emails_sent_count}." + rescue Pundit::NotAuthorizedError + flash[:alert] = 'Not authorized to perform this action' + redirect_to root_path + end end def export_entries - @contest_instance = ContestInstance.find(params[:id]) - @contest_description = @contest_instance.contest_description - @container = @contest_description.container - - authorize @contest_instance + begin + @contest_instance = @contest_description.contest_instances.find(params[:id]) + authorize @contest_instance, :export_entries? - @entries = @contest_instance.entries.active.includes(:profile, :category) + @entries = @contest_instance.entries.active.includes(:profile, :category) - respond_to do |format| - format.csv do - filename = "#{@contest_description.name.parameterize}-entries_printed-#{Time.zone.today}.csv" + respond_to do |format| + format.csv do + filename = "#{@contest_description.name.parameterize}-entries_printed-#{Time.zone.today}.csv" - csv_data = generate_entries_csv(@entries, @contest_description, @contest_instance) + csv_data = generate_entries_csv(@entries, @contest_description, @contest_instance) - send_data csv_data, - type: 'text/csv; charset=utf-8; header=present', - disposition: "attachment; filename=#{filename}" + send_data csv_data, + type: 'text/csv; charset=utf-8; header=present', + disposition: "attachment; filename=#{filename}" + end end + rescue Pundit::NotAuthorizedError + flash[:alert] = 'Not authorized to access this contest instance' + redirect_to root_path end end @@ -176,7 +188,7 @@ def set_contest_instance end def set_container - @container = Container.find(params[:container_id]) + @container = policy_scope(Container).find(params[:container_id]) end def set_contest_description diff --git a/app/controllers/entries_controller.rb b/app/controllers/entries_controller.rb index decfd68c..7135969f 100644 --- a/app/controllers/entries_controller.rb +++ b/app/controllers/entries_controller.rb @@ -1,6 +1,7 @@ class EntriesController < ApplicationController include AvailableContestsConcern - before_action :set_entry, only: %i[ show edit update destroy soft_delete toggle_disqualified applicant_profile ] + before_action :set_entry, only: %i[ show edit update destroy soft_delete toggle_disqualified ] + before_action :set_entry_for_profile, only: %i[ applicant_profile ] before_action :authorize_entry, only: %i[show edit update destroy] before_action :authorize_index, only: [ :index ] @@ -117,10 +118,10 @@ def applicant_profile if @profile.user == current_user || current_user.axis_mundi? @entries = Entry.active.where(profile: @profile) else - # For container administrators, only show entries from their containers + # For container administrators and managers, only show entries from their containers + admin_role_ids = Role.where(kind: ['Collection Administrator', 'Collection Manager']).pluck(:id) admin_container_ids = current_user.assignments - .where(role: 'container_admin') - .joins(:container) + .where(role_id: admin_role_ids) .pluck(:container_id) @entries = Entry.active @@ -133,6 +134,11 @@ def applicant_profile private # Use callbacks to share common setup or constraints between actions. def set_entry + @entry = policy_scope(Entry).find(params[:id]) + end + + # For applicant_profile, we want to find the entry first, then authorize it + def set_entry_for_profile @entry = Entry.find(params[:id]) end diff --git a/app/policies/contest_description_policy.rb b/app/policies/contest_description_policy.rb index f91282ff..47a3e010 100644 --- a/app/policies/contest_description_policy.rb +++ b/app/policies/contest_description_policy.rb @@ -30,4 +30,21 @@ def destroy? def eligibility_rules? true end + + class Scope < Scope + def resolve + if user&.axis_mundi? + scope.all + elsif user&.judge? + # For judges, scope to contest_descriptions they're assigned to judge + scope.joins(contest_instances: :judging_assignments) + .where(contest_instances: { judging_assignments: { user_id: user.id, active: true } }) + .distinct + else + # For users with container roles, scope to their containers + container_ids = user&.containers&.pluck(:id) || [] + scope.where(container_id: container_ids) + end + end + end end diff --git a/app/policies/contest_instance_policy.rb b/app/policies/contest_instance_policy.rb index 9f68bf5f..e7fff93e 100644 --- a/app/policies/contest_instance_policy.rb +++ b/app/policies/contest_instance_policy.rb @@ -1,4 +1,23 @@ class ContestInstancePolicy < ApplicationPolicy + class Scope < Scope + def resolve + if user&.axis_mundi? + scope.all + else + # Return contest instances for containers where the user has a role + # First get the container IDs where the user has a role + container_ids = user&.containers&.pluck(:id) || [] + + if container_ids.any? + # Then find contest instances linked to those containers through contest descriptions + scope.joins(contest_description: :container).where(contest_descriptions: { container_id: container_ids }) + else + scope.none + end + end + end + end + def index? user&.has_container_role?(record.contest_description.container) || axis_mundi? end diff --git a/app/policies/entry_policy.rb b/app/policies/entry_policy.rb index 85e2cf43..7c4c7704 100644 --- a/app/policies/entry_policy.rb +++ b/app/policies/entry_policy.rb @@ -12,8 +12,41 @@ def toggle_disqualified? end def view_applicant_profile? + container = record.contest_instance.contest_description.container record.profile.user == user || - user&.has_container_role?(record.contest_instance.contest_description.container) || + user&.has_container_role?(container, ['Collection Administrator', 'Collection Manager']) || axis_mundi? end + + class Scope < Scope + def resolve + base_scope = scope.where(deleted: false) # Only show non-deleted entries by default + + if user.nil? + scope.none + elsif user.axis_mundi? + # Axis mundi can see all entries + base_scope + elsif user.administrator? || user.manager? + # Collection administrators and managers can see entries from their containers + admin_role_ids = Role.where(kind: ['Collection Administrator', 'Collection Manager']).pluck(:id) + admin_container_ids = user.assignments + .where(role_id: admin_role_ids) + .pluck(:container_id) + + base_scope.joins(contest_instance: { contest_description: :container }) + .where(containers: { id: admin_container_ids }) + elsif user.judge? + # Judges can only see entries they've been assigned to judge + judged_contest_instance_ids = user.judging_assignments.pluck(:contest_instance_id) + base_scope.where(contest_instance_id: judged_contest_instance_ids) + elsif user.profile.present? + # Regular users can only see their own entries + base_scope.where(profile: user.profile) + else + # User without a profile can't see any entries + scope.none + end + end + end end diff --git a/app/views/static_pages/_choose_interaction.html.erb b/app/views/static_pages/_choose_interaction.html.erb index 25830f9e..db3e0d18 100644 --- a/app/views/static_pages/_choose_interaction.html.erb +++ b/app/views/static_pages/_choose_interaction.html.erb @@ -18,7 +18,7 @@ - <% if current_user.is_employee? %> + <% if current_user.is_employee? || current_user.administrator? || current_user.manager? %>
diff --git a/config/brakeman.yml b/config/brakeman.yml new file mode 100644 index 00000000..8a46a032 --- /dev/null +++ b/config/brakeman.yml @@ -0,0 +1,45 @@ +--- +# Brakeman configuration file +# https://brakemanscanner.org/docs/options/ + +# Scan paths +app_path: . +additional_checks_path: [] +noscan: + - vendor + - node_modules + - tmp + - log + - coverage + - spec + - test + +# Output options +output_files: + - tmp/brakeman-report.html + - tmp/brakeman-report.json +quiet: false +report_progress: true +pager: false +exit_on_warn: true # Exit with non-zero status if warnings are found + +# Scanning options +min_confidence: 2 # Only show high and medium confidence warnings +run_all_checks: true +warning_limit: 100 + +# Ignore specific categories of warnings +# This is much more targeted than the previous configuration +exclude_checks: + # Skip less critical or noisy checks + - UnscopedFind # Often reports unscoped find calls which may be acceptable in admin controllers + - Render # Dynamic render paths with weak confidence + - LinkToHref # Link issues which may be handled through other validations + +# Note: We deliberately don't ignore major security issues such as: +# - SQL Injection +# - Command Injection +# - Cross-Site Scripting (XSS) +# - Cross-Site Request Forgery (CSRF) +# - Mass Assignment +# - etc. diff --git a/lib/tasks/brakeman.rake b/lib/tasks/brakeman.rake new file mode 100644 index 00000000..19b39b48 --- /dev/null +++ b/lib/tasks/brakeman.rake @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +namespace :brakeman do + desc "Run Brakeman security scan" + task :check do + require 'brakeman' + result = Brakeman.run app_path: ".", print_report: true + exit 1 if result.filtered_warnings.any? + end + + desc "Run Brakeman security scan without failing" + task :run do + require 'brakeman' + Brakeman.run app_path: ".", print_report: true + end +end diff --git a/lib/tasks/test.rake b/lib/tasks/test.rake index cd7b8cf1..789d42b3 100644 --- a/lib/tasks/test.rake +++ b/lib/tasks/test.rake @@ -1,31 +1,39 @@ namespace :test do - desc 'Run all tests (RSpec and Jest)' - task all: :environment do - puts "\n=== Running RSpec Tests ===\n" - rspec_success = system('bundle exec rspec') + def run_test(name, command) + puts "\n=== Running #{name} ===\n" + success = system(command) + puts "\n#{success ? '✅' : '❌'} - #{name} #{success ? 'passed' : 'failed'}" + success + end - puts "\n=== Running Jest Tests ===\n" - jest_success = system('yarn test') + desc 'Run all tests (RSpec, Jest, and Brakeman)' + task all: :environment do + rspec_success = run_test('RSpec Tests', 'bundle exec rspec') + jest_success = run_test('Jest Tests', 'yarn test') + brakeman_success = run_test('Brakeman Security Scan', 'bundle exec brakeman') - if !rspec_success || !jest_success - puts "\n❌ Tests failed!" - exit 1 - else - puts "\n✅ All tests passed!" - end + puts "\n================================================" + puts rspec_success ? "\n✅ RSpec Tests passed!" : "\n❌ RSpec Tests failed!" + puts jest_success ? "\n✅ Jest Tests passed!" : "\n❌ Jest Tests failed!" + puts brakeman_success ? "\n✅ Brakeman Security Scan passed!" : "\n❌ Brakeman Security Scan failed!" + puts "\n================================================" + puts "\n❌ Tests failed!" unless rspec_success && jest_success && brakeman_success + exit 1 unless rspec_success && jest_success && brakeman_success end desc 'Run only JavaScript tests' task js: :environment do - puts "\n=== Running Jest Tests ===\n" - exit 1 unless system('yarn test') + run_test('Jest Tests', 'yarn test') || exit(1) end desc 'Run only RSpec tests' task rspec: :environment do - puts "\n=== Running RSpec Tests ===\n" - exit 1 unless system('bundle exec rspec --format documentation') - puts "\n✅ RSpec tests passed!" + run_test('RSpec Tests', 'bundle exec rspec --format documentation') || exit(1) + end + + desc 'Run only Brakeman security scan' + task brakeman: :environment do + run_test('Brakeman Security Scan', 'bundle exec brakeman') || exit(1) end end diff --git a/spec/system/homepage_navigation_spec.rb b/spec/system/homepage_navigation_spec.rb index 6e0bb5e1..a50c3916 100644 --- a/spec/system/homepage_navigation_spec.rb +++ b/spec/system/homepage_navigation_spec.rb @@ -35,4 +35,35 @@ expect(page).to have_css('[data-interaction-target="content"]') end end + + context 'when user has specific roles' do + let(:regular_user) { create(:user) } + let(:employee) { create(:user, :employee) } + let(:administrator) { create(:user, :with_collection_admin_role) } + let(:manager) { create(:user, :with_collection_manager_role) } + + it 'does not show containers_path button to regular users' do + sign_in regular_user + visit root_path + expect(page).not_to have_link('Contest Collections') + end + + it 'shows containers_path button to employees' do + sign_in employee + visit root_path + expect(page).to have_link('Contest Collections') + end + + it 'shows containers_path button to administrators' do + sign_in administrator + visit root_path + expect(page).to have_link('Contest Collections') + end + + it 'shows containers_path button to managers' do + sign_in manager + visit root_path + expect(page).to have_link('Contest Collections') + end + end end