Skip to content

Commit

Permalink
Validate user input to prevent GraphQL injection attack
Browse files Browse the repository at this point in the history
  • Loading branch information
jcoyne committed Jul 9, 2024
1 parent 9c793ac commit d8fda20
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 5 deletions.
3 changes: 3 additions & 0 deletions app/controllers/patron_requests_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ def current_request

def assign_new_attributes
@patron_request.assign_attributes(**new_params)
# This validation prevents an injection attack in GraphQL. Ideally, this would be a model validation,
# but there are other validations that require first running a GraphQL query.
raise ActionController::BadRequest unless /\Aa?\d+\z/.match?(new_params[:instance_hrid])
end

# SSO or library-id users don't need to re-login, but name/email users always need to provide their information
Expand Down
2 changes: 1 addition & 1 deletion config/environments/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
config.cache_store = :null_store

# Render exception templates for rescuable exceptions and raise for other exceptions.
config.action_dispatch.show_exceptions = :none
config.action_dispatch.show_exceptions = :all

# Disable request forgery protection in test environment.
config.action_controller.allow_forgery_protection = false
Expand Down
17 changes: 17 additions & 0 deletions spec/requests/create_request_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe 'Create a new request' do
let(:url) { '/requests/new' }
let(:user) { nil }

before do
login_as(user, run_callbacks: false)
get '/patron_requests/new?instance_hrid=%22p3nr6p&origin_location_code=SPEC-SAL3-U-ARCHIVES'
end

it 'is successful' do
expect(response).to have_http_status :bad_request
end
end
8 changes: 4 additions & 4 deletions spec/requests/sidekiq_requests_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,24 @@
require 'rails_helper'

RSpec.describe 'Sidekiq requests' do
let(:url) { '/sidekiq' }
let(:user) { nil }

before do
login_as(user, run_callbacks: false)
get '/sidekiq'
end

context 'with superadmin privileges' do
let(:user) { instance_double(CurrentUser, user_object: instance_double(User, super_admin?: true)) }

it 'is successful' do
expect { get(url) }.to raise_error(RedisClient::CannotConnectError)
expect(response).to have_http_status :internal_server_error # due to sidekiq not running in test
end
end

context 'without superadmin privileges' do
it 'raises an access denied error' do
expect { get(url) }.to raise_error(ActionController::RoutingError)
it 'shows the not found page' do
expect(response).to have_http_status :not_found
end
end
end

0 comments on commit d8fda20

Please sign in to comment.