Skip to content

ENG-3903: add error handling for missing target uri #155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pb-sobrien
Copy link

@pb-sobrien pb-sobrien commented Mar 24, 2025

Add validation for target_link_uri in LTI resource messages. Twice in the last week, two orgs have messed up this param (target_link_uri) not being in a launch and thrown a 500. This will catch the error nicely. :)

Purpose

This PR adds validation for the required target_link_uri claim in LTI Resource Link Requests, improving message security and conformance to the LTI 1.3 specification.

Importance

The target_link_uri is a required claim in LTI 1.3 resource link launches that specifies the intended destination URI for the launch. Validating this field:

  • Prevents processing incomplete/malformed LTI messages
  • Ensures proper implementation of security measures according to the specification
  • Protects applications from launch requests that might attempt to bypass launch target validation

Implementation

  • Added validation in ResourceMessageValidator::validate() that checks for the presence of the target_link_uri claim
  • Added corresponding test case in ResourceMessageValidatorTest to verify the validation behavior
  • Uses existing exception handling patterns consistent with other validations

Standards Reference

This change ensures conformance with the IMS Global LTI 1.3 Core Specification which requires the https://purl.imsglobal.org/spec/lti/claim/target_link_uri claim for all resource link
launch messages.

Testing

  • I have added automated tests for my changes
  • I ran composer test before opening this PR
  • I ran composer lint-fix before opening this PR

@pb-sobrien pb-sobrien requested a review from dbhynds as a code owner March 24, 2025 21:54
@pb-sobrien
Copy link
Author

pb-sobrien commented Mar 24, 2025

tests passing locally
image

@dbhynds
Copy link
Member

dbhynds commented Mar 25, 2025

@pb-sobrien Can you checkout a branch on our platform, point the LTI Lib to this branch in the composer.json, and make sure tests pass there?

Copy link
Member

@dbhynds dbhynds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. Just need to know it passes in our app

@dbhynds dbhynds self-assigned this Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants