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

      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.

        Gliffy Diagrams

          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: