Skip to content

Commit

Permalink
log error report when tool mangles parent_frame_context
Browse files Browse the repository at this point in the history
why:
* LTI tools are supposed to account for existing query parameters
in the launch and especially 1.1 content return URLs, but sometimes
do not
* this can mess up the parent_frame_context header, which is
necessary for nested LTI launches (like from RCE in NQ) to
function properly (sets the CSP header to allow Canvas to
be rendered within NQ)
* there isn't much we can do to fix this, the launch will be blocked
by the browser for CSP violations, so the least we can do is log
an ErrorReport to give the admins a heads up
* look for some form of community guide to troubleshoot CSP
errors in nested tools

closes INTEROP-8567
flag=none

test plan:
* visit some form of this url:
http://canvas.docker/courses/1/external_tools/retrieve?url=http://lti13testtool.docker/launch&parent_frame_context=97700000000003582return_type
replacing your canvas and tool domains as necessary
* check the /error_reports page and note a new report
* report should include the query params from the fatal request
* bonus points:
* embed a tool in your test tool by modifying its source code
to include an iframe pointing to either itself or another tool
* make sure that iframe has a url just like the one above
* launch the test tool and confirm that the nested tool is
CSP-blocked

Change-Id: Ie4550b1f40415a2814daceb03bef419914a1e80a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/346395
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Tucker Mcknight <tmcknight@instructure.com>
QA-Review: Tucker Mcknight <tmcknight@instructure.com>
Product-Review: Mark Starkman <mark.starkman@instructure.com>
  • Loading branch information
xandroxygen committed May 2, 2024
1 parent 986a628 commit cc79f27
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 2 deletions.
24 changes: 23 additions & 1 deletion app/controllers/lti/concerns/parent_frame.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,11 @@ def parent_frame_origin

# Don't look up tools for unauthenticated users
return nil unless @current_user && @current_pseudonym
return nil if parent_frame_context.blank?

tool = parent_frame_context.presence && ContextExternalTool.find_by(id: parent_frame_context)
validate_parent_frame_context

tool = ContextExternalTool.find_by(id: parent_frame_context)

@parent_frame_origin =
if !tool&.active? || !tool&.developer_key&.internal_service ||
Expand All @@ -59,6 +62,25 @@ def parent_frame_origin
end
end

# Tools sometimes may mangle the parent_frame_context header if
# they do not account for query parameters present in the launch
# or content return URL. This will result in this request being
# blocked by browsers since the CSP header is not correct. When
# this happens, log an ErrorReport to give admins visibility.
def validate_parent_frame_context
return if Api::ID_REGEX.match?(parent_frame_context.to_s)

extra = {
query_params: request.query_parameters,
message: "Invalid CSP header for nested LTI launch",
comments: <<~TEXT
Nested LTI launch likely failed. Check query parameters,
tool may have ignored query string in launch/content return URL.
TEXT
}
CanvasErrors.capture(:invalid_parent_frame_context, { extra: })
end

def override_parent_frame_origin(url)
uri = URI.parse(url)
origin = URI("#{uri.scheme}://#{uri.host}:#{uri.port}")
Expand Down
20 changes: 19 additions & 1 deletion spec/controllers/lti/concerns/parent_frame_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,15 @@
@pseudonym
end

let(:request) do
double("request", query_parameters: "hello=world")
end

before do
controller.instance_variable_set(:@current_user, current_pseudonym.user)
controller.instance_variable_set(:@current_pseudonym, current_pseudonym)
allow(controller).to receive_messages(parent_frame_context: tool.id.to_s, session: nil)
allow(controller).to receive_messages(parent_frame_context: tool.id.to_s, session: nil, request:)
allow(ContextExternalTool).to receive(:find_by).and_return(nil)
allow(ContextExternalTool).to receive(:find_by).with(id: tool.id.to_s).and_return(tool)
end

Expand All @@ -70,6 +75,19 @@

it { is_expected.to be_nil }
end

context "when parent_frame_context is malformed" do
before do
allow(controller).to receive(:parent_frame_context).and_return("10000000000001uhoh")
end

it "captures an error" do
subject
error_report = ErrorReport.last
expect(error_report.message).to include("Invalid CSP header for nested LTI launch")
expect(error_report.data).to include("query_params" => "hello=world")
end
end
end
end

Expand Down

0 comments on commit cc79f27

Please sign in to comment.