Validate translation strings on input/import

Looks good but needs some testing with more than one translation.

pwoladin, how about a simpletest test case?

Hum. I don't see the point in validating user input here. Doesn't Drupal assume that the administrator (which is the only one that can import or edit translations by default) is always fully trusted?

Moreover, the easier attack vector on translation files seems to me not from translation strings, but directly on the plural formula, that is evaluated as PHP code without any sanitation.

I don't like this line:

form_set_error('translations', t('The string you submitted is invalid - it may contain unsafe HTML.'));

Which string? This may be confusing not only for end users. Can you include the string with check_plain (so it's safe :)? Or at least some unique information about the string so you are able to identify it.

I was a little bit confusing in my last message. The line I was reffering to was correct. However, the same applies for importing the PO file.

Attaching a patch which remembers strings that were skipped and presents them to the user.

However: marking this as Code needs work because during testing, I noticed that this string gets skipped:

Custom title for the block. (Empty string will use block default title, <none> will remove the title, text will cause block to use specified title.)

Due to the tag <none> so I guess the approach with tags whitelisting may not be enough. Anyway, I've (I hope that correctly) grepped Czech translation of core, which is 100 % complete, for HTML tags and here we go:

<a> <abbr> <acronym> <br> <code> <description> <em> <h1> <li> <link> <none> <ol> <p> <pre> <span> <strong> <sub> <sup> <ul>

But there may be much more tags in contrib...

Oops, sorry. Here is the list:

<a> <abbr> <acronym> <br> <code> <description> <em> <h1> <li> <link> <none> <ol> <p> <pre> <span> <strong> <sub> <sup> <ul>

1. I think we'd better do a list of HTML tags appearing in Drupal core and contrib at this point and discuss support for those. I would say it is unlikely that more will appear. We clearly need the a tag, since inline HTML links are explicitly suggested for easier translations. meba has a great list for core. I've heard he might be doing a full contrib scan as well, so it would be great to see the bigger picture.

2. While I share Damien Tournoud's concerns on the plural formula which is converted and then evaluated as PHP code, we should tighten up where we can. We can certainly improve on validation of the plural forumla, since we know about the few characters which are allowed. So we can quite clearly define the possible parts of a plural formula and lock is down hopefully enough. That should be another issue. I've opened it at http://drupal.org/node/323155 with detailed background info and suggestions.

3. This issue is not only to be backported to 5.x and 6.x, it is also relevant for l10n_client and l10n_server, since those should also stop people submitting translations which are not valid. This whole situation of being worries about translations arose from the more commonplace translation tools, which allow anyone to contribute not just people with vetted CVS accounts.

Ok, http://drupal.org/node/323155 turned out to be a non-issue / false alarm. We should be fully back at dealing with locale input sanitization on the HTML :).

I think the HTML tags whiltelist is the way to go.

Just an idea to consider: What if we get out the html tags from the English string, and use that very same tags for validation of the translated string? Is there any case where we may need different tags?

Also, better if we have a reusable validation function, so it can be used by other module/s too like the l10n collection.

Attaching a list of tags I found in contrib. There are some very interesting tags :)

Please note that if a tag has several attributes, there can be any combination of them, I joined them together.

Oh, wow, <?php !invoke; print l($node-> is clearly a WTF, agreed. As for the others, there are mostly acceptable tags, but there are suspicious tags, like <description> and <destination>, which seem like indicating an inline tag requirement in t()-s where some "random" XML format is generated, not HTML. How do we deal with that? We suggest people using inline tags in t() to avoid breaking it down to untranslatable half-sentences and words. This is going to be quite interesting... Hm.

The PHP comes from:

!invoke; print l($node->title, $node->field_file_field[0]['filepath'], array('class' => 'media'));

jquery_media: 6.x-1.0 (1), 6.x-1.1 (1)

All unusual tags:

<description>

The parent website's description; comes from the <description> element in the feed.

Used in Drupal core, og_aggregator

<destination>, <command>, <classes>:

Usage: drush [options] sync <source> <destination>

Drush module obviously :)

<basic>:

<basic validation="">

Seems like Views module

<input>:
Several modules

<netnews>

...               generated for this Netnews Link. The file will be written to
               modules/netnews/<netnews link="" name="">.trace ...

Netnews module

<script>
Several modules

<use>:

<use sitewide setting>

FB module

<current>:

<current>Failed to load user from facebook fbu=%fbu

Facebook

<hx>:

Create table of Contents based on <hx></hx> tags.

<empty>:
CCK, Fieldreference, uc_cart_widget

Jose: validating against the source string does not really work when you import a .po file since then you have source and target strings as "user input", so the source can be tainted as well. When does that show up anywhere on the UI then is good question though.

meba: these look like things we can tell people to just move out of t(), since they are not stuff to actually translate. Then we could use the rest of the list for the whitelisting filtering code.

Erm, no. There should be HTML links for example in translatable text to preserve context for the text of the link for translation.

Looks good to me. Passes test. It does validate strings on UI submission, file import and provides an API function which modules such as l10n_server and l10n_client can use to do the same. I was thinking of two remaining things:

- It might be better to put the validation into _locale_import_one_string_db() since that is the last stop before the database. That would help people reuse that function to get prevented.
- _locale_import_one_string_db() might be improved then to also count 'refused' strings, which would be another possibility in $report, so that we can report refused strings on import as well. That would help people debug bad translations. Some modules might still use disallowed tags, so that would help us migrate to a safer localization environment with contributed modules. So in short, we should not be entirely silent about dropped strings. Let the user debug (and know we protected them :).

It is a security improvement, so I think a little bit of API change is acceptable in this case. It is really just:

- a new key in the $report array, existing ones are not changed
- a new message added to the number of things imported (I'd prefer to concatenate a new sentence, instead of modifying the existing one, so that we keep the existing); maybe modify the existing one in Drupal 7, add a new string on Drupal 6

Otherwise I have no issues with the patch, so keep up the good work!

Looks great. The only point I have to say now, is to move the comment from the top of locale_string_is_safe() to the phpdoc above it, so it appears better in API docs. That is minor, so setting back for review. As long as tests pass, I am all for this patch.

Committed to CVS HEAD. Thanks!

Thanks, committed to 6.x. Since the 5.x is a bit different, it probably needs some more testing there.

There is a minor typo in the committed code becuase. http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/locale.inc?r1=1.1...

This caused issues with i18n "block translation" feature, where a lot of markup should be allowed... Please see #352121: locale_string_is_safe() is not suitable for user provided text in non-default textgroups.

#352121: locale_string_is_safe() is not suitable for user provided text in non-default textgroups is a valid issue so I retitled it and following up there.

#40 says this was fixed in Drupal 7.x and #45 says this was fixed in Drupal 6.x. The issue remains open only to backport the change to Drupal 5.x. Since Drupal 5.x is no longer supported, I'm marking this issue as won't fix.