From acb9563ada715b04d673219df9c2217fd962724a Mon Sep 17 00:00:00 2001 From: Ethan Vizitei Date: Wed, 13 Jan 2021 15:09:58 -0600 Subject: [PATCH] fail gracefully on a bad tool launch url closes FOO-1464 flag=none TEST PLAN: 1) scrub a launch by forcing a silly url 2) you get a flash error rather than an app-level 500 Change-Id: I38ac55d85f505e62fc7368da6026e19cf4573a06 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/256673 Tested-by: Service Cloud Jenkins Reviewed-by: Rob Orton QA-Review: Ethan Vizitei Product-Review: Ethan Vizitei --- app/controllers/external_tools_controller.rb | 1 + app/models/lti/lti_outbound_adapter.rb | 19 ++++++++++++------- lib/lti/errors.rb | 1 + .../external_tools_controller_spec.rb | 15 ++++++++++++--- spec/models/lti/lti_outbound_adapter_spec.rb | 9 +++++++++ 5 files changed, 35 insertions(+), 10 deletions(-) diff --git a/app/controllers/external_tools_controller.rb b/app/controllers/external_tools_controller.rb index 60a7fc432bb..431d29bcf07 100644 --- a/app/controllers/external_tools_controller.rb +++ b/app/controllers/external_tools_controller.rb @@ -490,6 +490,7 @@ class ExternalToolsController < ApplicationController render_unauthorized_action nil rescue Lti::Errors::UnsupportedExportTypeError, + Lti::Errors::InvalidLaunchUrlError, Lti::Errors::InvalidMediaTypeError, Lti::Errors::UnsupportedPlacement => e Canvas::Errors.capture_exception(:lti_launch, e, :info) diff --git a/app/models/lti/lti_outbound_adapter.rb b/app/models/lti/lti_outbound_adapter.rb index d713f7c46ca..5ca064df83e 100644 --- a/app/models/lti/lti_outbound_adapter.rb +++ b/app/models/lti/lti_outbound_adapter.rb @@ -18,6 +18,7 @@ # with this program. If not, see . module Lti + class LtiOutboundAdapter cattr_writer :consumer_instance_class @@ -82,13 +83,17 @@ module Lti hash = @tool_launch.generate(@overrides) hash[:ext_lti_assignment_id] = assignment&.lti_context_id if assignment&.lti_context_id.present? hash[:ext_lti_student_id] = student_id if student_id - Lti::Security.signed_post_params( - hash, - @tool_launch.url, - @tool.consumer_key, - @tool.shared_secret, - disable_post_only? - ) + begin + return Lti::Security.signed_post_params( + hash, + @tool_launch.url, + @tool.consumer_key, + @tool.shared_secret, + disable_post_only? + ) + rescue URI::InvalidURIError + raise ::Lti::Errors::InvalidLaunchUrlError, "Invalid launch url: #{@tool_launch.url}" + end end def generate_post_payload_for_assignment(assignment, outcome_service_url, legacy_outcome_service_url, lti_turnitin_outcomes_placement_url) diff --git a/lib/lti/errors.rb b/lib/lti/errors.rb index 939843dd4b5..2d239cb7fe7 100644 --- a/lib/lti/errors.rb +++ b/lib/lti/errors.rb @@ -25,5 +25,6 @@ module Lti class UnsupportedPlacement < StandardError; end class InvalidMediaTypeError < StandardError; end class UnauthorizedToolError < StandardError; end + class InvalidLaunchUrlError < StandardError; end end end diff --git a/spec/controllers/external_tools_controller_spec.rb b/spec/controllers/external_tools_controller_spec.rb index 36557485618..56c9abef456 100644 --- a/spec/controllers/external_tools_controller_spec.rb +++ b/spec/controllers/external_tools_controller_spec.rb @@ -156,21 +156,30 @@ describe ExternalToolsController do "client_id" ] end - + it 'sets the "login_hint" to the current user lti id' do expect(assigns[:lti_launch].params['login_hint']).to eq Lti::Asset.opaque_identifier_for(@teacher) end - + it 'caches the the LTI 1.3 launch' do expect(cached_launch["https://purl.imsglobal.org/spec/lti/claim/message_type"]).to eq "LtiResourceLinkRequest" end - + it 'sets the "canvas_domain" to the request domain' do message_hint = JSON::JWT.decode(assigns[:lti_launch].params['lti_message_hint'], :skip_verification) expect(message_hint['canvas_domain']).to eq 'localhost' end end + context "with a bad launch url" do + it "fails gracefully" do + user_session(@teacher) + allow(controller).to receive(:basic_lti_launch_request).and_raise(Lti::Errors::InvalidLaunchUrlError) + get :show, params: {:course_id => @course.id, id: tool.id} + expect(response).to be_redirect + end + end + context 'current user is a student view user' do before do user_session(@course.student_view_student) diff --git a/spec/models/lti/lti_outbound_adapter_spec.rb b/spec/models/lti/lti_outbound_adapter_spec.rb index 39fd6a069ff..58cc9d7cc7d 100644 --- a/spec/models/lti/lti_outbound_adapter_spec.rb +++ b/spec/models/lti/lti_outbound_adapter_spec.rb @@ -216,6 +216,15 @@ describe Lti::LtiOutboundAdapter do adapter.generate_post_payload end + it "errors if the url is ridiculous" do + tool_launch = double('tool launch') + expect(tool_launch).to receive_messages(generate: {}) + allow(tool_launch).to receive_messages(url: "wat;no-way") + allow(LtiOutbound::ToolLaunch).to receive(:new).and_return(tool_launch) + adapter.prepare_tool_launch(return_url, variable_expander) + expect{ adapter.generate_post_payload }.to raise_error(::Lti::Errors::InvalidLaunchUrlError) + end + it "does not copy query params to the post body if oauth_compliant tool setting is enabled" do allow(account).to receive(:all_account_users_for).with(user).and_return([]) tool.settings = {oauth_compliant: true}