Prevent strings unsafe for translation in code

Making the html tags show up in the issue summary and adjusting title. It's for XSS and other reasons like defacement and phishing.

OK... I think you are saying that there is a list of HTML tags that, effectively, cannot or should not be used in any translatable strings, because if they are translated on localize.drupal.org, then those translations are imported into a site, they will be skipped. Meaning those strings are effectively not translatable with our tools. Right?

Then should we generate errors in POTX when such strings are read from projects, and make issues in projects to remove them? It would be good do document this somewhere too?

And/or do we need a Coder rule to detect this?

Adding issue summary.

I believe div/code are not the problems. Attributes inside a div (<div class='alert alert-warning alert-sm'><strong>WARNUNG:</strong>)or invalid html like <front>.

All html-elements which are not part of the list in locale_string_is_safe() should be detected. perhaps a fancy combination of potx + coder.

Here's a thought about how we could enforce this in Drupal core at least.

When anything is run through t() and the other translation functions in Drupal, the actual string translation ends up being run through the 'string_translation' service.

So, we could instrument all Functional tests (the ones that visit UI pages) with their own string translation service class, which could override the doTranslate() method so that it runs all the strings that come in through locale_string_is_safe(), and flags an error if they fail that test. This could be run even if the locale module wasn't turned on.

Here's a patch that implements #14, more or less. What it does: Unless a test using BrowserTestBase overrides the default member variable $strictStrings = TRUE, every translatable string that is rendered during the test is checked to see that it is locale-safe. I added this to the existing FunctionalTestSetupTrait, which is also where the config strict checking is done. As the patch is now, it would also affect any contrib modules that run BrowserTestBase tests.

I verified locally that I could make a test fail by editing the code so that there was a string rendered with a <div> tag in it. Then when I removed that line from my code, my test passed. (Eventually, this patch probably needs to have a unit or functional test in it that verifies that the test is actually working right, by introducing a string that isn't locale safe and verifying an exception is thrown, but let's see if this is even a viable approach first.)

So... Let's see what happens when we run the Core tests. Are there locale-unsafe strings in Core? If so, they should fail the tests and we should fix them. I expect there might be some extraneous fails too if there are some tests in Core that are using locale-unsafe strings to test something.

Fix code sniff.

Two tests in the System module apparently have non-locale-safe strings being used in the tests. So, for those tests, disable checking. Also fix up some docs.

OK, tests pass now... To verify that this test addition would have caught non-locale-safe strings, I made up a should-fail patch by adding a div tag in some HTML output for a page that hopefully is displayed in a test. The real patch still probably needs a test for the test?

The test bar says "Build successful", but if you click through to the results on that last patch, you'll see that about 15 tests failed due to the div tag in the string. So... Any thoughts about this approach?

I discussed this in Slack today with @alexpott and he suggested putting an assert into the TranslatableMarkup constructor instead of the current approach of an exception in a custom translator. I'm working on a patch for that.

Here's a patch with this new approach. The tests pass locally for me, with an update to the Breadcrumb test module so that it doesn't use an non-locale-safe string.

Interdiff is not useful -- this is a completely new approach and every line of the patch is different from the previous patch.

OK! This is good. The new test found an actual error -- a typo in a string in the node module where it had <br/> instead of <br />. Here's a fix.

Looks like 1 test had the bad string from node module hard-wired in it.

Updating issue summary. Also made a draft change record. https://www.drupal.org/node/3199803

I believe this is ready for review.

Updating title too.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Re-rolled #26

Patch compatible with version 11.3.2