celtic-project / LTI-PHP

PHP class library for building LTI integrations
GNU Lesser General Public License v3.0
48 stars 38 forks source link

Invalid redirect url with the new 4.0.1 library #9

Closed alexvanniel closed 4 years ago

alexvanniel commented 4 years ago

After upgrading from 4.0.0 to 4.0.1 I seem to be getting an error when trying to launch a resource link from within Blackboard saying 'Invalid redirect url: ...'. Something must have changed for this to happen. When I downgrade to 4.0.0 the problem goes away. I have not been able to pinpoint myself where this issue occurs but it must be one of the recent changes done to the library.

spvickers commented 4 years ago

Make sure that you have configured Blackboard with the full redirect URI including any query string. This error was fixed in 4.0.1.

alexvanniel commented 4 years ago

I am actually using 4.0.1 and the full redirect URI is configured in Blackboard.

spvickers commented 4 years ago

Does the error message from Blackboard indicate the URL which it deems to be invalid? If so, does it look invalid? Is it different from the one registered in the Blackboard configuration?

alexvanniel commented 4 years ago

The url I configured is indeed different from the one I get back in the error message.

The one I configured was along the lines of https://testing.********.nl/api/lti/tool

In the error message I see that because of the change done in https://github.com/celtic-project/LTI-PHP/commit/5595e26c6d568f44ce02a0d091132354708cdbec I now have things like ?iss=https%3A%2F%2Fblackboard.com&login_hint=.... etc etc added to the end of the preconfigured url. I doubt all the OAuth parameters are expected to be added to the url. Am I to add those to the url in the Blackboard configuration? It includes the entire message_hint and everything ... I doubt this is stuff that should be added to the query string.

spvickers commented 4 years ago

OK, this bug needs to be fixed. The current code assumes that the URL used to access the tool is the one which it configures in the platform. Clearly this is not the case when a login initiate request is made using an HTTP GET. I'll add a check for this.

alexvanniel commented 4 years ago

Would it be enough to replace:

                if (!empty($_SERVER['QUERY_STRING'])) {
                    $redirectUri .= "?{$_SERVER['QUERY_STRING']}";
                }

with

                if (!empty($_SERVER['QUERY_STRING']) && $_SERVER['REQUEST_METHOD'] !== 'GET') {
                    $redirectUri .= "?{$_SERVER['QUERY_STRING']}";
                }

???

spvickers commented 4 years ago

Not really. What if a tool has its own query parameters on the URL? The code must try to work out those which have been added by the platform and remove these. Fortunately most platforms use the POST method; Blackboard is the only one I have come across which is using GET.

alexvanniel commented 4 years ago

So, you'd have to filter things out that Blackboard is sending in the query string? Wouldn't that pose a problem when Blackboard decides to add some data to the query string at some point again? Or is the query string sent by a GET request predefined and standardised?

spvickers commented 4 years ago

Yes, there is no easy answer to this issue. At present the simplest solution I can think of is to remove any of the parameters permitted by the spec; I see no reason why a platform would add any others of its own but this could change.

alexvanniel commented 4 years ago

Or not allow custom query string parameters at all, but I guess you're not a fan of that approach? ;-)

spvickers commented 4 years ago

You are quite right, I do not regard not allowing query parameters on the intiate login URL as being appropriate. My aim is for the library to be usable by any PHP tool and there are many examples of applications which have a single entry point and use query parameters to identify the code to be executed, WordPress being one example which comes to mind. Hence denying query parameters on tool URLs would prevent the library from being used with platforms such as Blackboard. I have committed code which I believe should resolve the issue.