Account created on 31 January 2012, over 13 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States cmlara

waiting for the update compatible with drupal 11.2

📌 Allow PHPStan ^2 Active is the current blocker, as the tests runs in this issue indicated it now identifies numerous errors (most legitimate however had previously been baseline ignored, and at least one bug in phpstan-drupal). It is only after working through a significant portion of the reported errors that it became apparent that some will not be 'dual fixable' and the discussions around that have been started.

There is talk that core may roll back its demand for ^2 however that, has of yet, not occurred. End of the day we want/need to know we can expect to have clean testing runs before we formally release support for ^11.2.

I can suggest if site owners want to help avoid this in the future a few items they can consider:
Encourage Core to make only bugfix changes after Beta 1.
Encourage Core to ensure major changes are committed to the repo earlier in the development cycle.
Encourage Core to extend the Beta/RC window lengths to allow contirb more time to update before the production release is made available.
Encourage Core to withhold releases when there is any doubt expressed about the suitability of the release (the PHPStan issues were identified in Slack with a Release Manager however the 11.2.0 release was pushed ahead as it was a week behind schedule).
Encourage Core to make more of its API covered by BC Policy (the primary reason we list 'by minor' rather than 'by major')

With the above said, we are currently only 7 days into the lifecycle of a release that has 1 year until its EOL and have ~6 months until 11.1 goes EOL. Would I prefer this was released the same day (or sooner) than Core, yes, however if core makes last minute major decisiosn that impact contrib there is little to nothing we can do except work through it.

The irony of this issue I actually looked into adding add PHPStan ^2 support in November however Drupal Core's requirements prevented it at the time and it appeared we would be good until ^12 based on discussions with other core devs.

🇺🇸United States cmlara

Considering the above document is not a policy is there anything actually stopping the Project Application Queue from testing the proposed process for 3-6 months and seeing how it works?

🇺🇸United States cmlara

There is discussion on Slack that 🐛 Can't update to 11.2.0 Active may be re-purposed to include rolling back 📌 Update PHPStan to 2.1.17 Active as part of a hotfix release.

🇺🇸United States cmlara

@dimitriskr

You may want to comment on 🌱 More flexible language for git vetted status for co-maintainers of existing projects Active . This application came up in relationship to that issues today.

I'm not sure where the question regarding are these specific documents are policy have been answered unless @avpaderno is referring to discussion that other documents (Project Adoption) which has been stated are not policy without firm statements regarding other documents on D.O. Perhaps the intention is to mean that no documents on D.O. are policy (in which case how can they be used as evidence to refuse a request?).

I will note the use of "Privileged User" was intentionally chosen to identify users with enhanced privileges not available to the average user, and to align with the D.O. Code of Conduct to not question the motives of specific individuals, instead questioning the process and its implementation. @avpaderno was previously made aware of the terms NIST definition on Slack.

🇺🇸United States cmlara

@avpaderno For those of us who are not D.O. Privileged Users, since there has been discussion on other pages with the same concern, can you confirm which of the links in #10 are policy documents (if any) vs just general documentation?

@avpaderno I believe the point of #7 and #9 is that the D.O. Privileged Users are required by the Drupal Code of Conduct (regarding collaborative and abusive behavior) to apply gates equally to all users.

In the linked issue the D.O. Privileged User repeatedly refused to agree that the code did not demonstrate their individual skill, non-privileged user thus must assume that such little edits is acceptable and that they can expect similar treatment on other issues.

Note: I have not read the code in full, I have simply skimmed the related MR which appears to contain a similar level of work as the linked issue that was repeatedly considered 'acceptable'

🇺🇸United States cmlara

My apologies, I had thought I had submitted my response on this issue previously.

Are you interested in contacting the upstream maintainer there

My entire knowledge of this reported vulnerability is based off the vulnogram, ideally someone who knows more or has been involved in working on the repair to the Webform Multi File Upload module would do so as they would likely be better informed.

it seems we could issue a CVE if the upstream maintainer is not interested in doing that

.

Possibly there are other parts of the CNA rules to consider as well:

4.2.1.1 The CNA with the most appropriate scope MUST be contacted and given the first opportunity to assign.

4.2.1.2 also comes into play that if the most appropriate scope refuses the Root CNA must make a determination and direct a CNA to issue.

then an appropriate Root MUST make a Vulnerability determination. If the Root determines that one or more Vulnerabilities exist, the Root MUST direct a CNA-LR or another CNA with appropriate scope to assign as quickly as possible and no later than 72 hours after becoming aware of the first refusal.

Question is, who is most appropriate scope?

4.2.2.1.3 scope determination is relevant to review.
Drupal.org CNA scope is “All projects hosted under drupal.org, including End of Life (EOL) code.”

What is the interpretation of hosted? Is it being the official development platform, or is redistributing also hosting?
How integrated is the code in the Drupal module (is it line for line, or is it a fork that deviated some time in the past and could legitimately called its own project)?
Is there another CNA with better scope?

