Contrib.social

t($user_input) needs hardening

Open on Drupal.org →
Created on 4 December 2016, about 8 years ago
Updated 16 March 2023, almost 2 years ago

Problem/Motivation

https://www.drupal.org/node/2584837 →

Create a field with <script> tags in it, make it multi-value and boom, you get JS executed

Thoughts:

  • Create fields is a permission with restrict access, I guess you can't change a field label without that, so this specific example might be OK
  • It was possible as well in 7.x to have XSS or so through t(), which is why translate interface is also a restricted permission. But the fact that t() now returns an object that is defined as safe markup that does not require escaping ever makes this IMHO so much worse. The above wouldn't have been possible in 7.x because input to l() was always escaped unless you explicitly opted out. But now we pass in a string that says, Yeah, I'm safe, don't escape me.
  • However, It's very easy to make a mistake like this, we've been saying for many years to not translate user input, but I've never seen us mention the security aspect of it? And we definitely never treated t($user_input) as a security problem? I know modules that had such code and public issues for years.
  • We went through all this work to prevent XSS by default and make it close to impossible to accidentally have user input displayed unescaped, so considering that, this seems like a pretty huge hole in that wall?
  • Is there a valid use case for allowing full html in a translatable string? should we maybe xss filter it? I don't think there's any valid use case. Note that locale_string_is_safe() prevents such a use case in the translated string, so I think it would be fine for us to prevent it in the source string too.
  • In Drupal 8, the point here is that we seem to be recommending to people "don't worry about filtering stuff yourself, the theme system will automatically take care of it for you", but if you run user input through t() the theme system won't take care of it for you anymore. Running all t() strings through an admin-XSS filter seems worth considering, but I wonder if there are any negative performance implications from doing that.

Created by: Berdir

Steps to reproduce

Proposed resolution

TBA

Remaining tasks

Determine the next steps here. Will this be child issues etc?
TBA

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Active

Version

10.1 ✨

Component
Bootstrap  →

Last updated 26 days ago

No maintainer
Created by

🇺🇸United States micnap

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 2 years ago →
    🇳🇿New Zealand quietone

    There has been no activity here for in the 6 years this issue has been opened. And, of course, things get fixed in the meantime.

    Is there anything in the issue summary that is still relevant and needs work?

    I asked about this in #bugsmash. lendude pointed out that t($user_input) should never happen. So perhaps the whole premise here is outdated?

    Since we need more information to move forward with this issue, I am setting the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

    Thanks!

  • Status changed to Active almost 2 years ago
  • Comment almost 2 years ago →
    🇨🇭Switzerland berdir Switzerland

    As mentioned in slack, I fully agree it _should_ not happen but it does frequently enough in contrib and custom code. http://grep.xnddx.ru/search?text=-%3Et%28%24&filename= has 30 pages of results. Not all of that is user input, but a fair share is going to be. One common pattern is translating widget and formatter configuration like that as core still lacks a UI to do that.

    I tried reporting this once as a security issue as well and it was decided to make it public.

    Agreed it's a task though and not a bug.

  • Comment almost 2 years ago →
    🇳🇿New Zealand quietone

    @Berdir, thank you!

    So this needs an issue summary update to identify the next steps here.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024