If the next minor version of core has a security release, status still says "Security update required!" even if the site is on an equivalent, secure release already

Same issue, here. Ran "drush pm-update drupal" and received "Project drupal was updated successfully. Installed version is now 8.1.10." Also ran drush updb and drush cr.

https://www.drupal.org/project/drupal/releases/8.2.0-rc2 was also marked as a security release since it had the same fixes. We removed this and expect this issue to be fixed short-term for 8.1.10 installs.

I think core is looking to see if it has the latest security release in https://updates.drupal.org/release-history/drupal/8.x, without considering 8.2.x being different from 8.1.x.

In core/modules/update/update.compare.inc, update_calculate_project_update_status() looks like it processes all the releases linearly. So a security release in 8.2.x makes it think all of 8.1.x is insecure.

This might be a decent place to start digging into this:

    if (isset($release['terms']['Release type'])
        && in_array('Security update', $release['terms']['Release type'])) {
      $project_data['security updates'][] = $release;
    }

Non-full-releases do occasionally get marked as security releases. In this case, I think it makes sense. Alerting your 8.2.0-rc1 install that it isn't secure is a nice thing to do.

IIRC, there is some overlap when full releases of 8.2.x and 8.1.x are supported. If there happened to be a security release during that window, we'd hit this bug quite a bit harder.

And for future contrib semver support, we should have this. Contrib modules can and will make up their own schemes for what branches are supported.

I can confirm that this is registering properly.

So a security release in 8.2.x makes it think all of 8.1.x is insecure.

Once 8.2.0 ships in two weeks, any security release in the 8.2.x series will actually mean all of 8.1.x is insecure, because we will not make another 8.1.x. So the current behavior probably is actually better than the alternative. I guess we can avoid marking RCs as security releases rather than risking some site on 8.1.10 not know they need the security fixes that ship in the next security release of 8.2.x.

IIRC, there is some overlap when full releases of 8.2.x and 8.1.x are supported. If there happened to be a security release during that window, we'd hit this bug quite a bit harder.

For 8.1.0, we had the final security release window of 8.0.0 on the same day, so if we had shipped a sec release that day the issue would indeed have been problematic. However, we've since changed this for a few reasons. In the future, minor support ends with the final security release window before the minor release (barring a zero-day or something that requires an emergency security release) and the minor is always scheduled on a patch release window.

All of this is specific release policy though that could change in the future, so it probably does not make sense to encode it for d.o or the update manager. What would make sense would be a way to indicate which SAs are included in a security release, because any release that includes 2016-004 is equivalent from a security perspective. They are valid now but will not be once there is a release that contains (say) a future 2016-005 or 2017-001 or whatnot.

Agreed, this sort of thing does get into surprising messiness. I added a couple related issues. They could use a bit more detail, so I’m not thinking about closing this as a duplicate yet.

#2461167: Create Security announcement content type will get us a bit closer to being able to provide which SA is for which release.

Retitling since I forgot why this was still open after we made 8.2.0-rc1 not a security release anymore.

To actually support this, I think we would need a way of saying that two security release include the same SA's worth of security fixes. For example, using a scenario like:

• 8.6.0 is released October 2018
• An SA is relesed in 8.6.1 two weeks later during the October window; we decide we also want to release 8.5.9 on that date since 8.5.x users have had only a couple weeks to deal with any needed updates related to internal code, behavior changes, intentional breaks, etc.
• 8.6.2 is released normally on November whateverth
• 8.6.3 is released as a security release on the November security window.

So in October, having 8.6.1 should show you green cheer. Having 8.5.9 should be yellow neutral-face and indicate that there are updates available, but not that said updates are security update.

But after the November window, we absolutely need to make sure both 8.5.9 and 8.6.1 are marked as insecure angry-face.

The API-equivalent releases would hopefully have the same release date, but that's definitely a fragile thing to rely on (lol timezones). So we do need some added API or functionality to support this.

Keep in mind that it's also possible for 8.5.9 to be released the last security window before 8.6.0, and become insecure with the release of 8.6.1, or going the other direction, for 8.5.10 to be released along with 8.6.3 and be equivalent in terms of security support even though very deficient in features and that's starting to look like security support for two minor branches which we do not want and not a part of any release proposal. However, there could be a scenario where we'd do that, even.

To support this, I think the core update feed would need to include information on what SAs are included in the core release. Otherwise, core has no meaningful way of comparing security releases other than which version number is higher, and if there is a security update with a higher version number, that one will be safest. It may just be equally safe to the one you're on if you were on the previous minor. You still should update because we don't support the minors concurrently; they're just the release-by-release evolution of one major version. The only difference is that you might install a security update, get the message again, and then do what is only a bugfix and feature update thereafter.

