Deprecate SafeMarkup::checkPlain() for Drupal 8.0.x

The biggest advantage to removing it is that it will force people to use the new methods. It it remains, I forsee a lot of SA-CONTRIB going out because contrib maintainers will just convert check_plain() usages into SafeMarkup::checkPlain(). Given all of the XSS SA that have gone out this year, I think this is a legitimate concern. I also think that we have enough examples to pull from from the current purge to fill out https://www.drupal.org/node/28984 nicely.

#3: Nicely put :)

Discussed with @effulgentsia, @alexpott, @catch, and @webchick. We agreed to deprecate checkPlain() and that it is a critical part of minimizing SafeMarkup use. We will initially deprecate it for removal before 8.0.0.

Removing checkPlain() entirely or removing its core usages is not critical, but we can continue that work up to RC.

Just to clarify what I think the plan is:

Deprecate for 8.0.0 now, without removing all usages yet, to send a clear message via change notices etc. that contrib should remove their own usages asap.

If we're able to remove all core usages before a release candidate, try to remove it.

If we're not able to remove all core usages before a release candidate, deprecate for 9.x instead and/or mark @internal and possibly try to make it so neither do anything at all with the safe list.

Tagging this as beta target since it's a deliberate API change that we want to expose to contrib as soon as possible.

Could we do something like this, on both methods:

@deprecated Will be removed before Drupal 8.0.0. If possible, rely on Twig's auto-escaping feature, or use the '#plain_text' key when constructing a render array that contains plain text in order to use the renderer's auto-escaping feature. If neither of these are possible, Html::escape() can be used in places where explicit escaping is needed.

...whenever (and if) #2555931: Add #plain_text to escape text in render arrays goes in?

A novice critical? A novice can create the patch.

Something like this?

Just committed #2555931: Add #plain_text to escape text in render arrays which was soft-blocking this.

I think this wording is a little more direct. To me, "possibly" used twice sounds like an invitation to do whatever I want.

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -169,6 +169,13 @@ public static function getAll() {
+   *   auto-escaping feature, or, use the '#plain_text' key when constructing a

What about pointing to the corresponding docs (https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...) ? I think you can use @link for that, not sure about the syntax though

Adding in that link and removing a superfluous comma

+++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php
@@ -169,6 +169,13 @@ public static function getAll() {
+   * @deprecated Will be removed before Drupal 8.0.0. Rely on Twig's
+   *   auto-escaping feature, or use the @link theme_render #plain_text @endlink

IMO this comment should not be written only for the Twig because we still have to support PHP templating engine. At least we shouldn't make assumptions that everyone is using Twig.

So it doesn't actually leave out users of other templating engines but instead the structure was a little hard to process for me. Anyway I think the structure is good as it is because now it very explicitly tells in the first sentence that if you use Twig, there shouldn't be need to care about escaping manually.

#20 - The confusing sentence structure was the intention of the "superfluous" comma.

I agree that , or, was confusing to read, at least in english.

Does this issue needs its own dedicated change record, or should we prepare one big one for all the issues?

Updated the CR at https://www.drupal.org/node/2549395 with a bullet point about checkPlain

Committed/pushed to 8.0.x, thanks!

In #2545972-95: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping I listed out a bunch of CRs that also needed to be updated for this change (others are/were mentioned in #2545972-84: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping). It might be worth reopening this for that, because it's not just a simple replacement; we want to dissuade people from escaping inappropriately.

Also, I think that the section on checkPlain() in https://www.drupal.org/node/2549395 needs more work to better clarify when checkPlain() should be replaced and with what. For example, it says "SafeMarkup::checkPlain() - If the string will end up in a Twig template, rely on Twig's auto-escaping feature." This is pretty vague and confusing -- one way or another, most things will "end up" in Twig templates. If modules remove their escaping incorrectly, it will still open XSS vulnerabilities. It might be better to say that Html::escape() is the most direct replacement for SafeMarkup::checkPlain() so people who don't pay attention to the details start there, and then to go into more detail about other strategies, with more examples. We should also emphasize that the developer should test carefully when converting code to rely on Twig's autoescape.

Also, I think it's important to emphasize that non-HTML output must still be sanitized, either by using Html::escape() when (e.g.) constructing the response, or by escaping in (e.g.) JavaScript à la #2503963: XSS in Quick Edit: entity title is not safely encoded.

Reopening for that as well as #28. Thanks everyone!

Draft for the related g.d.o/core announcement: https://docs.google.com/document/d/1OM33gYwcIXUB-QP2uiF-M-H54QyZdGyJHTj4...

Sorry, forgot to update the title. Also, the doc in #30 now has public comment access, but the post should go out in a bit anyway. :)

Alright, @alexpott, @effulgentsia, and I reworked the main change record for the method removals in SafeMarkup methods are removed. There are still a couple outstanding things that I think should be improved (copying and expanding notes from our google docs draft):

1. If the string is used in a Twig template, rely on Twig's auto-escaping feature. Simply remove the call to SafeMarkup::checkPlain(). For example, most SafeMarkup::checkPlain() calls in template preprocess functions can be removed. Another common case is the construction of a render array that uses a Twig template. For example, data that is being themed using an item list or table does not need to be escaped as Twig will do this for you. If text that has already been escaped using Html::escape() ends up in a Twig template variable, it will be double-escaped.

   This is clearer now, but as a reader of this I still would not entirely understand "is used in a template". First of all, there are ways to use strings unsafely in a template. Do we have a Twig resource we can reference about how output ends up in templates and how it is used safely? Secondly, there are many cases where a developer is creating output that will be used in an existing template without the developer knowing anything about it. (Maybe we need a chart somewhere indicating which strings will be escaped, which strings will be filtered, and which will not be sanitized. That doesn't need to block this though.)

2. For example, most SafeMarkup::checkPlain() calls in template preprocess functions can be removed

   We should clarify when they can't be removed. For example, if the string is XSS-filtered, this will be more permissive than escaping and bypass it because the resulting string will be safe.

3. Another common case is the construction of a render array that uses a Twig template. For example, data that is being themed using an item list or table does not need to be escaped as Twig will do this for you.

   This example is helpful. A code snippet might make it more so. However, some render array elements, like #prefix, #suffix, #value, and #markup are not escaped (only admin filtered) so this sentence is somewhat too vague.

I updated some of the references to check_plain or checkPlain yesterday, and it looks like someone worked on them today as well (thanks!). Some remain; adding links to search to the summary as well as the remaining task for the main CR.

SafeMarkup::checkPlain() is dangerous ... decide if we want to remove it.

Here, I edited the summary for you to help with the decision.

#37 looks good to me. Thanks @alexpott.

#32 has not been addressed entirely, but I don't think that's critical at this point, so moving that to the scope of #2494297: [no patch] Consolidate change records relating to safe markup and filtering/escaping to ensure cross references exist.

It's looking like #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping might land before RC, so shouldn't this issue be demoted from critical and postponed on that one, in-case removal rather than deprecation is possible?

We need to deprecate before we remove, and ideally give as much notice as possible so people can update.

Also it was committed already.