🇺🇸United States @cmlara

Let’s target to 2.x first as a feature request.

I have not yet loaded in a browser to validate no unexpected/negative changes occur, though I would suspect the “worst” would now be a browser with an autofill integration might now pop-up a selector where it did not in the past.

Quick glance looks like this would also target the setup form. we might see auto-fillers populate that field now with the “old” code. Is there a way to indicate it’s a “new” token similar to “new-password”?

@apaderno:
The module is security covered and the applicant does not appear to have the security opt in role.

Additionally this issue was only open ~10 days.

It is my understanding per https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... → this should have been moved back to the project queue on both counts above.

📌 Automate the majority of the ownership transfer process (retain human approval) Active would have (if implemented as proposed) prevented this from reaching this point at multiple checkpoints.

Wrong parent issue.

cmlara → created an issue.

I would really like to see this committed ASAP because it's part of a complex version dependency ring on my current site.

I had noted in Slack (but not here) that I hoped to commit this today. It is not as manually tested as I would normally like to do for a release, however at least the tests are passing.

I usually try to avoid calling things "New-X" because inevitably their newness degrades over time...

Good Point. NewS3fsFileSystem Could probably be renamed to S3fsFileSystemD103 or similar since it is is for D10.3+. NewS3fsImageStyleDownloadController I’m less sure of a descriptive name as it completely replace the existing controller and will not be D10.3 specific.

The Core Version Requirement line could be optimised a bit using > and < version operators.

True, though I do not believe the current syntax is causing any issues is it? IIRC the core logic is simplistic compared to composer. I believe it is safe to defer this to a followup issue so we don’t accidentally breach the constraints limitations by rushing it.

If we need to make other improvements isn't that better to do in the issue you linked?

That is a possibility as well.

I will note that the Entity itself does not assume that public is used https://git.drupalcode.org/project/remote_stream_wrapper/-/blob/2.x/src/...

I will leave it up to the modules maintainer how they want to split the fix.

It is just important to know the fix as is (even with the update to use the config) does not truly resolve the reasons why the core signature change occurred.

I think we are going to roll with this.

Tested back to D9.5/PHP 7.4 in https://git.drupalcode.org/issue/s3fs-3447227/-/pipelines/186921 (Gitlab CI for us is not setup to test further back at moment)

I don’t want to exclude the old image style generating class so we are stuck with a single PHPStan error.

Long term we probably need a config file that varies based on the version of Drupal Core however I think I will make that a followup issue.

There appears to be an incorrect assumption made that the derivative scheme will always be public://.

The new checks in 10.3 enforces that the $required_derivative_scheme to match the scheme of buildUri to assert that the route is responsible for the scheme and that a route for a different scheme isn’t being abused.

https://git.drupalcode.org/issue/remote_stream_wrapper-3437974/-/blob/34...

This is related to #3302994: Bypass of RSW file_managed access protection and arbitrary HTTP HEAD reqeusts → which was part of SA-CORE-2022-012.

What likely needs to happen is:

• RSW obtains the default configured scheme
• Locates the route for that scheme
• Adds the RSW mappings atop those routes.

Alternatively some of these protections might actually be better as part of a Route Subscriber.

Indeed looks like a couple of mainline no longer needed ignore removals slipped into this (one annoyance of PHPStan as the scanning is getting better some of our baseline ignores resolve with no action by us)

One of the items looks like an entry jumped locations, I’m not sure if that was manual manipulation or if we somehow had an entry out of order however that should at least be verified as we don’t want a programmatic run of this in the future moving it around again if this was a manual mis-positioning.

NW on the two entries that should be left in and handled in a dedicated baseline update and possibly the one entry that jumped as well.

No, I focus on the fact that part applies when other parts do not apply. Otherwise, that full documentation page would be much shorter: It would need to just contain the PHP and JavaScript Classes section, instead of having all those parts about classes, interfaces, and properties.

Respectfully I disagree, the section must be used in context of the other sections, since the entire page is about PHP classes this section acts as a general clarification that is binding over the entire page unless explicitly revoked.

Addtionaly the majority of that page is actually clarification for Core Committers (and core contributors) to understand the BC policy rather than direction regarding the use of Drupal's methods.

There are many should not, and no must not phrases listed in the BC policy page.

OOP standards indicate that if the Drupal Core Team is to list as a class to never be extended it MUST be final and for a property to never be accessed it must be private. Lacking final/private the it may not be recommended to do so however the extender is free to accept that risk.

