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

Merge Requests

More

Recent comments

🇺🇸United States cmlara

D.O. has migrated away from patch files.

In order for tests to run we require all submissions to be in the form of a Merge Request.

More details may be found at: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

Quick glancing:
I suspect there will be a PHPCS warning on the comment in the patch file,
I haven’t analyzed the logic flow.

🇺🇸United States cmlara

We can call this a duplicate of 🐛 Installing contrib modules can lead to TFA accidently being bypassed Fixed ( Not scheduled for backport to 8.x-1.x due to significant infrastructure changes) as it resolves the long term issues of enforcing TFA on all requests.

📌 Warn users that TFA is incompatible with modules overiding specific routes Active and 📌 Evaluate restoring using a form alter instead of extending UserLoginForm Active are also candidates for this to be considered a duplicate of.

Note: If this was not an already known and publicly disclosed it would have been considered a potential security vulnerability inside TFA and would be desired to be reported privately.

🇺🇸United States cmlara

See 📌 [policy, no patch] Decide policy related to use of novice tag Active regarding the (lack of) policy regarding Novice tagged issues.

🇺🇸United States cmlara

Note: This will cause the system to no longer render HTML logs on Drupal 11 tests, see 🐛 browser_output is empty when testing with Drupal 11 Active .

Since we can run the tests locally that is a minder hindrance that we can eventually solve with custom PHPUnit configuration in our repo.

🇺🇸United States cmlara

PHPUnit fail is expected. No stream_context deprecation seen.

This is a pre-requisite to resolving PHPUnit failing on deprecation as it can not be ignored in phpunit configuration.

🇺🇸United States cmlara

We may want to re-open 📌 Contact Material+ (formerly Srijan Technologies) about Contribution Best Practices Active .

Well intention or not, Material+ has been previously instructed to both train and audit its employees contributions.

🇺🇸United States cmlara

I believe this will need to ported to 2.x.

🐛 Original page lost after TOTP authentication Active does not on cursory glance appear to be applicable as this is our 'force to redirect' where we do want to ignore the destination.

🇺🇸United States cmlara

Initial thoughts are why we should be even touching the Global Request.. Turns out this is from a OLD core bug that this was even needed. 🐛 Allow form redirects to ignore ?destination query parameter Fixed .

https://www.drupal.org/node/3375113 for how this would apply to newer versions of core.

That all aside, it appears this is the way it needs to be for now and matches code later in the same class.

Forcing commit to bypass tests due to issues from 🐛 beStrictAboutOutputDuringTests=true or failOnRisky=true causing test failures due to PHP8.4 Implicit Nullable deprecation Active .

🇺🇸United States cmlara

A hook requirements warning sounds good here.

I would suggest if there are 2 possibly unconnected issues it might be time to take this to the next level and upgrade install.php to not not allow executing if a settings.php exists and does not have an "allow install" setting contained inside.

🇺🇸United States cmlara

This sounds like it could go into Drupal.org Customization as a new feature for the website or at least perhaps some additions to the Update Status module in Drupal Core.

Re-opening and transferring queue for a proper review.

I can not run a script to scrape the data of the Drupal.org website.

