Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-73826

Better handling of lti_load_cartridge() empty responses

    XMLWordPrintable

Details

    • MOODLE_311_STABLE, MOODLE_400_STABLE
    • MOODLE_311_STABLE
    • MDL-73826_311
    • Hide

      Requirements

      1. This issue requires to hack locally a little bit to verify the previous behavior is fixed.

      Instructions

      1. Init phpunit

        php admin/tool/phpunit/cli/init.php

      2. Run LTI's locallib tests:

        vendor/bin/phpunit mod/lti/tests/locallib_test.php

      3. Verify that it passes ok, without errors.
      4. Edit the mod/lti/locallib.php file.
      5. Go to the lti_load_cartridge function
      6. Change these lines, (so we sort of revert the fix):

            if (trim($response) === '') {
                throw new moodle_exception('errorreadingfile', '', '', $url);
            }
         
        TO:
         
            // if (trim($response) === '') {
                // throw new moodle_exception('errorreadingfile', '', '', $url);
            // }
        

      7. Run LTI's locallib tests again:

        vendor/bin/phpunit mod/lti/tests/locallib_test.php

      8. Verify that it fails, with one error about "ValueError" (that's the problem the fix solves):

        Failed asserting that exception of type "ValueError" matches expected
        exception "moodle_exception". Message was: "DOMDocument::loadXML():
        Argument #1 ($source) must not be empty"
        

      Show
      Requirements This issue requires to hack locally a little bit to verify the previous behavior is fixed. Instructions Init phpunit php admin/tool/phpunit/cli/init.php Run LTI's locallib tests: vendor/bin/phpunit mod/lti/tests/locallib_test.php Verify that it passes ok, without errors. Edit the mod/lti/locallib.php file. Go to the lti_load_cartridge function Change these lines, (so we sort of revert the fix): if (trim($response) === '') { throw new moodle_exception('errorreadingfile', '', '', $url); }   TO:   // if (trim($response) === '') { // throw new moodle_exception('errorreadingfile', '', '', $url); // } Run LTI's locallib tests again: vendor/bin/phpunit mod/lti/tests/locallib_test.php Verify that it fails, with one error about "ValueError" (that's the problem the fix solves): Failed asserting that exception of type "ValueError" matches expected exception "moodle_exception". Message was: "DOMDocument::loadXML(): Argument #1 ($source) must not be empty"

    Description

      It has been detected that, sometimes, the curl::get() call, when invoking a non-existing URL, can return an empty-string response.

      (note that curl returns the error as response when there is any problem, but in this case, both the response and the error are empty).

      This happens, for example, when running GHA jobs under windows and there isn't any web server running. The mod_lti generators set the URL to non-existing http://localhost/not/real/tool.php and, when lti_load_cartridge() is called, the curl::get() call returns empty string.

      Note that under other environments (linux, macos, local windows with bash...) that doesn't happen and the error is returned as response (as expected):

       Failed to connect to localhost port 80: Connection refused

      But, as said, at very least under GHA Windows environment, empty-string is returned (link to job output with the empty-string failures).

      Then, DOMDocument::loadXML($response) is called and that ends with an error, because that function doesn't accept empty contents to be processed.

      ValueError: DOMDocument::loadXML(): Argument #1 ($source) must not be empty

      And the problem is worse with PHP8, because the silent operator (@) is unable to do its work (maybe it was silenced ok with previous PHP versions).

      So we need to control that situation in advance, and whenever an empty response is returned... instead of allowing the loadXML() to happen, throw the moodle_exception earlier, without waiting for libxml_get_errors() to do its job some lines later (too late).

      Attachments

        Issue Links

          Activity

            People

              stronk7 Eloy Lafuente (stronk7)
              stronk7 Eloy Lafuente (stronk7)
              Ilya Tregubov Ilya Tregubov
              Jun Pataleta Jun Pataleta
              Angelia Dela Cruz Angelia Dela Cruz
              Jake Dallimore, Ilya Tregubov, Kevin Percy, Mathew May, Mihail Geshoski, Shamim Rezaie
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                14/Mar/22

                Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 3 hours, 40 minutes
                  3h 40m