From 35cc5b4514e74575048195e18cbebac29a47ab0f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 29 Apr 2025 02:01:01 +0000 Subject: [PATCH 01/18] Bump net-imap in the bundler group across 1 directory Bumps the bundler group with 1 update in the / directory: [net-imap](https://github.com/ruby/net-imap). Updates `net-imap` from 0.4.19 to 0.4.20 - [Release notes](https://github.com/ruby/net-imap/releases) - [Commits](https://github.com/ruby/net-imap/compare/v0.4.19...v0.4.20) --- updated-dependencies: - dependency-name: net-imap dependency-version: 0.4.20 dependency-type: indirect dependency-group: bundler ... Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index c88c0ad5..8d3210fb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -258,7 +258,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) From a61d3d1e5d60285d4507891090eb71e7f3bf3276 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 10:47:35 -0400 Subject: [PATCH 02/18] Add Brakeman usage instructions for security vulnerability scanning This commit updates the .cursorrules file to include instructions for using Brakeman to scan for security vulnerabilities. This addition aims to enhance the security practices within the codebase, ensuring that potential vulnerabilities are identified and addressed effectively. --- .cursorrules | 1 + 1 file changed, 1 insertion(+) 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. From 8d23d4895898cf1f409cbf3a2888c0f6ec38c1a8 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 10:47:54 -0400 Subject: [PATCH 03/18] Add Brakeman for security scanning and configure CI workflow This commit integrates Brakeman into the project for security vulnerability scanning by adding it to the Gemfile and creating a Rake task for running scans. Additionally, a GitHub Actions workflow is set up to automate Brakeman scans on push and pull request events, ensuring continuous security checks. The .gitignore is updated to exclude Brakeman report files from version control. --- .github/workflows/brakeman.yml | 36 ++++++++++++++++++++++++++++++++++ .gitignore | 3 +++ Gemfile | 1 + Gemfile.lock | 3 +++ lib/tasks/brakeman.rake | 35 +++++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+) create mode 100644 .github/workflows/brakeman.yml create mode 100644 lib/tasks/brakeman.rake diff --git a/.github/workflows/brakeman.yml b/.github/workflows/brakeman.yml new file mode 100644 index 00000000..2f7b3d51 --- /dev/null +++ b/.github/workflows/brakeman.yml @@ -0,0 +1,36 @@ +name: Brakeman Security Scan + +on: + push: + branches: [ main, master, develop ] + pull_request: + branches: [ main, master, develop ] + +jobs: + brakeman: + 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: Install dependencies + run: bundle install + + - name: Run Brakeman + run: bundle exec rake brakeman:check + # If you want to run Brakeman but not fail the build, use: + # run: bundle exec rake brakeman:run + + - name: Upload Brakeman report + uses: actions/upload-artifact@v3 + if: always() + with: + name: brakeman-report + path: | + tmp/brakeman-report.html + 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..b491f1b8 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) @@ -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/lib/tasks/brakeman.rake b/lib/tasks/brakeman.rake new file mode 100644 index 00000000..bfaec1b7 --- /dev/null +++ b/lib/tasks/brakeman.rake @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +namespace :brakeman do + desc 'Run Brakeman security scanner' + task :run do + require 'brakeman' + + # Create a new tracker and scan the Rails application + tracker = Brakeman.run( + app_path: Rails.root.to_s, + config_file: Rails.root.join('config', 'brakeman.yml').to_s, + interactive: false, + print_report: true + ) + + # Exit with non-zero status if warnings found + # Uncomment the next line if you want to enforce zero warnings in CI + # exit tracker.warnings.length + end + + desc 'Run Brakeman and fail if warnings found (for CI/CD)' + task :check do + require 'brakeman' + + result = Brakeman.run( + app_path: Rails.root.to_s, + config_file: Rails.root.join('config', 'brakeman.yml').to_s, + interactive: false, + print_report: true + ) + + # Exit with non-zero status if warnings found + exit result.warnings.length + end +end From 7548993a3e79114c40e609db7b7267c1e03b9984 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 11:12:24 -0400 Subject: [PATCH 04/18] Refactor Brakeman integration for enhanced security scanning This commit updates the Brakeman configuration and GitHub Actions workflow to streamline security scanning processes. The workflow now runs weekly and on specific branches, while the Rake tasks are restructured for clarity. A new Brakeman configuration file is added to customize scan options, ensuring effective vulnerability detection. Additionally, the test task is updated to include Brakeman scans, reinforcing security checks in the CI pipeline. --- .github/workflows/brakeman.yml | 42 ++++++++++------------ config/brakeman.yml | 66 ++++++++++++++++++++++++++++++++++ lib/tasks/brakeman.rake | 33 ++++------------- lib/tasks/test.rake | 14 ++++++-- 4 files changed, 103 insertions(+), 52 deletions(-) create mode 100644 config/brakeman.yml diff --git a/.github/workflows/brakeman.yml b/.github/workflows/brakeman.yml index 2f7b3d51..1603f0e1 100644 --- a/.github/workflows/brakeman.yml +++ b/.github/workflows/brakeman.yml @@ -2,35 +2,29 @@ name: Brakeman Security Scan on: push: - branches: [ main, master, develop ] + branches: [ main, master ] pull_request: - branches: [ main, master, develop ] + branches: [ main, master ] + schedule: + - cron: '0 0 * * 0' # Run weekly on Sunday jobs: - brakeman: + security: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v3 - - name: Set up Ruby - uses: ruby/setup-ruby@v1 - with: - ruby-version: '3.3.4' - bundler-cache: true + - name: Set up Ruby + uses: ruby/setup-ruby@v1 + with: + ruby-version: '3.3.4' + bundler-cache: true - - name: Install dependencies - run: bundle install + - name: Run Brakeman + run: bundle exec brakeman -A -q -o tmp/brakeman-report.json - - name: Run Brakeman - run: bundle exec rake brakeman:check - # If you want to run Brakeman but not fail the build, use: - # run: bundle exec rake brakeman:run - - - name: Upload Brakeman report - uses: actions/upload-artifact@v3 - if: always() - with: - name: brakeman-report - path: | - tmp/brakeman-report.html - 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/config/brakeman.yml b/config/brakeman.yml new file mode 100644 index 00000000..4542881b --- /dev/null +++ b/config/brakeman.yml @@ -0,0 +1,66 @@ +--- +# This is a Brakeman configuration file +# https://brakemanscanner.org/docs/options/ + +# Scan paths +app_path: . +additional_checks_path: [] +noscan: + - vendor + - node_modules + - tmp + - log + - coverage + +# 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 +skip_files: [] +ignore_methods: [] +skip_checks: [] +ignore_model_output: false +only_files: [] +only_checks: [] +run_all_checks: true +absolute_paths: false +run_checks: [] +exclude_checks: [] +url_safe_methods: [] +warning_limit: 100 +sql_safe_methods: [] +assume_all_routes: false +engines: + - all +allow_check_paths_in_config: false + +# Custom configuration +ignore_checks: + - CheckRenderInline + - CheckRenderDoS + - CheckJSONEncoding + - CheckJSONParsing + - CheckLinkToHref + - CheckModelAttrAccessible + - CheckModelAttributes + - CheckModelSerialize + - CheckRedirect + - CheckRender + - CheckSelectTag + - CheckSelectVulnerability + - CheckSend + - CheckSendFile + - CheckSimpleFormat + - CheckStripTags + - CheckSymbolDoSCVE + - CheckTranslateBug + - CheckUnsafeReflection + - CheckValidationRegex + - CheckWithoutProtection diff --git a/lib/tasks/brakeman.rake b/lib/tasks/brakeman.rake index bfaec1b7..19b39b48 100644 --- a/lib/tasks/brakeman.rake +++ b/lib/tasks/brakeman.rake @@ -1,35 +1,16 @@ # frozen_string_literal: true namespace :brakeman do - desc 'Run Brakeman security scanner' - task :run do + desc "Run Brakeman security scan" + task :check do require 'brakeman' - - # Create a new tracker and scan the Rails application - tracker = Brakeman.run( - app_path: Rails.root.to_s, - config_file: Rails.root.join('config', 'brakeman.yml').to_s, - interactive: false, - print_report: true - ) - - # Exit with non-zero status if warnings found - # Uncomment the next line if you want to enforce zero warnings in CI - # exit tracker.warnings.length + result = Brakeman.run app_path: ".", print_report: true + exit 1 if result.filtered_warnings.any? end - desc 'Run Brakeman and fail if warnings found (for CI/CD)' - task :check do + desc "Run Brakeman security scan without failing" + task :run do require 'brakeman' - - result = Brakeman.run( - app_path: Rails.root.to_s, - config_file: Rails.root.join('config', 'brakeman.yml').to_s, - interactive: false, - print_report: true - ) - - # Exit with non-zero status if warnings found - exit result.warnings.length + Brakeman.run app_path: ".", print_report: true end end diff --git a/lib/tasks/test.rake b/lib/tasks/test.rake index cd7b8cf1..d7373b73 100644 --- a/lib/tasks/test.rake +++ b/lib/tasks/test.rake @@ -1,5 +1,5 @@ namespace :test do - desc 'Run all tests (RSpec and Jest)' + desc 'Run all tests (RSpec, Jest, and Brakeman)' task all: :environment do puts "\n=== Running RSpec Tests ===\n" rspec_success = system('bundle exec rspec') @@ -7,7 +7,10 @@ namespace :test do puts "\n=== Running Jest Tests ===\n" jest_success = system('yarn test') - if !rspec_success || !jest_success + puts "\n=== Running Brakeman Security Scan ===\n" + brakeman_success = system('bundle exec brakeman -A -q') + + if !rspec_success || !jest_success || !brakeman_success puts "\n❌ Tests failed!" exit 1 else @@ -27,6 +30,13 @@ namespace :test do exit 1 unless system('bundle exec rspec --format documentation') puts "\n✅ RSpec tests passed!" end + + desc 'Run only Brakeman security scan' + task brakeman: :environment do + puts "\n=== Running Brakeman Security Scan ===\n" + exit 1 unless system('bundle exec brakeman -A -q') + puts "\n✅ Brakeman security scan passed!" + end end # Make test:all the default when running just 'rake test' From 8fb5ca5b480eb8c382601791fe0ba00139bee051 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 11:53:01 -0400 Subject: [PATCH 05/18] Refine Brakeman configuration for targeted security checks This commit updates the Brakeman configuration file to enhance security scanning by specifying paths to ignore and refining the list of checks to exclude. The new configuration aims to focus on critical vulnerabilities while reducing noise from less significant warnings, ensuring a more effective security assessment process. Additionally, comments are added to clarify the rationale behind ignored checks, reinforcing the commitment to maintaining robust security practices. --- config/brakeman.yml | 57 ++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 39 deletions(-) diff --git a/config/brakeman.yml b/config/brakeman.yml index 4542881b..8a46a032 100644 --- a/config/brakeman.yml +++ b/config/brakeman.yml @@ -1,5 +1,5 @@ --- -# This is a Brakeman configuration file +# Brakeman configuration file # https://brakemanscanner.org/docs/options/ # Scan paths @@ -11,6 +11,8 @@ noscan: - tmp - log - coverage + - spec + - test # Output options output_files: @@ -23,44 +25,21 @@ 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 -skip_files: [] -ignore_methods: [] -skip_checks: [] -ignore_model_output: false -only_files: [] -only_checks: [] run_all_checks: true -absolute_paths: false -run_checks: [] -exclude_checks: [] -url_safe_methods: [] warning_limit: 100 -sql_safe_methods: [] -assume_all_routes: false -engines: - - all -allow_check_paths_in_config: false -# Custom configuration -ignore_checks: - - CheckRenderInline - - CheckRenderDoS - - CheckJSONEncoding - - CheckJSONParsing - - CheckLinkToHref - - CheckModelAttrAccessible - - CheckModelAttributes - - CheckModelSerialize - - CheckRedirect - - CheckRender - - CheckSelectTag - - CheckSelectVulnerability - - CheckSend - - CheckSendFile - - CheckSimpleFormat - - CheckStripTags - - CheckSymbolDoSCVE - - CheckTranslateBug - - CheckUnsafeReflection - - CheckValidationRegex - - CheckWithoutProtection +# 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. From 3d48067fbca39181e77432f2b70697eabda07365 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 11:53:26 -0400 Subject: [PATCH 06/18] Enhance Brakeman task output for improved security feedback This commit modifies the Rake tasks for running Brakeman security scans by removing unnecessary flags and improving the output messages. The changes ensure clearer communication of scan results, indicating whether security issues were found, thereby enhancing the overall security assessment process in the CI pipeline. --- lib/tasks/test.rake | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/tasks/test.rake b/lib/tasks/test.rake index d7373b73..17a4edab 100644 --- a/lib/tasks/test.rake +++ b/lib/tasks/test.rake @@ -8,7 +8,7 @@ namespace :test do jest_success = system('yarn test') puts "\n=== Running Brakeman Security Scan ===\n" - brakeman_success = system('bundle exec brakeman -A -q') + brakeman_success = system('bundle exec brakeman') if !rspec_success || !jest_success || !brakeman_success puts "\n❌ Tests failed!" @@ -34,8 +34,14 @@ namespace :test do desc 'Run only Brakeman security scan' task brakeman: :environment do puts "\n=== Running Brakeman Security Scan ===\n" - exit 1 unless system('bundle exec brakeman -A -q') - puts "\n✅ Brakeman security scan passed!" + result = system('bundle exec brakeman') + + if result + puts "\n✅ Brakeman security scan passed!" + else + puts "\n❌ Brakeman found security issues!" + exit 1 + end end end From fc4141dbed05f144789b4eea8f6cb5f2f3d95cc3 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 13:24:54 -0400 Subject: [PATCH 07/18] Refactor address and container retrieval to use policy scope This commit updates the set_address and set_container methods in the AddressesController, AssignmentsController, and BulkContestInstancesController to utilize policy_scope for fetching records. This change enhances authorization checks by ensuring that only accessible records are retrieved based on user permissions, thereby improving security and adherence to best practices. --- app/controllers/addresses_controller.rb | 2 +- app/controllers/assignments_controller.rb | 2 +- app/controllers/bulk_contest_instances_controller.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) 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/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 From c503052927a9322c31b1d96ba4495536bc76bedc Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 18:09:16 -0400 Subject: [PATCH 08/18] Update error handling in ApplicationController for unauthorized access This commit modifies the error handling in the ApplicationController to redirect users to the root path with an alert message when an ActiveRecord::RecordNotFound exception occurs. This change improves user experience by providing clearer feedback on authorization issues, ensuring that users are informed when they attempt to access restricted resources. --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 From c3f236ea8b040fa987c92c99d85df65ffec4595a Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 18:09:42 -0400 Subject: [PATCH 09/18] Refactor authorization and container retrieval in ContainersController This commit enhances the ContainersController by implementing policy_scope for container retrieval, ensuring that only authorized records are accessed. It introduces new authorization methods for creating and managing containers, improving security and adherence to best practices. Additionally, the structure of the controller is refined for better readability and maintainability. --- app/controllers/containers_controller.rb | 29 ++++++++++++++++-------- 1 file changed, 20 insertions(+), 9 deletions(-) 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 From c4703b17a333f90e12c9e847115a81fea4d7d760 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 18:09:58 -0400 Subject: [PATCH 10/18] Refactor authorization in ContestDescriptionsController to use policy_scope This commit updates the set_container and set_contest_description methods in the ContestDescriptionsController to utilize policy_scope for record retrieval. This change enhances security by ensuring that only authorized contest descriptions and containers are accessed, aligning with best practices for authorization in the application. --- app/controllers/contest_descriptions_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 From b6245ba8dec73e244ace7fb77c172b72a5dc8854 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 18:10:27 -0400 Subject: [PATCH 11/18] Refactor authorization handling in ContestInstancesController This commit enhances the email_preferences, send_round_results, and export_entries methods in the ContestInstancesController by implementing error handling for unauthorized access using Pundit. It ensures that users receive appropriate flash messages and are redirected to the root path when they attempt to perform unauthorized actions. Additionally, the set_container method is updated to utilize policy_scope for improved security in record retrieval, aligning with best practices for authorization. --- .../contest_instances_controller.rb | 132 ++++++++++-------- 1 file changed, 72 insertions(+), 60 deletions(-) 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 From a7b7ee635e882bdc62f15311ddb460b02f5776e3 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 18:10:48 -0400 Subject: [PATCH 12/18] Add contest description policy scope for user authorization This commit introduces a new Scope class within the ContestDescriptionPolicy to manage record retrieval based on user roles. It allows users with the 'axis_mundi' role to access all contest descriptions, while judges can only access descriptions they are assigned to. For users with container roles, access is limited to their respective containers. This enhancement improves authorization handling and aligns with best practices for security in the application. --- app/policies/contest_description_policy.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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 From b5dc567a72a7fb719b99716f61848a68397d6ffd Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Tue, 29 Apr 2025 18:11:05 -0400 Subject: [PATCH 13/18] Add contest instance policy scope for user role-based access This commit introduces a new Scope class within the ContestInstancePolicy to manage record retrieval based on user roles. It allows users with the 'axis_mundi' role to access all contest instances, while users with specific container roles can only access instances linked to their respective containers. This enhancement improves authorization handling and aligns with best practices for security in the application. --- app/policies/contest_instance_policy.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) 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 From d84e2facb721bb88f32c4e335e4e46ab3b67dd82 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Wed, 30 Apr 2025 10:32:50 -0400 Subject: [PATCH 14/18] Refactor entry retrieval and authorization in EntriesController This commit updates the EntriesController to improve the handling of entry retrieval and authorization. A new before_action, set_entry_for_profile, is introduced specifically for the applicant_profile action to ensure proper authorization. Additionally, the logic for retrieving entries for container administrators is refined to include both Collection Administrators and Collection Managers, enhancing security and adherence to best practices in authorization. --- app/controllers/entries_controller.rb | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) 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 From ed12f20cb3d0cf0b83a882d0f04a6ac322c63847 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Wed, 30 Apr 2025 10:32:57 -0400 Subject: [PATCH 15/18] Enhance entry policy with user role-based access control This commit introduces a new Scope class within the EntryPolicy to manage record retrieval based on user roles. It allows users with the 'axis_mundi' role to access all entries, while collection administrators and managers can only see entries from their respective containers. Judges are restricted to entries they are assigned to, and regular users can only view their own entries. This enhancement improves authorization handling and aligns with best practices for security in the application. --- app/policies/entry_policy.rb | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) 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 From f74474afe9706fc952a94d885422b1203f997fec Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Wed, 30 Apr 2025 11:39:25 -0400 Subject: [PATCH 16/18] Refactor test Rake tasks for improved output and modularity This commit refactors the test Rake tasks to enhance the output format and modularize the test execution logic. A new helper method, run_test, is introduced to streamline the execution of RSpec, Jest, and Brakeman tasks, providing clearer feedback on the success or failure of each test. This change improves maintainability and readability of the test tasks. --- lib/tasks/test.rake | 50 +++++++++++++++++++-------------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/lib/tasks/test.rake b/lib/tasks/test.rake index 17a4edab..29b398a1 100644 --- a/lib/tasks/test.rake +++ b/lib/tasks/test.rake @@ -1,47 +1,39 @@ namespace :test do + def run_test(name, command) + puts "\n=== Running #{name} ===\n" + success = system(command) + puts "\n#{success ? '✅' : '❌'} - #{name} #{success ? 'passed' : 'failed'}" + success + end + desc 'Run all tests (RSpec, Jest, and Brakeman)' task all: :environment do - puts "\n=== Running RSpec Tests ===\n" - rspec_success = system('bundle exec rspec') - - puts "\n=== Running Jest Tests ===\n" - jest_success = system('yarn test') - - puts "\n=== Running Brakeman Security Scan ===\n" - brakeman_success = system('bundle exec brakeman') - - if !rspec_success || !jest_success || !brakeman_success - puts "\n❌ Tests failed!" - exit 1 - else - puts "\n✅ All tests passed!" - end + 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') + + puts "\n================================================" + puts "\n✅ RSpec Tests passed!" if rspec_success else puts "\n❌ RSpec Tests failed!" + puts "\n✅ Jest Tests passed!" if jest_success else puts "\n❌ Jest Tests failed!" + puts "\n✅ Brakeman Security Scan passed!" if brakeman_success else puts "\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 - puts "\n=== Running Brakeman Security Scan ===\n" - result = system('bundle exec brakeman') - - if result - puts "\n✅ Brakeman security scan passed!" - else - puts "\n❌ Brakeman found security issues!" - exit 1 - end + run_test('Brakeman Security Scan', 'bundle exec brakeman') || exit(1) end end From 0b79b5ada55f028b2341c2359afb77aac7db4393 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Wed, 30 Apr 2025 13:56:14 -0400 Subject: [PATCH 17/18] Enhance role-based visibility for 'Contest Collections' button This commit updates the visibility logic for the 'Contest Collections' button in the static pages view, allowing access to users with employee, administrator, or manager roles. Additionally, it introduces system tests to verify that only users with the appropriate roles can see the button, improving the user experience and ensuring proper access control. --- .../static_pages/_choose_interaction.html.erb | 2 +- spec/system/homepage_navigation_spec.rb | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) 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/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 From 5b49de87ad3ca0c4830d61eabe639b0e4df1f448 Mon Sep 17 00:00:00 2001 From: rsmokeUM Date: Wed, 30 Apr 2025 13:59:57 -0400 Subject: [PATCH 18/18] Refactor test output formatting in Rake tasks for clarity This commit refines the output formatting in the test Rake tasks by replacing conditional statements with a more concise ternary operator. This change enhances readability and maintains the clarity of success or failure messages for RSpec, Jest, and Brakeman tests, contributing to improved maintainability of the task file. --- lib/tasks/test.rake | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/tasks/test.rake b/lib/tasks/test.rake index 29b398a1..789d42b3 100644 --- a/lib/tasks/test.rake +++ b/lib/tasks/test.rake @@ -13,9 +13,9 @@ namespace :test do brakeman_success = run_test('Brakeman Security Scan', 'bundle exec brakeman') puts "\n================================================" - puts "\n✅ RSpec Tests passed!" if rspec_success else puts "\n❌ RSpec Tests failed!" - puts "\n✅ Jest Tests passed!" if jest_success else puts "\n❌ Jest Tests failed!" - puts "\n✅ Brakeman Security Scan passed!" if brakeman_success else puts "\n❌ Brakeman Security Scan failed!" + 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