๐Ÿ‡บ๐Ÿ‡ธUnited States @cmlara

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

Merge Requests

More

Recent comments

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Unable to duplicate the fault and it has been 2 weeks without additional information provided by the reporter.

If you are able to duplicate this and provide additional details the issue may be reopened on the future.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Needs work for issue in #15.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

the module/theme/etc which is receiving the MR is, by definition, not a custom module.

In general Iโ€™m contending once you patch/modify/change the code of the official module it can be perceived as a custom module (an uncommitted MR is custom code).

End of the day it does not make a huge difference which of the paths we test under, just giving some arguments from my mind of why custom makes sense and perhaps why it was never questioned when it was implemented.

Iโ€™m not against this as long as it waits to a 2.x branch of the templates, I just equally donโ€™t see the value as it sounds like more like brittle tests than a true fault of the Ci environment.

Modules/themes/etc hosted on Drupal.org expect to be installed in the contrib folder, not the custom folder, and thus the test system is not performing an accurate test.

These deployment paths are to my knowledge just convention and are not mandatory by core correct? My recollection is the parser looks recursively for all *.info.yml files in modules and that you could find your module in any path correct?

Technically the module is not even installed in the custom folder with the current template, it is in the GitLab build root directory, and is symlinked from the custom folder (Iโ€™m not a fan of this since Drupal does not officially support this and symlinks have caused me issues with pcov)

I was discussing the other day in Slack how we may want to populate DRUPAL_ROOT as part of UnitTestBase, and I imagine a VENDOR_ROOT too to make these tests a bit more position agnostic.

haven't ran into a case where paths are being manipulated after a composer build.

I run several projects that generate code coverage reports and part of this is normalizing the paths so they render in the Gitlab UI. This change would break that normalization. In fairness I could have been more robust in the sed globs I used to support more robust path locations, however it was not foreseen at the time since this was job specced.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

It is my understanding 'contrib' is commonly used for modules that are downloaded from D.O while 'custom' is used when they are "local" modules not distributed on D.O.

I would opine that the GitlabCi template aligns with this as the code being tested is not guaranteed to be in the official repo and as such is closer to a 'custom' module.

Note: this would probably need to wait until a 2.x branch if it is accepted as it will change path locations impacting those who implement and extend off the templates.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

I think, this should be addressed in a separate issue and carefully thought through.

I could see some argument for that in that the module name code is wrong in general for even referring to the root module without the upgrade status job.

That said why make this overridable when we could use code to find the name of the root module from the info.yml? (Basically implementing half of the suggestion above, obtain module name from code without recursion)

. Some submodules are optional, i.e. they can only be enabled if a certain dependency is available, where that is deliberately not a hard dependency.

Those dependencies should already be present as require-dev. That doesnโ€™t make it a hard requirement for deployment just testing. Though it does run risk of the job failing now. Fair that could be deferred to a follow-up issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Some modules may have multiple submodules.

Should the upgrade status job instead of using the $MODULE_NAME variable search for each info.yml and test each module and submodule by name?

Should this be a hidden variable ? (Will the majority of users running a custom job from the gitlab pipeline need to change the variable?)

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Closing as a duplicate of the core issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

More than 2 weeks without providing feedback closing the issue.

If you are able to provide the requested information the issue may be re-opened.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

IMHO, that's not in scope of this issue and there's no need to hold this up.

Making sure we fully flush out the BC layer is directly related to this issue, and is partly why were on multiple issues related to the same problem. I wish we hadn't missed the 10.3.1 bugfix, however equally I don't want to see us here again at 10.3.2 or 12.0.0 questioning why we made a decision without looking at the relevant side issues.

Even if the new interface would implement the old one , the opposite can still fail: module A requiring 10.3 and adding a type on UserAuthenticationInterface but module B hasn't been updated yet and overrides/decorates the service with UserAuthInterface.

True, I guess until 12.0 everyone is going to have to assume UserAuthenticationInterface isn't implemented add the same BC layer.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

This still hasn't been fixed by the translation team, however there is very little we can do from our side.

We have released new versions since this issue was opened and they do appear in the translation database.

I'm going to calls this a Duplicate of the upstream issue and close it out.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

it would be good to make sure that the granting and revocation of this access is automated,

Once we learn to embrace GitLab most of this becomes much easier.

