From 9fdf55d9c1c3f12d1aca2c25f5b60e72ae33f20a Mon Sep 17 00:00:00 2001
From: Justin Coyne <jcoyne@justincoyne.com>
Date: Tue, 9 Jul 2024 10:06:25 -0500
Subject: [PATCH] Validate user input to prevent GraphQL injection attack

---
 app/controllers/patron_requests_controller.rb |  3 +++
 config/environments/test.rb                   |  2 +-
 spec/requests/create_request_spec.rb          | 17 +++++++++++++++++
 spec/requests/sidekiq_requests_spec.rb        |  8 ++++----
 4 files changed, 25 insertions(+), 5 deletions(-)
 create mode 100644 spec/requests/create_request_spec.rb

diff --git a/app/controllers/patron_requests_controller.rb b/app/controllers/patron_requests_controller.rb
index ded9198d0..2be5b5129 100644
--- a/app/controllers/patron_requests_controller.rb
+++ b/app/controllers/patron_requests_controller.rb
@@ -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 /\A(a|L|in)?\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
diff --git a/config/environments/test.rb b/config/environments/test.rb
index 3491faf35..e8fe8c61d 100644
--- a/config/environments/test.rb
+++ b/config/environments/test.rb
@@ -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
diff --git a/spec/requests/create_request_spec.rb b/spec/requests/create_request_spec.rb
new file mode 100644
index 000000000..b18fd25be
--- /dev/null
+++ b/spec/requests/create_request_spec.rb
@@ -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 'prevents graphql injection' do
+    expect(response).to have_http_status :bad_request
+  end
+end
diff --git a/spec/requests/sidekiq_requests_spec.rb b/spec/requests/sidekiq_requests_spec.rb
index f65859ffc..67a3d1e83 100644
--- a/spec/requests/sidekiq_requests_spec.rb
+++ b/spec/requests/sidekiq_requests_spec.rb
@@ -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