From 159a131989d5a12c9ab81e8adfbd267ac03516a6 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 5 May 2023 10:18:51 +0800 Subject: [PATCH 1/2] MDL-77990 enrol_lti: test covering the response header parsing Makes sure the http_client shim properly returns the correct, final http headers, not intermediate headers such as 302, 100 etc. --- .../local/ltiadvantage/lib/http_client_test.php | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/enrol/lti/tests/local/ltiadvantage/lib/http_client_test.php b/enrol/lti/tests/local/ltiadvantage/lib/http_client_test.php index 392f57d534e..578e11a14f7 100644 --- a/enrol/lti/tests/local/ltiadvantage/lib/http_client_test.php +++ b/enrol/lti/tests/local/ltiadvantage/lib/http_client_test.php @@ -24,7 +24,7 @@ namespace enrol_lti\local\ltiadvantage\lib; * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later * @coversDefaultClass \enrol_lti\local\ltiadvantage\lib\http_client */ -class http_client_test extends \basic_testcase { +class http_client_test extends \advanced_testcase { /** * Verify the http_client delegates to curl during a "GET" request. @@ -116,4 +116,17 @@ class http_client_test extends \basic_testcase { 'delete' => ['DELETE'], ]; } + + /** + * Verify that the response headers are properly read from curl, and exclude things like redirect headers, or 100-continues. + * @covers ::request + */ + public function test_header_parsing(): void { + $testurl = $this->getExternalTestFileUrl('/test_redir.php'); + $client = new http_client(new \curl()); + $response = $client->request('POST', "$testurl?redir=1", + ['headers' => ['Expect' => '100-continue'], 'body' => 'foo']); + $headers = $response->getHeaders(); + $this->assertEquals('200 OK', reset($headers)); + } } From 75ba439f5be1e049a8ba680bd7736b57cf65161d Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 5 May 2023 10:00:28 +0800 Subject: [PATCH 2/2] MDL-77990 enrol_lti: fix http_client shim by using curl getResponse Fixes an error in the parsing of response headers containing multiple HTTP responses in the raw response. Curl already handles this, so let it do the work. --- .../local/ltiadvantage/lib/http_client.php | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/enrol/lti/classes/local/ltiadvantage/lib/http_client.php b/enrol/lti/classes/local/ltiadvantage/lib/http_client.php index 1911ea2e1d0..7d77cf89570 100644 --- a/enrol/lti/classes/local/ltiadvantage/lib/http_client.php +++ b/enrol/lti/classes/local/ltiadvantage/lib/http_client.php @@ -77,23 +77,11 @@ class http_client implements IHttpClient { $info = $this->curlclient->get_info(); if (!$this->curlclient->get_errno() && !$this->curlclient->error) { // No errors, so format the response. - $headersize = $info['header_size']; - $resheaders = substr($res ?? '', 0, $headersize); - $resbody = substr($res ?? '', $headersize); - $headerlines = array_filter(explode("\r\n", $resheaders)); - $parsedresponseheaders = [ - 'httpstatus' => array_shift($headerlines) - ]; - foreach ($headerlines as $headerline) { - $headerbits = explode(':', $headerline, 2); - if (count($headerbits) == 2) { - // Only parse headers having colon separation. - $parsedresponseheaders[$headerbits[0]] = $headerbits[1]; - } - } - $response = new http_response(['headers' => $parsedresponseheaders, 'body' => $resbody], intval($info['http_code'])); + $resbody = substr($res ?? '', $info['header_size']); + $headers = $this->curlclient->getResponse(); + $response = new http_response(['headers' => $headers, 'body' => $resbody], intval($info['http_code'])); if ($response->getStatusCode() >= 400) { - throw new http_exception($response, "An HTTP error status was received: '{$response->getHeaders()['httpstatus']}'"); + throw new http_exception($response, "An HTTP error status was received: '".reset($headers)."'"); } return $response; }