From 3456230e221de9f3df32e0cfe017f9421e7673a5 Mon Sep 17 00:00:00 2001 From: Karol Date: Tue, 28 Jan 2025 22:58:05 +0100 Subject: [PATCH 1/4] 1680: Improve error when exporting through an account_link --- app/services/task_service/push_external.rb | 6 +++++- config/locales/de/controllers/tasks.yml | 1 + config/locales/en/controllers/tasks.yml | 1 + spec/services/task_service/push_external_spec.rb | 7 +++++++ 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/services/task_service/push_external.rb b/app/services/task_service/push_external.rb index 972b70ad3..5a40926ae 100644 --- a/app/services/task_service/push_external.rb +++ b/app/services/task_service/push_external.rb @@ -12,7 +12,11 @@ def execute body = @zip.string begin response = self.class.connection.post(@account_link.push_url) {|request| request_parameters(request, body) } - response.success? ? nil : response.body + if response.success? + nil + else + response.status == 401 ? I18n.t('tasks.export_external_confirm.not_authorized', account_link: @account_link.name) : response.body + end rescue StandardError => e e end diff --git a/config/locales/de/controllers/tasks.yml b/config/locales/de/controllers/tasks.yml index fe3b551c0..bbf9e5dc1 100644 --- a/config/locales/de/controllers/tasks.yml +++ b/config/locales/de/controllers/tasks.yml @@ -8,6 +8,7 @@ de: error_alert: Die Aufgabe konnte nicht dupliziert werden. export_external_confirm: error: 'Der Export der Aufgabe (%{title}) ist fehlgeschlagen.
Fehler: %{error}' + not_authorized: Die Autorisierung mit "%{account_link}" konnte nicht hergestellt werden. Ist der API-Schlüssel korrekt? success: Aufgabe (%{title}) erfolgreich exportiert. import: internal_error: Beim Import dieser Aufgabe ist auf CodeHarbor ein interner Fehler aufgetreten. diff --git a/config/locales/en/controllers/tasks.yml b/config/locales/en/controllers/tasks.yml index b3dc07df5..e8078c7f6 100644 --- a/config/locales/en/controllers/tasks.yml +++ b/config/locales/en/controllers/tasks.yml @@ -8,6 +8,7 @@ en: error_alert: Task could not be duplicated export_external_confirm: error: 'Export of task (%{title}) failed.
Error: %{error}' + not_authorized: Authorization with could not be established with "%{account_link}". Is the API Key correct? success: Task (%{title}) successfully exported. import: internal_error: An internal error occurred on CodeHarbor while importing the exercise. diff --git a/spec/services/task_service/push_external_spec.rb b/spec/services/task_service/push_external_spec.rb index f913ff2a7..937c7d698 100644 --- a/spec/services/task_service/push_external_spec.rb +++ b/spec/services/task_service/push_external_spec.rb @@ -53,6 +53,13 @@ it { is_expected.to be response } end + + context 'when response status is 401' do + let(:status) { 401 } + let(:response) { I18n.t('tasks.export_external_confirm.not_authorized', account_link: account_link.name) } + + it { is_expected.to eq response } + end end context 'when an error occurs' do From 3568eb08577da6cc807e1cabdc13f61e97dccf00 Mon Sep 17 00:00:00 2001 From: Karol Date: Tue, 4 Feb 2025 22:04:08 +0100 Subject: [PATCH 2/4] 1680: add sanitation of error-message from potentially external account_link partner --- app/services/task_service/push_external.rb | 25 ++++++++++-------- config/locales/de/controllers/tasks.yml | 3 ++- config/locales/en/controllers/tasks.yml | 3 ++- .../task_service/push_external_spec.rb | 26 ++++++++++++++++++- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/app/services/task_service/push_external.rb b/app/services/task_service/push_external.rb index 5a40926ae..d89982e92 100644 --- a/app/services/task_service/push_external.rb +++ b/app/services/task_service/push_external.rb @@ -9,21 +9,24 @@ def initialize(zip:, account_link:) end def execute - body = @zip.string - begin - response = self.class.connection.post(@account_link.push_url) {|request| request_parameters(request, body) } - if response.success? - nil - else - response.status == 401 ? I18n.t('tasks.export_external_confirm.not_authorized', account_link: @account_link.name) : response.body - end - rescue StandardError => e - e - end + response = self.class.connection.post(@account_link.push_url) {|request| request_parameters(request, @zip.string) } + return nil if response.success? + return I18n.t('tasks.export_external_confirm.not_authorized', account_link: @account_link.name) if response.status == 401 + + handle_error(message: response.body) + rescue Faraday::ServerError => e + handle_error(error: e, message: I18n.t('tasks.export_external_confirm.server_error', account_link: @account_link.name)) + rescue StandardError => e + handle_error(error: e) end private + def handle_error(message: nil, error: nil) + Sentry.capture_exception(error) if error.present? + ERB::Util.html_escape(message || error.to_s) + end + def request_parameters(request, body) request.tap do |req| req.headers['Content-Type'] = 'application/zip' diff --git a/config/locales/de/controllers/tasks.yml b/config/locales/de/controllers/tasks.yml index bbf9e5dc1..f2c025a2d 100644 --- a/config/locales/de/controllers/tasks.yml +++ b/config/locales/de/controllers/tasks.yml @@ -7,8 +7,9 @@ de: duplicate: error_alert: Die Aufgabe konnte nicht dupliziert werden. export_external_confirm: - error: 'Der Export der Aufgabe (%{title}) ist fehlgeschlagen.
Fehler: %{error}' + error: 'Der Export der Aufgabe (%{title}) ist fehlgeschlagen.

