SSRF vis role name on content translation screen

larowlan → credited MrSzymon → .

Updating steps to reproduce.

Edit issue title

The title is SSRF (server side request forgery) which I understand to mean a request from the server to other machines on the server's network as described in portswigger and owasp.

The description includes:

The vulnerability allows one Drupal user with administrative permissions to create a page which can force another Drupal user to make a call to a server under the control of an attacker.

and

The browser will make a call to the malicious server

That is not a server-side request.

I can see how this would let an admin gain data about the site visitors, which is not ideal, but on most sites an admin who can enter translations can probably already make other changes to a site that would also gain information about a visitor.

I'm surprised if this is the only place where a user with only the ability to translate could insert an image. I think other strings are similarly not filtered.

Seems the test-only failure is passing when would expect to fail so think that needs tweaking https://git.drupalcode.org/issue/drupal-3440399/-/jobs/4273224

I adjusted the test to fail without the fix.

this does a strip tags, that seems wrong and the escape then is pointless?

if the source config has some HTML, you want to see that as the translation needs the same.

It should just escape and should then also assert the escaped markup

I removed the strip_tags() call, I think it was added in order to not break ConfigTranslationUiModulesTest.
And I added an assertion with the escaped string (it needs to be precise because other parts of the page already contain the escaped payload in other places).

Both tests appearing to be failing as expected

1) Drupal\Tests\config_translation\Functional\ConfigTranslationUiModulesTest::testBooleanFieldConfigTranslation
Behat\Mink\Exception\ExpectationException: The string "On label (with <em>HTML</em> & things) Boolean settings" was not found anywhere in the HTML response of the current page.
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:363
/builds/issue/drupal-3440399/core/tests/Drupal/Tests/WebAssert.php:559
/builds/issue/drupal-3440399/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiModulesTest.php:280
FAILURES!
Tests: 7, Assertions: 198, Failures: 1.

New test

1) Drupal\Tests\config_translation\Functional\ConfigTranslationUiTest::testLabelEscaping
Behat\Mink\Exception\ExpectationException: The string "<img src="http://127.0.0.1/evil">" appears in the HTML response of this page, but it should not.
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:888
/builds/issue/drupal-3440399/vendor/behat/mink/src/WebAssert.php:380
/builds/issue/drupal-3440399/core/tests/Drupal/Tests/WebAssert.php:571
/builds/issue/drupal-3440399/core/modules/config_translation/tests/src/Functional/ConfigTranslationUiTest.php:353
FAILURES!
Tests: 9, Assertion

Solution matches what's in the summary which seems to be written by security team.

Believe this one is good.

Do we know why the nl2br() was originally there? Do we need to care about representing newlines in the string correctly still?

Traced this back to an issue in the original config_translation module, where nl2br() was explicitly added to fix a bug: #1945184: Original value display is broken →

Therefore I think we should keep this feature? (and probably add a test for it...)