I’m leaning more toward putting something more-direct in update status XML than SAs included. This puts the logic burden on each Drupal installation; if we update it, we have to wait for a core update. If we add something to the XML, I’d rather have a boolean that says “this release is after all known SAs, at this time.” (Kinda like #2766491: Update status should indicate whether installed contributed projects receive security coverage does for security advisory coverage.)

Better yet, I think, would be marking branches as unsupported, like module maintainers can do for major versions. So a core maintainer can mark all 8.5.x as unsupported on Drupal.org at the appropriate time. IIRC, this is also quite red in update status, but says “unsupported” instead of security update required. This would have to go with core’s logic for a security updates, 8.6.1 being a security update should have no effect on 8.5.x releases.

The first paragraph of #15 sounds like the best approach for now. So it would just become part of security release procedures to mark the releases that contain the current SA, as well as to un-mark previous security releases that do not contain the current SA? Since not all security fixes will be relevant for different branches.

The second paragraph of #15 seems like a separate scope to me, since d.o currently (correctly) assumes that 8.4.0 supercedes 8.3.9, etc., but that is a policy rather than a technical requirement.

I did a bit more reading of update.compare.inc, and it looks like core already has a way to mark specific releases as insecure:

      if (isset($release['terms']['Release type']) &&
          in_array('Insecure', $release['terms']['Release type'])) {
        $project_data['status'] = UPDATE_NOT_SECURE;
      }

This release type has never existed on www.drupal.org as far as I know. We should be able to:

• Make sure this vestigial code works.
• Add that term to …/admin/structure/taxonomy/vocabulary_7 on www.drupal.org.
• Lock it down as needed. I’m not sure if everyone making releases should have access to use that.
• Mark specific core releases as insecure on Drupal.org as needed. (Can be script-assisted.)
• Change update module’s logic to either depend on this for core; or update the logic for a security updates to be treat minor versions as the series instead of major, 8.6.1 being a security update should have no effect on 8.5.x releases.

Mark specific core releases as insecure on Drupal.org as needed. (Can be script-assisted.)

This would be every core security release every time there is a core SA, unless we specifically released an EOL sec release or security-covered release candidate.

It could also mean having to pay attention to whether unsupported branches include the vulnerabilities or not, which I'd rather not have to even think about. What we don't want to do though is give anyone the impression that 8.1.10 is just "unsupported" rather than "SECURITY UPDATE REQUIRED!".

To clarify, it could theoretically be possible that the first 8.3.x security release include issues that only affect 8.3.x. But eventually there will most likely be one in there that includes 8.2.x. I would not have to go back and figure that out each time.

This patch adds minor version parsing to update_process_project_info(), and for core only, awareness of that to update_calculate_project_update_status().

For testing, I faked the version number by editing const VERSION = … in core/lib/Drupal.php. Setting that to something completely insecure, 8.1.2, looks like:

All future 8.x.x security updates showing is existing behavior. I’m keeping that.

Without a change to Drupal.org, if I fake my core version number to 8.1.10, the final 8.1.x security release:

So it is now possible to have a future security release in the next minor version without saying what you are running is insecure. The security update is still mentioned as a possible upgrade.

I faked an update status server by:

• Getting the attached 8.x.txt to serve at http://drupalvm.dev/release-history/drupal/8.x
• Adding project status url: 'http://drupalvm.dev/release-history' to core/modules/automated_cron/automated_cron.info.yml (the update status url is populated from the first .info.yml file processed)
• In the XML, I added <term><name>Release type</name><value>Insecure</value></term> to 8.1.10.

Now, the same fake core version is labelled as insecure. Once an old minor branch becomes completely insecure, we would set this term on the patch releases after the final security release, of that minor version’s branch.

Kinda guessing at how to fix the coding standards messages - I added use \Drupal\update\UpdateManagerInterface; and now reference the constants like UpdateManagerInterface::NOT_SECURE;

Wow those reports are really overwhelming. Does the update status normally list every security update rather than just the most recent one?

Like, even on 8.1.2, should definitely not offer 8.1.3, 8.1.7, or 8.2.3 when 8.1.10 and 8.2.7 are available. Does that already happen?

Yep, I was surprised too. People working on core must not see many really outdated sites.

I made a separate issue for slimming down the security updates offered: #2865920: When a site is multiple security releases behind for a given project, they are all listed in a paralyzing wall of terror

I missed the security updates from before 8.1.10 in my last screenshot.

Otherwise, ignore unpublished, insecure, or unsupported releases was being hit before Stop searching once we hit the currently installed version. This patch rearranges the code to do those in a better order, along with the Look for the … sections.

+++ b/core/modules/update/update.compare.inc
@@ -438,8 +447,20 @@ function update_calculate_project_update_status(&$project_data, $available) {
+    if ($project_data['project_type'] === 'core') {
+      // For core, mark the installed release as insecure if there are security
+      // releases which match the minor version number component.
+      foreach ($project_data['security updates'] as $security_update) {
+        if ($security_update['version_minor'] == $project_data['existing_minor']) {
+          $project_data['status'] = UpdateManagerInterface::NOT_SECURE;
+          break;
+        }
+      }
+    }

So this bit is the bit that's problematic. A security release within that minor branch might be secure... but it also might not be if the branch is EOL. So at a certain point the release is insecure.

I think that rather than magic quessing and boolean, we should consider just marking releases with the latest SA they include. If we release a 2018-SA-CORE-002 that fixes both branches, the update module should know that and not display a warning.

But if we then release 2018-SA-CORE-003 and that fix is not backported to the previous branch, then that branch is not marked for the latest SA and should be shown as insecure.

The boolean in #15 is a similar idea I guess. We could add some process for that on d.o:

• It's a "latest seccurity release" checkbox on your release note, under where it says "security release".
• By default, all older release notes get that flag unset when you submit your new release. A submit handler for the release node form, I guess.
• Then, maintainers can optionally go back to their other secure release and set the flag there as well.

All of this would be totally agnostic of which actual releases, branches, etc., since those are things that change and differ from project to project.

Thoughts?

Bumping this to major since it's important to solve for our security release processes, especially for core.

@larowlan and I think this should maybe be critical, since affects how site owners receive security updates.

Specifically, this is a priority to solve because there’s a good chance of simultaneous 8.x security releases when pre-releases are happening next to supported releases. Those pre-releases aren’t technically covered by security advisory policy, but it would still be good to mark them as security updates.

I filed a followup for Drupal.org based on #15 and later comments (yes we do want some automation/options to mass-mark releases as insecure) at #2942591: Start reporting specific releases as insecure in update status XML.

#21 has some details on testing, as far as I can tell from what I wrote, it worked. But there is no core test coverage for update status XML containing <term><name>Release type</name><value>Insecure</value></term>

Updating the title a bit and rewriting the IS to describe the actual requirements/background info.

This is the same patch as #27, it still applies cleanly to 8.6.x and tests pass. Just fixing the coding standards message.

#2942591: Start reporting specific releases as insecure in update status XML has been deployed to Drupal.org, so some releases are now being reported as Insecure, like https://updates.drupal.org/release-history/jsonapi/8.x

Mentioning _update_equivalent_security_releases() in case someone searches for that function.

So the current patch on this issue is out of date, because of a couple improvements to Drupal.org:

• Release managers and security team members have the option to manually mark individual releases as secure or insecure:
  
• When a new security release is created, by default, all older releases are automatically marked as insecure. However, the release manager may override this when publishing a release to indicate that specific past releases are also still secure (for example, if two security releases on different minor branches contain the same security fix, or if the security fix only affects the most recent branch, etc.):
  

So all we need to do now for core is change it to only pay attention to whether Drupal.org reports the release as insecure or not, not whether there's a security release with a higher version. Then _update_equivalent_security_releases() can be removed.

The simplest way to test this patch during development is to install HEAD but hack the version string in core/lib/Drupal.php, and change _update_equivalent_security_releases() to return an empty array.

Two interesting releases currently are Drupal 8.5.3 and 8.4.8. Expected results on the status report:

(Note: this is valid until the next core security release which will change what d.o reports.)

So all we need to do now for core is change it to only pay attention to whether Drupal.org reports the release as insecure or not, not whether there's a security release with a higher version.

The insecure release label was set on only past core releases on Drupal.org. The data hasn’t been populated on all contrib projects with security releases. We should verify it is being used correctly for recent contrib security releases; then we can backfill the rest.

In the meantime, the hybrid approach in the last patch is best.

Ah, I didn't know that contrib hadn't been backfilled yet. Maybe we can have a followup for contrib, and ensure that we are dealing with core only in this issue?

+++ b/core/modules/update/update.compare.inc
@@ -430,8 +439,20 @@ function update_calculate_project_update_status(&$project_data, $available) {
-    // If we found security updates, that always trumps any other status.
-    $project_data['status'] = UPDATE_NOT_SECURE;
+    if ($project_data['project_type'] === 'core') {
+      // For core, mark the installed release as insecure if there are security
+      // releases which match the minor version number component.
+      foreach ($project_data['security updates'] as $security_update) {
+        if ($security_update['version_minor'] == $project_data['existing_minor']) {
+          $project_data['status'] = UpdateManagerInterface::NOT_SECURE;
+          break;
+        }
+      }
+    }

This is the logic that is problematic. It's changing the logic from "A new security release means all previous ones are insecure" to "A new security release means all previous security releases on this minor branch are insecure", which is wrong. Let's say there was never a security release on 8.4.x at all (there almost wasn't). When the first security release of 8.5.x came out, if there isn't an equivalent secure release of 8.4.x, all 8.4.x and earlier releases should still say "security update required!" Having logic for both is dangerous.

Meanwhile, some manual tests and testing will help. The goal of this issue is to get rid of _update_equivalent_security_releases(), make the "insecure release" data on d.o the sole arbiter of core secureness, and get it in sooner rather than later because even once we fix it the manual process around _update_equivalent_security_releases() will have to stick around for an entire minor release afterward since a site on an old release will not yet have this code.

@tedbow is currently working on tests for this issue.

Ok. I have add more test cases for security updates. The only test case that currently fails is the case where the current minor version has a security update it also shows the next versions security update in the list.

Here are the screenshots @tedbow provided me WRT the case that's failing in #45. Before the patch, with 8.4.8, as of today when 8.4.8 includes the latest security advisory:

(Edit: See the following comment for the "after".)

Adding an annotation to make the issue clearer:

1. Is the idea that, in the fixture's scenario, 8.1.1 and 8.0.2 were the most recent security release, and 8.1.2 is a subsequent patch release? If so, I think that would mean 8.0.1 and 8.0.0 should also be marked as insecure releases in the fixture. (Currently only 8.1.0 is marked as insecure.)

   Edit: Just noticed that it's actually two fixtures, but same idea.

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,70 @@ public function testMajorUpdateAvailable() {
   +      '0, 0.2' => [
   +        'current_minor' => 0,
   +        'security_releases' => ['0.2'],
   +        'expected_security_release' => '0.2',
   +      ],
   +      '1, 1.2' => [
   +        'current_minor' => 1,
   +        'security_releases' => ['1.2'],
   +        'expected_security_release' => '1.2',
   +      ],
   +      '0, 0.2 1.2' => [
   +        'current_minor' => 0,
   +        'security_releases' => ['0.2', '1.2'],
   +        'expected_security_release' => '0.2',
   +
   +      ],
   +      '1, 0.2 1.2' => [
   +        'current_minor' => 1,
   +        'security_releases' => ['0.2', '1.2'],
   +        'expected_security_release' => '1.2',
   +      ],
   +      '0, 1.2' => [
   +        'current_minor' => 0,
   +        'security_releases' => ['1.2'],
   +        'expected_security_release' => NULL,
   +      ],
   

   Can we add inline comments for each case in the provider explaining the condition and expected result? (So that the reader doesn't need to sift through the fixture to see what's what.)

3. expected_security_release also seems incorrect to me as a provider key, or at least incomplete. I would have expected a boolean to indicate whether the given release is considered secure or not.

   Also, I think we shouldn't use the word "current" minor version, or shouldn't frame the test in that way. One of the reasons I want to not add special logic about whether it's a "current" minor or not is that this hardcodes release policy in the Update module. We're planning to adopt a policy where there's one minor that's in active support and one that's in security support only, but we could theoretically have a policy where there were two minors in active support, or something else entirely. So I don't think Drupal should try to guess about that. That's why I think we should only rely on the metadata from d.o to tell us whether the releases are secure following #2942591: Start reporting specific releases as insecure in update status XML.

4. --- /dev/null
   +++ b/core/modules/update/tests/modules/update_test/drupal.0.1.2-sec.xml
   
   --- /dev/null
   +++ b/core/modules/update/tests/modules/update_test/drupal.0.2_1.2-sec.xml
   

   Is the naming of the fixtures with numbers part of the existing test, or could we give them descriptive names?

@xjm thanks for the review!
1. there are more fixtures now. I have added a new parameter $fixture so you can easily find the fixture file.
2. Added inline comments.
3. I like expected_security_release It is the expected security release for the test case.
I changed current_minor to site_minor because it is really what minor the site is on.
4. We could extend the fixture names but they would get very long. I think because now the test cases explicitly point to the fixture the names will matter a little less.

The last 10 contrib security releases on Drupal.org look like they've all properly labeled previous releases as insecure. So I'm comfortable with writing a script to back-fill all contrib security releases. This should be straightforward, but edge cases surface when looking this far back, and updating the release history XML might take some time. I'd like to help avoid separate core and contrib logic if possible.

Thanks @tedbow and @drumm!

1. site_minor is much clearer, +1.
2. Question about this:

   It is the expected security release for the test case.

   I am not sure what that means though. :)

   So there are a few different things we are checking (some of which are in scope here):

   • Whether the site says "Security update required!" Does a NULL for expected_security_release mean this message is not displayed, and any non-null value mean that it is?
   • When the site says "Security update required!", which security release(s) are listed. Maybe we should call this recommended_security_release or security_release_displayed or something?

     In a followup issue we will also probably want to list both the latest security release for the site minor and the latest security release overall. Not in scope here, but we should

   • Whether the site lists a latest release, and if so which it is. That might be a recommended_latest_release or something (not in scope for this issue, but maybe for our followups WRT the other test cases).

3. We could extend the fixture names but they would get very long. I think because now the test cases explicitly point to the fixture the names will matter a little less.

   Maybe we can at least change them to something that's at least more than a integer? The reason is that it's hard to tell at a glance what the fixture name is versus the Drupal release in the fixture. We could have something like scenario_security_2a maybe. Like in the actual filename also.

Removes test for out of scope issues:

1. Older security releases shown. Existing issue #2865920: When a site is multiple security releases behind for a given project, they are all listed in a paralyzing wall of terror
2. Current release not mark as "insecure" even when there is a newer security release. Create new issue: #2989616: The update report does not honor drupal.org's info about what releases are secure or not

I think this should be green. Is there other test cases we need to cover in this issue?

For posterity, and as a backup, here is the main round of releases being marked as insecure. Format is

the_security_release 3.11
  now_insecure_previous_version 3.10

The indented lines are the ones being marked as insecure. This does not cover pre-releases (alpha/beta/etc) labeled as security releases, because the logic I’m hijacking for this wasn’t written with that in mind.

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,122 @@ public function testMajorUpdateAvailable() {
   +        'site_patch_verision' => '0.0',
   

   Spelling verision

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,122 @@ public function testMajorUpdateAvailable() {
   +      // No security release available for site minor release 0 but not
   +      // marked insecure.
   +      // Security release also available for next minor.
   +      '0.0, 1.2' => [
   +        'site_patch_verision' => '0.0',
   +        'security_releases' => ['1.2'],
   +        'expected_security_release' => NULL,
   +        'fixture' => '1.2-sec',
   +      ],
   +    ];
   

   We need 1 more test case where:

   • There is security release for next minor
   • All the current minor releases are marked insecure

   adding this test case which emulates being a minor that is not longer receiving security updates and the next minor security update is needed.

   The test should pass.

3. re #56 @drumm for now this patch as separate logic for core and contrib because there will some contrib projects where the Insecure tag is not applied yet, correct?

   Should we make a follow to remove this separate logic when Insecure tag is propagated to all projects?

The Insecure tag should be applied to all applicable releases sometime tomorrow. I’d go ahead and take out the separate core and contrib logic.

@drumm ok thanks. this patch removes the core specific logic in the patch.

I don't think there is any test for contrib security updates. I work on making the ones for contrib that I add for core.

Whoops

The releases from #56 have all been backfilled and marked as insecure. The remainder is attached and running now.

The releases from #63 have updated now. Now update status XML simply reports “Insecure” if insecure; and all other security-update-related logic can be discarded.

I think the patch we want starts like this:

diff --git a/core/modules/update/update.compare.inc b/core/modules/update/update.compare.inc
index 2ba5fdce75..22f26b93d8 100644
--- a/core/modules/update/update.compare.inc
+++ b/core/modules/update/update.compare.inc
@@ -429,11 +429,6 @@ function update_calculate_project_update_status(&$project_data, $available) {
   // Check to see if we need an update or not.
   //
 
-  if (!empty($project_data['security updates'])) {
-    // If we found security updates, that always trumps any other status.
-    $project_data['status'] = UPDATE_NOT_SECURE;
-  }
-
   if (isset($project_data['status'])) {
     // If we already know the status, we're done.
     return;

There is probably more dead code around it.

@xjm yes I figured out yesterday that could remove that logic. All the new tests pass.

I just add to update on of the contrib test files to add the insecure tag.

Re #61

I don't think there is any test for contrib security updates. I work on making the ones for contrib that I add for core.

I am working on this now.

Oh, let's also add this:

diff --git a/core/modules/update/update.module b/core/modules/update/update.module
index 6f1ab74dcf..b7448ca57f 100644
--- a/core/modules/update/update.module
+++ b/core/modules/update/update.module
@@ -404,24 +404,6 @@ function update_get_available($refresh = FALSE) {
     $available = \Drupal::keyValueExpirable('update_available_releases')->getAll();
   }
 
-  // Check for security releases that are covered under the same security
-  // advisories as the site's current release, and override the update status
-  // data so that those releases are not flagged as needed security updates.
-  // Any security releases beyond those specific releases will still be shown
-  // as required security updates.
-
-  // @todo This is a temporary fix to allow minor-version backports of security
-  //   fixes to be shown as secure. It should not be included in the codebase of
-  //   any release or branch other than such backports. Replace this with
-  //   https://www.drupal.org/project/drupal/issues/2804155.
-  foreach (_update_equivalent_security_releases() as $equivalent_release) {
-    if (!empty($available['drupal']['releases'][$equivalent_release]['terms']['Release type'])) {
-      $security_release_key = array_search('Security update', $available['drupal']['releases'][$equivalent_release]['terms']['Release type']);
-      if ($security_release_key !== FALSE) {
-        unset($available['drupal']['releases'][$equivalent_release]['terms']['Release type'][$security_release_key]);
-      }
-    }
-  }
   return $available;
 }
 
@@ -443,21 +425,7 @@ function update_get_available($refresh = FALSE) {
  *   https://www.drupal.org/project/drupal/issues/2766491.
  */
 function _update_equivalent_security_releases() {
-  switch (\Drupal::VERSION) {
-    case '8.3.8':
-      return ['8.4.5', '8.5.0-rc1'];
-    case '8.3.9':
-      return ['8.4.6', '8.5.1'];
-    case '8.4.5':
-      return ['8.5.0-rc1'];
-    case '8.4.6':
-      return ['8.5.1'];
-    case '8.4.7':
-      return ['8.5.2'];
-    case '8.4.8':
-      return ['8.5.3'];
-  }
-
+  // @todo Deprecate me.
   return [];
 }
 

I also think we can now remove the changes for parsing the minor version and moving the logic for it.

The docblock for update_calculate_project_update_status() also has some docs that will need to be updated. This bit:

Finally, and most importantly, the release history continues to be scanned until the currently installed release is reached, searching for anything marked as a security update. If any security updates have been found between the recommended release and the installed version, all of the releases that included a security fix are recorded so that the site administrator can be warned their site is insecure, and links pointing to the release notes for each security update can be included (which, in turn, will link to the official security announcements for each vulnerability).

Oh also. I'd really like to commit a patch that includes all the test cases that are passing in HEAD, including the previously removed ones, before we make this change. Can we get a separate issue for that? Review and commit should go fairly quickly, and then this issue can just add the test cases relevant for the change.

Here are tests for contrib. Some are failing

+++ b/core/modules/update/update.compare.inc
@@ -30,11 +32,14 @@ function update_process_project_info(&$projects) {
+      if (preg_match('/^(\d+\.x-)?(?<major>\d+)\.((?<minor>\d+)\.)?.*$/', $info['version'], $matches)) {

This regex doesn't actually work for contrib.
So for 8.x-1.3 it won't find 3 as the minor version. This may be causing the test failure.

Also in this sometimes the last character in the the contrib version is called "patch" and here it is called "minor"

Removed debug line left in.

#2990511: Add comprehensive test coverage for update status for security releases is now RTBC. Rebuilding this patch on top of it.

1. Adding explicit test cases for contrib and core on the latest sec release for major version and when there is also a sec release for next major. These should pass now.
2. Removing the big block of code that in update.compare.inc that was moved dow the file unchanged. This is no longer necessary now that the Insecure tag has been propagated to all projects.

UPDATE: I should have made 2804155-76.patch a do-not-test patch file. canceled test.

1. Looking the original changes to update.compare.inc in this patch I was trying figure what are part actually is needed for this fix.

   Re #72 the regex changes don't actually work for contrib.

   So I removed them.

   +++ b/core/modules/update/update.compare.inc
   @@ -425,15 +432,6 @@ function update_calculate_project_update_status(&$project_data, $available) {
   -  //
   -  // Check to see if we need an update or not.
   -  //
   -
   -  if (!empty($project_data['security updates'])) {
   -    // If we found security updates, that always trumps any other status.
   -    $project_data['status'] = UPDATE_NOT_SECURE;
   -  }
   -
   

   This part seems to be the only part that is necessary.

Do #67 and #69 still need addressed? Could you upload a test only patch as well to show what doesn't work in HEAD?

@samuel.mortenson good point

1. re #67 remove the this logic but also removed _update_equivalent_security_releases() and not just deprecated it. looking at #2989243: _update_equivalent_security_releases() should not diverge per branch I don't understand why it just needs to be deprecate because it not called anywhere and since it start with "_" I think by policy it is an internal function.

   I am probably missing something though.

2. #69

   I think this

   all of the releases that included a security fix are recorded so that the site administrator can be warned their site is insecure, and links pointing to the release notes for each security update can be included

   refers to this bug(or feature) #2865920: When a site is multiple security releases behind for a given project, they are all listed in a paralyzing wall of terror

   I don't think anything in this patch addressed that problem so changing seems out of scope.

   That being said I can't not reproduce #2865920: When a site is multiple security releases behind for a given project, they are all listed in a paralyzing wall of terror by checking out 8.7.x and changing \Drupal::VERSION to 8.1.10

   I have manually tested and with or without this current patch in 8.7.x if change \Drupal::VERSION to 8.1.10 after update_calculate_project_update_status() is run called in \update_calculate_project_data()(it's only call).

   $projects["drupal"]["security updates"] only has 1 security release 8.5.6.

   Well.... I see that @xjm already made a comment to that effect
   Since that issue will now be used to add test coverage it seems reasonable to make the comment change there.

3. Adding the test only patch

+++ b/core/modules/update/update.compare.inc
@@ -154,7 +154,7 @@ function update_calculate_project_data($available) {
- * included a security fix are recorded so that the site administrator can be
+   * included a security fix are recorded so that the site administrator can be

Nit: some extra indentation here.

Fixing #83, not uploading combined patch.

We're waiting for #2990511: Add comprehensive test coverage for update status for security releases still but once that's in I consider this RTBC.

+++ b/core/modules/update/update.module
@@ -405,63 +405,9 @@ function update_get_available($refresh = FALSE) {
-function _update_equivalent_security_releases() {
-  switch (\Drupal::VERSION) {
-    case '8.3.8':
-      return ['8.4.5', '8.5.0-rc1'];
-    case '8.3.9':
-      return ['8.4.6', '8.5.1'];
-    case '8.4.5':
-      return ['8.5.0-rc1'];
-    case '8.4.6':
-      return ['8.5.1'];
-    case '8.4.7':
-      return ['8.5.2'];
-    case '8.4.8':
-      return ['8.5.3'];
-  }

This is indeed internal, but: https://www.drupal.org/core/deprecation#internal

So let's make it throw a deprecation warning instead.

Since the function will no longer be maintained once we land this, we should also consider: https://www.drupal.org/core/deprecation#how-unintended

1. Adding back _update_equivalent_security_releases() as per #86

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -160,20 +160,229 @@ public function testMajorUpdateAvailable() {
   +        'expected_security_release' => '0.2',
   +        'update_available' => FALSE,
   

   The new test cases that are being added now have case where there are security releases available but not required.

   So changing 'update_available' to 'expected_update_message_type'. This will have the possible values

     /**
      * Denotes a security update will be required in the test case.
      */
     const SECURITY_UPDATE_REQUIRED = 'SECURITY_UPDATE_REQUIRED';
   
     /**
      * Denotes an update will be available in the test case.
      */
     const UPDATE_AVAILABLE = 'UPDATE_AVAILABLE';
   
     /**
      * Denotes no update will be available in the test case.
      */
     const UPDATE_NONE = 'UPDATE_NONE';
   

whoops heres the patch.

If you want to run the test locally combine with #2990511-47: Add comprehensive test coverage for update status for security releases

1. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -273,7 +281,7 @@ public function securityUpdateAvailabilityProvider() {
            // @todo Change to use fixture 'sec.0.2-rc2' in
            // https://www.drupal.org/node/2804155. Currently this case would fail
            // because 8.2.0-rc2 would be the recommend security release.
   

   This todo is for this issue. Fixing

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -284,14 +292,28 @@ public function securityUpdateAvailabilityProvider() {
            'expected_security_releases' => [],
   -        'update_available' => FALSE,
   +        'expected_update_message_type' => static::SECURITY_UPDATE_REQUIRED,
   

   Whoops this has no security updates but also static::SECURITY_UPDATE_REQUIRED

   Let make sure a test like this would fail and fix this.

Rerolled after #2990511: Add comprehensive test coverage for update status for security releases

Looks good to me! Nice work.

Looks good to me too.

Here is a test only patch to prove the new cases would not pass with the fix.

Expected fail in test only patch.

1. +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   @@ -445,14 +445,14 @@ public function testHookUpdateStatusAlter() {
   -   * @param bool $update_available
   -   *   Whether an update should be available.
   +   * @param string $expected_update_message_type
   +   *   The type of update message expected.
   

   +1 for this; I was thinking after I committed the last issue that something like this would be good. However, maybe we should fix it in a separate issue first to avoid confusion with what's the key fix here?

2. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -239,16 +239,24 @@ public function securityUpdateAvailabilityProvider() {
   +      // Site on latest security release available for site minor release 0.
   +      // Minor release 1 also has a security release.
   

   For the code comment here (and possibly elsewhere) it matters whether the security release for the previous minor is marked insecure or not. Probably we should mention that.

3. +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -272,25 +280,36 @@ public function securityUpdateAvailabilityProvider() {
   -        'update_available' => FALSE,
   -        // @todo Change to use fixture 'sec.0.2-rc2' in
   -        // https://www.drupal.org/node/2804155. Currently this case would fail
   -        // because 8.2.0-rc2 would be the recommend security release.
   -        'fixture' => 'sec.0.2-rc2-b',
   +        'expected_update_message_type' => static::UPDATE_NONE,
   +        'fixture' => 'sec.0.2-rc2',
   

   I checked whether the "b" fixture was still in use. It's used for the foreach loop for pre-release versions still.

4. index 0000000000..8665e46b36
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/update/tests/modules/update_test/drupal.sec.1.2_secure.xml
   

   Can we add this fixture to our list on the data provider?

5. +++ b/core/modules/update/tests/modules/update_test/drupal.sec.1.2_secure.xml
   --- a/core/modules/update/tests/src/Functional/UpdateContribTest.php
   +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   
   +++ b/core/modules/update/tests/src/Functional/UpdateContribTest.php
   --- a/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   
   +++ b/core/modules/update/tests/src/Functional/UpdateCoreTest.php
   @@ -272,25 +280,36 @@ public function securityUpdateAvailabilityProvider() {
   +        'expected_update_message_type' => static::UPDATE_NONE,
   +        'fixture' => 'sec.0.2-rc2',
   

   So changing this from sec.0.2-rc-2b to sec-0.2-rc2 as the effect of switching it to a schema where there is a security release for an RC of the 8.2.0 version. That's one of the bugs we're trying to solve here. 👍

   However, this calls my attention to the fact that the site fixtures seem to be named incorrectly. Shouldn't they be (e.g.) sec.2.0-rc2 rather than sec.0.2-rc2? Edit: Note that this is not in scope for this issue.

We filed #2995076: Let UpdateTestBase check for multiple status messages to split the test improvements out into their own issue.

I applied the latest patch from #2995076: Let UpdateTestBase check for multiple status messages in reverse on top of #90, deleted drupal.sec.1.2_secure.xml as it was identical to drupal.sec.1.2.xml, and updated documentation per #96.2

Had a hard time converting the new tests back to the old data format.

Retesting now that #2995076: Let UpdateTestBase check for multiple status messages is in.

Re-roll.

+++ b/core/modules/update/update.module
@@ -439,11 +421,13 @@ function update_get_available($refresh = FALSE) {
+  trigger_error('This internal function was a temporary fix and will be removed before 9.0.0. Use the \'Internal\' release type tag in update XML provided by drupal.org to determine if releases are insecure.', E_USER_DEPRECATED);

Oh, there's an error in the message here. It should be the "Insecure" release type rather than the "Internal" release type.

Also, the canonical name for d.o is "Drupal.org" (with a capital "D").

Addressed #105.

Okay, this looks good to me! (Assuming tests don't unexpectedly fail).

Thanks @tedbow and @samuel.mortenson for the perseverance on this issue! This is the one we definitely want to have in prior to 8.6.0 for extended minor security coverage to be viable.

+++ b/core/modules/update/update.module
@@ -439,11 +421,14 @@ function update_get_available($refresh = FALSE) {
+  trigger_error('_update_equivalent_security_releases() was a temporary fix and will be removed before 9.0.0. Use the \'Insecure\' release type tag in update XML provided by Drupal.org to determine if releases are insecure.', E_USER_DEPRECATED);

any reason not to use "" here and avoid \' ? bit neater

Yep, seems sensible.

Leaving at RTBC since this is a small change.

Note for other reviewers (I forgot to repeat it here after discussing it with @samuel.mortenson yesterday). It's a full trigger_error() because the release list will not be maintained going forward, so we want users to get the deprecation warning about it without it being suppressed. This is along the lines of: https://www.drupal.org/core/deprecation#how-unintended

Committed as adf3293 and pushed to 8.7.x.

Cherry picked as 49c3ed2 and pushed to 8.6.x.

Thanks everyone

Backported to 8.5.x also