1.1.4 Broke Signature verification.

[Rakhmanov]

Pull request #10,

New code call urldecode on all of the POST properties effectively substituting + sign for the white space in OAuth Signature. Thus causing all signatures that have + to be invalid.

I will see if have time to make pull request.

[dac514]

Pull request #10 makes a call to:

    // This decode function isn't taking into consideration the above
    // modifications to the encoding process. However, this method doesn't
    // seem to be used anywhere so leaving it as is.
    public static function urldecode_rfc3986($string) {
        return urldecode($string);
    }

There are at least two things wrong with urldecode_rfc3986().

1) The code comment says it's not used anywhere. That's wrong. It's used in 4 places.

2) In PHP, rawurlencode() implements RFC 3986 while urlencode() implements application/x-www-form-urlencoded. Thusly, rawurldecode() does not decode plus symbols ('+') into spaces. urldecode() does.

Brilliant...

We can either change \IMSGlobal\LTI\OAuth\OAuthUtil::urldecode_rfc3986 to use rawurldecode() which I think is a bit dangerous since it's used in three other places...

Or change PR #10 to:

foreach ($_POST as $key => $value) {
    $parameters[$key] = rawurldecode($value);
} 

What do others think?

[dac514]

I tested the second proposed fix with Canvas. I went from "Sorry, there was an error connecting you to the application" when the oauth_signature had a + symbol in it, to working again. I'll wait an hour for feedback but will make pull a request with that change if I don't hear back.

[dac514]

Thinking about this some more. The old code in PR #10 used php://input (ie. $HTTP_RAW_POST_DATA) whereas the new code uses $_POST.

$_POST can be said as and outcome after splitting the $HTTP_RAW_POST_DATA, php splits the raw post data and formats in the way we see it in the $_POST. For example when $HTTP_RAW_POST_DATA looks something like this:

key1=value1&key2=value2

then $_POST would look like this:

$_POST = [ "key1" => "value1", "key2" => "value2",];

Source: http://php.net/manual/en/reserved.variables.httprawpostdata.php

Why do we need to decode at all?

[Izumi-kun]

I have no idea why $_POST values should be decoded. @markn86?

[Izumi-kun]

I am about to change https://github.com/Izumi-kun/LTI-Tool-Provider-Library-PHP/blob/389508581aac892bdba1eac4a663138b122252f4/src/OAuth/OAuthRequest.php#L73-L75 to

$parameters = array_merge($parameters, $_POST);

Correct?

[dac514]

$parameters = array_merge($parameters, $_POST);

Everything I've tested so far says this is fine. Would love a 2nd opinion.

[Rakhmanov]

Hey, didn't expect such an activity. That change will do it. However the lines below that handle header authentication says explicitly that they will override the get and post parameters, is there reason not to continue doing it? If so lets change comment to reflect that.

// We have a Authorization-header with OAuth data. Parse the header // and add those overriding any duplicates from GET or POST

And also remove line 20 since this variable is never used now. public static $POST_INPUT = 'php://input';