🇺🇸United States @cmlara

Linking 🐛 Full Setup not working on 8.x-1.7 Active as it proposed similar for the core portion of the patch.

does this really need blocking on additional tests for multistep forms?

I will note I would not be expecting a test to test everything, only that we end up up without being redirected back to the same plugin, essentially only needing to explicitly test this particular (that it doesn't redirect back to the same setup page).

I will give it a a bit more thought before giving a final decision on this, there is part of me that does want to cleanup bugs, and wishes to avoid excessive burden, there is also a part of me that sees this as one of very few faults that can arise from multi-step forms making it relevant as a 'first start to a test'

🐛 Redirect correctly after first time plugin setup Needs review seems relevant, once the code hits

 $this->messenger()->addStatus($this->t('TFA setup complete.'));

we should not end up on the setup page for another module. We may not need the wording change, although that may indicate there is still something else at play here that we should not be reaching that far into the code execution.

@smustgrave I was just filling in the record explaining why the tag and why no-comment since it was quesiton by a Project Ownership Queue admin.

Based on the statements from @avpaderno it is not policy that the permission is actually required so should not be a concern. There is some ongoing discussion in 📌 Create governance dcoumentation stored in repo/GitLab Pages Active to try and avoid this type of confusion going forward.

For context of others.
#19 was part of an attempt Discussed on Slack with @hestenet to avoid increased tensions on issues by only tagging them without comments that might tensions. @avpaderno was in the Slack thread where it was discussed.

The concerned being tagged in #19 was that @zenphp does not appear to have the "Can opt projects into security advisory coverage" permission and that the Password Suggestions (PUG) project is enrolled in the Security Advisory Program.

It has since been stated by @avpaderno in https://www.drupal.org/project/drupalorg/issues/3452333#comment-16095263 📌 Automate the majority of the ownership transfer process (retain human approval) Active that project moderators will not add as maintainer/co-maintainer, is treated by Project Ownership Admins to mean that Project Ownership Admins have the free will to approve/deny such requests and that there is no policy prohibiting such transfers.

Nothing of what you describe changes moving the documentation pages in the repository,

My understanding from previous conversations is that the pages are editable by even average users today (I can open the edit page, I have not tried saving to confirm), if so that would change as moving it to the repository.

to which any project maintainer for the Drupal.org project ownership would have access, especially when they are project moderators or have a higher role.

Are you saying we do not have a policy for when the document is allowed to be updated or are you saying that we can't trust the Project Ownership Queue admins to not make make nu-approved changes?

That documentation page has been created time ago to describe

If we do not think the current page as-is is useful I'm willing to help write something more governance like based on the current documentation published.

Should QueueInterface::claimItem() return mapping be updated to include the claim_count?
Can we add a phpstan return type showing the object shape?

Can we gurantee that the claim_count is available on all possible queue types such that putting this on the primary interface will not cause issues?

I don't see an answer to #9 regarding the metadata object lcass, if a dedicated object is not created can the new parameter be object-shaped to aid phpstan diagnosis of what properties are available?

Is there any edit which defaced the documentation page? F

Your text provides a key concern I have with these pages. It is being described as a "documentation page' not a 'Policy page".

I'm not aware of any deliberate modification to deface the page, although I am aware of lines that may not be written the best as they could be as they use phrases such as "Must" and "will" in a manner that appears to imply these are mandatory non-discretionary steps when other posts by site moderators appear to imply these are general cases, however not mandatory.

Without a formal policy I am unable to ascertain with certainty which of the two interpretations is correct in order to propose documentation edits.

In the past i have tried quoting from the relevant pages and been told these are not policy.

Offering to become a project owner, maintainer, or co-maintainer is not a policy, nor is a documentation page describing the terms of service. Please stop making false claims.

-- https://www.drupal.org/project/drupalorg/issues/3452333#comment-16094934 📌 Automate the majority of the ownership transfer process (retain human approval) Active
The page links both to

https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... →

At this point I keep receiving conflicting statement a large part of it may be a language barrier with the involved individuals. Having a well defined policy page with a tracked record of every change linked directly to the discussion involved (where not just the text of the policy but the thought process behind the wording is present) would provide more clear context to both understand the policy and to provide references when communications difficulties may be involved.

It is not necessary to move the documentation to GitLab Pages.
While perhaps not 100% necessary it would provide assertions on authenticity.

Drupal Core governance has done this in 📌 Make gitlab pages for governance.drupal.org Active .
The coding standards group is considering the same in 📌 Convert Coding Standards to GitLab pages Active .

I am a fan of governance policies in repo as well

- Tim Lehnen
CTO of the Drupal Association
https://drupal.slack.com/archives/C5ABFBZ5Z/p1746562504563009?thread_ts=...

greggles → credited cmlara → .

greggles → credited cmlara → .

greggles → credited cmlara → .

cmlara → created an issue.

Re: #20 I believe that would need a large D.A. policy discussion first, separate from this issue.

Though I will warn I would suggest the D.A. refuse to accept such a policy. The Drupal Community lost uncountable hours of possible contributions in the old system. The long term problem of "unable to find maintainers' can likely track some of situation back to those early policies.

gábor hojtsy → credited cmlara → .

No further discussion has occurred on this issue.

Closing this as Works as Designed as the 3rd party contrib module has made changes to the Image Generation process.

I wonder if this will solve that too.

Thinking deeper my issue would likely not be solved by this solution. The module would still be disabled so code would still not exist in the auto loader even if the process occurred sooner.

I believe there is an issue(s) somewhere..?

I thought I saw something in the past, however on a quick search the only issue I turn up is #2536610: Ensure all modules are autoloaded with PSR-4 only if enabled → advocating for the exact opposite.

fjgarlin → credited cmlara → .

I'm going to use myself as an example.

I went through the Vetted Process in #3247119: [D9] S3FS SNS Metadata Update → .

Never during the process was I instructed to sign an agreement or even asked any of the following:

• Do you believe you know how to write secure Drupal PHP Code without being prompted?
• Did you personally write the code being reviewed?
• Do you agree to diligently review your commits for security concerns prior to submitting them to D.O. (
• Do you agree to work with the Security Team to resolve issues in modules you maintain? (one could argue this was implied somewhere, however it was never actually explicitly asked when applying for the role.)

During the process is D.O.'s chance to establish a 'contract' with the applicant.

Admittedly none of those questions actually stop me from doing so in the future, however the fact they were not asked I would contend is a an indicator how D.O. does not expect me to act in a special manner going forward, that D.O. failed to reaffirm its expectations of me at the time of role grant.

These are just examples based on some of the limited comments, and why I mention that each revocation should inherently trigger a process review, not every incident will identify a part of the process that can be improved, however those that do will help better define the program and (hopefully) reduce the number of incidents.

Looks good.

I did add the new page to the menu above exempting authentication provides.

Minor suggestion on possible text change to be a bit more agnostic on providers provided in text.

Please stop making false claims.

I am referring to the following statements where users have been refused project transfer for not having the security opt in permission.

The project is covered by the security advisory policy; project moderators will not add as maintainer/co-maintainer, or project owner, somebody who cannot opt projects into security advisory coverage, as in this case.
I sometimes contact the maintainers, hoping they will reply back accepting the offer. They are the only ones who can decide to add as maintainers/co-maintainers people who cannot opt projects into security advisory coverage, though.

— avpaderno
https://www.drupal.org/project/marvelous/issues/3492162#comment-16028940 💬 Offering to maintain Marvelous Theme Active

Project moderators will not add as co-maintainers/maintainers people who cannot opt projects into security advisory coverage

— avpaderno
https://www.drupal.org/project/simple_cron/issues/3453929#comment-15651081 →

@cmlara You are right. I thought I already checked that before, but I obviously got confused.

@Chandreshgiri Gauswami I have to remove you from the maintainers list, for the moment.

— avpaderno
https://www.drupal.org/project/adminer/issues/3441835#comment-15594906 💬 Offering to maintain Adminer Active

IIRX I’ve hit something similar, in the past.

IIRC I was trying to use a decorator with autowiring and an “ignore if service doesn’t exist” attribute for an optional core service. However since the core module wasn’t loaded IIRC the references to the class caused similar faults.

Maybe that’s not in scope here though I wonder if this will solve that too.

I wonder is this is a sign that Drupal should consider adopting a “more standard” composer layout of having modules always register their namespaces into the composer autoloader instead of the Kernel registering enabled modules?

Obviously adjusting the Kernel would make more sense here as an immediate fix, I’m just spitballing “long term” if it might be time to let composer handle more of this.

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.

The proposed automated gates in this issue would reduce the risk of such incidents.

After talking with the team, we are curious about the “open issue for logging of nudges.” Can you provide a link

#3177770: Nudge reporting(?) → which already attached as a related issue.

My suggestion would be without doing a proper post incident analysis of everyone involved (those warned, those requesting the warning, and those who only observed the warning) the CWG is missing out to understand if the nudges are actually having a positive effect.

I only observe a fraction of the nudges, its possible a majority of the time they have a positive outcome, however without doing proper, data collection an analysis none of us truly know if these should continue in public, moved private, or just be discontinued.

Anecdotal observations:
I've seen a number of incidents where it does inflame the tensions, perhaps not in the thread itself, however every other thread after that will remember the incident. Humans hold grudges and those grudges will make every single discussion after that more tense.

Worse than that, I recall an issue having been silent for an extended period of time when a nudge was posted, this added 'fuel to the fire' of an issue that was otherwise no longer being discussed. Had the nudge never been sent the issue would of likely been forgotten, with the nudge it was remembered much longer and not in a positive view (refer to humans hold grudges).

📌 Bulk LLM-generated module publishing by bigbabert Active may be an example of a 'too soon' nudge, and the chilling effect it may have. Never under estimate the damage of "the thread goes silent". I have observed too many times in my life where continuously internalizing problems has disrupted projects.

📌 Drupal core will require Queue Factories to implement QueueFactoryInterface Active would be the QueuFactoryInterface issue.

The problem was solved by changing:
(I'm not sure why the previous name doesn't work, might be another bug)

That error sounds like an issue related to Drupal Core moving to closures.

That may be a more significant problem as it implies the queue factory no longer can access our services added during the container build (I hit something similar on another module). The container build process will need to be investigated. The workaround shown will work for the default queue, however adding specific non defaults will be a likely trouble spot.

if it is deemed unsupportable by the security team.

Is this related to the recent LLM discussions?

What would make a project or user unsupportable in light that the DST is primarily a clerical position? The only item I can think of is “excessive reports”?

Removing projects:
The security team is impacting site owners more than they are impacting the maintainers. The rules for this I would assume would be limited in order to protect site owners.

The Drupal CNA may still need to process a request for an advisory for unsupported projects, refuse it and process a MITRE dispute response (in other words the Drupal Security Team may create significantly more paperwork for itself rather than reducing the paperwork when they revoke a project).

Removing Vetted Users permissions:
I would assume the allowed reasons for this would be fairly limited, the only one I’ve seen published so far is when a user fails to respond to the security team leading to a module being marked unsupported the Security team may revoke the access (not sure which of the pages I saw that on).

Wouldn’t users generally be operating within the context of the permission? The Vetting permission is suppose to determine if a user is a “qualified” developer with a knowledge or secure PHP coding principals.

Any revocation of the user permission raises a question of “how did the user pass vetting originally (and how will the vetting process be updated to prevent it from reoccurring)”.

I do no longer see that part in my user profile, and I cannot see that part when editing other accounts.)

IIRC that was moved to GitLab as part of migration and GitLab now prompts on first use (or presumably whenever the policy inside GitLab is updated).

@rajeshreeputra

That seems significantly out of scope for this issue.

The latest MR makes the API problem worse.

Adding type hints and return types this would generally need to be a new major only change as it breaks compatibility for those of us who have built modules implementing these interface.

Suggest addressing on the feedback from #9, that rather than assuming the API is wrong perhaps the code itself is wrong.

Consider the fact that most of ecosystem is likely not checking for a NULL return (due to API spec) and allowing it does r make sense. An exception would be better since it should be an abnormal condition.

Has any post nudge surveying been done? I see there is an open issue for logging of nudges however it appears to never been moved forward.

Would suggest data from those directly involved in the thread, and those who are observing the thread would be useful.

Some questions that come across my mind:

Do they actually defuse the situation or do they raise tensions in the threads?
Does anyone feel that a CWG Nudge was used too soon in a discussion?
Are contributors more aggressive out of concern a nudge will be given on a sensitive topic shutting down discussion before a point can be made?
Are nudges used by 'less experienced' developers or only used by 'well established public figures' of the Drupal Community? (Are they being used to shut-down discussion creating a non-collaborative environment.)

I'm sure others would have suggestions for questions that could be considered in a post incident survey (say 7 days after a nudge is delivered)

This method can in fact accept arrays for use with multi-value keys.

Can it ?

KeyTypeMultivalueInterface calls for a serialize method to be present which allows converting the multiple value to string. To me that would imply that these should only be strings by the time it reaches the storage layer.

This sounds more like the referenced project was actually using the Key module outside the API and PHPstan correctly flagged it.

Needs work for method signature change, though I believe this is more likley works as designed.

From MR comments it appears this should be Postponed on 🐛 KeyProviderInterface Active as there is question if NULL is an acceptable return type.

It is used in one location in existing code however is not documented in the API to be allowed.

Until a decison is made to update the API new deviations should likely not be added. (Avoid creating more work for others if the API is correct.)

The current MR proposes adding return types to the method that conflict with the interface.

The alternative here is that the interface is 100% accurate and the methods returning NULL/FALSE are wrong.

I will note there is no API documented method to indicate an error, it appears the interface was written with the assumption of success always occurring.

Updating title to better reflect the status of this issue after #14's creation of policy by D.A. staff.

I am not sure why we mention it as required, but it probably should be changed to recommended for now pending policies.

That appears to be a direct pull from the Issue Etiquette page .

Hestnet added basic policy in comment #14 to the Cresir A use policy and added the Etiquette page text.

Discussion on this issue switched to “does the policy need changes” after that post.

For the 2.x we are embracing using GitLab Pages for the documentation/manual. This is rendered by mkdocs from the markdown files in the docs folder and published on each 2.x pipeline execution. It is also currently included in each download for offline viewing.

We likely should place this somewhere in the docs (there may be no great location for it at the moment as they are sparsly populated, a new section may be necessary).

I'm going to set back to NW for including in the

For REST logins I would suggest sites I would recommend the site owner consider using an API token auth provider (unless we are talking about the post login to obtain a cookie).

This will impact password access that uses the user.auth service to validate a use. Known scenarios this is relevant for:

• Password confirmation forms that do not check the database directly
• HTTP Post to /user/login (this is always enabled in Drupal Core).
• http_basic authentication

Given the above not sure it if makes sense to limit this to just the rest module enabled.

To me it feels a bit unusual to inline this, though at the moment it is indeed not documented anywhere except in the deep issues logs.

Note: I am in both the linked Slack threads, I am one of the individual who first hand saw significant flaws in minutes of glancing at the code. I am in full agreement that Advanced File Destination is unsuitable for install on any site in its current condition. I agree that other commenters made points that raise reasonable questions regarding Static Node being functional for the described purpose.

Adding a bit of complication to this discussion:

The individual has published a few other AI written modules in the past, I make no comment at this time in regards to the quality of the code related to the modules in discussion here.

Projects with Stable and Security Covered releases:
https://www.drupal.org/project/auto_translation → - 290 reported installs.
https://www.drupal.org/project/graph_element → - 10 reported installs.
https://www.drupal.org/project/jw_player_media_source → - 6 reported installs

Projects enrolled in security opt in with no stable release:
https://www.drupal.org/project/content_reporting → - 42 reported installs

I think that revoking git access and unpublishing the projects temporarily might be warranted

I certainly understand the desire to stop the publication of significantly buggy code before site owners install it and to avoid new code from being published.

I am however a bit concerned that this could lead to a repeat of community disruption that was discussed in #3094854: Plan for dealing with projects that may be abandoned → .

Is there a plan to minimize the impact? Thankfully these are smaller install count modules compared to the previous incident, however that does not help the unlucky site owner who is using the module and finds the maintainers unable to commit code.

Thanks @scott_euser.

I should also put a note in this issue that I have mentioned in a few issues now.

I see many of the "Should have" list as long term essential, however in the past year since this list was originally started each new vulnerability has given more insight into limitations of the 8.x-1.x branch that can not be resolved without the 2.x branch.

While the "Should have" list is long and incomplete at this time, it may be time to consider focusing in on narrowing the 2.x target to more essentials, releasing 2.x (through an alpha/beta/stable) and opening a 3.x branch for additional API breaks.

Speaking as a dev who targeted D8+ while other devs were responsible for the D7 releases:

Some contrib issues never were assigned to D8 releases and only existed in D7 branches. After EOL I went through my modules issue by issue and migrating several still relevant into the active release branch.

It would seem treasonable to suspect similar to exist across the ecosystem which makes closing contrib issues far more questionable.

If contrib issues were to be closed I highly recommend they be given a unique issue tag so that a future developer can easily query them without needing to sort through unrelated issues.

First off: I'm aware of the user and modules refereed to in #23, and agree they are poorly written (to the extent I've added them to an internal list of modules to not to utilize and maintainers to avoid).

