search

Cvmcosta / ltijs

Turn your application into a fully integratable LTI 1.3 tool provider.
https://cvmcosta.github.io/ltijs/
Apache License 2.0
299 stars 67 forks source link

Missing "state" field in OIDC login response #16

Closed esellin closed 4 years ago

esellin commented 4 years ago

Hi there! Utils.Reqest.ltiAdvantageLogin() doesn't seem to set the state field in the OIDC login response, which I believe is mandatory according to specs, so it can later be validated together with the id_token in Utils.Auth.validateToken(). Am I missing something, or is this something you plan to add? Thanks!

Cvmcosta commented 4 years ago

Hello @esellin!

The oficial LTI spec lists the state parameter as "recommended" instead of "required": 5.1.1.2 Step 2: Authentication Request. Because of this i think i might have overlooked it when first implementing that portion of the protocol. So i spent some time researching and i think it would be a good idea to implement it for security reasons. But a loose implementation (Will only attempt to validade if present in the authentication response), because it seems that not all platforms implement this parameter yet.

Thanks a lot!

Cvmcosta commented 4 years ago

All done, the new security update is now live. I also added iss claim validation and updated some dependencies. This patch was tested on Moodle 3.8, and i'll probably test on Canvas tomorrow.

Thanks for pointing this out! :smiley:

Cvmcosta commented 4 years ago

Just to be clear, i ended up not going for a loose implementation, mostly because i think it would kinda defeat the purpose of adding the security feature in the first place. I'll just test this change on the most popular LMS's and see if it works on all of them.

esellin commented 4 years ago

Excellent stuff!