Skip to content

Commit

Permalink
fixed remote-code-execution vulnerability, added tests for new code
Browse files Browse the repository at this point in the history
  • Loading branch information
Janell-Huyck committed Sep 7, 2023
1 parent 6df60d8 commit 5e44b17
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 13 deletions.
3 changes: 2 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ group :development, :test do
end

group :development do
gem 'brakeman', '~> 6.0'
gem 'capistrano', '~> 3.17.1', require: false
gem 'capistrano-bundler', '~> 1.6', require: false
gem 'capistrano-rails', '~> 1.4', require: false
Expand All @@ -73,10 +74,10 @@ group :development do
gem 'capistrano-rvm', require: false
# Access an interactive console on exception pages or by calling 'console' anywhere in the code.
gem 'listen', '>= 3.0.5', '< 3.2'
gem 'web-console', '>= 3.3.0'
# Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring
gem 'spring'
gem 'spring-watcher-listen', '~> 2.0.0'
gem 'web-console', '>= 3.3.0'
end

group :test do
Expand Down
16 changes: 9 additions & 7 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ GEM
airbrussh (1.4.2)
sshkit (>= 1.6.1, != 1.7.0)
ast (2.4.2)
autoprefixer-rails (10.4.13.0)
autoprefixer-rails (10.4.15.0)
execjs (~> 2)
base64 (0.1.1)
bcrypt_pbkdf (1.1.0)
Expand All @@ -76,6 +76,7 @@ GEM
autoprefixer-rails (>= 9.1.0)
popper_js (>= 1.14.3, < 2)
sassc-rails (>= 2.0.0)
brakeman (6.0.1)
builder (3.2.4)
byebug (11.1.3)
capistrano (3.17.3)
Expand Down Expand Up @@ -139,8 +140,8 @@ GEM
factory_bot (~> 6.2.0)
railties (>= 5.0.0)
ffi (1.15.5)
globalid (1.1.0)
activesupport (>= 5.0)
globalid (1.2.1)
activesupport (>= 6.1)
htmlentities (4.3.4)
i18n (1.14.1)
concurrent-ruby (~> 1.0)
Expand Down Expand Up @@ -168,7 +169,7 @@ GEM
matrix (0.4.2)
method_source (1.0.0)
mini_mime (1.1.5)
minitest (5.19.0)
minitest (5.20.0)
msgpack (1.7.2)
mysql2 (0.5.5)
net-imap (0.3.7)
Expand Down Expand Up @@ -196,7 +197,7 @@ GEM
puma (3.12.6)
racc (1.7.1)
rack (2.2.8)
rack-proxy (0.7.6)
rack-proxy (0.7.7)
rack
rack-test (2.1.0)
rack (>= 1.3)
Expand Down Expand Up @@ -294,7 +295,7 @@ GEM
sprockets (> 3.0)
sprockets-rails
tilt
selenium-webdriver (4.11.0)
selenium-webdriver (4.12.0)
rexml (~> 3.2, >= 3.2.5)
rubyzip (>= 1.2.2, < 3.0)
websocket (~> 1.0)
Expand Down Expand Up @@ -339,7 +340,7 @@ GEM
uglifier (4.2.0)
execjs (>= 0.3.0, < 3)
unicode-display_width (2.4.2)
web-console (4.2.0)
web-console (4.2.1)
actionview (>= 6.0.0)
activemodel (>= 6.0.0)
bindex (>= 0.4.0)
Expand All @@ -364,6 +365,7 @@ DEPENDENCIES
bcrypt_pbkdf
bootsnap (>= 1.1.0)
bootstrap (~> 4.4.1)
brakeman (~> 6.0)
byebug
capistrano (~> 3.17.1)
capistrano-bundler (~> 1.6)
Expand Down
37 changes: 32 additions & 5 deletions app/controllers/admin_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,24 @@

class AdminController < ApplicationController
skip_before_action :check_date

ALLOWED_CONTROLLERS_TO_MODELS = {
'artworks' => Artwork,
'books' => Book,
'book_chapters' => BookChapter,
'digital_projects' => DigitalProject,
'editings' => Editing,
'films' => Film,
'journal_articles' => JournalArticle,
'musical_scores' => MusicalScore,
'photographies' => Photography,
'physical_media' => PhysicalMedium,
'plays' => Play,
'public_performances' => PublicPerformance,
'submitters' => Submitter,
'other_publications' => OtherPublication
}.freeze

def login; end

def validate
Expand All @@ -14,11 +32,16 @@ def validate
end

def csv
if session[:admin] && params[:id].nil?
instance_variable_set('@instance_variable', Object.const_get(params[:controller_name].classify).all)
respond_to do |format|
format.html { redirect_to publications_path }
format.csv { send_data @instance_variable.to_csv }
if session[:admin] && params[:id].nil? && allowed_model
begin
@instance_variable = allowed_model.all
respond_to do |format|
format.html { redirect_to publications_path }
format.csv { send_data @instance_variable.to_csv }
end
rescue StandardError => e
logger.error "CSV generation failed: #{e}"
redirect_to publications_path, notice: 'Something went wrong while generating the CSV.'
end
else
redirect_to publications_path
Expand Down Expand Up @@ -47,4 +70,8 @@ def toggle_links
def check_credentials(username, password)
username == ENV['ADMIN_USERNAME'] && password == ENV['ADMIN_PASSWORD']
end

def allowed_model
ALLOWED_CONTROLLERS_TO_MODELS[params[:controller_name]]
end
end
37 changes: 37 additions & 0 deletions spec/controllers/admin_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,41 @@
expect(response).to redirect_to('/publications')
end
end

describe 'ALLOWED_CONTROLLERS_TO_MODELS' do
let(:excluded_model_names) { ['ActiveRecord::InternalMetadata', 'ActiveRecord::SchemaMigration', 'ApplicationRecord', 'College', '...eges', 'HABTM_Colleges', 'HABTM_Books', 'HABTM_OtherPublications'] }

def load_all_models
Dir[Rails.root.join('app/models/**/*.rb')].each { |file| require file }
end

def fetch_all_model_names
ActiveRecord::Base.descendants.map(&:name).sort
end

def fetch_allowed_model_names
AdminController::ALLOWED_CONTROLLERS_TO_MODELS.values.map(&:to_s).sort
end

before(:all) do
load_all_models
end

it 'contains all the relevant models in app/models' do
all_model_names = fetch_all_model_names
allowed_model_names = fetch_allowed_model_names
relevant_model_names = (all_model_names - excluded_model_names).sort
expect(allowed_model_names).to match_array(relevant_model_names)
end

it 'each model should have a to_csv class method' do
missing_methods = []

AdminController::ALLOWED_CONTROLLERS_TO_MODELS.each_value do |model|
missing_methods << "#{model.name} is missing a to_csv class method" unless model.respond_to?(:to_csv)
end

expect(missing_methods).to be_empty, missing_methods.join(', ')
end
end
end

0 comments on commit 5e44b17

Please sign in to comment.