locale_string_is_safe() is not suitable for user provided text in non-default textgroups

Got a tip at http://drupal.org/node/276111 that it caused problems.

Allowing img and div in translations would open all Drupal sites to XSS attacks once we put the online translation server in place with automated distribution of translations. People seem to want to have more automated distribution for translations, but we could not do it without having proper protection against attacks on your site.

We might be able to use the textgroup to assume non-default textgroups strings as safe (given we treat them as user input) regardless of their content to let i18nstrings module to work. The non-default textgroups are used to store stuff which will be displayed through the filter system or other mechanisms to escape content (at least they are assumed to be used as such).

Retitled.

Grm, I am just saying that if the form state says we deal with a non-default textgroup, we should skip this security check. Your patch always removes this check which is not right.

Yes, except some coding style changes. We'd need to have the comment on the line before in sentence format (capitalized at the start, dot at the end). Also, the same needs to be done with .po files imported with the textgroup specified. That should cover the cases we have in core IMHO.

When .po files are imported on the locale interface, you can choose a textgroup. .po files not imported for the default textgroup should also omit this check to be in line with what we do here.

@valthebald: The l10n_client currently does not pull translations from a server, and the reason was exactly this problem (of using unfiltered strings) not being fixed. Now that we are filtering those, we can finally implement it. However, the l10n_server's currently export .po files for import in Drupal. Now the .po import always does this check. But you can export and do your block translations outside of Drupal as well with the same .po mechanism. So there we should also have a textgroup dependent check.

The patch looks good. Some (minor) coding style issues:

