[5.6] Revert "escape lang directive echos"

[bonzai]

d3c0a36 is a breaking change because you can no longer use HTML tags inside translations:

5.6.35:

5.6.36:

Now instead of:

@lang('Hello')

I need to use something like:

{{ new HtmlString(__('Hello')) }}
{!! __('Hello') !!}

This affected more than one application.

[brunogaspar]

Isn't it perhaps a security fix? Just wondering though, otherwise it's a big breaking change indeed.

[GrahamCampbell]

If it's a security fix, it should have also gone into 5.5, but I don't see anything. Perhaps this needs reverting in 5.6 and re-adding in 5.7?

[taylorotwell]

It's a security fix and won't be reverted. There was an XSS vulnerability in that situation when passing parameters as the second method to @lang.

[taylorotwell]

@GrahamCampbell it is in 5.5 as well. I pushed fixes to both branches and will be discussing more tomorrow.

[stayallive]

Shouldn't the parameters be escaped then? Not the full output... language strings can be considered safe!?

This was used a lot in my (and from other comments, many) applications to make some text bold or add some other inline styling without breaking up in multiple translation strings.

[cbaconnier]

It also breaks the "verify email address" link

If you’re having trouble clicking the "Verify Email Address" button, copy and paste the URL below into your web browser: https://57.test/email/verify/1?expires=1536138767&signature=6a4634ddb2d4c0037aecadfc1ab22ba766a0717ecfa00edcec7a955e3108b64c

The link appear as (notice &)
https://57.test/email/verify/1?expires=1536138767&signature=6a4634ddb2d4c0037aecadfc1ab22ba766a0717ecfa00edcec7a955e3108b64c and the user get a 403 error

[zanozik]

Thanks for breaking all my apps in this micro @taylorotwell without any heads-ups, reverting to 5.6.35 for now.

[marcbelletre]

Same here. Everything is broken in all my apps because I was always using @lang directive for displaying translations.

[stayallive]

I don't recommend it for the long term (especially since this was done out of security reasons, check good what you pass to a language variable!).

But in my case I did not have the time to go through all 250 or so language strings with <b> and <em> tags in them and fix / rewrite them so I replaced the laravel ViewServiceProvider in config/app.php with this one:

<?php

namespace App\Providers;

use Illuminate\View\Engines\CompilerEngine;
use Illuminate\View\Compilers\BladeCompiler;
use Illuminate\View\ViewServiceProvider as IlluminateViewServiceProvider;

class ViewServiceProvider extends IlluminateViewServiceProvider
{
    /**
     * Register the Blade engine implementation.
     *
     * @param  \Illuminate\View\Engines\EngineResolver $resolver
     *
     * @return void
     */
    public function registerBladeEngine($resolver)
    {
        // The Compiler engine requires an instance of the CompilerInterface, which in
        // this case will be the Blade compiler, so we'll first create the compiler
        // instance to pass into the engine so it can compile the views properly.
        $this->app->singleton('blade.compiler', function () {
            return new class($this->app['files'], $this->app['config']['view.compiled']) extends BladeCompiler
            {
                /**
                 * Compile the lang statements into valid PHP.
                 *
                 * @param  string $expression
                 *
                 * @return string
                 */
                protected function compileLang($expression)
                {
                    if (is_null($expression)) {
                        return '<?php $__env->startTranslation(); ?>';
                    } elseif ($expression[1] === '[') {
                        return "<?php \$__env->startTranslation{$expression}; ?>";
                    }

                    return "<?php echo app('translator')->getFromJson{$expression}; ?>";
                }

                /**
                 * Compile the end-lang statements into valid PHP.
                 *
                 * @return string
                 */
                protected function compileEndlang()
                {
                    return '<?php echo $__env->renderTranslation(); ?>';
                }

                /**
                 * Compile the choice statements into valid PHP.
                 *
                 * @param  string $expression
                 *
                 * @return string
                 */
                protected function compileChoice($expression)
                {
                    return "<?php echo app('translator')->choice{$expression}; ?>";
                }
            };
        });

        $resolver->register('blade', function () {
            return new CompilerEngine($this->app['blade.compiler']);
        });
    }
}

It reverts back to the old way it worked while I go through the process of reworking all things but still can move forward with 5.7 and it's goodness.

[Orillian]

@taylorotwell No chance of escaping just the parameters? Content coming out of the translation files themselves should be considered safe should they not?

[at-dro]

While it is obvious that there is an XSS vulnerability here I don't think this is a proper fix for the problem:

• The translation strings themselves can be considered XSS-safe. If there was malicious code in a translation file we would have a completely different problem anyway.
• HTML-Tags in translation strings are a valid requirement - the need to break up the string and hardcode things like bold, emphasis etc. would defeat the purpose of the translation system.
• Even though this addresses a potential security problem it is still an undocumented breaking change. The documentation never made any claims about @lang escaping anything.

The way this was fixed means that in order to have HTML in translation strings we have to use {!! !!}. This is a bit better than the previous way, since everyone should be aware of the security implications now - nevertheless it re-introduces the XSS problem for parameters. Hence the fix is incomplete, because we can't have both HTML-Tags and XSS sanitation at the same time (other than escaping manually).

The proper fix should escape each replacement value individually (in Illuminate\Translation\Translator::makeReplacements). Since in some cases unescaped values might be desired it would probably be best to add an optional parameter to the translation methods to turn off escaping entirely.

In any case this change should be documented on the Localization page, or better yet a paragraph added to the release notes of affected versions (like it has been done for the cookie encryption fix).

[gerardojbaez]

@at-dro I was about to suggest the same (adding the fix to lluminate\Translation\Translator::makeReplacements). The translator class is a better place for the fix. The goal is to escape end-user inputs, not your own copy.

[staudenmeir]

@at-dro Have you looked into an implementation?

I'm not sure the Translator class should escape the replacements by default, I don't think that's its job.
We can add a $escape = false parameter to getFromJson() etc., but I didn't find a clean way to pass true when the view is compiled (CompilesTranslations). The regex "parsing" makes this quite difficult.