diff --git a/core/lib/Drupal/Component/Utility/UrlHelper.php b/core/lib/Drupal/Component/Utility/UrlHelper.php index 1d133c9d6b1508a2d183b3858d0af547f0c71c15..9e2365c1aef5c1de8b311a3f1d50f807b3b37972 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 8397bdef4e50e5f25b1e48e6bd4bcc7ebfcbed00..67a4aae4220ff236716353dfc4fe771cde391121 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 e5361492b591148f7b488c432036208e837b8fad..27e5c76e5d8977a9d2940e6f460c30c812a1077d 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 b85737f3cf13da873875959633c315fa93904bee..d0690fee203dd65644c5779a662f1bfdba51a44f 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 3b5c8d256b1037266fcd1c0d20b4855c545f0ee3..853a5f7b4e1babda77e423a4777e49b433b12f2d 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 3f48f3d59cb7ff1d189f6be84755534394c4ff36..e1626ed3831fc5315592b473e4343f44bc2e256c 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 0a18d7fa0fd74c87a9dbe4da2d34bd55e93b0862..cdb19982415b790feca0ba70185bfec32b90626c 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 7f7c756b6b9bcb60d6934e438386eec3225458a3..4fcde36059a454ab8efa631fc89291fb1cd4f03e 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 65fc2a0c509a1110375c53d5241e21f1eeb73a79..c3b9c815fe05d156ec6216b3404a8290e41e5d04 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 01b2ad8458200c1a2488aa7957343cf70f805858..35d657e5503ac150e06764461e95185d42c547b6 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 1acbf052fd064607aa015bfa94a7fcab83d9564c..c793fe53e275309ae53d60557a3d1e94c7bce525 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 11deaa72c061096ff003c5b935a59d5bf6deafa5..5fd168d0bb5e38ee585bb18d6b4245e5e0dfe4ad 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 dc1e7f6917f92b69fc350c7196258f9b59af355b..df90bf63c88ad06f095fb6c62c15994fbbce5d8d 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 b9d61b76d2e732ce93fad1ee5756cfc1c94a8535..8b9fc36fd7b1011d14d45d2b3f5b37873efa6372 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 0000000000000000000000000000000000000000..8decad05f07efc33dc92af8acc1a32d69c90f8cc --- /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 049233b4e139a0c6c64a5c39b20897ddcc749454..d51eba21a94445292907307279b31095932341df 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 58e42ecd6ba140b2c25bf7405231f1478dff9b75..d05c6a852726e0f5c220de88b9b65de1cd48da45 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 97afde9a244ff583c9a693b237b2a19de2785cd6..5e993941a6aafc798c301de9e93425202f3d4dd4 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 340b60821fb0da11c9398f0460d33229106a8d91..74a6d504e80ad81515d4402365b22f61871e39f3 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/node/src/Tests/Views/NodeContextualLinksTest.php b/core/modules/node/src/Tests/Views/NodeContextualLinksTest.php deleted file mode 100644 index dc23d0ce55f297cda9b74b77908c0e3034687d1b..0000000000000000000000000000000000000000 --- a/core/modules/node/src/Tests/Views/NodeContextualLinksTest.php +++ /dev/null @@ -1,118 +0,0 @@ -drupalCreateContentType(['type' => 'page']); - $this->drupalCreateNode(['promote' => 1]); - $this->drupalGet('node'); - - $user = $this->drupalCreateUser(['administer nodes', 'access contextual links']); - $this->drupalLogin($user); - - $response = $this->renderContextualLinks(['node:node=1:'], 'node'); - $this->assertResponse(200); - $json = Json::decode($response); - $this->setRawContent($json['node:node=1:']); - - // @todo Add these back when the functionality for making Views displays - // appear in contextual links is working again. - // $this->assertLinkByHref('node/1/contextual-links', 0, 'The contextual link to the view was found.'); - // $this->assertLink('Test contextual link', 0, 'The contextual link to the view was found.'); - } - - /** - * Get server-rendered contextual links for the given contextual link ids. - * - * Copied from \Drupal\contextual\Tests\ContextualDynamicContextTest::renderContextualLinks(). - * - * @param array $ids - * An array of contextual link ids. - * @param string $current_path - * The Drupal path for the page for which the contextual links are rendered. - * - * @return string - * The response body. - */ - protected function renderContextualLinks($ids, $current_path) { - // Build POST values. - $post = []; - for ($i = 0; $i < count($ids); $i++) { - $post['ids[' . $i . ']'] = $ids[$i]; - } - - // Serialize POST values. - foreach ($post as $key => $value) { - // Encode according to application/x-www-form-urlencoded - // Both names and values needs to be urlencoded, according to - // http://www.w3.org/TR/html4/interact/forms.html#h-17.13.4.1 - $post[$key] = urlencode($key) . '=' . urlencode($value); - } - $post = implode('&', $post); - - // Perform HTTP request. - return $this->curlExec([ - CURLOPT_URL => \Drupal::url('contextual.render', [], ['absolute' => TRUE, 'query' => ['destination' => $current_path]]), - CURLOPT_POST => TRUE, - CURLOPT_POSTFIELDS => $post, - CURLOPT_HTTPHEADER => [ - 'Accept: application/json', - 'Content-Type: application/x-www-form-urlencoded', - ], - ]); - } - - /** - * Tests if the node page works if Contextual Links is disabled. - * - * All views have Contextual links enabled by default, even with the - * Contextual links module disabled. This tests if no calls are done to the - * Contextual links module by views when it is disabled. - * - * @see https://www.drupal.org/node/2379811 - */ - public function testPageWithDisabledContextualModule() { - \Drupal::service('module_installer')->uninstall(['contextual']); - \Drupal::service('module_installer')->install(['views_ui']); - - // Ensure that contextual links don't get called for admin users. - $admin_user = User::load(1); - $admin_user->setPassword('new_password'); - $admin_user->pass_raw = 'new_password'; - $admin_user->save(); - - $this->drupalCreateContentType(['type' => 'page']); - $this->drupalCreateNode(['promote' => 1]); - - $this->drupalLogin($admin_user); - $this->drupalGet('node'); - } - -} diff --git a/core/modules/node/src/Tests/NodeRevisionsTest.php b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php similarity index 92% rename from core/modules/node/src/Tests/NodeRevisionsTest.php rename to core/modules/node/tests/src/Functional/NodeRevisionsTest.php index fdc929a84c7002e2ca4b10f7e68f6dddac122407..6b16ce8bdcec93f627ba1300934b687f7e032178 100644 --- a/core/modules/node/src/Tests/NodeRevisionsTest.php +++ b/core/modules/node/tests/src/Functional/NodeRevisionsTest.php @@ -1,6 +1,6 @@ assertTrue($node->isDefaultRevision(), 'Third node revision is the default one.'); - // Confirm that the "Edit" and "Delete" contextual links appear for the - // default revision. - $ids = ['node:node=' . $node->id() . ':changed=' . $node->getChangedTime()]; - $json = $this->renderContextualLinks($ids, 'node/' . $node->id()); - $this->verbose($json[$ids[0]]); - - $expected = '
  • Edit
    • '; - $this->assertTrue(strstr($json[$ids[0]], $expected), 'The "Edit" contextual link is shown for the default revision.'); - $expected = '
  • Delete
    • '; - $this->assertTrue(strstr($json[$ids[0]], $expected), 'The "Delete" contextual link is shown for the default revision.'); - // Confirm that revisions revert properly. $this->drupalPostForm("node/" . $node->id() . "/revisions/" . $nodes[1]->getRevisionid() . "/revert", [], t('Revert')); $this->assertRaw(t('@type %title has been reverted to the revision from %revision-date.', [ @@ -188,15 +177,6 @@ public function testRevisions() { $node = node_revision_load($node->getRevisionId()); $this->assertFalse($node->isDefaultRevision(), 'Third node revision is not the default one.'); - // Confirm that "Edit" and "Delete" contextual links don't appear for - // non-default revision. - $ids = ['node_revision::node=' . $node->id() . '&node_revision=' . $node->getRevisionId() . ':']; - $json = $this->renderContextualLinks($ids, 'node/' . $node->id() . '/revisions/' . $node->getRevisionId() . '/view'); - $this->verbose($json[$ids[0]]); - - $this->assertFalse(strstr($json[$ids[0]], '
  • '), 'The "Edit" contextual link is not shown for a non-default revision.'); - $this->assertFalse(strstr($json[$ids[0]], '
  • '), 'The "Delete" contextual link is not shown for a non-default revision.'); - // Confirm revisions delete properly. $this->drupalPostForm("node/" . $node->id() . "/revisions/" . $nodes[1]->getRevisionId() . "/delete", [], t('Delete')); $this->assertRaw(t('Revision from %revision-date of @type %title has been deleted.', [ diff --git a/core/modules/node/src/Tests/NodeTypeTest.php b/core/modules/node/tests/src/Functional/NodeTypeTest.php similarity index 94% rename from core/modules/node/src/Tests/NodeTypeTest.php rename to core/modules/node/tests/src/Functional/NodeTypeTest.php index 9938bb0ab53cfd4d75fe2da892bd5ea07e06af43..84c549e8d5b323eac171cb800dda37f67d5e4eb0 100644 --- a/core/modules/node/src/Tests/NodeTypeTest.php +++ b/core/modules/node/tests/src/Functional/NodeTypeTest.php @@ -1,11 +1,12 @@ assertSession(); $this->drupalPlaceBlock('system_breadcrumb_block'); $web_user = $this->drupalCreateUser(['bypass node access', 'administer content types', 'administer node fields']); $this->drupalLogin($web_user); @@ -96,8 +99,8 @@ public function testNodeTypeEditing() { // Verify that title and body fields are displayed. $this->drupalGet('node/add/page'); - $this->assertRaw('Title', 'Title field was found.'); - $this->assertRaw('Body', 'Body field was found.'); + $assert->pageTextContains('Title'); + $assert->pageTextContains('Body'); // Rename the title field. $edit = [ @@ -106,8 +109,8 @@ public function testNodeTypeEditing() { $this->drupalPostForm('admin/structure/types/manage/page', $edit, t('Save content type')); $this->drupalGet('node/add/page'); - $this->assertRaw('Foo', 'New title label was displayed.'); - $this->assertNoRaw('Title', 'Old title label was not displayed.'); + $assert->pageTextContains('Foo'); + $assert->pageTextNotContains('Title'); // Change the name and the description. $edit = [ @@ -117,11 +120,11 @@ public function testNodeTypeEditing() { $this->drupalPostForm('admin/structure/types/manage/page', $edit, t('Save content type')); $this->drupalGet('node/add'); - $this->assertRaw('Bar', 'New name was displayed.'); - $this->assertRaw('Lorem ipsum', 'New description was displayed.'); + $assert->pageTextContains('Bar'); + $assert->pageTextContains('Lorem ipsum'); $this->clickLink('Bar'); - $this->assertRaw('Foo', 'Title field was found.'); - $this->assertRaw('Body', 'Body field was found.'); + $assert->pageTextContains('Foo'); + $assert->pageTextContains('Body'); // Change the name through the API /** @var \Drupal\node\NodeTypeInterface $node_type */ @@ -146,7 +149,7 @@ public function testNodeTypeEditing() { ]); // Check that the body field doesn't exist. $this->drupalGet('node/add/page'); - $this->assertNoRaw('Body', 'Body field was not found.'); + $assert->pageTextNotContains('Body'); } /** diff --git a/core/modules/node/src/Tests/PagePreviewTest.php b/core/modules/node/tests/src/Functional/PagePreviewTest.php similarity index 97% rename from core/modules/node/src/Tests/PagePreviewTest.php rename to core/modules/node/tests/src/Functional/PagePreviewTest.php index 2bc9cd3ce1e00c266a45d0d1dc34e6d797cb9bca..70305349d46d1859fdaf651c3ec234007e1c3f35 100644 --- a/core/modules/node/src/Tests/PagePreviewTest.php +++ b/core/modules/node/tests/src/Functional/PagePreviewTest.php @@ -1,6 +1,6 @@ drupalPostForm(NULL, ['field_image[0][alt]' => 'Picture of llamas'], t('Preview')); // Check that the preview is displaying the title, body and term. - $this->assertTitle(t('@title | Drupal', ['@title' => $edit[$title_key]]), 'Basic page title is preview.'); + $expected_title = $edit[$title_key] . ' | Drupal'; + $this->assertSession()->titleEquals($expected_title); $this->assertEscaped($edit[$title_key], 'Title displayed and escaped.'); $this->assertText($edit[$body_key], 'Body displayed.'); $this->assertText($edit[$term_key], 'Term displayed.'); @@ -210,13 +215,13 @@ public function testPagePreview() { $this->assertFieldByName($body_key, $edit[$body_key], 'Body field displayed.'); $this->assertFieldByName($term_key, $edit[$term_key], 'Term field displayed.'); $this->assertFieldByName('field_image[0][alt]', 'Picture of llamas'); - $this->drupalPostAjaxForm(NULL, [], ['field_test_multi_add_more' => t('Add another item')], NULL, [], [], 'node-page-form'); + $this->getSession()->getPage()->pressButton('Add another item'); $this->assertFieldByName('field_test_multi[0][value]'); $this->assertFieldByName('field_test_multi[1][value]'); // Return to page preview to check everything is as expected. $this->drupalPostForm(NULL, [], t('Preview')); - $this->assertTitle(t('@title | Drupal', ['@title' => $edit[$title_key]]), 'Basic page title is preview.'); + $this->assertSession()->titleEquals($expected_title); $this->assertEscaped($edit[$title_key], 'Title displayed and escaped.'); $this->assertText($edit[$body_key], 'Body displayed.'); $this->assertText($edit[$term_key], 'Term displayed.'); @@ -353,8 +358,8 @@ public function testPagePreview() { $this->assertText('Basic page ' . $title . ' has been created.'); $node = $this->drupalGetNodeByTitle($title); $this->drupalGet('node/' . $node->id() . '/edit'); - $this->drupalPostAjaxForm(NULL, [], ['field_test_multi_add_more' => t('Add another item')]); - $this->drupalPostAjaxForm(NULL, [], ['field_test_multi_add_more' => t('Add another item')]); + $this->getSession()->getPage()->pressButton('Add another item'); + $this->getSession()->getPage()->pressButton('Add another item'); $edit = [ 'field_test_multi[1][value]' => $example_text_2, 'field_test_multi[2][value]' => $example_text_3, diff --git a/core/modules/node/tests/src/Functional/Views/NodeContextualLinksTest.php b/core/modules/node/tests/src/Functional/Views/NodeContextualLinksTest.php new file mode 100644 index 0000000000000000000000000000000000000000..73ccfef7582afc4d5176dcb686a1ab0a25e99a9d --- /dev/null +++ b/core/modules/node/tests/src/Functional/Views/NodeContextualLinksTest.php @@ -0,0 +1,47 @@ +uninstall(['contextual']); + \Drupal::service('module_installer')->install(['views_ui']); + + // Ensure that contextual links don't get called for admin users. + $admin_user = User::load(1); + $admin_user->setPassword('new_password'); + $admin_user->passRaw = 'new_password'; + $admin_user->save(); + + $this->drupalCreateContentType(['type' => 'page']); + $this->drupalCreateNode(['promote' => 1]); + + $this->drupalLogin($admin_user); + $this->drupalGet('node'); + } + +} diff --git a/core/modules/node/tests/src/FunctionalJavascript/ContextualLinksTest.php b/core/modules/node/tests/src/FunctionalJavascript/ContextualLinksTest.php new file mode 100644 index 0000000000000000000000000000000000000000..98051262ec288ecaf838ee603ce06b4d95342dce --- /dev/null +++ b/core/modules/node/tests/src/FunctionalJavascript/ContextualLinksTest.php @@ -0,0 +1,117 @@ +drupalCreateContentType([ + 'type' => 'page', + 'name' => 'Basic page', + 'display_submitted' => FALSE, + ]); + + // Create initial node. + $node = $this->drupalCreateNode(); + + $nodes = []; + + // Get original node. + $nodes[] = clone $node; + + // Create two revisions. + $revision_count = 2; + for ($i = 0; $i < $revision_count; $i++) { + + // Create revision with a random title and body and update variables. + $node->title = $this->randomMachineName(); + $node->body = [ + 'value' => $this->randomMachineName(32), + 'format' => filter_default_format(), + ]; + $node->setNewRevision(); + + $node->save(); + + // Make sure we get revision information. + $node = Node::load($node->id()); + $nodes[] = clone $node; + } + + $this->nodes = $nodes; + + $this->drupalLogin($this->createUser( + [ + 'view page revisions', + 'revert page revisions', + 'delete page revisions', + 'edit any page content', + 'delete any page content', + 'access contextual links', + 'administer content types', + ] + )); + } + + /** + * Tests the contextual links on revisions. + */ + public function testRevisionContextualLinks() { + // Confirm that the "Edit" and "Delete" contextual links appear for the + // default revision. + $this->drupalGet('node/' . $this->nodes[0]->id()); + $page = $this->getSession()->getPage(); + $page->waitFor(10, function () use ($page) { + return $page->find('css', "main .contextual"); + }); + + $this->toggleContextualTriggerVisibility('main'); + $page->find('css', 'main .contextual button')->press(); + $links = $page->findAll('css', "main .contextual-links li a"); + + $this->assertEquals('Edit', $links[0]->getText()); + $this->assertEquals('Delete', $links[1]->getText()); + + // Confirm that "Edit" and "Delete" contextual links don't appear for + // non-default revision. + $this->drupalGet("node/" . $this->nodes[0]->id() . "/revisions/" . $this->nodes[1]->getRevisionId() . "/view"); + $this->assertSession()->pageTextContains($this->nodes[1]->getTitle()); + $page->waitFor(10, function () use ($page) { + return $page->find('css', "main .contextual"); + }); + + $this->toggleContextualTriggerVisibility('main'); + $contextual_button = $page->find('css', 'main .contextual button'); + $this->assertEmpty(0, $contextual_button); + } + +} diff --git a/core/modules/path/tests/src/Functional/PathAliasTest.php b/core/modules/path/tests/src/Functional/PathAliasTest.php index b8ac5968db2607fbbbb54e354dd7140fc69852c4..19115cd27b5c6d7f25509c4bb2a7263d33837eab 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 83a9c55b39775589e2eeb1640f1b7eb36644df06..8d7c43e86a883a2b06167a3a9d27dacb615ce771 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 d185219c9a2ae898cc6d65afff378af536a915e6..beaa472c26b2be24f15fe717ae4404a103c55f92 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 8659a6f12647a58e1879758339bdc1b421f9f288..85b3da313cd0119e82e42b9b670236cbabb87d1a 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 ea523028a8dcb87b10d8a412daac506f04c31b96..de2e3d943faa6f13b6e9a21d3d42d6da070c5eff 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 53147f3b7da45f429ddaa372c43ea0ff6ce8a26f..e828de086e0b451fe5e239315ba6eb7762b211cd 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. *