Replace use of whitelist/blacklist in \Drupal\Core\Security\RequestSanitizer and its test

#8.1: SANITIZE_INPUT_SAFE_KEYS ?

#8.2: Yes, I think this whole file needs s/whitelist/safe_keys/. That's what I came up with on a dreditor review before reading #8.

Patch updated according to comment #8 and #9. Please review.

Great, thanks. Getting closer, but a few more things to fix before RTBC:

1. +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
   @@ -17,9 +17,9 @@ class RequestSanitizer {
   +   * The name of the setting that configures the sanitize input safe keys.
   ...
   -  const SANITIZE_WHITELIST = 'sanitize_input_whitelist';
   +  const SANITIZE_INPUT_SAFE_KEYS = 'sanitize_input_safe_keys';
   

   Drat, that means people can have 'sanitize_input_whitelist' in their site settings. :( We're going to need a BC layer, and to trigger_error() if we see the legacy key, etc. This is more complicated than just renaming things in this one file.

2. +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
   @@ -31,15 +31,15 @@ class RequestSanitizer {
   +   * @param string[] $safe_keys
   +   *   An array of keys to sanitize input as safe. See default.settings.php.
   

   A) Simplify first sentence to: "An array of keys to consider safe."

   B) "See default.settings.php" -- good idea! ;) I just checked core/assets/scaffold/files/default.settings.php and see no mention of this setting nor of "whitelist" at all. Weird. Probably we should remove that part of this comment, since there's nothing to see.

   core/lib/Drupal/Core/Security/RequestSanitizer.php is the only file in all of core that mentions the string 'sanitize_input_whitelist'.