These modules should be removed from security coverage and their publisher needs to lose their security coverage privileges IMO perhaps based on a strike system.

I am weary of this as an option. Responsible/Ethical/Coordinated disclosure is a key aspect of security. It would be one concept if D.O. did not use wording such as Security issues do not need to be privately reported for the module_name project., with this wording D.O. is actively encouraging public uncoordinated disclosure for those not 'opted in'.

Any action D.O. takes to remove the ability for maintainers to compel private disclosure is an affront to security. D.O. requiring a test and to remain in the 'good blessings' of the Security Team would be conflict with security industry norms.

To put this in another light, how would the community feel if I demanded the Core Team pass an arbitrary test that I personally create, with a drawn out process of months to approve with the ability to revoke based on a condition I decide,, before I would cease disclosing vulnerabilities publicly? I presume the community would be very upset, especially in the case of a Drupelgeddon level vulnerability, and yet that is the standard applied to contrib maintainers today that this suggests extending.

Whatever that occurs for a policy regarding AI generated code there should not be considering the compromising of security privileges in the equation. Consider LLM code spam, consider LLM code unacceptable for commit due to copyright laws, consider it unwanted and target the user on those grounds, whatever is done, do not make a policy that would compromise the ability to request private disclosure.

Could you please let me know if there's anything I'm missing that would enable me to provide a file path located on a different server or outside the Drupal project scope?

