#Security

Yeah I think showing nothing for recipes is better than hard-coding, and then we can try to figure out what it actually should show.

Agreed with #10 that the policy could change so it would be better to hardcode it. However I also think doing a quick fix to match the current policy might be worth doing anyway, because the final fix seems non-trivial to me.

Maybe the best short-term answer here is not to show anything.

In the long run it seems great if we could do the recursive calculating you mention, catch, and then have icons/text to explain "all modules are covered with stable releases" or "It's a mix of statuses and stable releases, get details."

for example the Forms recipe in Drupal CMS still contains Webform 6.3.0-alpha3, which means that you will end up with a not SA covered module(s) in specific cases

I think this might be a general issue with project browser and dependencies of projects that are going to be installed, rather than just recipes.

e.g. if I install https://www.drupal.org/project/webform_booking → that has stable 11.x support via project browser, this project depends on webform which is alpha.

Unless project browser recursively calculates the dependencies, and shows me all the things that are going to be installed and which version, then installing a stable, security supported project doesn't guarantee this across its dependencies.

I've replied to @pameela and @poker10's other points on #3447882-14: Determine some heuristics for module inclusion to avoid conflicts with core functionality, set expectations for security coverage etc. → because we're at risk of going off-topic here.

is the badge purely related to the opt in status

Yes.

I agree that if the concept of security status does not exist in recipes, we should just not show it, regardless of where the recipe comes from.

We can differentiate between true/false and null to get over the technical hurdle in the code. This way, we won't need to refactor too much. Just a check for null, and ignore in that case.

Webform 6.3.x (alpha) is used by 2,159 sites now and that is their decision (because it needs to be installed manually). For example it is not something, which is allowed in most company/goverment enviroments. But Drupal CMS/Forms recipe/PB will make the decision (without acknowledgment) instead of users, which is the difference.

which is not what users expect from a stable product

Does this mean that the >381,000 sites that are using webform should consider D11 unstable, because they can only use an alpha version of webform with it? I'm genuinely asking because although I completely understand the concerns, this feels quite far from the reality for most people building sites. If you were starting a new project today that had to use webform, would you really use D10 so that you had security coverage?

@pameeela We can hide it, but it overlaps with other issues - for example the Forms recipe in Drupal CMS still contains Webform 6.3.0-alpha3, which means that you will end up with a not SA covered module(s) in specific cases, which is not what users expect from a stable product (Drupal CMS 1.0.0). But if we solve this one 🐛 Missing information about the version of the project going to be installed Active , we can probably also list the all the modules which will be installed by a recipe?

Yeah I agree that a good first step would be to show nothing instead of misleading information.

What happens now in project browser if a module is opted into security coverage, but it depends on a module that is not opted into security coverage? Does project browser expand the dependency tree before installing dependencies or not? If it doesn't then this is probably applies to modules and themes too.

And is the badge purely related to the opt in status or does it also check for stable releases? Projects can and do opt into security coverage when they only have alphas, just means the coverage actually starts at stable. So if we need to conditionally show the badge this comes back to the other related issues here about which version will get installed.

But it's not showing the module status, it's showing the recipe status which is hard coded to say it's covered. It's certainly not checking the status of the modules required by the recipe. If that's what we all agree should happen that's fine, but it's pretty far from what's happening now.

This isn't much of an issue right now because the only recipes that are available are either from Drupal core or Drupal CMS, so they are vetted at least. And until we figure it out, I think it's better to remove the inaccurate info.

It seems important to show the coverage before the modules are installed on a site. The status is pretty prominent on the project page to help people make informed choices.

I think it might be better to just remove the security coverage status from recipes. As far as I know, there is no official policy around this yet anyway. A recipe might contain a bunch of contrib modules, some of which are covered and some not. There are a few other nuances that make it pretty unclear how security coverage would even work for recipes.

@ram4nd apologies for missing your comment.

If you can demonstrate an exploitable vulnerability that arises from e.g. having the old copy of jQuery in D7 core when it's not actually used by the application (which uses up-to-date jQuery libraries via jQuery Update), could you please report that in private to the Drupal Security Team? Thanks!

quietone → closed merge request !6329

quietone → reopened merge request !6329

Project Browser can install anything it wants regardless of the preferences in composer.json if it makes calls like $installer->require(['drupal/webform:@alpha']);

To do that, would it not need to know that webform:@beta didn't work first? I guess it could try with gradually decreasing levels of stability though?

Did a code review of the MR.

The factored out ::buildCreateAccessCid lgtm, it does what it announces, and defaulting complex cases to uncacheable seems reasonable.
Test coverage looks reasonable, though i did not think about each case. (Only found an Übernit in the code.)
Setting RTBC. You should revert though that if you think that more thorough test review is needed.

So it's up to the AccesControlHandler to add all "possible" $context variations to $cid. And if other things are passed in $context and used in an access hook, that is not supported (which may go to the docs...).

It's not my intent to bloat this, just to clarify.
Thanks a lot for pushing this forward!

But what about this: the $context (and things derived from it) CAN be used in hook_entity_create_access_alter, with no way to broadcast that fact back to the AccessControlHandler. (And it may add the need for additional caching contexts / $cid additions.)

Yes, hooks can use anything provided as by context and the handler doesn't know what they used. So everything in context needs to be considered when addressing caching, even if its not used by the handler. Things derived from context don't matter, they should be handled by caching context, and its up to hook implementations to add cacheable metadata themselves if they do anything more exotic.