GitLab has well defined API's available, including user management API's. I would not expect it to be hard to write a script that looks at activity of all the users with privileges and downgrades those that have been inactive for a period of time to Reporters or less (reporters still have access to issues marked /confidential and as such may still be too high a privilege long term) .

Equally I would expect the steps in the user guide could be scripted after the policy is adopted.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

That would be closer to what I expected.

I have not validated the interactions however if both modules do implement overriding the same route I would expect incompatibility.

I would prefer we not have to maintain an exhaustive list as such I would encourage any such documentation be written generically allowing site owners to make judgments based on what they know about their site to make a determination on if they will encounter issues.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Note, we might need to move to xdebug in the future if reports are true that PHP 8.4 does not support PCOV and is abandoned https://thephp.cc/articles/pcov-or-xdebug. For now we stick to PCOV as it is faster and it has been configured.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Visually (review of code action only) looks good to me, and appears to address my previously raised concerns.

Question on the feature side of this: You mention test work, does that include adding a flag to KernelTestBase and above to allow this to be easily enabled for phpunit runs? Possibly pair it with an environment variable for setup in phpunit.xml or GitlabCi?

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Visually reviewed, and (very quickly) smoke tested a theory.

The problem is that core-dev is not 'versioning limiting'. We recently had a discussion on slack about this and were reminded of https://www.drupal.org/project/drupal/issues/3163114 โœจ Match the version of drupal/core-* metapackages Needs review

The MR as proposed may stop the alpha error that occurred on this issue, however I don't see this helping a module declaring ^10 || ^11 that sets $_TARGET_CORE=^10.2 without setting ignore_project_core_version (which is only suppose to be used for next minor/major when the module is expected to show itself as incompatible and is pre-testing compatibility)

At best the MR currently appears to be treating the symptom of a larger root fault.

We have been abusing core-recommended to get an explicit version of core to install this, however that is probably not correct. Some of us moved to GitlabCI originally because DrupalCi enforced core-recommended prohibiting using a new major of a library (see ๐Ÿ“Œ Come up with a way to allow core-recommended (and tarballs?) to install Guzzle 7 for PHP 8.1 compatibility and Laminas-feed 2.19 for PHP 8.2 compatibility Closed: won't fix ). Similar issues are likely to occur in the future and we need to leave this open to developers to have flexibility in the deployment.

This does have me thinking this may not have been a regression and was just a bug from the start as I seem to recall abusing this to test modules, and it may have been after I had tested scheduled jobs that targeted explicit Core releases.

What we probably need to do is:

  • Not add core-recommended unless requested (or allow an opt-out of core-recommended) (possibly out of scope for this issue, mentioning it as its closely linked to our fault here)(this moves towards my point that the expand script does too much, it needs to as it was intended to be an easy migration from DrupalCi, however eventually we should get rid of most of this and depend on the maintainer to keep their file manageable.)
    • If ignore_project_core_version is set:
      • Overide drupal/core et al. to the defined constraint.
    • If ignore_project_core_version is not set:
      • Override drupal/core et al. to the defined constraint.
    • Validate (using composer::semver) that $_TARGET_CORE is compatible with the project drupal/core.
      • If not throw an error and end the run.
      • If it is compatible hard set drupal/core to $_TARGET_CORE to ensure we test using the requested release.
๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Still -1 to unhiding even if we rename.

But in this case weโ€™re just following the naming conventions that were set early on.

I wonder if we are at a point we should consider opening up 2.x changes.

We have been backlogging a number of ideas and we are starting to see the architectural negatives of some of our early choices with each issue . If we can layout a 2.x to fix this we could cleanup some of our BC problem.

If we plan to reverse this in the future I would suggest against a current rename as it would be extra noise for no gain.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

-1

There is no need for COMPOSER_EXTRA to be "hidden" and undocumented as it's a very useful variable that might be needed.

# The variables in this file in general should not need to be overriden in contrib jobs. They are defined without a separate
# value and description so that they do not appear in the 'run pipelines' form. The values can be overriden in the project's
# .gitlab-ci.yml file if highly specialized custom pipelines are needed.
##################

The variable is useful in general however does an average run from the GitLab UI need to make changes to this ? I think not and that hidden is better. You can still override hidden variables by the RUN pipeline page if you want to, you just need to know about the variable name.

If anything we probably want to just remove โ€œshould not need overrideโ€ portion of the text and explicitly call out the variables they should be internal.

If anything I think Jonathan was correct in #3459196: Add new variable $_UPGRADE_STATUS_COMPOSER_EXTRA for the upgrade status job โ†’ that the new variable for upgrade status should be hidden

FWIW I somewhat think we have these backwards, I would more think $_SOMETHING is intended to not be touched (internal) while $SOMETHING is intended to be (public).

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Going to sleep so very quick response, can provide more in morning:

Ideally, modules shouldn't need to set "drupal/core" as a dependency, because in the context of Drupal, this is totally implied.
report

Disagree,
1) When we publish actual releases we add this via the fascade
2) The facade explicitly caters for modules setting this in their json.
3) Modules should call out their dependencies (I view our current exapand composer script as more of a stepping stone to a time where everyone has a properly defined composer.json, especially once the issues of allowing modules to be developed in root folder makes it into core.
4) Git based deployments (adding the module as a source repo) need this data to be in the composer.json to properly calculate an install dependencies. Composer needs to know Drupal/core comparability, the info.yml is not a replacement for this.