Using the S3fs module with the file at s3://test.txt would be an example that works key when this patch is not applied. Once this patch enforces realpath I would expect the following to no longer function.

> file_put_contents('s3://test.txt', 'testkey');
= 7

> file_get_contents('s3://test.txt');
= "testkey"

> Drupal::service('key.repository')->getKey('s3key')->getKeyValue()
= "testkey"

@srtmdev I suggest reading the links in #3

Specifically in the what to expect:

Unfortunately, the application queue does occasionally experience a large backlog, and applications may sit in the queue up to a year before getting reviewed. 

There is no Service Level Agreement(SLA) in this process.

The application will be processed, when is a 'wait and see', it could be today, it could be several months before anyone makes another review. Following the priority steps listed in the links given may reduce the amount of time waiting, although there is no gurantee it will.

Note that the process does not enroll your project in the security advisory opt in process, it tests a developer(yourself) to have the permission to do so. It is not the module being tested here, it is yourself as an individual and if/when process completes you will opt the project in not the reviewers in this queue.

I'm assuming based on your issues that your primarily doing this so that the company module is not subject to D.O. suggesting users publicly disclose security vulnerabilities, if so you are unfortunately caught in a portion of D.O. that is not very corporate or security industry friendly and will just have to 'wait it out'.

I will suggest something along the lines of paratest may be better option for the performance gain and that we phase out the use of run-tests.sh in a v2 in the templates, (or add its as a 3rd option that is preferred).

