🇬🇧🇪🇺
Account created on 9 March 2008, almost 17 years ago
  • Senior Principal, Security Operations at Acquia 
#

Merge Requests

More

Recent comments

🇬🇧United Kingdom 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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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?

🇬🇧United Kingdom 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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom 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!

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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:

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:

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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).

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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");
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom 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.
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Sorry didn't mean to change status.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

@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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

#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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Great, thanks!

Leaving the "Needs change record" tag in place.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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..?

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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!

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Thanks everyone!

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

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Adding credit.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Agree that this looks like the most sensible pragmatic solution.

Also LOL at the dry humour in https://gitlab.gnome.org/GNOME/libxml2/-/issues/474

Thanks!

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Ah okay, sorry I missed that.

How about CWE-835: Loop with Unreachable Exit Condition ('Infinite Loop')?

Not vastly different to 1050, but IIRC an infinite loop was pretty much the issue in 001, and it looks like 835 is "allowed" (if I'm looking at the right field).

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

SA-CORE-2024-002 was such a weird edge case that I don't know if we'll find perfect categories for it, but CWE-390 and CAPEC-165 both look like a pretty good match.

+1

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Thanks for all the work that's gone into this.

I think SA-CORE-2024-001 might map more closely to CWE-400 than CWE-405.

It looks like 405 relates to amplification (e.g. sending a small message to a DNS server which results in it sending a large response).

400 is about exhausting resources such as memory, which I think is more aligned with the core issue in this case.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Tests pass with mysql, sqlite, and postgres (including the new one).

Anyone happy to RTBC this on that basis?

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Added a new test that verifies user_requirements() doesn't incorrectly flag a couple of user accounts that both have their mail set to an empty string.

This passes locally for my with mysql.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

+1 to make the scope broad so that there's an option to assign CVEs for EoL versions if that makes sense in particular cases.

I think possibly we could make it clear somewhere that this is not an invitation to file security issues for ancient unmaintained branches, but that doesn't necessarily need to be included in docs on the CNA scope.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I've added the tests that the Security Team used when working on SA-CORE-2024-004; we'd typically wait a few weeks before committing tests that accompany a security fix, but there's no need to do so in this case.

We should add to these tests to verify the behaviour of user_requirements() when there are multiple users with a blank email.

We might want to account for the fact that could mean a couple of different things e.g. a truly NULL field in the db, or an empty string.

Plus we need to check that all 3 of core's db drivers behave themselves.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

The "further steps" were to review and commit the patch that was RTBC'd by several commenters :)

However, certainly appreciate what it's like trying to clean up a messy issue queue!

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Ideally we'd add tests to prove the fix.

However, looks like tests have not been committed from the private security issue yet. Perhaps we could do that early in this case (tests that accompany security fixes are often not added for a few weeks to avoid disclosing details of potential attacks, but that doesn't really seem to apply here).

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

This may be as simple as adding:

$query->isNotNull('mail');
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Thanks for reporting this.

https://git.drupalcode.org/project/drupal/-/blob/11.0.9/core/modules/use...

$query = \Drupal::database()->select('users_field_data');
$query->addExpression('LOWER(mail)', 'lower_mail');
$query->groupBy('lower_mail');
$query->having('COUNT(uid) > :matches', [':matches' => 1]);
$conflicts = $query->countQuery()->execute()->fetchField();

if ($conflicts > 0) {

..is how user_requirements() detects problems.

Looks like we should add a condition to exclude rows where mail is empty / null?

Anyone want to spin up an MR?

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I've checked the box so you should be able to edit maintainers now @manarth.

Thanks for your help @vladimiraus!

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

xjm credited mcdruid .

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

xjm credited mcdruid .

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

xjm credited mcdruid .

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I'm afraid this is definitely a Support Request rather than an obvious problem with the Drupal module.

The issue you're having could be quite specific to your environment.

Making sure StreamMaxLength isn't too low would be one of the first things I'd recommend you check, but looks like you've done that already.

If you really want to know what's happening, it might help to look at the clamd logs which might be at /var/log/clamav/clamav.log or perhaps going into syslog?

If that doesn't help, you can perhaps use tcpdump or similar to capture the network traffic between Drupal and clamd when a scan fails, in order to see if there are any clues there.

It would be good if the module did a better job of "surfacing" errors if that's possible; for example if the daemon responds with an error message.

Not something I can work on right now, but if there's not already an issue for that in the module's queue it'd be good to file one.

Have you made sure the module is in verbose mode to try to capture as much detail as you can about what's going on?

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

mcdruid created an issue.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Tested this with up-to-date 11.x and 10.3.x

I do see an error message at /core/authorize.php but the full path is not displayed e.g.

The website encountered an unexpected error. Try again later.
PDOException: SQLSTATE[HY000] [1044] Access denied for user 'db'@'%' to database 'some_invalid_db_name' in Drupal\Component\DependencyInjection\PhpArrayContainer->createService() (line 77 of core/lib/Drupal/Component/DependencyInjection/PhpArrayContainer.php).

update.php just shows a simple "The website encountered an unexpected error. Try again later." message.

I do not agree that this needs a CVE.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

bigpipe explicitly sets the secure and httponly attributes to false.

What's the justification for that to be changed?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#block_access_t...

The big_pipe_nojs cookie is just a boolean flag, the purpose of which is explained in \Drupal\big_pipe\Render\Placeholder\BigPipeStrategy

The HTTPonly and Secure attributes are intended to protect potentially sensitive information contained in cookies, and to prevent JS/client side code from being able to manipulate sensitive cookies.

None of that applies to this cookie, so it's legitimate for it not to have those attributes set.

Is the only reason to change them to prevent false positives from security scans?

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

@ram4nd I think you've pushed a couple of unrelated branches to the MR remote? No harm done, but just wanted to make sure you're aware :)

@poker10 thanks for the review, and yes I think the testing looks okay as it is (I've removed the tag).

I don't love the fallback:

    else {
      // As a fallback, make sure DRUPAL_ROOT's value is not in the path.
      $error['%file'] = str_replace(DRUPAL_ROOT, 'DRUPAL_ROOT', $error['%file']);
}

...but my thinking was that if the value of DRUPAL_ROOT appears somewhere other than at the very beginning of a file path for some reason, it'd be pretty confusing to just make that part disappear. I'm not sure when this would ever happen, but let's say for some reason the docroot path is reflected in a longer path to a network mount or something like that..

I think it's probably worth leaving it in, but I've made a little tweak to the conditional to avoid the fallback running at all when DRUPAL_ROOT is not present in the string in the first place.

A wise man once told me to avoid assignments inside if conditions, but I'd argue it's worth doing here to avoid doing unnecessary work for every error.

If you're happy with this little tweak @poker10 (and assuming tests all pass) I think it's ready to commit.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Looks like the test fail that @catch originally flagged up was under PostgreSQL:

SIMPLETEST_DB = pgsql://drupaltestbot:drupaltestbotpw@database/drupaltestbot?module=pgsql
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Not sure how helpful this will be, but debugging MySQL and PostgreSQL side by side, this seems to be where it goes wrong for postgres:

\Drupal\Core\TypedData\DataReferenceDefinition::getTargetDefinition successfully finds a targetDefinition in MySQL but that property doesn't seem to be set in Postgres AFAICS.

I'm not sure why.

The stack is:

DataReferenceDefinition.php:49, Drupal\Core\TypedData\DataReferenceDefinition->getTargetDefinition()
EntityReference.php:52, Drupal\Core\Entity\Plugin\DataType\EntityReference->getTargetDefinition()
EntityReference.php:73, Drupal\Core\Entity\Plugin\DataType\EntityReference->getTarget()
DataReferenceBase.php:37, Drupal\Core\TypedData\DataReferenceBase->getValue()
FieldItemBase.php:154, Drupal\Core\Field\FieldItemBase->__get()
FieldItemList.php:116, Drupal\Core\Field\FieldItemList->__get()
Comment.php:356, Drupal\comment\Entity\Comment->getCommentedEntity()
CommentAccessControlHandler.php:36, Drupal\comment\CommentAccessControlHandler->checkAccess()
EntityAccessControlHandler.php:109, Drupal\Core\Entity\EntityAccessControlHandler->access()
ContentEntityBase.php:738, Drupal\Core\Entity\ContentEntityBase->access()
EntityAccessCheck.php:68, Drupal\Core\Entity\EntityAccessCheck->access()
AccessManager.php:160, call_user_func_array:{/var/www/html/core/lib/Drupal/Core/Access/AccessManager.php:160}()
AccessManager.php:160, Drupal\Core\Access\AccessManager->performCheck()
AccessManager.php:136, Drupal\Core\Access\AccessManager->check()
AccessManager.php:93, Drupal\Core\Access\AccessManager->checkNamedRoute()
Url.php:813, Drupal\Core\Url->access()
StringFormatter.php:132, Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter->viewElements()
FormatterBase.php:91, Drupal\Core\Field\FormatterBase->view()
EntityViewDisplay.php:268, Drupal\Core\Entity\Entity\EntityViewDisplay->buildMultiple()
EntityFieldRenderer.php:257, Drupal\views\Entity\Render\EntityFieldRenderer->buildFields()
EntityFieldRenderer.php:143, Drupal\views\Entity\Render\EntityFieldRenderer->render()
EntityField.php:869, Drupal\views\Plugin\views\field\EntityField->getItems()
FieldPluginBase.php:1195, Drupal\views\Plugin\views\field\FieldPluginBase->advancedRender()
views.theme.inc:231, template_preprocess_views_view_field()
ThemeManager.php:261, call_user_func_array:{/var/www/html/core/lib/Drupal/Core/Theme/ThemeManager.php:261}()
ThemeManager.php:261, Drupal\Core\Theme\ThemeManager->render()
Renderer.php:446, Drupal\Core\Render\Renderer->doRender()
Renderer.php:203, Drupal\Core\Render\Renderer->render()
FieldPluginBase.php:1796, Drupal\views\Plugin\views\field\FieldPluginBase->theme()
StylePluginBase.php:769, Drupal\views\Plugin\views\style\StylePluginBase->elementPreRenderRow()
DoTrustedCallbackTrait.php:107, call_user_func_array:{/var/www/html/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php:107}()
DoTrustedCallbackTrait.php:107, Drupal\Core\Render\Renderer->doTrustedCallback()
Renderer.php:825, Drupal\Core\Render\Renderer->doCallback()
Renderer.php:387, Drupal\Core\Render\Renderer->doRender()
Renderer.php:203, Drupal\Core\Render\Renderer->render()
Renderer.php:120, Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure:/var/www/html/core/lib/Drupal/Core/Render/Renderer.php:119-121}()
Renderer.php:593, Drupal\Core\Render\Renderer->executeInRenderContext()
Renderer.php:119, Drupal\Core\Render\Renderer->renderInIsolation()
StylePluginBase.php:711, Drupal\views\Plugin\views\style\StylePluginBase->renderFields()
StylePluginBase.php:574, Drupal\views\Plugin\views\style\StylePluginBase->renderGrouping()
StylePluginBase.php:462, Drupal\views\Plugin\views\style\StylePluginBase->render()
DisplayPluginBase.php:2177, Drupal\views\Plugin\views\display\DisplayPluginBase->render()
ViewExecutable.php:1592, Drupal\views\ViewExecutable->render()
DisplayPluginBase.php:2467, Drupal\views\Plugin\views\display\DisplayPluginBase->preview()
ViewExecutable.php:1721, Drupal\views\ViewExecutable->preview()
CommentUserNameTest.php:158, Drupal\Tests\comment\Kernel\Views\CommentUserNameTest->testUsername()
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

This one seems to fail consistently on PostgreSQL as far as I can see; I've reproduced the failure manually with postgres:16 (in ddev).

mcdruid@drupal-10:/var/www/html$ git status
Refresh index: 100% (18217/18217), done.
On branch 11.x
Your branch is up to date with 'origin/11.x'.

mcdruid@drupal-10:/var/www/html$ vendor/phpunit/phpunit/phpunit -c core core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php
PHPUnit 10.5.35 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml.pgsql

E                                                                   1 / 1 (100%)

Time: 00:02.411, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\comment\Kernel\Views\CommentUserNameTest::testUsername
Error: Call to a member function access() on null

...snip...
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

AFAICS doing composer self-update 2.8.0 looks like it should be a good workaround until this is resolved upstream.

...that is assuming composer does revert the change. If not, we may have to make more significant changes.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I think another test is also affected:

---- Drupal\Tests\Composer\Plugin\Scaffold\Functional\ComposerHookTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-1.xml        0 Drupal\Tests\Composer\Plugin\Scaffo
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/security/drupal-security/core/phpunit.xml.dist
    
    E..                                                                 3 / 3
    (100%)
    
    Time: 00:03.088, Memory: 8.00 MB
    
    There was 1 error:
    
    1)
    Drupal\Tests\Composer\Plugin\Scaffold\Functional\ComposerHookTest::testComposerHooks
    RuntimeException: Exit code: 1
    
    Creating a "fixtures/drupal-drupal" project at "./create-project-test"
    Installing fixtures/drupal-drupal (1.0.0)
    
    In PathDownloader.php line 51:
                                                                               
       
      Source path "./drupal-drupal" is not found for package
    fixtures/drupal-drupal                                                                       
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Cool, thanks - I've posted in #drupal-infrastructure in slack as I wasn't sure if it was coincidence that the composer version was bumped here..

Thanks for filing an issue in the right place.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I think this may have broken some tests?

See e.g. https://git.drupalcode.org/issue/drupal-3479411/-/jobs/2990908

Drupal\BuildTests\Composer\Template\ComposerProjectTemplates   0 passes   17s   1 fails   <===                         
Drupal\BuildTests\Composer\Component\ComponentsTaggedRelease   4 passes   41s                                      
Drupal\BuildTests\Command\GenerateThemeTest                   16 passes   88s                                      
Drupal\BuildTests\Composer\Component\ComponentsIsolatedBuild  24 passes  134s                                      
Test run duration: 2 min 13 sec
Detailed test results
---------------------
---- Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-1.xml        0 Drupal\BuildTests\Composer\Template
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/issue/drupal-3479411/core/phpunit.xml.dist
    
    ..FF                                                                4 / 4
    (100%)
    
    Time: 00:16.442, Memory: 14.00 MB
    
    There were 2 failures:
    
    1)
    Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest::testTemplateCreateProject
    with data set "recommended-project" ('drupal/recommended-project',
    'composer/Template/RecommendedProject', '/web')
    COMMAND:
    COMPOSER_HOME=/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home
    COMPOSER_ROOT_VERSION=11.0 composer create-project --no-ansi
    drupal/recommended-project test_project 11.0 -vvv --repository
    /tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/test_repository/packages.json
    OUTPUT: 
    ERROR: Running 2.8.1 (2024-10-04 11:31:01) with PHP 8.3.12 on Linux /
    5.10.225-213.878.amzn2.x86_64
    Reading ./composer.json
    (/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer.json)
    Loading config file
    /tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home/config.json
    Loading config file
    /tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home/auth.json
    Loading config file ./composer.json
    (/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer.json)

    ...snip...

