diff --git a/core/lib/Drupal/Component/Utility/PlaceholderTrait.php b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php index 5922b3f..cc8d28f 100644 --- a/core/lib/Drupal/Component/Utility/PlaceholderTrait.php +++ b/core/lib/Drupal/Component/Utility/PlaceholderTrait.php @@ -33,7 +33,11 @@ protected static function placeholderFormat($string, array $args, &$safe = TRUE) foreach ($args as $key => $value) { switch ($key[0]) { case '@': - // Escaped only. + // Escape if not already safe. 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(). if (!SafeMarkup::isSafe($value)) { $args[$key] = Html::escape($value); } @@ -41,7 +45,9 @@ protected static function placeholderFormat($string, array $args, &$safe = TRUE) case '%': default: - // Escaped and placeholder. + // Escape if not already safe, and add wrapping markup to render as a + // placeholder. Not for use within attributes, per the warning above + // about SafeMarkup::isSafe() and also due to the wrapping markup. if (!SafeMarkup::isSafe($value)) { $value = Html::escape($value); } @@ -49,11 +55,15 @@ protected static function placeholderFormat($string, array $args, &$safe = TRUE) 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 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 Html::decodeEntities() on it prior to passing it in as a + // placeholder value of this type. + $args[$key] = Html::escape($value); break; case '!': diff --git a/core/lib/Drupal/Component/Utility/SafeMarkup.php b/core/lib/Drupal/Component/Utility/SafeMarkup.php index c1c86f5..fee70c3 100644 --- a/core/lib/Drupal/Component/Utility/SafeMarkup.php +++ b/core/lib/Drupal/Component/Utility/SafeMarkup.php @@ -168,38 +168,53 @@ public static function checkPlain($text) { * 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 method 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 t() rather than calling this function * directly, since it will translate the text (on non-English-only sites) in - * addition to formatting it. + * addition to formatting it. Exception messages, messages for test + * assertions, and variables concatenated without the insertion of + * language-specific words or punctuation are some examples where translation + * is not applicable and calling this function directly is appropriate. + * + * This function is designed for formatting messages that are mostly text, + * not as an HTML template language. As such: + * - The passed-in string should contain minimal HTML. + * - Variable placeholders should not be used within HTML attributes; they + * are not safe for this purpose, and doing so represents a security risk. + * Only the "href" attribute is supported via the special ":variable" + * placeholder, to allow simple links to be inserted: + * - Secure: link text + * - Insecure: link text + * For non-trivial HTML building that cannot meet the above restrictions, use + * an HTML template language, such as Twig, rather than this function. * * @param string $string - * A string containing placeholders. The string itself is not escaped, any - * unsafe content must be in $args and inserted via placeholders. + * A string containing placeholders. The string itself is treated as + * correctly formatted 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 + * 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 + * - @variable: Displayed as HTML, with HTML sanitization performed by the + * caller before the string is passed in. If an unsanitized string (that + * has never been marked as HTML-safe) is passed in, it will be + * automatically sanitized using Html::escape(). Use this placeholder as + * the default choice for anything displayed on the site, subject to the + * following restrictions: + * - Never use this within HTML attributes, JavaScript, or CSS. Doing so + * is a security risk. + * - %variable: Displayed as HTML just like @variable, but also wrapped in * tags, which makes the following HTML code: * @code * text output here. * @endcode - * As with @variable, do not use this within HTML attributes. + * As with @variable, do not use this within HTML attributes, JavaScript + * or CSS. Doing so is a security risk. * - :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: + * this when using the "href" attribute, ensuring the value is always + * wrapped in quotes: * - Secure: @variable * - Insecure: @variable * When ":variable" comes from arbitrary user input, the result is secure, @@ -217,8 +232,8 @@ public static function checkPlain($text) { * - Some other special reason for suppressing sanitization. * * @return string - * The formatted string, which is marked as safe unless sanitization of an - * unsafe argument was suppressed (see above). + * A formatted HTML string, which is marked as safe unless sanitization of + * an unsafe argument was suppressed (see above). * * @ingroup sanitization * diff --git a/core/lib/Drupal/Core/Template/Attribute.php b/core/lib/Drupal/Core/Template/Attribute.php index 3629cda..7bfceab 100644 --- a/core/lib/Drupal/Core/Template/Attribute.php +++ b/core/lib/Drupal/Core/Template/Attribute.php @@ -41,7 +41,7 @@ * * The attribute keys and values are automatically escaped for output with * Html::escape(). No protocol filtering is applied, so when using user-entered - * input as a value for an attribute that expects an URI (href, src, ...), + * input as a value for an attribute that expects a URI (href, src, ...), * UrlHelper::stripDangerousProtocols() should be used to ensure dangerous * protocols (such as 'javascript:') are removed. For example: * @code