LTI: remove request.host from LTI parameters

apluslms / a-plus

A+ frontend portal - A+ LMS documentation:

https://apluslms.github.io/

Other

66 stars 73 forks source link

LTI: remove request.host from LTI parameters #422

Closed raphendyr closed 4 years ago

raphendyr commented 4 years ago

Remove request.get_host() from all LTI parameter calculations. Use settings.BASE_URL instead. request.get_host comes from the browser, thus user can use it to manipulate the data send as part of the post.

These related issues can be fixed in connection or should be created as another issues:

[x] tool_consumer_instance_guid this field doesn't seem to include relevant information. it should be created from settings.BASE_URL
[x] launch_presentation_return_url this should include link to the page in A+, which will render the launch button (basically to the page this link is rendered at).
[ ] Interesting second part is to include or keep query parameters, so we can request reauthentication by the tool provider (e.g. koodisäilö), which would be sending browser to {launch_presentation_return_url}?tc_return_url=<ulr> or such. TODO: raphendyr needs to write down better explanation (i.e. create an issue) - Moved to an issue #431

alalav1 commented 4 years ago

It seems that there is no SITE_URL defined in the course settings. Should we use BASE_URL instead?

markkuriekkinen commented 4 years ago

request.get_host() can be only used when the URL is directly visible to the client (web browser). Request.get_host() must not be used for server-to-server communications or protocols. The assignee of the issue should check all uses of request.get_host() in A+ since it has been misused in the past.

markkuriekkinen commented 4 years ago

Note that the duplicate issue https://github.com/apluslms/a-plus/issues/347 has some additional details.

raphendyr commented 4 years ago

LTI documentation with the fields and their meanings https://www.imsglobal.org/specs/ltiv1p0/implementation-guide

HenrikSundell commented 4 years ago

Is request.get_host() only an issue in the LTI parameters? It is also used in lib/viewbase.py and shibboleth_login/views.py. I don't really understand the problem well enough to know if these are wrong.

raphendyr commented 4 years ago

Is request.get_host() only an issue in the LTI parameters? It is also used in lib/viewbase.py and shibboleth_login/views.py. I don't really understand the problem well enough to know if these are wrong.

@HenrikSundell request.get_host() returns the Host header from the web request. The client (web browser) defines the content of the header and technically it could be anything. In short, we can trust that information for anything else, except when we pass it back to the browser, which shouldn't be needed normally. This bloc post might give some insight into the issue.

In LTI context, if we pass url based on Host header to backend service, then we let the user to define where the backend makes connections (as they will connect the URL provided to them). The goal here is to mitigage possible vulnerabilities. Specially with _return_url as it will be taken from a single user and shown to others, which could lead to huge problems.

In addition to LTI, all other locations which trust in the Host header should be fixed too. No one just haven't done anything about it, e.g., created issues.