Validate and safely quote course configuration values.

lib/WeBWorK/ConfigObject.pm

@@ -34,24 +34,49 @@ sub get_value ($self, $ce) {
 	return $value;
 }
 
+# The submitted value of this setting.  This is the html form element value unless an input_name was given, in which
+# case that request parameter is used instead.  In list context all values of the parameter are returned.
+sub new_value ($self) {
+	return $self->{c}->param($self->{input_name} // $self->{name});
+}
+
 # If use_current is true then return the current course environment value for this setting.
-# Otherwise use the value of the html form element.
+# Otherwise use the submitted value.
 sub convert_newval_source ($self, $use_current) {
 	if ($use_current) {
 		return $self->comparison_value($self->get_value($self->{c}->ce));
 	} else {
-		return $self->{c}->param($self->{name}) // '';
+		return scalar($self->new_value) // '';
 	}
 }
 
+sub quote ($self, $value) {
+	return "'" . ($value =~ s/([\\'])/\\$1/gr) . "'";
+}
+
+sub add_error ($self, $message) {
+	push(@{ $self->{errors} }, $message);
+	return;
+}
+
+sub errors ($self) {
+	return @{ $self->{errors} // [] };
+}
+
+# Record that the submitted value is invalid, and save the current course environment value instead.
+sub reject ($self, $oldval, $use_current, $message) {
+	return '' if $use_current;
+	$self->add_error($message);
+	return $self->save_string($oldval, 1);
+}
+
 # Bit of text to put in the configuration file.  The result should be an assignment which is executable by perl.  oldval
 # will be the value of the perl variable, and newval will be whatever an entry widget produces.
 sub save_string ($self, $oldval, $use_current = 0) {
 	my $newval = $self->convert_newval_source($use_current);
 	return '' if $self->comparison_value($oldval) eq $newval || ($self->{secret} && $newval eq SECRET_STRING);
 
-	$newval =~ s/['"`]//g;
-	return "\$$self->{var} = '$newval';\n";
+	return "\$$self->{var} = " . $self->quote($newval) . ";\n";
 }
 
 # A widget to interact with the user

lib/WeBWorK/ConfigObject/boolean.pm

@@ -10,7 +10,10 @@ sub display_value ($self, $val) {
 
 sub save_string ($self, $oldval, $use_current = 0) {
 	my $newval = $self->convert_newval_source($use_current);
-	return '' if $self->comparison_value($oldval) eq $newval;
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('Invalid boolean value "[_1]" for variable $[_2].', $newval, $self->{var}))
+		unless $newval =~ /^[01]\z/;
+	return '' if $self->comparison_value($oldval) == $newval;
 	return "\$$self->{var} = $newval;\n";
 }
 

lib/WeBWorK/ConfigObject/checkboxlist.pm

@@ -10,19 +10,23 @@ sub convert_newval_source ($self, $use_current) {
 	if ($use_current) {
 		return @{ $self->get_value($self->{c}->ce) };
 	} else {
-		return $self->{c}->param($self->{name});
+		return $self->new_value;
 	}
 }
 
 sub save_string ($self, $oldval, $use_current = 0) {
 	my @newvals = $self->convert_newval_source($use_current);
-	if ($self->{min} && scalar(@newvals) < $self->{min}) {
-		$self->{c}->addbadmessage(qq!You need to select at least $self->{min} option for "$self->{doc}".!);
-		return '' if $use_current;
-		return $self->save_string($oldval, 1);
-	}
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('You need to select at least [_1] option for "[_2]".', $self->{min}, $self->{doc}))
+		if $self->{min} && @newvals < $self->{min};
+
+	my %allowed = map { $_ => 1 } @{ $self->{values} };
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('Invalid value "[_1]" for variable $[_2].', join(', ', @newvals), $self->{var}))
+		if !$use_current && grep { !$allowed{$_} } @newvals;
+
 	return '' if $self->comparison_value($oldval) eq $self->comparison_value(\@newvals);
-	return "\$$self->{var} = [" . join(',', map {"'$_'"} @newvals) . "];\n";
+	return "\$$self->{var} = [" . join(',', map { $self->quote($_) } @newvals) . "];\n";
 }
 
 sub comparison_value ($self, $val) {

lib/WeBWorK/ConfigObject/list.pm

@@ -25,7 +25,7 @@ sub save_string ($self, $oldval, $use_current = 0) {
 	return '' if $newval eq $oldval;
 
 	# This is a new value.  Turn it back into a string and return it.
-	return "\$$self->{var} = [" . join(',', map {"'$_'"} map { $_ =~ s/['"`]//gr } split(',', $newval)) . "];\n";
+	return "\$$self->{var} = [" . join(',', map { $self->quote($_) } split(',', $newval)) . "];\n";
 }
 
 sub entry_widget ($self, $default, $is_secret = 0) {

lib/WeBWorK/ConfigObject/lms_context_id.pm

@@ -13,12 +13,13 @@ sub get_value ($self, $ce) {
 sub save_string ($self, $oldval, $use_current = 0) {
 	return '' if $use_current;
 
-	my $c  = $self->{c};
-	my $ce = $c->ce;
+	my $c      = $self->{c};
+	my $ce     = $c->ce;
+	my $newval = scalar $self->new_value;
 
 	# Unlike all other configuration settings, this one needs to be validated. It must be ensured that this course does
 	# not have the same context id as any other course on the server that has the same LTI configuration.
-	if (defined $c->param($self->{name}) && $c->param($self->{name}) ne '') {
+	if (defined $newval && $newval ne '') {
 		my @courseMaps = $c->db->getLTICourseMapsWhere;
 
 		# If a context id is going to be set and the course is configured for LTI 1.3, then make sure it has all of the
@@ -27,7 +28,7 @@ sub save_string ($self, $oldval, $use_current = 0) {
 		if ($ce->{LTIVersion} eq 'v1p3'
 			&& !($ce->{LTI}{v1p3}{PlatformID} && $ce->{LTI}{v1p3}{ClientID} && $ce->{LTI}{v1p3}{DeploymentID}))
 		{
-			$c->addbadmessage($c->maketext(
+			$self->add_error($c->maketext(
 				'An LMS context id is requested to be assigned to this course which is set to use LTI 1.3, but '
 					. 'this course is missing LTI 1.3 authentication parameters. This is not allowed, and so this '
 					. 'setting was not saved.'
@@ -38,7 +39,7 @@ sub save_string ($self, $oldval, $use_current = 0) {
 		for my $courseMap (@courseMaps) {
 			next
 				if $courseMap->course_id eq $c->ce->{courseName}
-				|| $c->param($self->{name}) ne $courseMap->lms_context_id;
+				|| $newval ne $courseMap->lms_context_id;
 
 			my $other_ce = eval { WeBWorK::CourseEnvironment->new({ courseName => $courseMap->course_id }) };
 
@@ -50,7 +51,7 @@ sub save_string ($self, $oldval, $use_current = 0) {
 						|| $ce->{LTI}{v1p1}{ConsumerKey} eq $other_ce->{LTI}{v1p1}{ConsumerKey})
 					)
 				{
-					$c->addbadmessage($c->maketext(
+					$self->add_error($c->maketext(
 						'The requested LMS Context ID is the same as that of another course that is also configured '
 							. 'to use LTI 1.1, but the consumer keys for both this course and that course are either '
 							. 'both not set or are the same. This is not allowed, and so this setting was not saved.',
@@ -64,7 +65,7 @@ sub save_string ($self, $oldval, $use_current = 0) {
 					&& $ce->{LTI}{v1p3}{ClientID} eq $other_ce->{LTI}{v1p3}{ClientID}
 					&& $ce->{LTI}{v1p3}{DeploymentID} eq $other_ce->{LTI}{v1p3}{DeploymentID})
 				{
-					$c->addbadmessage($c->maketext(
+					$self->add_error($c->maketext(
 						'The requested LMS Context ID is the same as that of another course that is also configured '
 							. 'to use LTI 1.3, but this course and that course have the same LTI 1.3 authentication '
 							. 'parameters. This is not allowed, and so this setting was not saved.'
@@ -74,11 +75,11 @@ sub save_string ($self, $oldval, $use_current = 0) {
 			}
 		}
 
-		eval { $c->db->setLTICourseMap($c->ce->{courseName}, $c->param($self->{name})) };
-		$c->addbadmessage($c->maketext('An error occurred saving the lms_context_id: [_1]', $@)) if $@;
+		eval { $c->db->setLTICourseMap($c->ce->{courseName}, $newval) };
+		$self->add_error($c->maketext('An error occurred saving the lms_context_id: [_1]', $@)) if $@;
 	} else {
 		eval { $c->db->deleteLTICourseMapWhere({ course_id => $c->ce->{courseName} }) };
-		$c->addbadmessage($c->maketext('An error occurred deletinglms_context_id: [_1]', $@)) if $@;
+		$self->add_error($c->maketext('An error occurred deletinglms_context_id: [_1]', $@)) if $@;
 	}
 
 	return '';

lib/WeBWorK/ConfigObject/number.pm

@@ -2,12 +2,10 @@ package WeBWorK::ConfigObject::number;
 use Mojo::Base 'WeBWorK::ConfigObject', -signatures;
 
 sub save_string ($self, $oldval, $use_current = 0) {
-	my $newval = $self->convert_newval_source($use_current) =~ s/['"`]//gr;
-	if ($newval !~ m/^[-+]?(\d+(\.\d*)?|\.\d+)$/) {
-		$self->{c}->addbadmessage(qq{Invalid numeric value "$newval" for variable \$$self->{var}.  }
-				. 'Reverting to the system default value.');
-		return '';
-	}
+	my $newval = $self->convert_newval_source($use_current);
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('Invalid numeric value "[_1]" for variable $[_2].', $newval, $self->{var}))
+		unless $newval =~ m/^[-+]?(\d+(\.\d*)?|\.\d+)$/;
 
 	return '' if $self->comparison_value($oldval) == +$newval;
 	return "\$$self->{var} = $newval;\n";

lib/WeBWorK/ConfigObject/permission.pm

@@ -15,8 +15,11 @@ sub display_value ($self, $val) {
 
 sub save_string ($self, $oldval, $use_current = 0) {
 	my $newval = $self->convert_newval_source($use_current);
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('Invalid permission level "[_1]" for variable $[_2].', $newval, $self->{var}))
+		unless $use_current || exists $self->{c}->ce->{userRoles}{$newval};
 	return '' if $self->comparison_value($oldval) eq $newval;
-	return "\$$self->{var} = '$newval';\n";
+	return "\$$self->{var} = " . $self->quote($newval) . ";\n";
 }
 
 sub entry_widget ($self, $default, $is_secret = 0) {

lib/WeBWorK/ConfigObject/permission_checkboxlist.pm

@@ -16,19 +16,23 @@ sub convert_newval_source ($self, $use_current) {
 			return @{ role_and_above($self->{c}->ce->{userRoles}, $self->get_value($self->{c}->ce)) };
 		}
 	} else {
-		return $self->{c}->param($self->{name});
+		return $self->new_value;
 	}
 }
 
 sub save_string ($self, $oldval, $use_current = 0) {
 	my @newvals = $self->convert_newval_source($use_current);
-	if ($self->{min} && scalar(@newvals) < $self->{min}) {
-		$self->{c}->addbadmessage("You need to select at least $self->{min} display mode.");
-		return '' if $use_current;
-		return $self->save_string($oldval, 1);
-	}
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('You need to select at least [_1] option for "[_2]".', $self->{min}, $self->{doc}))
+		if $self->{min} && @newvals < $self->{min};
+
+	my $userRoles = $self->{c}->ce->{userRoles};
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('Invalid value "[_1]" for variable $[_2].', join(', ', @newvals), $self->{var}))
+		if !$use_current && grep { $_ eq 'nobody' || !exists $userRoles->{$_} } @newvals;
+
 	return '' if $self->comparison_value($oldval) eq $self->comparison_value(\@newvals);
-	return "\$$self->{var} = [" . join(',', map {"'$_'"} @newvals) . "];\n";
+	return "\$$self->{var} = [" . join(',', map { $self->quote($_) } @newvals) . "];\n";
 }
 
 sub comparison_value ($self, $val) {

lib/WeBWorK/ConfigObject/popuplist.pm

@@ -10,8 +10,11 @@ sub display_value ($self, $val) {
 
 sub save_string ($self, $oldval, $use_current = 0) {
 	my $newval = $self->convert_newval_source($use_current);
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('Invalid value "[_1]" for variable $[_2].', $newval, $self->{var}))
+		unless $use_current || grep { $_ eq $newval } @{ $self->{values} };
 	return '' if $self->comparison_value($oldval) eq $newval;
-	return ("\$$self->{var} = '$newval';\n");
+	return "\$$self->{var} = " . $self->quote($newval) . ";\n";
 }
 
 sub entry_widget ($self, $default, $is_secret = 0) {

lib/WeBWorK/ConfigObject/setting.pm

@@ -16,7 +16,7 @@ sub get_value ($self, $ce) {
 # so that it is not represented in the course's simple.conf file.
 sub save_string ($self, $oldval, $use_current = 0) {
 	return '' if $use_current;
-	$self->{c}->db->setSettingValue($self->{var}, scalar $self->{c}->param($self->{var}));
+	$self->{c}->db->setSettingValue($self->{var}, scalar $self->new_value);
 	return '';
 }
 

lib/WeBWorK/ConfigObject/time.pm

@@ -7,10 +7,9 @@ sub save_string ($self, $oldval, $use_current = 0) {
 	my $newval = $self->convert_newval_source($use_current);
 	return '' if $self->comparison_value($oldval) eq $newval;
 
-	if ($newval !~ /^(01|1|02|2|03|3|04|4|05|5|06|6|07|7|08|8|09|9|10|11|12):[0-5]\d(am|pm|AM|PM)$/) {
-		$self->{c}->addbadmessage(qq{String "$newval" is not a valid time.  Reverting to the system default value.});
-		return '';
-	}
+	my ($hour, $ampm) = $newval =~ /^\s*(\d{1,2})\s*:\s*[0-5]\d\s*(am|pm|AM|PM)?\s*\z/;
+	return $self->reject($oldval, $use_current, $self->{c}->maketext('String "[_1]" is not a valid time.', $newval))
+		unless defined $hour && ($ampm ? $hour >= 1 && $hour <= 12 : $hour <= 23);
 
 	return "\$$self->{var} = '$newval';\n";
 }

lib/WeBWorK/ConfigObject/timezone.pm

@@ -9,13 +9,11 @@ sub save_string ($self, $oldval, $use_current = 0) {
 	my $newval = $self->convert_newval_source($use_current);
 	return '' if $self->comparison_value($oldval) eq $newval;
 
-	if (not DateTime::TimeZone->is_valid_name($newval)) {
-		$self->{c}->addbadmessage("String '$newval' is not a valid time zone.  Reverting to the system default value.");
-		return '';
-	}
+	return $self->reject($oldval, $use_current,
+		$self->{c}->maketext('String "[_1]" is not a valid time zone.', $newval))
+		unless DateTime::TimeZone->is_valid_name($newval);
 
-	$newval =~ s/['"`]//g;
-	return "\$$self->{var} = '$newval';\n";
+	return "\$$self->{var} = " . $self->quote($newval) . ";\n";
 }
 
 1;

lib/WeBWorK/ConfigValues.pm

@@ -14,7 +14,7 @@ allowed.
 
 =item text
 
-A text string (no quotes allowed).
+A text string.
 
 =item number
 
@@ -62,6 +62,7 @@ This is specifically for The LMS context id used for content selection.
 =cut
 
 use Mojo::File qw(path);
+use XML::LibXML;
 
 use WeBWorK::Utils qw(x);
 

lib/WeBWorK/ContentGenerator/API/CourseActions.pm

@@ -12,6 +12,7 @@ use WeBWorK::Utils                   qw(cryptPassword isValidEmailAddress);
 use WeBWorK::Utils::CourseManagement qw(addCourse);
 use WeBWorK::Utils::Files            qw(surePathToFile path_is_subdir);
 use WeBWorK::ConfigValues            qw(getConfigValues);
+use WeBWorK::CourseConfig            qw(findConfigObject canChangeConfig buildSimpleConfig writeSimpleConfig);
 use WeBWorK::Debug                   qw(debug);
 
 our @apiCalls = qw(
@@ -441,51 +442,19 @@ sub updateSetting ($c) {
 	return $c->renderError('You do not have permission for the updateSetting API command.')
 		unless $c->authz->hasPermissions($c->authen->{user_id}, 'manage_course_files');
 
-	my $ce = $c->ce;
+	my $ce     = $c->ce;
+	my $setVar = $c->req->param('var') // '';
 
-	# FIXME: There is no check in this method that the var and value passed in are valid.
-	my $setVar   = $c->req->param('var');
-	my $setValue = $c->req->param('value');
-
-	my $filename = "$ce->{courseDirs}{root}/simple.conf";
-
-	my $fileoutput = "#!perl
-# This file is automatically generated by WeBWorK's web-based
-# configuration module.  Do not make changes directly to this
-# file.  It will be overwritten the next time configuration
-# changes are saved.\n\n";
-
-	# Read in the file
-	open(my $DAT, '<', $filename)
-		or return $c->renderError("Unable to read $filename. "
-			. "Ensure that the file exists and the server has write permission for this file.");
-	my @raw_data = <$DAT>;
-	close($DAT);
-
-	my $varFound = 0;
-
-	for my $line (@raw_data) {
-		chomp $line;
-		if ($line =~ /^\$/) {
-			my @tmp = split(/\$/, $line);
-			my ($var, $value) = split(/\s+=\s+/, $tmp[1]);
-			if ($var eq $setVar) {
-				$fileoutput .= "\$$var = $setValue;\n";
-				$varFound = 1;
-			} else {
-				# The value includes the semicolon that hopefully was in the file.
-				$fileoutput .= "\$$var = $value\n";
-			}
-		}
-	}
+	return $c->renderError("Invalid course setting $setVar.") unless findConfigObject($c, $setVar);
+	return $c->renderError("You do not have permission to change the course setting $setVar.")
+		unless canChangeConfig($c, $setVar);
 
-	$fileoutput .= "\$$setVar = $setValue;\n" unless $varFound;
+	my ($fileoutput, $errors) = buildSimpleConfig($c, var => $setVar, input_name => 'value');
+	return $c->renderError(join(' ', @$errors)) if @$errors;
 
-	open(my $OUTPUTFILE, '>', $filename)
-		or return $c->renderError(
-			"Unable to write to $filename. Ensure that the server has write permission for this file.");
-	print $OUTPUTFILE $fileoutput;
-	close $OUTPUTFILE;
+	return $c->renderError("Unable to write to $ce->{courseDirs}{root}/simple.conf. "
+			. 'Ensure that the server has write permission for this file.')
+		unless writeSimpleConfig($ce, $fileoutput);
 
 	return $c->render(json => { message => 'Successfully updated course setting' });
 }