How to securely run D7 themes?

[effulgentsia]

#12 is cool!

I'm assuming the motivation for it is to be able to run a site's custom Drupal 7 theme in Drupal 10. What are your thoughts on how to handle print statements within tpl.php files? In Twig, {{ foo }} is smart about knowing whether foo needs to be escaped, and so in Drupal 10 we don't pre-escape variables passed to the template.

One possible approach would be to instruct the site owner to replace all occurrences of print with retrofit_drupal_print() or similar, so that that function could then do something similar to what Twig's {{ foo }} does.

Any other approaches worth considering?

[mglaman]

The answer is: folks should migrate their PHPTemplate templates over to Twig!

The goal behind Retrofit is to lower the effort required to move to a supported version of Drupal in incremental stages. The security concern expressed is already existing on their site. Moving that code to a newer version of Drupal does not exacerbate the security concerns.

This package could provide a retrofit_print function to safeguard and escape output. But the primary goal is to allow moving onto a supported version of Drupal as fast as possible with a minimal amount of code rewrites.

[effulgentsia]

The answer is: folks should migrate their PHPTemplate templates over to Twig!

I agree with this. Do you think Retrofit (or another project) should provide a tpl.php to twig converter to automate the parts of that that can be automated? There's some prior art in https://github.com/makinacorpus/php-twig-converter and discussion in rectorphp/rector#7097; not sure if there are any other projects that have taken it further.

The security concern expressed is already existing on their site. Moving that code to a newer version of Drupal does not exacerbate the security concerns.

I disagree with this. What exacerbates the concern is that in D7, you could mostly rely on preprocess functions to escape the variables that need escaping and in D10 you can't. For example, D7's field.tpl.php does print $label and that's okay because template_preproces_field() calls check_plain() on it. D10's template_preprocess_field() does not call check_plain() because instead we defer to Twig's autoescaping, so running field.tpl.php on a D10 site would introduce XSS via that. Retrofit could add preprocess functions to check_plain() a bunch of variables, at least from core and popular contrib, but it would almost certainly miss some, so I think a retrofit_print function would probably be more robust.

But the primary goal is to allow moving onto a supported version of Drupal as fast as possible with a minimal amount of code rewrites.

Are you saying that the idea would be to only run Retrofit during development and not on a production site? In that case, should docs/warnings be added to make that super clear to site owners? Because otherwise, I could easily imagine people thinking they can use Retrofit to upgrade from 7 to 10 in stages, but also deploy those interim stages to production.

[mglaman]

Do you think Retrofit (or another project) should provide a tpl.php to twig converter

No, that is out of scope. This just ensures teams can start iterating on their refactors without it being a complete brick wall.

What exacerbates the concern is that in D7, you could mostly rely on preprocess functions to escape the variables that need escaping and in D10 you can't.

I understand. D7 had the escaping built into it's preprocess functions, we now delegate to Twig. So someone calling print may not have sanitized output that they previously thought may have been. This package can add retrofit_print which can be documented as a recommended replacement for print calls that are from user input.

Or this package or wire in its own preprocess function to replicate what D7 did.

  $variables['label'] = check_plain($variables['label']);

It'd be a game of whack-a-mole for these kind of things, but this entire package is a game of that.

Are you saying that the idea would be to only run Retrofit during development and not on a production site?

No, this is a production tool so that folks can get off Drupal 7 in production onto Drupal 10+ and iteratively migrate their code.

[Greg-Boggs]

Drupal 10 also requires you to filter user input manually in the same way that Drupal 7 did. It's just that Drupal 10 has a bonus feature to save your bacon when you forget.