Revert "Issue #2570431 by stefan.r, YesCT, David_Rothstein, xjm, catch,...

Revert "Issue #2570431 by stefan.r, YesCT, David_Rothstein, xjm, catch, effulgentsia, Heine, dawehner, plach, lauriii, alexpott, joelpittet: Document that certain (non-"href") attribute values in t() and SafeMarkup::format() are not supported and may be insecure"

This reverts commit 4d8e0102.

core/includes/bootstrap.inc

@@ -304,23 +304,25 @@ function drupal_get_path($type, $name) {
  * break up strings for translation.
  *
  * @section sec_translating_vars Translating Variables
- * You should never use t() to translate variables, such as calling t($text).
- * Doing that can lead to cross-site scripting vulnerabilities and other
- * security problems.
- *
- * However, you can use placeholder replacement in your string, to put variable
- * text such as user names or link URLs into translated text. Variable
- * substitution looks like this:
+ * You should never use t() to translate variables, such as calling t($text)
+ * unless the text that the variable holds has been passed through t()
+ * elsewhere (e.g., $text is one of several translated literal strings in an
+ * array). It is especially important never to call t($user_text) where
+ * $user_text is some text that a user entered - doing that can lead to
+ * cross-site scripting and other security problems. However, you can use
+ * variable substitution in your string, to put variable text such as user
+ * names or link URLs into translated text. Variable substitution looks like
+ * this:
  * @code
  * $text = t("@name's blog", array('@name' => $account->getDisplayName()));
  * @endcode
  * Basically, you can put variables like @name into your string, and t() will
  * substitute their sanitized values at translation time. (See the
  * Localization API pages referenced above and the documentation of
- * \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat()
- * for details about how to safely and correctly define variables in your
- * string.). Translators can then rearrange the string as necessary for the
- * language (e.g., in Spanish, it might be "blog de @name").
+ * \Drupal\Component\Utility\SafeMarkup::format() for details about how to
+ * define variables in your string.). Translators can then rearrange the string
+ * as necessary for the language (e.g., in Spanish, it might be "blog de
+ * @name").
  *
  * @param $string
  *   A string containing the English string to translate.
@@ -335,10 +337,10 @@ function drupal_get_path($type, $name) {
  *   - 'context' (defaults to the empty context): The context the source string
  *     belongs to.
  *
- * @return \Drupal\Core\StringTranslation\TranslatableString
- *   An object that, when cast to a string, will yield the translated string.
+ * @return
+ *   The translated string.
  *
- * @see \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat()
+ * @see \Drupal\Component\Utility\SafeMarkup::format()
  * @ingroup sanitization
  */
 function t($string, array $args = array(), array $options = array()) {

core/lib/Drupal/Component/Utility/FormattableString.php

@@ -10,36 +10,28 @@
 /**
  * Formats a string for HTML display by replacing variable placeholders.
  *
- * When cast to a string, this object replaces variable placeholders in the
- * string with the arguments passed in during construction and escapes the
- * values so they can be safely displayed as HTML. See the documentation of
- * \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat() for
- * details on the supported placeholders and how to use them securely. Incorrect
- * use of this class can result in security vulnerabilities.
+ * When cast to a string it replaces variable placeholders in the string with
+ * the arguments passed in during construction and escapes the values so they
+ * can be safely displayed as HTML. It should be used on any unknown text that
+ * is intended to be printed to an HTML page (especially text that may have come
+ * from untrusted users, since in that case it prevents cross-site scripting and
+ * other security problems).
+ *
+ * This class is not intended for passing arbitrary user input into any HTML
+ * attribute value, as only URL attributes such as "src" and "href" are
+ * supported (using ":variable"). Never use this method on unsafe HTML
+ * attributes such as "on*" and "style" and take care when using this with
+ * unsupported attributes such as "title" or "alt" as this can lead to
+ * unexpected output.
  *
  * In most cases, you should use TranslatableString or PluralTranslatableString
  * rather than this object, since they will translate the text (on
- * non-English-only sites) in addition to formatting it. Variables concatenated
- * without the insertion of language-specific words or punctuation are some
- * examples where translation is not applicable and using this class directly
- * directly is appropriate.
- *
- * This class is designed for formatting messages that are mostly text, not as
- * an HTML template language. As such, $string passed to the constructor should
- * contain minimal HTML.
- *
- * The result of casting this object to a string should not be used within the
- * "<" and ">" of an HTML tag, such as in HTML attribute values. This would be a
- * security risk.
- *
- * To build HTML that cannot meet these restrictions, use an HTML template
- * language such as Twig, rather than this class.
+ * non-English-only sites) in addition to formatting it.
  *
  * @ingroup sanitization
  *
  * @see \Drupal\Core\StringTranslation\TranslatableString
  * @see \Drupal\Core\StringTranslation\PluralTranslatableString
- * @see \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat()
  */
 class FormattableString implements SafeStringInterface {
 

core/lib/Drupal/Component/Utility/PlaceholderTrait.php

@@ -15,100 +15,44 @@ trait PlaceholderTrait {
   /**
    * Formats a string by replacing variable placeholders.
    *
-   * This method is designed for formatting messages that are mostly text, not
-   * as an HTML template language. As such, $string should contain minimal HTML.
-   *
-   * In $string the placeholders should not be within the "<" and ">" of an HTML
-   * tag, except a placeholder starting with : as described below. This would be
-   * a security risk.
-   *
-   * The return value should not be used within the "<" and ">" of an HTML tag,
-   * such as in HTML attribute values. Also, do not concatenate the return
-   * value into JavaScript or CSS. This would be a security risk.
-   *
-   * To build HTML that cannot meet these restrictions, use an HTML template
-   * language such as Twig, rather than this trait.
+   * This trait is not intended for passing arbitrary user input into any HTML
+   * attribute value, as only URL attributes such as "src" and "href" are
+   * supported (using ":variable"). Never use this method on unsafe HTML
+   * attributes such as "on*" and "style" and take care when using this with
+   * unsupported attributes such as "title" or "alt" as this can lead to
+   * unexpected and unsafe output.
    *
    * @param string $string
-   *   A string containing placeholders. The string itself is expected to be
-   *   safe and correct HTML. Any unsafe content must be in $args and
-   *   inserted via placeholders.
+   *   A string containing placeholders.
    * @param array $args
-   *   An associative array of replacements. Each array key should be the same
-   *   as a placeholder in $string. The corresponding value should be a string
-   *   or an object that implements
-   *   \Drupal\Component\Utility\SafeStringInterface. The value replaces the
-   *   placeholder in $string. Sanitization and formatting will be done before
-   *   replacement. The type of sanitization and formatting depends on the first
-   *   character of the key:
-   *   - @variable: Use as the default choice for anything displayed on the
-   *     site. Do not use within the "<" and ">" of an HTML tag, such as in HTML
-   *     attribute values. Doing so is a security risk.
-   *     When the placeholder replacement value is:
-   *     - A string, it will be sanitized using
-   *       \Drupal\Component\Utility\Html::escape().
-   *     - A SafeStringInterface object, the object will be cast to a string and
-   *       not sanitized. To force sanitization of a SafeStringInterface object,
-   *       cast the replacement value to a string, since it is not known what
-   *       strategy a SafeStringInterface object uses for santitization.
-   *     A call like:
-   *     @code
-   *       placeholderFormat("Prefix @something", ['@something' => 'text output.']);
-   *     @endcode
-   *     returns the following HTML string:
+   *   An associative array of replacements to make. Occurrences in $string of
+   *   any key in $args are replaced with the corresponding value, after
+   *   optional sanitization and formatting. The type of sanitization and
+   *   formatting depends on the first character of the key:
+   *   - @variable: Escaped to HTML using Html::escape() unless the value is
+   *     already HTML-safe. Use this as the default choice for anything
+   *     displayed on a page on the site, but not within HTML attributes.
+   *   - %variable: Escaped to HTML just like @variable, but also wrapped in
+   *     <em> tags, which makes the following HTML code:
    *     @code
-   *       Prefix text output.
+   *       <em class="placeholder">text output here.</em>
    *     @endcode
-   *   - %variable: Use when @variable would be appropriate, but the placeholder
-   *     value will also be wrapped in <em> tags with a class. As with
-   *     @variable, do not use within the "<" and ">" of an HTML tag, such as in
-   *     HTML attribute values. Doing so is a security risk.
-   *     A call like:
-   *     @code
-   *       placeholderFormat("Prefix %something", ['%something' => 'text output.']);
-   *     @endcode
-   *     returns the following HTML string:
-   *     @code
-   *       Prefix <em class="placeholder">text output.</em>
-   *     @endcode
-   *   - :variable: Use when the return value is to be used as a URL value of an
-   *     HTML attribute, for example the href attribute in an a tag.
-   *     Return value is escaped with
-   *     \Drupal\Component\Utility\Html::escape() and filtered for dangerous
-   *     protocols using UrlHelper::stripDangerousProtocols(). Use this when
-   *     using the "href" attribute, ensuring the value is always wrapped in
-   *     quotes.
+   *     As with @variable, do not use this within HTML attributes.
+   *   - :variable: Escaped to HTML using Html::escape() and filtered for
+   *     dangerous protocols using UrlHelper::stripDangerousProtocols(). Use
+   *     this when passing in a URL, such as when using the "src" or "href"
+   *     attributes, ensuring the value is always wrapped in quotes:
+   *     - Secure: <a href=":variable">@variable</a>
+   *     - Insecure: <a href=:variable>@variable</a>
    *     When ":variable" comes from arbitrary user input, the result is secure,
    *     but not guaranteed to be a valid URL (which means the resulting output
    *     could fail HTML validation). To guarantee a valid URL, use
    *     Url::fromUri($user_input)->toString() (which either throws an exception
    *     or returns a well-formed URL) before passing the result into a
    *     ":variable" placeholder.
-   *     @todo Add some advice and stronger warnings.
-   *       https://www.drupal.org/node/2569041
-   *
-   *   In $string, only HTML attributes that take a URL as the value, are
-   *   supported via the special ":variable" placeholder. For example, with the
-   *   "href" attribute, the ":variable" placeholder allows simple links to be
-   *   inserted:
-   *     - Secure: @code <a href=":variable">link text</a> @endcode
-   *     - Secure: @code <a href=":variable" title="static text">link text</a> @endcode
-   *     - Secure: @code <a href=":variable">@variable</a> @endcode
-   *   Insecure $string examples:
-   *     - It is insecure in $string to have @ or % placeholders within the "<"
-   *       and ">" of an HTML tag:
-   *       - Insecure: @code <@variable>text</@variable> @endcode
-   *       - Insecure: @code <a @variable>link text</a> @endcode
-   *       - Insecure: @code <a href="@variable">link text</a> @endcode
-   *       - Insecure: @code <a title="@variable">link text</a> @endcode
-   *       - Insecure: @code <a href=":variable" title="@variable">link text</a>@endcode
-   *     - The : placeholder should be used for HTML attribute values, but is
-   *       insecure not to have quotes around the attribute value:
-   *       - Insecure: @code <a href=:variable>@variable</a>@endcode
-   *       - Insecure: @code <img src=:variable />@endcode
    *
    * @return string
-   *   A formatted string with the placeholders replaced.
+   *   The string with the placeholders replaced.
    *
    * @ingroup sanitization
    *
@@ -118,39 +62,27 @@ trait PlaceholderTrait {
    * @see \Drupal\Component\Utility\Html::escape()
    * @see \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols()
    * @see \Drupal\Core\Url::fromUri()
-   * @see http://stackoverflow.com/questions/2725156/complete-list-of-html-tag-attributes-which-have-a-url-value/2725168#2725168
    */
   protected static function placeholderFormat($string, array $args) {
     // Transform arguments before inserting them.
     foreach ($args as $key => $value) {
       switch ($key[0]) {
         case '@':
-          // Escape if the value is not an object from a class that implements
-          // \Drupal\Component\Utility\SafeStringInterface, for example strings
-          // will be escaped.
-          // \Drupal\Component\Utility\SafeMarkup\SafeMarkup::isSafe() may
-          // return TRUE for content that is safe within HTML fragments, but not
-          // within other contexts, so this placeholder type must not be used
-          // within HTML attributes, JavaScript, or CSS.
+          // Escaped only.
           $args[$key] = static::placeholderEscape($value);
           break;
 
         case ':':
-          // Strip URL protocols that can be XSS vectors.
-          $value = UrlHelper::stripDangerousProtocols($value);
-          // Escape unconditionally, without checking
-          // \Drupal\Component\Utility\SafeMarkup\SafeMarkup::isSafe(). This
-          // forces characters that are unsafe for use in an "href" HTML
-          // attribute to be encoded.
-          $args[$key] = Html::escape($value);
+          // URL attributes must be escaped unconditionally (even if they were
+          // already marked safe) since content that has been filtered for XSS
+          // can still contain characters that are unsafe for use in attributes.
+          // @todo decide what to do about non-URL attribute values (#2570431)
+          $args[$key] = Html::escape(UrlHelper::stripDangerousProtocols($value));
           break;
 
         case '%':
         default:
-          // Acts like @, but adds wrapping markup: an <em> tag with a class.
-          // Not for use within attributes, per the warning above regarding @
-          // about \Drupal\Component\Utility\SafeMarkup\SafeMarkup::isSafe() and
-          // also due to the wrapping markup.
+          // Escaped and placeholder.
           $args[$key] = '<em class="placeholder">' . static::placeholderEscape($value) . '</em>';
           break;
       }

core/lib/Drupal/Core/StringTranslation/TranslatableString.php

@@ -19,7 +19,6 @@
  * This is useful for using translation in very low level subsystems like entity
  * definition and stream wrappers.
  *
- * @see \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat()
  * @see \Drupal\Core\StringTranslation\TranslationManager::translate()
  * @see \Drupal\Core\StringTranslation\TranslationManager::translateString()
  * @see \Drupal\Core\Annotation\Translation