From 1688ce8fee25052930efa11e5a905b8a648d136a Mon Sep 17 00:00:00 2001 From: Marvin Hackfort Date: Thu, 3 Sep 2026 12:31:59 +0200 Subject: [PATCH] [FIX] CmiXapi: Handle xAPI document resource preconditions in the proxy The xAPI proxy did not forward If-Match/If-None-Match to the LRS. Both headers are missing from the allowlist in createProxyRequest(), so a content player could not announce the document revision it expects. For the xAPI document resources the specification requires an LRS to reject a PUT on an already existing document that carries neither header with 409 Conflict, which checkResponse() then replaced by a generic "412 Wrong Response". A specification compliant LRS therefore rejected every rewrite of a state document and the content never learned why. Learning Locker ignores both headers, so the defect only surfaced on strict LRS. Both headers are now relayed, the ETag of a document is exposed to the content via CORS, and the conditional status codes reach the content unchanged instead of being masked. TinCanJS, which is bundled with common content players, sends no precondition at all on a state write unless the caller supplied the SHA1 of the document it read before, so a write rejected with 409 is repeated once with an If-Match built from the current ETag. --- .../classes/XapiProxy/XapiProxyRequest.php | 135 +++++++++++++++++- .../classes/XapiProxy/XapiProxyResponse.php | 16 ++- .../ILIAS/CmiXapi/resources/xapiproxy.php | 4 +- .../CmiXapi/tests/XapiProxyRequestTest.php | 77 ++++++++++ .../CmiXapi/tests/XapiProxyResponseTest.php | 77 ++++++++++ 5 files changed, 303 insertions(+), 6 deletions(-) create mode 100644 components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php create mode 100644 components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php diff --git a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php index cbb42a9ac46e..d84519cfc60a 100755 --- a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php +++ b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyRequest.php @@ -1,7 +1,5 @@ + */ + private const CONDITIONAL_REQUEST_HEADERS = ['If-Match', 'If-None-Match']; + + /** + * @var list + */ + private const DOCUMENT_RESOURCES = ['activities/state', 'activities/profile', 'agents/profile']; + public function __construct(XapiProxy $xapiproxy) { $this->dic = $GLOBALS['DIC']; @@ -279,6 +293,25 @@ private function handleProxy(\Psr\Http\Message\RequestInterface $request, $fakeP $this->xapiproxy->log()->error($this->msg($e->getMessage())); } + $responses['default'] = $this->repeatDocumentWriteWithPrecondition( + $httpclient, + $request, + $responses['default'], + $uriDefault, + $authDefault, + $body, + $req_opts + ); + $responses['fallback'] = $this->repeatDocumentWriteWithPrecondition( + $httpclient, + $request, + $responses['fallback'], + $uriFallback, + $authFallback, + $body, + $req_opts + ); + $defaultOk = $this->xapiProxyResponse->checkResponse($responses['default'], $endpointDefault); $fallbackOk = $this->xapiProxyResponse->checkResponse($responses['fallback'], $endpointFallback); @@ -318,6 +351,16 @@ private function handleProxy(\Psr\Http\Message\RequestInterface $request, $fakeP } catch (\Exception $e) { $this->xapiproxy->log()->error($this->msg($e->getMessage())); } + $responses['default'] = $this->repeatDocumentWriteWithPrecondition( + $httpclient, + $request, + $responses['default'], + $uriDefault, + $authDefault, + $body, + $req_opts + ); + if ($this->xapiProxyResponse->checkResponse($responses['default'], $endpointDefault)) { try { $this->xapiProxyResponse->handleResponse( @@ -372,10 +415,98 @@ private function createProxyRequest(\Psr\Http\Message\RequestInterface $request, $headers['Connection'] = $request->getHeader('Connection'); } + foreach (self::CONDITIONAL_REQUEST_HEADERS as $conditionalHeader) { + if ($request->hasHeader($conditionalHeader)) { + $headers[$conditionalHeader] = $request->getHeader($conditionalHeader); + } + } + //$this->xapiproxy->log()->debug($this->msg($body)); $req = new Request(strtoupper($request->getMethod()), $uri, $headers, $body); return $req; } + + /** + * TinCanJS, which is bundled with common content players, only sends If-Match on a state + * write when the caller supplied the SHA1 of the document it read before, and no precondition + * at all otherwise. A specification compliant LRS answers such a write with 409 Conflict. In + * that case the current ETag is looked up and the write is repeated once with an If-Match + * built from it. An LRS that does not demand a precondition never answers 409 and therefore + * never causes the additional roundtrip. + * @param array{state: string, value?: \GuzzleHttp\Psr7\Response, reason?: mixed} $response + * @param array $req_opts + * @return array{state: string, value?: \GuzzleHttp\Psr7\Response, reason?: mixed} + */ + private function repeatDocumentWriteWithPrecondition( + Client $httpclient, + \Psr\Http\Message\RequestInterface $request, + array $response, + Uri $uri, + string $auth, + string $body, + array $req_opts + ): array { + if ($response['state'] !== 'fulfilled' || $response['value']->getStatusCode() !== 409) { + return $response; + } + if (!$this->requiresSynthesizedPrecondition($request)) { + return $response; + } + $etag = $this->fetchDocumentEtag($httpclient, $request, $uri, $auth, $req_opts); + if ($etag === '') { + return $response; + } + + $this->xapiproxy->log()->debug($this->msg('lrs requires a precondition for ' . $uri . ', repeating request with If-Match: ' . $etag)); + + try { + /** @var \GuzzleHttp\Psr7\Response $repeated */ + $repeated = $httpclient->send( + $this->createProxyRequest($request, $uri, $auth, $body)->withHeader('If-Match', $etag), + $req_opts + ); + } catch (\Exception $e) { + $this->xapiproxy->log()->error($this->msg($e->getMessage())); + return $response; + } + + return ['state' => 'fulfilled', 'value' => $repeated]; + } + + private function requiresSynthesizedPrecondition(\Psr\Http\Message\RequestInterface $request): bool + { + return strtoupper($request->getMethod()) === 'PUT' + && in_array($this->xapiproxy->cmdParts()[3] ?? '', self::DOCUMENT_RESOURCES, true) + && !$request->hasHeader('If-Match') + && !$request->hasHeader('If-None-Match'); + } + + /** + * Returns the current ETag of an xAPI document, or an empty string if it does not exist. + * Some LRS send an ETag along with the 404 of a missing document, so only a 200 is trusted. + * @param array $req_opts + */ + private function fetchDocumentEtag( + Client $httpclient, + \Psr\Http\Message\RequestInterface $request, + Uri $uri, + string $auth, + array $req_opts + ): string { + $headers = ['Authorization' => $auth]; + if ($request->hasHeader('X-Experience-API-Version')) { + $headers['X-Experience-API-Version'] = $request->getHeader('X-Experience-API-Version'); + } + + try { + $probe = $httpclient->send(new Request('GET', $uri, $headers), $req_opts); + } catch (\Exception $e) { + $this->xapiproxy->log()->error($this->msg($e->getMessage())); + return ''; + } + + return $probe->getStatusCode() === 200 ? $probe->getHeaderLine('ETag') : ''; + } } diff --git a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php index 843038b59d67..571ef353ff21 100755 --- a/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php +++ b/components/ILIAS/CmiXapi/classes/XapiProxy/XapiProxyResponse.php @@ -1,6 +1,5 @@ + */ + private const RELAYED_STATUS_CODES = [200, 204, 304, 404, 409, 412]; + // private $dic; private XapiProxy $xapiproxy; //private $xapiProxyRequest; @@ -40,7 +49,7 @@ public function checkResponse(array $response, string $endpoint): bool { if ($response['state'] == 'fulfilled') { $status = $response['value']->getStatusCode(); - if ($status === 200 || $status === 204 || $status === 404) { + if (in_array($status, self::RELAYED_STATUS_CODES, true)) { return true; } else { $this->xapiproxy->log()->error("LRS error {$endpoint}: " . $response['value']->getBody()); @@ -241,6 +250,9 @@ public function emit(\GuzzleHttp\Psr7\Response $response): void } } + // the content may only read the relayed ETag of a document if it is exposed + header('Access-Control-Expose-Headers: ETag, Last-Modified, X-Experience-API-Version', true, $statusCode); + // statusline header(sprintf( 'HTTP/%s %d%s', diff --git a/components/ILIAS/CmiXapi/resources/xapiproxy.php b/components/ILIAS/CmiXapi/resources/xapiproxy.php index d86acf0a20c1..34d019a3ca06 100755 --- a/components/ILIAS/CmiXapi/resources/xapiproxy.php +++ b/components/ILIAS/CmiXapi/resources/xapiproxy.php @@ -35,7 +35,7 @@ header('Access-Control-Allow-Origin: ' . $_SERVER["HTTP_ORIGIN"]); header('Access-Control-Allow-Credentials: true'); header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS'); - header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Modified-Since,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With'); + header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Match,If-Modified-Since,If-None-Match,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With'); exit; } @@ -54,7 +54,7 @@ header('Access-Control-Allow-Origin: ' . $_SERVER["HTTP_ORIGIN"]); header('Access-Control-Allow-Credentials: true'); header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS'); - header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Modified-Since,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With'); + header('Access-Control-Allow-Headers: X-Experience-API-Version,Accept,Authorization,Etag,Cache-Control,Content-Type,DNT,If-Match,If-Modified-Since,If-None-Match,Keep-Alive,Origin,User-Agent,X-Mx-ReqToken,X-Requested-With'); exit; } diff --git a/components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php b/components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php new file mode 100644 index 000000000000..89b0bf3dec3b --- /dev/null +++ b/components/ILIAS/CmiXapi/tests/XapiProxyRequestTest.php @@ -0,0 +1,77 @@ + $headers + * @dataProvider preconditionCases + */ + public function testPreconditionIsOnlySynthesizedForUnconditionalDocumentWrites( + string $method, + string $resource, + array $headers, + bool $expected + ): void { + // XapiProxy declares its own method(), so the stub is configured through expects() + $proxy = $this->createMock(XapiProxy::class); + $proxy->expects($this->any())->method('cmdParts')->willReturn(['', '', '', $resource, '']); + + $method_under_test = new ReflectionMethod(XapiProxyRequest::class, 'requiresSynthesizedPrecondition'); + $method_under_test->setAccessible(true); + + $this->assertSame( + $expected, + $method_under_test->invoke( + new XapiProxyRequest($proxy), + new Request($method, 'https://ilias.example.org/xapiproxy.php/' . $resource, $headers) + ) + ); + } + + /** + * @return array, 3: bool}> + */ + public static function preconditionCases(): array + { + return [ + 'unconditional state write' => ['PUT', 'activities/state', [], true], + 'unconditional activity profile write' => ['PUT', 'activities/profile', [], true], + 'unconditional agent profile write' => ['PUT', 'agents/profile', [], true], + 'client sends If-Match' => ['PUT', 'activities/state', ['If-Match' => '"abc"'], false], + 'client sends If-None-Match' => ['PUT', 'activities/state', ['If-None-Match' => '*'], false], + 'merging POST is not guarded' => ['POST', 'activities/state', [], false], + 'document read is not guarded' => ['GET', 'activities/state', [], false], + 'document delete is not guarded' => ['DELETE', 'activities/state', [], false], + 'statements are immutable' => ['PUT', 'statements', [], false], + ]; + } +} diff --git a/components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php b/components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php new file mode 100644 index 000000000000..b9c85f965d5f --- /dev/null +++ b/components/ILIAS/CmiXapi/tests/XapiProxyResponseTest.php @@ -0,0 +1,77 @@ +createMock(XapiProxy::class); + $proxy->expects($this->any())->method('log')->willReturn($this->createMock(ilLogger::class)); + + $this->assertSame( + $expected, + (new XapiProxyResponse($proxy))->checkResponse( + ['state' => 'fulfilled', 'value' => new Response($status)], + 'https://lrs.example.org/xapi' + ) + ); + } + + /** + * @return array + */ + public static function statusCodes(): array + { + return [ + 'ok' => [200, true], + 'no content' => [204, true], + 'not modified' => [304, true], + 'document does not exist' => [404, true], + 'precondition required by the lrs' => [409, true], + 'precondition failed' => [412, true], + 'bad request' => [400, false], + 'server error' => [500, false], + ]; + } + + public function testConnectionErrorsRemainAnError(): void + { + $proxy = $this->createMock(XapiProxy::class); + $proxy->expects($this->any())->method('log')->willReturn($this->createMock(ilLogger::class)); + + $this->assertFalse( + (new XapiProxyResponse($proxy))->checkResponse( + ['state' => 'rejected', 'reason' => new Exception('connection refused')], + 'https://lrs.example.org/xapi' + ) + ); + } +}