/tmp/build_workspace_142b3cf5f42e57cba5237822b73defbcu4lXMx/composer-home/cache/repo/file----tmp-build-workspace-142b3cf5f42e57cba5237822b73defbcu4lXMx-test-repository-packages.json/packages.json
into cache
Installing drupal/recommended-project (11.0)
In PathDownloader.php line 51:
                                                                           
   
  [RuntimeException]                                                       
   
  Source path "composer/Template/RecommendedProject" is not found for
package  
   drupal/recommended-project

I can reproduce this by e.g. replacing ddev's current composer (2.7.9) with the phar for 2.8.1 then trying to run just that one test manually:

mcdruid@drupal-10:/var/www/html$ vendor/phpunit/phpunit/phpunit -c core core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php
PHPUnit 10.5.29 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.10
Configuration: /var/www/html/core/phpunit.xml.mysql

..FF                                                                4 / 4 (100%)

Time: 00:08.780, Memory: 14.00 MB

There was 1 PHPUnit test runner deprecation:

1) Your XML configuration validates against a deprecated schema. Migrate your XML configuration using "--migrate-configuration"!

--

There were 2 failures:

1) Drupal\BuildTests\Composer\Template\ComposerProjectTemplatesTest::testTemplateCreateProject with data set "recommended-project" ('drupal/recommended-project', 'composer/Template/RecommendedProject', '/web')
COMMAND: COMPOSER_HOME=/tmp/build_workspace_558dd1b0a4860999e23c68f3b3390b60dlYMeC/composer-home COMPOSER_ROOT_VERSION=11.0.6 composer create-project --no-ansi drupal/recommended-project test_project 11.0.6 -vvv --repository /tmp/build_workspace_558dd1b0a4860999e23c68f3b3390b60dlYMeC/test_repository/packages.json
OUTPUT: 
ERROR: Running 2.8.1 (2024-10-04 11:31:01) with PHP 8.3.10 on Linux / 6.8.0-40-generic