The Drupal Core team has a history of not using final or private in order to allow code to be extended even if it is off API (see SLACK #contribute there were recent discussions on this regarding final and Access Policies). Furthermore the Drupal Core Team routinely and intentionally goes out of its way to maintain BC with non API classes, methods and properties (to the detriment of Cores code base) under the assumption that non API classes will be used directly.

Extending a non-final class is not on its own a sign of insecure programming or misuse of the Drupal API, only that the program has accepted additional support burden.

More importantly, IIRC this queue has approved applications extending non API classes in the past year.

That said to the applicant:
My suggestion is to not extend core classes, and do not use ServiceContollerBase. Extending classes will just make life more painful and tie your code to specific patch releases of core while ServiceControllerBase breaches the standard of Dependency Injection. Both of these make your code brittle and harder to test. Code duplication is not always bad.

I have not reviewed the code in detail, this message is solely in response to @apaderno's assertions regarding the BC policy and its impact on Code Review.

Setting parent issue to 🌱 [META] Stronger Typing for Entity API Active .

@brandonpost
https://www.drupal.org/about/core/policies/core-change-policies/bc-policy → Is the backend “API” policy.

Short version: very little of Drupal’s code is actually considered API, a significant portion of code is @internal by default. Even for code(interfaces) that is API in one scenario(caller) may not be API in another scenario (implementer).

You cannot develop such a module in days. And you have to have a name in order to plan the project accordingly.

Also if you check the issue queue you can see the team is working towards a plan very actively.

The D.O. Rules are that it should be planned and developed before reserving a name.

Namespace changes are a fairly simple search and replace change if needed with minimal impact.

I’m not asserting @shadcn has any right to the namespace given the current rules, only that the same rules that granted the namespace to the new maintainer should be considered on if the namespace should now be returned to the general pool of available names for adoption given the modules issue queue appears to show no firm code exists and the project is only in the planning stages.

I will note that the new maintainers of the namespace appear to be in breach of the Druapl namespace squatting rules as well as per the links given in #10 modules should be ready and code should be avaliable within days not week before obtaining a namespace.

The boiler plate commit published this morning (possibly in response to this issue) can not be considered code avaliable.

@apaderno:
That may need to be reviewed by you and the namespace reverted to .

@shadcn if you do create an issue it may be worth linking it to 🌱 [META] Increase Security of Project Ownership Transfer Process Active as the parent meta.

If there is no ability to "adopt" modules the majority of the supply chain attack risk disappears and the ecosystem is protected.

This was intended to be read in the narrow context of targeting code ownership changes on D.O. and not the much wider acope of all supply chain attack vectors.

A supply chain attack (if there ever is one) has a high likelyhood of beeing discovered by someone reading the code.

Code review is one of the better methods, though this is not simple. This conversation started as a tangent discussion from the XZ exploit, a situation that with all the developers using and reviewing the code came down to the Linux community being saved by luck. PHP code is harder to obfuscate compared to the compiled build of XZ, but not impossible.

XZ also again shows how much of what we do is “trusting others” none of us have time to review every single line of code in every single program/module we use. We for better or worse depend upon others. XZ reminded us angain that Open Source does not mean backdoor free.

My alternative proposal is that we try to safeguard "adopted" modules against supply chain attacks by reviewing changes to the code (as documented in the commit history log) for some time after a new maintainer has been added and look for potential exploits.

That would be an interesting alternative if we continue to allow adoption (which I’ll admit is very probable given the opinion I’ve seen most of the well known D.O. users express) though I fear this may significantly increase the project ownership queue admins burden and change it from an administrative skill position to a programmer position. Not as good as preventing an individual from taking over a project but certainly far better than the current system of virtually no oversight at all after a change.

Adding on to my comment in #4:

WordPress does appear to allow adoption, though with what appears to be more stringent requirements than we do. https://developer.wordpress.org/plugins/wordpress-org/take-over-an-exist...

I could not find a process for taking over abandoned extensions for Joomla.

I would imagine this would be conceptually similar to the old “web of trust” system where a person (ideally local) who knows and collaborates with a person over a period of time vets them out reducing the burden and limiting the travel needs.

Thank you for the response, that helps clarify the steps to reproduce (request password reset link and visit the link received via email).

📌 Use an EventSubscriber to process one time login links Needs work is partially related as Password Reset links are not yet fully functional in 2.x after adding 🐛 Installing contrib modules can lead to TFA accidently being bypassed Fixed . We currently refuse all resets as part of the fail-secure code.

The attached screenshot in #6 is also not a normal Drupal Password reset page, possibly added to workaround 🐛 Bingpreview invalidates one time login links Active in core. As noted previously we would need to know the name of the module that generates the page.

I'm not sure if 📌 Use an EventSubscriber to process one time login links Needs work will solve the issue or not however until we have unmodified core working we really can't evaluate the impact of custom code.

Setting postponed on the parent issue. Please provide the additional details regarding the custom module installs and we can re-evaluate this after the parent fix is in mainline.

I've updated the IS to try and make it more clear since this issue is in context of the adoption procedure and not globally trying to fix every supply chain attack possible through D.O. (as noted in the META issue). If anyone has a better overreaching solution to fix all the supplychain vectors instead of targeting small known risks please open an issue and add it to the META.

As an argument for this:

GitHub: Does not allow this
GitLab.com: Does not allow this
NPM: Generally does not allow takeovers for lack of activity, does have a process for dealing with Squatting which is different from code ever having a published valid release. Does explicitly call out that due to the risk of supply chain attacks some otherwise valid disputes may not be approved.
Packagist: I can't find a policy however I don not believe they allow this since an owner controls an entire namespace not just a package.

Personal note:
This solution had the least support on Slack, though by all security standards it is IMHO the most reasonable solution, it requires the least actions from site owners and closes this specific supply chain attack vector the most.

I would suggest whatever the new variables are it may be better to switch to YES/NO instead of 1/0 and set them as explicit options with drop-down menus for use inside of the GitLab Pipeline display.

I will agree that the GitLab activity feed provides some data.

I will note a couple activities that are the GitLab API may not provide data for:
Change of a maintainer to owner will not report a permission change as they hold the same level in GitLab.
Adding a new user to owner will report the same as adding a maintainer.
There is no easy way to determine if it was Project Ownership that added the user except to assume if its one of the 'known' queue admins that performed the action (and that they joined/left the project in rapid succession) that it may have been project ownership queue.

Not to complexly discount it, the activity stream is useful, there are just a few edge cases it may not be able to capture.

Relevant quotes from https://drupal.slack.com/archives/C2AAKNL13/p1715712783335239:

Is there a message on the project page that displays when a maintainer has switched? If not, perhaps a "the maintainers of this module has changed recently" message could be helpful.

-- Joe GL

Or more sophisticated approach - maintainers history - where all maintainers changes will be logged and displayed (with dates, etc). And you can easily check, when someone was added, removed, changed permissions and so on.

-- Juraj Nemec

Pulling relevant quotes from https://drupal.slack.com/archives/C2AAKNL13/p1715712783335239

Wouldn't forking a module with a new maintainer be a better experience than silently passing hands to a new maintainer, at least in terms of security? With a module takeover all the community members using it are completely unaware any new release was actually made by a new maintainer. The built up trust over a long period of time in the previous maintainer shouldn't immediately transfer to the new one.

-- Joe GL (emphasis mine)

Just to do a concrete statement about the starting comment of this thread: Moving ownership is a potential security risk. So we need more checks about everything related to it.

-- c_logemann

Taking over a module is to easy also in my opinion. Another situation is if the module "owner" is not available or doesn't care about because taking over can just be done with bypassing of co-maintainers. I had exactly this situation where I was a co-maintainer for a D7 Module. A new project owner stepped in to claim just the namespace for a complete different approach for a D8 module and just closed all D7 Issue and removed me as a Co Maintainer. Luckily my customer project which was the reason to step in ended long time before incl. shutting down. So I had no impact to my own work. But I was shocked how it easy to just push me aside and I lost a little bit of trust to d.o "meta project management".

-- c_logemann

cmlara → created an issue.

I'm going to need more details on reproducing this.

On a default install of core the password reset request page does not send a user to the login page nor does a password reset send a user to a login page.
Is this on every account or just an account with TFA enabled?
Any other login related modules installed and if so does the error occur with them disabled?

\Drupal::service('file_system')->copy('private://my-folder/my-file.txt', 's3://my-bucket/my-file.txt');

Did you include the actual bucket name here ? If so that would be incorrect use of the Drupal S3FS StreamWrapper. For us s3:// is defined as being “the root of the configured bucket plus any configured sub path prefix” (this is different than if you have previously worked with the AWSSDK directly where you need to include the bucket name on each access).

Should do it check for the existence of the bucket/directory on the remote S3 service instead?

The s3fs_file table is considered and authoritative cache of the bucket status and is required to be kept up to date if making changes outside of the s3fs module (see README). While we call the internal decorated method, that method relies on standard PHP functions which are handled by the S3fsStream::class that routes back to the checking the s3fs_file table.

Is this for "Git vetted user" or membership in (and the role) of "security team" ?

Note: this suggestion originated by another user on Slack as related to 🌱 [META] Increase Security of Project Ownership Transfer Process Active . I'm posting it as part of the followup of creating issues for possible solutions. It could possibly use refinement, though on initial glance the concept is in my opinion reasonable.

This is for the "Permission to opt projects into Security Coverage" permission.

On a personal note: My account currently holds this permission. Under the proposed changes I likely would not as I have not been to any Drupal Convention, user group, etc nor do I have any plans to do so in the foreseeable future. This change would make it harder for myself to obtain, though that is not necessarily bad. No one here has met me, no one know if I'm actually 20 people working together under a common identity to build up my reputation for a big attack. Given the ability that comes with this role (takeover ownership of project namespaces and the ability to obtain security vulnerability information when working to adopt modules unsupported due to an unfixed security issues) it is fair to view it as sensitive.

Personally: I could live without the permission if we de-coupled it from the ability to opt projects into security coverage and the ability to adopt modules required an additional 'identity vouched' permission.

In the issue summary "Proposed solution" is left blank (so far). I shall review anything proposed in a timely manner,

This is a META, proposed solutions are linked as child issues and are still (slowly) being added to from the suggestions in Slack. Not all are expected to be accepted however they have been posted for formal evaluation in attempt to incrementally increase the security of the ecosystem.

I am just saying that the problems need to be more clearly documented in order for real progress to improve it be made.

My goal was to talk more about vectors rather than name specific incidents, however I can dig through my notes to pull out the incidents that cause me wish to see improvements if that is what is desired. I will counter that if we focus only on the known incidents we can miss the equally obvious avenues that have not yet been taken advantage of.

As you said, there will always be risk, we can’t completely eliminate it, we can however attempt to mitigate what we can foresee.

does not need to adopt and existing component (abandoned or not). Creating a brand new one and getting it hosted as part of the ecosystem will do equally well

That probably is best discussed in ✨ Prohibit the ability to adopt a project Active however as a short response, there is a significant difference between being able to publish to an established user base and forcing an attacker to create a new namespace with the effort of cultivating a new following. We do still have the typo risk style attacks though that would probably fall outside the intended scope of this meta.

The principal safeguard against supply-chain attacks is peer-review, and that goes for abandoned projects resurrected under new ownership, and for new and forked projects…

Currently there is little to no tooling for site owners to know that the Project Ownership team other than following every thread in a support forum. Drupal Auto Updates will likely make this worse as it is being promoted as a “set it up and your site is always up to date” encouraging site owners to update without review (there was one thread in Slack about possible mitigations for this that I will create the follow-up issue for later today). I would contend AutoUpdates puts more onus on the backend adoption side to have tighter gates.

and to focus narrowly on the Project Ownership Process in order to solve this is just going to miss the mark.

It has been my experience D.O. historically wants targeted incremental improvements, this issue is in line with that mindset, it’s goal is not to fix every problem instead attempting to mitigate possible exploit paths to reduce the overall risks.

Updated the IS to match the steps agreed in the issue and SLACK conversations.

There is an instance in core that returns FALSE for dirname

Our reviews did not find this in functional code. I've updated the IS based on the belief this isn't the case.

cmlara → created an issue.

cmlara → created an issue.

cmlara → created an issue.

cmlara → created an issue.

The 7.x-2.x branch is no longer supported, please use a 7.x-3.x release.

As a test dependency we need to wait on Key:
📌 [META] Key for Drupal 11 Active

cmlara → created an issue.

I would like to maintain/support Maintenance 200, especially the Drupal 7 version.

It is my understanding Drupal 7 versions are no longer eligible for support.

Community support for contributed modules and themes will continue as it has to date. However, beginning August 1, 2023, once the Drupal 7 branch of a contributed module or theme is marked unsupported it will not be eligible for new maintainership and will not be marked supported again. This will be true if an existing maintainer marks the module/theme unsupported, or if the security team marks it unsupported for lack of response. If there are Drupal 7 modules or themes that you or your clients rely on, then we strongly encourage you to adopt these projects proactively.

https://www.drupal.org/psa-2023-06-07 →

If the applicant adopts the module they will only be able to publish releases for D8 and above.

Setting to Needs Work as the applicant should confirm that they still want to support the module given the limitation against providing D7 releases..

Closing as the issue has been postponed for 2 weeks without feedback.

This issue maybe reopened in the future if answers to previous questions are provided.

Thank you for the patch.

The patch file workflow has been deprecated on D.O. due to the upcoming decommissioning of DrupalCi on July 1st when DrupalCi will be fully replaced by GitLabCi.

Patch files can only be tested in DrupalCi while MR’s can be tested in both DrupalCi and GitLabCi. This means after July 1st only MR’s will be testable on Drupal.org.

Many modules, including TFA, have already adopted GitLab CI in order to be prepared for decommissioning. Part of the adoption processes involves disabling DrupalCi to conserve resources.

Due to above reasons the TFA project can only work with MR’s and will need the patch file converted to a MR to continue.

cmlara → created an issue.

Note: there are a few user facing (translated string) spelling spelling errors I'm specifically ignoring as part of this fix. Since they can have ecosystem impact its worth evaluating them later.

I'm also ignoring errors that are from test strings (these can be solved later), or values that we can't rename.

Main goal being to cleanup the easy fixes and make it less likely new spelling errors are introduced going forward.

A note from a PHPStan standpoint:

@return string|bool
Means that anyone calling the method needs to check for TRUE as well before passing the return to a function that only accepts strings.

This means the classic if ($value === FALSE) {}
check needs to usually be re-written toif (is_bool($value)) {} even though the comments only describe FALSE.

cmlara → created an issue.

(Bringing Slack notes into the issue)

Lack of BC was intentional.

As noted in 🐛 ImageStyleDownloadController routes do not limit schemes served Fixed the change was for security reasons related to SA-CORE-2022-012 → .

The class and method in question are 100% internal. Per Drupal API policy no one should be extending, calling, or otherwise using the method unless they are willing to accept breaking changes in point releases.

A lot of this happens in Drupal Core with module maintainers not realizing that their Core Compatibility should actually be "<=X.Y.Z" or at most "~X.Y.0" rather than the much more common "^X"

Setting back to NW to prevent an accidental fast-track of this into core.

I'm going to take a moment to plug ✨ Split ImageStyle into the config entity and a separate event-based image processing service Needs work as a likely long term solution for many modules that are currently providing their own ImageStyleDownloadController class.

I’ve given a very cursory glance at the code and would like to see some documentation on how this is suppose to be deployed given the TFA module policy that REST authentication should be prohibited.

Personally I'd better added local runner's cache at least - distributed has own downsides of download/unpack issues

I'm not sure local cache would be sufficient. The odds of obtaining the same runner where the local cache is present are at most ((1/current_fleet_size) * probability_last_runner_is_still_in_fleet).

We would need INFRA to give us those statistical points for us to determine viability.

OTOH runners are from kubernetes, so could use its approach https://docs.gitlab.com/runner/executors/kubernetes/#using-the-cache-wit...

If I'm reading correctly that is part of GitLabs cache system correct? If so indeed another option for configuring how the cache data is shared internally and a solution for #3387117: Enable distributed caching in GitLab Runner →

Caching proxy in Gitlab is still beta and slowly moving, but it has caching for proxied

Ah I had missed GitLab was looking into adding composer into their package registry. Maybe that is a good long term goal, however a generic caching proxy (squid or any other alternative) could bridge us so that we are not waiting on a solution that does not yet exist.

cmlara → created an issue.

Notes on changes:

Added a NewS3fsFileSystem class. This will be used on D10.3+
If I had realized Drupal Core's API policy stated " the interface is treated as an API for callers but not for implementers. In more detail" and been better at defining API as it relates to the Drupal Ecosystem (I was mostly relying on a transient acceptance that Drupal Core's policy was s3fs as well) I would of marked the previous S3fsFileSystem class final to prevent extension. From quick testing the old class actually will still work with D10.3 however the protected method internally did strictly call for integers only and unlike core I don't agree with opening these up outside a major version. The replacement is a final class so that this isn't an issue in the future.

