🇬🇧United Kingdom @mcdruid

greggles → credited mcdruid → .

sorry, didn't mean to change status

Thanks, that's a good start. Looks like you've borrowed the list of chars to strip from wordpress. Perhaps we could remove a few of the "safe" ones based on the list in #7; we're only talking about a few e.g. % + = :.

FWIW here's how typo3 does this:

https://github.com/TYPO3-CMS/core/blob/v13.4.9/Classes/Resource/Driver/L...

The comments suggest that it's pretty strict:

... any character not matching [.a-zA-Z0-9_-] is substituted by '_'

...but in actual fact it's a bit more nuanced as there are some settings around allowing / remapping utf8 characters.

I'll try to do some work on this but am short on time just now.

FWIW by default Wordpress removes a load of "special" characters from filenames:

https://github.com/WordPress/wordpress-develop/blob/6.7.2/src/wp-include...

function sanitize_file_name( $filename ) {

...

	$special_chars = array( '?', '[', ']', '/', '\\', '=', '<', '>', ':', ';', ',', "'", '"', '&', '$', '#', '*', '(', ')', '|', '~', '`', '!', '{', '}', '%', '+', '’', '«', '»', '”', '“', chr( 0 ) );

...

	$filename = str_replace( $special_chars, '', $filename );

Joomla seems to have decided to let the underlying OS decide what it will and will not allow in filenames :shrug:

https://github.com/joomla/joomla-cms/issues/33214

Having said that, I went to look for a definitive list of "dangerous characters" and ended up with quite a lot of "it depends".

This is not definitive, but e.g. https://stackoverflow.com/questions/15783701/which-characters-need-to-be...

There are some quite good lists using e.g. the shell escaping option that printf (and apparently ls) offer - e.g.

No, character % does not need to be escaped
No, character + does not need to be escaped
No, character - does not need to be escaped
No, character . does not need to be escaped
No, character / does not need to be escaped
No, character : does not need to be escaped
No, character = does not need to be escaped
No, character @ does not need to be escaped
No, character _ does not need to be escaped
Yes, character   needs to be escaped
Yes, character ! needs to be escaped
Yes, character " needs to be escaped
Yes, character # needs to be escaped
Yes, character $ needs to be escaped
Yes, character & needs to be escaped
Yes, character ' needs to be escaped
Yes, character ( needs to be escaped
Yes, character ) needs to be escaped
Yes, character * needs to be escaped
Yes, character , needs to be escaped
Yes, character ; needs to be escaped
Yes, character < needs to be escaped
Yes, character > needs to be escaped
Yes, character ? needs to be escaped
Yes, character [ needs to be escaped
Yes, character \ needs to be escaped
Yes, character ] needs to be escaped
Yes, character ^ needs to be escaped
Yes, character ` needs to be escaped
Yes, character { needs to be escaped
Yes, character | needs to be escaped
Yes, character } needs to be escaped

I feel like most of those make sense; and there are a fair number of characters that would be allowed.

Whether this would be more or less confusing / user friendly than just stripping all punctuation though is perhaps a usability question.

Looks like the code mostly uses regex; I wonder whether the punct character class would be any use. It'd remove the "safe" characters from the list above which we may not want to do.

To be clear what I'm looking for here is something that would remove all the dangerous shell characters but not limit users to strict ASCII alphanum without e.g. accented letters etc..

@kim.pepper although the existing options would remove all of the characters I highlighted as a concern, they do more than that.

I wouldn't advocate stripping filenames down to only ASCII / alpha-numeric characters (by default or as a mandatory setting) because of how restrictive that is for different languages.

I'm proposing a fairly limited number of forbidden characters - those which have significance for the shell - which would be a subset of what would be stripped by any of the existing options IIUC?

I'd recommend that we enforce the removal (/ substitution) of the most dangerous characters.

For removal of spaces, that'd be ok as a default (would people really want to turn it off?!)

greggles → credited mcdruid → .

greggles → credited mcdruid → .

greggles → credited mcdruid → .

mcdruid → created an issue.

Ok, thanks for confirming.

I think 1.0.x is still marked as the default at https://git.drupalcode.org/project/cms/ FWIW.

mcdruid → created an issue.

mcdruid → created an issue.

benjifisher → credited mcdruid → .

poker10 → credited mcdruid → .

poker10 → credited mcdruid → .

poker10 → credited mcdruid → .

poker10 → credited mcdruid → .

Is this a duplicate of 📌 Add debug mode (and fix virus name for executable) Needs work (which has an MR)?

I've verified that if we inject something like an XSS payload into the scanner output, it gets logged verbatim.

For example (this is a low effort quick hacky test):

+++ b/src/Scanner/Executable.php
@@ -65,6 +65,9 @@ class Executable implements ScannerInterface {
     // values greater than 2. Any value of 2 or greater means that the scan
     // failed, and the file has not been checked.
     exec($cmd, $output, $return_code);
+
+    $output[] = '<script>alert("oh noes!")</script>';
+
     $this->_output = implode("\n", $output);

An example log entry in the db looks like:

> SELECT * FROM watchdog \G

*************************** 78. row ***************************
      wid: 78
      uid: 1
     type: Clam AV
  message: Virus %virusname detected in uploaded file %filename. Debug output: %output
variables: a:3:{s:9:"%filename";s:14:"/tmp/phpALzyP1";s:10:"%virusname";s:15:"Eicar-Signature";s:7:"%output";s:361:"/tmp/phpALzyP1: Eicar-Signature FOUND

----------- SCAN SUMMARY -----------
Known viruses: 8704988
Engine version: 1.0.7
Scanned directories: 0
Scanned files: 1
Infected files: 1
Data scanned: 0.00 MB
Data read: 0.00 MB (ratio 0.00:1)
Time: 10.241 sec (0 m 10 s)
Start Date: 2025:03:14 14:01:59
End Date:   2025:03:14 14:02:09
<script>alert("oh noes!")</script>";}
 severity: 3

The traditional approach in Drupal is to filter on output ( https://www.drupal.org/docs/develop/security/why-does-drupal-filter-on-o... → ). The dblog admin UI escapes this appropriately.

We could pass the debug output through a filter before logging it.. but I'm not sure that's necessary, or best practice.

We don't want to fill syslog log entries with lots of html-encoded characters unnecessarily, for example (I also don't think the typical output of the clam scanner will contain anything bad.. especially as - per my earlier comment - the user-chosen filename is not currently included in these logs).

So I think this MR's ready for review.

Same deal for connection to the daemon by socket - I've added the same scanner output as for the tcp/ip mode.

This is a bit duplicative.. but for now I've added the same message about the failed connection to the daemon to the $_output property so that it gets logged as the scanner output when debug is enabled.

I suppose we could omit the other log entry when debug is enabled, but I'm not certain that's a good idea; it might already be used for monitoring etc..

Daemon mode example log entries (again, verbose + debug enabled):

Uploaded file /tmp/phpuLzC2k checked and found clean. Debug output: stream: OK

Virus Eicar-Signature detected in uploaded file /tmp/phpnGu0G4. Debug output: stream: Eicar-Signature FOUND

...and if I tried to deliberately break the daemon set up..

Uploaded file /tmp/phpkCRZnu could not be checked, and was deleted. Debug output: 

Hmm that's not so useful. In that case the daemon wasn't actually listening for connections at all. Would probably be useful to try to log something vaguely informative in that situation.

If I try to break the config of the daemon it just doesn't start up.. so I think not being able to connect is one of the more realistic problems.

The module does actually log messages when it can't connect:

https://git.drupalcode.org/project/clamav/-/blob/2.1.x/src/Scanner/Daemo...

Unable to connect to ClamAV TCP/IP daemon on clamav:3310

Manually tested with executable mode; seems to work as expected.

Example log message when the scanner found a problem (with both verbose and debug enabled):

Virus Eicar-Signature detected in uploaded file /tmp/phpqQahST. Debug output: /tmp/phpqQahST: Eicar-Signature FOUND ----------- SCAN SUMMARY ----------- Known viruses: 8704988 Engine version: 1.0.7 Scanned directories: 0 Scanned files: 1 Infected files: 1 Data scanned: 0.00 MB Data read: 0.00 MB (ratio 0.00:1) Time: 10.072 sec (0 m 10 s) Start Date: 2025:03:14 11:57:30 End Date: 2025:03:14 11:57:40

It'd ideally be better if the original filename of the upload was logged instead of the tmp name, but that's a different issue.

Another example where I deliberately broke the clamscan executable - this is the sort of thing debug mode may be more useful for in reality:

Uploaded file /tmp/phpXYXjA4 could not be checked, and was deleted. Debug output: LibClamAV Error: cli_load(): Can't open file /var/lib/clamav/main.cvd LibClamAV Error: cli_loaddbdir: error loading database /var/lib/clamav/main.cvd LibClamAV Error: cli_loaddbdir: No supported database files found in /var/lib/clamav ERROR: Can't open file or directory ----------- SCAN SUMMARY ----------- Known viruses: 0 Engine version: 1.0.7 Scanned directories: 0 Scanned files: 0 Infected files: 0 Data scanned: 0.00 MB Data read: 0.00 MB (ratio 0.00:1) Time: 0.004 sec (0 m 0 s) Start Date: 2025:03:14 12:02:03 End Date: 2025:03:14 12:02:03

I'll try to do some similar tests with the daemon mode too, and socket if I have time (https://github.com/fwust/ddev-clamav looks like it'll make testing daemon mode pretty easy.. socket mode may involve a bit more manual set up).

There are quite a few linting errors between cspell / phpcs / phpstan.

A couple of them are introduced by the changes in the MR, but that's as a result of following the patterns / conventions that are already there e.g.

 32 | WARNING | Property name "$_output" should not be prefixed with an underscore to indicate visibility
    |         | (PSR2.Classes.PropertyDeclaration.Underscore)
 32 | ERROR   | Class property $_output should use lowerCamel naming without underscores
    |         | (Drupal.NamingConventions.ValidVariableName.LowerCamelName)

...where there are other properties like $_virus_name that hit the same problems.

It would probably make sense to address all of this in a dedicated issue, and just follow the existing patterns in this MR.

Rerolled the old patch against 2.1.x and put it into an MR.

There's a todo list in #3

I don't think the _file property is an issue any more - I've removed it as it wasn't being used for anything much.

I've not tested this manually (with any of the different configurations) yet though, and the output from the scanner should be sanitised.

Moving to NR for the testing, but we shouldn't commit this without the sanitisation.

mcdruid → changed the visibility of the branch 3178741-add-debug-mode to hidden.

Switching branches to create a new MR.

Oh, this was already fixed in https://git.drupalcode.org/project/ai/-/commit/ede64f611afb54e8d6919b3ff...

mcdruid → created an issue.

greggles → credited mcdruid → .

greggles → credited mcdruid → .

Thanks - you're right.. but I already applied the same changes :)

mcdruid → credited mcdruid → .

Thank you for the offer - very happy to accept it!

Adding credit from the private security issue.

Tests are passing.

Noting that this test is very similar to the one in 📌 Add tests for SA-CORE-2024-006 Active so anything we want to change here will likely also apply over there.

mcdruid → changed the visibility of the branch 3508240-add-tests-for to hidden.

This was only for 10.x

mcdruid → created an issue.

Adding credit from the private security issue.

mcdruid → created an issue.

Adding credit from the private security issue.

In general we'd use expectException in a test, but in this case we want to do some more checks after the exception is thrown.. so we have a try / catch instead.

I am in two minds about having this very specific assertion about the TypeError:

$this->assertStringContainsString('Cannot assign Drupal\Component\PhpStorage\FileStorage to property Drupal\Core\Config\StorageComparer::$targetCacheStorage', $e->getMessage());

That feels a bit like it might break in future, and we wouldn't necessarily care so long as the TypeError is still thrown.

I'll leave it in for now.

Tests are passing on the MR.

mcdruid → created an issue.

greggles → credited mcdruid → .

+1

v4 is the current version - so e.g. https://nvd.nist.gov/vuln-metrics/cvss/v4-calculator

I had a look at scoring a Drupal SA recently and it seems we'll have to get used to some new concepts if we're going to use v4 such as "Vulnerable System Impact" vs "Subsequent System Impact".

There are comprehensive docs:

https://www.first.org/cvss/user-guide#Assessment-Guide

I think perhaps we should aim to start doing a CVSS (v4) score in parallel to the existing system for a while, with the aim of switching over as soon as practical.

I think the problem is here:

https://git.drupalcode.org/project/securitydrupalorg/-/blob/7.x-1.x/feat...

  $handler->display->display_options['filters']['field_issue_status_value']['expose']['remember_roles'] = array(
    2 => '2',
    3 => 0,
    1 => 0,
    9 => 0,
    18 => 0,
    23 => 0,
    5 => 0,
    4 => 0,
  );

Not sure if the status values have changes since this view was exported to code, but looks like the default selected value of "2" doesn't match the db any more?

Do we need to change this to:

  $handler->display->display_options['filters']['field_issue_status_value']['expose']['remember_roles'] = array(
    'Open' => 'Open',
);

(Been a while since I looked at one of these - perhaps it needs to be numerical ids that are not reflected in the UI.. don't remember).

I'm not certain if the view has been overridden and therefore changes to this file wouldn't actually make any difference, so I won't create a patch/MR - I am happy to if that would help though.

mcdruid → created an issue.

mcdruid → credited mcdruid → .

mcdruid → credited mcdruid → .

The docs → currently say:

Project moderators will not add as owner, maintainer, or co-maintainer a person who cannot opt projects into security coverage

Only a project owner (/ existing maintainer) can add a user without the opt-in to security coverage attribute as a co-maintainer.

The docs also suggest that users interested in being added as a co-maintainer...

File an issue in the project's queue. Use Support request as the category and state your interest in becoming maintainer or co-maintainer. Adding links to issues for the relevant project in which you have added merge requests or patches or where you have reviewed merge requests or patches from other users will help demonstrate that you are actively involved in the project. Describe the motivation behind your request.

We could change the wording here a little to move from "should" to "must"; rather than "providing some examples of your contributions might help the application" we could mandate that such examples are provided, possibly specifying a threshold (e.g. minimum 5 examples).

We could also provide some counter-examples of "contributions" which might not be considered as valid e.g. +1's, screenshots of patches applying etc..

All of that's fairly soft in terms of being enforceable but it would at least mean that the cases of inadequately supported applications to be added as co-maintainers we've looked at here would be in clear contravention of the stated rules, which would justify action being taken to undo the applications.

It would also help to reinforce the arguments - put forward in previous comments in this issue - that contributions can and should be made without maintainership status of a project, in order to establish some credibility and track record before a user is considered as a suitable candidate to be added as a co-maintainer.

As for whether anyone has the "right" to undo an action such as granting a user co-maintainership of a project on d.o, I'd defer to the Drupal Association and their legal advisors, but the general gist of e.g. https://www.drupal.org/docs/develop/git/setting-up-git-for-drupal/drupal... → and the other Terms of Service published for d.o., suggests to me that it's quite likely that the DA would be permitted to take whatever action (on d.o and associated systems) is deemed necessary to maintain the security of the overall Drupal project and ecosystem.

I think it would be more disruptive to the users in question here to have their accounts suspended than it would for the co-maintainership to be revoked. It's a bit of a pyrrhic victory if the users remain as maintainers but cannot access their accounts.

Personally I'd support the removal of the role in these specific cases, on the basis that the spirit of the rules was not adequately followed, and that the documentation will be improved to clarify those rules to avoid a recurrence.

benjifisher → credited mcdruid → .

The only way seckit can do anything for static assets is if Drupal's private files (or a similar stream-wrapper) is used to serve those files through Drupal / PHP.

Otherwise Drupal doesn't get involved in serving the files at all - the webserver does it without "spinning up" a Drupal / PHP process.

In the majority of cases it's not worth the extra overhead of having Drupal/PHP serve your static files; the webserver is typically really good at doing that job efficiently.

I will close this as "works as designed" as there's nothing for the project to do; this is really down to the configuration of the application and the webserver.

Drupal apps will typically try to let a webserver (e.g. apache, nginx) handle requests for static files such as images, css files etc..

You can make PHP/Drupal handle those requests (e.g. with the private files functionality) but that's usually far less efficient than letting a webserver do the job it's designed for.

That means that Drupal often isn't involved in serving those static assets at all.

If you want to emit particular security headers for them, you'll often have to configure your webserver to do that - e.g. with .htaccess rules or whatever the equivalent is for your webserver of choice.

Some security headers are less relevant when serving static files anyway.

So I think this is "works as designed" unless I'm missing something?

benjifisher → credited mcdruid → .

Just to be clear, by "not yet verified" I don't (necessarily) mean git verified; I was thinking more about this "NEW" status:

I'm not sure how that's reflected in d.o's schema but looks like the proper term is "unconfirmed":

https://www.drupal.org/drupalorg/docs/user-accounts/become-a-confirmed-user →

Adding a user with that status as a maintainer seems fairly bonkers, to use a technical term.

If it's technically possible, we should disallow adding users that are not yet verified as a maintainer of a project.

greggles → credited mcdruid → .

poker10 → credited mcdruid → .

@ram4nd apologies for missing your comment.

If you can demonstrate an exploitable vulnerability that arises from e.g. having the old copy of jQuery in D7 core when it's not actually used by the application (which uses up-to-date jQuery libraries via jQuery Update), could you please report that in private to the Drupal Security Team? Thanks!

📌 [DRAFT] [META] Drupal 7 policy on jQuery related vulnerabilities Active is also relevant.

Okay to get all "existential" about it (what's the module actually for?)..

The reasons for stripping out all but the final 1.x and 2.x jQuery releases from the module and moving to the "custom paths" approach were explained in detail here:

• #3166985: [Proposal] provide supported / recommended jQuery versions for Security coverage →
• 🌱 Plan for jQuery Update 7.x-4.0 release Fixed

From fairly early on the module provided some CDN integration, and the previous maintainer had argued for a long time that it should focus only on providing integration with CDNs so that jQuery libs could be brought in that way - see #1869928: Better CDN/API/automation support → .

The problem was that nobody actually implemented that improved CDN functionality.

Given the time (and/or community contributions) I'd have happily committed changes to the module that allowed a slick CDN integration where e.g. the admin UI presented a choice of versions available from a choice of CDNs.

However, what we had to settle for instead was a basic implementation which is somewhat DIY for site maintainers - i.e. they have to provide the URLs of a CDN version, or to a local copy.

This certainly isn't fancy (although we did manage to add some functionality to advise site owners when new releases are available etc..) but it solved some of the fundamental problems that the module had whereby it was - at one point - a collection of outdated and insecure copies of the various jQuery libraries, which was for the most part unmaintained but nevertheless used by a large percentage of all D7 sites.

Going back down the path of committing copies of the libraries to the module seems like a big step backwards to me (for the reasons outlined), and I'd need to be convinced that there's a very good reason for doing so.

In the meantime, I'd welcome an MR that provides compatibility shims etc.. for more recent release of jQuery 3.x and other libraries.

There's also:

• ✨ jQuery Cookie 1.4.1 replace with Js-Cookie 3.0.5 and Shim Needs review - seems worthwhile, but has been waiting for (basic) tests for a long time.
• ✨ jQuery 4.0 Support Active

Sorry I think missed an important point in my previous comments.

It's not just that the module gets bloated and messy if it includes a full copy of every upstream release.

It also means that the module maintainers are expected to do a new module release for every (point) release that comes from upstream.

That's probably the most compelling reason to decouple the module from the upstream releases.

The module should handle the integration and any shims etc.. but when it comes to updating to a new upstream release sites should be able to do that for themselves.

(Another reason it makes some sense for the old 1.x and 2.x releases to be included in the module as we don't expect any new releases on those branches).

I'm going to set this to NW as I'm pretty convinced that we shouldn't add the actual JS libraries to the module.

I am of course happy to discuss it further though.

Personally I'd prefer to add the style tweaks / any other shims to the module, plus make any logic changes to e.g. add UI whenever jQuery is being added - if that is necessary..

..but leave the actual libraries out of the module.

The final releases of the 1.x and 2.x branches were retained for legacy / BC, but you can see all the old cruft that was removed in #3311834: remove unsupported JS libraries → .

I would really prefer not to add any more copies of the actual libraries to the module.

They will go out of date and the module will bloat (again) if a new copy has to be committed with every release.

Perhaps after EoL the logic changes, but one motivation for removing almost all of the copies of the jQuery libs from the module was that otherwise there's some expectation that if a copy is included in the module, it's somehow "Supported". Moving to the actual JS code being managed externally removes that; once a release is no longer supported by upstream it's not supported by this module either (sort of with the exception of those two final releases from the 1.x and 2.x branches).

Sorry I haven't had time to review the MR in detail.

Can you please explain why it doesn't work to use custom paths to update to the newest versions of:

* jQuery
* jQuery UI
* jQuery Migrate

?

One of the problems with committing all the JS for specific versions - e.g. jQuery 3.7.1 - is that it's likely to go out of date so quickly.

Do we then add a full copy of 3.7.2 when it's released (leaving 3.7.1 in place too)?

That's how we ended up with tens of megabytes of JS in the module before; most of it outdated.

Sorry, I haven't had time to look at the details properly, but do you need to apply a patch?

Can't you fix your problem with the two variables that control the line-endings?

modules/system/system.mail.inc:55:    $line_endings = variable_get('mail_line_endings', MAIL_LINE_ENDINGS);
modules/system/system.mail.inc:65:    $headers_line_endings = variable_get('mail_headers_line_endings', "\n");

longwave → credited mcdruid → .

xjm → credited mcdruid → .

xjm → credited mcdruid → .

Converted drupal-2783153-45.patch to an MR.

It does seem to cause one test to fail (not verified causation but seems like a strange coincidence if not):

JavaScript 160 passes, 1 fail, 1 exception, and 10 debug messages

---------------------
---- JavaScriptTestCase ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Warning    locale.inc        1527 _locale_parse_js_file()            
    file_get_contents(misc/drupal-settings-loader.js): Failed to open stream: No
    such file or directory
Fail      Other      common.test       1693 JavaScriptTestCase->testJavaScriptA
    When "javascript_always_use_jquery" is FALSE: The front page of the site
    does not include Drupal settings loader.

We looked at getting this one into 7.103 but didn't manage to do so.

I think this only makes sense if it makes absolutely no changes on a D7 site that does not enable the option.

The complete lack of JS testing in D7 makes it particularly risky to introduce something like this.

For example @poker10 spotted this straight away:

        'misc/drupal.js' => array(
          'data' => 'misc/drupal.js',
          'type' => 'file',
          'scope' => 'header',
          'group' => JS_LIBRARY,
          'every_page' => TRUE,
-         'weight' => -1,
+        'weight' => -2,
          'requires_jquery' => TRUE,

That would affect all sites whether they implemented the new option or not.

Perhaps that's no big deal, but with no automated testing it's hard to tell.. plus who knows what the consequences would be for real sites with lots of modules installed. Some may have carefully tweaked all of the relative weights.. possibly years ago.

My acceptance criteria for this would be that if the new option is not enabled, absolutely nothing changes.

mcdruid → made their first commit to this issue’s fork.

Sorry didn't mean to change status.

Thanks @greggles

For the Gadget Chains I'm not sure that CWE-502 is a perfect fit, although it's certainly very relevant.

Seems to me that one is tied to the initial vector whereby an application passes untrusted data to unserialize(), which is not what was addressed in these SAs.

https://cwe.mitre.org/data/definitions/915.html might be a better fit as that's more about the idea that it's possible for an attacker to modify properties of objects in an unintended way such that they can influence the state of the application (with malicious intent).

It seems like these two CWEs are both closely associated with PHP Object Injection, but I think 915 maps better to the Gadget Chains as they are not directly exploitable, but rather represent tools that an attacker can leverage if they find a way to pass their malicious payload to unserlialize().

I think CAPEC-586 looks good.

@paul_constantine so to be clear the problem you're having is the message on the admin status report?

This really should have been a warning rather than an error, and I missed that in review:

  // See if trusted hostnames have been configured, and warn the user if they   
  // are not set.                                                                   
  if ($phase == 'runtime') {                                                        
    $trusted_host_patterns = variable_get('trusted_host_patterns', array());    
    if (empty($trusted_host_patterns)) {                                            
      $requirements['trusted_host_patterns'] = array(                               
        'title' => $t('Trusted Host Settings'),                                     
        'value' => $t('Not enabled'),                                               
        'description' => $t('The trusted_host_patterns setting is not configured in settings.php. This can lead to security vulnerabilities. It is <strong>highly recommended</strong> that you configure this. See <a href="@url">Protecting against HTTP HOST Header attacks</a> for more information.', array('@url' => 'https://www.drupal.org/node/1992030')),
        'severity' => REQUIREMENT_ERROR,                                            
      );                                                                            
    }                                                                               
    else {                                                                          
      $requirements['trusted_host_patterns'] = array(                               
        'title' => $t('Trusted Host Settings'),                                     
        'value' => $t('Enabled'), 

...snip...

One workaround for this which comes to mind is to set a very permissive trusted host pattern such as:

$conf['trusted_host_patterns'] = array('.*');

So that should match anything.

It'd be better to set it up properly if possible.

I'm curious as to how the status report message would not go away if you did set a value for trusted_host_patterns in settings.php - and also how it would not break your site(s) quite badly if you set an example value which doesn't actually match the host / domain in use.

Could you please double check that?

In the meantime, we'll have to decide whether this warrants a follow up / hotfix release or something similar.

Apologies for the disruption; as mentioned I think a warning would have been more appropriate.

https://www.drupal.org/project/drupal/releases/7.103 → was released today.

That's it for the scheduled releases of D7.

Unfortunately we didn't manage to tick off every item on the todo list, but hopefully we're leaving D7 in pretty good shape.

Thank you to everyone that's contributed to D7 over ~14 years and more than a hundred releases.

mcdruid → credited mcdruid → .

mcdruid → credited mcdruid → .

Thanks everyone!

I think the new patch looks good.

Having yet another conf / variable is not ideal but it obviously avoids the situation where a change like this breaks sites with a certain mail setup and they're left with no choice but to hack core.

@poker10 and I looked at this to do some manual testing; it's a little tricky because MTAs / SMTP servers etc.. tend to be so accommodating of all sorts of weird stuff in email. For example mailpit (very useful tool included in ddev) has a relevant option - which defaults to false:

--smtp-strict-rfc-headers / MP_SMTP_STRICT_RFC_HEADERS

Force Mailpit to return an SMTP error if message headers contain \n instead to \r\n line breaks. By default Mailpit will silently fix incorrect line breaks generated by some broken sendmail clients (see related Github issue).

https://mailpit.axllent.org/docs/configuration/runtime-options/#smtp-server

However turning that on didn't seem to help us verify that anything was broken without the patch. Almost every way of actually testing mail that we tried (including with e.g. mimemail enabled or not) did not seem to show any problem without the patch on PHP8 (although I think we did reproduce html mail appearing a bit broken).

We eventually managed to observe the specific difference that the patch makes using strace to observe what PHP emits when Drupal's mail system sends a message. (FWIW we used https://www.drupal.org/project/drush_debug_tools → which includes a command to send test email from drush - it's old and dusty but still works).

So we verified that without the patch, on PHP 8.2, just a couple of the mail headers were separated by only \n. For example:

Errors-To: hello@example.com\nSender: hello@example.com\nFrom: hello@example.com

Whereas with the patch applied, this was:

Errors-To: hello@example.com\r\nSender: hello@example.com\r\nFrom: hello@example.com

It looks like - at least in the testing we were doing - those were the only headers being inserted by the code in question. All other headers were not influenced by these changes (and always had the correct line endings).

So I believe that verifies that this patch is having the desired effect.

If it has any unintended consequences with a particular mail set up, the new variable should allow for the change to be overridden without hacking core.

Sounds like it might make sense to add another variable; perhaps one we wouldn't add to default.settings.php if it's intended to accommodate sites with edge case mail setups.

Yeah, I think you're right that we've already addressed the problem described in the IS:

Warning: trim() expects parameter 1 to be string, array given in user_pass_validate() (line 69 of ***/modules/user/user.pages.inc).

It doesn't look like that could happen any more; core's checking is_scalar pretty early on in the form processing.

This was not the case when the (private) issue was originally filed.

I think we can close this as outdated, but thanks everyone that's worked on it.

#29 looks reasonable but I'd be happier committing this if we could verify that the comment above the changes to the constant is true:

	  * $conf['mail_line_endings'] will override this setting.
	  */

In other words, I'd like to ensure that sites can override these changes if they need to (e.g. because they use a quirky MTA or similar).

An earlier comment in this issue suggested that the $conf / variable does not in fact override the constant.

It'd also be good to see tests passing in an MR with the new d.o testing pipeline.

Great, thanks!

Leaving the "Needs change record" tag in place.

Yeah I'm not sure we need the replacement of the newline characters?

Along the lines of @alexpott's comment (which is now outdated for D10/11, IIUC) are there any other form fields we might want to add this to as well?

I'm thinking particularly of those that are typically exposed to anon users like the (rest of the) login form, perhaps search etc..?

I generally agree with the "core doesn't babysit bad code" approach, but this seems like a simple change which aligns the behaviour of the code with how the docs/comments say it behaves (..and avoids noise in logs when PHP is updated).

Thanks everyone!

One small problem - the test data uses short array syntax (D7 uses the old array() syntax).

If we can fix that, I'm happy to commit this.

I think this looks good, and would be an excellent thing to add to D7 (even this late in the day).

However it has to be an opt-in change.

Just sanity checking that the code only enforces the host pattern checking if the variable is set:

  // Check trusted HTTP Host headers to protect against header attacks.
  if (PHP_SAPI !== 'cli') {
    $host_patterns = variable_get('trusted_host_patterns', array());
    if (!empty($host_patterns)) {
      if (!drupal_check_trusted_hosts($_SERVER['HTTP_HOST'], $host_patterns)) {
        header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request');
...snip...

So if a site doesn't define any patterns, there's no change.

Site's can add patterns (e.g. in settings.php) and they will then start to be enforced.

If that's all true, +1 from me; I'll double check this and then am happy to commit.

Thanks everyone!

n.b. leaving the "Needs change record" tag in place.

Adding credit.

Thanks!

Thanks everyone!