Problem/Motivation

Lets remove usage of "blacklist" and "whitelist" in Filter module.

They are:

  • An historic bad labelling of people
  • Provide no context: "what is listed in them"?

Proposed resolution

core/modules/filter/src/Element/TextFormat: s/$blacklist/$keys_not_to_copy/
Otherwise, it's all comments. Generally replace "whitelist" with "allowed" and "blacklist" with "denied".

Remaining tasks

  1. Come up with replacement names.
  2. Decide what needs BC + deprecation and what can be changed directly. Everything are comments and local variables. No BC, deprecations or CR needed.
  3. Fix everything:
    • core/modules/filter/tests/src/Kernel/FilterKernelTest.php
    • core/modules/filter/src/Entity/FilterFormat.php
    • core/modules/filter/src/Plugin/Filter/FilterHtml.php
    • core/modules/filter/src/Element/TextFormat.php
    • core/modules/filter/src/FilterFormatInterface.php
  4. Reviews / improvements.
  5. RTBC.
  6. Commit.

User interface changes

None

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#20 3151101.16_20.interdiff.txt903 bytesdww
#20 3151101-20.patch12.17 KBdww
#16 interdiff_14-16.txt1.46 KBravi.shankar
#16 3151101-16.patch12.18 KBravi.shankar
#14 interdiff_10-14.txt3.86 KBDeepak Goyal
#14 3151101-14.patch12.17 KBDeepak Goyal
#10 interdiff_6-10.txt10.1 KBDeepak Goyal
#10 3151101-10.patch11.95 KBDeepak Goyal
#6 d9whiteblacklist-3151101-6.patch11.37 KBshetpooja04
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

ashishdalvi’s picture

ashishdalvi
Primary language English
Location Mumbai
CreditAttribution: ashishdalvi at QED42 for Drupal India Association commented
Assigned: Unassigned » ashishdalvi

I will be picking this issue today.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes

Flesh out Remaining tasks.

ashishdalvi’s picture

ashishdalvi
Primary language English
Location Mumbai
CreditAttribution: ashishdalvi at QED42 for Drupal India Association commented
Assigned: ashishdalvi » Unassigned
shetpooja04’s picture

shetpooja04 CreditAttribution: shetpooja04 at QED42 commented
Assigned: Unassigned » shetpooja04
shetpooja04’s picture

shetpooja04 CreditAttribution: shetpooja04 at QED42 commented
Assigned: shetpooja04 » Unassigned
Status: Active » Needs review
FileSize
d9whiteblacklist-3151101-6.patch11.37 KB

I have worked on issue as per the issue description

Status: Needs review » Needs work

The last submitted patch, 6: d9whiteblacklist-3151101-6.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented 
  1. +++ b/core/modules/filter/src/Element/TextFormat.php
