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

Improve debugging when using default cleaning

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.5
    • Fix Version/s: 2.5
    • Component/s: Forms Library
    • Testing Instructions:
      Hide
      1. Ensure that the course form displays without any warnings and that course format options can be changed and saved
      2. Edit the course/edit_form.php, comment out this line:
        $mform->setType('idnumber', PARAM_RAW);
      3. VERIFY: that a DEVELOPER debug warning is thrown
      4. Change your debugging level lower.
      5. VERIFY: that a debugging warning is not thrown.
      6. Uncomment the line
      7. Try different forms and ensure they still work

      PLEASE NOTE: there may be more debugging warnings thrown around core with this. These are new bugs to address in new issues.

      Show
      Ensure that the course form displays without any warnings and that course format options can be changed and saved Edit the course/edit_form.php, comment out this line: $mform->setType('idnumber', PARAM_RAW); VERIFY: that a DEVELOPER debug warning is thrown Change your debugging level lower. VERIFY: that a debugging warning is not thrown. Uncomment the line Try different forms and ensure they still work PLEASE NOTE: there may be more debugging warnings thrown around core with this. These are new bugs to address in new issues.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-34311-master

      Description

      This is just an idea I wanted to explore...

      Right now people use mforms and it occasionally happens that devs forget to specify a PARAM type and we fall back to PARAM_RAW. This brings us relatively regular security issues. This seems preventable, if devs want PARAM_RAW the they can specify it manually and in other cases, we can throw a developer warning to catch it early.

      I attach a proof of concept patch below. It has the problem that we are throwing warnings in selects etc where not necessary, so we'd need to resolve that. Is there another reason we haven't considered this before?

      diff --git a/lib/formslib.php b/lib/formslib.php
      index 306c4e5..11b4292 100644
      --- a/lib/formslib.php
      +++ b/lib/formslib.php
      @@ -1466,6 +1466,7 @@ class MoodleQuickForm extends HTML_QuickForm_DHTMLRulesTableless {
                           $type = $this->_types[$key];
                       } else {
                           $type = PARAM_RAW;
      +                    debugging("Reverting to defaulting to DEFAULT cleaning of param $key", DEBUG_DEVELOPER);
                       }
                       if (is_array($s)) {
                           $submission[$key] = clean_param_array($s, $type, true);

        Attachments

          Issue Links

          There are no Sub-Tasks for this issue.

            Activity

              People

              Assignee:
              poltawski Dan Poltawski
              Reporter:
              poltawski Dan Poltawski
              Peer reviewer:
              Frédéric Massart
              Integrator:
              Damyon Wiese
              Tester:
              David Monllaó
              Participants:
              Component watchers:
              Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:
                Fix Release Date:
                14/May/13