Especially for those of us who use code coverage that is our primary reason for not using the run-tests.sh job.

I've heard of some issues with paratest in the past though I do not believe I hit any game stoppers in my testing. IIRC I did run across some phpunit options not supported however I believe they all had workarounds. I need to restore working on that project again.

Long term in my eyes run-tests.sh is non-viable, as it is harder to push fixes for.

As a site owner, I'm going to miss Parentheses, Ampersand and Space., A little less Brackets, Pound Sign and Exclamation Point though I encounter them.

As a security engineer: This is not a bad hardening, however it is certainly not a fix for wherever the faults actually occur.

As someone who has used this in the past to perform sample RCE's against site setups; I can say yes this would make it much harder to exploit.

I also wish to reiterate that for these characters to cause issue someone has had to make some very serious mistakes down stream, mistakes that this change likely does not actually remove the vulnerabilities , just at most makes it significantly harder to exploit (move from Basic to Complex, saving 1 point on the Drupal Vulnerability Scoring ) and maybe move the Impacted Environment from All to Uncommon (2 points on the Drupal Score scale).

I agree with @smustgrave the 'excluded' ideas would be nice to have listed for historical purposes.

IIRC the reason to keep only latest image is security because distros getting updates

Although the image is avaliable doesn’t mean a site has to use it. We see this all the time on DockerHub older releases are kept so they can be referred to in the future.

This is analogous to arguing all the Drupal 9 core releases should be deleted due to the fact they are known vulnerable.

and PHP has monthly release cycle, so no reason to bloat storage...

First off, retention and semver are different.

Semver would be about ensuring images are generally the same and compatible between build, PHP majors and minors. This could be done even with the current system of never retaining anything except the latest build.

Additional one could set a retention window for images (6 months of same “major” and at least the last major of a semver for a few years?) to reduce storage bloat and yet still allow retaining images.

I will also add that shared storage of being in the same repository could allow image layer reuse further reducing size (this would require streamlining the builds a bit).

Pulling note in from Slack:

I recommended evaluate including both versions of the package on the images (since these are PHP Extensions this should be fairly straight forward) and possibly add a helper script to allow switching versions.