I’ll note the goal behind the process is that for any single vulnerability there is only a single CVE to refer to it no matter who re-distributes the code and that any program that contains the flaw references that CVE. All of the above questions are above my current knowledge on the module and how the code was distributed.

🇺🇸United States cmlara

I should note Drupal may not technically be able to answer "yes" to "Publicly known vulnerabilities fixed"

Drupal Core likely breached the timeline with CVE-2024-45440 which took untiill at least Decmeber 4th to commit to the D7 repo.

There is still an open second part in 🐛 Maintenance Pages leak sensitive environment information - pt2 Active .

If we ignore that for a moment, we should also consider that there are a large number of issues that are public that may qualify for a CVE if escalate through the CNA dispute process. At the moment they would not technically disqualify Drupal, however is it in the best interest of the project to assert a strong security focus if it has not resolved those open issues?

🇺🇸United States cmlara

@dimitriskr

You may want to look at 📌 [1.0.0] Accessibility tools Needs review to see if it can provide you any solid points of argument. That was an application where a project forked a 3rd party and made minimal changes to itself. It may provide you the evidence you need that you have made 'sufficient' changes to apply..

🇺🇸United States cmlara

Call to method Drupal\Core\Entity\Query\QueryInterface::accessCheck() with false will always evaluate to true.
is related to https://github.com/mglaman/phpstan-drupal/issues/825

🇺🇸United States cmlara

to make a check against the database to see if any images are using the wrapper in the files_managed table.

Image styles may be generated for un-managed files as well (albeit not common), I don't see the files_managed table as a viable solution.

We should definitely see if we can at least warn users. I just don't know where/how that could obe done

We have had this subject come up for other issues in the past. Documentation has to start somewhere, this issue is as good as any other to do so, stick it somewhere in the README, or some "deployment guide" for Site Owners to note that '/styles/' is a reserved path on all streamWrappers.

Potentially a feature to be able to select where images styles may be located could be a solution.

Split ImageStyle into the config entity and a separate event-based image processing service Needs work would be the best for this, it would allow solutions for numerous concerns. The new FlushEvent it proposes likely could allow this to be handled per streamWrapper by later changing the streamWrapper spec to require each streamWrapper provider listen and perform the flush as needed.

🇺🇸United States cmlara

cmlara created an issue.

🇺🇸United States cmlara

The relevant config is controlled by the Core team not infra.

Noted in Slack this sounds exactly like the cspell issue previously encountered in 🐛 Autoresolve cspell GIT_DEPTH issue when diff fails Fixed .

🇺🇸United States cmlara

The --audit flag is enabled for every package manager operation - this includes unattended automatic updates which could be run via a cli cron job.

I was reading #4 and following as suggesting that audits NOT be a part of the package manager operation (wasn't aware flag explicitly was there vs the implicit composer use of it by default).

so "silently allowing a 'known vulnerable' package to be installed" is the status quo.

Understood on that being current and IMO bug on its own that it should be handled rather than silently allowed.

I think a new issue to add an explicit composer audit and show the results would do a lot more here.

There can be some value in this. Assuming that an audit call is added as part of a transaction that prevents a vulnerable package from being installed beyond just a 'sandbox' and transferred to 'production' that would likely be acceptable.

I would suggest that should be done before passing the audit ignore flag which may cause a successful return from Composer (in other words, postpone this issue).

Note: I'm not familiar with Project Browsers internals. I see comments such as 'sandbox' above and assume its doing a standard A/B on disk with a staged swap over only after passing basic checks such as composer success.

🇺🇸United States cmlara

Lets keep this one as the common issue/meta, especially since It is already being linked to.

As I mentioned I've went ahead and changed scope a bit.