(I would rather we had a drupal/api-spec metapackage to replace drupal/core however we use what we have).

why do you think it's a regression?

Unless Iโ€™m mis-remembering, way back before we ever added next version testing Iโ€™m fairly sure I had this working for testing where I explicitly declared core versions in the composer.json and was able to override using the core target variables to select a specific release.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Are you implying that headless TFA is prohibited?

As part of

">SA-CONTRIB-2023-030 the TFA module locked down authentication and advised all sites to disable additional login methods (such as REST) due to known faults where TFA was not enforcing authentication. This advice is still current for the 1.x branch (the 2.x branch is believed to no longer have this constraint and Iโ€™ve spent the time since that announcement last year overhauling TFAโ€™s architecture).

To the best of my knowledge that generally prohibits a headless deployment with the 1.x branch of TFA as only the login form properly enforces the use of second factor authentication.

Im sorry, but I don't see any policy regarding this topic. Can you provide further information regarding this?

My concern is based on the limited documentation in the module their either is a step to secure TFA to work in a headless environment that is not documented (users will need to know this), that code exists in your repo that provides this protection (that on a cursory review Iโ€™m not seeing) or that the TFA module is being (accidentally) encouraged to be used in a setup that is known insecure.

Iโ€™m trying to give benefit of the doubt and assume there is something in how you or another skilled user may deploy this that renders the concerns of the security advisory not relevant that if provided here would help this review process move forward.

Ensuring that code is used correctly and securely is a critical part of this review process. While I would not necessarily consider any of the above to be direct security vulnerability in TFA Headless (as TFA holds initial responsibility for securing the platform) if no additional lockdown is provided it would appear the module would either be non-functional or that key information related to the secure use of the TFA is missing. Both directly relate to securely using the TFA modules public methods.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

With assistance from @jurgenhass on Slack I was reminded of the normal solution for dealing with symbols that are not found however exist on disk.

Added fix into code and latest D11 run shows the Drush related validation is no longer present. https://git.drupalcode.org/issue/s3fs-3449990/-/pipelines/215416

I'm not going to hold this up on the PHPUnit(Code coverage) failure as this code can cleanly commits and runs under D10 and the tests otherwise pass in D11.

Setting to Needs Review for community comment (and to allow me time to step away and return to re-evaluate any possible improvements in the code as currently written).

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

cmlara โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Dealing with a PHPStan warning that only shows up on D11.
Call to method confirm() on an unknown class Drush\Style\DrushStyle
Have a query into #drush and #phpstan.

https://git.drupalcode.org/issue/s3fs-3449990/-/pipelines/215377 is the latest pipeline with D11.

Lack of code coverage driver is causing a PHPUnit warning leading to a step failure. PHPUnit is otherwise passing.
Will open a separate issue to restore code coverage.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara
๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Good reason. I had it in my mind that it was done in the module handler.

For the second half of #61 could this instead be something like:

$is_compatible = Semver::satisfies(\Drupal::VERSION, $parsed_info['core_version_requirement']);
$parsed_info['core_incompatible'] =!( $is_compatible || $ignore_core_version );

(where $ignore_core_version is set to TRUE earlier in the run instead of appending the current core version to the parsed data)

Goal being to avoid tamping with the data that is used elsewhere (like UI) to avoid hiding the fact the module is not actually current version compatible even though itโ€™s installed.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Line 40 looks like a regression to me.

Not setting require-dev of core-recommended just because require Drupal/core is set to indicate compatibility would be overkill.