But none of this matters in this issue. We already have a context. And we have caching. And caching ignores context, leading to false postives which are a bad thing, and so we should not use our current caching if we have context because no caching is much better than bad caching. And we should do this now, because its broken now and easy to fix, whereas making cool ways to cache context for edge cases is hard.

Thanks for the reply. You are right, mostly.

But what about this: the $context (and things derived from it) CAN be used in hook_entity_create_access_alter, with no way to broadcast that fact back to the AccessControlHandler.

???

This is a bug. And one with security implications. So in this issue probably we should fix it in the quickest, simplest way we can.

Creating a good modern-Drupal caching system for context is the subject of 📌 Use a memory cache in EntityAccessControlHandler Active . I'd be grateful if you could share the idea of using VariationCache there.

What things are passed in $context anyway?

We don't know, by design. The API is intentionally agnostic. That's a part of the challenge.

This needs a different approach.
The access result can vary by all sorts of context ($context, dereved from it, and external ones). This is the role of cache contexts to be returned with $accessResult (maybe from a hook_access_alter). These cache contexts can not be known in advance.
Sounds familiar? Yes, this is exactly what VariationCache is for.

a site is only allowing stable releases

To be clear -- the site is only allowing stable releases in the absence of a stability flag to override that. Project Browser can install anything it wants regardless of the preferences in composer.json if it makes calls like $installer->require(['drupal/webform:@alpha']);.

Yeah I think we could probably detect when something will definitely not get installed, but not when it might be possible to install but its own dependencies cause it to be not-installable, especially given some of those could be non-Drupal projects from packagist etc.

If we got the release data from the d.o API instead (which I assume project browser actually uses), could we then cross-reference this against what the site has set for composer minimum stability?

I think that might be doable. It'd be imperfect, since Composer is a bit of a black box, and the solving of transitive dependencies could affect what happens to higher-level dependencies, but at the very least we could probably give some kind of indication of what stability could be expected.

I imagine a power user would set minimum-stability to their comfort level and users using PB would just click install, not caring about module versions.

I don't think this can be relied on.

For example Gin has never had a stable release, and it's quite likely that someone would want to install Gin with project_browser. If their minimum_stability is stable, they won't be able to do that, so they'll be searching how to install a release candidate.

Overall I think it's a good thing if there's some friction involved in that.

But assuming they figure out how to change their minimum stability globally, the next project they install could also be in permanent beta/rc and project browser will happily install it without them even realising it's not stable.

The problem is that the output isn't really human-readable (and I'm not sure if it can be made machine-readable), and if Composer doesn't leave behind any artifacts for us to parse (for example, the lock file that would result from a real run), then I'm not sure we have anything we can produce a user-facing summary from.

This makes sense.

If we got the release data from the d.o API instead (which I assume project browser actually uses), could we then cross-reference this against what the site has set for composer minimum stability?

This would also potentially pre-warn people when they're not going to be able to install the project then - not sure what the messaging looks like when project browser fails to install a project.

To be clear, Drupal CMS does use a permissive minimum-stability, but it also has prefer-stable engaged, which I'd hope would mitigate the possibility of weirdness to some degree. But I do agree that the wider net Drupal CMS casts around its dependencies makes things slightly less predictable.

Yes this is the problem - if people install from composer templates that set minimum stability to something other than stable, which will be the case for any pre-release of a distribution like Drupal CMS (and maybe stable releases if the stability doesn't get raised before the release day), then they'll be defaulted to alpha or beta or rc without knowing about it. This might not be project browser's problem as such but the combination of the two seems very bad.

I imagine a power user would set minimum-stability to their comfort level and users using PB would just click install, not caring about module versions.

@chrisfromredfin - Was there some research done on this topic? From the security point of view, I do not think this will be a general practice. If we look at the Wordpress, you can see, that there is a plugin version displayed if you click on the plugin - so you are aware, what you are going to install. Modules can also have multiple branches, with some new features and/or BC breaks (so you will be affected even if minimum stability will be stable). Not saying that you can jump from security covered release to not covered. This seems to me very important that users are aware, what is going to be installed.

And Drupal CMS set minimum stability to alpha (as of now), so that is even worse: 🐛 Alpha stability flag in composer.json allows project_browser to download any alpha stabiility module Active . And we are talking about non-dev users here.

@chrisfromredfin asked me to reply to #5.

From Package Manager's perspective, we could pass a --dry-run flag to composer require. The problem is that the output isn't really human-readable (and I'm not sure if it can be made machine-readable), and if Composer doesn't leave behind any artifacts for us to parse (for example, the lock file that would result from a real run), then I'm not sure we have anything we can produce a user-facing summary from.

As for minimum-stability, Package Manager has no real opinion on that as far as I know. We just delegate to Composer. We could certainly supply stability flags to the StageBase::require() method to override whatever the minimum-stability is, same as you could do with composer require.

Related to this, does project_browser/package_manager respect a minimum stabiility flag?

Yes, PM does the equivalent of `composer require drupal/some_project` so that's why we don't really know what package will be installed, because it specifically depends on Minimum Stability.

What is the minimum-stability set to in drupal/recommended-project? In institutional settings, I imagine a power user would set minimum-stability to their comfort level and users using PB would just click install, not caring about module versions. That's very much in our user world - I would be reticent to add more additional info to cards, and would be more inclined to support pre-or-post messaging to the user per ✨ Add confirmation page when installing module Active or ✨ Show users what's changed on confirmation screen Active .

A pre-or-post confirmation screen also may relate to ✨ Create a way to accept recipe input when applying one with Project Browser Active .

This is a blocker (not sure if soft or hard) for contrib in 📌 Add 'create * host permissions' Postponed