From 4dfab43fd5c229aa33d0a3c9aa04d9095e2411d4 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Thu, 18 Oct 2018 08:46:04 +1000 Subject: [PATCH] SA-CORE-2018-006 by alexpott, attilatilman, bkosborne, catch, bonus, Wim Leers, Sam152, Berdir, Damien Tournoud, Dave Reid, Kova101, David_Rothstein, dawehner, dsnopek, samuel.mortenson, stefan.r, tedbow, xjm, timmillwood, pwolanin, njbooher, dyates, effulgentsia, klausi, mlhess, larowlan --- .../Drupal/Component/Utility/UrlHelper.php | 10 ++ .../RedirectResponseSubscriber.php | 32 ---- .../Drupal/Core/Mail/Plugin/Mail/PhpMail.php | 48 +++++- .../Core/PathProcessor/PathProcessorAlias.php | 9 ++ core/lib/Drupal/Core/Routing/UrlGenerator.php | 5 + .../Drupal/Core/Security/RequestSanitizer.php | 13 +- .../src/Functional/Views/DisplayBlockTest.php | 10 +- .../Constraint/ModerationStateConstraint.php | 1 + .../ModerationStateConstraintValidator.php | 90 ++++++++--- .../src/StateTransitionValidation.php | 10 ++ .../StateTransitionValidationInterface.php | 19 +++ .../Functional/ModerationStateNodeTest.php | 23 +-- .../EntityStateChangeValidationTest.php | 107 +++++++++++++ core/modules/contextual/contextual.module | 8 +- .../contextual/contextual.post_update.php | 14 ++ core/modules/contextual/js/contextual.es6.js | 21 ++- core/modules/contextual/js/contextual.js | 20 ++- .../contextual/src/ContextualController.php | 12 +- .../Element/ContextualLinksPlaceholder.php | 9 +- .../ContextualDynamicContextTest.php | 91 +++++++++-- .../tests/src/Functional/PathAliasTest.php | 31 +++- .../system/src/Tests/Routing/RouterTest.php | 7 + .../Tests/Component/Utility/UrlHelperTest.php | 4 + .../RedirectResponseSubscriberTest.php | 71 --------- .../Tests/Core/Mail/MailManagerTest.php | 9 ++ .../Core/Security/RequestSanitizerTest.php | 141 ++++++++++++++++++ 26 files changed, 639 insertions(+), 176 deletions(-) create mode 100644 core/modules/contextual/contextual.post_update.php diff --git a/core/lib/Drupal/Component/Utility/UrlHelper.php b/core/lib/Drupal/Component/Utility/UrlHelper.php index 1d133c9d6b..9e2365c1ae 100644 --- a/core/lib/Drupal/Component/Utility/UrlHelper.php +++ b/core/lib/Drupal/Component/Utility/UrlHelper.php @@ -248,6 +248,16 @@ public static function isExternal($path) { * Exception thrown when a either $url or $bath_url are not fully qualified. */ public static function externalIsLocal($url, $base_url) { + // Some browsers treat \ as / so normalize to forward slashes. + $url = str_replace('\\', '/', $url); + + // Leading control characters may be ignored or mishandled by browsers, so + // assume such a path may lead to an non-local location. The \p{C} character + // class matches all UTF-8 control, unassigned, and private characters. + if (preg_match('/^\p{C}/u', $url) !== 0) { + return FALSE; + } + $url_parts = parse_url($url); $base_parts = parse_url($base_url); diff --git a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php index 8397bdef4e..67a4aae422 100644 --- a/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php +++ b/core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php @@ -8,7 +8,6 @@ use Drupal\Core\Routing\RequestContext; use Drupal\Core\Utility\UnroutedUrlAssemblerInterface; use Symfony\Component\HttpFoundation\Response; -use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; use Symfony\Component\HttpKernel\Event\FilterResponseEvent; use Symfony\Component\HttpFoundation\RedirectResponse; @@ -129,36 +128,6 @@ protected function getDestinationAsAbsoluteUrl($destination, $scheme_and_host) { return $destination; } - /** - * Sanitize the destination parameter to prevent open redirect attacks. - * - * @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event - * The Event to process. - */ - public function sanitizeDestination(GetResponseEvent $event) { - $request = $event->getRequest(); - // Sanitize the destination parameter (which is often used for redirects) to - // prevent open redirect attacks leading to other domains. Sanitize both - // $_GET['destination'] and $_REQUEST['destination'] to protect code that - // relies on either, but do not sanitize $_POST to avoid interfering with - // unrelated form submissions. The sanitization happens here because - // url_is_external() requires the variable system to be available. - $query_info = $request->query; - $request_info = $request->request; - if ($query_info->has('destination') || $request_info->has('destination')) { - // If the destination is an external URL, remove it. - if ($query_info->has('destination') && UrlHelper::isExternal($query_info->get('destination'))) { - $query_info->remove('destination'); - $request_info->remove('destination'); - } - // If there's still something in $_REQUEST['destination'] that didn't come - // from $_GET, check it too. - if ($request_info->has('destination') && (!$query_info->has('destination') || $request_info->get('destination') != $query_info->get('destination')) && UrlHelper::isExternal($request_info->get('destination'))) { - $request_info->remove('destination'); - } - } - } - /** * Registers the methods in this class that should be listeners. * @@ -167,7 +136,6 @@ public function sanitizeDestination(GetResponseEvent $event) { */ public static function getSubscribedEvents() { $events[KernelEvents::RESPONSE][] = ['checkRedirectUrl']; - $events[KernelEvents::REQUEST][] = ['sanitizeDestination', 100]; return $events; } diff --git a/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php index e5361492b5..27e5c76e5d 100644 --- a/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php +++ b/core/lib/Drupal/Core/Mail/Plugin/Mail/PhpMail.php @@ -18,6 +18,20 @@ */ class PhpMail implements MailInterface { + /** + * The configuration factory. + * + * @var \Drupal\Core\Config\ConfigFactoryInterface + */ + protected $configFactory; + + /** + * PhpMail constructor. + */ + public function __construct() { + $this->configFactory = \Drupal::configFactory(); + } + /** * Concatenates and wraps the email body for plain-text mails. * @@ -86,7 +100,10 @@ public function mail(array $message) { // On most non-Windows systems, the "-f" option to the sendmail command // is used to set the Return-Path. There is no space between -f and // the value of the return path. - $additional_headers = isset($message['Return-Path']) ? '-f' . $message['Return-Path'] : ''; + // We validate the return path, unless it is equal to the site mail, which + // we assume to be safe. + $site_mail = $this->configFactory->get('system.site')->get('mail'); + $additional_headers = isset($message['Return-Path']) && ($site_mail === $message['Return-Path'] || static::_isShellSafe($message['Return-Path'])) ? '-f' . $message['Return-Path'] : ''; $mail_result = @mail( $message['to'], $mail_subject, @@ -112,4 +129,33 @@ public function mail(array $message) { return $mail_result; } + /** + * Disallows potentially unsafe shell characters. + * + * Functionally similar to PHPMailer::isShellSafe() which resulted from + * CVE-2016-10045. Note that escapeshellarg and escapeshellcmd are inadequate + * for this purpose. + * + * @param string $string + * The string to be validated. + * + * @return bool + * True if the string is shell-safe. + * + * @see https://github.com/PHPMailer/PHPMailer/issues/924 + * @see https://github.com/PHPMailer/PHPMailer/blob/v5.2.21/class.phpmailer.php#L1430 + * + * @todo Rename to ::isShellSafe() and/or discuss whether this is the correct + * location for this helper. + */ + protected static function _isShellSafe($string) { + if (escapeshellcmd($string) !== $string || !in_array(escapeshellarg($string), ["'$string'", "\"$string\""])) { + return FALSE; + } + if (preg_match('/[^a-zA-Z0-9@_\-.]/', $string) !== 0) { + return FALSE; + } + return TRUE; + } + } diff --git a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php index b85737f3cf..d0690fee20 100644 --- a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php +++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php @@ -43,6 +43,15 @@ public function processOutbound($path, &$options = [], Request $request = NULL, if (empty($options['alias'])) { $langcode = isset($options['language']) ? $options['language']->getId() : NULL; $path = $this->aliasManager->getAliasByPath($path, $langcode); + // Ensure the resulting path has at most one leading slash, to prevent it + // becoming an external URL without a protocol like //example.com. This + // is done in \Drupal\Core\Routing\UrlGenerator::generateFromRoute() + // also, to protect against this problem in arbitrary path processors, + // but it is duplicated here to protect any other URL generation code + // that might call this method separately. + if (strpos($path, '//') === 0) { + $path = '/' . ltrim($path, '/'); + } } return $path; } diff --git a/core/lib/Drupal/Core/Routing/UrlGenerator.php b/core/lib/Drupal/Core/Routing/UrlGenerator.php index 3b5c8d256b..853a5f7b4e 100644 --- a/core/lib/Drupal/Core/Routing/UrlGenerator.php +++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php @@ -297,6 +297,11 @@ public function generateFromRoute($name, $parameters = [], $options = [], $colle if ($options['path_processing']) { $path = $this->processPath($path, $options, $generated_url); } + // Ensure the resulting path has at most one leading slash, to prevent it + // becoming an external URL without a protocol like //example.com. + if (strpos($path, '//') === 0) { + $path = '/' . ltrim($path, '/'); + } // The contexts base URL is already encoded // (see Symfony\Component\HttpFoundation\Request). $path = str_replace($this->decodedChars[0], $this->decodedChars[1], rawurlencode($path)); diff --git a/core/lib/Drupal/Core/Security/RequestSanitizer.php b/core/lib/Drupal/Core/Security/RequestSanitizer.php index 3f48f3d59c..e1626ed383 100644 --- a/core/lib/Drupal/Core/Security/RequestSanitizer.php +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php @@ -90,7 +90,8 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo } if ($bag->has('destination')) { - $destination_dangerous_keys = static::checkDestination($bag->get('destination'), $whitelist); + $destination = $bag->get('destination'); + $destination_dangerous_keys = static::checkDestination($destination, $whitelist); if (!empty($destination_dangerous_keys)) { // The destination is removed rather than sanitized because the URL // generator service is not available and this method is called very @@ -101,6 +102,16 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo trigger_error(sprintf('Potentially unsafe destination removed from %s parameter bag because it contained the following keys: %s', $bag_name, implode(', ', $destination_dangerous_keys))); } } + // Sanitize the destination parameter (which is often used for redirects) + // to prevent open redirect attacks leading to other domains. + if (UrlHelper::isExternal($destination)) { + // The destination is removed because it is an external URL. + $bag->remove('destination'); + $sanitized = TRUE; + if ($log_sanitized_keys) { + trigger_error(sprintf('Potentially unsafe destination removed from %s parameter bag because it points to an external URL.', $bag_name)); + } + } } return $sanitized; } diff --git a/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php index 0a18d7fa0f..cdb1998241 100644 --- a/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php +++ b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php @@ -3,6 +3,8 @@ namespace Drupal\Tests\block\Functional\Views; use Drupal\Component\Serialization\Json; +use Drupal\Component\Utility\Crypt; +use Drupal\Core\Site\Settings; use Drupal\Core\Url; use Drupal\Tests\block\Functional\AssertBlockAppearsTrait; use Drupal\Tests\system\Functional\Cache\AssertPageCacheContextsAndTagsTrait; @@ -360,14 +362,16 @@ public function testBlockContextualLinks() { $this->drupalGet('test-page'); $id = 'block:block=' . $block->id() . ':langcode=en|entity.view.edit_form:view=test_view_block:location=block&name=test_view_block&display_id=block_1&langcode=en'; + $id_token = Crypt::hmacBase64($id, Settings::getHashSalt() . $this->container->get('private_key')->get()); $cached_id = 'block:block=' . $cached_block->id() . ':langcode=en|entity.view.edit_form:view=test_view_block:location=block&name=test_view_block&display_id=block_1&langcode=en'; + $cached_id_token = Crypt::hmacBase64($cached_id, Settings::getHashSalt() . $this->container->get('private_key')->get()); // @see \Drupal\contextual\Tests\ContextualDynamicContextTest:assertContextualLinkPlaceHolder() - $this->assertRaw(' $id]) . '>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $id])); - $this->assertRaw(' $cached_id]) . '>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $cached_id])); + $this->assertRaw(' $id, 'data-contextual-token' => $id_token]) . '>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $id])); + $this->assertRaw(' $cached_id, 'data-contextual-token' => $cached_id_token]) . '>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $cached_id])); // Get server-rendered contextual links. // @see \Drupal\contextual\Tests\ContextualDynamicContextTest:renderContextualLinks() - $post = ['ids[0]' => $id, 'ids[1]' => $cached_id]; + $post = ['ids[0]' => $id, 'ids[1]' => $cached_id, 'tokens[0]' => $id_token, 'tokens[1]' => $cached_id_token]; $url = 'contextual/render?_format=json,destination=test-page'; $this->getSession()->getDriver()->getClient()->request('POST', $url, $post); $this->assertResponse(200); diff --git a/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraint.php b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraint.php index 7f7c756b6b..4fcde36059 100644 --- a/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraint.php +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraint.php @@ -16,5 +16,6 @@ class ModerationStateConstraint extends Constraint { public $message = 'Invalid state transition from %from to %to'; public $invalidStateMessage = 'State %state does not exist on %workflow workflow'; + public $invalidTransitionAccess = 'You do not have access to transition from %original_state to %new_state'; } diff --git a/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php index 65fc2a0c50..c3b9c815fe 100644 --- a/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php @@ -2,10 +2,13 @@ namespace Drupal\content_moderation\Plugin\Validation\Constraint; +use Drupal\content_moderation\StateTransitionValidationInterface; use Drupal\Core\DependencyInjection\ContainerInjectionInterface; +use Drupal\Core\Entity\ContentEntityInterface; use Drupal\Core\Entity\EntityInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\content_moderation\ModerationInformationInterface; +use Drupal\Core\Session\AccountInterface; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; @@ -29,6 +32,20 @@ class ModerationStateConstraintValidator extends ConstraintValidator implements */ protected $moderationInformation; + /** + * The current user. + * + * @var \Drupal\Core\Session\AccountInterface + */ + protected $currentUser; + + /** + * The state transition validation service. + * + * @var \Drupal\content_moderation\StateTransitionValidationInterface + */ + protected $stateTransitionValidation; + /** * Creates a new ModerationStateConstraintValidator instance. * @@ -36,10 +53,16 @@ class ModerationStateConstraintValidator extends ConstraintValidator implements * The entity type manager. * @param \Drupal\content_moderation\ModerationInformationInterface $moderation_information * The moderation information. + * @param \Drupal\Core\Session\AccountInterface $current_user + * The current user. + * @param \Drupal\content_moderation\StateTransitionValidationInterface $state_transition_validation + * The state transition validation service. */ - public function __construct(EntityTypeManagerInterface $entity_type_manager, ModerationInformationInterface $moderation_information) { + public function __construct(EntityTypeManagerInterface $entity_type_manager, ModerationInformationInterface $moderation_information, AccountInterface $current_user, StateTransitionValidationInterface $state_transition_validation) { $this->entityTypeManager = $entity_type_manager; $this->moderationInformation = $moderation_information; + $this->currentUser = $current_user; + $this->stateTransitionValidation = $state_transition_validation; } /** @@ -48,7 +71,9 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager, Mod public static function create(ContainerInterface $container) { return new static( $container->get('entity_type.manager'), - $container->get('content_moderation.moderation_information') + $container->get('content_moderation.moderation_information'), + $container->get('current_user'), + $container->get('content_moderation.state_transition_validation') ); } @@ -76,32 +101,59 @@ public function validate($value, Constraint $constraint) { return; } + $new_state = $workflow->getTypePlugin()->getState($entity->moderation_state->value); + $original_state = $this->getOriginalOrInitialState($entity); + // If a new state is being set and there is an existing state, validate // there is a valid transition between them. + if (!$original_state->canTransitionTo($new_state->id())) { + $this->context->addViolation($constraint->message, [ + '%from' => $original_state->label(), + '%to' => $new_state->label(), + ]); + } + else { + // If we're sure the transition exists, make sure the user has permission + // to use it. + if (!$this->stateTransitionValidation->isTransitionValid($workflow, $original_state, $new_state, $this->currentUser)) { + $this->context->addViolation($constraint->invalidTransitionAccess, [ + '%original_state' => $original_state->label(), + '%new_state' => $new_state->label(), + ]); + } + } + } + + /** + * Gets the original or initial state of the given entity. + * + * When a state is being validated, the original state is used to validate + * that a valid transition exists for target state and the user has access + * to the transition between those two states. If the entity has been + * moderated before, we can load the original unmodified revision and + * translation for this state. + * + * If the entity is new we need to load the initial state from the workflow. + * Even if a value was assigned to the moderation_state field, the initial + * state is used to compute an appropriate transition for the purposes of + * validation. + * + * @return \Drupal\workflows\StateInterface + * The original or default moderation state. + */ + protected function getOriginalOrInitialState(ContentEntityInterface $entity) { + $state = NULL; + $workflow_type = $this->moderationInformation->getWorkflowForEntity($entity)->getTypePlugin(); if (!$entity->isNew() && !$this->isFirstTimeModeration($entity)) { $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId()); if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) { $original_entity = $original_entity->getTranslation($entity->language()->getId()); } - - // If the state of the original entity doesn't exist on the workflow, - // we cannot do any further validation of transitions, because none will - // be setup for a state that doesn't exist. Instead allow any state to - // take its place. - if (!$workflow->getTypePlugin()->hasState($original_entity->moderation_state->value)) { - return; - } - - $new_state = $workflow->getTypePlugin()->getState($entity->moderation_state->value); - $original_state = $workflow->getTypePlugin()->getState($original_entity->moderation_state->value); - - if (!$original_state->canTransitionTo($new_state->id())) { - $this->context->addViolation($constraint->message, [ - '%from' => $original_state->label(), - '%to' => $new_state->label(), - ]); + if ($workflow_type->hasState($original_entity->moderation_state->value)) { + $state = $workflow_type->getState($original_entity->moderation_state->value); } } + return $state ?: $workflow_type->getInitialState($entity); } /** diff --git a/core/modules/content_moderation/src/StateTransitionValidation.php b/core/modules/content_moderation/src/StateTransitionValidation.php index 01b2ad8458..35d657e550 100644 --- a/core/modules/content_moderation/src/StateTransitionValidation.php +++ b/core/modules/content_moderation/src/StateTransitionValidation.php @@ -4,7 +4,9 @@ use Drupal\Core\Entity\ContentEntityInterface; use Drupal\Core\Session\AccountInterface; +use Drupal\workflows\StateInterface; use Drupal\workflows\Transition; +use Drupal\workflows\WorkflowInterface; /** * Validates whether a certain state transition is allowed. @@ -47,4 +49,12 @@ public function getValidTransitions(ContentEntityInterface $entity, AccountInter }); } + /** + * {@inheritdoc} + */ + public function isTransitionValid(WorkflowInterface $workflow, StateInterface $original_state, StateInterface $new_state, AccountInterface $user) { + $transition = $workflow->getTypePlugin()->getTransitionFromStateToState($original_state->id(), $new_state->id()); + return $user->hasPermission('use ' . $workflow->id() . ' transition ' . $transition->id()); + } + } diff --git a/core/modules/content_moderation/src/StateTransitionValidationInterface.php b/core/modules/content_moderation/src/StateTransitionValidationInterface.php index 1acbf052fd..c793fe53e2 100644 --- a/core/modules/content_moderation/src/StateTransitionValidationInterface.php +++ b/core/modules/content_moderation/src/StateTransitionValidationInterface.php @@ -4,6 +4,8 @@ use Drupal\Core\Entity\ContentEntityInterface; use Drupal\Core\Session\AccountInterface; +use Drupal\workflows\StateInterface; +use Drupal\workflows\WorkflowInterface; /** * Validates whether a certain state transition is allowed. @@ -23,4 +25,21 @@ interface StateTransitionValidationInterface { */ public function getValidTransitions(ContentEntityInterface $entity, AccountInterface $user); + /** + * Checks if a transition between two states if valid for the given user. + * + * @param \Drupal\workflows\WorkflowInterface $workflow + * The workflow entity. + * @param \Drupal\workflows\StateInterface $original_state + * The original workflow state. + * @param \Drupal\workflows\StateInterface $new_state + * The new workflow state. + * @param \Drupal\Core\Session\AccountInterface $user + * The user to validate. + * + * @return bool + * Returns TRUE if transition is valid, otherwise FALSE. + */ + public function isTransitionValid(WorkflowInterface $workflow, StateInterface $original_state, StateInterface $new_state, AccountInterface $user); + } diff --git a/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php b/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php index 11deaa72c0..5fd168d0bb 100644 --- a/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php +++ b/core/modules/content_moderation/tests/src/Functional/ModerationStateNodeTest.php @@ -158,32 +158,15 @@ public function testNoContentModerationPermissions() { ]); $this->drupalLogin($limited_user); - // Check the user can add content, but can't see the moderation state - // select. + // Check the user can see the content entity form, but can't see the + // moderation state select or save the entity form. $this->drupalGet('node/add/moderated_content'); $session_assert->statusCodeEquals(200); $session_assert->fieldNotExists('moderation_state[0][state]'); $this->drupalPostForm(NULL, [ 'title[0][value]' => 'moderated content', ], 'Save'); - - // Manually move the content to archived because the user doesn't have - // permission to do this. - $node = $this->getNodeByTitle('moderated content'); - $node->moderation_state->value = 'archived'; - $node->save(); - - // Check the user can see the current state but not the select. - $this->drupalGet('node/' . $node->id() . '/edit'); - $session_assert->statusCodeEquals(200); - $session_assert->pageTextContains('Archived'); - $session_assert->fieldNotExists('moderation_state[0][state]'); - $this->drupalPostForm(NULL, [], 'Save'); - - // When saving they should still be on the edit form, and see the validation - // error message. - $session_assert->pageTextContains('Edit Moderated content moderated content'); - $session_assert->pageTextContains('Invalid state transition from Archived to Archived'); + $session_assert->pageTextContains('You do not have access to transition from Draft to Draft'); } } diff --git a/core/modules/content_moderation/tests/src/Kernel/EntityStateChangeValidationTest.php b/core/modules/content_moderation/tests/src/Kernel/EntityStateChangeValidationTest.php index dc1e7f6917..df90bf63c8 100644 --- a/core/modules/content_moderation/tests/src/Kernel/EntityStateChangeValidationTest.php +++ b/core/modules/content_moderation/tests/src/Kernel/EntityStateChangeValidationTest.php @@ -7,6 +7,7 @@ use Drupal\node\Entity\Node; use Drupal\node\Entity\NodeType; use Drupal\Tests\content_moderation\Traits\ContentModerationTestTrait; +use Drupal\Tests\user\Traits\UserCreationTrait; /** * @coversDefaultClass \Drupal\content_moderation\Plugin\Validation\Constraint\ModerationStateConstraintValidator @@ -15,6 +16,7 @@ class EntityStateChangeValidationTest extends KernelTestBase { use ContentModerationTestTrait; + use UserCreationTrait; /** * {@inheritdoc} @@ -29,6 +31,13 @@ class EntityStateChangeValidationTest extends KernelTestBase { 'workflows', ]; + /** + * An admin user. + * + * @var \Drupal\Core\Session\AccountInterface + */ + protected $adminUser; + /** * {@inheritdoc} */ @@ -40,6 +49,9 @@ protected function setUp() { $this->installEntitySchema('user'); $this->installEntitySchema('content_moderation_state'); $this->installConfig('content_moderation'); + $this->installSchema('system', ['sequences']); + + $this->adminUser = $this->createUser(array_keys($this->container->get('user.permissions')->getPermissions())); } /** @@ -48,6 +60,8 @@ protected function setUp() { * @covers ::validate */ public function testValidTransition() { + $this->setCurrentUser($this->adminUser); + $node_type = NodeType::create([ 'type' => 'example', ]); @@ -76,6 +90,8 @@ public function testValidTransition() { * @covers ::validate */ public function testInvalidTransition() { + $this->setCurrentUser($this->adminUser); + $node_type = NodeType::create([ 'type' => 'example', ]); @@ -125,6 +141,7 @@ public function testInvalidState() { * Test validation with content that has no initial state or an invalid state. */ public function testInvalidStateWithoutExisting() { + $this->setCurrentUser($this->adminUser); // Create content without moderation enabled for the content type. $node_type = NodeType::create([ 'type' => 'example', @@ -156,15 +173,24 @@ public function testInvalidStateWithoutExisting() { // validating. $workflow->getTypePlugin()->deleteState('deleted_state'); $workflow->save(); + + // When there is an invalid state, the content will revert to "draft". This + // will allow a draft to draft transition. $node->moderation_state->value = 'draft'; $violations = $node->validate(); $this->assertCount(0, $violations); + // This will disallow a draft to archived transition. + $node->moderation_state->value = 'archived'; + $violations = $node->validate(); + $this->assertCount(1, $violations); } /** * Test state transition validation with multiple languages. */ public function testInvalidStateMultilingual() { + $this->setCurrentUser($this->adminUser); + ConfigurableLanguage::createFromLangcode('fr')->save(); $node_type = NodeType::create([ 'type' => 'example', @@ -220,6 +246,8 @@ public function testInvalidStateMultilingual() { * Tests that content without prior moderation information can be moderated. */ public function testExistingContentWithNoModeration() { + $this->setCurrentUser($this->adminUser); + $node_type = NodeType::create([ 'type' => 'example', ]); @@ -254,6 +282,8 @@ public function testExistingContentWithNoModeration() { * Tests that content without prior moderation information can be translated. */ public function testExistingMultilingualContentWithNoModeration() { + $this->setCurrentUser($this->adminUser); + // Enable French. ConfigurableLanguage::createFromLangcode('fr')->save(); @@ -293,4 +323,81 @@ public function testExistingMultilingualContentWithNoModeration() { $node_fr->save(); } + /** + * @dataProvider transitionAccessValidationTestCases + */ + public function testTransitionAccessValidation($permissions, $target_state, $messages) { + $node_type = NodeType::create([ + 'type' => 'example', + ]); + $node_type->save(); + $workflow = $this->createEditorialWorkflow(); + $workflow->getTypePlugin()->addState('foo', 'Foo'); + $workflow->getTypePlugin()->addTransition('draft_to_foo', 'Draft to foo', ['draft'], 'foo'); + $workflow->getTypePlugin()->addTransition('foo_to_foo', 'Foo to foo', ['foo'], 'foo'); + $workflow->getTypePlugin()->addEntityTypeAndBundle('node', 'example'); + $workflow->save(); + + $this->setCurrentUser($this->createUser($permissions)); + + $node = Node::create([ + 'type' => 'example', + 'title' => 'Test content', + 'moderation_state' => $target_state, + ]); + $this->assertTrue($node->isNew()); + $violations = $node->validate(); + $this->assertCount(count($messages), $violations); + foreach ($messages as $i => $message) { + $this->assertEquals($message, $violations->get($i)->getMessage()); + } + } + + /** + * Test cases for ::testTransitionAccessValidation. + */ + public function transitionAccessValidationTestCases() { + return [ + 'Invalid transition, no permissions validated' => [ + [], + 'archived', + ['Invalid state transition from Draft to Archived'], + ], + 'Valid transition, missing permission' => [ + [], + 'published', + ['You do not have access to transition from Draft to Published'], + ], + 'Valid transition, granted published permission' => [ + ['use editorial transition publish'], + 'published', + [], + ], + 'Valid transition, granted draft permission' => [ + ['use editorial transition create_new_draft'], + 'draft', + [], + ], + 'Valid transition, incorrect permission granted' => [ + ['use editorial transition create_new_draft'], + 'published', + ['You do not have access to transition from Draft to Published'], + ], + // Test with an additional state and set of transitions, since the + // "published" transition can start from either "draft" or "published", it + // does not capture bugs that fail to correctly distinguish the initial + // workflow state from the set state of a new entity. + 'Valid transition, granted foo permission' => [ + ['use editorial transition draft_to_foo'], + 'foo', + [], + ], + 'Valid transition, incorrect foo permission granted' => [ + ['use editorial transition foo_to_foo'], + 'foo', + ['You do not have access to transition from Draft to Foo'], + ], + ]; + } + } diff --git a/core/modules/contextual/contextual.module b/core/modules/contextual/contextual.module index b9d61b76d2..8b9fc36fd7 100644 --- a/core/modules/contextual/contextual.module +++ b/core/modules/contextual/contextual.module @@ -191,13 +191,19 @@ function _contextual_links_to_id($contextual_links) { /** * Unserializes the result of _contextual_links_to_id(). * - * @see _contextual_links_to_id + * Note that $id is user input. Before calling this method the ID should be + * checked against the token stored in the 'data-contextual-token' attribute + * which is passed via the 'tokens' request parameter to + * \Drupal\contextual\ContextualController::render(). * * @param string $id * A serialized representation of a #contextual_links property value array. * * @return array * The value for a #contextual_links property. + * + * @see _contextual_links_to_id() + * @see \Drupal\contextual\ContextualController::render() */ function _contextual_id_to_links($id) { $contextual_links = []; diff --git a/core/modules/contextual/contextual.post_update.php b/core/modules/contextual/contextual.post_update.php new file mode 100644 index 0000000000..8decad05f0 --- /dev/null +++ b/core/modules/contextual/contextual.post_update.php @@ -0,0 +1,14 @@ + { - const html = storage.getItem(`Drupal.contextual.${contextualID}`); + const uncachedIDs = []; + const uncachedTokens = []; + ids.forEach(contextualID => { + const html = storage.getItem(`Drupal.contextual.${contextualID.id}`); if (html && html.length) { // Initialize after the current execution cycle, to make the AJAX // request for retrieving the uncached contextual links as soon as @@ -182,13 +186,14 @@ // Drupal.contextual.collection. window.setTimeout(() => { initContextual( - $context.find(`[data-contextual-id="${contextualID}"]`), + $context.find(`[data-contextual-id="${contextualID.id}"]`), html, ); }); - return false; + return; } - return true; + uncachedIDs.push(contextualID.id); + uncachedTokens.push(contextualID.token); }); // Perform an AJAX request to let the server render the contextual links @@ -197,7 +202,7 @@ $.ajax({ url: Drupal.url('contextual/render'), type: 'POST', - data: { 'ids[]': uncachedIDs }, + data: { 'ids[]': uncachedIDs, 'tokens[]': uncachedTokens }, dataType: 'json', success(results) { _.each(results, (html, contextualID) => { diff --git a/core/modules/contextual/js/contextual.js b/core/modules/contextual/js/contextual.js index 049233b4e1..d51eba21a9 100644 --- a/core/modules/contextual/js/contextual.js +++ b/core/modules/contextual/js/contextual.js @@ -95,25 +95,31 @@ var ids = []; $placeholders.each(function () { - ids.push($(this).attr('data-contextual-id')); + ids.push({ + id: $(this).attr('data-contextual-id'), + token: $(this).attr('data-contextual-token') + }); }); - var uncachedIDs = _.filter(ids, function (contextualID) { - var html = storage.getItem('Drupal.contextual.' + contextualID); + var uncachedIDs = []; + var uncachedTokens = []; + ids.forEach(function (contextualID) { + var html = storage.getItem('Drupal.contextual.' + contextualID.id); if (html && html.length) { window.setTimeout(function () { - initContextual($context.find('[data-contextual-id="' + contextualID + '"]'), html); + initContextual($context.find('[data-contextual-id="' + contextualID.id + '"]'), html); }); - return false; + return; } - return true; + uncachedIDs.push(contextualID.id); + uncachedTokens.push(contextualID.token); }); if (uncachedIDs.length > 0) { $.ajax({ url: Drupal.url('contextual/render'), type: 'POST', - data: { 'ids[]': uncachedIDs }, + data: { 'ids[]': uncachedIDs, 'tokens[]': uncachedTokens }, dataType: 'json', success: function success(results) { _.each(results, function (html, contextualID) { diff --git a/core/modules/contextual/src/ContextualController.php b/core/modules/contextual/src/ContextualController.php index 58e42ecd6b..d05c6a8527 100644 --- a/core/modules/contextual/src/ContextualController.php +++ b/core/modules/contextual/src/ContextualController.php @@ -2,8 +2,10 @@ namespace Drupal\contextual; +use Drupal\Component\Utility\Crypt; use Drupal\Core\DependencyInjection\ContainerInjectionInterface; use Drupal\Core\Render\RendererInterface; +use Drupal\Core\Site\Settings; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\JsonResponse; use Symfony\Component\HttpFoundation\Request; @@ -63,8 +65,16 @@ public function render(Request $request) { throw new BadRequestHttpException(t('No contextual ids specified.')); } + $tokens = $request->request->get('tokens'); + if (!isset($tokens)) { + throw new BadRequestHttpException(t('No contextual ID tokens specified.')); + } + $rendered = []; - foreach ($ids as $id) { + foreach ($ids as $key => $id) { + if (!isset($tokens[$key]) || !Crypt::hashEquals($tokens[$key], Crypt::hmacBase64($id, Settings::getHashSalt() . \Drupal::service('private_key')->get()))) { + throw new BadRequestHttpException('Invalid contextual ID specified.'); + } $element = [ '#type' => 'contextual_links', '#contextual_links' => _contextual_id_to_links($id), diff --git a/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php index 97afde9a24..5e993941a6 100644 --- a/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php +++ b/core/modules/contextual/src/Element/ContextualLinksPlaceholder.php @@ -2,6 +2,8 @@ namespace Drupal\contextual\Element; +use Drupal\Component\Utility\Crypt; +use Drupal\Core\Site\Settings; use Drupal\Core\Template\Attribute; use Drupal\Core\Render\Element\RenderElement; use Drupal\Component\Render\FormattableMarkup; @@ -43,7 +45,12 @@ public function getInfo() { * @see _contextual_links_to_id() */ public static function preRenderPlaceholder(array $element) { - $element['#markup'] = new FormattableMarkup('', ['@attributes' => new Attribute(['data-contextual-id' => $element['#id']])]); + $token = Crypt::hmacBase64($element['#id'], Settings::getHashSalt() . \Drupal::service('private_key')->get()); + $attribute = new Attribute([ + 'data-contextual-id' => $element['#id'], + 'data-contextual-token' => $token, + ]); + $element['#markup'] = new FormattableMarkup('', ['@attributes' => $attribute]); return $element; } diff --git a/core/modules/contextual/tests/src/Functional/ContextualDynamicContextTest.php b/core/modules/contextual/tests/src/Functional/ContextualDynamicContextTest.php index 340b60821f..74a6d504e8 100644 --- a/core/modules/contextual/tests/src/Functional/ContextualDynamicContextTest.php +++ b/core/modules/contextual/tests/src/Functional/ContextualDynamicContextTest.php @@ -3,9 +3,10 @@ namespace Drupal\Tests\contextual\Functional; use Drupal\Component\Serialization\Json; +use Drupal\Component\Utility\Crypt; +use Drupal\Core\Site\Settings; use Drupal\Core\Url; use Drupal\language\Entity\ConfigurableLanguage; -use Drupal\Core\Template\Attribute; use Drupal\Tests\BrowserTestBase; /** @@ -140,17 +141,76 @@ public function testDifferentPermissions() { $this->assertRaw('
  • Edit menu - contextual
    • '); } + /** + * Tests the contextual placeholder content is protected by a token. + */ + public function testTokenProtection() { + $this->drupalLogin($this->editorUser); + + // Create a node that will have a contextual link. + $node1 = $this->drupalCreateNode(['type' => 'article', 'promote' => 1]); + + // Now, on the front page, all article nodes should have contextual links + // placeholders, as should the view that contains them. + $id = 'node:node=' . $node1->id() . ':changed=' . $node1->getChangedTime() . '&langcode=en'; + + // Editor user: can access contextual links and can edit articles. + $this->drupalGet('node'); + $this->assertContextualLinkPlaceHolder($id); + + $http_client = $this->getHttpClient(); + $url = Url::fromRoute('contextual.render', [], [ + 'query' => [ + '_format' => 'json', + 'destination' => 'node', + ], + ])->setAbsolute()->toString(); + + $response = $http_client->request('POST', $url, [ + 'cookies' => $this->getSessionCookies(), + 'form_params' => ['ids' => [$id], 'tokens' => []], + 'http_errors' => FALSE, + ]); + $this->assertEquals('400', $response->getStatusCode()); + $this->assertContains('No contextual ID tokens specified.', (string) $response->getBody()); + + $response = $http_client->request('POST', $url, [ + 'cookies' => $this->getSessionCookies(), + 'form_params' => ['ids' => [$id], 'tokens' => ['wrong_token']], + 'http_errors' => FALSE, + ]); + $this->assertEquals('400', $response->getStatusCode()); + $this->assertContains('Invalid contextual ID specified.', (string) $response->getBody()); + + $response = $http_client->request('POST', $url, [ + 'cookies' => $this->getSessionCookies(), + 'form_params' => ['ids' => [$id], 'tokens' => ['wrong_key' => $this->createContextualIdToken($id)]], + 'http_errors' => FALSE, + ]); + $this->assertEquals('400', $response->getStatusCode()); + $this->assertContains('Invalid contextual ID specified.', (string) $response->getBody()); + + $response = $http_client->request('POST', $url, [ + 'cookies' => $this->getSessionCookies(), + 'form_params' => ['ids' => [$id], 'tokens' => [$this->createContextualIdToken($id)]], + 'http_errors' => FALSE, + ]); + $this->assertEquals('200', $response->getStatusCode()); + } + /** * Asserts that a contextual link placeholder with the given id exists. * * @param string $id * A contextual link id. - * - * @return bool - * The result of the assertion. */ protected function assertContextualLinkPlaceHolder($id) { - return $this->assertRaw(' $id]) . '>', format_string('Contextual link placeholder with id @id exists.', ['@id' => $id])); + $this->assertSession()->elementAttributeContains( + 'css', + 'div[data-contextual-id="' . $id . '"]', + 'data-contextual-token', + $this->createContextualIdToken($id) + ); } /** @@ -158,12 +218,9 @@ protected function assertContextualLinkPlaceHolder($id) { * * @param string $id * A contextual link id. - * - * @return bool - * The result of the assertion. */ protected function assertNoContextualLinkPlaceHolder($id) { - return $this->assertNoRaw(' $id]) . '>', format_string('Contextual link placeholder with id @id does not exist.', ['@id' => $id])); + $this->assertSession()->elementNotExists('css', 'div[data-contextual-id="' . $id . '"]'); } /** @@ -178,6 +235,7 @@ protected function assertNoContextualLinkPlaceHolder($id) { * The response object. */ protected function renderContextualLinks($ids, $current_path) { + $tokens = array_map([$this, 'createContextualIdToken'], $ids); $http_client = $this->getHttpClient(); $url = Url::fromRoute('contextual.render', [], [ 'query' => [ @@ -188,9 +246,22 @@ protected function renderContextualLinks($ids, $current_path) { return $http_client->request('POST', $this->buildUrl($url), [ 'cookies' => $this->getSessionCookies(), - 'form_params' => ['ids' => $ids], + 'form_params' => ['ids' => $ids, 'tokens' => $tokens], 'http_errors' => FALSE, ]); } + /** + * Creates a contextual ID token. + * + * @param string $id + * The contextual ID to create a token for. + * + * @return string + * The contextual ID token. + */ + protected function createContextualIdToken($id) { + return Crypt::hmacBase64($id, Settings::getHashSalt() . $this->container->get('private_key')->get()); + } + } diff --git a/core/modules/path/tests/src/Functional/PathAliasTest.php b/core/modules/path/tests/src/Functional/PathAliasTest.php index b8ac5968db..19115cd27b 100644 --- a/core/modules/path/tests/src/Functional/PathAliasTest.php +++ b/core/modules/path/tests/src/Functional/PathAliasTest.php @@ -4,6 +4,7 @@ use Drupal\Core\Cache\Cache; use Drupal\Core\Database\Database; +use Drupal\Core\Url; /** * Add, edit, delete, and change alias and verify its consistency in the @@ -24,7 +25,7 @@ protected function setUp() { parent::setUp(); // Create test user and log in. - $web_user = $this->drupalCreateUser(['create page content', 'edit own page content', 'administer url aliases', 'create url aliases']); + $web_user = $this->drupalCreateUser(['create page content', 'edit own page content', 'administer url aliases', 'create url aliases', 'access content overview']); $this->drupalLogin($web_user); } @@ -327,6 +328,34 @@ public function testNodeAlias() { $node5->delete(); $path_alias = \Drupal::service('path.alias_storage')->lookupPathAlias('/node/' . $node5->id(), $node5->language()->getId()); $this->assertFalse($path_alias, 'Alias was successfully deleted when the referenced node was deleted.'); + + // Create sixth test node. + $node6 = $this->drupalCreateNode(); + + // Create an invalid alias with two leading slashes and verify that the + // extra slash is removed when the link is generated. This ensures that URL + // aliases cannot be used to inject external URLs. + // @todo The user interface should either display an error message or + // automatically trim these invalid aliases, rather than allowing them to + // be silently created, at which point the functional aspects of this + // test will need to be moved elsewhere and switch to using a + // programmatically-created alias instead. + $alias = $this->randomMachineName(8); + $edit = ['path[0][alias]' => '//' . $alias]; + $this->drupalPostForm($node6->toUrl('edit-form'), $edit, t('Save')); + $this->drupalGet(Url::fromRoute('system.admin_content')); + // This checks the link href before clicking it, rather than using + // \Drupal\Tests\BrowserTestBase::assertSession()->addressEquals() after + // clicking it, because the test browser does not always preserve the + // correct number of slashes in the URL when it visits internal links; + // using \Drupal\Tests\BrowserTestBase::assertSession()->addressEquals() + // would actually make the test pass unconditionally on the testbot (or + // anywhere else where Drupal is installed in a subdirectory). + $link_xpath = $this->xpath('//a[normalize-space(text())=:label]', [':label' => $node6->getTitle()]); + $link_href = $link_xpath[0]->getAttribute('href'); + $this->assertEquals($link_href, base_path() . $alias); + $this->clickLink($node6->getTitle()); + $this->assertResponse(404); } /** diff --git a/core/modules/system/src/Tests/Routing/RouterTest.php b/core/modules/system/src/Tests/Routing/RouterTest.php index 839f944d4f..57649fbcad 100644 --- a/core/modules/system/src/Tests/Routing/RouterTest.php +++ b/core/modules/system/src/Tests/Routing/RouterTest.php @@ -320,6 +320,13 @@ public function testLeadingSlashes() { $this->drupalGet($url); $this->assertEqual(1, $this->redirectCount, $url . " redirected to " . $this->url); $this->assertUrl($request->getUriForPath('/router_test/test1') . '?qs=test'); + + // Ensure that external URLs in destination query params are not redirected + // to. + $url = $request->getUriForPath('/////////////////////////////////////////////////router_test/test1') . '?qs=test&destination=http://www.example.com%5c@drupal8alt.test'; + $this->drupalGet($url); + $this->assertEqual(1, $this->redirectCount, $url . " redirected to " . $this->url); + $this->assertUrl($request->getUriForPath('/router_test/test1') . '?qs=test'); } } diff --git a/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php index d185219c9a..beaa472c26 100644 --- a/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php +++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php @@ -563,6 +563,10 @@ public function providerTestExternalIsLocal() { ['http://example.com/foo', 'http://example.com/bar', FALSE], ['http://example.com', 'http://example.com/bar', FALSE], ['http://example.com/bar', 'http://example.com/bar/', FALSE], + // Ensure \ is normalised to / since some browsers do that. + ['http://www.example.ca\@example.com', 'http://example.com', FALSE], + // Some browsers ignore or strip leading control characters. + ["\x00//www.example.ca", 'http://example.com', FALSE], ]; } diff --git a/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php index 8659a6f126..85b3da313c 100644 --- a/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php @@ -11,7 +11,6 @@ use Symfony\Component\HttpFoundation\RedirectResponse; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpKernel\Event\FilterResponseEvent; -use Symfony\Component\HttpKernel\Event\GetResponseEvent; use Symfony\Component\HttpKernel\HttpKernelInterface; use Symfony\Component\HttpKernel\KernelEvents; @@ -192,74 +191,4 @@ public function providerTestDestinationRedirectWithInvalidUrl() { return $data; } - /** - * Tests that $_GET only contain internal URLs. - * - * @covers ::sanitizeDestination - * - * @dataProvider providerTestSanitizeDestination - * - * @see \Drupal\Component\Utility\UrlHelper::isExternal - */ - public function testSanitizeDestinationForGet($input, $output) { - $request = new Request(); - $request->query->set('destination', $input); - - $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext); - $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); - $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); - - $dispatcher = new EventDispatcher(); - $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'sanitizeDestination'], 100); - $dispatcher->dispatch(KernelEvents::REQUEST, $event); - - $this->assertEquals($output, $request->query->get('destination')); - } - - /** - * Tests that $_REQUEST['destination'] only contain internal URLs. - * - * @covers ::sanitizeDestination - * - * @dataProvider providerTestSanitizeDestination - * - * @see \Drupal\Component\Utility\UrlHelper::isExternal - */ - public function testSanitizeDestinationForPost($input, $output) { - $request = new Request(); - $request->request->set('destination', $input); - - $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext); - $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'); - $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST); - - $dispatcher = new EventDispatcher(); - $dispatcher->addListener(KernelEvents::REQUEST, [$listener, 'sanitizeDestination'], 100); - $dispatcher->dispatch(KernelEvents::REQUEST, $event); - - $this->assertEquals($output, $request->request->get('destination')); - } - - /** - * Data provider for testSanitizeDestination(). - */ - public function providerTestSanitizeDestination() { - $data = []; - // Standard internal example node path is present in the 'destination' - // parameter. - $data[] = ['node', 'node']; - // Internal path with one leading slash is allowed. - $data[] = ['/example.com', '/example.com']; - // External URL without scheme is not allowed. - $data[] = ['//example.com/test', '']; - // Internal URL using a colon is allowed. - $data[] = ['example:test', 'example:test']; - // External URL is not allowed. - $data[] = ['http://example.com', '']; - // Javascript URL is allowed because it is treated as an internal URL. - $data[] = ['javascript:alert(0)', 'javascript:alert(0)']; - - return $data; - } - } diff --git a/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php b/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php index ea523028a8..de2e3d943f 100644 --- a/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php +++ b/core/tests/Drupal/Tests/Core/Mail/MailManagerTest.php @@ -7,6 +7,7 @@ namespace Drupal\Tests\Core\Mail; +use Drupal\Core\DependencyInjection\ContainerBuilder; use Drupal\Core\Render\RenderContext; use Drupal\Core\Render\RendererInterface; use Drupal\Tests\UnitTestCase; @@ -103,6 +104,9 @@ protected function setUpMailManager($interface = []) { 'system.mail' => [ 'interface' => $interface, ], + 'system.site' => [ + 'mail' => 'test@example.com', + ], ]); $logger_factory = $this->getMock('\Drupal\Core\Logger\LoggerChannelFactoryInterface'); $string_translation = $this->getStringTranslationStub(); @@ -110,6 +114,11 @@ protected function setUpMailManager($interface = []) { // Construct the manager object and override its discovery. $this->mailManager = new TestMailManager(new \ArrayObject(), $this->cache, $this->moduleHandler, $this->configFactory, $logger_factory, $string_translation, $this->renderer); $this->mailManager->setDiscovery($this->discovery); + + // @see \Drupal\Core\Plugin\Factory\ContainerFactory::createInstance() + $container = new ContainerBuilder(); + $container->set('config.factory', $this->configFactory); + \Drupal::setContainer($container); } /** diff --git a/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php b/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php index 53147f3b7d..e828de086e 100644 --- a/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php +++ b/core/tests/Drupal/Tests/Core/Security/RequestSanitizerTest.php @@ -197,6 +197,147 @@ public function providerTestRequestSanitization() { return $tests; } + /** + * Tests acceptable destinations are not removed from GET requests. + * + * @param string $destination + * The destination string to test. + * + * @dataProvider providerTestAcceptableDestinations + */ + public function testAcceptableDestinationGet($destination) { + // Set up a GET request. + $request = $this->createRequestForTesting(['destination' => $destination]); + + $request = RequestSanitizer::sanitize($request, [], TRUE); + + $this->assertSame($destination, $request->query->get('destination', NULL)); + $this->assertNull($request->request->get('destination', NULL)); + $this->assertSame($destination, $_GET['destination']); + $this->assertSame($destination, $_REQUEST['destination']); + $this->assertArrayNotHasKey('destination', $_POST); + $this->assertEquals([], $this->errors); + } + + /** + * Tests unacceptable destinations are removed from GET requests. + * + * @param string $destination + * The destination string to test. + * + * @dataProvider providerTestSanitizedDestinations + */ + public function testSanitizedDestinationGet($destination) { + // Set up a GET request. + $request = $this->createRequestForTesting(['destination' => $destination]); + + $request = RequestSanitizer::sanitize($request, [], TRUE); + + $this->assertNull($request->request->get('destination', NULL)); + $this->assertNull($request->query->get('destination', NULL)); + $this->assertArrayNotHasKey('destination', $_POST); + $this->assertArrayNotHasKey('destination', $_REQUEST); + $this->assertArrayNotHasKey('destination', $_GET); + $this->assertError('Potentially unsafe destination removed from query parameter bag because it points to an external URL.', E_USER_NOTICE); + } + + /** + * Tests acceptable destinations are not removed from POST requests. + * + * @param string $destination + * The destination string to test. + * + * @dataProvider providerTestAcceptableDestinations + */ + public function testAcceptableDestinationPost($destination) { + // Set up a POST request. + $request = $this->createRequestForTesting([], ['destination' => $destination]); + + $request = RequestSanitizer::sanitize($request, [], TRUE); + + $this->assertSame($destination, $request->request->get('destination', NULL)); + $this->assertNull($request->query->get('destination', NULL)); + $this->assertSame($destination, $_POST['destination']); + $this->assertSame($destination, $_REQUEST['destination']); + $this->assertArrayNotHasKey('destination', $_GET); + $this->assertEquals([], $this->errors); + } + + /** + * Tests unacceptable destinations are removed from GET requests. + * + * @param string $destination + * The destination string to test. + * + * @dataProvider providerTestSanitizedDestinations + */ + public function testSanitizedDestinationPost($destination) { + // Set up a POST request. + $request = $this->createRequestForTesting([], ['destination' => $destination]); + + $request = RequestSanitizer::sanitize($request, [], TRUE); + + $this->assertNull($request->request->get('destination', NULL)); + $this->assertNull($request->query->get('destination', NULL)); + $this->assertArrayNotHasKey('destination', $_POST); + $this->assertArrayNotHasKey('destination', $_REQUEST); + $this->assertArrayNotHasKey('destination', $_GET); + $this->assertError('Potentially unsafe destination removed from request parameter bag because it points to an external URL.', E_USER_NOTICE); + } + + /** + * Creates a request and sets PHP globals for testing. + * + * @param array $query + * (optional) The GET parameters. + * @param array $request + * (optional) The POST parameters. + * + * @return \Symfony\Component\HttpFoundation\Request + * The request object. + */ + protected function createRequestForTesting(array $query = [], array $request = []) { + $request = new Request($query, $request); + + // Set up globals. + $_GET = $request->query->all(); + $_POST = $request->request->all(); + $_COOKIE = $request->cookies->all(); + $_REQUEST = array_merge($request->query->all(), $request->request->all()); + $request->server->set('QUERY_STRING', http_build_query($request->query->all())); + $_SERVER['QUERY_STRING'] = $request->server->get('QUERY_STRING'); + return $request; + } + + /** + * Data provider for testing acceptable destinations. + */ + public function providerTestAcceptableDestinations() { + $data = []; + // Standard internal example node path is present in the 'destination' + // parameter. + $data[] = ['node']; + // Internal path with one leading slash is allowed. + $data[] = ['/example.com']; + // Internal URL using a colon is allowed. + $data[] = ['example:test']; + // Javascript URL is allowed because it is treated as an internal URL. + $data[] = ['javascript:alert(0)']; + return $data; + } + + /** + * Data provider for testing sanitized destinations. + */ + public function providerTestSanitizedDestinations() { + $data = []; + // External URL without scheme is not allowed. + $data[] = ['//example.com/test']; + // External URL is not allowed. + $data[] = ['http://example.com']; + return $data; + } + /** * Catches and logs errors to $this->errors. * -- GitLab