Fehler: %{error}' not_authorized: Die Autorisierung mit "%{account_link}" konnte nicht hergestellt werden. Ist der API-Schlüssel korrekt? + server_error: Verbindung zu %{account_link} fehlgeschlagen. Gegenseite nicht erreichbar. success: Aufgabe (%{title}) erfolgreich exportiert. import: internal_error: Beim Import dieser Aufgabe ist auf CodeHarbor ein interner Fehler aufgetreten. diff --git a/config/locales/en/controllers/tasks.yml b/config/locales/en/controllers/tasks.yml index e8078c7f6..7db64d2a8 100644 --- a/config/locales/en/controllers/tasks.yml +++ b/config/locales/en/controllers/tasks.yml @@ -7,8 +7,9 @@ en: duplicate: error_alert: Task could not be duplicated export_external_confirm: - error: 'Export of task (%{title}) failed.
Error: %{error}' + error: 'Export of task (%{title}) failed.

Error: %{error}' not_authorized: Authorization with could not be established with "%{account_link}". Is the API Key correct? + server_error: Connection to %{account_link} failed. Remote host unreachable. success: Task (%{title}) successfully exported. import: internal_error: An internal error occurred on CodeHarbor while importing the exercise. diff --git a/spec/services/task_service/push_external_spec.rb b/spec/services/task_service/push_external_spec.rb index 937c7d698..86d332fe0 100644 --- a/spec/services/task_service/push_external_spec.rb +++ b/spec/services/task_service/push_external_spec.rb @@ -51,7 +51,13 @@ let(:status) { 500 } let(:response) { 'an error occurred' } - it { is_expected.to be response } + it { is_expected.to eql response } + + context 'when response contains problematic characters' do + let(:response) { 'an occurred' } + + it { is_expected.to eql 'an <error> occurred' } + end end context 'when response status is 401' do @@ -60,6 +66,24 @@ it { is_expected.to eq response } end + + context 'when faraday throws an error' do + let(:connection) { instance_double(Faraday::Connection) } + let(:error) { Faraday::ServerError } + + before do + allow(Faraday).to receive(:new).and_return(connection) + allow(connection).to receive(:post).and_raise(error) + end + + it { is_expected.to eql I18n.t('tasks.export_external_confirm.server_error', account_link: account_link.name) } + + context 'when another error occurs' do + let(:error) { 'another error' } + + it { is_expected.to eql 'another error' } + end + end end context 'when an error occurs' do From acdeaf69a02eacd43cce4a80756d006aa5e458dc Mon Sep 17 00:00:00 2001 From: karol Date: Tue, 3 Jun 2025 23:26:42 +0200 Subject: [PATCH 3/4] 1680: add generic error message to avoid leaking potential sensitive information --- app/services/task_service/push_external.rb | 18 +++++++++++------- config/locales/de/controllers/tasks.yml | 1 + config/locales/en/controllers/tasks.yml | 1 + .../task_service/push_external_spec.rb | 16 +++++----------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/services/task_service/push_external.rb b/app/services/task_service/push_external.rb index d89982e92..759ed3abf 100644 --- a/app/services/task_service/push_external.rb +++ b/app/services/task_service/push_external.rb @@ -10,21 +10,25 @@ def initialize(zip:, account_link:) def execute response = self.class.connection.post(@account_link.push_url) {|request| request_parameters(request, @zip.string) } - return nil if response.success? - return I18n.t('tasks.export_external_confirm.not_authorized', account_link: @account_link.name) if response.status == 401 - - handle_error(message: response.body) + handle_response(response) rescue Faraday::ServerError => e handle_error(error: e, message: I18n.t('tasks.export_external_confirm.server_error', account_link: @account_link.name)) rescue StandardError => e - handle_error(error: e) + handle_error(error: e, message: I18n.t('tasks.export_external_confirm.generic_error')) end private - def handle_error(message: nil, error: nil) + def handle_response(response) + return nil if response.success? + return I18n.t('tasks.export_external_confirm.not_authorized', account_link: @account_link.name) if response.status == 401 + + handle_error(message: response.body) + end + + def handle_error(message:, error: nil) Sentry.capture_exception(error) if error.present? - ERB::Util.html_escape(message || error.to_s) + ERB::Util.html_escape(message) end def request_parameters(request, body) diff --git a/config/locales/de/controllers/tasks.yml b/config/locales/de/controllers/tasks.yml index f2c025a2d..d3e9593a3 100644 --- a/config/locales/de/controllers/tasks.yml +++ b/config/locales/de/controllers/tasks.yml @@ -8,6 +8,7 @@ de: error_alert: Die Aufgabe konnte nicht dupliziert werden. export_external_confirm: error: 'Der Export der Aufgabe (%{title}) ist fehlgeschlagen.