We should avoid the scope creep, Changing the testing environment (not required to test this, see https://git.drupalcode.org/issue/s3fs-3528246/-/pipelines where manual runs were used to determine the information above)(this was a key feature of how gitlab_templates was designed to replicate older drupal_ci, although I admit many developers have never learned about it) and adding support for PHPStan 2.0 are indeed both likely reserved to their own issues. (I haven't even looked at the 2.x problem yet to see how it impacts our ability to run against ^10 and ^11)

Smaller the changeset the quicker I can approve and release a targeted 10.5 instead of having to review everything related to ^11.2 before push.

I've simplified version constraints

Did have simplifying slated into this issue, although I will note I saw <10.6 and thought "wait 10.6 doesn't exist yet" before noticing the lack of an = sign. Technically its as equivalent to what was initially proposed in the MR to simplify, I'm not sure which does a better job conveying the logic, open to community feedback.

PS I would suggest dropping support of D8 and D9 altogether, as well as PHP7 .

Discussed elsewhere, short version my opinion is that (under semver) these all require a major release. Technically we could get away with it as we are not semver yet however there is no need for us to not treat 8.x-3.x as equivalent to 3.x.0 as much as possible. I absolutely do NOT suggest running it on these old of versions unless you are backporting security critical changes (and I still suggest upgrading). I mentioned elsewhere as it relates do drupal/core we are writing to an API spec (which never really goes EOL) not specific versions of Core (which do go EOL).

It's not being tested anyway, so is it still working ?

While we may not be testing it on the weekly test run (gitlab_templates has been poor on maintaining BC leading to fears of adding it) I do run manual tests back as far as PHP 7.4 Drupal:^9.5 periodically, especially when major code changes take place.

drupal-gitlab-local --variable='_TARGET_CORE=^9.5' --variable='PHP_VERSION=7.4' --variable='PHP_IMAGE_VARIANT=apache' composer phpunit (this should be re-producible on GitLabCi with the same config)

OK (88 tests, 600 assertions)

Generating code coverage report in Cobertura XML format ... done [00:00.771]


Code Coverage Report:        
  2025-06-20 02:57:19        
                             
 Summary:                    
  Classes: 17.95% (7/39)     
  Methods: 37.87% (89/235)   
  Lines:   49.17% (2078/4226)

Eventually I will get back to work on 📌 Convert to Quasar Components Active (last I worked on it I had testing working back as far as ^8.9). Ideally this project would have proper MIN/MAX testing. We have accidentally breached our support as discussed in 📌 PHP7.0 support Active and we are waiting on drupal/coder to allow us to implement 📌 [PP-1] Create Gitlab job to test Minimum PHP Language syntax Postponed so our testing right now is indeed not perfect.

🇺🇸United States cmlara

I want to re-afirm @tedbow comments from #11 Generally with testing you want to test "worst case" in this case that means Audit On as it will likely be enabled on sites.

could exit with exit code 1, even if the install was succesful. In a programmatic way, this will for sure create confusion

Arguably an error has occurred and package_manager does need to deal with it.

I would imagine we will never show the results of the audit directly in the composer UI, it would be an explicit operation,

Also I think we should skip the audit operation in regular package_manager operations too -

I can understand not displaying, however silently allowing a 'known vulnerable' package to be installed? To my knowledge php-tuf will not be replacement for composer audit(security advisory parsing).

Large commercial sites are much less likely to use Package Manager, they are going to run controlled labs with dev environments. Tooling like this is really an advantage for the SMB/Home User market. Both of these are signifcantly more likely to run in production. Home users have much less concern, the SMB market however, their insurance agent will look for any reason to reject a claim, intentionally ignoring/not processing AUDIT results may make a very good reason to reject.

Even just having the code on the server may be enough for an auditor to flag a concern. From an audit standpoint it would likely be better to encourage users to set the COMPOSER_NO_AUDIT environment variable through settings.php to disable processing so that it does not impact more secure sites and is not enabled by default.

Is #18 a formal Security Review or should this issue be flagged for that as well?

🇺🇸United States cmlara

https://www.drupal.org/project/drupal/issues/3526142#comment-16151180 📌 Update PHPStan to 2.1.17 Active is also likely to impact D11.2 tests runs as we are configed

The issues in #4 for 10.5 were likely due to an out of date origin locally (missing the deprecation silence fixes).

📌 Update PHPStan to 2.1.17 Active may add some complications for 11.2 as core is now forcing PHPStan v2 which is causing us to download mismatched dependencies. Once core releases 11.2.1 I expect composer failures to block all testing.

  - Locking drupal/core (11.2.0)
  - Locking drupal/core-composer-scaffold (11.2.0)
  - Locking drupal/core-dev (11.2.0-beta1)

Considering 10.5 was released a few days ago and we would be 'rushing' 11.2 if we forced it out today we might consider separating D10.5 into its own issue and have 11.2 be a followup a (hopefully) a few days later..

🇺🇸United States cmlara

@kim.pepper Thanks for the quick review. Was typing up the following while your review came in:

This was actually not as embedded in core as I expected it to be.

Set deprecation for 11.3 removal in 13.0 based on (the not yet accepted) discussion in 📌 Defer disruptive 11.3 deprecations for removal until 13.0 Active out of concern it might be considered 'disruptive' (removing a method).

As an argument it is not disruptive: basename()is PHP native and works in PHP8.1 which is the minimum supported version for any D10.x release. A module could easily support ^10 || ^11 || ^12 with just the native function.

Above my position to make the final call on D12 vs D13 (willing to edit to D12 if it is approved).

This does impact a deprecation added in 🐛 Separate MIME type mapping from ExtensionMimeTypeGuesser Needs work the file_system was added to the ExtensionMimeTypeGuesser as optional in D11.2 and would have been required in D12 will now be removed in D11.3.

🇺🇸United States cmlara

The Password Reset tests were handled in [##3392427] which was the primary hold on this issue for 2.x.

Manual rebase on head, with corrections for accidental rollback.

🇺🇸United States cmlara

Until https://github.com/php-tuf/composer-integration/issues/128 is resolved, I have basically no capability to help with issues like this.

Given we had issues in #30 is there any background process running to routinely verify the integrity of the repository (and alert when integrity is breached) that could identify these issues prior to a report from a user and without need to resolve the issue in TUF?

🇺🇸United States cmlara

If I am not missing something to discuss, shall we close this at WAD?

The original request was to formally adopt a standard. That would mean this would still need a coding standards vote to adopt and update https://project.pages.drupalcode.org/coding_standards/ with new text. That would not be WAD, if nothing is to be formally adopted it would be "won't fix" which would be saying "each project may write markdown files however they want"

Various projects, including the Drupal governance and this project are moving documentation to GitLab pages.

I will note the coding standards and Drupal governance project pages are not using GFLM, since the pages are generated by mkdocs they must align to python markdown, which is is different from and in some cases incompatible with, GFLM. 🐛 Formatting of PUWG charter mkdocs output is wrong Active is a real world example where parsing differences between GFLM and Python Markdown.

In #10 and #23 i mentioned that
Adopting GLFM is equivalent to saying "The coding standard for PHP files is the PHP engine specification", nothing about unified writing style has been said other than "the parser must not error'.

Here are a few questions that are related to standardized writing of markdown from the first couple pages of the GFLM specifications, each is a decision to make, each choice is valid, and each option can be mixed in the same file (and in some cases even the same line) and still be valid GFLM:

  • Underline or Hash style heading indicators
  • Max line lengths (do we keep the old PHP 80 char limit or do we embrace unlimited line lengths which better align to markdown usage)
  • Double underscores or double asterisk for bold
  • Hyphens, asterisks, or underscores for horizontal rules
  • Should ordered lists use the 'repeating same digit' method of numbering or should they manually number each item (so that it is readable in plaintext too)

This is just a fraction of the choices that would need to be made to unify the writing of markdown files under a common style.

🇺🇸United States cmlara

Would be good to have a subsystem maintainer sign-off before any effort is put into code on this.

Pinging @kimb0 (Kim Pepper) on Slack.

🇺🇸United States cmlara

Digging a bit more into the core side history of this, there there is/was apparently a locales issues.

https://bugs.php.net/bug.php?id=37738

It does appear this issue is no longer present in modern PHP versions, although it does exist for the entire PHP7 line.
https://3v4l.org/XcflK.

Perhaps we will want to copy the

FileSystem::basename()

as PHP7.1 is listed for S3FS supported environments.

🇺🇸United States cmlara

Added notes to the file comment referring to core and this issue.

As a deeper summary:

The history on s3fs having initialized its own copy of the MimeTypeExtensionGuesser can be traced back to #3259065: Circular reference detected for service "s3fsfileservice" with mime_type guesser . We were avoiding a circular reference after having added s3fsfileservice which decorates the core file_system service.

Some (many?) MimeType guessers make use of the FileSystemInterface::basename() method. In the past this has been via a global container call \Drupal::service('file_system') in-line, however as part of proper service injection these calls are slowly being removed.

The s3fsfileservice is somewhat unique with the fact it needs to know the mime type of a file as part of the Move/Copy operations to correctly set the headers from the S3 Bucket.

FileSystemInterface::basename() is unfortunately just in a poor location for this scenario.

To solve this our copy will use the native basename() function. FileSystem::basename() appears functionally identical to the PHP basename(). It would make no sense for a streamWrapper to be implemented in a manner where basename() does not follow the native format as such streamWrappers would be incompatible with normal PHP scripts. The entire purpose of streamWrappers is to provide program agnostic storage.

Ideally as a followup we would encourage core to deprecate/remove FileSystemInterface::basename() to allow us to return to operating with the Core instance of mime type guessing services.

🇺🇸United States cmlara

The error as described was never able to be re-produced.

🇺🇸United States cmlara

As noted in #30 I suggest this be a contrib module if desired.

🇺🇸United States cmlara

Testing against Drupal CMS fails.

mkdir -p /tmp/dcms
cd /tmp/dcms
/usr/bin/time composer create-project drupal/cms
cd cms
/usr/bin/time composer -vv update
composer config allow-plugins.php-tuf/composer-integration true
composer require php-tuf/composer-integration:dev-main --dev
mkdir tuf
curl -o tuf/packages.drupal.org.json https://packages.drupal.org/8/metadata/1.root.json
curl -o tuf/packagist-signed.drupalcode.org.json https://packagist-signed.drupalcode.org/metadata/1.root.json
composer tuf:protect https://packages.drupal.org/8
composer config repositories.drupal-core composer https://packagist-signed.drupalcode.org
composer tuf:protect https://packagist-signed.drupalcode.org
composer -vv update
> pre-file-download: Tuf\ComposerIntegration\Plugin->preFileDownload

In RootVerifier.php line 36:
                                                                          
  [Tuf\Exception\Attack\RollbackAttackException]                          
  Remote 'root' metadata version "$1" does not the expected version "$2"  
                                                                          

Exception trace:
  at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Metadata/Verifier/RootVerifier.php:36
 Tuf\Metadata\Verifier\RootVerifier->checkRollbackAttack() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Metadata/Verifier/RootVerifier.php:25
 Tuf\Metadata\Verifier\RootVerifier->verify() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Metadata/Verifier/UniversalVerifier.php:59
 Tuf\Metadata\Verifier\UniversalVerifier->verify() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Client/Updater.php:231
 Tuf\Client\Updater->updateRoot() at /tmp/dcms/cms/vendor/php-tuf/php-tuf/src/Client/Updater.php:152
 Tuf\Client\Updater->refresh() at /tmp/dcms/cms/vendor/php-tuf/composer-integration/src/ComposerCompatibleUpdater.php:51
 Tuf\ComposerIntegration\ComposerCompatibleUpdater->getLength() at /tmp/dcms/cms/vendor/php-tuf/composer-integration/src/TufValidatedComposerRepository.php:220
 Tuf\ComposerIntegration\TufValidatedComposerRepository->prepareComposerMetadata() at /tmp/dcms/cms/vendor/php-tuf/composer-integration/src/Plugin.php:55
 Tuf\ComposerIntegration\Plugin->preFileDownload() at /var/www/html/vendor/composer/composer/src/Composer/EventDispatcher/EventDispatcher.php:220
 Composer\EventDispatcher\EventDispatcher->doDispatch() at /var/www/html/vendor/composer/composer/src/Composer/EventDispatcher/EventDispatcher.php:115
 Composer\EventDispatcher\EventDispatcher->dispatch() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:1653
 Composer\Repository\ComposerRepository->asyncFetchFile() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:1111
 Composer\Repository\ComposerRepository->startCachedAsyncDownload() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:1039
 Composer\Repository\ComposerRepository->loadAsyncPackages() at /var/www/html/vendor/composer/composer/src/Composer/Repository/ComposerRepository.php:540
 Composer\Repository\ComposerRepository->loadPackages() at /var/www/html/vendor/composer/composer/src/Composer/DependencyResolver/PoolBuilder.php:429
 Composer\DependencyResolver\PoolBuilder->loadPackagesMarkedForLoading() at /var/www/html/vendor/composer/composer/src/Composer/DependencyResolver/PoolBuilder.php:279
 Composer\DependencyResolver\PoolBuilder->buildPool() at /var/www/html/vendor/composer/composer/src/Composer/Repository/RepositorySet.php:332
 Composer\Repository\RepositorySet->createPool() at /var/www/html/vendor/composer/composer/src/Composer/Installer.php:501
 Composer\Installer->doUpdate() at /var/www/html/vendor/composer/composer/src/Composer/Installer.php:298
 Composer\Installer->run() at /var/www/html/vendor/composer/composer/src/Composer/Command/UpdateCommand.php:281
 Composer\Command\UpdateCommand->execute() at /var/www/html/vendor/symfony/console/Command/Command.php:279
 Symfony\Component\Console\Command\Command->run() at /var/www/html/vendor/symfony/console/Application.php:1076
 Symfony\Component\Console\Application->doRunCommand() at /var/www/html/vendor/symfony/console/Application.php:342
 Symfony\Component\Console\Application->doRun() at /var/www/html/vendor/composer/composer/src/Composer/Console/Application.php:396
 Composer\Console\Application->doRun() at /var/www/html/vendor/symfony/console/Application.php:193
 Symfony\Component\Console\Application->run() at /var/www/html/vendor/composer/composer/src/Composer/Console/Application.php:136
 Composer\Console\Application->run() at /var/www/html/vendor/composer/composer/bin/composer:99
 include() at /var/www/html/vendor/bin/composer:119
🇺🇸United States cmlara

Wondering if the UI should become a tableselect like this instead as some of these descriptions are getting rather long. Perhaps this makes it easier to read?

That does appear to be easier to read and allow for a nice delineation between the role/description that is otherwise missing. I also like how the role is called out for both.

+1 from me (based on the screenshot) for the changes to be implemented.

🇺🇸United States cmlara

Will want to backprot this to 8.x-1.x as well, even though the form is not likely to change we do not want to encounter this flaw in the future.

🇺🇸United States cmlara

It is a documentation guide written for project moderators to make sure the followed procedure is the same for every project

This is not a Site Moderator only guide. It is also used by the community by applicants to understand when and how they can apply.

This page lineage can be tracked to the "Getting involved - Dealing with unsupported (abandoned) projects " which was intended for community members. An archived copy can be seen at https://web.archive.org/web/20170717080153/https://www.drupal.org/node/2.... That node was deleted (twice) in 2022 by @avpaderno when they moved the page to its new location, see 🐛 Taking over unsupported abandoned projects has gone away Needs review for incident information.

for project moderators to make sure the followed procedure is the same for every project

If it does not cover all scenarios how can it ensure that the procedure is the same for every project?

I am asking for the documentation to be updated to include the scenario that occurred in 📌 Request to assume module maintenance Active that a user without the opt in permission was able to be added to a project that was already enrolled in the security advisory policy. That way a user without the permission knows there is a chance they they can be approved (although not required to be approved) (or if that application wasn't suppose to be approved it would be wise for the project ownership queue to cleanup the issue).

As an aside since the contention on "will" is a large part of our discussion (I suspect English as a Second Langue is involved here):

I guess nobody would complain about using I will come to live in Italy. saying the sentence must make clear that coming to live in Italy is optional.

You have used this in the past to explain will as 'optional' however in this sentence I would contend the normal interpreted to mean the person writing the statement saying that in the future (either as wish/desire/goal or known truth) their home shall be Italy. Traditionally in this phrasing it is intended to indicate a future certainty. If there is no certainty it would be written "I want to live in Italy" to express the desire without the intention to ensure it occurs (not a life goal).

🇺🇸United States cmlara

This appears to needs an IS update to have those running the composer steps gather memory (and time if it really took 10 minutes) usage to be included in reports.

🇺🇸United States cmlara

Should have set back to active after providing feedback that
Policy “is optional” is based on actions and statements of Project Ownership admins.

🇺🇸United States cmlara

Core now has a way to highlight when config has been overriden:

Interesting, I knew that config_target would allow usage of Core validation of config, and IIRC it could be used to render the config forms (we are not ready for that in s3fs) however was not aware it was now the solution for indicating overrides.

Could be useful to adopt.

🇺🇸United States cmlara

📌 Increase minimum requierments for 2.x branch Active makes this issue much easier as we only need to support Drush13+ in the 2.x branc

🇺🇸United States cmlara

Committed to Dev on 2.x, no backport due to changing the plugins architecture (generally all internal however API not well defined in 8.x-1.x)

🇺🇸United States cmlara

Fixed, will not backport as it could impact 1.x plugins in unexpected ways.

🇺🇸United States cmlara

As noted this is required in order to maintain SQL compatibility. The 4.x branch already has a fix for this deployed.

🇺🇸United States cmlara

cmlara changed the visibility of the branch 3501484-error-while-using to hidden.

🇺🇸United States cmlara

Committed to dev. We will likely have a new release to add D11.2/D10.4 support in the near future.

Tagging for 4.x as we likely need this there eventually.

🇺🇸United States cmlara

PHP Min is now committed at 7.1 allowing this to proceed.

🇺🇸United States cmlara

is not a policy. It is a documentation page written by

According to @hestenet (CTO of D.A.) it is the closest to a canonical document we have on the subject. See https://drupal.slack.com/archives/C5ABFBZ5Z/p1746562497353509?thread_ts=...

I’ve accepted it isn’t a policy document, that in my mind doesn’t mean we can’t update it to show edge cases to better align to policy (whatever it is) so that applicants are better aware of when they apply vs when they can’t.

may you point out to any policy that says that being able to opt projects into security advisory coverage is optional?

As I have never been provided a document I am compelled to go by statements and actions of Project Ownership queue admins on D.O. and Slack. Perhaps some of this is misunderstanding of edge cases hence the question from comment #4.

For examples:
An ownership queue admin in 📌 Request to assume module maintenance Active transferred a project enrolled in the Drupal Security Advisory program to a user that does not have the security opt in privilege. Either an error was made, or there isn’t a policy prohibiting transfer of enrolled projects.

In #345233: Parent Menu Collapases on Panel Page I said Linking two issues where a Project Ownership Queue Admin transferred projects that were opted in to Security Coverage to a user who did not appear to have the opt in permission required per policy. and was told by a project ownership queue admin the document discussed in this issue is not a policy and to Please stop making false claims.. If such a policy existed prohibiting such a transfer there would be no to call out “false claims” of a policy existing.

In #345233: Parent Menu Collapases on Panel Page I quoted a project ownership queue admin as saying Project moderators will not add as co-maintainers/maintainers people who cannot opt projects into security advisory coverage and in response was told

 In English, will does not mean certainty.
That part means people who cannot opt projects into security advisory policy cannot expect to be added as maintainer, co-maintainer, or project owners. It does not mean a project moderator will not add them as maintainers, co-maintainers, or project owners

. It is always possible the individual was just trying to provide an English lesson rather than answer questions , however unless they indicate as such I have to presume they were attempting to be collaborative in helping correct a misunderstanding of how project ownership queue admins can process requests and inform me that my suggestion to automate requiring a user has the permission if the project enrolled should not be implemented as it is not policy.

🇺🇸United States cmlara

Ready for review..

I have questions about should we keep all of the older TFA Annotation fields (like helpLinks) which we may wish to consider removing before this becomes API spec however we have until Beta1 to reconsider that.

🇺🇸United States cmlara

@avpaderno has updated the relevant section to read

Project moderators will not add as owner, maintainer, or co-maintainer a person who cannot opt projects into security advisory coverage, when a project is covered by Drupal's security advisory policy. In that case, the request needs to be accepted by a project maintainer, including the project owner, or a project co-maintainer with the Administer maintainers permission.

It would be nice to have clarification on what this is suppose to mean so that we can refer to it in the future.

Scenario 1:
A user does not have the security opt in permission and applies to a module that is opted in to the security advsiroy program. The module has a stable release that supports a currently supported version of Drupal Core, neither the project maintainer nor a user with "administer maintainers permissions" on that module responds with consent.

Scenario 2:
A user does not have the security opt in permission and applies to a module that is opted in to the security advisory program. The module does not stable release for a currently supported version of Drupal core (it may have a stable release for a prior version of core, such as D10.1 when core only supports D10.3+), neither the project maintainer nor a user with "administer maintainers permissions" on that module responds with consent.

For each example scenario above: can moderators add the user to the module?

🇺🇸United States cmlara

If there is dispute on if the 2nd sentence needs to be adjusted I suggest we focus this issue solely on the first sentence as it can be changed independently in order to avoid this issue being derailed.

Updated IS.

🇺🇸United States cmlara

Since this is more technically accurate and brings up less questions merging as is..

Can be backported to 8.x-1.x manually (cherry-pick fails)

🇺🇸United States cmlara

Failed to consdier the tfa_service module and older versions mentioned in the IS, as such yes this should still fall back and add a timestamp when one is missing to add immediate protection.

🇺🇸United States cmlara

For image styles to work, we would have to either a process where the image is decrypted then styles are applied and then re-encrypted.

To sufficiently not cause unencrypted data at rest it would better to not sure that feature and for it to be removed.

This is the purpose of streamWrappers, they handle this without 3rd party code needing to be aware this is occurring. Anything stored in the encrypt:// scheme should always be stored on disk encrypted.

I am not sure that image styles works at all as the Image styles process cannot read the file since it is not decrypted for it.

I no longer have the development lab for this report provisioned due to the incident age.

At this time I can not positively confirm that I did not accidentally test scenario 2 on a file experiencing the same faults as described in scenario 1. It is possible that this scenario has never seen a public exploit path. However the scenario 2 as noted in the issue would still exist if the file becomes readable and is separate from a reading fault.

I want to reiterate what was provided in the original security report and above, all 3 of these faults can be traced to the same root cause and should be 'same fix solution' to resolve. I would additionally not be surprised if fixing the root cause of this issue would resolve other non-security related bug reports.

🇺🇸United States cmlara

Sadly it seems like we have.

I likely just need to accept that there is no longer a “we won’t break contrib” BC policy for gitlab_templates and avoid expressing those concerns going forward.

🇺🇸United States cmlara

In the spirit of previous questions about determining usefulness of this feature, linking an issue that would of been testable with Tugboat previews.

🇺🇸United States cmlara

In 8.x-1.x $plugin is populated early while in 2.x we populate $plugin inside of the IF statment.

This means that in the case of clicking cancel on the password prompt page the default text of "plugin" is being displaed rather than the plugin name as would occur on the 8.x-1.x branch.

Leaving as NR as I want to look into the background issues of why the initialization was moved and if it should be moved back allow the actual plugin name to be populated.

Either way this is already an improvement from the current mainline.

🇺🇸United States cmlara

Spun off creating the job to prevent occurrence into 📌 [PP-1] Create Gitlab job to test Minimum PHP Language syntax Postponed as that will need to wait on the Coder project (we can't adopt the lint job due to phplint control comments causing false positives in PHPCS.

As noted above from Slack the plan will be to put a 7.1 constraint into composer.json, validate that all code syntax is compatible to 7.1.

🇺🇸United States cmlara

but this work is part of a general advancement of the state of Drupal code,

I will note that Contrib is not Drupal code, each contrib project is its own unique environment, what is good for Core may not be best for contrib, the only ones who can make that decision is the individual maintainers (Note: I've not reviewed the new flagword list this is a general concept statement).

This becomes more true when I look at the linked issue and it says "Many of these will have to be ignored, not fixed. So we could see an increase in the simple counts" as already and indicator that the change could be considered disruptive.

Many of the projects that have their own cspell config are doing it for other reasons, not to avoid using core flagWords.

This is true, however every single maintainer accepted that they would manually need to sync their lists when they deployed this file. I'm not against it as an opt-in.

I would still suggest that it should be opt-out, not opt-in.

For context:
Yesterday I had a test fail, due to the changes I wasn't surprised and I attributed it to a change in PHPUnit that I would need to track down. Later another MR came in with a similar failure on a change I was not expecting to cause a test failure. My first thought after this second failure was "What did gitlab_templates change this time that my tests that passed a week ago no longer pass". It turned out that it was a legitimate failure, however that is the situation that gitlab_templates has left me in as a maintainer.

It is only in typing this that I realize that means I no longer trust the tooling, that is a dangerous mindset for a maintainer to be in, and every time we make tests that cause failures we increase the burden on maintainers. This was why I pushed for the "any change that could cause a test that previously succeeded to fail" to be opt-in/major-only.

🇺🇸United States cmlara

Setting NR to remind me to look at this in the morning.

🇺🇸United States cmlara

Overall looks good.

There is one small oddity I encountered in a quick validation check.

If one sets the permission for a role (such as content editor) and then sets the Authenticated user to have the permission the content editor role permission is still present so it will not display the inheritance warning, however at the same time a site owner can not remove the permission from the role as it is inherited.

While the patch is actually technically more correct as currently written, should we consider performing the inheritance check first and then the hasPermission check for a more consistent UI?

If we were to do that however I imagine the Administrator role would show as inheriting, is that a better or worse UX than what is currently in the patch?

Leaving NR for additional feedback.

🇺🇸United States cmlara

-1,

We chose to have our own config, gitlab_templates should not override us.

If this is added it should be opt-in not opt-out (per the BC rule they no change should be made that breaks existing test runs).

There is also a sample here or how a project that already has a cspell file can be referencing core without gitlab templates needing to be involved https://git.drupalcode.org/project/quasar_cspell/-/tree/main/docs/sample...

found that there are currently 117 projects that have their own .cspell.json file

I clicked a random one in the sample and found it implanting a variant of above to include cores dictionary.

https://git.drupalcode.org/project/vertex_ai_search/-/blob/1.0.x/.cspell...

🇺🇸United States cmlara

Coming from Add support for Druapl 10.5/11.1 Active where we now have a circular reference fault related to this change..

S3fs implements a decorator of the core file_system service primarily to prevent silent data loss with an added bonus of better performance.

However the decorator needs to obtain the mime type of files in order to set the relevant delivery headers in the S3 bucket.

We hit this once in the past and as such we created a duplicate stand alone instance of the guesser service to avoid, however now that the guesser itself directly requires the file_system service we encounter the circular reference errors again.

🇺🇸United States cmlara

🐛 Separate MIME type mapping from ExtensionMimeTypeGuesser Needs work appears to be the cause of the circular reference
https://git.drupalcode.org/project/drupal/-/blob/2d44347bce438454f160096...

We have our own copy of the mimetype guesser because of the file_system service conflict in the past.

Now the guessers require the file_system directly to access FileSystemInterface::basename().

🇺🇸United States cmlara

Additional note:

If a developer really wants to implement this, I imagine they could do so with a Login plugin if they detected a password reset page and signaled the plugin to allow access.

TFA can't control what other plugins exist or installed, I would advise against such a plugin (see related Security Advisories) however if a site feels strongly this is absolutely needed that may provide them an option to do so without TFA providing the feature.

🇺🇸United States cmlara

As described I'm going to classify this as a won't fix.

It is very intentionally that TFA is required in this scenario and not permitted to be disabled.

This is not just a convention between the TFA module and the site owner, it is also a convention between the end user and the TFA module.

A primary purpose of MFA is to ensure that a second factor is always provided. If this feature were to be added it would undermine protection against one of the most common attack vectors that MFA is designed to protect against.

If the user doesn't know their Token's it is time to be speaking with the site support who should be running procedures on how to grant access. I admit we could possible provide more tools around this (other than the currently available 'disable tfa' on the single user account) however that is a separate discussion from a global disable of the prompting.

Somewhat related security concept:
CWE-620: Unverified Password Change

🇺🇸United States cmlara

On 11.2:
Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException: Circular reference detected for service "s3fsfileservice", path: "plugin.manager.archiver -> s3fsfileservice -> s3fs.mime_type.guesser -> drupal.proxy_original_service.s3fs.mime_type.guesser.extension"

On 10.5:
This looks like some change in the test runners is again breaking tests on non-critical deprecation.

🇺🇸United States cmlara

cmlara changed the visibility of the branch 3528246-add-11.1-10.5 to hidden.

🇺🇸United States cmlara

and is not able to use TFA, he is stuck at login.

Can you clarify Are you saying the password reset feature does not function when a user does not have any TFA tokens provisioned, or are you saying that the user has tokens configured however does not recall/does not have the token in their possession?

Related security advisories:
One Time Password - Moderately critical - Access bypass - SA-CONTRIB-2025-061
Two-factor Authentication (TFA) - Critical - Access bypass - SA-CONTRIB-2023-030
Enterprise MFA - TFA for Drupal - Moderately critical - Access bypass - SA-CONTRIB-2025-053

🇺🇸United States cmlara

We can likely go a bit further here and allow the fields to be configured by site owners using tokens.

🇺🇸United States cmlara

Unable to reproduce the problem described in the original issue.

🇺🇸United States cmlara

The exact date in December is internals of the release schedule

Putting on my Systems Engineer hat:

Exact dates for EOL are should not be internal process, they should be set and given to the public for us to design our plans, how the core team makes that date happen is an internal process.

Releasing a final security release on the EOL date of a major would be downright cruel IMO.

As a Security Engineer:
One last final security fix on the EOL is appreciated (as long as it’s bug free, otherwise a courtesy patch after EOL is called for).

As a Systems engineer: My plans are to be off the software before that date, if for some reason I’m not, I would appreciate the one last final fix, though I do have concerns about system breakage.

I won’t say it is super common that Security EOL is also a release day, though I’ve encounter it a number of times in my life. Ideally yes 100% of the known security issues were buttoned up in November, though all software runs the risk of a new discovery after the last window and that is what the final release covers.

Side note: since Drupal runs a scheduled release cycle, D12.0 release notes should likewise be able to include the date that the 12.x branch will loose all security support and how the Core team makes that date happen would be an internal process.

Production build 0.71.5 2024