That if section was intended for dealing with next major version testing where we need to override a version.

If weโ€™re not overriding we want to try and merge the request from test run with the composer.json to end up with either an error of the release being incompatible with the version under test, or being able to set the specific core release.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

From comment #61:
Why are we changing this in the InfoParser instead of the MoudleInstaller?

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Bringing discussion from the MR into the issue.

I think we made a pretty large mistake when we made UserAuthenticationInterface not extend UserAuthInterface.

I see no clean way to guarantee full support for D11 (and by extension d10.last) and D12 with the current BC layer.

It is possible modules may go all the way until D11.last using UserAuthInterface while at the same time modules may support only UserAuthenticationInterface as early as D10.3.

This could/will create issues for decorations when Core implements both interfaces and two decorators each implement alternative interfaces.

Iโ€™m not sure we can change the inheritance now in a patch release (interfaces are NOT api for implementers) but worth at least discussing how we want this to all work and what we are expecting from contrib.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Still -1 on the grounds it would be adding a known security fault into the code.

It is now also possible some code has already gone as far as to remove any protections they have had added for older releases (<10.3) now that this safety measure is in place and we could be exposing those site owners to renewed risk.

Policy or not, public method signatures shouldn't be changed like they were here in a minor revision without regard to BC.

Disagree, public just means the method can be called from another class, it has no implication on the guarantee that method signature will not change. The only โ€œguaranteeโ€ of a method not changing is if itโ€™s declared formal @api.

The "~X.Y.0" pattern has been a pain point for our update processes not something maintainers should be doing.

What we really shouldnโ€™t be doing is modifying or utilizing non-api code (however this is near impossible for many modules since typehinting an interface is API but implementing an interface is not) which would allow us as maintainers (with reasonable assurance) to just declare โ€œ^Xโ€.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or... โ†’ Is the process for taking over an โ€œabandonedโ€ module on Drupal.org.

@vensires and @jrglasgow are users on this thread that have the necessary permission to proceed through the process if they desire to do so.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Spoke with berdir, he confirms he has already updated his module however he opened the issue to ensure BC for other modules.

Here is a possibly less invasive version than rewriting to move all the logic to the end by instead just adding an else block for the call.

Also combined the tests and the BC break from @Anybody into !8581

Clearing the needs tests tag as I believe the tests I have extended are sufficient to cover the faults we are fixing though perhaps we still want one more tests to exercise the "user account is invalid" scenario/lookup

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

cmlara โ†’ changed the visibility of the branch 3456738-extended_tests to hidden.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

+1 for allowing maintainers tho self-indicate an issue has security fix concerns when the module is either not covered by the DST or the DST has declined to issue an SA.

For context I have had 3 incidents in the past year related to the Two-factor Authentication (TFA) module with internal faults that were unable to be marked as security releases:
Access Bypass Through Replay Attacks โ†’
Authentication bypass when default plugin is not configured โ†’
Replay and Denial of Service vulnerability in HOTP plugin โ†’

None of these were "Drupalgedon" level exploitable however TFA is a security solution module and site owners expect it to enforce second factor authentication.

Module maintainers may not always be right when they determine a release is not a security fix, however when a maintainer determines a release is a security fix we should generally trust them as the experts of their module to understand the fix does indeed pose a security risk.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Is the root issue here trying to maintain Drupal 8.8 compatibility in the current major? Because if so, thats not valid -- You can and should drop Drupal 8 support in this minor release. The minimum should be 9.5. You do not need to change the major version to drop earlier support

While I know a sizeable population of D.O. contrib maintainers hold that opinion I personally (along with other maintainers) do not.

My rule is that if I install 3.x on a version of Drupal I should be able to install ANY 3.x on that release of Drupal. Addtionaly in my eyes it is important to realize we are not actually developing for a specific release of Drupal Core, we are developing for an API spec defined by Drupal Core. No one should be running stock Drupal 8.8, howeve much like D6 continued on and D7 appears it will continue on after their EOL's its probable to assume that somewhere a D8 site does the same by back-patching core security fixes and changes. This is what it means to support an 'operating enviroment'.

Now is a good time for me to note, I am primarily a Security Engineer, followed by a Systems Admin, followed by a Web Dev. My view on these matters comes from my own personal history of dealing with software deployments and customer deployments of complex solutions as such I don't look at these decisions from a "How easy is it for me as a maintainer" I look at them as "How much pain is this causing my end user who is out of date trying to update to my latest release"