Fehler: %{error}' + generic_error: Ein unbekannter Fehler ist beim exportieren der Aufgabe aufgetreten. not_authorized: Die Autorisierung mit "%{account_link}" konnte nicht hergestellt werden. Ist der API-Schlüssel korrekt? server_error: Verbindung zu %{account_link} fehlgeschlagen. Gegenseite nicht erreichbar. success: Aufgabe (%{title}) erfolgreich exportiert. diff --git a/config/locales/en/controllers/tasks.yml b/config/locales/en/controllers/tasks.yml index 7db64d2a8..c9c573ecf 100644 --- a/config/locales/en/controllers/tasks.yml +++ b/config/locales/en/controllers/tasks.yml @@ -8,6 +8,7 @@ en: error_alert: Task could not be duplicated export_external_confirm: error: 'Export of task (%{title}) failed.

Error: %{error}' + generic_error: An unknown error has occurred while exporting the task. not_authorized: Authorization with could not be established with "%{account_link}". Is the API Key correct? server_error: Connection to %{account_link} failed. Remote host unreachable. success: Task (%{title}) successfully exported. diff --git a/spec/services/task_service/push_external_spec.rb b/spec/services/task_service/push_external_spec.rb index 86d332fe0..9adffb89f 100644 --- a/spec/services/task_service/push_external_spec.rb +++ b/spec/services/task_service/push_external_spec.rb @@ -72,28 +72,22 @@ let(:error) { Faraday::ServerError } before do - allow(Faraday).to receive(:new).and_return(connection) + allow(TaskService).to receive(:connection).and_return(connection) allow(connection).to receive(:post).and_raise(error) end it { is_expected.to eql I18n.t('tasks.export_external_confirm.server_error', account_link: account_link.name) } - - context 'when another error occurs' do - let(:error) { 'another error' } - - it { is_expected.to eql 'another error' } - end end end context 'when an error occurs' do + let(:error) { StandardError.new('Standard error occurred') } + before do - # Un-memoize the connection to force a reconnection - described_class.instance_variable_set(:@connection, nil) - allow(Faraday).to receive(:new).and_raise(StandardError) + allow(TaskService).to receive(:connection).and_raise(error) end - it { is_expected.not_to be_nil } + it { is_expected.to eql I18n.t('tasks.export_external_confirm.generic_error') } end end end From 7d59b3078ee9a4b718c54f6fb2956fb6553805fe Mon Sep 17 00:00:00 2001 From: kkoehn Date: Tue, 3 Jun 2025 23:29:53 +0200 Subject: [PATCH 4/4] Apply suggestions from code review Co-authored-by: Sebastian Serth --- config/locales/de/controllers/tasks.yml | 2 +- config/locales/en/controllers/tasks.yml | 4 ++-- spec/services/task_service/push_external_spec.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config/locales/de/controllers/tasks.yml b/config/locales/de/controllers/tasks.yml index d3e9593a3..576cc0215 100644 --- a/config/locales/de/controllers/tasks.yml +++ b/config/locales/de/controllers/tasks.yml @@ -10,7 +10,7 @@ de: error: 'Der Export der Aufgabe (%{title}) ist fehlgeschlagen.

