Security Advisory coverage status is hardcoded on Recipes

Created on 6 December 2024, 16 days ago

Problem/Motivation

The function Drupal\project_browser\Plugin\ProjectBrowserSource\Recipes::getProjects() collects information about all available recipes to display them in Project Browser UI.

When looping via each recipe, the function hardcodes the SA coverage status of the each recipe - isCovered: TRUE:

        $projects[] = new Project(
          logo: [
            'file' => [
              'uri' => $logo_url,
              'resource' => 'image',
            ],
            'alt' => (string) $this->t('@name logo', [
              '@name' => $recipe['name'],
            ]),
          ],
          isCompatible: TRUE,
          isCovered: TRUE,
          projectUsageTotal: 0,
          machineName: basename($path),
          body: $description ? ['value' => $description] : [],
          title: $recipe['name'],
          author: [],
          packageName: $package_name,
          type: ProjectType::Recipe,
          url: $url ?? NULL,
        );

https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

It was added by this issue: https://www.drupal.org/project/project_browser/issues/3446257 ✨ Create a source plugin that exposes recipes in the file system Active (and this commit: https://git.drupalcode.org/project/project_browser/-/commit/7e060424b701...).

The information whether a recipe is covered by SA policy must be based on a projects/modules included in the recipe. So it should display that a recipes is covered by SA policy only if all included modules and their used releases/branches are covered as well (which is for example not a case of the Forms recipe included in Drupal CMS, because it includes an alpha release of the Webform). The same should also apply if a module will not be opted-in to the security advisory policy - then the recipe should not be marked as covered.

It is not good idea to hardcode a security information like this and it can lead to false sense of security.

I have added a Security tag and a Critical priority, as this have security implications.

Steps to reproduce

Open project browser (/admin/modules/browse)
Open a recipes tab
Observe that all recipes are covered by SA policy
https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

Proposed resolution

Based on the previous discussion, the core recipes can be marked as "Covered", but non-core recipes needs to have the coverage flag set according to the included modules and their releases/branches going to be installed.

πŸ› Bug report
Status

Active

Version

2.0

Component

Code

Created by

πŸ‡ΈπŸ‡°Slovakia poker10

Live updates comments and jobs are added and updated live.
  • Security

    It is used for security vulnerabilities which do not need a security advisory. For example, security issues in projects which do not have security advisory coverage, or forward-porting a change already disclosed in a security advisory. See Drupal’s security advisory policy for details. Be careful publicly disclosing security vulnerabilities! Use the β€œReport a security vulnerability” link in the project page’s sidebar. See how to report a security issue for details.

Sign in to follow issues

Comments & Activities

  • Issue created by @poker10
  • πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

    I think the answer here might be to allow the use of NULL in the isMaintained column, meaning 'unknown' and we would not show the shield for those.

  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    I think that it should just take the same status as the project that those recipes come from. We are looking for recipes in core and contrib modules in here: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

    Recipes from core can be safely put as "Covered", and the ones from contrib should be able to change depending on the module.

    When navigating through the above recipes (gathered from multiple sources), we already have an "if/else" block for core vs not-core in here: https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/src/Plug...

    So I suggest doing the calculations there and then adapting the code below with the calculated value.

  • πŸ‡¦πŸ‡ΊAustralia pameeela
  • πŸ‡¦πŸ‡ΊAustralia pameeela

    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.

  • πŸ‡ΊπŸ‡ΈUnited States greggles Denver, Colorado, USA

    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.

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡ΈπŸ‡°Slovakia poker10

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

  • πŸ‡¦πŸ‡ΊAustralia pameeela

    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?

  • πŸ‡ΈπŸ‡°Slovakia poker10

    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.

  • πŸ‡ͺπŸ‡ΈSpain fjgarlin

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch
  • πŸ‡ΊπŸ‡ΈUnited States greggles Denver, Colorado, USA

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

  • πŸ‡¬πŸ‡§United Kingdom catch

    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.

Production build 0.71.5 2024