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

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

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

@@ -10,28 +10,34 @@
 /**
  * Formats a string for HTML display by replacing variable placeholders.
  *
- * 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.
+ * 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.
  *
  * 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.
+ * 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:
+ * - The passed in string should contain no (or minimal) HTML.
+ * - Variable placeholders 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 non-minimal HTML, use an HTML template language such as Twig,
+ * rather than this class.
  *
  * @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,35 +15,87 @@ trait PlaceholderTrait {
   /**
    * Formats a string by replacing variable placeholders.
    *
-   * 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.
+   * This trait is designed for formatting messages that are mostly text, not as
+   * an HTML template language. As such:
+   * - The passed in string should contain no (or minimal) HTML.
+   * - Variable placeholders should not be used within the "<" and ">" of an
+   *   HTML tag, such as in HTML attribute values. This would be a security
+   *   risk. Examples:
+   *   @code
+   *     // Insecure (placeholder within "<" and ">"):
+   *     $this->placeholderFormat('<@variable>text</@variable>', ['@variable' => $variable]);
+   *     // Insecure (placeholder within "<" and ">"):
+   *     $this->placeholderFormat('<a @variable>link text</a>', ['@variable' => $variable]);
+   *     // Insecure (placeholder within "<" and ">"):
+   *     $this->placeholderFormat('<a title="@variable">link text</a>', ['@variable' => $variable]);
+   *   @endcode
+   *   Only the "href" attribute is supported via the special ":variable"
+   *   placeholder, to allow simple links to be inserted:
+   *   @code
+   *     // Secure (usage of ":variable" placeholder for href attribute):
+   *     $this->placeholderFormat('<a href=":variable">link text</a>', [':variable' , $variable]);
+   *     // Secure (usage of ":variable" placeholder for href attribute):
+   *     $this->placeholderFormat('<a href=":variable" title="static text">link text</a>', [':variable' => $variable]);
+   *     // Insecure (the "@variable" placeholder does not filter dangerous
+   *     // protocols):
+   *     $this->placeholderFormat('<a href="@variable">link text</a>', ['@variable' => $variable]);
+   *     // Insecure ("@variable" placeholder within "<" and ">"):
+   *     $this->placeholderFormat('<a href=":url" title="@variable">link text</a>', [':url' => $url, '@variable' => $variable]);
+   *   @endcode
+   * To build non-minimal HTML, use an HTML template language such as Twig,
+   * rather than this trait.
    *
    * @param string $string
-   *   A string containing placeholders.
+   *   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.
    * @param array $args
-   *   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:
+   *   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: When the placeholder replacement value is:
+   *     - A string, the replaced value in the returned string will be sanitized
+   *       using \Drupal\Component\Utility\Html::escape().
+   *     - A SafeStringInterface object, the replaced value in the returned
+   *       string will not be sanitized.
+   *     - A SafeStringInterface object cast to a string, the replaced value in
+   *       the returned string be forcibly sanitized using
+   *       \Drupal\Component\Utility\Html::escape().
+   *       @code
+   *         $this->placeholderFormat('This will force HTML-escaping of the replacement value: @text', ['@text' => (string) $safe_string_interface_object));
+   *       @endcode
+   *     Use this placeholder as the default choice for anything displayed on
+   *     the site, but not within HTML attributes, JavaScript, or CSS. Doing so
+   *     is a security risk.
+   *   - %variable: Use when the replacement value is to be wrapped in <em>
+   *     tags.
+   *     A call like:
+   *     @code
+   *       $string = "%output_text";
+   *       $arguments = ['output_text' => 'text output here.'];
+   *       $this->placeholderFormat($string, $arguments);
+   *     @endcode
+   *     makes the following HTML code:
    *     @code
    *       <em class="placeholder">text output here.</em>
    *     @endcode
-   *     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>
+   *     As with @variable, do not use this within HTML attributes, JavaScript,
+   *     or CSS. Doing so is a security risk.
+   *   - :variable: 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 attribute value is always
+   *     wrapped in quotes:
+   *     @code
+   *     // Secure (with quotes):
+   *     $this->placeholderFormat('<a href=":url">@variable</a>', [':url' => $url, @variable => $variable]);
+   *     // Insecure (without quotes):
+   *     $this->placeholderFormat('<a href=:url>@variable</a>', [':url' => $url, @variable => $variable]);
+   *     @endcode
    *     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
@@ -52,7 +104,7 @@ trait PlaceholderTrait {
    *     ":variable" placeholder.
    *
    * @return string
-   *   The string with the placeholders replaced.
+   *   A formatted HTML string with the placeholders replaced.
    *
    * @ingroup sanitization
    *
@@ -68,21 +120,40 @@ protected static function placeholderFormat($string, array $args) {
     foreach ($args as $key => $value) {
       switch ($key[0]) {
         case '@':
-          // Escaped only.
+          // 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. See
+          // \Drupal\Component\Utility\SafeMarkup::format().
           $args[$key] = static::placeholderEscape($value);
           break;
 
         case ':':
-          // 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));
+          // 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. If a caller wants to pass a value that is
+          // extracted from HTML and therefore is already HTML encoded, it must
+          // invoke
+          // \Drupal\Component\Utility\OutputStrategyInterface::renderFromHtml()
+          // on it prior to passing it in as a placeholder value of this type.
+          // @todo Add some advice and stronger warnings.
+          //   https://www.drupal.org/node/2569041.
+          $args[$key] = Html::escape($value);
           break;
 
         case '%':
         default:
-          // Escaped and placeholder.
+          // Similarly to @, escape non-safe values. Also, add wrapping markup
+          // in order to render as a placeholder. Not for use within attributes,
+          // per the warning above about
+          // \Drupal\Component\Utility\SafeMarkup\SafeMarkup::isSafe() and also
+          // due to the wrapping markup.
           $args[$key] = '<em class="placeholder">' . static::placeholderEscape($value) . '</em>';
           break;
       }

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

@@ -19,6 +19,7 @@
  * 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