From b144c6f3bc79edf003af0ca01529cb41f5c3cd65 Mon Sep 17 00:00:00 2001 From: Jan-Hendrik Willms <tleilax+studip@gmail.com> Date: Fri, 30 Aug 2024 11:30:47 +0000 Subject: [PATCH] fix incorrect usages of CSRFProtection::verifySecurityToken(), fixes #4548 Closes #4548 Merge request studip/studip!3343 --- app/controllers/admin/statusgroups.php | 4 +- app/controllers/blubber.php | 11 +-- app/controllers/calendar/calendar.php | 6 +- app/controllers/course/basicdata.php | 19 ++-- app/controllers/course/grouping.php | 2 +- app/controllers/help_content.php | 8 +- app/controllers/news.php | 4 +- app/controllers/settings/privacy.php | 4 +- app/controllers/shared/contacts.php | 6 +- app/controllers/tour.php | 10 +- app/views/admin/statusgroups/_group.php | 119 ++++++++++++++---------- app/views/course/basicdata/view.php | 16 ++-- app/views/shared/contacts/details.php | 7 +- app/views/shared/contacts/range.php | 7 +- lib/showNews.inc.php | 8 +- 15 files changed, 127 insertions(+), 104 deletions(-) diff --git a/app/controllers/admin/statusgroups.php b/app/controllers/admin/statusgroups.php index 22970d692ac..5851d4843cd 100644 --- a/app/controllers/admin/statusgroups.php +++ b/app/controllers/admin/statusgroups.php @@ -242,7 +242,7 @@ class Admin_StatusgroupsController extends AuthenticatedController $this->check('edit'); $this->group = new Statusgruppen($group_id); if (Request::submitted('confirm')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); // move all subgroups to the parent $children = SimpleORMapCollection::createFromArray($this->group->children); @@ -268,7 +268,7 @@ class Admin_StatusgroupsController extends AuthenticatedController $this->check('edit'); $this->group = new Statusgruppen($group_id); if (Request::submitted('confirm')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $this->group->sortMembersAlphabetic(); $this->redirect('admin/statusgroups/index#group-' . $group_id); } diff --git a/app/controllers/blubber.php b/app/controllers/blubber.php index 6588014be44..b9e039a0864 100644 --- a/app/controllers/blubber.php +++ b/app/controllers/blubber.php @@ -149,17 +149,16 @@ class BlubberController extends AuthenticatedController public function delete_action($thread_id) { + CSRFProtection::verifyUnsafeRequest(); + $this->thread = BlubberThread::find($thread_id); if (!$this->thread->isWritable()) { throw new AccessDeniedException(); } - if (Request::isPost()) { - CSRFProtection::verifySecurityToken(); - $this->thread->delete(); - PageLayout::postSuccess(_('Der Blubber wurde gelöscht.')); - } + + $this->thread->delete(); + PageLayout::postSuccess(_('Der Blubber wurde gelöscht.')); $this->redirect('blubber/index'); - return; } public function write_to_action($user_id = null) diff --git a/app/controllers/calendar/calendar.php b/app/controllers/calendar/calendar.php index cc29e5524be..6923f8f8cd1 100644 --- a/app/controllers/calendar/calendar.php +++ b/app/controllers/calendar/calendar.php @@ -812,7 +812,7 @@ class Calendar_CalendarController extends AuthenticatedController public function import_file_action() { if (Request::submitted('import')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $range_id = Context::getId() ?? User::findCurrent()->id; $calendar_import = new ICalendarImport($range_id); $calendar_import->convertPublicToPrivate(Request::bool('import_privat', false)); @@ -928,13 +928,13 @@ class Calendar_CalendarController extends AuthenticatedController { $this->short_id = null; if (Request::submitted('delete_id')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); IcalExport::deleteKey(User::findCurrent()->id); PageLayout::postSuccess(_('Die Adresse, unter der Ihre Termine abrufbar sind, wurde gelöscht')); } if (Request::submitted('new_id')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $this->short_id = IcalExport::setKey(User::findCurrent()->id); PageLayout::postSuccess(_('Eine Adresse, unter der Ihre Termine abrufbar sind, wurde erstellt.')); } else { diff --git a/app/controllers/course/basicdata.php b/app/controllers/course/basicdata.php index 62385e435d3..3d920c3d8c9 100644 --- a/app/controllers/course/basicdata.php +++ b/app/controllers/course/basicdata.php @@ -424,10 +424,11 @@ class Course_BasicdataController extends AuthenticatedController $text = ''; } if ($newstatus !== '' && $text !== '') { - $widget->addLink($text, + $widget->addLink( + $text, $this->url_for('course/basicdata/switchdeputy', $this->course_id, $newstatus), Icon::create('persons') - ); + )->asButton(); } } if (Config::get()->ALLOW_DOZENT_DELETE || $GLOBALS['perm']->have_perm('admin')) { @@ -455,7 +456,7 @@ class Course_BasicdataController extends AuthenticatedController { global $perm; - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $course_number_format = Config::get()->COURSE_NUMBER_FORMAT; $sem = Seminar::getInstance($course_id); @@ -593,7 +594,7 @@ class Course_BasicdataController extends AuthenticatedController public function add_member_action($course_id, $status = 'dozent') { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); // load MultiPersonSearch object $mp = MultiPersonSearch::load("add_member_{$status}{$course_id}"); @@ -851,9 +852,9 @@ class Course_BasicdataController extends AuthenticatedController */ public function priorityupfor_action($course_id, $user_id, $status = "dozent") { - global $user, $perm; + global $perm; - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $sem = Seminar::getInstance($course_id); $this->msg = []; @@ -888,9 +889,9 @@ class Course_BasicdataController extends AuthenticatedController */ public function prioritydownfor_action($course_id, $user_id, $status = "dozent") { - global $user, $perm; + global $perm; - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $sem = Seminar::getInstance($course_id); $this->msg = []; @@ -918,7 +919,7 @@ class Course_BasicdataController extends AuthenticatedController public function switchdeputy_action($course_id, $newstatus) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); switch($newstatus) { case 'dozent': diff --git a/app/controllers/course/grouping.php b/app/controllers/course/grouping.php index 2ff3c43a4b2..b7e4e320a75 100644 --- a/app/controllers/course/grouping.php +++ b/app/controllers/course/grouping.php @@ -506,7 +506,7 @@ class Course_GroupingController extends AuthenticatedController */ public function add_members_action() { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $fail = []; // Iterate over selected courses... diff --git a/app/controllers/help_content.php b/app/controllers/help_content.php index 4162d85e216..c2025f15125 100644 --- a/app/controllers/help_content.php +++ b/app/controllers/help_content.php @@ -158,7 +158,7 @@ class HelpContentController extends AuthenticatedController */ public function store_action($id = '') { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $content_id = md5(uniqid('help_content', 1)); $create_new_content = false; @@ -244,14 +244,16 @@ class HelpContentController extends AuthenticatedController */ public function delete_action($id) { - CSRFProtection::verifySecurityToken(); PageLayout::setTitle(_('Hilfe-Text löschen')); $this->help_content = HelpContent::GetContentByID($id); if (is_object($this->help_content)) { if (Request::submitted('delete_help_content')) { - PageLayout::postMessage(MessageBox::success(sprintf(_('Der Hilfe-Text zur Route "%s" wurde gelöscht.'), htmlReady($this->help_content->route)))); + CSRFProtection::verifyUnsafeRequest(); + $this->help_content->delete(); + PageLayout::postSuccess(sprintf(_('Der Hilfe-Text zur Route "%s" wurde gelöscht.'), htmlReady($this->help_content->route))); + $this->response->add_header('X-Dialog-Close', 1); $this->render_nothing(); return; diff --git a/app/controllers/news.php b/app/controllers/news.php index 96ecd927b99..89b6c6ed57d 100644 --- a/app/controllers/news.php +++ b/app/controllers/news.php @@ -97,8 +97,8 @@ class NewsController extends StudipController } // Check if user wrote a comment - if (Request::submitted('accept') && trim(Request::get('comment_content')) && Request::isPost()) { - CSRFProtection::verifySecurityToken(); + if (Request::submitted('accept') && trim(Request::get('comment_content'))) { + CSRFProtection::verifyUnsafeRequest(); $news_id = Request::get('comsubmit'); $comment = StudipComment::create([ diff --git a/app/controllers/settings/privacy.php b/app/controllers/settings/privacy.php index e61e4be9091..8d63ad84108 100644 --- a/app/controllers/settings/privacy.php +++ b/app/controllers/settings/privacy.php @@ -64,7 +64,7 @@ class Settings_PrivacyController extends Settings_SettingsController */ public function global_action() { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $visibility = Request::option('global_visibility'); @@ -183,7 +183,7 @@ class Settings_PrivacyController extends Settings_SettingsController */ public function homepage_action() { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); // If no bulk action is performed set all visibilitysettings seperately if (!$this->bulk()) { diff --git a/app/controllers/shared/contacts.php b/app/controllers/shared/contacts.php index 963e60bcb5e..79396c7502b 100644 --- a/app/controllers/shared/contacts.php +++ b/app/controllers/shared/contacts.php @@ -485,7 +485,7 @@ class Shared_ContactsController extends MVVController $this->ext_contact = $ext_contact; if (Request::submitted('store_ansprechpartner')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); if (!$user_id) { if (Request::get('exansp_name')) { @@ -585,7 +585,7 @@ class Shared_ContactsController extends MVVController } public function store_ansprechpartner_action ($contact_range_id, $origin = 'index') { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $contact_range = MvvContactRange::find($contact_range_id); if (!$contact_range) { @@ -621,7 +621,7 @@ class Shared_ContactsController extends MVVController public function delete_range_action($contact_range_id) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $range = MvvContactRange::find($contact_range_id); $contact = $range->contact; diff --git a/app/controllers/tour.php b/app/controllers/tour.php index 5150c6c503d..46052b5ad8b 100644 --- a/app/controllers/tour.php +++ b/app/controllers/tour.php @@ -211,7 +211,7 @@ class TourController extends AuthenticatedController } // delete tour if (Request::option('confirm_delete_tour')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $this->delete_tour(Request::option('tour_id')); } // load tours @@ -370,7 +370,7 @@ class TourController extends AuthenticatedController $this->tour = new HelpTour($tour_id); if (Request::submitted('yes')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $this->response->add_header('X-Action', 'complete'); $this->tour->delete(); } elseif (Request::submitted('no')) { @@ -401,7 +401,7 @@ class TourController extends AuthenticatedController } if (Request::submitted('yes')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $this->response->add_header('X-Action', 'complete'); $this->tour->deleteStep($step_nr); } elseif (Request::submitted('no')) { @@ -484,7 +484,7 @@ class TourController extends AuthenticatedController } // save step if ($mode === 'save') { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); if (Request::option('tour_step_editmode') == 'new') { $this->tour = new HelpTour($tour_id); if ($tour_id && $this->tour->isNew()) { @@ -696,7 +696,7 @@ class TourController extends AuthenticatedController } if (Request::submitted('save_tour_details')) { - CSRFProtection::verifySecurityToken(); + CSRFProtection::verifyUnsafeRequest(); $this->tour->name = trim(Request::get('tour_name')); $this->tour->description = trim(Request::get('tour_description')); if (Request::option('tour_language')) { diff --git a/app/views/admin/statusgroups/_group.php b/app/views/admin/statusgroups/_group.php index 9770d7be345..84ba55a6d98 100644 --- a/app/views/admin/statusgroups/_group.php +++ b/app/views/admin/statusgroups/_group.php @@ -9,56 +9,75 @@ */ ?> <a name="group-<?= $group->id ?>"></a> -<table id="<?= $group->id ?>" class="default movable"> - <colgroup> - <col width="1"> - <col width="1"> - <col width="10"> - <col> - <col width="10%"> - </colgroup> - <caption> - <?= htmlReady($group->name) ?> - <? if ($tutor): ?> - <span class="actions"> - <? $menu = ActionMenu::get()->setContext($group->name) ?> - <? $menu->addLink($controller->url_for("admin/statusgroups/editGroup/{$group->id}"), - _('Gruppe bearbeiten'), Icon::create('edit'), ['data-dialog' => 'size=auto']) ?> - <? $menu->addMultiPersonSearch( - MultiPersonSearch::get("add_statusgroup" . $group->id) - ->setLinkText(_('Personen hinzufügen')) - ->setDefaultSelectedUser($group->members->pluck('user_id')) - ->setExecuteURL($controller->url_for("admin/statusgroups/memberAdd/{$group->id}")) - ->setSearchObject($searchType) - ->addQuickfilter(_("aktuelle Einrichtung"), $membersOfInstitute) - ->addQuickfilter(_('Nicht zugeordnet'), $not_assigned) - ) ?> - <? $menu->addLink($controller->url_for("admin/statusgroups/deleteGroup/{$group->id}"), - _('Gruppe löschen'), Icon::create('trash'), ['data-dialog' => 'size=auto']) ?> - <? $menu->addLink($controller->url_for("admin/statusgroups/sortAlphabetic/{$group->id}"), - _('Gruppe alphabetisch sortieren'), Icon::create('arr_2down'), ['data-dialog' => 'size=auto']) ?> - <? if ($group->children): ?> - <? $menu->addLink($controller->link_for("admin/statusgroups/sortGroupsAlphabetical/{$group->id}"), - _('Untergruppen alphabetisch sortieren'), Icon::create('filter2'), - ['data-confirm' => _('Sollen die Untergruppen dieser Gruppe alphabetisch sortiert werden?')]) ?> - <? endif ?> - <?= $menu->render() ?> - </span> - <? endif; ?> - </caption> - <thead> - <tr> - <th colspan="4"> - <?= sprintf(ngettext('%u Mitglied', '%u Mitglieder', count($group->members)), - count($group->members)) ?> - </th> - <th class="actions"></th> - </tr> - </thead> - <tbody> - <?= $this->render_partial('admin/statusgroups/_members.php', ['group' => $group]) ?> - </tbody> -</table> +<form method="post"> + <?= CSRFProtection::tokenTag() ?> + <table id="<?= $group->id ?>" class="default movable"> + <colgroup> + <col width="1"> + <col width="1"> + <col width="10"> + <col> + <col width="10%"> + </colgroup> + <caption> + <?= htmlReady($group->name) ?> + <? if ($tutor): ?> + <span class="actions"> + <? $menu = ActionMenu::get()->setContext($group->name) ?> + <? $menu->addLink($controller->url_for("admin/statusgroups/editGroup/{$group->id}"), + _('Gruppe bearbeiten'), Icon::create('edit'), ['data-dialog' => 'size=auto']) ?> + <? $menu->addMultiPersonSearch( + MultiPersonSearch::get("add_statusgroup" . $group->id) + ->setLinkText(_('Personen hinzufügen')) + ->setDefaultSelectedUser($group->members->pluck('user_id')) + ->setExecuteURL($controller->url_for("admin/statusgroups/memberAdd/{$group->id}")) + ->setSearchObject($searchType) + ->addQuickfilter(_("aktuelle Einrichtung"), $membersOfInstitute) + ->addQuickfilter(_('Nicht zugeordnet'), $not_assigned) + ) ?> + <? $menu->addButton( + 'delete', + _('Gruppe löschen'), + Icon::create('trash'), + [ + 'data-dialog' => 'size=auto', + 'formaction' => $controller->url_for("admin/statusgroups/deleteGroup/{$group->id}"), + + ] + ) ?> + <? $menu->addButton( + 'sort', + _('Gruppe alphabetisch sortieren'), + Icon::create('arr_2down'), + [ + 'data-dialog' => 'size=auto', + 'formaction' => $controller->url_for("admin/statusgroups/sortAlphabetic/{$group->id}"), + + ] + ) ?> + <? if ($group->children): ?> + <? $menu->addLink($controller->link_for("admin/statusgroups/sortGroupsAlphabetical/{$group->id}"), + _('Untergruppen alphabetisch sortieren'), Icon::create('filter2'), + ['data-confirm' => _('Sollen die Untergruppen dieser Gruppe alphabetisch sortiert werden?')]) ?> + <? endif ?> + <?= $menu->render() ?> + </span> + <? endif; ?> + </caption> + <thead> + <tr> + <th colspan="4"> + <?= sprintf(ngettext('%u Mitglied', '%u Mitglieder', count($group->members)), + count($group->members)) ?> + </th> + <th class="actions"></th> + </tr> + </thead> + <tbody> + <?= $this->render_partial('admin/statusgroups/_members.php', ['group' => $group]) ?> + </tbody> + </table> +</form> <? if ($group->children): ?> <ul class='tree-seperator'> diff --git a/app/views/course/basicdata/view.php b/app/views/course/basicdata/view.php index 380fdb48ecf..785ae95fcb2 100644 --- a/app/views/course/basicdata/view.php +++ b/app/views/course/basicdata/view.php @@ -138,14 +138,14 @@ $message_types = ['msg' => "success", 'error' => "error", 'info' => "info"]; <td class="actions"> <? if ($perm_dozent && !$dozent_is_locked): ?> <? if ($num > 0) : ?> - <a href="<?= $controller->link_for('course/basicdata/priorityupfor', $course_id, $dozent['user_id'], 'dozent') ?>" <?= $dialog_attr ?>> + <button class="as-link" formaction="<?= $controller->link_for('course/basicdata/priorityupfor', $course_id, $dozent['user_id'], 'dozent') ?>" <?= $dialog_attr ?>> <?= Icon::create('arr_2up', Icon::ROLE_SORT)->asImg(['class' => 'middle']) ?> - </a> + </button> <? endif; ?> <? if ($num < count($dozenten) - 1): ?> - <a href="<?= $controller->link_for('course/basicdata/prioritydownfor', $course_id, $dozent['user_id'], 'dozent') ?>" <?= $dialog_attr ?>> + <button class="as-link" formaction="<?= $controller->link_for('course/basicdata/prioritydownfor', $course_id, $dozent['user_id'], 'dozent') ?>" <?= $dialog_attr ?>> <?= Icon::create('arr_2down', Icon::ROLE_SORT)->asImg(['class' => 'middle']) ?> - </a> + </button> <? endif; ?> <?= Icon::create('trash')->asInput([ 'formaction' => $controller->url_for('course/basicdata/deletedozent', $course_id, $dozent['user_id']), @@ -275,14 +275,14 @@ $message_types = ['msg' => "success", 'error' => "error", 'info' => "info"]; <td class="actions"> <? if ($perm_dozent && !$tutor_is_locked): ?> <? if ($num > 0) : ?> - <a href="<?= $controller->link_for('course/basicdata/priorityupfor', $course_id, $tutor['user_id'], 'tutor') ?>" <?= $dialog_attr ?>> + <button class="as-link" formaction="<?= $controller->link_for('course/basicdata/priorityupfor', $course_id, $tutor['user_id'], 'tutor') ?>" <?= $dialog_attr ?>> <?= Icon::create('arr_2up', Icon::ROLE_SORT)->asImg(['class' => 'middle']) ?> - </a> + </button> <? endif; ?> <? if ($num < count($tutoren) - 1) : ?> - <a href="<?= $controller->link_for('course/basicdata/prioritydownfor', $course_id, $tutor['user_id'], 'tutor') ?>" <?= $dialog_attr ?>> + <button class="as-link" formaction="<?= $controller->link_for('course/basicdata/prioritydownfor', $course_id, $tutor['user_id'], 'tutor') ?>" <?= $dialog_attr ?>> <?= Icon::create('arr_2down', Icon::ROLE_SORT)->asImg(['class' => 'middle']) ?> - </a> + </button> <? endif; ?> <?= Icon::create('trash')->asInput([ 'formaction' => $controller->url_for('course/basicdata/deletetutor', $course_id, $tutor['user_id']), diff --git a/app/views/shared/contacts/details.php b/app/views/shared/contacts/details.php index a4605ce915e..036035de13d 100644 --- a/app/views/shared/contacts/details.php +++ b/app/views/shared/contacts/details.php @@ -60,13 +60,14 @@ Icon::create('edit'), ['data-dialog' => 'size=auto'] ); - $actions->addLink( - $controller->url_for('shared/contacts/delete_range', $rel['contact_range_id']), + $actions->addButton( + 'delete_range', _('Ansprechpartner-Zuordnung löschen'), Icon::create('trash'), [ 'data-confirm' => _('Wollen Sie die Zuordnung des Ansprechpartners wirklich entfernen?'), - 'data-dialog' => 'size=auto' + 'data-dialog' => 'size=auto', + 'formaction' => $controller->url_for('shared/contacts/delete_range', $rel['contact_range_id']), ] ); echo $actions; diff --git a/app/views/shared/contacts/range.php b/app/views/shared/contacts/range.php index f58b286503b..eeb06abc57b 100644 --- a/app/views/shared/contacts/range.php +++ b/app/views/shared/contacts/range.php @@ -56,13 +56,14 @@ ); } if ($perm_contacts >= MvvPerm::PERM_CREATE) { - $actions->addLink( - $controller->url_for('shared/contacts/delete_range', $mvv_contact->id), + $actions->addButton( + 'delete_range', _('Ansprechpartner-Zuordnung löschen'), Icon::create('trash'), [ 'data-confirm' => _('Wollen Sie die Zuordnung des Ansprechpartners wirklich entfernen?'), - 'data-dialog' => 'size=auto' + 'data-dialog' => 'size=auto', + 'formaction' => $controller->url_for('shared/contacts/delete_range', $mvv_contact->id), ] ); } diff --git a/lib/showNews.inc.php b/lib/showNews.inc.php index 882e9d4d423..c3734d432d4 100644 --- a/lib/showNews.inc.php +++ b/lib/showNews.inc.php @@ -40,8 +40,8 @@ function delete_news($delete_news_array) if (!is_array($delete_news_array)) { $delete_news_array = [$delete_news_array]; } - if (Request::submitted('yes') && Request::isPost()) { - CSRFProtection::verifySecurityToken(); + if (Request::submitted('yes')) { + CSRFProtection::verifyUnsafeRequest(); $confirmed = true; } $delete_news_titles = []; @@ -116,8 +116,8 @@ function remove_news($remove_array) if (!is_array($remove_array)) { return ''; } - if (Request::submitted('yes') && Request::isPost()) { - CSRFProtection::verifySecurityToken(); + if (Request::submitted('yes')) { + CSRFProtection::verifyUnsafeRequest(); $confirmed = true; } foreach ($remove_array as $news_id => $ranges) { -- GitLab