Moodle
  1. Moodle
  2. MDL-26282

CMI element value seems to be wrongly cleaned up from backslashes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.0.1
    • Fix Version/s: None
    • Component/s: SCORM
    • Labels:
      None
    • Environment:
      Initial Reporter: unknown at this time
      Matteo: Apache/2.0.59 (Win32) PHP/5.2.12
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      15888

      Description

      See http://moodle.org/mod/forum/discuss.php?d=167672#p736900 and below for the full description and first analysis.
      See http://moodle.org/mod/forum/discuss.php?d=171515 for another report about the same issue.

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          I've replicated it using the Diagnostic SCO for SCORM runtime, using the SCORM 1.2 package. It affects the initialization of CMI elements values: in this test case it affects content bookmarking since the business logic of the content relies on the suspend_data lines-based payload i.e. it is not related with an issue in managing cmi...lesson_location.

          The data is not corrupted into the database and lib/weblib.php::addslashes_js() is not the root for such behaviour, being my first thought since it manipulates those data: 2nd thought, I was guessing it was related with the new way of playing around magic quotes... kind of, look at mod/scorm/locallib.php::scorm_get_tracks():

          Index: locallib.php
          ===================================================================
          RCS file: /cvsroot/moodle/moodle/mod/scorm/locallib.php,v
          retrieving revision 1.139
          diff -u -r1.139 locallib.php
          --- locallib.php	2 Feb 2011 13:20:22 -0000	1.139
          +++ locallib.php	5 Feb 2011 15:18:36 -0000
          @@ -436,7 +436,6 @@
                   $usertrack->timemodified = 0;
                   foreach ($tracks as $track) {
                       $element = $track->element;
          -            $track->value = stripslashes($track->value); // TODO: this is probably wrong, the stripslashes() has undefined meaning now; was this related to JS quoting or magic quotes?
                       $usertrack->{$element} = $track->value;
                       switch ($element) {
                           case 'cmi.core.lesson_status':
          

          Need some more testing to answer what the inline TODO was already reasoning about: JS quoting is already and correctly addressed by addslashes_js(). In Moodle 1.9 the function involved is lib/weblib.php::stripslashes_safe() which is really safe about such issue when magic_quotes_gpc in ON .

          Show
          Matteo Scaramuccia added a comment - I've replicated it using the Diagnostic SCO for SCORM runtime , using the SCORM 1.2 package. It affects the initialization of CMI elements values: in this test case it affects content bookmarking since the business logic of the content relies on the suspend_data lines-based payload i.e. it is not related with an issue in managing cmi...lesson_location . The data is not corrupted into the database and lib/weblib.php::addslashes_js() is not the root for such behaviour, being my first thought since it manipulates those data: 2nd thought, I was guessing it was related with the new way of playing around magic quotes ... kind of, look at mod/scorm/locallib.php::scorm_get_tracks() : Index: locallib.php =================================================================== RCS file: /cvsroot/moodle/moodle/mod/scorm/locallib.php,v retrieving revision 1.139 diff -u -r1.139 locallib.php --- locallib.php 2 Feb 2011 13:20:22 -0000 1.139 +++ locallib.php 5 Feb 2011 15:18:36 -0000 @@ -436,7 +436,6 @@ $usertrack->timemodified = 0; foreach ($tracks as $track) { $element = $track->element; - $track->value = stripslashes($track->value); // TODO: this is probably wrong, the stripslashes() has undefined meaning now; was this related to JS quoting or magic quotes? $usertrack->{$element} = $track->value; switch ($element) { case 'cmi.core.lesson_status': Need some more testing to answer what the inline TODO was already reasoning about: JS quoting is already and correctly addressed by addslashes_js() . In Moodle 1.9 the function involved is lib/weblib.php::stripslashes_safe() which is really safe about such issue when magic_quotes_gpc in ON .
          Hide
          Matteo Scaramuccia added a comment -

          Update: it looks like removing stripslashes() doesn't solve the issue, addslashes_js() has responsibilities too. It seems we should remove the first replace but this could break the function for other modules.

          It is easy to test what goes into the Database and than what comes out, and how it has been modified:

          • Publish an AICC/SCORM activity based on Claude Ostyn prodder;
          • Initialize, SetValue of cmi.suspend_data using
            Line1[ENTER]Line2[ENTER]Line3

            as value, Terminate. Note: before performing a Terminate, doing a GetValue after a multiline SetValue will return the same value, confirming that the issue is on the server side implementation of the "SCORM player", better, on the review of slashes management;

          • Exit the activity and re-enter;
          • Initialize, GetValue("cmi.suspend_data").
          Show
          Matteo Scaramuccia added a comment - Update: it looks like removing stripslashes() doesn't solve the issue, addslashes_js() has responsibilities too. It seems we should remove the first replace but this could break the function for other modules. It is easy to test what goes into the Database and than what comes out, and how it has been modified: Publish an AICC/SCORM activity based on Claude Ostyn prodder; Initialize, SetValue of cmi.suspend_data using Line1[ENTER]Line2[ENTER]Line3 as value, Terminate. Note: before performing a Terminate, doing a GetValue after a multiline SetValue will return the same value, confirming that the issue is on the server side implementation of the "SCORM player", better, on the review of slashes management; Exit the activity and re-enter; Initialize, GetValue("cmi.suspend_data").
          Hide
          Matteo Scaramuccia added a comment -

          After some more tests here is my patch proposal: https://github.com/scara/moodle/commit/9ac6ab7868babab41fbdeec63d7d1d8fbd6b5ca6
          It definitively removes extra slashes management from scorm_insert_track() and scorm_get_tracks(): now it works as expected.
          It should require confirmations also about the new management of "magic quotes" in the DB Layer: now it seems to be always active.

          Test case: see above.

          Show
          Matteo Scaramuccia added a comment - After some more tests here is my patch proposal: https://github.com/scara/moodle/commit/9ac6ab7868babab41fbdeec63d7d1d8fbd6b5ca6 It definitively removes extra slashes management from scorm_insert_track() and scorm_get_tracks() : now it works as expected. It should require confirmations also about the new management of "magic quotes" in the DB Layer: now it seems to be always active. Test case: see above.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          any ETA for a PULL request?

          Show
          Matteo Scaramuccia added a comment - Hi Dan, any ETA for a PULL request?
          Hide
          Dan Marsden added a comment -

          pull request submitted - remaining usages of addslashes_js seem to be correct - note this won't be reviewed for inclusion until next week. Thanks to all!

          Show
          Dan Marsden added a comment - pull request submitted - remaining usages of addslashes_js seem to be correct - note this won't be reviewed for inclusion until next week. Thanks to all!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: