Skip to content

Commit 37643a5

Browse files
protitudeprotitude
committed
Attempt at cleaning up the code a bit and hardening where it makes sense
1 parent b6d9f9f commit 37643a5

13 files changed

Lines changed: 68 additions & 39 deletions

‎oit.module‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
*/
77

88
use Drupal\block\Entity\Block;
9+
use Drupal\Component\Utility\Html;
10+
use Drupal\Component\Utility\UrlHelper;
911
use Drupal\Component\Utility\Xss;
1012
use Drupal\Core\Access\AccessResult;
1113
use Drupal\Core\Cache\Cache;
@@ -202,7 +204,12 @@ function oit_form_alter(&$form, FormStateInterface $form_state, $form_id) {
202204
$query['field_domain_access_target_id'] = 'oit_colorado_edu';
203205
$query = http_build_query($query);
204206
$response = new RedirectResponse('/admin/content?' . $query);
207+
// @todo (C1): Calling send() inside hook_form_alter() bypasses the Symfony
208+
// kernel response pipeline. The proper fix is to convert this to a
209+
// #submit handler or KernelEvents::RESPONSE subscriber. Until then,
210+
// return immediately after send() so no further form-build code runs.
205211
$response->send();
212+
return;
206213
}
207214
}
208215
}
@@ -406,18 +413,18 @@ function oit_form_alter(&$form, FormStateInterface $form_state, $form_id) {
406413
if (!$show_login_form) {
407414
// Cache is breaking the redirect, so kill it.
408415
\Drupal::service('page_cache_kill_switch')->trigger();
409-
$dest_get = \Drupal::request()->get('dest') != NULL ? Xss::filter(\Drupal::request()->get('dest')) : '';
410-
$destination_get = \Drupal::request()->get('destination') != NULL ? Xss::filter(\Drupal::request()->get('destination')) : '';
411-
$destination = "";
412-
if (!empty($dest_get)) {
413-
$destination = '?destination=' . preg_replace('/https:\/\/[^\/]+/', '', $dest_get);
416+
$dest_get = \Drupal::request()->get('dest') ?? '';
417+
$destination_get = \Drupal::request()->get('destination') ?? '';
418+
$destination = '';
419+
if (!empty($dest_get) && !UrlHelper::isExternal($dest_get)) {
420+
$destination = '?destination=' . Xss::filter($dest_get);
414421
}
415-
if (!empty($destination_get)) {
416-
$destination = '?destination=' . preg_replace('/https:\/\/[^\/]+/', '', $destination_get);
422+
if (!empty($destination_get) && !UrlHelper::isExternal($destination_get)) {
423+
$destination = '?destination=' . Xss::filter($destination_get);
417424
}
418425
// Drupal 10 add log message with $destination.
419426
\Drupal::logger('oit')->notice('User login form redirecting to saml_login with destination: @destination', ['@destination' => $destination]);
420-
$response = new RedirectResponse('/saml_login' . $destination, 301);
427+
$response = new RedirectResponse('/saml_login' . $destination, 302);
421428
$response->send();
422429
unset($form['name']);
423430
unset($form['pass']);
@@ -487,7 +494,12 @@ function _oit_form_set_domain($page, $field_domain) {
487494
$query['field_domain_access_target_id'] = $field_domain;
488495
$query = http_build_query($query);
489496
$response = new RedirectResponse('/admin/' . $page . '?' . $query);
497+
// @todo (C1): Calling send() inside hook_form_alter() bypasses the Symfony
498+
// kernel response pipeline. The proper fix is to convert this to a
499+
// #submit handler or KernelEvents::RESPONSE subscriber. Until then,
500+
// return immediately after send() so no further form-build code runs.
490501
$response->send();
502+
return;
491503
}
492504
}
493505

@@ -716,7 +728,7 @@ function oit_tokens($type, $tokens, array $data, array $options, BubbleableMetad
716728
}
717729
}
718730
}
719-
$replacements[$original] = $user_name;
731+
$replacements[$original] = Html::escape($user_name);
720732
break;
721733

722734
case 'tweet_pic':
@@ -878,7 +890,7 @@ function oit_responsive_menu_off_canvas_tree_alter(array &$rendered_tree) {
878890
* Create social link.
879891
*/
880892
function oit_social_links($title, $url) {
881-
return "<a class='social-icon $title' title='$title' href='$url'><img alt='' src='/themes/custom/dingo/images/icons/$title.svg'></a>";
893+
return "<a class='social-icon " . Html::escape($title) . "' title='" . Html::escape($title) . "' href='" . Html::escape($url) . "'><img alt='' src='/themes/custom/dingo/images/icons/" . Html::escape($title) . ".svg'></a>";
882894
}
883895

884896
/**

‎src/Controller/OitController.php‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Drupal\oit\Controller;
44

5+
use Drupal\Component\Utility\Html;
56
use Drupal\Component\Utility\Xss;
67
use Drupal\Core\Config\ConfigFactory;
78
use Drupal\Core\Controller\ControllerBase;
@@ -357,9 +358,9 @@ public function oitDenied() {
357358
* HTML string for the denied page body.
358359
*/
359360
private function deniedContent() {
360-
if ($_SERVER["REQUEST_URI"]) {
361-
$clean_uri = Xss::filter($_SERVER["REQUEST_URI"]);
362-
$requested_path = '?destination=' . $clean_uri;
361+
$uri = $this->requestStack->getRequestUri();
362+
if ($uri) {
363+
$requested_path = '?destination=' . Html::escape($uri);
363364
}
364365
else {
365366
$requested_path = '';
@@ -390,9 +391,7 @@ public function oitUserEdit() {
390391
$nid = $this->account->id();
391392
$path = Url::fromRoute('entity.user.edit_form', ['user' => $nid])->toString();
392393

393-
$response = new RedirectResponse($path);
394-
$response->send();
395-
exit;
394+
return new RedirectResponse($path);
396395
}
397396

398397
/**

‎src/Form/AbuseConfirmForm.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,9 +206,12 @@ private function banIpRemove($ip) {
206206
*/
207207
private function ipWhitelist($ip) {
208208
$autoban_settings = $this->configFactory->getEditable('autoban.settings');
209-
$whitelist = $autoban_settings->get('autoban_whitelist');
210-
$whitelist .= "\n" . $ip;
211-
$autoban_settings->set('autoban_whitelist', $whitelist)->save();
209+
$whitelist_raw = $autoban_settings->get('autoban_whitelist') ?? '';
210+
$entries = array_filter(array_map('trim', explode("\n", $whitelist_raw)));
211+
if (!in_array($ip, $entries, TRUE)) {
212+
$entries[] = $ip;
213+
$autoban_settings->set('autoban_whitelist', implode("\n", $entries))->save();
214+
}
212215
}
213216

214217
/**

‎src/Plugin/Block/FrontActionLinks.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
use Drupal\Core\Entity\EntityTypeManagerInterface;
77
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
88
use Drupal\oit\Plugin\BlockUuidQuery;
9-
use Drupal\shortcode_svg\Plugin\ShortcodeIcon;
9+
use Drupal\shortcode_svg\ShortcodeIcon;
1010
use Drupal\views\Views;
1111
use Symfony\Component\DependencyInjection\ContainerInterface;
1212

@@ -38,7 +38,7 @@ class FrontActionLinks extends BlockBase implements
3838
/**
3939
* Call shortcode svg icon.
4040
*
41-
* @var \Drupal\shortcode_svg\Plugin\ShortcodeIcon
41+
* @var \Drupal\shortcode_svg\ShortcodeIcon
4242
*/
4343
protected $shortcodeSvgIcon;
4444

@@ -80,7 +80,7 @@ public static function create(ContainerInterface $container, array $configuratio
8080
* Invokes renderer.
8181
* @param \Drupal\oit\Plugin\BlockUuidQuery $block_uuid_query
8282
* Loads block.
83-
* @param \Drupal\shortcode_svg\Plugin\ShortcodeIcon $shortcode_svg_icon
83+
* @param \Drupal\shortcode_svg\ShortcodeIcon $shortcode_svg_icon
8484
* Call shortcode svg icon.
8585
*/
8686
public function __construct(

‎src/Plugin/Block/FrontServiceHealth.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace Drupal\oit\Plugin\Block;
44

5+
use Drupal\Component\Utility\Html;
56
use Drupal\Core\Block\BlockBase;
67
use Drupal\Core\Cache\Cache;
78
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
@@ -97,7 +98,7 @@ public function build() {
9798
$link = !empty($category[$service_key]['link']) ? $category[$service_key]['link'] : '';
9899
$services .= "<li class='truncate'>$svg ";
99100
if (empty($link)) {
100-
$services .= "<a href='/service-health#$service_name_id'>$service_name</a>";
101+
$services .= "<a href='/service-health#" . Html::escape($service_name_id) . "'>" . Html::escape($service_name) . "</a>";
101102
}
102103
else {
103104
$services .= "$link";
@@ -133,7 +134,7 @@ public function build() {
133134
$link = !empty($category[$service_key]['link']) ? $category[$service_key]['link'] : '';
134135
$services .= "<li class='truncate'>$svg ";
135136
if (empty($link)) {
136-
$services .= "<a href='/service-health#$service_name_id'>$service_name</a>";
137+
$services .= "<a href='/service-health#" . Html::escape($service_name_id) . "'>" . Html::escape($service_name) . "</a>";
137138
}
138139
else {
139140
$services .= "$link";

‎src/Plugin/Block/PageOverview.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,8 @@ public function build() {
150150
}
151151
return [
152152
'#type' => 'inline_template',
153+
// The 'raw' filter is safe here: $summary is assembled from
154+
// check_markup()-processed content (Drupal's text filter pipeline).
153155
'#template' => '{{ summary | raw }} ',
154156
'#context' => [
155157
'summary' => $summary,

‎src/Plugin/Block/ServiceAlertTweet.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,9 @@ public function build() {
124124

125125
$route_match = $this->routeMatchInterface->getRawParameters()->getIterator();
126126

127+
// Role machine name 'service_alert_er_' is correct — trailing underscore
128+
// is intentional. See config/default/user.role.service_alert_er_.yml
129+
// (label: 'Service Alert(er)').
127130
if ($route_match->key() != 'node' || (!in_array('administrator', $roles) && !in_array('oit_administration', $roles) && !in_array('service_alert_er_', $roles))) {
128131
return [];
129132
}

‎src/Plugin/Block/TutorialBlock.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ public function build() {
118118
}
119119
return [
120120
'#type' => 'inline_template',
121+
// The 'raw' filter is safe here: all variables are processed through
122+
// check_markup() (Drupal's text filter pipeline) before rendering.
121123
'#template' => '<div class="flex">
122124
<div class="flex-one-half">{{ icon | raw }}</div>
123125
<div class="flex-one-half tutorial-layout">

‎src/Plugin/RedirectAddAnalytics.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,9 @@ public function __construct(
5959
$path = $redirect->redirect_source__path;
6060
$query = $this->connection->update('redirect');
6161
$query->condition('rid', $redirect->rid);
62+
$separator = str_contains($uri, '?') ? '&' : '?';
6263
$query->fields([
63-
'redirect_redirect__uri' => $uri .
64-
'?utm_source=' .
65-
$path .
66-
'&utm_campaign=redirect',
64+
'redirect_redirect__uri' => $uri . $separator . 'utm_source=' . rawurlencode($path) . '&utm_campaign=redirect',
6765
]);
6866
$query->execute();
6967
}

‎src/Plugin/TeamsAlert.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public function sendMessage(
106106
curl_setopt($ch, CURLOPT_POSTFIELDS, $teams_card);
107107
curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
108108
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 10);
109-
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, 3);
109+
curl_setopt($ch, CURLOPT_TIMEOUT, 30);
110110
curl_setopt($ch, CURLOPT_HTTPHEADER, [
111111
'Content-Type: application/json',
112112
'Content-Length: ' . strlen($teams_card),

0 commit comments

Comments
 (0)