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/includes/bootstrap.inc

@@ -304,25 +304,23 @@ 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)
- * 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:
+ * 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:
  * @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\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").
+ * \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").
  *
  * @param $string
  *   A string containing the English string to translate.
@@ -337,10 +335,10 @@ function drupal_get_path($type, $name) {
  *   - 'context' (defaults to the empty context): The context the source string
  *     belongs to.
  *
- * @return
- *   The translated string.
+ * @return \Drupal\Core\StringTranslation\TranslatableString
+ *   An object that, when cast to a string, will yield the translated string.
  *
- * @see \Drupal\Component\Utility\SafeMarkup::format()
+ * @see \Drupal\Component\Utility\PlaceholderTrait::placeholderFormat()
  * @ingroup sanitization
  */
 function t($string, array $args = array(), array $options = array()) {

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

@@ -10,28 +10,36 @@
 /**
  * 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, $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.
  *
  * @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,44 +15,100 @@ 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 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.
    *
    * @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: 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:
    *     @code
-   *       <em class="placeholder">text output here.</em>
+   *       Prefix text output.
    *     @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>
+   *   - %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.
    *     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
-   *   The string with the placeholders replaced.
+   *   A formatted string with the placeholders replaced.
    *
    * @ingroup sanitization
    *
@@ -62,27 +118,39 @@ 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 '@':
-          // 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.
           $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.
+          $args[$key] = Html::escape($value);
           break;
 
         case '%':
         default:
-          // Escaped and placeholder.
+          // 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.
           $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