DDEV does similar in its image design that all the versions of PHP are present and a script sets the active version.

I will note that this is an example of what #3404082: Adopt (semver) versioning for DrupalCi images → is trying to address.

Major changes to the CI images that are hard for contrib to work with should generally not be done to existing images without some way to differentiate them. Including both versions and allowing a test run to 'opt in' to the newer version would better align with having well designed and versioned images.

We can use the realpath() method of FileSystem.

Given the following documentation of the API Method can you provide more justification?

    * The use of this method is discouraged, because it does not work for
     * remote URIs. Except in rare cases, URIs should not be manually resolved.
     *
     * Only use this function if you know that the stream wrapper in the URI uses
     * the local file system, and you need to pass an absolute path to a function
     * that is incompatible with stream URIs.
     *

— FileSystemInterface::realpath()

Does the key module know that a path is a local filesystem? Is this a rare case? If so why? Does the module need to pass an absolute path to a function? If so which function only accepts absolute paths?

Triaging older open Merge Requests:

I'm inclined to believe this, as currently proposed, should be a 3rd party module and not included in the base code.

I tend to align that plugins should be kept outside of TFA to allow more flexibility and allow TFA to focus primarily on API.

Is there a strong use case that the overwhelming majority of sites would use this feature?

Side note: Also consider IPv6 when this is implemented.

Only sets the status to enabled in the user settings if a default validation plugin is configured and the plugin has been configured

See #5. We do not want to force that a single plugin must always be used.

Only shows the number of times validation skipped text if TFA is enabled for the user's account

See #3. We absolutely do need this to be present when TFA is disabled for a user.

Duplicate of 🐛 Deprecations PHP 8.4 Active

Additionally, the patch file workflow is no longer utilized on D.O., submissions should be made using MR’s. See https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... → for more details

While at it we can cleanup the PHPCS, disable the nullability rule on the 1.x branch as we may not be able to implement it, see 🌱 PHP 7.0 Support Active .

And cleanup 2 warnings caused by PHPCS not recognizing a cspell line

cmlara → created an issue.

Committed to dev on 2.x and 1.x.

Thank you again for the report on this issue.

Given the data from #3 (I was under the impression packagist would use releases) it seems reasonable to won't-fix this and instead find (or create) a separate meta to remove packaging customization.