@@ -84,7 +84,7 @@ public static function processFormat(&$element, FormStateInterface $form_state,
-    $blacklist = [
+    $blocklist = [

@@ -108,7 +108,7 @@ public static function processFormat(&$element, FormStateInterface $form_state,
-      if (!in_array($key, $blacklist)) {
+      if (!in_array($key, $blocklist)) {

    Let's call this $keys_not_to_copy as $blocklist or the original term is not actually that helpful here. 

  2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -304,7 +304,7 @@ public function getHtmlRestrictions() {
-          // Track the union of forbidden (blacklisted) tags.
+          // Track the union of forbidden (blocklisted) tags.

@@ -314,15 +314,15 @@ public function getHtmlRestrictions() {
-          // Track the intersection of allowed (whitelisted) tags.
+          // Track the intersection of allowed (allowlisted) tags.

    We can remove the terms between the brackets. They're not necessary. 

  3. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -314,15 +314,15 @@ public function getHtmlRestrictions() {
-              // If the current tag is not whitelisted by the new filter, then
+              // If the current tag is not allowlisted by the new filter, then

    allowed 

  4. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -314,15 +314,15 @@ public function getHtmlRestrictions() {
-                // does not need to be whitelisted by every filter in order to be
+                // does not need to be allowlisted by every filter in order to be

    allowed 

  5. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -375,10 +375,10 @@ public function getHtmlRestrictions() {
-      // Simplification: if we have both a (intersected) whitelist and a (unioned)
-      // blacklist, then remove any tags from the whitelist that also exist in the
-      // blacklist. Now the whitelist alone expresses all tag-level restrictions,
-      // and we can delete the blacklist.
+      // Simplification: if we have both a (intersected) allowlist and a (unioned)
+      // blocklist, then remove any tags from the allowlist that also exist in the
+      // blocklist. Now the allowlist alone expresses all tag-level restrictions,
+      // and we can delete the blocklist.

          // Simplification: if we have both allowed (intersected) and forbidden
      // (unioned) tags, then remove any allowed tags that are also forbidden.
      // Once complete, the list of allowed tags expresses all tag-level
      // restrictions,  and the list of forbidden tags can be removed.

  6. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -390,7 +390,7 @@ public function getHtmlRestrictions() {
       // contains attribute restrictions that apply to all tags), and only
-      // whitelisting filters were used, then effectively nothing is allowed.
+      // allowlisting filters were used, then effectively nothing is allowed.

    and there are no forbidden tags, the effectively nothing is allowed. 

  7. +++ b/core/modules/filter/src/FilterFormatInterface.php
@@ -73,7 +73,7 @@ public function getFilterTypes();
    * @return array|false
    *   A structured array as returned by FilterInterface::getHTMLRestrictions(),
    *   but with the intersection of all filters in this text format.
-   *   Will either indicate blacklisting of tags or whitelisting of tags. In
+   *   Will either indicate blocklisting of tags or allowlisting of tags. In
    *   the latter case, it's possible that restrictions on attributes are also
    *   stored. FALSE means there are no HTML restrictions.

    Let's go with:

       * @return array|false
   *   A structured array as returned by FilterInterface::getHTMLRestrictions(),
   *   but with the intersection of all filters in this text format. The
   *   restrictions will either forbid or allow a list of tags. In the latter
   *   case, it's possible that restrictions on attributes are also stored.
   *   FALSE means there are no HTML restrictions.

  8. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -113,7 +113,7 @@ public function filterAttributes($text) {
-      // globally whitelisted attributes, it is impossible for a tag to actually
+      // globally allowlisted attributes, it is impossible for a tag to actually

    allowed 

  9. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
-   * Filter attributes on an element by name and value according to a whitelist.
+   * Filter attributes on an element by name and value according to a allowlist.

    I think we can remove the "according to a list". There's a parameter called $allowed_attributes - I think that's enough. 

  10. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
-   *   The attributes whitelist as an array of names and values.
+   *   The attributes allowlist as an array of names and values.

    The list of allowed attributes as an array... 

  11. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
-      // Remove attributes not in the whitelist.
+      // Remove attributes not in the allowlist.
...
-        // Check the attribute values whitelist.
+        // Check the attribute values allowlist.

    the list of allowed attributes. 

  12. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -247,7 +247,7 @@ public function getHTMLRestrictions() {
-    // Parse the allowed HTML setting, and gradually make the whitelist more
...
     // specific.

    the list of allowed tags more specific. 

  13. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -283,7 +283,7 @@ public function getHTMLRestrictions() {
-          // would mean whitelisting all possible attribute values. But in that
+          // would mean allowlisting all possible attribute values. But in that

    allowing 

  14. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -311,14 +311,14 @@ public function getHTMLRestrictions() {
-    // allowed. The value whitelist for the 'dir' attribute is enforced by
+    // allowed. The value allowlist for the 'dir' attribute is enforced by

    The list of allowed values for the 'dir' attribute... 

  15. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -311,14 +311,14 @@ public function getHTMLRestrictions() {
-    // available style, so specific values should be explicitly whitelisted.
+    // available style, so specific values should be explicitly allowlisted.

    allowed. 

  16. +++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
@@ -405,7 +405,7 @@ public function testLineBreakFilter() {
    * @todo Class, id, name and xmlns should be added to disallowed attributes,
-   *   or better a whitelist approach should be used for that too.
+   *   or better a allowlist approach should be used for that too.

    This is a strange @todo - it would be nice to rewrite this with using the allowlist term. 

  17. +++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
@@ -460,11 +460,11 @@ public function testHtmlFilter() {
-    // All attributes of whitelisted tags are stripped by default.
+    // All attributes of allowlisted tags are stripped by default.

    allowed tags 

  18. +++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
@@ -460,11 +460,11 @@ public function testHtmlFilter() {
-    // Now whitelist the "llama" attribute on <a>.
+    // Now allowlist the "llama" attribute on <a>.

    allow 

  19. +++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
@@ -475,7 +475,7 @@ public function testHtmlFilter() {
-    // Restrict the whitelisted "llama" attribute on <a> to only allow the value
+    // Restrict the allowlisted "llama" attribute on <a> to only allow the value

    allowed

Deepak Goyal’s picture

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » Deepak Goyal
Deepak Goyal’s picture

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented
FileSize
3151101-10.patch11.95 KB
interdiff_6-10.txt10.1 KB

Hi @alexpott
Made changes as you suggested please review.
And #8.16 is pending I am not getting this.

Deepak Goyal’s picture

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Deepak Goyal » Unassigned
Status: Needs work » Needs review
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented 
  1. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -314,15 +314,15 @@ public function getHtmlRestrictions() {
-              // If the current tag is not whitelisted by the new filter, then
+              // If the current tag is not allowed by the new filter, then
               // it's outside of the intersection.

    The comment needs re-flowing now. it's will fit on the previous line now. 

  2. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -375,10 +375,10 @@ public function getHtmlRestrictions() {
+      // restrictions,  and the list of forbidden tags can be removed.

    There's a double space in front of and. 

  3. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
-      // Remove attributes not in the whitelist.
+      // Remove attributes not in the allowlist.

    Remove attributes not in the list of allowed attributes. 

  4. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
-        // Check the attribute values whitelist.
+        // Check the attribute values allowlist.

    Check the allowed attribute value list. 

  5. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -283,7 +283,7 @@ public function getHTMLRestrictions() {
+          // would mean allowing all possible attribute values. But in that
           // case, one would not specify an attribute value at all.

    Can re-flow comment. I think "case" now fits on the preceding line. 

  6. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -311,14 +311,14 @@ public function getHTMLRestrictions() {
+    // by self::filterAttributes().  Note that those two attributes are in the

    As we changing this line we can remove the double space after the fullstop. 

  7. +++ b/core/modules/filter/tests/src/Kernel/FilterKernelTest.php
@@ -405,7 +405,7 @@ public function testLineBreakFilter() {
    * @todo Class, id, name and xmlns should be added to disallowed attributes,
-   *   or better a whitelist approach should be used for that too.
+   *   or better a allowlist approach should be used for that too.

    I think 

       * @todo Class, id, name and xmlns should be added to the list of forbidden
   *   attributes, or, better yet, use an allowed attribute list.

    might work here.

Deepak Goyal’s picture

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Unassigned » Deepak Goyal
Deepak Goyal’s picture

Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commented
Assigned: Deepak Goyal » Unassigned
FileSize
3151101-14.patch12.17 KB
interdiff_10-14.txt3.86 KB

Hi @alexpott
Made changes as you suggested please review.
For #12.5 "case" is not fits on the preceding line.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work

Almost there, thanks! Just a few lingering things, then this is RTBC:

  1. +++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -388,9 +388,9 @@ public function getHtmlRestrictions() {
+      // (which contains attribute restrictions that apply to all tags),
+      // and there are no forbidden tags, the effectively nothing is allowed.

    A) 80 char flow: "and" can move up to the previous line.

    B) s/the/then/ effectively... 

  2. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
-   * Filter attributes on an element by name and value according to a whitelist.
+   * Filter attributes on an element by name and value.

    'Filters' needs an 's'. Also, I think it's still worth mentioning the allowed list. Something like:

    "Filters attributes on an element according to an allow list." (or something).

But otherwise, we're done. Before:

 egrep -ir '(white|black)list' core/modules/filter | wc
      25     278    2980

After:

% git apply 3151101-14.patch
% egrep -ir '(white|black)list' core/modules/filter | wc
       0       0       0

(and for good measure):

% egrep -ir '(white|black)' core/modules/filter
core/modules/filter/tests/src/Kernel/FilterKernelTest.php:    $this->assertNormalized($f, 'rel="nofollow"', 'Spam deterrent evasion -- non whitespace character after tag name.');
core/modules/filter/filter.module:      // under certain strange conditions it could create a P of entirely whitespace
ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs work » Needs review
FileSize
3151101-16.patch12.18 KB
interdiff_14-16.txt1.46 KB

Here I have addressed #15.1 and #15.2 points of comment #15.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Excellent, thanks!

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

I was curious to know what words whitelist and blacklist were being replaced with and found that the IS states 'TBD'. So I looked at the patch for answer and that raised questions. 

  1. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
+   * Filters attributes on an element according to an allow list.

    Surely 'allow' is a verb and not an adjective. Instead of 'allow list' maybe 'listed of allowed values'? 

  2. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -149,23 +149,23 @@ public function filterAttributes($text) {
+        // Check the allowed attribute value list.

    Shouldn't this be 'Check the list of allowed of attribute values'?

For the IS the proposed resolution needs to be updated with the suggested replacements for whitelist/blacklist. The remaining tasks section shows that there are 5 tasks still to do. Is that one tasks queries the need for BC but there is nothing in the issue that addresses that point.

Sorry, back to NW.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Thanks, @quietone! Right you are. I even bothered to make a good list of remaining tasks, then didn't check we got them all. Updated.

core/modules/filter/filter.module was a false positive, so I removed it. Everything else is indeed gone (per greps at the end of #15).

Re: specific review points in #18:

  1. "a listed of allowed values"
  2. "Check the list of allowed of attribute values."

otherwise, +1 to those improvements.

Thanks again,
-Derek

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
3151101-20.patch12.17 KB
3151101.16_20.interdiff.txt903 bytes

Now it just needs RTBC. ;)

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs review » Reviewed & tested by the community

@derek, thanks for the changes.

I reviewed the interdiff and the suggested changes were agreed to and made. The IS has been updated. Yeah, I don't have to read the whole patch to learn the solution!

Back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: Replace use of whitelist/blacklist in Filter module » [backport] Replace use of whitelist/blacklist in Filter module
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 1d44c8e and pushed to 9.1.x. Thanks!

Will backport once the release is out and the freeze is over.

  • alexpott committed 1d44c8e on 9.1.x 
    Issue #3151101 by Deepak Goyal, dww, ravi.shankar, shetpooja04, alexpott...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Fixed » Reviewed & tested by the community

Oops meant to leave this at RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed aca70f2cd2 to 9.0.x and 7fafcd6952 to 8.9.x. Thanks!

Backported to 8.9.x as this is docs and internal variable names only.

  • alexpott committed aca70f2 on 9.0.x 
    Issue #3151101 by Deepak Goyal, dww, ravi.shankar, shetpooja04, alexpott...

  • alexpott committed 7fafcd6 on 8.9.x 
    Issue #3151101 by Deepak Goyal, dww, ravi.shankar, shetpooja04, alexpott...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: [backport] Replace use of whitelist/blacklist in Filter module » Replace use of whitelist/blacklist in Filter module

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.