Why not? (This data is available via various API's) (I'm not saying making everyone do this is the best method, however I'm sure this would be asked so should at least have an answer for the D.O. developers ready).

🇺🇸United States cmlara

This appears to be a duplicate of Confirmation forms should not require passwords Needs review .
Closing in favor of the existing open issue to centralize discussion.

We need to restrict access to local Drupal accounts for users authenticated via SSO.

The view/policy established in #2855394-8: Support TFA on sites with LDAP authentication is generally that an external identity provider should provide the MFA and related login protections.

Addtionaly:
D.O. has migrated away from patch files.

In order for tests to run we require all submissions to be in the form of a Merge Request.

More details may be found at: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

🇺🇸United States cmlara

possibly add a Service Decorator on the update.fetcher service. The decorator could always provide an empty value to the the site_key parameter,

I have not published a release yet, however there is now https://www.drupal.org/project/site_key_mutator which will (by default) empty out the site id removing the Unique Identifier (phone home) portion of the update checker.

Moving project telemetry reporting from the update module into core Active would have been the better issue however its postponed and unlikely to see resolution anytime soon.

🇺🇸United States cmlara

This one kinda fell through and was lost.

Wording is still the biggest one. With a fresh set of eyes it may need to be something along the lines of "Account is known to have configured TFA" or similar as the isReady() method really is checking for a known configured plugin yet doesn't assert that it is properly operational.

The entire 'check if a user needs to login with TFA' subsystem is seeing patch after patch as we find new bad assumptions (at the time my last comments were made https://www.drupal.org/sa-contrib-2024-003 was likely running through my mind).
Eventually we can come back to firm it up to be more towards known operational.

Needs work as it requires a rebase at a minimal. We also may want to look at the fact ECA 2.x is now out and see what impact this has, if both can be supported or not.

🇺🇸United States cmlara

Having a heck of a time over in 📌 Deprecations should not fail the pipeline Active trying to get PHPUnit to allow tests to pass with this deprecation.

It appears PHPUnit may force function deprecation's up to an Exception while allowing other PHP deprecation to proceed (opinionated PHPUnit strikes again?).

The replacement method was only added in PHP8.3 so this will need a function_exists wrapper. Also looks appears we may have to resolve some baselined phpstan warnings at the same time to add the new code.

📌 PHP7.0 support Active is not a limiter as this can be done in a manner that supports all versions of PHP.

🇺🇸United States cmlara

I'm going to re-open this, the method is used in core requiring third party streamWrappers to implement even though it is not on interface.

modules/system/src/Routing/AssetRoutes.php:    $directory_path = $this->streamWrapperManager->getViaScheme('assets')->getDirectoryPath(); // @see 3504164
modules/image/src/Routing/ImageStyleRoutes.php:    $directory_path = $this->streamWrapperManager->getViaScheme('public')->getDirectoryPath();
modules/image/src/PathProcessor/PathProcessorImageStyles.php:    $directory_path = $this->streamWrapperManager->getViaScheme('public')->getDirectoryPath();
modules/image/src/Entity/ImageStyle.php:      $directory_path = $stream_wrapper_manager->getViaUri($uri)->getDirectoryPath();
lib/Drupal/Core/Updater/Updater.php:    return \Drupal::service('stream_wrapper_manager')->getViaScheme('temporary')->getDirectoryPath();
lib/Drupal/Core/File/FileSystem.php:      if ($filename = tempnam($wrapper->getDirectoryPath(), $prefix)) {
modules/package_manager/src/PathExcluder/SiteFilesExcluder.php:        $path = $wrapper->getDirectoryPath(); // This has a LocalStreams check, however that makes an assumption all replacment local streamWrappers will inherit from LocalStreams which is bad assumption.

Since not all streamWrappers will have a directoryPath my suggestion would instead to be that we remove the method from the public usage and find alternative solutions Core should not assume a streamWrapper path is local. This might create some initial pain, however longer term if we get into the mindset to not assume local disk access for anything expect local file paths we will be better aligned to the intended usage of streamWrappers.

Leaving as 'file system' for component as initial decisions need to come from there, eventually though this may need to expand to 'base system' or similar.

🇺🇸United States cmlara

Tagging related issue where its been known in the past this method was used when not on an API and should not be used in core.

🇺🇸United States cmlara

I will add that @makbay could fork the project into its own namespace and continue development.

Would have zero delay and would more closely align to how the rest of the world handles abandoned projects (the whole “steal a namespace”/supply chain attack of Drupal is an oddity).

Not required if @kreynen receives and assigns maintainer rights, mostly posting this for other developers who find themselves in a similar situation without a benefactor to volunteer in their name.

🇺🇸United States cmlara

Pulling in some notes that should be considered:

📌 Make igbinary the default serializer if available, it saves 50% time on unserialize and memory footprint Active is considering swapping the serlaizer, the change to using serialize/unserialize directly may hinder them.

🐛 Json and PHP serializers should throw exception on failure Active is working to have the serializers throw an exception as documented on the API .

I have questions on if returning a cache miss is the correct response here.

If we are having issues unserializing data that implies a significant issue MM in the internal backend.

With a swappable serlaizer the risks are higher however in theory even with this implementation if serialize/unserialize ever has a BC break in how it stores data (not expected) and one has multiple front ends that are operating on competing PHP versions this is going to have a significant amount flutter reverting back and forth with little to no knowledge or is occurring.

Sure this keeps the site online however at what cost? Is hiding a possibly significant system problem worth working in the rare case where this will occur ?

The core/rebuild.php script exists as a standalone (no container needed) method to deal with the cache system purge in the case that it is somehow compromised.

🇺🇸United States cmlara

I mentioned in Slack that the raised concern with Database Cache Backend likely needs significant architectural decisions that should not be rushed into this issue however I would implement it if it is the only way to get this to move forward.

It turns out that there already is an issue to handle the request in 🐛 Database backends cannot deal with corrupt serialized data: app completey broken Needs review .

Setting back to NR.

🇺🇸United States cmlara

PHP leaves this fairly undefined.

Falling back to referencing POSIX

shall also cause the directory stream to refer to the current state of the corresponding directory, as a call to opendir() would have done

https://pubs.opengroup.org/onlinepubs/9799919799/functions/rewinddir.html

🇺🇸United States cmlara

Pulling out of Slack:

At the time I had worked on this module I had though D7.1 was the minimal version of PHP that worked with Drupal 8.8. Had I of known at the time I likely would have hard coded 7.1 into the docs.

Clarifying this as 7.1 appears to be the long term solution.

🇺🇸United States cmlara

Module is not currently compatible with Drupal 11.

The supported 8.x-3.x branch currntly supports D11, only the not alpha dev branch 4.0.x is behind.

Linking more related issues. Not 100% sure why I closed some of these instead of setting them as patch-to-be-ported (might have been to avoid noise to other developers), however I did so might as well roll with them now.

Now that Core has a solid D11 cycle it is likely time to jump the minimal version of 4.0.x to start at 11.1 and newer (which will likely be bumped again before final release of 4.0.0) allowing the removal of deprecated Core code and jumping of the system requirements

Changing title of the issue, may become a meta. Will need to update the IS.

🇺🇸United States cmlara

Opened MR72, fix may be as simple as a reset() of our array.

This does not update the listing at reset and will return the same listing the second time through the iterator. Need to validate that this is aligned with normal stream wrappers. Setting as NR while that is being validated.

Need to fix branch failures before able to commit this.

🇺🇸United States cmlara

I have removed the account from the following modules:

Vault - AppRole Authentication
Vault - AWS Dynamic Key Provider
Vault - KV Key Provider
Vault - Token Authentication
Vault - Transit Encryption

The following modules list themselves as part of the Vault Ecosystem however they have no code and were not “adopted”/“stolen” previously so I will be unable to remove you from them:

HashiCorp Vault - TLS Authentication
HashiCorp Vault - TOTP

The Project Ownership queue can help you with them.

Historical note:
The account had already been downgraded to no-permissions at a previous time.

The README had previously been updated to list Nick as a previous maintainer.

🇺🇸United States cmlara

Thank you Nick for the starting base on this and the well wishes!

I will remove you from the list on this and the associated ecosystem modules.

🇺🇸United States cmlara

Adjusting tag assigned in #32.

Security is for disclosing vulnerabilities that need fixing.

🇺🇸United States cmlara

Had a moments to run the steps to reproduce through Drush:

$ ddev drush --xdebug php
Psy Shell v0.12.7 (PHP 8.3.14 — cli) by Justin Hileman
s3fs test site (Drupal 11.1.0)
> $iterator = new \FilesystemIterator('s3://');
= FilesystemIterator {#8904
    path: "s3:/",
    filename: "2024-12",
    basename: "2024-12",
    pathname: "s3://2024-12",
    extension: "",
    realPath: false,
    aTime: 1969-12-31 16:00:00,
    mTime: 1969-12-31 16:00:00,
    cTime: 1969-12-31 16:00:00,
    inode: 0,
    size: 0,
    perms: 040777,
    owner: 0,
    group: 0,
    type: "dir",
    writable: true,
    readable: true,
    executable: true,
    file: false,
    dir: true,
    link: false,
  }

> $iterator->rewind()

   TypeError  Drupal\s3fs\StreamWrapper\S3fsStream::resolvePath(): Argument #1 ($path) must be of type string, null given, called in modules/custom/s3fs/src/StreamWrapper/S3fsStream.php on line 1123.

Here is the relevant portion of the stack trace:

S3fsStream.php:1123, Drupal\s3fs\StreamWrapper\S3fsStream->dir_opendir()
StreamWrapper.php:477, Aws\S3\StreamWrapper->Aws\S3\{closure:/var/www/html/vendor/aws/aws-sdk-php/src/S3/ StreamWrapper.php:475-479}()
StreamWrapper.php:941, Aws\S3\StreamWrapper->boolCall()
StreamWrapper.php:475, Aws\S3\StreamWrapper->dir_rewinddir()
35:1, FilesystemIterator->rewind()

The parent AWS's code is passing $this->openedPath as null. We intentional don't call parent::dir_opendir() in our dir_opendir() where it would be set. I don't believe we would want to use parent::opendir() as we depend upon the database. This is a private variable so we can't just set it in our dir_opendir().

Above stack should be enough to determine a proper solution.

Cursory thoughts:
We likely need to implement dir_rewinddir() ourselves.

🇺🇸United States cmlara

Closing as a duplicate of Allow TFA requirement to be configured per user Needs work based on description.

Based on the photo:
It looks like the request may be intended to allow REST login while using 8.x-1.x.

A reminder that 8.x-1.x does not support having any REST based modules installed and enabled.

See https://www.drupal.org/sa-contrib-2023-030

Allow TFA authentication through REST routes Active which was solved by 📌 Decorate the user.auth service Fixed would also be related for the long term resolution in the 2.x branch.

Additionally to note:
D.O. has moved to a system of exclusively using Merge Requests.

Projects on D.O. no longer work with patch files as they can not be run through CI tests. See https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... for more information on working with GitLab and MR’s

🇺🇸United States cmlara

D.O. has migrated away from patch files.

In order for tests to run we require all submissions to be in the form of a Merge Request.

More details may be found at: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

🇺🇸United States cmlara

Any hint on debugging this would be helpful

I would suggest starting with xdebug and a conditional breakpoint for when $uri is null.

I suspect we will find this is likely a child call where the stack will show a parent call somewhere else in the streamWrapper code is passing NULL. I am not expecting this will be an issue in PHP proper, although if the stack trace stops with just the dir_opendir() call that could be a possibility.

🇺🇸United States cmlara

How is $uri null here? The relevant specs appear to only permit a string value. PHPStan alerts similar with warning "Call to function is_null() with string will always evaluate to false."

This implies there is a bug somewhere else in the stack that needs to be identified and resolved, setting back to needs work to identify the root fault and for eventual tests to be included with the fix.

@see https://www.php.net/manual/en/streamwrapper.dir-opendir.php
@see Drupal\Core\StreamWrapper\PhpStreamWrapperInterface

🇺🇸United States cmlara

@vishal.kadam this is a well known issue on D.O that has nothing to do with stability levels.

@batigolix 🌱 Change how modules and submodule dependencies are handled to have more consistent namepsaces [DRAFT]. Active has some related discussions about a long term fix for this (though there appears to be no movement to resolve this long term).

The short version is the MOM project (likely) now has control of the namespace as it had a project with that module name first.

You can move this to Drupal infrastructure and ask them to rename, no gurantee they will, however they have in the past.

🇺🇸United States cmlara

If you want a reference point I have pulled some of the gitlab_templates out into components though they have been modified to better align to my personal build style.

https://www.drupal.org/project/quasar/ecosystem

Project is slightly on hold as I’ve decided the DrupalCi Image and its build process doesn’t align to my design needs and I need to create a new custom image (along with the associated build infrastructure ).

I do have a mostly built phpunit job I haven’t uploaded that’s designed so in theroy you could run the build stage and phpunit stage at once, and customize each job run. Environment variables became a bit key in the design to allow creating base jobs and inheritance.

My comment about components:
While it can all be done in this project I would suggest individual projects for each component (this lets you version them separately)
Only move to components if your willing to adhere to a strict versioning scheme, this is easier now that gitlab has a well defined “inputs”
System

🇺🇸United States cmlara

There are also at least 3 messages in Slack #contribute channel from Ivnish:

And from other people:

None of these 5 messages directly make note of the ownership transfer request being open.

Here is the proof that I have contacted the maintainers in Slack

This one is better, and I missed its linking earlier in message #9.

This message includes @shrop and @pwolanin and none of these other maintainers. @shrop has permission to manage maintainers and responded they were willing to discuss though it appears they never visited this issue or took actual actions.

As this was message #9 there is still no note to the maintainer that they MUST take action or D.O. staff/volunteers will intervene, at that point it still looks like a low priority “I would like to help, but it’s entirely your choice”.

Rather than re-pinging @shrop (who has permissions) the messages instead re-pinged @pwolanin. This is questionable as well since @shrop could have legitimately taken action on the module while @pwolanin is still unclear.

I will add that @dieterholvoet was not pinged until @pwolanin sent a message in the thread mentioning they had added the user. This looks like a missed opportunity early on for them to have talked to @shrop and maybe moved this forwards. Not a requirement by any rule, just pointing out communications disconnects here that helped end up in this situation we are now.

I will add a ping in there to @shrop that he can clear up the current ethical concerns regarding @dieterholvoet. That likely will not clear up the all the questions regarding the transfer before now that will still need to be addresses however it can at least set a clean date for actions by @dieterholvoet going forward.

I understand that there are rules, but having 13 maintainers and no one is responding and the module that has a lot of installations and people are waiting for Drupal 11 support.

12 prior to adding @dieterholvoet, Only 4 or which have the authority to add other maintainers.

Having a large number of installations makes following the rules even more important and taking clear actions even more important not less. As noted above this is D.O. injecting a new user with the ability to commit code without the consent of the namespace owner, while D.O.’s adoption process has made rbis seem like common place behavior it is still manipulation of the supply chain.

I think here could be some help from d.o admins to solve this problem

This is a fair criticism. I mentioned somehere recently that it might be worth having D.O. better lay out expectations for those with increased power, the word “volunteers” is often used to excuse away inaction on D.O. without regard for the fact that taking on enhanced roles can and should increase the expectations of responsibility for those with the role.

At some point this module was considered to be added to Drupal CMS, but because of lack of Drupal 11 support, it was not added there too

This should not factor into the overall situation, the process for Drupal CMS started early last year allowing plenty of time for this process to occur. There is also the Project Update Group that could have addressed this concern. Both od these could have been completed easily on the time allotted.

🇺🇸United States cmlara

I would also recommend instituting a mandatory periodic review of maintainers for modules (with current versions of supported Drupal - shield "covered by security team") reportedly used on over 10k (or some threshold) sites.

Not exactly sure what you are suggesting so throwing out a couple of possibly related issues for you to look into to see if they meet your desire.

🌱 Automatically degrade maintenance and development status of projects over time Active was also opened to try and indicate to the public that modules are not being maintained that might be related to your suggesiton.

Something similar was also suggested to ping maintainers to ensure they are still active (to try and avoid modules reaching a state where a security vulnerability was found yet no maintainers were responsive) in #3260712-12: Discuss involving ecosystem maintainers in security support degradation process .

I will note that I have another issue open (that is somewhat divisive) that argues we should never allow adoption of modules at all Prohibit the ability to adopt a project Active instead we should encourage forking of modules. @dieterholvoet would of been able to do so back in November with zero delay, and a simple post in the D11 compatibility issue could have started user migrating to a new namespaced module (allowing the existing namespace module to die a natural death through attrition, which could also be helped by the above issue of auto degrading maintenance status).

🇺🇸United States cmlara

So no indication via update status that there is a secure/release project that can be updated to from an unsupported one, which feels like a severe regression from the current situation.

Nothing technically stopping us from adding that as field on the module that Update Status can pull from to display the alternative. Equally Composer could also show this as part of its security audit report by including the alternative module in the description of the report. This would go under the comment above of 'other announcement streams that have not yet been created"

Current namespace 'owners' are always given at least a month to respond to any project ownership requests, this is quite a long time - especially considering the actual length of time when they're unresponsive is usually a lot longer than a month in practice.

It can actually be as low as 14 days if they have not logged in in over a year the Project Ownership Queue will not necessarily reach out to them. Addtionaly I have seen the Project Ownership Queue maintainers reach out before the first 14 day window expires and consider the second 14 day window clock already started. Both provide means that the request could be as little as 14 days.

I imply over in 📌 Automate the majority of the ownership transfer process (retain human approval) Active that the not reaching out 100% of the time also means it is possible they may never have been emailed expending them to look at every issue in the issue queue daily to know such contact has occurred (there have also been raised questions in the pastt of possibility of using slight naming differences to trick the project ownership queue maintainers though that issue may have been solved last year).

Why is it preferable to be able to strip the namespace from the current owner, something you apparently agree with, than for e.g. a member of the security team, a core committer, or a well known contrib maintainer of other high usage modules to be able to fix security releases for what could be tens of thousands of sites using a project?

I assume you mean "not be able" (clarifying here as that is how I'm answering the question).

For me this centers around the built trust that maintainer has gained from me over the years. It is impossible to review 100% of commits from 100% of projects I use on a day to day basis from Drupal to my web server to my web cache to my server operating system. There are likely numerous studies on this fact that open source does not necessarily mean 'more secure' since not all commits are indeed audited.

What this means is at the end of the day I"m trusting an owner to make decisions and audit their modules and those whom they give permissions to, a decision I would not trust to the Project Ownership Queue.

The XZ backdoor last year gave us an example where this can fail, its not a perfect system to trust just the code owner, however at least in that case the code owner made the decisions, they personally signed off that "I will allow this user to commit and publish releases in my name" while the D.O. adoption process allows a small group of people (who take no long term responsibility to audit the actions of the users they approve) to give someone whom an owner may not agree with access to attack silently attack as you put it "tens of thousands" of sites (and their tens of thousands of end users).

I can say with confidence that the only safeguard preventing me from pulling off the example attack given in the Issue Summary is the fact that I am not an attacker and that I have no desire to successfully pull of such an attack.

Getting enough social reputation is only a question of time and effort. For many the time and effort would not be worth perusing, for the dedicated attacker looking at the long attack, it could be easy and cheap.

At least with a forked project it is very apparent to me as a site owner I'm working with a new maintainer that needs to earn my trust.

🇺🇸United States cmlara

"reaching out to the official maintainer" - unless I'm missing something, it appears both @dieterholvoet and @a.dmitriiev attempted to reach out to the maintainers

The current process docs described in https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or-distribution-project/maintainership/offering-to-become-a-project-owner-maintainer-or-co-maintainer/how-to-become-project-owner-maintainer-or-co and How project moderators handle requests to be project owner, maintainer, or co-maintainer calls for a Project Ownership queue maintainer to reach out on their own to validate.

I will note I've been told these are not binding policy's and that they are more guidelines.

While not intending to accuse anyone here of perhaps not reaching out, there is no way in the current system to verify @dieterholvoet (the important one as the requestor) and @a.dmitriiev (not as important as they are not a queue admin) actually did reach out to the maintainers via the contact form (security engineering: assume everything it is a misleading statement until proven otherwise) telling them if they did not respond they would loose access to the module.

If you look at the message at #2 for example, it in no ways conveys that failure to respond could lead to a new maintainer being added without the existing full level maintainer consent. Compare this to a message such This one sent by @avpaderno 📌 Offering to Co-maintain Entity Extra Field Active which makes it more clear that failure to respond will result in permission changes.

This is important when we consider the project adoption process is essentially a supply chain attack, forcing a new maintainer onto a project without the consent of the maintainers that are authorized to add/remove users.

I mention over in 📌 Automate the majority of the ownership transfer process (retain human approval) Active that some of this ambiguity could be removed by moving to a more code controlled processed where automated systems send out the email on the requestor behalf (this allows bypassing the contact form being disabled, and provides a 'trusted' record that messages have been sent).

are there guidelines for recommended/required:

The recommendation are that Project Ownership Queue Admins will attempt to reach out again to be sure the maintainer is aware of the request unless it is clearly obviously they inactive.

Based on the public data of GitLab appear to have the maintainer logged into D.O. in the past year, its close to the year mark, however still within a year which in my experience has been treated as "not presumed completely inactive" and given the chance to respond.

should @dieterholvoet be made a co-maintainer instead to proceed?

If this application were to be completed I would say yes. Lets assume for sake of argument @avpaderno did contact the module maintainers, they would have been told that the user requested a role that will give them only permission to commit to the project, not to add other users.

From a risk analysis standpoint while this is still risky, it is much less risky than a user being given full privileges where they could remove other maintainers (but not the owner) that could still act as a checks and balance. It is fair to believe that such an issue will receive a lower priority to response and is more likely to accidentally not be responded to.
(Again not intended to imply @dieterholvoet has any ill intention, this is solely from a security analysis standpoint).

what should happen now? next step?

For me the big question is, by what authority did @pwolanin grant the access?

Is the security team allowed to interact in this queue for modules that are not unsupported due to security issues? I see that nowhere in the documentation on D.O. They are allowed to transfer ownership for known insecure modules.

Is @pwolanin actually a Project Ownership Queue maintainer allowed to enforce the policy? If so why did they mention using their security role?

I can't speak for D.O. and the D.A., however to me these raise serious questions about possible permissions misuse. I've been told on Slack that D.O. permissions have been misused in the past with CWG reports made but no apparent action taken (hence the need to track these scenarios).

That is probably the main problem, including the fact there are site moderators who, with the excuse a contact form is not enabled, proceed to change the project owner, or add maintainers without even notifying the project owner. (edited)

It would be a CWG report. I have already posted one, but it does not seem it helped.

-- @avpaderno_
https://drupal.slack.com/archives/C2AAKNL13/p1718570858385279?thread_ts=...

If the permissions were not misused it could be helpful for D.O. to better document this, and @pwolanin could help by providing the normal comments that Project Ownership queue maintainers put in (eg: "I've contacted the maintainers, postponing for 14 days" ) (for some reason we do seem to see a number of people contacted by a Project Ownership Queue moderator respond when they do not respond to normal users) followed by with the transfer of ownership.
Side Note: I'm aware @pwolanin was pinged via Slack to consider taking action on this issue, unsure if they saw the message.

If the permissions were misused that would be a question for the D.A. on how they respond. I would suggest at a minimal the process should be restarted at the "Project Ownership Queue Moderator reaches out to Project Maintainers" stage of the process.

but it's not actually helpful in rectifying the issue without some additional context

Unfortunately I don't have any authority to enforce this one way or the other.

I had hoped @avpaderno would have commented which would have given this issue guidance.

I question if D.O. users with the enhanced powers feel they have any right to intervene as well. I do question if I should fire this up to @hestnet and/or the Drupal Security Team as a possible breach, however I want to give @pwolanin more time to respond before I go that far as perhaps there is some policy somewhere I am either not aware of or have forgotten that allows this.

I'm making an assumption however I read #21 to mean this transfer was made outside of the normal Project Ownership Queue approval process which further causes me question how this transfer occurred.

🇺🇸United States cmlara

I was primarily referring to the warning text added per https://www.drupal.org/drupal-security-team/security-team-procedures/mar... as it will be present on the top of the modules info page and it is already generally touched (removed) as the last step in https://www.drupal.org/drupal-security-team/becoming-primary-maintainer-... the security team would not be excessively burdened by adding a link to an alternative instead of deleting a notification block.

This proposed policy would not limit where the D.S.T. could publish that a module is unsupported for security reasons or limit where they could publish helpful information. The security team could choose to amend the Security Advisory, publish an auxiliary newsletter either through the PSA system or a dedicated system regarding alternatives for unsupported modules, or utilize any other announcement system that may as of yet not already been created.

This policy would just prevent transferring a namespace to another user without the approve or the current namespace “owner”.

🇺🇸United States cmlara

As I read this issue, it would prevent any of this from happening, so what would happen in that case?

Yes this policy would prevent that existing practice from occurring.

We would be compelled to follow the more general method of maintainers fork the project into a new repository and work from there. D.O. or the D.S.T. could still place an announcement flagging the module as insecure and in that message they could recommend an alternative.

I do note in the cons that yes this will create more namespace pollution in exchange for increased supply chain integrity. This could be minimized for example by search not displaying modules that are known security vulnerable.

Forking is a method that works for the vast majority of the internet, there is no reason to belive it can not or would not work here. This could already occur today if a maintainer decides to do so. At least for users who do not have the security opt in permissions has occurred to create a new project when a maintainer is inactive on a security covered module.

Also as noted yes this would require a site owner to change their deployment to use the new project over the old in order to obtain the security fix. Best practice is that you should uninstall the existing project the moment the unsupported SA is announced. We know from experience this does not always occur. This would need some community time for site owners to learn that they need to stay on top of the modules and can no longer assume that if left installed they will eventually “magically” become supported again.

There also has been recommendations in other issues on how to avoid modules becoming unsupported in the first place (such as contacting maintainers regularly to ensure they are still interested in working with the security team which could act as a point for them to willfully transfer ownership to another community member that is active on the project before they leave the community).

🇺🇸United States cmlara

Is there a way to monitor their commits to quickly see if they do anything malicious?

The Gitlab UI and API provide some data:
https://git.drupalcode.org/jayapriya-18 For example shows push history.

The rest API can also likely be utilized https://docs.gitlab.com/ee/api/events.html#get-user-contribution-events for a more automated method to monitor and filter.

🇺🇸United States cmlara

Can @dieterholvoet start to contribute?

I have no control on this one way or the other, I am primarily just tagging the issue so that it can be referenced easier at some point in the future when discussions come up regarding the security and integrity of the existing process such as in 🌱 [META] Increase Security of Project Ownership Transfer Process Active or it’s children.

There have been a number of incidents in this queue in the past that may raise questions (such as this one) that are hard to locate without some sort of tracking flag being added.

@pwolanin addressing the concerns raised would go a far way into clearing up the situation if @dieterholvoet is concerned about the ethical questions this raises regarding their new access.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

I do not believe the situation as described (login without password yet prompted for OTP)_ can occur in the 8.x-1.x and newer branches,

We have a number of locations where we forcefully set the destination. I suspect this issue still exists in some form.

This should be manually tested in latest versions.

🇺🇸United States cmlara

Note that related issue 💬 Remove contributor listed on company profile Active shows a case of malicious association (account was never an employee of the organization and appears to be spam) as an argument in favor of the original request of allowing organizations to confirm users before they are added.

Failing to implement such a feature as originally requested could be argued as the D.A. and D.O. knowingly and willfully supporting the malicious behavior.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

There is already an open issue for the D8+ branches at #3323866: Offer TFA flow for other use cases than login that can be used for further consideration of this issue for supported releases..

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches. The templates can be modified under TFA configuration.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

I believe this request may no longer be necessary in the 2.x branches as TFA is enforced for logins at the User.Auth service level.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

We allow configuring the issuer in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

My understanding is the D.O. Ecosystem wants PHP-TUF to be adopted by other providers correct? Sites like packagist.org and really every composer repository out there?

If that is a fair statement I would suggest a radially different policy compared to what is currently proposed:

Separate the TUF projects from Drupal core.

Do not allow core maintainer status to play any role in the TUF project management, give the project an initial developer team and let them choose how the project is run, who is a maintainer, and have sole authority independent of the desires of the Drupal Core project or the Drupal.org infrastructure team.

Give the TUF project the freedom to make decisions that Drupal would disagree with in order to promote the protocols growth. Allow it to become its own ecosystem, do not try and force control over it from one single project that represents a fraction of the global Composer usage.

🇺🇸United States cmlara

and how will we prevent future problems?

For now that may just have to be the Human Review process.

Core could create a PHPCS rule that the description must contain a format like “True if . False if .” when BOOL is used however that might be a bit more disruptive. I would suggest such should be a follow-up issue as fixing the broken docs is the bigger concern.

Core adopting higher level PHPStans would also help long term but is outside the scope or this issue.

What is the plan here so that we know that they are all fixed

Some missed would be better than none fixed.

🇺🇸United States cmlara

Drupal CMS doesn't really exist without project browser because otherwise you can't install recipes and other modules etc., it may end up getting integrated into the installer itself.

I'll note that if these services are that critical for Drupal CMS, the project could possibly add a Service Decorator on the update.fetcher service. The decorator could always provide an empty value to the the site_key parameter, that should be sufficient to mitigate the 'phone home' issue allowing Drupal CMS to keep the dependencies.

Drupal CMS would still want to disclose it makes web connections either as part of its quick start guide or the installer (I've worked in several environments that an unexpected network request from a server will result in the Network Access Control killing the network port) however it would not need to receive telemetry consent since the unique identifier is removed.

Drupal CMS would need to keep an eye on core to be sure it does not add any other telemetry data which could necessitate adding a consent form.

🇺🇸United States cmlara

Opining from the other side of this:

I don't enable the update module because of the telemetry reporting. This also prohibits project browser from being deployed.

If the update module only requested a list of modules I would be more likely to allow it to be enabled.

Polar opposite reason as to the original request, however equally tied to the same problem, mixing two different functionalities into the same code.

Re opt-in/opt-out: I'm very much a fan of 'opt-in', we need to have consent for the data collection, opt-out does not give that consent.

🇺🇸United States cmlara

https://duanestorey.com/posts/down-the-rabbit-hole-a-deep-look-at-the-wo... is likely worth a read as it relates to Wordpress and the recent concern that Wordpress.org may have used telemetry data for less than ideal purposes (the Wordpress Engine Tracker site) raising questions about data collection in general by a CMS.

There is a lot of talk in this issue about collecting random data on a whim of someone had the idea it would be useful.

I would suggest that if this initiative ever does go forward that the first question asked regarding any data point should be "what problem will this solve in the next 3-6 months?" if you can not identify a specific concrete problem that can not be solved without the data point the collection is questionable.

I note this in other issues, even the collection of a site specific ID is enough to cause Update Manager to be disabled in many environments I work with. In general, the more data collection you add the less likely users are to be willing to share it.

🇺🇸United States cmlara
Unfortunately it does not go into details about what the WordPress FUD was.

I can’t speak to what it was that long ago, I however note that Wordpress has been involved in more issues around their update module lately being believed to be uploading significantly private details leading to the creation of the WPEngineTracker site.

D.O. too my knowledge couldn’t leak as much details as that system did (I don’t believe we send the plaintext hostnames as part of any headers) however even a unique ID per site is sufficient to create a similar counting system.

See https://duanestorey.com/posts/down-the-rabbit-hole-a-deep-look-at-the-wo... for the recent 2024 issues with their update API.

This will likely be something those migrating from Wordpress will be more sensitive about allowing given the perceived risks.

🇺🇸United States cmlara

I'm seeing warnings in the interface and drush, just wanted to fix those.

That is usually error_reporting including E_DEPRECATED, often used in debug labs however not generally recommended in production or any other location that warnings are not significantly useful.

sYou are right :facepalm: sorry for the noise.

To be fair I’m surprised this made it into the Drupal standard, I really expected it to be more likely to make it into a Rector rule.

This will eventually get fixed (either in a new major or we make the decision to drop PHP7.0)

Just more pointing out that with existing automation this deprecation is already well know and we are attempting to avoid dealing with the routine credit farming that we see on D.O.

Even with the warning I wouldn’t be surprised to see one of the companies listed in 🌱 [META] Tracking issue for organisations who received educational messages Active do a drive-by of adding RTBC that the patch applies (gitlab already tells us that) and that the issue is solved (PHPCS already confirms that).

🇺🇸United States cmlara

I don't think phpcbf fixes this one, I wish :(

The sniff is: SlevomatCodingStandard.TypeHints.NullableTypeForNullDefaultValue.Null

The sniff is part of the Drupal standard published in recent druapl/coder releases.

The sniff is marked as fixable by phpcbf in the logs: https://git.drupalcode.org/project/s3fs/-/jobs/4029235#L119

Just trying to update a project to 8.4 and share it, don't need you to give me credit but I am

I belive you mean PHP9 (which has not yet been announced) as a deprecations do not stop the code from working in PHP 8.4,

🇺🇸United States cmlara

Also, the Update module is not collecting any kind of identifying information whatsoever, as far as I know.

https://git.drupalcode.org/project/drupal/-/blob/928a5cbd476d536530ba427...

It uses an HMAC’ed copy of the site domain coupled the site salt file.

In other words: a unique identifier that identifies an individual install. Likely equivalent to how a Customer ID is considered PII under various privacy laws.

D.O. Infra has said in the past the ID is recorded to remove duplicates call-in’s from the installed module count reporting so it is known to be recorded.

The identifier breaches some environments security policies on telemetry reporting. Calling home to get an update list can pass review. Once you include a unique ID (especially one that can be used to validate the validity of a leaked secret key) it pushes it over the limit in some security reviews.

Not uncommon in my experience for corporate installs of software to prohibit all telemetry (though by no means would I say it’s universally refused).

🇺🇸United States cmlara

Note:
This issue contains fixes that are detected by phpcs and automatically fixed by phpcbf.

This issue will not receive D.O. issue credit under Abuse of the Contribution Credit system .

🇺🇸United States cmlara

Postponed on 📌 PHP7.0 support Active being solved first.

We also might need to work around 🐛 beStrictAboutOutputDuringTests=true or failOnRisky=true causing test failures due to PHP8.4 Implicit Nullable deprecation Active to get working tests before any new code changes go in.

🇺🇸United States cmlara

Filing as a child of 🌱 [META] Increase Security of Project Ownership Transfer Process Active

Regarding #4 and #8
📌 Add ability for organziations to manage/approve contributors(employees) Active was opened to require orgs to approve users, it was rejected by Drupa Infra and narrowed to only allowing them to self delete users.

🇺🇸United States cmlara

Those users should be removed as maintainers post haste and the original maintainers should be warned to properly vet applicants before handing them the keys

What right do we have to override a maintainers decision?

Do I think TATA’s devs are qualified developers: No.
Do I think maintainers are negligent in assigning them any level of maintainer status:: Yes
Do I believe the entire adoption process system is a massive supply chain attack: Yes
Do I believe we as a community have a right to overrule a maintainer: No.

Certainly consider sending them a formal warning about vetting maintainers before blindly accepting them. Even possibly consider blocking any account that has gained maintainer level status (so that it can’t access any projects), while a cooldown is in place, however at the end of the day a maintainer made a choice. So as I asked to start with, as bad as we may think it is what right do we have to overrule them?

This is the balancing act between maintainer rights and dealing with problems we as an ecosystem created.

This is pure copypasta and I think it should be treated as spam.

Ill note it shows we finally got vendors to adopt standardized training processes with formal material. How is this copy pasta differnt than any of our other docs that say “copy this template and use it as part of the request”? Let’s be careful on shutting down vendor training.

By all means call it spam due to its bulk nature, or its use by unqualified individuals, but let’s be careful on causing the vendors to move away from centralized training repositories by complaining about the use of a template.

🇺🇸United States cmlara

Adding related issue 🌱 [META] Proposal: Track ##: Telemetry Active regarding Drupal CMS and Telemetry in general.

🇺🇸United States cmlara

cmlara created an issue.

🇺🇸United States cmlara

Adding the Questionable approval tag for tracking.

@pwolanin is not listed on https://www.drupal.org/project/projectownership as a user allowed to approve applications.

@pwolanin by own admission used their security team role to override their authority as a co-maintainer.

No note in this issue of having reached out to the official maintainer or of official maintainer being inactive for over a year.

@dieterholvoet requested co-maintainer not maintainer.

🇺🇸United States cmlara

🐛 StreamWrapperInterface realpath() and dirname() return docblocs are is inconsistent with documented use, actual Core implementation, and intention Needs work as related, it discusses some of the streamWrapper returns including LocalStream::getLocalPath()

Not reviewed the MR, however a general +1 to the concept to cleanup documented return types as this hurts contrib that run at PHPStan and will eventually mask errors in Core that PHPStan can detect.

Agree this isn't a Coding Standard issue.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

It appears we could still use a documentation related to this, moving to 2.x-dev.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches (as long as at least one skipped login is allowed).

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

There is already an open issue for the D8+ branches at 📌 Evaluate restoring using a form alter instead of extending UserLoginForm Active that can be used for further consideration of this issue for supported releases..

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-1.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of TFA do not have any additional planned releases.

The current recommendations for module developers not to auto-focus due to breaking screen reading software.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

Since the issue was created we have cleaned up cache management to invalidate Drupal pages that may have a styles/ link after a style has been generated.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

While I have no doubt that AWS being offline will cause issue with code that attempts to read files, I believe we currently capture exception and return them as file errors (the same as if local disk reads failed).

It is possible much code in contrib land will expect all file reads to succeed however that would be an error in that external code.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The requests in this issue do not appear to exist in the 8.x-3.x and newer branches.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

The 8.x-3.x branch explicitly calls out a protocol is required. While we could auto-populate this we would be forced to make an assumption on the protocol, generally it will be HTTPS, however not all sites use such buckets, especially when buckets are internal. That is extra logic that would have to be in every bootstrap until config validation can enforce a protocol.

I won't say there isn't a chance this could be added in the future, however in 8.x-3.x newer we have net seen it cause significant concern to justify the effort at this time.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Moving this to a feature request in 4.x. This wouldn't be too hard to implement into the file controller to issue a redirect.

🇺🇸United States cmlara

Drupal 7 end-of-life triage:
Drupal 7 reached end of life on January 5th.

The 7.x branches of S3FS do not have any additional planned releases.

Moving this to a feature request in 4.0.x.

This may be already possible, as it appears the endpoint errors raised may have been related to other known issues regarding configuration of the endpoint (protocol required) however that does not mean we can't consider adding this as an advanced option for ease of configuration.

Production build 0.71.5 2024