Moodle

String datatypes defined in datamodels/scorm_13.js.php reject newlines

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.8
  • Fix Version/s: 1.8
  • Component/s: SCORM
  • Labels:
    None
  • Affected Branches:
    MOODLE_18_STABLE
  • Fixed Branches:
    MOODLE_18_STABLE

Description

Definitions like

CMIString200 = '^.{0,200}$';

reject followinf Unicode chars:
U+000A LINE FEED
U+000D CARRIAGE RETURN
U+2028 LINE SEPARATOR
U+2029 PARAGRAPH SEPARATOR

An attempt to call SetValue(param_name, str_value), where str_value contains any of these chars fails.

What is the reason for such restriction?

Issue Links

This issue has been marked as being related by:
MDL-8193 Incorrect handling of quotes in SetValue processing Major Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda (skodak) added a comment -

I guess the main problem is in following code from SetValue:
eval(element+'="'value'";');

which can not work for value containing quotes enters, etc. I am no JS expert but IMHO it could be solved by:
element = element+String(value);
right?

In anycase if there are any quotes or newlines there is a new PHP function addslashes_js() in weblib.php that should be used whenever you construct javascript strings from PHP code

I am attaching patch for Scorm1.2 and 1.3 that with changes described above.

Show
Petr Škoda (skodak) added a comment - I guess the main problem is in following code from SetValue: eval(element+'="'value'";'); which can not work for value containing quotes enters, etc. I am no JS expert but IMHO it could be solved by: element = element+String(value); right? In anycase if there are any quotes or newlines there is a new PHP function addslashes_js() in weblib.php that should be used whenever you construct javascript strings from PHP code I am attaching patch for Scorm1.2 and 1.3 that with changes described above.
Hide
Permalink
Dmitry Severin added a comment -

> guess the main problem is in following code from SetValue:
> eval(element+'="'value'";');
> which can not work for value containing quotes enters, etc. I am no JS expert but IMHO it could be solved by:
> element = element+String(value);
> right?

wrong.
calls to eval(element+'="'value'";');
are used to dynamically set properties of cmi object in API instance, e.g. when SCO calls API.LMSSetValue("cmi.suspend_data", "some-sco-data-to-resume") JavaScript runtime evaluates
eval('cmi.suspend_data="some-sco-data-to-resume";')
so that evals should really be smth like
eval(element+'="' + value.replace( /(
|")/g, "
$1") +'";');

But rejecting newlines is just result of using regexp to validate length.

Show
Dmitry Severin added a comment - > guess the main problem is in following code from SetValue: > eval(element+'="'value'";'); > which can not work for value containing quotes enters, etc. I am no JS expert but IMHO it could be solved by: > element = element+String(value); > right? wrong. calls to eval(element+'="'value'";'); are used to dynamically set properties of cmi object in API instance, e.g. when SCO calls API.LMSSetValue("cmi.suspend_data", "some-sco-data-to-resume") JavaScript runtime evaluates eval('cmi.suspend_data="some-sco-data-to-resume";') so that evals should really be smth like eval(element+'="' + value.replace( /(
|")/g, "
$1") +'";'); But rejecting newlines is just result of using regexp to validate length.
Hide
Permalink
Petr Škoda (skodak) added a comment -

yes, you are correct.
I guess also quotes should be replaced, right?

Show
Petr Škoda (skodak) added a comment - yes, you are correct. I guess also quotes should be replaced, right?
Hide
Permalink
Dmitry Severin added a comment -

actually, simply changing to
eval(element+'=value;')
should work, without using regexp replacing of quotes.

Besides, there's one more bug, related to use of escape() js function to store data.
If string value has '+' (plus sign), it won't be escaped when sent to server, so it will be interpreted on server side as a space, and e.g. when one calls API.LMSSetValue("cmi.comments", "test+plus+sign") it will be stored in database as "test plus sign". Consider using encodeURIComponent() instead of escape() in functions CollectData (datamodels/*/js.php)

But still, what's the reason to reject newlines?

Show
Dmitry Severin added a comment - actually, simply changing to eval(element+'=value;') should work, without using regexp replacing of quotes. Besides, there's one more bug, related to use of escape() js function to store data. If string value has '+' (plus sign), it won't be escaped when sent to server, so it will be interpreted on server side as a space, and e.g. when one calls API.LMSSetValue("cmi.comments", "test+plus+sign") it will be stored in database as "test plus sign". Consider using encodeURIComponent() instead of escape() in functions CollectData (datamodels/*/js.php) But still, what's the reason to reject newlines?
Hide
Permalink
Petr Škoda (skodak) added a comment -

sure - eval(element+'=value;') is best, hmm what was I thinking?

Show
Petr Škoda (skodak) added a comment - sure - eval(element+'=value;') is best, hmm what was I thinking?
Hide
Permalink
Roberto Pinna added a comment -

Changed all escape() to encodeURIcomponent().
changed CMIStrings to '^.{0,xxxx}$'
Please let me know if it's all ok.

Bobo.

Show
Roberto Pinna added a comment - Changed all escape() to encodeURIcomponent(). changed CMIStrings to '^.{0,xxxx}$' Please let me know if it's all ok. Bobo.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.5#665-sha1:422aead) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.