Fehler: %{error}' generic_error: Ein unbekannter Fehler ist beim exportieren der Aufgabe aufgetreten. not_authorized: Die Autorisierung mit "%{account_link}" konnte nicht hergestellt werden. Ist der API-Schlüssel korrekt? - server_error: Verbindung zu %{account_link} fehlgeschlagen. Gegenseite nicht erreichbar. + server_error: Verbindung zu "%{account_link}" fehlgeschlagen. Gegenseite nicht erreichbar. success: Aufgabe (%{title}) erfolgreich exportiert. import: internal_error: Beim Import dieser Aufgabe ist auf CodeHarbor ein interner Fehler aufgetreten. diff --git a/config/locales/en/controllers/tasks.yml b/config/locales/en/controllers/tasks.yml index c9c573ecf..1ebf823ec 100644 --- a/config/locales/en/controllers/tasks.yml +++ b/config/locales/en/controllers/tasks.yml @@ -9,8 +9,8 @@ en: export_external_confirm: error: 'Export of task (%{title}) failed.

Error: %{error}' generic_error: An unknown error has occurred while exporting the task. - not_authorized: Authorization with could not be established with "%{account_link}". Is the API Key correct? - server_error: Connection to %{account_link} failed. Remote host unreachable. + not_authorized: Authorization with "%{account_link}" could not be established. Is the API key correct? + server_error: Connection to "%{account_link}" failed. Remote host unreachable. success: Task (%{title}) successfully exported. import: internal_error: An internal error occurred on CodeHarbor while importing the exercise. diff --git a/spec/services/task_service/push_external_spec.rb b/spec/services/task_service/push_external_spec.rb index 9adffb89f..a511cda0f 100644 --- a/spec/services/task_service/push_external_spec.rb +++ b/spec/services/task_service/push_external_spec.rb @@ -76,7 +76,7 @@ allow(connection).to receive(:post).and_raise(error) end - it { is_expected.to eql I18n.t('tasks.export_external_confirm.server_error', account_link: account_link.name) } + it { is_expected.to eql ERB::Util.html_escape(I18n.t('tasks.export_external_confirm.server_error', account_link: account_link.name)) } end end