Do you mean? ImageStyleDownloadController This is not marked @internal. I'm struggling to see why this should have been refactored to work with Drupal 10.3?

I mention this in other issues. I've started to realize many(most) developers do not realize just how much of Drupal Core is NOT API. Drupal Core is essentially a "Internal by default" specification. Core controller routes like the ImageStyleDownloadController are "not part of the API of the module unless specifically marked with an @api tag on the method or class" per the Core BC policy. I'll note we have a side discussion in Slack #phsptan https://drupal.slack.com/archives/C033S2JUMLJ/p1718388676560139 where we actually shock ourselves in how some classes that may be frequently abused are techcnaly not recomended to do so (and anyone doing so accepts that a patch release may break their entire module)

As noted in the other issues, this was a security change in core, that alone is grounds to breach Backwards Compatibility per Drupal Core's policy. Addtionaly as noted as a Controller route core had no requirement to provide BC in the first place. They strive to do so, but they have not requirement to do so. I strongly stand behind the change in core. I personally observed it provide an open proxy exploit on systems.

We made a mistake extending it in the past, I can't undo that. 4.x was already going to not extend the core class to allow us to avoid the issues (especially since we called none of the code from the core provided class).

With the changes in 10.3 including a method signature change for deliver() we really didn't have much choice there was going to be some sort of break with 10.3, the question is how big a break.

Yes I could add the extra NULL on the method and gain compatibility with Drupal 10.2 and 10.3 (as long as no one extends us) and for all direct callers, however the impact is now that anyone extending us on the currently supported core 10.2 release will be broken. By creating a new class and using it for all releases going forward we make the break be solely impacting to 10.3+ installs which are already broken. This falls in line with me aiming to cause my fellow systems admins as little pain as possible, pain for software belongs to the developers. This also tracks with my view on Majors that if your running a set release of Drupal Core and you update to the latest release in the same major your site will not break. I absolutely agree I'm pushing the limits of that with the "old" class not being upgraded to 10.3

Usage/Modification of non-api code are also why I limit us to specific minors. My responsibility as a developer is to only assert comparability I feel I can guarantee, and based on the history of Core (within their API specs) we can't guarantee a full Core Major support so I will not add a wide constraint that I can't back up. This is as much about site owners protection as its is my own mental self care, I don't want to be dealing with Drupal Core compatibility issues under Downtime Response procedures. We also often argue that "Well semver means they will get the latest release that is already compatible with D10.3" 1) is not always the case we meet that deadline (though I try for it this release cylce I've been in a spot where I could not spend long coding/testing sessions.) 2) We assume site owners will upgrade the module before upgrading core or at the same time as core, we can not guarantee that occurs with a wide constraint.

Same with S3fsFileService -- I don't see anything in there that must be changed. I see some new coding standards you'd like to use, but again -- those aren't needed for D10.3 (or 11) compatibility.

The real big issue here is that Core doesn't actually define an API for Interfaces for Implementers.

Core is are allowed to change interfaces in a breaking way as long as callers are not impacted. I often mainatin that now that strict typedefs are being used more often we will actually see more of these issues.

Callers are indeed not impacted, they can pass either older Integers or newer ENUMs without issue (other than deprecation messages) however for us as an implementer this actually changes our method signatures. I note above that yes the old class actually works because of the lack of strict typedefinitions, however this is technicaly a method signature change. Our code is working only because it does nothing that explicitly expected integers.

I tend to allign myself closer to Symonfy BC policy https://symfony.com/doc/current/contributing/code/bc.html#changing-classes where I don't view it as acceptable to increase the allowed scopes on those extending us. Core does it, and thats their right to set their API and BC policy, I just don't agree with it and do not use the same 'relxed' impact.

The remainder of the changes is twofold 1) A requirment that 'new code' (even a duplicated class) needs to be brought up to latest standards (including PHPStan error resolution that was previously baselined) which should not be skipped. 2) The syntax changes since it is know the class will only be used on PHP8 installs and doign so made importing the class use less boiler plate. You are correct we didn't have to change the syntax to PHP, I absolutly could have kept the older boilerplate in the conversion however that would in my opinion created a class that is harder to mainatin long term, it cost me a few minutes to make future me curse past me less.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Key now supports D11.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Iโ€™ll note I pinged Berdir on slack a couple days ago asking if we moved the ifelse code back under the block if it would still be acceptable to them (it seemed looking at ๐Ÿ› UserAuth BC layer is not working for modules that use it to provide email based logins Active it should be, and the real error was in extending the userauth class rather than implementing a proper decorator )

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Same as the Raven's admin I'm not able to reproduce at this time. Awaiting reproduction steps.