...snip...

Installing drupal/recommended-project (11.0.6)

In PathDownloader.php line 51:
                                                                                                          
  [RuntimeException]                                                                                      
  Source path "composer/Template/RecommendedProject" is not found for package drupal/recommended-project

...snip...

Installing drupal/legacy-project (11.0.6)

In PathDownloader.php line 51:
                                                                                                
  [RuntimeException]                                                                            
  Source path "composer/Template/LegacyProject" is not found for package drupal/legacy-project

(sorry the test output is quite verbose and it's hard to know exactly what might be significant)

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

It might not be easy to test the exact scenario from the IS, but I'd think that we should be able to add a basic test that verifies this is working as expected when an error is displayed..

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Thanks, new branch / MR which adds a helper function to strip DRUPAL_ROOT from errors that will be displayed.

I'm not sure if replacing the actual path with the text DRUPAL_ROOT is less confusing than just removing it completely, but it could be a bit misleading if e.g. instead of:

/var/www/html/sites/default/settings.php

..we output:

/sites/default/settings.php

I don't think we want to use something like /path/to/drupal which would probably also confuse some people as it looks real but is not.

Open to suggestions but as it stands this will show an error like:

No such file or directory in include_once() (line 848 of DRUPAL_ROOT/sites/default/settings.php).
🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

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

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I had a bit of time to tidy this up, but I'd still like to see some new tests added to verify that e.g. the checkbox / variable for the shim works as it's supposed to.

So leaving at NW + Needs tests for now.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

There are a few things to address in the comments, plus needs tests.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Great, thanks for suggesting this @mustanggb, and your help getting it done.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

Yeah, I hadn't noticed any problems with those booleans.

However, it does seem like we could make it all a bit simpler and reduce the requirement for mental contortion with those ternaries :)

