Skip to content
Snippets Groups Projects
Unverified Commit 2ab4c467 authored by Alex Pott's avatar Alex Pott
Browse files

Issue #3038241 by seanB, Wim Leers, phenaproxima, alexpott: Implement a... 

Issue #3038241 by seanB, Wim Leers, phenaproxima, alexpott: Implement a tamper-proof hash for the media library state
parent c7beade9
No related branches found
No related tags found
2 merge requests!7452Issue #1797438. HTML5 validation is preventing form submit and not fully...,!789Issue #3210310: Adjust Database API to remove deprecated Drupal 9 code in Drupal 10
Hide whitespace changes
Inline Side-by-side
Showing
with 212 additions and 40 deletions
core/modules/media_library/src/MediaLibraryState.php
+ 56
− 6
View file @ 2ab4c467
......@@ -2,8 +2,11 @@
namespace Drupal\media_library;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\Site\Settings;
use Symfony\Component\HttpFoundation\ParameterBag;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
/**
* A value object for the media library state.
......@@ -41,6 +44,8 @@ class MediaLibraryState extends ParameterBag {
public function __construct(array $parameters = []) {
$this->validateParameters($parameters['media_library_opener_id'], $parameters['media_library_allowed_types'], $parameters['media_library_selected_type'], $parameters['media_library_remaining']);
parent::__construct($parameters);
// Add a hash to the state parameters.
$this->set('hash', $this->getHash());
}
/**
......@@ -60,12 +65,13 @@ public function __construct(array $parameters = []) {
* A state object.
*/
public static function create($opener_id, array $allowed_media_type_ids, $selected_type_id, $remaining_slots) {
return new static([
$state = new static([
'media_library_opener_id' => $opener_id,
'media_library_allowed_types' => $allowed_media_type_ids,
'media_library_selected_type' => $selected_type_id,
'media_library_remaining' => $remaining_slots,
]);
return $state;
}
/**
......@@ -76,19 +82,32 @@ public static function create($opener_id, array $allowed_media_type_ids, $select
*
* @return \Drupal\media_library\MediaLibraryState
* A state object.
*
* @throws \Symfony\Component\HttpKernel\Exception\BadRequestHttpException
* Thrown when the hash query parameter is invalid.
*/
public static function fromRequest(Request $request) {
$query = $request->query;
// Create a MediaLibraryState object through the create method to make sure
// all validation runs.
$state = static::create(
$request->query->get('media_library_opener_id'),
$request->query->get('media_library_allowed_types'),
$request->query->get('media_library_selected_type'),
$request->query->get('media_library_remaining')
$query->get('media_library_opener_id'),
$query->get('media_library_allowed_types'),
$query->get('media_library_selected_type'),
$query->get('media_library_remaining')
);
// The request parameters need to contain a valid hash to prevent a
// malicious user modifying the query string to attempt to access
// inaccessible information.
if (!$state->isValidHash($query->get('hash'))) {
throw new BadRequestHttpException("Invalid media library parameters specified.");
}
// Once we have validated the required parameters, we restore the parameters
// from the request since there might be additional values.
$state->replace($request->query->all());
$state->replace($query->all());
return $state;
}
......@@ -140,6 +159,37 @@ protected function validateParameters($opener_id, array $allowed_media_type_ids,
}
}
/**
* Get the hash for the state object.
*
* @return string
* The hashed parameters.
*/
public function getHash() {
// Create a hash from the required state parameters.
$hash = implode(':', [
$this->getOpenerId(),
implode(':', $this->getAllowedTypeIds()),
$this->getSelectedTypeId(),
$this->getAvailableSlots(),
]);
return Crypt::hmacBase64($hash, \Drupal::service('private_key')->get() . Settings::getHashSalt());
}
/**
* Validate a hash for the state object.
*
* @param string $hash
* The hash to validate.
*
* @return string
* The hashed parameters.
*/
public function isValidHash($hash) {
return Crypt::hashEquals($this->getHash(), $hash);
}
/**
* Returns the ID of the opener of the media library.
*
......
core/modules/media_library/src/MediaLibraryUiBuilder.php
+ 23
− 15
View file @ 2ab4c467
......@@ -6,12 +6,12 @@
use Drupal\Core\Form\FormBuilderInterface;
use Drupal\Core\Form\FormState;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\EventSubscriber\MainContentViewSubscriber;
use Drupal\Core\Session\AccountInterface;
use Drupal\Core\StringTranslation\StringTranslationTrait;
use Drupal\Core\Url;
use Drupal\views\ViewExecutableFactory;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
/**
* Service which builds the media library.
......@@ -159,12 +159,26 @@ protected function buildLibraryContent(MediaLibraryState $state) {
* Check access to the media library.
*
* @param \Drupal\Core\Session\AccountInterface $account
* Run access checks for this account.
* (optional) Run access checks for this account.
* @param \Drupal\media_library\MediaLibraryState $state
* (optional) The current state of the media library, derived from the
* current request.
*
* @return \Drupal\Core\Access\AccessResult
* The access result.
*/
public function checkAccess(AccountInterface $account = NULL) {
public function checkAccess(AccountInterface $account = NULL, MediaLibraryState $state = NULL) {
if (!$state) {
try {
MediaLibraryState::fromRequest($this->request);
}
catch (BadRequestHttpException $e) {
return AccessResult::forbidden($e->getMessage());
}
catch (\InvalidArgumentException $e) {
return AccessResult::forbidden($e->getMessage());
}
}
// Deny access if the view or display are removed.
$view = $this->entityTypeManager->getStorage('view')->load('media_library');
if (!$view) {
......@@ -206,21 +220,15 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
],
];
// Get the state parameters but remove the wrapper format, AJAX form and
// form rebuild parameters. These are internal parameters that should never
// be part of the vertical tab links.
$query = $state->all();
unset($query[MainContentViewSubscriber::WRAPPER_FORMAT], $query[FormBuilderInterface::AJAX_FORM_REQUEST], $query['_media_library_form_rebuild']);
// Add the 'media_library_content' parameter so the response will contain
// only the updated content for the tab.
// @see self::buildUi()
$query['media_library_content'] = 1;
$allowed_types = $this->entityTypeManager->getStorage('media_type')->loadMultiple($allowed_type_ids);
$selected_type_id = $state->getSelectedTypeId();
foreach ($allowed_types as $allowed_type_id => $allowed_type) {
$query['media_library_selected_type'] = $allowed_type_id;
$link_state = MediaLibraryState::create($state->getOpenerId(), $state->getAllowedTypeIds(), $allowed_type_id, $state->getAvailableSlots());
// Add the 'media_library_content' parameter so the response will contain
// only the updated content for the tab.
// @see self::buildUi()
$link_state->set('media_library_content', 1);
$title = $allowed_type->label();
if ($allowed_type_id === $selected_type_id) {
......@@ -232,7 +240,7 @@ protected function buildMediaTypeMenu(MediaLibraryState $state) {
$menu['#links']['media-library-menu-' . $allowed_type_id] = [
'title' => $title,
'url' => Url::fromRoute('media_library.ui', [], [
'query' => $query,
'query' => $link_state->all(),
]),
'attributes' => [
'class' => ['media-library-menu__link'],
......
core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
+ 23
− 0
View file @ 2ab4c467
......@@ -185,6 +185,29 @@ public function testWidgetAccess() {
$this->drupalGet('media-library', $url_options);
$assert_session->elementExists('css', '.view-media-library');
$assert_session->fieldExists('Add files');
// Assert the media library can not be accessed if the required state
// parameters are changed without changing the hash.
$this->drupalGet('media-library', [
'query' => array_merge($url_options['query'], ['media_library_opener_id' => 'fail']),
]);
$assert_session->responseContains('Access denied');
$this->drupalGet('media-library', [
'query' => array_merge($url_options['query'], ['media_library_allowed_types' => ['type_one', 'type_two']]),
]);
$assert_session->responseContains('Access denied');
$this->drupalGet('media-library', [
'query' => array_merge($url_options['query'], ['media_library_selected_type' => 'type_one']),
]);
$assert_session->responseContains('Access denied');
$this->drupalGet('media-library', [
'query' => array_merge($url_options['query'], ['media_library_remaining' => 3]),
]);
$assert_session->responseContains('Access denied');
$this->drupalGet('media-library', [
'query' => array_merge($url_options['query'], ['hash' => 'fail']),
]);
$assert_session->responseContains('Access denied');
}
/**
......
core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
+ 24
− 19
View file @ 2ab4c467
......@@ -2,9 +2,9 @@
namespace Drupal\Tests\media_library\Kernel;
use Drupal\Core\Access\AccessResult;
use Drupal\image\Entity\ImageStyle;
use Drupal\KernelTests\KernelTestBase;
use Drupal\media_library\MediaLibraryState;
use Drupal\Tests\user\Traits\UserCreationTrait;
use Drupal\views\Views;
......@@ -78,6 +78,10 @@ public function testMediaLibraryAccess() {
/** @var \Drupal\media_library\MediaLibraryUiBuilder $ui_builder */
$ui_builder = $this->container->get('media_library.ui_builder');
// Create a media library state to test access.
$state = MediaLibraryState::create('test', ['file', 'image'], 'file', 2);
// Create a clone of the view so we can reset the original later.
$view_original = clone Views::getView('media_library');
// Create our test users.
......@@ -86,13 +90,11 @@ public function testMediaLibraryAccess() {
// Assert the 'view media' permission is needed to access the library and
// validate the cache dependencies.
$this->assertSame(AccessResult::forbidden()->isAllowed(), $ui_builder->checkAccess($forbidden_account)->isAllowed());
$this->assertSame("The 'view media' permission is required.", $ui_builder->checkAccess($forbidden_account)->getReason());
$this->assertSame($view_original->storage->getCacheTags(), $ui_builder->checkAccess($forbidden_account)->getCacheTags());
$this->assertSame(['user.permissions'], $ui_builder->checkAccess($forbidden_account)->getCacheContexts());
$this->assertSame(AccessResult::allowed()->isAllowed(), $ui_builder->checkAccess($allowed_account)->isAllowed());
$this->assertSame($view_original->storage->getCacheTags(), $ui_builder->checkAccess($allowed_account)->getCacheTags());
$this->assertSame(['user.permissions'], $ui_builder->checkAccess($allowed_account)->getCacheContexts());
$access_result = $ui_builder->checkAccess($forbidden_account, $state);
$this->assertFalse($access_result->isAllowed());
$this->assertSame("The 'view media' permission is required.", $access_result->getReason());
$this->assertSame($view_original->storage->getCacheTags(), $access_result->getCacheTags());
$this->assertSame(['user.permissions'], $access_result->getCacheContexts());
// Assert that the media library access is denied when the view widget
// display is deleted.
......@@ -101,25 +103,28 @@ public function testMediaLibraryAccess() {
unset($displays['widget']);
$view_storage->set('display', $displays);
$view_storage->save();
$this->assertSame(AccessResult::forbidden()->isAllowed(), $ui_builder->checkAccess($allowed_account)->isAllowed());
$this->assertSame('The media library widget display does not exist.', $ui_builder->checkAccess($forbidden_account)->getReason());
$this->assertSame($view_original->storage->getCacheTags(), $ui_builder->checkAccess($forbidden_account)->getCacheTags());
$this->assertSame([], $ui_builder->checkAccess($forbidden_account)->getCacheContexts());
$access_result = $ui_builder->checkAccess($allowed_account, $state);
$this->assertFalse($access_result->isAllowed());
$this->assertSame('The media library widget display does not exist.', $access_result->getReason());
$this->assertSame($view_original->storage->getCacheTags(), $access_result->getCacheTags());
$this->assertSame([], $access_result->getCacheContexts());
// Restore the original view and assert that the media library controller
// works again.
$view_original->storage->save();
$this->assertSame(AccessResult::allowed()->isAllowed(), $ui_builder->checkAccess($allowed_account)->isAllowed());
$this->assertSame($view_original->storage->getCacheTags(), $ui_builder->checkAccess($allowed_account)->getCacheTags());
$this->assertSame(['user.permissions'], $ui_builder->checkAccess($allowed_account)->getCacheContexts());
$access_result = $ui_builder->checkAccess($allowed_account, $state);
$this->assertTrue($access_result->isAllowed());
$this->assertSame($view_original->storage->getCacheTags(), $access_result->getCacheTags());
$this->assertSame(['user.permissions'], $access_result->getCacheContexts());
// Assert that the media library access is denied when the entire media
// library view is deleted.
Views::getView('media_library')->storage->delete();
$this->assertSame(AccessResult::forbidden()->isAllowed(), $ui_builder->checkAccess($allowed_account)->isAllowed());
$this->assertSame('The media library view does not exist.', $ui_builder->checkAccess($forbidden_account)->getReason());
$this->assertSame([], $ui_builder->checkAccess($forbidden_account)->getCacheTags());
$this->assertSame([], $ui_builder->checkAccess($forbidden_account)->getCacheContexts());
$access_result = $ui_builder->checkAccess($allowed_account, $state);
$this->assertFalse($access_result->isAllowed());
$this->assertSame('The media library view does not exist.', $access_result->getReason());
$this->assertSame([], $access_result->getCacheTags());
$this->assertSame([], $access_result->getCacheContexts());
}
}
core/modules/media_library/tests/src/Kernel/MediaLibraryStateTest.php
+ 86
− 0
View file @ 2ab4c467
......@@ -5,6 +5,8 @@
use Drupal\KernelTests\KernelTestBase;
use Drupal\media_library\MediaLibraryState;
use Drupal\Tests\media\Traits\MediaTypeCreationTrait;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
/**
* Tests the media library state value object.
......@@ -256,4 +258,88 @@ public function providerCreate() {
return $test_data;
}
/**
* Tests the hash validation when the state is created from a request.
*
* @param array $query_overrides
* The query parameters to override.
* @param bool $exception_expected
* Whether an AccessDeniedHttpException is expected or not.
*
* @covers ::fromRequest
* @dataProvider providerFromRequest
*/
public function testFromRequest(array $query_overrides, $exception_expected) {
// Override the query parameters and verify an exception is thrown when
// required state parameters are changed.
$query = MediaLibraryState::create('test', ['file', 'image'], 'image', 2)->all();
$query = array_merge($query, $query_overrides);
if ($exception_expected) {
$this->setExpectedException(BadRequestHttpException::class, "Invalid media library parameters specified.");
}
$state = MediaLibraryState::fromRequest(new Request($query));
$this->assertInstanceOf(MediaLibraryState::class, $state);
}
/**
* Data provider for testFromRequest().
*
* @return array
* The data sets to test.
*/
public function providerFromRequest() {
$test_data = [];
// Assert no exception is thrown when we use valid state parameters.
$test_data['valid parameters'] = [
[],
FALSE,
];
// Assert no exception is thrown when we override all query parameters with
// the same data.
$test_data['changed with same values'] = [
[
'media_library_opener_id' => 'test',
'media_library_allowed_types' => ['file', 'image'],
'media_library_selected_type' => 'image',
'media_library_remaining' => 2,
],
FALSE,
];
// Assert an exception is thrown when we change the opener ID parameter.
$test_data['changed opener ID'] = [
['media_library_opener_id' => 'fail'],
TRUE,
];
// Assert an exception is thrown when we change the allowed types parameter.
$test_data['changed allowed types'] = [
['media_library_allowed_types' => ['audio', 'image']],
TRUE,
];
// Assert an exception is thrown when we change the selected type parameter.
$test_data['changed selected type'] = [
['media_library_selected_type' => 'file'],
TRUE,
];
// Assert an exception is thrown when we change the remaining slots
// parameter.
$test_data['changed remaining'] = [
['media_library_remaining' => 4],
TRUE,
];
// Assert an exception is thrown when we change the actual hash.
$test_data['changed hash'] = [
['hash' => 'fail'],
TRUE,
];
return $test_data;
}
}
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment