From 4a8366bb623ad65fbda061678266b03569f9a3b9 Mon Sep 17 00:00:00 2001
From: Alex Pott <alex.a.pott@googlemail.com>
Date: Tue, 29 Sep 2015 11:43:27 +0100
Subject: [PATCH] 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 4d8e0102f389a6c7c01a79f10b78d7f417a1a1f6.
---
core/includes/bootstrap.inc | 30 ++--
.../Component/Utility/FormattableString.php | 36 ++---
.../Component/Utility/PlaceholderTrait.php | 132 +++++-------------
.../StringTranslation/TranslatableString.php | 1 -
4 files changed, 62 insertions(+), 137 deletions(-)
diff --git a/core/includes/bootstrap.inc b/core/includes/bootstrap.inc
index 7e7180f7277f..0b1f6337b019 100644
--- a/core/includes/bootstrap.inc
+++ b/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()) {
diff --git a/core/lib/Drupal/Component/Utility/FormattableString.php b/core/lib/Drupal/Component/Utility/FormattableString.php
index 4ad604a18713..ea7861c325b6 100644
--- a/core/lib/Drupal/Component/Utility/FormattableString.php
+++ b/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 {
diff --git a/core/lib/Drupal/Component/Utility/PlaceholderTrait.php b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
index 671f5d8e6e33..e2db26b3ecb1 100644
--- a/core/lib/Drupal/Component/Utility/PlaceholderTrait.php
+++ b/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;
}
diff --git a/core/lib/Drupal/Core/StringTranslation/TranslatableString.php b/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
index 6d587a1902d9..0dbb3a6a3f7e 100644
--- a/core/lib/Drupal/Core/StringTranslation/TranslatableString.php
+++ b/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
--
GitLab