--- includes/locale.inc
+++ includes/locale.inc
@@ -846,8 +846,9 @@
  * Validate string editing form submissions.
  */
 function locale_translate_edit_form_validate($form, &$form_state) {
+  $safe_check_needed = $form_state['values']['textgroup']=='default'; // locale string check is needed for default textgroup only
   foreach ($form_state['values']['translations'] as $key => $value) {
-    if (!locale_string_is_safe($value)) {
+    if ($safe_check_needed && !locale_string_is_safe($value)) {
       form_set_error('translations', t('The submitted string contains disallowed HTML: %string', array('%string' => $value)));
       watchdog('locale', 'Attempted submission of a translation string with disallowed HTML: %string', array('%string' => $value), WATCHDOG_WARNING);
     }

We (virtually) always put comments on their own line. Plus comments should be proper sentences (beginning with an uppercase letter, ending with a period).

@@ -1338,8 +1339,11 @@
   $lid = db_result(db_query("SELECT lid FROM {locales_source} WHERE source = '%s' AND textgroup = '%s'", $source, $textgroup));
 
   if (!empty($translation)) {
-     // Skip this string unless it passes a check for dangerous code.
-     if (!locale_string_is_safe($translation)) {
+     /**
+      * Skip this string unless it passes a check for dangerous code.
+      * Yet text groups other than default still can contain HTML tags (i.e. translatable blocks)
+      */
+     if ($textgroup=="default" && !locale_string_is_safe($translation)) {
        $report['skips']++;
        $lid = 0;
      }

We don't use C style comments inline. Please transform this block using "//"-style comments. Plus a period is missing at the end.

This should apply (unmodified) to 7 too, bumping version.

We are getting there :) Some additional code style issues (including one I missed on first review, sorry):

@@ -1339,7 +1341,8 @@
 
   if (!empty($translation)) {
      // Skip this string unless it passes a check for dangerous code.
-     if (!locale_string_is_safe($translation)) {
+     // Text groups other than default still can contain HTML tags (i.e. translatable blocks).
+     if ($textgroup=="default" && !locale_string_is_safe($translation)) {
        $report['skips']++;
        $lid = 0;
      }

- The comment line should not cross the 80 character boundary. You will need to split that comment line (for example before "(i.e.").
- We always add a space before and after an operator: $textgroup=="default" should be $textgroup == "default"

Small typo catch by Damien :

We always add a space before and after an operator: $textgroup=="default" should be $textgroup == "default"

The patch itself looks good.

I extended the test to cover this case, confirmed that the test fails when run alone and that the patch makes it pass.

Because the test bot was happy earlier (http://testing.drupal.org/pifr/file/1/issue-352121.patch says Passed: 8866 passes, 0 fails, 0 exceptions), and because the approach got Gabor approval, I'm marking this RTBC right away.

Committed to HEAD, per Gábor.

Marking back to 6.x.

Err.. I'm fairly sure the CVS commit didn't go smoothly, and the local_test module wasn't added to CVS, so HEAD is broken.

+1 Subscribe.

Trying to set up a site with 2 languages using i18n, but we're hampered by the problems outlined in this issue when it comes to translating blocks.

A quick glance does indicate that the above patch did make it into 7.x-dev so not sure what the status of this issue is (should it be backported, or did something go wrong against 7.x-dev?)

HEAD is fixed now.

#19 seems to be the "right" patch for Drupal 6 (it does only include the fix without the test, as Drupal 6 expects), BUT it does not apply anymore. Porting required. If #20 changed the logic as well, then porting that would be desired.

subscribing (broke a site upgrading from 6.6 to 6.9)

re-rolled the patch from #19 so it applies to Drupal 6 HEAD again. #21 did not change the logic, only added the tests.

I've applied this patch...but it doesn't seem to solve my problem. I'm am translating a page which has javascript (insert of flash xml slideshow) and it won't display on the translated page - so I'm assuming the code isn't being allow. Any ideas on a way to fix this?
Example: http://cima.laurelterlesky.ca/shipping-logistics (english works fine / spanish won't display)
This is with il8n version 6.1

Awesome thanks! It just wasn't picking up the javascript path correctly on the translated pages. off and running now.

I am using the patch in #30 on drupal-6.12 and works ok for me.
Now I can translate custom blocks, thanks.

Can you please consider applying it to D6 as well?

Regards,
Antonio

Committed to Drupal 6 too, thanks!

which makes it almost impossible to implement translatable blocks

just a note on the original problem: translation of custom block should be done as node translation. It is wrong to use the string translation functionality to translate blocks.. This idea of translating large blob of text same as simple string translation with t() is absolutely unusable..

So for D6 the right approach would be to create a new block and assign language for that block..

this patch is wrong.

+  // Locale string check is needed for default textgroup only.
+  $safe_check_needed = $form_state['values']['textgroup'] == 'default';

WTF? How can I create a new textgroup where the safe string check should be applied?

The check should be applied for every textgroup, if you want to omit the check then you should specially say so (in configuration or hook). It is wrong to omit the check for every textgroup as most of the strings in different textgroups should not contain disallowed html..

Pasqualle: Features like nodes as blocks is not part of core. Also, as said multiple times above, the non-default textgroups are used for things like blocks, menus, taxonomy, etc, which is assumed to be user input and is therefore filtered or escaped on use.

The block translation is also not part of core. I just say when you want to translate a block it is better to create a new block than using t() on a HTML text blob..

The textgroups should have a format value associated. As format used in views_handler_field_markup in views module. So the strings can be easily checked with check_markup($value, $format, FALSE); at entry also.

I correct myself: the patch is wrong the patch is not good enough for Drupal 7

Marked #368173: Full HTML Blocks cannot be translated as a duplicate of this issue.

Marked #421228: Blocks html code differs between languages as a duplicate of this issue.

Marked #465436: Error: The submitted string contains disallowed HTML as a duplicate of this issue.

The same problem, but in the "Help text" for CCK field. Re-saving the field does not help, neither the refreshing cck strings for translation.

My string: "Enter your education details. For pasting test from other source (e.g., MS Word) use the icon "Paste text": . You will find the template text in Employees/Example-name."

It is really important for me to have an icon in the explanation text...

Hi, I have a view with a field using the "Rewrite the output of this field" option. The text to display is a concatenation of other fields:

<strong>[title]</strong>
[body]
<div><i>Package quantity</i>: [field_qty_value]</div>
<div><i>Weight</i>: [weight]</div>

I need to localize it in another language, but .../admin/build/translate/edit/... form doesn't allow it (because The submitted string contains disallowed HTML).

Am I posting in the right thread (since it is locale.inc giving that error) ?
Or is it a view bug or a i18n bug ?

Thanks in advance.

I think this is not a bug, this is some validation that happens on purpose because we need validation for these strings.

Btw, we've added the validation back into i18n translations so the patch here may have 'fixed' the issue for a while for some people, but not anymore. See #1437146: Rework string translation access. (After 1.4 update, block translation disallowed HTML error)

Now if anyone wants translators to submit unfiltered strings, please code your own module and share it (and deal with the related security issues.)

Or if anyone wants to go for a real solution, add proper 'text formats' for these texts so they can be filtered.

Please don't backport it to 6.x because then we'll need to fix i18n for 6.x too.

Well, after reading this again, it seems maybe we don't need to care about security for translators, ?, http://drupal.org/node/1471870

Anyway things like backporting these changes to D6 may require some long time running sites to change their security policy and rethink who has the 'translate interface' permission. Just to note it