I think we can do:

        'migrateMute' => !(bool) variable_get('jquery_update_jquery_migrate_warnings', FALSE),
        'migrateTrace' => (bool) variable_get('jquery_update_jquery_migrate_trace', FALSE),

It's good if we're future-proofing this against the conversion of Drupal.settings to JSON, so it was lucky you had the patch in place.

Thanks for the review.

🇬🇧United Kingdom mcdruid 🇬🇧🇪🇺

I've tweaked the MR as we're only adding a new .js file when we need to apply the settings for migrate (I had initially thought the file may apply several different settings).

This hopefully means it's less of a significant / global change, and we're able to stick with the very specific weighting etc.. for the file that's being added to $libraries.

However, in doing this I've had to add to the js file so that it's doing this:

var Drupal = Drupal || { "settings": {}, "behaviors": {}, "locale": {} };

...otherwise I was getting errors about Drupal not being defined, and then settings/behaviors/locale not being defined etc..

As far as I can see this is all working okay; I've tested manually with the jQuery Update defaults and checked with the admin and front-end theme with jQuery Migrate enabled.

My testing hasn't gone far beyond just checking for errors in the console and clicking around a bit to try to verify that nothing's obviously broken.

I would appreciate a review and/or some more manual testing before I commit this.

@mustanggb are you able to take a look please?

Production build 0.71.5 2024