Remove the session key as a hidden field in forms when session_manage…

…ment_via is "cookie_session".

This means that the session key is not visible anywhere in the DOM or in
URL parameters when using cookies for session management, and JavaScript
has no access to it.  This is something that malicious javascript can
exploit.  Of course this also means that webwork2's own javascript can't
access it either.  So for this to work, the rpc endpoints used by
webwork2 need to have cookies enabled so that the endpoints used by the
library browser and such still work.  However, the html2xml endpoint
still has cookies always disabled.  For the render_rpc endpoint, the
disableCookies parameter can be set to disable cookies.

Note that an authentication module can disable cookies by setting the
disable_cookies stash value.  This is how the html2xml endpoint disables
cookies.  The Cosign and Shibboleth authentication modules have been
adapted to use this as well, instead of overriding the `WeBWorK::Authen`
cookie methods.  In fact overriding those methods won't disable cookies
anymore entirely.  The Proctor authen module overrides those methods for
a different reason.  It does not disable cookies, but does not set the
user_id, key, and timestamp authentication values in the cookie.

Note that the "theme" hidden field has also been removed.  That isn't
even an authentication parameter, and doesn't work anyway.

htdocs/js/PGProblemEditor/pgproblemeditor.js

@@ -35,11 +35,12 @@
 	// Send a request to the server to save the temporary file for the currently edited file.
 	// This temporary file could be used for recovery, and is displayed if the page is reloaded.
 	const saveTempFile = () => {
-		const request_object = {
-			user: document.getElementById('hidden_user')?.value,
-			courseID: document.getElementsByName('courseID')[0]?.value,
-			key: document.getElementById('hidden_key')?.value
-		};
+		const request_object = { courseID: document.getElementsByName('courseID')[0]?.value };
+
+		const user = document.getElementsByName('user')[0];
+		if (user) request_object.user = user.value;
+		const sessionKey = document.getElementsByName('key')[0];
+		if (sessionKey) request_object.key = sessionKey.value;
 
 		request_object.rpc_command = 'saveFile';
 		request_object.outputFilePath = document.getElementsByName('temp_file_path')[0]?.value ?? '';
@@ -106,11 +107,12 @@
 
 	// Send a request to the server to perltidy the current PG code in the CodeMirror editor.
 	const tidyPGCode = () => {
-		const request_object = {
-			user: document.getElementById('hidden_user')?.value,
-			courseID: document.getElementsByName('courseID')[0]?.value,
-			key: document.getElementById('hidden_key')?.value
-		};
+		const request_object = { courseID: document.getElementsByName('courseID')[0]?.value };
+
+		const user = document.getElementsByName('user')[0];
+		if (user) request_object.user = user.value;
+		const sessionKey = document.getElementsByName('key')[0];
+		if (sessionKey) request_object.key = sessionKey.value;
 
 		request_object.rpc_command = 'tidyPGCode';
 		request_object.pgCode =
@@ -342,13 +344,18 @@
 				return;
 			}
 
+			const authenParams = {};
+			const user = document.getElementsByName('user')[0];
+			if (user) authenParams.user = user.value;
+			const sessionKey = document.getElementsByName('key')[0];
+			if (sessionKey) authenParams.key = sessionKey.value;
+
 			const isProblem = fileType && /problem/.test(fileType) ? 1 : 0;
 
 			renderProblem(
 				new URLSearchParams({
-					user: document.getElementById('hidden_user')?.value,
+					...authenParams,
 					courseID: document.getElementsByName('courseID')[0]?.value,
-					key: document.getElementById('hidden_key')?.value,
 					problemSeed: document.getElementById('action_view_seed_id')?.value ?? 1,
 					sourceFilePath: document.getElementsByName('edit_file_path')[0]?.value,
 					rawProblemSource:
@@ -359,8 +366,8 @@
 					showAnswerNumbers: 0,
 					// The set id is really only needed by set headers to get the correct dates for the set.
 					set_id: document.getElementsByName('hidden_set_id')[0]?.value ?? 'Unknown Set',
-					// This should not be an actual problem number in the set.  If so the current user's seed for that problem
-					// will be used instead of the seed from the editor form.
+					// This should not be an actual problem number in the set.  If so the current user's seed for that
+					// problem will be used instead of the seed from the editor form.
 					probNum: 0,
 					showHints: 1,
 					showSolutions: 1,
@@ -446,15 +453,20 @@
 		const controller = new AbortController();
 		const timeoutId = setTimeout(() => controller.abort(), 30000);
 
+		const authenParams = {};
+		const user = document.getElementsByName('user')[0];
+		if (user) authenParams.user = user.value;
+		const sessionKey = document.getElementsByName('key')[0];
+		if (sessionKey) authenParams.key = sessionKey.value;
+
 		try {
 			const response = await fetch(renderURL, {
 				method: 'post',
 				mode: 'same-origin',
 				signal: controller.signal,
 				body: new URLSearchParams({
-					user: document.getElementById('hidden_user')?.value,
+					...authenParams,
 					courseID: document.getElementsByName('courseID')[0]?.value,
-					key: document.getElementById('hidden_key')?.value,
 					problemSeed: document.getElementById('action_hardcopy_seed_id')?.value ?? 1,
 					sourceFilePath: document.getElementsByName('edit_file_path')[0]?.value,
 					rawProblemSource:

htdocs/js/ProblemGrader/problemgrader.js

@@ -73,8 +73,11 @@
 		saveButton.addEventListener('click', async () => {
 			const saveData = saveButton.dataset;
 
-			const user = document.getElementById('hidden_user').value;
-			const sessionKey = document.getElementById('hidden_key').value;
+			const authenParams = {};
+			const user = document.getElementsByName('user')[0];
+			if (user) authenParams.user = user.value;
+			const sessionKey = document.getElementsByName('key')[0];
+			if (sessionKey) authenParams.key = sessionKey.value;
 
 			const messageArea = document.getElementById(`grader_messages_problem${saveData.problemId}`);
 
@@ -98,8 +101,7 @@
 					method: 'post',
 					mode: 'same-origin',
 					body: new URLSearchParams({
-						user: user,
-						key: sessionKey,
+						...authenParams,
 						rpc_command: saveData.versionId !== '0' ? 'putProblemVersion' : 'putUserProblem',
 						courseID: saveData.courseId,
 						user_id: saveData.studentId,
@@ -154,8 +156,7 @@
 								const response = await fetch(basicWebserviceURL, {
 									method: 'post',
 									body: new URLSearchParams({
-										user: user,
-										key: sessionKey,
+										...authenParams,
 										rpc_command: 'putPastAnswer',
 										courseID: saveData.courseId,
 										answer_id: saveData.pastAnswerId,

htdocs/js/RenderProblem/renderproblem.js

@@ -11,8 +11,6 @@
 			if (iframe && iframe.iFrameResizer) iframe.contentDocument.location.replace('about:blank');
 
 			const ro = {
-				user: document.getElementById('hidden_user')?.value,
-				key: document.getElementById('hidden_key')?.value,
 				courseID: document.getElementsByName('hidden_course_id')[0]?.value,
 				language: document.getElementsByName('hidden_language')[0]?.value ?? 'en',
 				displayMode: document.getElementById('problem_displaymode').value ?? 'MathJax',
@@ -35,6 +33,11 @@
 				...renderOptions
 			};
 
+			const user = document.getElementsByName('user')[0];
+			if (user) ro.user = user.value;
+			const sessionKey = document.getElementsByName('key')[0];
+			if (sessionKey) ro.key = sessionKey.value;
+
 			const controller = new AbortController();
 			const timeoutId = setTimeout(() => controller.abort(), 10000);
 

htdocs/js/SetMaker/setmaker.js

@@ -48,12 +48,17 @@
 	};
 
 	const init_webservice = (command) => {
+		const authenParams = {};
+		const user = document.getElementsByName('user')[0];
+		if (user) authenParams.user = user.value;
+		const sessionKey = document.getElementsByName('key')[0]?.value;
+		if (sessionKey) authenParams.key = sessionKey.value;
+
 		return {
 			rpc_command: 'listLib',
 			library_name: 'Library',
 			command: 'buildtree',
-			user: document.getElementById('hidden_user')?.value,
-			key: document.getElementById('hidden_key')?.value,
+			...authenParams,
 			courseID: document.getElementsByName('hidden_course_id')[0]?.value,
 			rpc_command: command
 		};

htdocs/js/TagWidget/tagwidget.js

@@ -73,13 +73,18 @@
 	};
 
 	const createWebServiceObject = (command, values = {}) => {
+		const authenParams = {};
+		const user = document.getElementsByName('user')[0];
+		if (user) authenParams.user = user.value;
+		const sessionKey = document.getElementsByName('key')[0]?.value;
+		if (sessionKey) authenParams.key = sessionKey.value;
+
 		return {
 			rpc_command: command,
 			library_name: 'Library',
 			command: 'searchLib',
-			user: document.getElementById('hidden_user')?.value,
 			courseID: document.getElementsByName('hidden_course_id')[0]?.value,
-			key: document.getElementById('hidden_key')?.value,
+			...authenParams,
 			...values
 		};
 	};

lib/FormatRenderedProblem.pm

@@ -241,12 +241,13 @@ sub formatRenderedProblem {
 		lh                       => $lh,
 		rh_result                => $rh_result,
 		SITE_URL                 => $SITE_URL,
-		FORM_ACTION_URL          => $SITE_URL . $ws->c->webwork_url . '/render_rpc',
+		FORM_ACTION_URL          => $SITE_URL . $ws->c->webwork_url . '/' . $ws->c->current_route,
 		COURSE_LANG_AND_DIR      => get_lang_and_dir($formLanguage),
 		theme                    => $ws->{inputs_ref}{theme} || $ce->{defaultTheme},
-		courseID                 => $ws->{inputs_ref}{courseID} // '',
-		user                     => $ws->{inputs_ref}{user}     // '',
-		passwd                   => $ws->{inputs_ref}{passwd}   // '',
+		courseID                 => $ws->{inputs_ref}{courseID}       // '',
+		user                     => $ws->{inputs_ref}{user}           // '',
+		passwd                   => $ws->{inputs_ref}{passwd}         // '',
+		disableCookies           => $ws->{inputs_ref}{disableCookies} // '',
 		key                      => $ws->authen->{session_key},
 		PROBLEM_LANG_AND_DIR     => $PROBLEM_LANG_AND_DIR,
 		problemSeed              => $rh_result->{problem_seed} // $ws->{inputs_ref}{problemSeed} // 6666,

lib/WeBWorK.pm

@@ -90,16 +90,19 @@ async sub dispatch ($c) {
 	if ($c->current_route =~ /^(render_rpc|instructor_rpc|html2xml)$/) {
 		$c->{rpc} = 1;
 
+		$c->stash(disable_cookies => 1) if $c->current_route eq 'render_rpc' && $c->param('disableCookies');
+
 		# This provides compatibility for legacy html2xml parameters.
 		# This should be deleted when the html2xml endpoint is removed.
 		if ($c->current_route eq 'html2xml') {
+			$c->stash(disable_cookies => 1);
 			for ([ 'userID', 'user' ], [ 'course_password', 'passwd' ], [ 'session_key', 'key' ]) {
 				$c->param($_->[1], $c->param($_->[0])) if defined $c->param($_->[0]) && !defined $c->param($_->[1]);
 			}
 		}
 
 		# Get the courseID from the parameters for a remote procedure call.
-		$routeCaptures{courseID} = $c->param('courseID') if $c->param('courseID');
+		$routeCaptures{courseID} = $c->stash->{courseID} = $c->param('courseID') if $c->param('courseID');
 	}
 
 	# If this is the login phase of an LTI 1.3 login, then extract the courseID from the target_link_uri.

lib/WeBWorK/Authen.pm

@@ -329,7 +329,7 @@ sub get_credentials {
 	}
 
 	if (defined $cookieUser) {
-		$self->{user_id}           = trim($cookieUser);
+		$self->{user_id}           = $cookieUser;
 		$self->{session_key}       = $cookieKey;
 		$self->{cookie_timestamp}  = $cookieTimeStamp;
 		$self->{login_type}        = "normal";
@@ -468,7 +468,7 @@ sub maybe_send_cookie {
 	my $c    = $self->{c};
 	my $ce   = $c->{ce};
 
-	return if $c->{rpc};
+	return if $c->stash('disable_cookies');
 
 	my ($cookie_user, $cookie_key, $cookie_timestamp) = $self->fetchCookie;
 
@@ -511,7 +511,7 @@ sub maybe_send_cookie {
 
 sub maybe_kill_cookie {
 	my $self = shift;
-	return if $self->{c}{rpc};
+	return if $self->{c}->stash('disable_cookies');
 	$self->killCookie;
 	return;
 }
@@ -522,7 +522,7 @@ sub set_params {
 
 	$c->param('user',   $self->{user_id});
 	$c->param('key',    $self->{session_key});
-	$c->param('passwd', '') unless $c->{rpc};
+	$c->param('passwd', '') unless $c->{rpc} && $c->stash->{disable_cookies};
 
 	debug("params user='", $c->param("user"), "' key='", $c->param("key"), "'");
 
@@ -673,9 +673,8 @@ sub session {
 	my $c = $self->{c};
 
 	# If session_management_via is not "session_cookie" (so should be "key"), then use the database session.
-	if ($c->ce->{session_management_via} ne 'session_cookie') {
-		$c->stash->{'webwork2.database_session'} //= { user_id => $self->{user_id}, session => {} };
-		my $session = $c->stash->{'webwork2.database_session'}{session};
+	if ($c->ce->{session_management_via} ne 'session_cookie' || $c->stash('disable_cookies')) {
+		my $session = $c->stash->{'webwork2.database_session'} ? $c->stash->{'webwork2.database_session'}{session} : {};
 
 		# Note that the return values are the same as those returned by the
 		# Mojolicious::Controller::session method in the following cases.
@@ -726,7 +725,7 @@ sub store_session {
 		eval { $db->deleteKey($self->{user_id}) };
 	}
 
-	return if $self->{c}->ce->{session_management_via} ne 'session_cookie';
+	return if $self->{c}->ce->{session_management_via} ne 'session_cookie' || $self->{c}->stash('disable_cookies');
 
 	# The cookie will actually be sent by the next line of the Mojolcious::Controller::rendered method after the
 	# after_dispatch hook in which this method is called.
@@ -774,7 +773,7 @@ sub check_session {
 
 	if ($keyMatches && $timestampValid && $updateTimestamp) {
 		$Key->timestamp($currentTime);
-		$self->{c}->stash->{'webwork2.database_session'} = { $Key->toHash } if $keyMatches && $timestampValid;
+		$self->{c}->stash->{'webwork2.database_session'} = { $Key->toHash };
 	}
 
 	return (1, $keyMatches, $timestampValid);
@@ -814,7 +813,7 @@ sub fetchCookie {
 	my $c    = $self->{c};
 	my $ce   = $c->ce;
 
-	return if $c->{rpc};
+	return if $c->stash('disable_cookies');
 
 	my $userID    = $c->session->{user_id};
 	my $key       = $c->session->{key};
@@ -836,7 +835,7 @@ sub sendCookie {
 	my $c  = $self->{c};
 	my $ce = $c->ce;
 
-	return if $c->{rpc};
+	return if $c->stash('disable_cookies');
 
 	my $courseID = $c->stash('courseID');
 

lib/WeBWorK/Authen/Cosign.pm

@@ -48,6 +48,8 @@ sub get_credentials {
 	if ($ce->{cosignoff}) {
 		return $self->SUPER::get_credentials();
 	} else {
+		$c->stash(disable_cookies => 1);
+
 		if (defined($ENV{'REMOTE_USER'})) {
 			$self->{'user_id'} = $ENV{'REMOTE_USER'};
 			$self->{c}->param("user", $ENV{'REMOTE_USER'});
@@ -76,51 +78,13 @@ sub site_checkPassword {
 
 	if ($self->{c}->ce->{cosignoff}) {
 		return 0;
-		#return $self->SUPER::checkPassword( $userID, $clearTextPassword );
 	} else {
 		# this is easy; if we're here at all, we've authenticated
 		# through cosign
 		return 1;
 	}
 }
 
-# disable cookie functionality
-sub maybe_send_cookie {
-	my ($self, @args) = @_;
-	if ($self->{c}->ce->{cosignoff}) {
-		return $self->SUPER::maybe_send_cookie(@args);
-	} else {
-		# nothing to do here
-	}
-}
-
-sub fetchCookie {
-	my ($self, @args) = @_;
-	if ($self->{c}->ce->{cosignoff}) {
-		return $self->SUPER::fetchCookie(@args);
-	} else {
-		# nothing to do here
-	}
-}
-
-sub sendCookie {
-	my ($self, @args) = @_;
-	if ($self->{c}->ce->{cosignoff}) {
-		return $self->SUPER::sendCookie(@args);
-	} else {
-		# nothing to do here
-	}
-}
-
-sub killCookie {
-	my ($self, @args) = @_;
-	if ($self->{c}->ce->{cosignoff}) {
-		return $self->SUPER::killCookie(@args);
-	} else {
-		# nothing to do here
-	}
-}
-
 # this is a bit of a cheat, because it does the redirect away from the
 #   logout script or what have you, but I don't see a way around that.
 sub forget_verification {

lib/WeBWorK/Authen/Proctor.pm

@@ -198,7 +198,9 @@ sub set_params {
 sub create_session { }
 sub check_session  { }
 
-# Disable cookie functionality for proctors.
+# Prevent this module from setting or using cookie authentication parameters. This does not disable cookies.
+# Don't set the disable_cookies stash value for this because cookie session values still need to be set and used,
+# just not the authentication parameters user_id, key, and timestamp.
 sub maybe_send_cookie { }
 sub maybe_kill_cookie { }
 sub fetchCookie       { }