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

validate_param badly broken

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.9.5, 3.0.3
    • Fix Version/s: None
    • Component/s: Libraries
    • Labels:
    • Affected Branches:
      MOODLE_29_STABLE, MOODLE_30_STABLE

      Description

      I need to talk about types here, so just to be clear when I say:

      • true/false - I mean the boolean values true and false
      • 'true'/'false' - I mean the strings 'true' and 'false'
      • 0/1 - I mean the integers 0 and 1
      • '0'/'1' - I mean the strings '0' and '1'

      When we clean_param with a PARAM_BOOL, we do this:

              case PARAM_BOOL:
                  // Convert to 1 or 0.
                  $tempstr = strtolower($param);
                  if ($tempstr === 'on' or $tempstr === 'yes' or $tempstr === 'true') {
                      $param = 1;
                  } else if ($tempstr === 'off' or $tempstr === 'no'  or $tempstr === 'false') {
                      $param = 0;
                  } else {
                      $param = empty($param) ? 0 : 1;
                  }
                  return $param;
      

      Which produces this mapping:

      in out
      true 1
      false 0
      1 1
      0 0
      'true' 1
      'false' 0
      '1' 1
      '0' 0
      'on' 1
      'off' 0
      'yes' 1
      'no' 0

      Then in validate_param we do this:

      ...
          } else if ((string)$param !== (string)$cleaned) {
              // Conversion to string is usually lossless.
              throw new invalid_parameter_exception($debuginfo);
          }
      

      Where $param is the original param, and $cleaned is the cleaned one. This is really broken with PARAM_BOOL. For example if you pass 'yes', then clean_param will produce 1, and of course 'yes' !== 1

      The worst case (and the case that I personally ran in to) is when boolean false is passed. When false is cast to a string, we get the empty string, but clean_param returns 0 so we end up comparing '' to 0, and it flakes out. Small snippet to demonstrate:

      function clean_param($param) {
      	// Convert to 1 or 0.
      	$tempstr = strtolower($param);
      	if ($tempstr === 'on' or $tempstr === 'yes' or $tempstr === 'true') {
      		$param = 1;
      	} else if ($tempstr === 'off' or $tempstr === 'no'  or $tempstr === 'false') {
      		$param = 0;
      	} else {
      		$param = empty($param) ? 0 : 1;
      	}
      	return $param;
      }
       
      $param = false;
      $cleaned = clean_param($param);
       
      if ((string)$param !== (string)$cleaned) {
      	echo "OH NO!";
      }
      

        Attachments

          Issue Links

            Activity

              People

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

                Dates

                • Created:
                  Updated: