From 2ece2d0514d1124c23a8c2e24091ba71ec734900 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Sun, 18 Feb 2024 15:07:32 -0600 Subject: [PATCH] Remove the session key as a hidden field in forms when session_management_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 | 44 +++++++++------ htdocs/js/ProblemGrader/problemgrader.js | 13 ++--- htdocs/js/RenderProblem/renderproblem.js | 7 ++- htdocs/js/SetMaker/setmaker.js | 9 +++- htdocs/js/TagWidget/tagwidget.js | 9 +++- lib/FormatRenderedProblem.pm | 9 ++-- lib/WeBWorK.pm | 5 +- lib/WeBWorK/Authen.pm | 21 ++++---- lib/WeBWorK/Authen/Cosign.pm | 40 +------------- lib/WeBWorK/Authen/Proctor.pm | 4 +- lib/WeBWorK/Authen/Shibboleth.pm | 39 +------------- lib/WeBWorK/ContentGenerator.pm | 54 +++++++++++-------- lib/WeBWorK/ContentGenerator/ShowMeAnother.pm | 1 - lib/WeBWorK/Controller.pm | 2 + lib/WeBWorK/DB.pm | 2 +- templates/RPCRenderFormats/default.html.ep | 1 + 16 files changed, 115 insertions(+), 145 deletions(-) diff --git a/htdocs/js/PGProblemEditor/pgproblemeditor.js b/htdocs/js/PGProblemEditor/pgproblemeditor.js index 80dd2c138f..9c21c20764 100644 --- a/htdocs/js/PGProblemEditor/pgproblemeditor.js +++ b/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: diff --git a/htdocs/js/ProblemGrader/problemgrader.js b/htdocs/js/ProblemGrader/problemgrader.js index e400d5f1fa..70cbb92914 100644 --- a/htdocs/js/ProblemGrader/problemgrader.js +++ b/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, diff --git a/htdocs/js/RenderProblem/renderproblem.js b/htdocs/js/RenderProblem/renderproblem.js index ad61bdd8f1..9bdd58bfdf 100644 --- a/htdocs/js/RenderProblem/renderproblem.js +++ b/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); diff --git a/htdocs/js/SetMaker/setmaker.js b/htdocs/js/SetMaker/setmaker.js index afb1ea610c..b8f174914b 100644 --- a/htdocs/js/SetMaker/setmaker.js +++ b/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 }; diff --git a/htdocs/js/TagWidget/tagwidget.js b/htdocs/js/TagWidget/tagwidget.js index 27cbd206c1..db052bbe91 100644 --- a/htdocs/js/TagWidget/tagwidget.js +++ b/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 }; }; diff --git a/lib/FormatRenderedProblem.pm b/lib/FormatRenderedProblem.pm index 03fe636223..5caa548bdb 100644 --- a/lib/FormatRenderedProblem.pm +++ b/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, diff --git a/lib/WeBWorK.pm b/lib/WeBWorK.pm index 2c6c42ada2..5fcce651e7 100644 --- a/lib/WeBWorK.pm +++ b/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. diff --git a/lib/WeBWorK/Authen.pm b/lib/WeBWorK/Authen.pm index feaa292068..962dc3d9c5 100644 --- a/lib/WeBWorK/Authen.pm +++ b/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'); diff --git a/lib/WeBWorK/Authen/Cosign.pm b/lib/WeBWorK/Authen/Cosign.pm index 09f7e752a2..106386b296 100644 --- a/lib/WeBWorK/Authen/Cosign.pm +++ b/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,7 +78,6 @@ 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 @@ -84,43 +85,6 @@ sub site_checkPassword { } } -# 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 { diff --git a/lib/WeBWorK/Authen/Proctor.pm b/lib/WeBWorK/Authen/Proctor.pm index 56ccc0df35..ba02efd3c7 100644 --- a/lib/WeBWorK/Authen/Proctor.pm +++ b/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 { } diff --git a/lib/WeBWorK/Authen/Shibboleth.pm b/lib/WeBWorK/Authen/Shibboleth.pm index 79bf35642d..b6e5edc0da 100644 --- a/lib/WeBWorK/Authen/Shibboleth.pm +++ b/lib/WeBWorK/Authen/Shibboleth.pm @@ -64,6 +64,8 @@ sub get_credentials { return $self->SUPER::get_credentials(@_); } + $c->stash(disable_cookies => 1); + debug("Shib is on!"); # set external auth parameter so that Login.pm knows @@ -122,43 +124,6 @@ sub site_checkPassword { } } -# disable cookie functionality -sub maybe_send_cookie { - my ($self, @args) = @_; - if ($self->{c}->ce->{shiboff}) { - return $self->SUPER::maybe_send_cookie(@_); - } else { - # nothing to do here - } -} - -sub fetchCookie { - my ($self, @args) = @_; - if ($self->{c}->ce->{shiboff}) { - return $self->SUPER::fetchCookie(@_); - } else { - # nothing to do here - } -} - -sub sendCookie { - my ($self, @args) = @_; - if ($self->{c}->ce->{shiboff}) { - return $self->SUPER::sendCookie(@_); - } else { - # nothing to do here - } -} - -sub killCookie { - my ($self, @args) = @_; - if ($self->{c}->ce->{shiboff}) { - return $self->SUPER::killCookie(@_); - } 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 { diff --git a/lib/WeBWorK/ContentGenerator.pm b/lib/WeBWorK/ContentGenerator.pm index 22254e641d..ff2d3c5970 100644 --- a/lib/WeBWorK/ContentGenerator.pm +++ b/lib/WeBWorK/ContentGenerator.pm @@ -399,17 +399,22 @@ Create the link to the webwork installation landing page with a logo and alt tex =cut sub webwork_logo ($c) { - my $ce = $c->ce; - my $theme = $c->param('theme') || $ce->{defaultTheme}; - my $htdocs = $ce->{webwork_htdocs_url}; + my $ce = $c->ce; if ($c->authen->was_verified && !$c->authz->hasPermissions($c->param('user'), 'navigation_allowed')) { # If navigation is restricted for this user, then the webwork logo is not a link to the courses page. - return $c->tag('span', $c->image("$htdocs/themes/$theme/images/webwork_logo.svg", alt => 'WeBWorK')); + return $c->tag( + 'span', + $c->image( + "$ce->{webwork_htdocs_url}/themes/$ce->{defaultTheme}/images/webwork_logo.svg", + alt => 'WeBWorK' + ) + ); } else { return $c->link_to( - $c->image("$htdocs/themes/$theme/images/webwork_logo.svg", alt => $c->maketext('to courses page')) => - $ce->{webwork_url}); + $c->image("$ce->{webwork_htdocs_url}/themes/$ce->{defaultTheme}/images/webwork_logo.svg", + alt => $c->maketext('to courses page')) => $ce->{webwork_url} + ); } } @@ -420,12 +425,10 @@ Create the link to the host institution with a logo and alt text =cut sub institution_logo ($c) { - my $ce = $c->ce; - my $theme = $c->param("theme") || $ce->{defaultTheme}; - my $htdocs = $ce->{webwork_htdocs_url}; + my $ce = $c->ce; return $c->link_to( $c->image( - "$htdocs/themes/$theme/images/" . $ce->{institutionLogo}, + "$ce->{webwork_htdocs_url}/themes/$ce->{defaultTheme}/images/$ce->{institutionLogo}", alt => $c->maketext("to [_1] main web site", $ce->{institutionName}) ) => $ce->{institutionURL} ); @@ -448,24 +451,21 @@ the template is looked up in the course environment. sub content ($c) { my $ce = $c->ce; - my $theme = $c->param('theme') || $ce->{defaultTheme}; - $theme = $ce->{defaultTheme} if $theme =~ m!(?:^|/)\.\.(?:/|$)!; - my $layout = $ce->{defaultThemeTemplate} // 'system'; - my $layoutName = "$theme/$layout"; + my $layoutName = "$ce->{defaultTheme}/$layout"; # Attempt to prevent disaster when the theme layout file is missing. - unless (-r "$ce->{webworkDirs}{themes}/$theme/$layout.html.ep") { + unless (-r "$ce->{webworkDirs}{themes}/$ce->{defaultTheme}/$layout.html.ep") { if (-r "$ce->{webworkDirs}{themes}/math4/$layout.html.ep") { $layoutName = "math4/$layout"; - $theme = HTML::Entities::encode_entities($theme); + my $theme = HTML::Entities::encode_entities($ce->{defaultTheme}); warn "Theme $theme is not one of the available themes. " . 'Please check the theme configuration ' . 'in the files localOverrides.conf, course.conf and ' . "simple.conf and on the course configuration page.\n"; } else { - $theme = HTML::Entities::encode_entities($theme); + my $theme = HTML::Entities::encode_entities($ce->{defaultTheme}); die "Neither the theme $theme nor the defaultTheme math4 are available. " . 'Please notify your site administrator that the structure of the ' . 'themes directory needs attention.'; @@ -1056,6 +1056,8 @@ inputs that are created. =cut +# FIXME: Hidden fields have no need for an id attribute. Fix the javascript that finds these in by using the id, and +# remove the id here. Then the id_prefix hack isn't needed. The name does not need to be unique. sub hidden_fields ($c, @fields) { my %options = ref $fields[0] eq 'HASH' ? %{ shift @fields } : (); my $id_prefix = $options{id_prefix} // ''; @@ -1081,12 +1083,19 @@ authentication. An optional $id_prefix may be passed as the first argument of this method. +If session_management_via is "session_cookie" then the hidden authentication +fields that are return are for the "user" and the "effectiveUser". If +session_management_via is "key" then the "key" is added. + =cut +# FIXME: The "user" also should not be added to forms when session_management_via is "session_cookie". However, the +# user param is used everywhere to get the user id. That should be changed. sub hidden_authen_fields ($c, $id_prefix = undef) { - return $c->hidden_fields({ id_prefix => $id_prefix }, 'user', 'effectiveUser', 'key', 'theme') - if defined $id_prefix; - return $c->hidden_fields('user', 'effectiveUser', 'key', 'theme'); + my @fields = ('user', 'effectiveUser'); + push(@fields, 'key') if $c->ce->{session_management_via} ne 'session_cookie'; + return $c->hidden_fields({ id_prefix => $id_prefix }, @fields) if defined $id_prefix; + return $c->hidden_fields(@fields); } =item url_args(@fields) @@ -1122,9 +1131,9 @@ sub url_authen_args ($c) { # to reveal the user and key in the URL. Putting it there makes session # hijacking easier, in particular should a student share such a URL. if ($ce->{session_management_via} eq 'session_cookie') { - return $c->url_args('effectiveUser', 'theme'); + return $c->url_args('effectiveUser'); } else { - return $c->url_args('user', 'effectiveUser', 'key', 'theme'); + return $c->url_args('user', 'effectiveUser', 'key'); } } @@ -1203,7 +1212,6 @@ sub systemLink ($c, $urlpath, %options) { } $params{effectiveUser} = undef unless exists $params{effectiveUser}; - $params{theme} = undef unless exists $params{theme}; } my $url = $options{use_abs_url} ? $urlpath->to_abs : $urlpath; diff --git a/lib/WeBWorK/ContentGenerator/ShowMeAnother.pm b/lib/WeBWorK/ContentGenerator/ShowMeAnother.pm index 015cd85c3b..e36322fe61 100644 --- a/lib/WeBWorK/ContentGenerator/ShowMeAnother.pm +++ b/lib/WeBWorK/ContentGenerator/ShowMeAnother.pm @@ -35,7 +35,6 @@ async sub pre_header_initialize ($c) { my $problemNumber = $c->stash('problemID'); my $userName = $c->param('user'); my $effectiveUserName = $c->param('effectiveUser'); - my $key = $c->param('key'); my $editMode = $c->param('editMode'); # We want to run the existing pre_header_initialize with diff --git a/lib/WeBWorK/Controller.pm b/lib/WeBWorK/Controller.pm index 78ac6abbc2..ed49eceaf9 100644 --- a/lib/WeBWorK/Controller.pm +++ b/lib/WeBWorK/Controller.pm @@ -62,6 +62,8 @@ sub param ($c, $name = undef, $val = undef) { # Override the Mojolicious::Controller session method to set the cookie parameters # from the course environment the first time it is called. sub session ($c, @args) { + return if $c->stash('disable_cookies'); + # Initialize the cookie session the first time this is called. unless ($c->stash->{'webwork2.cookie_session_initialized'}) { $c->stash->{'webwork2.cookie_session_initialized'} = 1; diff --git a/lib/WeBWorK/DB.pm b/lib/WeBWorK/DB.pm index 02fa05f33a..c0bdefb04d 100644 --- a/lib/WeBWorK/DB.pm +++ b/lib/WeBWorK/DB.pm @@ -2356,7 +2356,7 @@ sub check_user_id { # (valid characters are [-a-zA-Z0-9_.,@]) # The (optional) second argument to checkKeyfields is to support versioned # (gateway) sets, which may include commas in certain fields (in particular, -# set names (e.g., setDerivativeGateway,v1) +# set names (e.g., setDerivativeGateway,v1)). sub checkKeyfields($;$) { my ($Record, $versioned) = @_; foreach my $keyfield ($Record->KEYFIELDS) { diff --git a/templates/RPCRenderFormats/default.html.ep b/templates/RPCRenderFormats/default.html.ep index a8440d131a..4007388527 100644 --- a/templates/RPCRenderFormats/default.html.ep +++ b/templates/RPCRenderFormats/default.html.ep @@ -76,6 +76,7 @@ %= hidden_field courseID => $courseID %= hidden_field user => $user %= hidden_field passwd => $passwd + %= hidden_field disableCookies => $disableCookies %= hidden_field displayMode => $displayMode %= hidden_field key => $key %= hidden_field outputformat => $formatName