Initial contact was made on Slack last night see thread:https://drupal.slack.com/archives/C1BB308HH/p1719385964866899

Advised I usually see this type of error when Autowire cant locate a service, and to try clearing cache.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Updated the tests fork to add one more test. it kinda tests the basic_auth through maybe I should have done it via BasicAuthTest instead.

On a positive note it seems like in my quick testing it may be just these two known faults.

I will note the elseif added in ๐Ÿ“Œ Optimize user logins by avoiding duplicate entity queries Needs work may not be exercised with just these 3 tests.

I also want to call out that โœจ [PP-1] Bump PHPStan to level 9 and accept a large baseline Postponed would have detected the issue with the UserAuthenticationController.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Pushed a quick set of sample tests to 3456738-extended_tests.

My local lab is being a bit difficult at the moment (and I'm about to call it a night) so not tested in depth or validated for any coding standards or similar, however the root logic should be there if someone wants to run with it. We might just need to extend a few more of the existing tests to prove we fix all the scenarios (look at the files that were modified in the previous two issues and extend any tests that exercise the same code paths)

On a quick look it did appear that UserLoginHttpDecoratedTest did catch the BC break @Anybody reported.

Still I think we should have a hotfix first, if it's clearly wrong, as updating to 10.3.0 is currently supposed to break a lot of Drupal projects logins using any of the contrib modules listed here: #3427298: [11.1] Deprecate UserAuthInterface

I'll defer to core team, my concern at the moment is while this is a bad break, fixes have already been rushed once before, it would be nice to figure out what other tests we could extend and do a quick 'proof' run if it only takes a few more minutes.

I will note the linked issue indicates most of those modules are NOT implementers and would be less likely to be impacted (they are calling userAuth->authenticate() ). That is not to say this issue should be downgraded on priority, just that I'm not sure the core team is going to rush this into an emergency fix, if we end up waiting for the monthly bugfix window we might as well try and test to find out if any other faults exist (even if the tests don't make it into mainline).

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

https://git.drupalcode.org/project/drupal/-/blob/77b8e596048aac5becd5646...

Yep that looks wrong too, it is a different bug than what we have reported here but is in the same ballpark of problems.

I wonder if it would be useful to add a test module that is a decorator for the UserAuth service that only implements the old interface and passes everything directly through (no modification) and duplicate several of the Functional/kernel Tests adding in the new test module (eg DecoratedSomethingTest extends SomethingTest).

If those tests fail it means the BC logic is faulty and may help us determine any other hidden bugs (otherwise given we have now had 3 separate issues with this it might be time for a line by line audit)

Unit tests would have been better but core is a far ways from being able to h it test everything.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

D10.3 has landed.

I had forgotten in the past Key could run a bit behind, we may want to debate reverting ๐Ÿ› Key module should be a Dev Dependency. Fixed since none of the tests depend upon it and it is only phpstan reporting. 4.x will need key, we might want to get use to the slight delay.

We can start working on the Automated D11 issue import, though the vast majority of it is not useable (it detects our D9/D8 BC layers and attempts to use D10 only features.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Joining from ๐Ÿ› UserAuth BC layer not working for modules that use username Active , same interpretation of root fault when using TFA 2.x (dev only) branch.

Linking the related issues to increase visibility.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

On high level glance it sounds like it is

Closing as a duplicate

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Opened ๐Ÿ› UserAuth BC layer not working for modules that use username Active as this change appears to have broken the login form when a decorator of the UserAuth services uses real account names that exist (the authenticate method is never called)

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Could not think of a better name for the ImageStyleDownloadController. Its internal and final so we can rename if we ever need to.

Did rename src/NewS3fsFileService.php to src/S3fsFileSystemD103.php.

Committing and tagging a release shortly.

๐Ÿ‡บ๐Ÿ‡ธ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โ€?

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

Setting parent issue to ๐ŸŒฑ [META] Stronger Typing for Entity API Active .

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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?

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara
\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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara
๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

As a test dependency we need to wait on Key:
๐Ÿ“Œ [META] Key for Drupal 11 Active

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States cmlara

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.

Production build 0.69.0 2024