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.

I'm not sure how translation server is invoked to import translations.

Does it call locale_translate_edit_form_validate()? 'Cause if not, and locale_translate_edit_form_validate is called in case of user input,
then it's possible to patch locale.inc somewhat like (attached as patch as well)

--- includes/locale.inc
+++ includes/locale.inc
@@ -847,10 +847,7 @@
*/
function locale_translate_edit_form_validate($form, &$form_state) {
foreach ($form_state['values']['translations'] as $key => $value) {
- if (!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);
- }
+ $form_state['values']['translations'][$key] = check_plain($value);
}
}

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.

Ok, didn't understand your initial comment.
Here's updated version (is it what you meant?):

Index: includes/locale.inc
===================================================================
--- 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);
}

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.

the same needs to be done with .po files imported with the textgroup specified - what do you mean?

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.

I still don't get the point why add some global check for all locale strings, when in fact, we need only to check input from translation server?
Maybe it's better to check input from translation server, and that's it? I'm not so aware of drupal core internals to quickly find callback which handles such input, the only thing I found is i10n_server/i10n_client pair. If I'm right, then it's no need to place check in locale.inc at all, but place it in i10n_client.

I still don't get the point why add some global check for all locale strings, when in fact, we need only to check input from translation server?
Maybe it's better to check input from translation server, and that's it? I'm not so aware of drupal core internals to quickly find callback which handles such input, the only thing I found is i10n_server/i10n_client pair. If I'm right, then it's no need to place check in locale.inc at all, but place it in i10n_client.

@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.

Sorry for delay - you know, all these holidays and damn fanatics on our South...
Anyway, attached patch added check on strings import (for default textgroup only), as well as form submit check

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

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

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.

Another try...

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

- 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"

...and once again.

The last submitted patch failed testing.

Small typo catch by Damien :

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

So, what's the next step? :)

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

Seems like JS error (XMLFlashSlideshow_v3 not defined), not translation-related. Try to compare JSs loaded for English and Spanish versions

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.

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

Automatically closed -- issue fixed for 2 weeks with no activity.

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.