[FIX] #48285 CmiXapi: Handle xAPI document resource preconditions in the proxy - #12026
Open
marvimarv wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mantis Issue: https://mantis.ilias.de/view.php?id=48285
Problem
The xAPI proxy does not forward the
If-MatchandIf-None-Matchrequest headers to the LRS. Both are missing from the allowlist inXapiProxyRequest::createProxyRequest(). It also never relays theETagof a document back to the content.For the xAPI document resources (
activities/state,activities/profile,agents/profile) the specification (xAPI 1.0.3, "Concurrency") requires an LRS to reject aPUTon an already existing document that carries neither header with 409 Conflict.XapiProxyResponse::checkResponse()accepts only 200, 204 and 404, and replaces everything else with a fabricatedHTTP/1.1 412 Wrong Response.Against an LRS that follows the specification, every rewrite of a state document is therefore rejected, the learning progress is lost, and the content never learns why. For a learner the module simply restarts from the beginning. Learning Locker ignores both headers, so the defect does not surface there.
TinCanJS, which is bundled with common content players, treats the resources differently.
saveActivityProfile()andsaveAgentProfile()fall back toIf-None-Match: "*"when they know no revision, but the state write does not:Without a
lastSHA1it sends no precondition at all, and the state resource is exactly the one carrying the learning progress. A player can only supplylastSHA1if it received the document ETag beforehand, which the proxy does not relay either. Both halves of the defect reinforce each other.What this changes
If-MatchandIf-None-Matchare relayed to the LRS, and allowed in the CORS preflight.ETagof a document response is exposed to the content viaAccess-Control-Expose-Headers.If-Matchbuilt from the current ETag.An LRS that does not demand a precondition never answers 409, so point 4 causes no additional request there.
Testing
Verified end to end through a running ILIAS 10.10 against two real LRS, Yet Analytics lrsql and Learning Locker (
xapi-service2.1.10), with a real Articulate Storyline module, by clicking through the module, leaving it and resuming.Plus 18 unit tests. PHPStan reports no new findings, PHP-CS-Fixer is clean on all changed files.
Affected branches
The defect is present in release_9, release_10 (10.10), release_11 (11.3) and trunk. None of them forwards the headers or evaluates the ETag, and
checkResponse()masks the status identically. The proxy implementation differs considerably between branches though. Guzzle is used in 9 and 10, cURL from 11 onwards, and in trunksendCurlRequest()no longer receives the request object at all. So this cannot be cherry picked and has to be ported per branch.Transparency note: this patch was written with AI assistance. The analysis and all testing against both LRS were carried out and verified manually by me.