3. % egrep -irl SANITIZE_WHITELIST core
   core/lib/Drupal/Core/Security/RequestSanitizer.php
   core/lib/Drupal/Core/DrupalKernel.php
   

   But, checking for the constant, that *is* used. So we also need to fix DrupalKernel.php as part of this patch. Kind of surprising that failing to do so results in no test failures. :(
   Edit: Whoops, it's the first hunk of the patch file. ;) Not sure how I missed that.

4. +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
   @@ -65,8 +65,8 @@ public static function sanitize(Request $request, $whitelist, $log_sanitized_key
   +   *   An array of keys to sanitize request input as safe.
   
   @@ -121,18 +121,18 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo
   +   *   An array of keys to sanitize request input as safe.
   
   @@ -142,23 +142,23 @@ protected static function checkDestination($destination, array $whitelist) {
   +   *   An array of keys to sanitize request input as safe.
   

   "An array of keys to consider safe."

5. +++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
   @@ -121,18 +121,18 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo
   +   * @param array $safe_keys
   

   s/array/string[]/

+++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
@@ -121,18 +121,18 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo
-   *   An array of keys to whitelist as safe.
...
+   *   An array of keys to sanitize request input as safe.

@@ -142,23 +142,23 @@ protected static function checkDestination($destination, array $whitelist) {
-   *   An array of keys to whitelist as safe.
...
+   *   An array of keys to sanitize request input as safe.

The new sentence no longer describes what the parameter does, but rather what the function does.

Maybe this should be An array of allowed keys. or An array of keys considered safe.?

(Edit: Already mentioned in parent comment)

Working on it.

Oh sorry i am looking another issue wrong assignment .I opened two issue same time and assigned wrong.
Apologies for the same it will be great if someone can look on this.

Thanks
Pavnish

Oooh. Yeah, that BC looks trickier than I thought, since the setting is in settings.php rather than configuration.

I can see several approaches:

1. Deprecate RequestSanitizer::SANITIZE_WHITELIST, but leave its value unchanged. Add a validation method Settings::validate() which runs on initialization. It checks for the presence of the old key, emits a warning if needed, then checks for the presence of the new key, and copies the value of the old key to the new key if needed.

2. Deprecate RequestSanitizer::SANITIZER_WHITELIST, and also set it to the new value 'sanitize_input_safe_keys'.

The second will provide BC unless people inlined the value directly instead of using the constant... but we can add the ::validate() step in order to support that as well.

The first provides BC in all cases, and also ensures that the deprecation is both marked by PhpDoc and emitted in the log, which may be desirable for rarely changed files like settings.php that developers may not check for deprecations.

Option 1.

Missed a spot.

This should cover it.

1. If neither key is set, nothing happens.
2. If only the old key is set, it's copied into the new key and there is a deprecation notice.
3. If only the new key is set, it's copied into the old key.
4. If (strangely) both keys are set, the new key overwrites the old to ensure consistency.

Draft CR: https://www.drupal.org/node/3163148

Still needs deprecation tests, I guess.

Assuming #3163226-13: Add the ability to deprecate a Settings name is committed more or less as-is, here's a reroll of #21 on top of that. "do-not-test" since it won't work until the other is committed. But I wanted to show how this would work once that's done.

Thanks,
-Derek

#25 provides the tests we were missing here, so untagging.

Also, preemptively fix a few CS bugs in #25. ;)

Although #3163226: Add the ability to deprecate a Settings name is in, let's leave this Postponed on #3151094: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests to hash out if/how we want to do deprecation testing for real deprecated settings.

Thanks,
-Derek

If it turns out we agree that we don't need separate (duplicate?) test coverage of the deprecated settings API, this should be all we need.

It'll still conflict with #3151094: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests for the change to the Settings::$deprecatedSettings array, but that'll be easy to resolve on whichever issue lands 2nd.

Reroll now that #3151094: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests is in, and that our plan for testing real deprecated settings is clear. Interdiff is confused by the (expected) merge conflict, so here's a raw diff of the patch files.

This will need to remove the @runInSeparateProcess if #3164161: Move the @runInSeparateProcess annotation to the SettingsTest class so it covers all test methods lands first, or that issue will need a re-roll to remove it if this lands first. ;) Cross referencing via related issues.

Almost there! ;)

Thanks,
-Derek

According to #29, if #3164161: Move the @runInSeparateProcess annotation to the SettingsTest class so it covers all test methods is committed first, this will need a reroll. Tagging as such and setting to NW.

Great, thanks.

2 patches for consideration:

1. 3151093-33.dataprovider.patch: Re-asking #3151094-57: Replace use of whitelist/blacklist in \Drupal\Core\Template classes and their tests in the form of a patch:

   +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
   @@ -272,6 +272,27 @@ public function providerTestFakeDeprecatedSettings(): array {
      /**
   +   * Tests legacy sanitize_input_whitelist setting.
   +   *
   +   * @group legacy
   +   *
   +   * @expectedDeprecation The "sanitize_input_whitelist" setting is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use Drupal\Core\Security\RequestSanitizer::SANITIZE_INPUT_SAFE_KEYS instead. See https://www.drupal.org/node/3163148
   +   */
   +  public function testLegacySanitizeInputSetting(): void {
   +    $settings = <<<'EOD'
   +<?php
   +$settings['sanitize_input_whitelist'] = ['a', 'b'];
   +EOD;
   +    $class_loader = NULL;
   +    $vfs_root = vfsStream::setup('root');
   +    $sites_directory = vfsStream::newDirectory('sites')->at($vfs_root);
   +    vfsStream::newFile('settings.php')
   +      ->at($sites_directory)
   +      ->setContent($settings);
   +    Settings::initialize(vfsStream::url('root'), 'sites', $class_loader);
   +  }
   

   Instead of replicating almost this entire function with the existing one, how about converting all the real deprecation tests into a single method with a @dataProvider?

2. +++ b/core/lib/Drupal/Core/Site/Settings.php
   @@ -38,6 +38,10 @@
   +      'message' => 'The "sanitize_input_whitelist" setting is deprecated in drupal:9.1.0 and will be removed in drupal:10.0.0. Use Drupal\Core\Security\RequestSanitizer::SANITIZE_INPUT_SAFE_KEYS instead. See https://www.drupal.org/node/3163148',
   

   Realized this wants to end with a period. Previous patch includes this fix, too. But in case 1 is controversial, 3151093-33.period.patch is #32 with only this fixed.

Thoughts?

Thanks!
-Derek

#33 dataprovider version looks good to me, as you say the code is identical so why repeat it, we can put it all in one place and remove individual parts or or all of it in one go when Drupal 10 opens.

Committed ae59215 and pushed to 9.1.x. Thanks!

+++ b/core/lib/Drupal/Core/Security/RequestSanitizer.php
@@ -91,7 +100,7 @@ protected static function processParameterBag(ParameterBag $bag, $whitelist, $lo
     if ($bag->has('destination')) {
       $destination = $bag->get('destination');
-      $destination_dangerous_keys = static::checkDestination($destination, $whitelist);
+      $destination_dangerous_keys = static::checkDestination($destination, $safe_keys);
       if (!empty($destination_dangerous_keys)) {

The fact we already use 'dangerous keys' in the code makes 'safe keys' a very good choice.

Sweet, thanks! I published the CR.

The fact we already use 'dangerous keys' in the code makes 'safe keys' a very good choice.

+1

Cheers,
-Derek

p.s. I notice you opted for the @dataProvider patch. I hope @alexpott agrees. ;)