Please take that into account that while the comments below are not intended to be an 'alarmist' reaction, I am well aware that this particular incident is minor at the moment (and overall is one of the least significant I've encountered in my history with software), the goal is to ensure it does not become more significant and to ensure any future incidents also have a well grounded base in place to avoid them possibly becoming significant.

Should we not seek an opinion from a the licensing working group?

I would suggest no matter what the Core Team/The Project Lead (Dries) will want to run this past legal to ensure they properly unwind this to ensure no future legal entanglement.

I don't think so. This is an existing issue that has already been released. If it would be the first release with it in maybe but that's not the case.

The phrase "Knowingly, willfully, and repeatedly" could be used to describe publishing additional releases or commits after the core team is made aware of the issue. It is one thing for something like this to happen without knowledge there is some room for compassion and more lenient mitigation.

The Linksys GPLv2 case is one of the most notable example of case law on this subject. The platform went from being Closed Source to Open Source GPLv2. An equivalent scenario for the Drupal Project in this case could be that the code no longer qualify as GPLv2 and would be GPLv3 only, not the worst situation that could happen, however I also suspect this is not desired by the Core Team.

Agree, let's just remove the offending code for now, we don't strictly need it here.

Seems reasonable.

Saw blog post that the next Alpha will be released in the near future.

Should this be classified as an Alpha blocker and release blocker?

The monthly security window I believe is tomorrow, doss this have any impact on that window?

CR created https://www.drupal.org/node/3519175 →

Responded to MR comment.

wondering whether this needs to be a deprecation (or unsilenced E_USER_WARNING) changing to an exception in 12.x

My personal opinion is no. I can understand why this would be asked since this can be disruptive and given by the code being corrected there is clear proof that some implementations have misused this

However the API is well defined, the proper response is an exception. Code that has properly implemented SerilizationInterface::decode would already have a try/catch block surrounding it.

Today 3rd parties could replace the decoder using a class alias at the auto loader meaning the code that is broken by us fixing this can already be broken by a contrib module following reasonable design standards.

The FALSE return was/is an internal component of the JSON decode method that leaked and should not have been utilized.

As noted in the issue summary, this failure to throw an exception as documented on the API can lead to undetected data loss for those applications that properly follow the documented API.

This bug fundamentally breaks the ability to use DI of the serialization services.

I have no proven case to show this however a worst case fear is that silent data corruption could lead to security incidents.

Note: Some answers have been provided to this request in Slack #contrib-tfa https://drupal.slack.com/archives/C7SR7TWMS/p1744252987515989 and are duplicated below.

My personal recommendation is that 2.x should not be run by anyone except though who are developing for the next release.

My personal opinion is also that we should not be cutting releases for 2.x until it both has all the known security fixes/hardening committed and any major API overhauls have been completed. Some of the other maintainers have expressed differing opinions encouraging more frequent releases, some of this can be seen in #3416791-6: Resolve SA-CONTRIB-2024-003 in 2.x branch → Comment #6 and newer.

I would likely want to see 🐛 Insufficient entropy in loginHash generation Active completed as the security evaluation for it was done before committing 📌 Use an EventSubscriber to process one time login links Needs work , and while I'm not sure there is any more risk now I haven't actually done a full in depth review, rather than auditing the line by line just hardening the hash is preferred.

That said there is starting to become a good argument that it may be worth cutting a 2.x before a full API is established with the intent that a 3.x branch be opened up to continue API breaking changes on the roadmap 🌱 Roadmap for 2.0.0 release Active to get away from fundamental implementation flaws that can not be fixed in 1.x.

Added a few still open issues into the roadmap

No, there have not been 19 approvals in two hours. There have been 19 issues whose status was Fixed which have been edited.

Fair, point.

I had performed a random spot audit on the list and all the ones sampled appeared to be NR->RTBC->Fixed in that window.

I have gone through the list and there may have been a 5th that was already resolved prior to yesterday.. Additionally while reviewing I did notice (however did not account for time on every single issue) that one of those may have been reviewed several hours prior as well before being marked fixed in that ~2 hour window.

See also comment #17, to which none of the reviewers replied. It is not that an application is hold on because no reviewer has been able to make a correct review.

I am a bit confused as to what is being said here. Can you clarify for me?

I'm not sure if you are saying saying that it is policy that an applications will be automatically approved if its sits for an extended period of time without review, or if you were saying that applications should be held until they are thoroughly reviewed.

Apologies for the noise.

Adding a tracking flag for auditing when modules have been reviewed by the queue only for it to be announced after approval that reviewed code contained a security vulnerability.

SA-CONTRIB-2022-051

Apologies for the noise.

Adding a tracking flag for auditing when modules have been reviewed by the queue only for it to be announced after approval that reviewed code contained a security vulnerability.

SA-CONTRIB-2022-046

Per SA-CONTRIB-2025-030 versions older than 1.1.0 posses an Access Bypass and DoS. These versions would have been part of the review process.

Tagging as “Vulnerable when approved” for tracking purposes.

It appears this has self resolved.

I assume it was some form of library that was updated inside of the mkdocs plugins.

I usually do not like to object in this queue as it is used to provide access to the security opt in feature which should be a common feature of all projects on D.O.

I am flagging as a questionable approval for future tracking purposes.

As noted in #17 the module is essentially an exact duplicate of an existing module.

Looking at the number of approvals in the queue today (19 approvals in 2 hours) I am concerned that the queue maintainers may have rushed through the process after the queue was allowed to develop a sizeable backlog.

I am uploading a diff between the two modules for posterity.

The majority of the changes are:

• Twig changes of image sources from /modules/contrib/a11y/... to /modules/contrib/accesstools/... (itself likely being wrong since it presumes the module will be installed at the root of the server and under modules/contrib)
• PHPCS Changes
• Additional lines in hook_help()
• A couple of variants of using the logger to record errors.

Nothing further needed. Just had sent it to tests last night intending to followup after they finished.

No need to port to 2.x as we do appear to use use the library there.

Committed to Dev.

Thank you for tracking this down.

I have put some comments on Slack however making them more formal:

As a Contrib maintainer I do not see the value.

For me PHPStan exists to identify bugs in code, not future compatibility problems. Not having a return type when the parent method itself does not have a return type is not a bug.

PHPStan scans your whole codebase and looks for both obvious & tricky bugs. Even in those rarely executed if statements that certainly aren't covered by tests.

-- https://phpstan.org/

Looking at a few of the projects I maintain there is a good chance there will be a different branch for D12 and my current branches will never need to implement a hard return type (although in many cases I already added strong code typing as part of implementing Level 9 PHPStan).

A 'feature' like this will just be noise to me, and distract from working on issues more pressing (security hardening, bug fixes, and features).

My opinion is that 🌱 Consider always having the next major branch open Active is the better method to implement, it would allow modules to opt-in to scanning against the next major core branch early to identify where they need to make changes, and reduce creating future technical debt (no need to come back in D12 to remove the type-hint warning).

As an aside. I have suggested in Slack that if this is felt to be useful it might be worth considering putting this in its own third party repository to allow it to be available to others as a way for the Drupal Community to "give back" for all it receives. If this does not make sense to be an independent project I would suggest that should be treated as a warning that indicates other methods should be preferred.

Looking at the MR itself: I note the presence of // @phpstan-ignore phpstanApi.method, should code that uses PHPStan off-api be allowed considering the maintenance burden that could create?

Edit: Correction on the provenance, it started as CC BY-SA 3.0 not 2.5 (no change in statements above regarding compatibility concerns)

cmlara → created an issue.

cmlara → created an issue.

The motivation to release the information should be separated from publishing CVEs.

CNA rules
5.1.7 a CVE MUST identify the type of Vulnerability.
5.1.1 SHOULD contain sufficient information to uniquely identify the Vulnerability and distinguish it from similar Vulnerabilities.

5.1.7 is the one that likely puts us at most risk It requires we disclose at least a vulnerability type in the CVE. We can get away with just saying a basic (sample I have not looked at this particular advisory lately) "Remote Code exploit", "XSS" , "Authentication Bypass" or other similar category to maintain that compliance, however even to do that we have to disclose that specific bit of information which should likely be done under D.O. coordinating its disclosure of more details (why hide on the D.O. SA that it is a specific type when an attacker can go to the CVE to find that out).

5.1.1, we need to have a very good reason to refute why we do not align ourselves with what we are instructed that we should do. Will the CNA loose its contract for failing to comply with a should, no, will they loose respect in the industry, possibly, will they waste other organizations resources, likely.

We published sa-contrib-2024-075 as an unsupported module with CVE-2024-13311. This report does not appear to uniquely identify the vulnerability or indicate vulnerability type.

Looking at the enrichment data for CVE-2024-13311 if I am reading it correctly it appears that CISA was unable to provide full enrichment due to "not enough information".

Being flagged as "not enough information" should itself be a warning to us that we need to work on our disclosure process.

it's possible to create CVEs without specifying the CWE/CAPEC/risk score, so the title and some of the description could be updated to reflect that. It may be desirable for some to know that information.

Can you clarify what you are trying to say? You want this issues title updated? Or do you mean that that the title for modules previously published as "unsupported module" should be updated?

lostcarpark → credited cmlara → .

SA-CONTRIB-2025-023:
Curious how documented in the Security Advisory by the reporter and fixer as CAPEC-115 became CAPEC-85?

Disables users' ability to set up validation plugins except for the default validation plugin if the default validation isn't set up.

I believe we ruled that out in #5.

Even if we were to implement it, that is purely a TFA issue, the plugins should not be involved. Especially not injecting the @internal TfaPluginManager. While TFA could call that service adding it to the TfaBasePlugin would put the internal service into external plugins.

Last I understood we viewed putting default plugin first in the list as reasonable and we were open for researching if it would be possible for Recovery Tokens to not meet the "tfa is configured" provision (without breaking the edge cases such as a user needing to run on recovery tokens while awaiting for new tokens to arrive)

Does it mean that we can't merge https://git.drupalcode.org/project/rabbitmq/-/merge_requests/58 ? These changes are quite straightforward and allow the module to work in Drupal 11 as well.

It means that at this time I do not feel comfortable merging any more commits into the repository without a clear approval from maintainers added through a less ethically questionable process.

I've reached out to the current owner (who was seen active a few days ago) and have started looking at the fork process as an option if needed.

Could we start by looking at how we would change \Drupal\Core\File\FileSystem::tempnam() to not use ::getDirectoryPath()?

::realpath() of the scheme would accomplish the same outcome.

Do we need more strict type checks for LocalStream?

If using ::realpath() no, just validate the return is not FALSE. This allows classes that may implement local storage, however not extend LocalStream (such as extending a 3rd party framework to interface with core) to still function.

This sounds like you've identified a new Feature request for the module. Maybe a test encryption plugin that only gets enabled via a drush command to make testing in general easier?

One already exists as a testing module and we use it as part of unit Functional tests, however Drupal must be configured to use such modules on 'production' sites (not UI accessible to my knowledge).

After this is merged, on UX issues it can help people see the change visually or on multiple devices which screenshots can't really do.

We have very little in the way of UX issues directly, and those that do touch UX do not have any real customization to them, renderings would generally be controlled by site themes and the Drupal Core API meaning we have little need for multi-platform testing on a daily basis, if something is broken it is (in the current code) most likely going to be transferred to core as a FormAPI flaw.

When you are working in your "lab" setup, how easy is it for you to preview the changes on your mobile device?

Fairly easily, un-comment a port setting on the lab config and the site is directly exposed, even easier when its on one of internally routed dev machines where it has an internal hostname assigned to it (although that is rare).

Tugboat previews makes it very easy. It makes it easy for others, without extensive lab set-up to see it too.

That is the part I am wondering how useful it is, I'm involvded in many of the deeper issue so perhaps my vision is a bit narrow. I haven't kept any statistical logging of how often I reached an issue that would be acceptable to approve on just a visual without a lab setup.

Doing so means all co-maintainers and any new co-maintainers would have access to the power and benefit of your local lab type setup.

Fair, though these would likely involve a little more complex of a setup as some of the data is to my knowledge not exposed via direct commands. Nothing impossible to do, just another maintenance point to consider if the burden is worth the return, which is the root question, how often tugboat would actually help move issues forward, if it is is frequent enough it may be worth the maintenance burden

I think you are right, usefulness will differ module to module so it really up to you and the folks who are invested in this module.

The simple community question would be "What issues have you worked on in the TFA queue in the past year or two that Tugboat would have allowed you to avoid a local lab and expend less time/effort testing , and in what way would it have done so?"

Critically there is no encryption plugin installable through the UI which renders TFA impossible to setup and test inside of Tugboat. NW for resolution

As a maintainer it would be great to have MR environments so I can save tons of time evaluating MRs without needing to pull it locally.

As an actual (co)maintainer, I wonder how useful this is:

With previews only existing 5 days and most issues taking longer will we see an increase in issue noise? (although that may have just increased to 30 days in 📌 Increase Tugboat Preview expiration to 30 days Active , better but still possible noise for longer lived issues if users are closing/opening mr's to generate previews).

Is code updated when new code is pushed? If so are these new environments or continuations of the previous environment?

In this basic form there is a decent amount of setup that still needs to be done from an admin before they can test much/any of TFA's code. Add an encryption key, setup an encryption profile (which requires an encryption module), before TFA can be enabled, add a user, add a token, etc.

As a maintainer In the amount of time that takes I can (if at a terminal) easily checkout the MR and use an existing deployed lab to test (for context in my local lab I have multiple users, multiple tokens, multiple token types, user roles, and requirements setups configured to test most usage scenarios). There may be an advantage to this when I'm on a mobile device without being near a development terminal.

Presumably some of that setup time could be reduced by configuring the environment inside the template.

Beyond that I'm trying to think how often would I use this. I often review code first and then load into the IDE to validate API usage, sometimes with a requirement for a breakpoint in XDEBUG. To perform those operations I would already have the code in my local lab where I can view the changes.

How often would users use this? How many that do use this it will perform an actual test rather than a "it loaded" test which provide us little to no validation. We are not a theme, our issues are not usually visual they are technical. How much of our code is testable without edge cases that require server changes?

I don't have a good answer for those questions. Leaving this open for opinions from other maintainers and community on if tugboat adds value for our workflow.

Unable to reproduce and the reporter has not provided more information.

I will also add on that had this been able to be traced to an actual bug it likely should have been reported as a private security issue.

This project is in a bit of a weird spot right now.

I was added as a maintainer through the accelerated adoption process some time back.

That process (along with the entire D.O. adoption process) is subject to some rather significant ethic questions as it is essentially a social engineering and supply chain attack.

Factor in that one maintainers returned for a short period of time last yesr (before disappearing again) raising questions with some of my design choices and it raises even more ethical questions about my right to commit code to the project.

The project has been heavily on hold since those incidents.

I’ve been pondering my options, at the moment the following two look to be the most likely paths:

1. Owner transfers the project of their own free will (do not use the Project Ownershop Queue) (may not happen due to non responsive maintainer)
2. Fork into a new project.

It’s a situation that needs to be sorted out for this project to be able to continue on.

Side note:
I’ll note no one has actually tested the code above and reported back.
The user rajan kumar@2026 is well known for not testing code/credit farming leading to disciplinary action on D.O.

The other options beyond rolling back the API is setting the environment variables or placing the configuration parameters in the shared config file (not the credentials config file).

I have read that CloudFront R2 is also impacted

Minio and Ceph appear to have support for this header (as does LocalStack that we use for the GItLab pipelines) which means the impacted hosts should be limited in general.

Merged to 8.x-1.x

For 2.x we do not have the same concerns for design, however we can learn from 8.x-1.x fix and consider adding hook_requirements () checks for our core security concerns.

TfaUserSetSubscriber::class would be our prime candidate to monitor, to warn if another subscriber is before it so that site owners can validate security. We would likely want to implement this with an 'approve' list so site owners can approve a subscriber to move it from warning to notice.

We may also wish to monitor the logon routes unless 📌 Evaluate restoring using a form alter instead of extending UserLoginForm Active is adopted, however unlike 8.x-1.x where that monitoring is security critical in 2.x it (should) be only advisory as TfaUserSetSubscriber provides the backend security.

Was primarily allowing a little more time for anyone who might want to provide feedback since this decently significant code.

This is one the areas that has had security vulnerabilities in the past, including 2 weeks ago (note: the report that triggered two weeks ago would not have been allowed either with existing 2.x branch or the 2.x branch and this code.

That said I believe this is ready to go in and anything else can be followups.

Letting final test run occur after removing unused code and will merge.

Regarding Simple Password Reset:
Recent data indicates Password Reset Landing Page (PRLP) may be better aligned to API and provides a similar feature set. Currently PRLP does not work with this code however PRLP may be in a better spot than Simple Password Reset to modify their process to work with TFA.

cmlara → created an issue.

Looks like this was sitting on my machine waiting for the deprecation bug fixes.

Uploading current progress.

I’m not seeing any explicit notes about cached items expiring and being cleaned up, we will want to make sure it's not a runaway cost.

IIRC Gitlab currently expects that the storage layer will handle this (add lifecycle rules in S3).

Considering the propose config change has been waiting since September of 2023:

What is infras opinion on this? Is this something they actually plan to enable or not given its known positives and negatives?

The currently posted policy only strictly talks about ownership.

Should the policy be updated to note that if a Trademark owner raises objections thah site moderators may not add maintainer or co-maintainers?

It’s been suggested in #3304661-8: Change how modules and submodule dependencies are handled to have more consistent namepsaces [DRAFT]. → that this should be changed.

Or does this only happen when the submodule has its own composer.json?

It impacts all sub-modules.

I wan to reiterate this just essentially opens the 'next major' split early, we do so every major release so the process isn't abnormal in that regard. IMHO this tends to align with having a MAIN branch which is intended to eventually be done for core.

Pros:
New API can be implemented immediately.
No need to postpone API breaking change or major system overhauls to branch split (issues can be worked on when devs have the time rather than crushed into a shortened release window)
Ecosystem can begin testing with the new major, as it will exist, much sooner (possibly reducing the number of critical bugs to fix during alpha/beta). This goes beyond just phpstan
Less followup tickets (possibly less cluttered issue queue).

Cons:
An additional branch to maintain.

One way to avoid having two versions of an MR would be to just not require it for the next major commit

Considering we have a fairly standardized deprecation format, It may be possible to work the issues with deprecation and let rector strip them to create the mainline MR. Could be tested and possibly become an "on commit" task if it proves reliable.

Addtionaly a healthy workflow inside MR can make it easier if developers separate deprecation from non deprecation it could be a simple re-base with excluded commits. This would take a bit of training however is reasonable to expect long term we could accomplish this.

We almost somewhat have the work already separated, I frequently see issues get through review to only later get tagged "need deprecation" before the issue can be committed. For those issues it is not much more effort to cherry-pick the current work based on the mainline branch on top of the released branch and add the deprecation.

Bumping back to the top of the queue to remind myself to work on this issue this week.

Sounds like this may be a new default in recent SDK versions

https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/s3-checksums....

Version 3.337.0 is January 15th. Rolling back should work as an initial fix.

This sounds like BackBlaze being non-compliant with the Full S3 protocol that they admit to:
https://www.backblaze.com/docs/cloud-storage-s3-compatible-api

This isn’t a new feature, it has been around since at least the 2.x SDK, just added as a default enabled now. I would not want to try and disable this feature for all systems, and would like to avoid adding a configuration option for something that seems reasonable to expect to be standard.

I wonder if this can be disabled via a few of our hooks and this can go into documentation?

I am unclear on what you are trying to say in #5. All that was posted was an error message indicating an AWS Client configuration error.

teacher.-bucket</bucket> I've checked with AWS and they confirm <code>Bucket name must not contain dash next to period. This appears to be the first error based on the details provided.