Problem/Motivation

We have some places in code where we pass the result from t() to Html::escape().
This causes errors, because Html::escape() expects a string.

Besides, it is quite uncommon to further escape the result of t().
Also, we would call $this->t() and not t()..

Steps to reproduce

Visit /admin/config/l10n-server/connectors
For one of the connectors, choose "Parse" or "Scan" from "Operations" column.
In the confirm form, choose "Confirm".

Expected: Operation completes.
Actual: Error "TypeError: Drupal\Component\Utility\Html::escape(): Argument #1 ($text) must be of type string, Drupal\Core\StringTranslation\TranslatableMarkup given, called in /var/www/html/l10n_server/src/Form/ConnectorBatchConfirmScanForm.php"

Proposed resolution

Simple solution: Cast to string before calling Html::escape():

Html::escape((string) t(..));

Real solution: Review why we are translating and escaping this stuff.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork l10n_server-3563271

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

donquixote created an issue. See original summary.

donquixote’s picture

donquixote commented
Issue summary: View changes

donquixote opened merge request !105

donquixote’s picture

donquixote commented
Status: Active » Needs review

We could start with this simple fix but then keep the issue open to find a proper solution.

fmb’s picture

fmb commented
Status: Needs review » Needs work

Besides, it is quite uncommon to further escape the result of t().

I think we should just get rid of Html::escape(). Can you manually test without it?

Also, we would call $this->t() and not t().

Not in a static context.

Also, "hase" is just a typo for "has".

donquixote’s picture

donquixote commented

Alright.
I had a look how core handles bad html in t().
As an example, I looked at translated strings in block UI.

I tried with `<script>alert("hello")</script>` and with `<p>` (open tag without close tag).
Here "unfiltered" means that the alert pops up and the markup is garbled thanks to the incomplete markup.

  • If bad html is passed to t(), it is displayed unfiltered. This is ok because these strings are defined in code, not coming from any kind of user input.
  • If the translations database already contains bad html, it is displayed unfiltered.
  • The TranslateEditForm calls locale_string_is_safe() for validation.
  • The PoDatabaseWriter calls locale_string_is_safe() for validation of imported translations.
  • locale_string_is_safe() detects (and rejects) malicious html (scripts) but not all malformed html (the `<p>` tag)

As a conclusion, I'd say it is safe to drop the Html::escape(), and treat the result of t() as trustworthy, as this is also what core does.

donquixote’s picture

donquixote commented

The next question would be:
Does the $context['results'] in a batch operation allow html?

It seems yes: In _node_mass_update_batch_process() we see a link being assigned.

    $context['results'][] = Link::fromTextAndUrl($node->label(), $node->toUrl())->toString();

This means we don't need to remove html.

donquixote’s picture

donquixote commented

I want to mention that we currently cannot test the specific functionality in l10n_server until we fix it, which is out of scope for this issue.

donquixote’s picture

donquixote commented
Status: Needs work » Needs review

  • fmb committed 68df19b8 on 3.0.x 
    Merge branch '3563271-cannot-pass-t-to-html-escape' into '3.0.x'

Issue...

  • fmb committed 7fb34f3e on 3.0.x 
    Issue #3563271: get rid of unused uses

  • donquixote committed 6c6eb71e on 3.0.x 
    Issue #3563271: Drop the Html::escape() around t().

  • donquixote committed 44820c77 on 3.0.x 
    Issue #3563271: Cast to string before calling Html::escape().

fmb’s picture

fmb commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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