Added a NewS3fsImageStyleDownloadController (currently marked @internal and I'll probably make it final before this issue lands). This will be used on all deployments. The existing one extended an @internal class. We actually don't need the new feature added into core as we internally validate our routes and we are solely responsible for the s3fs provided routes and only s3fs routes.

Since the core class was @internal it could make breaking changes in a Minor release without consequence. We could easily adopt this change (or a variant of it) and have not issue inside of S3FS, however it would move the breaking change for anyone extending our class to include versions of Drupal core as far back as D8.8. I'm aware of sites that have extended the previous class in the past, they are going to break one way or another and need to make a code change.

This is extremely borderline from a semver standpoint. This is an attempt to choose the least damaging impact. The only argument in favor of this I can make is that "we extended an internal class and by extension we were internal as well" --- I really don't like that logic, its more of an excuse of why I'm not bumping us to a 4.x for this change and relates to our poorly documented definition of API.

Disclosure: I was the primary promoter of the Core change for the ImageStyleDownloadController that caused this break for security reasons.

Work I still need to do:
Cleanup new phpstan baselines related to these changes and validate any coding standard changes.

Closing as won't fix as this really needs a standardized API meaning Core, or a contrib module needs to provide this first and we will want a new issue to implement the API when it exists.

In my opinion ✨ Split ImageStyle into the config entity and a separate event-based image processing service Needs work is the best issue to support for a resolution to this request.

Closing as won't fix as this really needs a standardized API meaning Core, or a contrib module needs to provide this first and we will want a new issue to implement the API when it exists.

In my opinion ✨ Split ImageStyle into the config entity and a separate event-based image processing service Needs work is the best issue to support for a resolution to this request.

This was discussed and tested via Slack last month:
https://drupal.slack.com/archives/CGKLP028K/p1713382030886629

Attached instructions/procedures guide that was linked in that thread (no modifications since initially created).

Think his covers the issue summary changes so removing tag.

so we'd have different logic for next minor and next major?

Defensive programming would say we should add the same logic for next minor (and all the other variants in the future), however if the Drupal Core Team is following procedure it should never trigger as the next minor branch should always be opened before the promotion of stable due the necessity for targeting issues and fixes once the release freeze starts.

not sure about the alpha/beta phase of 11.1 when there is 11.1.x and 11.x

Until 11.1.0 is released it is still the “next” while 11.x is a “some time in the future release” this applies even when 11.2.x is opened prior to 11.1.0 release.

All this logic also works with a “main branch” (regardless the branch name) as it is necessary to separate “future release mainline” from “the next release” just the same as 10.4.x has already been opened even though 10.3.0 is not yet stable.

As we control the variables, we can just set them to empty, and:
- The "check_versions" job can check when they are available and warn us
- The related variants should not run if the variables are empty

To knowledge we have had no policy to date allowing it to be empty.

My recollection is for 10.x we left the 10.x value in place until 11.x was opened (we did not have the predefined next major job to make it a concern at the time).

I would instead propose the rule should be “if the current major constraint equals the next major constraint do not run” this runs less risk of breaking existing use of the variable.

Removing “PAreview: security” tag as it is reserved for tracking applications that have detected a security vulnerability.

Regarding comment #17
50% of those issues were after additional prompting of comment #15 with no time for a maintainer to review them.
83% of them were only since May 1st (over 5 months with no activity from the time the applicant was suggested to be involved and only starting around the time of comment #16)
16% of them (a single issue) was on an already RTBC issue in early 2023 and is the only issue linked that shows any involvement prior to opening an application to adopt.

While the security analysts in me would like to see where this issue goes without interference I am commenting since this is a recent subject on Drupa Slack on if adoption should even be allowed on D.O.

I suggest the users behavior shows they are unfit to adopt the module (and perhaps any other module in the future) if they are unable to follow simple instructions, choose to ignore the module for months and than demand action.

I will also add while there is no evidence this is an attempt to back door a module the behavior should be viewed in light of similar tactics used to perpetrate the Linux XZ backdoor.

Awaiting user feeeback, moving to PNMI.

I haven't tested however I'll ask to make sure it wasn't missed.

Do we need to make any changes for SQLITE?

D11 now requires SQLITE 3.45 which I believe now requires the drupalci/sqlite-3 php (Unless I missed a change the other PHP images have 3.26)

Sending back to Needs review as we can’t trust Specbee reports. There is nothing in comment #12 that indicates anything other than what DrupalCi would have already tested.

Specbee (including this specific user) has been seen to not fully test issues and may just be attempting to gain credits for their D.O. partner rating.

More importantly, this module is basically un-maintained at the moment. If someone is going to want to see issues closed out they are going to need to consider stepping up and working on the rest of the queue to show their interest in maintaining the code.

This example was posted in Slack:
https://www.drupal.org/project/jsonapi_filter_cache_tags/issues/3416953#... 🐛 400 response due to BadRequestHttpException thrown with filter parameter in URL in Drupal 10 Needs work

It was treated as a basic D10 Upgrade Bot issue using automated tooling when it is a scoped issue which is more involved. A report has confirmed the issue is not fully solved contrary to the post by the Specbee user.

The above post was made after my comment #10 which would indicate Specbee may not have developed a robust system to implement oversight.

Adding to the above I went through some of Specbee’s sponsored modules and they have open D10 compatibility issues or other development tickets which this user could be working on rather than targeting the rest of the community.

What exactly are you expecting to see that you do not currently see?

Have you logged in as a user with the same roles that you are attempting to masquerade as to determine if this works without masquerading?

cmlara → created an issue.

I feel it is worth asking:
Do we want to keep putting these sort of changes in?

If you run your tests with a version of SQL that is no longer going to be supported should we be blindly hiding that or should we be telling you it’s time to change the version of SQL you use for testing?

Back on DrupalCi you would set your core version then choose your php+sql version. Leave the job long enough and eventually it just wouldn’t work. There was nothing stopping you from trying to run with an invalid SQL version.

@apaderno

That module appears to be security covered and the applicant does not appear to have the security opt in permission.

I believe this is going to be a duplicate of #3314663: File links for paths with reserved characters ( '+' & '#' '?') generated wrong for external streamWrappers. →

Not noted in the other issue however IIRC the difference in core vs s3fs is that internal routes take a different code execution path with different manipulation.

To Reviewers:

Please ensure this module has mitigated the concerns raised in SA-CONTRIB-2023-030 when considering if this module properly uses Drupal API’s.

The TFA policy is that REST and similar authentication should be disabled with the 8.x-1.x branch for a secure deployment due to known architectural issues that the 2.x branch has been addressing.

Also worth mentioning a feature request that exists in the TFA module queue ✨ TFA module with headless Drupal Active which notes part of this module may be unnecessary with the 2.x branch and is looking for scoping details for the remainder of the issue.

I have performed no review of the submitted modules code.

Community support requests should not use a priority higher than Normal. Using a higher priority for community support will not decrease the response time and may increase the delay as the issue is recategorized.

If specbee is in need of a Service Level Agreement backed response time for client facing deployments they may wish to contract with a Subject Matter Expert (either from the community or if any of the maintainers are accepting support contracts).

@joshi88: I suggest you re-read comment #5 it includes details about timelines.

There are many applications in the queue that have waited longer than yours have and you have not taken any of the steps available to accelerate your application documented in the application process.

It is important to note that Security Coverage is not required to publish modules.

Security coverage means:

• You as a maintainer promise the community to fix security issues (you can do this without having the opt-in permission)
• The Drupal Association/Drupal.org website will no longer encourage uncoordinated public disclosure of vulnerabilities (this is an ethics issue on its own)
• The public might report vulnerability privately (even security covered modules may often find public reports)
• That the Drupal security team might remove public posts on D.O. regarding the issue to allow time to for private resolution. The security team is under no obligation to remove posts and may choose not to at their leisure. (GitLab Issues conversion may help this as module maintainers may be able to mark their own issues as private)
• Users of your module might get notified through an SA and composer that security bugs exist. (The DST does not publish all security vulnerabilities)

Closing as won’t-fix to allow s3fs_assets → to handle this.

I will again note for most sites this likely is not needed as assets:// is currently stateless safe (every server can re-create the missing files).

ptmkenny → credited cmlara → .

https://www.drupal.org/user/3722731/track → Appears to be targeting “Low effort” D10 Upgrade issues using upgrade status (automated tooling) as primary validation technique.

Most of these modules are likely abandoned given D9 is EOL.

At comment #2 this issue was moved to the security queue out of concern regarding “The user has tfa set-up but when this message is shown they don't have to enter the authentication code, they are just logged in”.

Reminder to all TFA users that any issue that leads to TFA not being promoted when expected should initially be considered a security issue.

In this case it was determined the site in question had the miniorange_2fa module installed. The error disappeared upon removal of the miniorange_2fa module.

The scenario as described would make this a duplicate of 🐛 Installing contrib modules can lead to TFA accidently being bypassed Fixed . The fix was not backported to 1.x as it was built using 2.x only security design changes.

The root cause of this fault remains as a reason for us to attempt to EOL 8.x-1.x branch as soon as reasonably possible.

it is solving a similar problem in a much more complicated way. I personally find the hook approach much easier and slightly better documented

If the primary difference is hook vs event that should also be discussed in the other issue. Events however are the way forward. Hooks are an outdated Drupalism that the current effort is to remove from code rather than adding new hooks.

Documentation is usually an implementation fault (failure to document when creating the emitter) and TFA has recently adopted GitLab Pages to help handle this.

The idea of showing implementers is not a bad idea, part of me was previously thinking that an interactive tester would also be useful however I was not going to force it as a requirement to add the feature.

I can’t see this issue being able to work without needing to undertake the same work as ✨ Allow TFA requirement to be configured per user Needs work . I’m going to close this as a duplicate.

It appears this may be a duplicate of ✨ Allow TFA requirement to be configured per user Needs work . Can you confirm it is not before this continues further?

Feature requests should target 2.x first.

The patch file workflow has been deprecated on D.O. due to the upcoming decommissioning of DrupalCi which is being replaced by GitLabCi.

Patch files can only be tested in DrupalCi while MR’s can be tested in both DrupalCi and GitLabCi

Many modules, including TFA, have already adopted GitLabCI in order to be prepared for decommissioning. Part of the adoption processes involves disabling DrupalCi to conserve resources.

Due to conversion the TFA project can only work with MR’s and will need the patch file converted to continue.

@baddysonja:
That is exactly the document that is raising these questions.

will probably take some weeks until we start to see more official documentation

That goes to my point above. This is basic “coordinated release” territory where a major initiative with a less than 1 year (~238 days at time of my writing) deadline would already have its documents written and ready to publish the moment the Dries presentation was completed in order to effectively recruit volunteer’s.

until then you can follow the discussions on #starshot on Drupal Slack

That would make good documentation as well of “where the initiative is being discussed” which is not a question I thought to ask.

I will note for others that from a Slack ping I received I have learned this initiative apparently has been mocked on GitHub, it is apparently just a Drupal Distribution built using the Recipes system rather than the Install Profile system (though is trying to be presented as not a distribution).

The GitHub repo is apparently linked from https://www.drupal.org/starshot → via “try now” (found via Google, I have not found where this is linked to on D.O. and did not observe it in the press release)

cmlara → created an issue.

@vishal.kadam:

Technically this application is waiting for a community reviewer to review.

Reviewers will then review the project. If they don't find any flaw or problem with the code, they set the status to Reviewed & tested by the community.

When a reviewer is confident that the application can be approved, a status change to Reviewed & tested by the community will inform the Code Review Administrators who will do a final review and set the status to Fixed upon approval, or bump it back to Needs work if there are still changes required.

I suggest re-reading “Reviewing security coverage applications” which is covered in “How to review security advisory coverage applications” from your initial post.

I think that if you're going to require people to follow a recommended README format, then the application instructions should say so.

This is covered in the Security advisory coverage application checklist which is linked in the first message and included in the documentation linked on main page regarding the process for applying.

cmlara → created an issue.

Why add it as an array pass through?” (more array mess)

“Implement a logger and try to figure out if a log event should be considered critical/fatal or not.”
I would be curious if you have a real life scenario that this information is actually useful for?

To me it appears this is internal method data that is not needed or useful outside the method. Primarily looks like it is intended to control if an error gets raised to the server logs as well as the Drupal logs.

If it is important why not add it directly to the logged message rather than just hoping for a logger to act on it?

I have been informed that a ecosystem module s3fs_assets → exists to fill this role.

While I am not sure that module as it currently exists would be a good fit for all sites as currently written improvements can be handled in s3fs_assets queue.

I'm inclined to mark this issue won't-fix and allow the ecosystem to handle it.

I will leave this open for 7 days to allow any objections to be raised.

High level reading linked issue:
That does indeed sound like an issue in IMCE if they are processing unrelated content for a single file during hook_file_download(). That is the sort of performance fault that may not show up in local storage yet will show up in network storage.

The 62% decrease in time is impressive to show what a good cache can deliver.

I don't personally see 20k files as a problem on its own. A 3-8 second time would normally not be as significant if this was a backend operation. I wonder how often stat() all files in a directory is done for fronted facing operations.

On top of that if a program did need all of this data for frontend usage they should be prepared to start paging the data.

There will always be some performance limitations we cant overcome. That said I don't want to say this is impossible, however I don't see it being a high priority either at the moment as this may be fixable by more efficient filesystem usage.

Setting to minor, if anyone can come up with a reliable method for this it would be worth considering however at the moment I'm not going to spend a significant amount of effort looking into this until less

Side note: we will drop that additional 14% for preventCrossSchemeAccess() in 4.x as we will not have that as a security concern.

This does make me wonder if we should (since we have GitLabCi now) run regular profiling runs as part of the build process.

Most modules I have seen do this have special handling for the password reset route and login_destination appears to be no different https://git.drupalcode.org/project/login_destination/-/blob/5ba35c477e9b...

We did see similar with https://www.drupal.org/project/tfa/issues/3108099#comment-15337414 ✨ Redirect to validation setup after login without tfa Needs work , while not the same code it is inherently the same problem, a 3rd party module is changing the destination after we have set a destination.

For now my suggestion would be to disable the login_destination module to allow Password Resets to function.

On cursory thoughts I can not think of a way we can avoid all of these redirect modules not having some issue that needs to be coded for on their side without a new API being added to core.

I'm moving this issue to login_destination as it is that module code that is changing the destination redirect.

I was unaware of s3fs_assets.

Sounds like this issue can be marked as fixed as none of that is related to answering the question of does s3fs support assets://.

cmlara → created an issue.

Untested theory:

If we solve #3387117: Enable distributed caching in GitLab Runner → we could investigate setting all the rules for commits to a branch to default to skip and use a launcher job to trigger the full pipeline if a commit key doesn’t exist in the cache.

Will not save 100% of the resources (would have to always launch one checker job for commits to the main branch), and makes the workflow a bit more complex Howber should cut down CI time to a fraction of it current usage for those tested through Merge train.

Components is still a BETA feature, would suggest postponing until stable release.

This one is about explicitly and as simply as possible explaining to users of the module who may not be as well versed in all this as yourself what the situation is.

To try and put it more clearly.

Change Description:
Prior to 10.1 Drupal generated CSS/JS files when a page is loaded (such as http://example.org/node/123).
Prior to 10.1 Drupal stored CSS/JS files in the public:// streamWrapper. These files must exist when the browser requests them or the page will not render correctly.

As of Drupal 10.1 Drupal Core now stores CSS/JS in the assets:// streamWrapper.
As of Drupal 10.1 these files are generated "on demand" when a user(browser) requests them.

What the means:
Persistent shared storage (s3fs) is no longer required for CSS/JS files.

The URL to retrieve these files contains all the information needed to create the file if it does not exist. Writable storage must exist on each server that will generate CSS/JS files to store these files. The storage does not need to be shared between servers. Drupal Core allows configuring where these files will be stored.

As of right now it is not possible to store CSS/JS assets in D10.1+ in s3fs. ✨ Add support for s3fs to use the assets:// stream wrapper Postponed is the feature request thread to discuss adding support for assets:// storage however it is awaiting justification that such a feature is actually needed given the significant technical costs and degradation in performance gains the feature would add.

Documentation:
Our ability to effectively document this is limited, as a project we only control the docs for s3fs while most of this should be in Drupal Core Documentation.

The s3fs README.txt and release notes were previously updated to include a note that this behavior change occurred.

@zeip: have you been able to test the proposed changes to confirm they solve the issue in your deployment?

Setting Postponed-NMI awaiting feedback on cache service change impact.