From a9884733f661d814c15063ddf1fab822201a38d9 Mon Sep 17 00:00:00 2001 From: Andrew Cain Date: Fri, 4 Apr 2025 21:07:12 +1100 Subject: [PATCH 1/3] test: fix test attempt model checks --- test/api/test_attempts_test.rb | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/test/api/test_attempts_test.rb b/test/api/test_attempts_test.rb index 5d5b4422f9..be0c02ae5e 100644 --- a/test/api/test_attempts_test.rb +++ b/test/api/test_attempts_test.rb @@ -75,10 +75,13 @@ def test_get_task_attempts add_auth_header_for(user: user) + response_keys = %w[id task_id terminated completion_status success_status score_scaled cmi_datamodel] + # When attempts exists get "api/projects/#{project.id}/task_def_id/#{td.id}/test_attempts" assert_equal 200, last_response.status - assert_json_equal last_response_body, [attempt] + assert_equal 1, last_response_body.size + assert_json_matches_model attempt, last_response_body.first, response_keys user1 = FactoryBot.create(:user, :student) @@ -137,17 +140,19 @@ def test_get_latest add_auth_header_for(user: user) + response_keys = %w[id task_id terminated completion_status success_status score_scaled cmi_datamodel] + # When attempts exist get "api/projects/#{project.id}/task_def_id/#{td.id}/test_attempts/latest" assert_equal 200, last_response.status - assert_json_equal last_response_body, attempt1 + assert_json_matches_model attempt1, last_response_body, response_keys add_auth_header_for(user: user) # Get completed latest get "api/projects/#{project.id}/task_def_id/#{td.id}/test_attempts/latest?completed=true" assert_equal 200, last_response.status - assert_json_equal last_response_body, attempt + assert_json_matches_model attempt, last_response_body, response_keys user1 = FactoryBot.create(:user, :student) @@ -233,7 +238,8 @@ def test_review_attempt attempt.review attempt.save! - assert_json_equal last_response_body, attempt + response_keys = %w[id task_id terminated completion_status success_status score_scaled cmi_datamodel] + assert_json_matches_model attempt, last_response_body, response_keys tutor = project.tutor_for(td) @@ -242,7 +248,7 @@ def test_review_attempt # When user is tutor get "api/test_attempts/#{attempt.id}/review" assert_equal 200, last_response.status - assert_json_equal last_response_body, attempt + assert_json_matches_model attempt, last_response_body, response_keys user1 = FactoryBot.create(:user, :student) From 27d781d5b4ba732f259539e8033a889d274107f3 Mon Sep 17 00:00:00 2001 From: theiris6 <143162156+theiris6@users.noreply.github.com> Date: Mon, 12 May 2025 17:58:35 +1000 Subject: [PATCH 2/3] Fix broken access control vulnerability in settings API - Modified app/api/settings_api.rb to require authentication for sensitive endpoints - Created a new public endpoint for non-sensitive settings - Added authentication requirement to privacy settings endpoint - Added SettingsApi to the authentication helpers list in app/api/api_root.rb - Prevents unauthorized access to system configuration --- app/api/api_root.rb | 1 + app/api/settings_api.rb | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/app/api/api_root.rb b/app/api/api_root.rb index b1e9ebce9b..80e1fdc8d5 100644 --- a/app/api/api_root.rb +++ b/app/api/api_root.rb @@ -114,6 +114,7 @@ class ApiRoot < Grape::API AuthenticationHelpers.add_auth_to LearningAlignmentApi AuthenticationHelpers.add_auth_to ProjectsApi AuthenticationHelpers.add_auth_to StudentsApi + AuthenticationHelpers.add_auth_to SettingsApi AuthenticationHelpers.add_auth_to Submission::PortfolioApi AuthenticationHelpers.add_auth_to Submission::PortfolioEvidenceApi AuthenticationHelpers.add_auth_to Submission::BatchTaskApi diff --git a/app/api/settings_api.rb b/app/api/settings_api.rb index 49968392ca..500568b544 100644 --- a/app/api/settings_api.rb +++ b/app/api/settings_api.rb @@ -1,11 +1,15 @@ require 'grape' class SettingsApi < Grape::API + helpers AuthenticationHelpers + helpers AuthorisationHelpers # # Returns the current auth method # desc 'Return configurable details for the Doubtfire front end' get '/settings' do + # Require authentication for the main settings endpoint + authenticated? response = { externalName: Doubtfire::Application.config.institution[:product_name], hasLogo: Doubtfire::Application.config.institution[:has_logo], @@ -19,6 +23,19 @@ class SettingsApi < Grape::API present response, with: Grape::Presenters::Presenter end + # + # Public endpoint - safe to access without authentication + # + desc 'Return public application settings without authentication' + get '/settings/public' do + response = { + externalName: Doubtfire::Application.config.institution[:product_name] + # Include only non-sensitive settings here + } + + present response, with: Grape::Presenters::Presenter + end + desc 'Return privacy policy details' get '/settings/privacy' do response = { From e5c20cc5d33c01987e0101eb744c1cf3cd18fad1 Mon Sep 17 00:00:00 2001 From: theiris6 <143162156+theiris6@users.noreply.github.com> Date: Wed, 21 May 2025 00:08:38 +1000 Subject: [PATCH 3/3] Added error handling for settings endpoint Added authentication check for /settings/privacy endpoint --- app/api/settings_api.rb | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/app/api/settings_api.rb b/app/api/settings_api.rb index 500568b544..f538c2414e 100644 --- a/app/api/settings_api.rb +++ b/app/api/settings_api.rb @@ -10,17 +10,23 @@ class SettingsApi < Grape::API get '/settings' do # Require authentication for the main settings endpoint authenticated? - response = { - externalName: Doubtfire::Application.config.institution[:product_name], - hasLogo: Doubtfire::Application.config.institution[:has_logo], - logoUrl: Doubtfire::Application.config.institution[:logo_url], - logoLinkUrl: Doubtfire::Application.config.institution[:logo_link_url], - overseerEnabled: Doubtfire::Application.config.overseer_enabled, - tiiEnabled: TurnItIn.enabled?, - d2lEnabled: D2lIntegration.enabled? - } - present response, with: Grape::Presenters::Presenter + begin + response = { + externalName: Doubtfire::Application.config.institution[:product_name], + hasLogo: Doubtfire::Application.config.institution[:has_logo], + logoUrl: Doubtfire::Application.config.institution[:logo_url], + logoLinkUrl: Doubtfire::Application.config.institution[:logo_link_url], + overseerEnabled: Doubtfire::Application.config.overseer_enabled, + tiiEnabled: TurnItIn.enabled?, + d2lEnabled: D2lIntegration.enabled? + } + + present response, with: Grape::Presenters::Presenter + rescue StandardError => e + logger.error "Error fetching settings: #{e.message}" + error!({ error: "Could not retrieve settings due to an internal error" }, 500) + end end # @@ -38,6 +44,8 @@ class SettingsApi < Grape::API desc 'Return privacy policy details' get '/settings/privacy' do + authenticated? + response = { privacy: Doubtfire::Application.config.institution[:privacy], plagiarism: Doubtfire::Application.config.institution[:plagiarism]