Problem/Motivation

To support the recent changes to the project application process, namely decoupling the ability to create full projects/releases from security coverage - we need to provide indicators on the update status page of which installed modules receive security coverage.

@hestenet, @drumm, and @mlhess hope to help drive this forward. @Dries has given his blessing to prioritize this patch.

We (the DA) want to get this committed as soon as possible, and so we're allocating our sprint time towards getting this ready, writing tests, and responding to any reviews.

Proposed resolution

The changes would need to:

  1. Indicate which modules have security coverage and which do not.
  2. Provide visual indicators of coverage status via the shield icon and the !-alert icon
  3. For modules that are explicitly unsupported for known security issues or other reasons, it should indicate that

In addition the changes could:

  • Provide an alert on each page for admins like the 'you are using a module with a security release' warning

Remaining tasks

Review

User interface changes

admin/reports/updates with added security information

Screenshot
Screenshot
The update status for available updates, including security updates and the row backgrounds, is left as-is for consistency. Security information is grouped with module support or update status.

Addition to status report

If everything is covered:
Screenshot

If something is not:
Screenshot

API changes

Theme additions in update module.

Data model changes

We have updated the update status xml to support this change: #2853696: Add security advisory coverage to update status XML and can make additional changes as needed.

Comments

hestenet created an issue. See original summary.

David_Rothstein’s picture

Issue tags: +needs backport to D7

For modules that do not have security coverage, highlight them in yellow.

I'm pretty sure yellow is already used for other things (for example, a module that has an update available, but the update isn't security-related). Also, even for a module that doesn't have security coverage, there should still be a way to indicate all the various states that are currently indicated via color: module is up-to-date, module is not up-to-date, module is not up-to-date and a security update is available, etc. So this needs some more thought.

Probably this should try to use something as similar as possible to whatever design comes out of #786702: Add information about releases covered by security advisories to project pages near download table (in order to promote consistency between what people see on drupal.org compared to what they see on their own sites)?

hestenet’s picture

Probably this should try to use something as similar as possible to whatever design comes out of #786702: Add information about releases covered by security advisories to project pages near download table (in order to promote consistency between what people see on drupal.org compared to what they see on their own sites)?

That's a good idea.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mforbes’s picture

With the new shield icon now deployed on production d.o, I naturally expected to find this thread on adding it to update.module's admin/reports/updates (to use D7 parlance, as I'm not a D8 user yet).

Having the shield on project pages informs about whether or not to install a project; having it at admin/reports/updates would inform about whether or not to keep a project installed. For someone looking to have only covered projects installed, indicators in both of these places would be more comprehensive than just the former alone.

cilefen’s picture

So that is a +1?

cilefen’s picture

I am asking around to find out if this isn't soft-postponed on a theoretical issue to update the XML.

drumm’s picture

Yes, the XML needs to have the data before core can show it.

I do recommend getting a head start on implementation here if possible.

The <release> elements will get a new child with a straightforward value. Maybe <security>covered</security>/<security>not covered</security>

mforbes’s picture

So that is a +1?

Yup.

<security>covered</security>

Something that simple surely makes sense, but is the word "covered" (or variations thereof) already being used in machine-readable places such as d.o's field names and views config? If not, meaning it's merely part of (translatable) strings like the shield's alt text and legend, then we might be wise to avoid it given this explanation of what the indicator really means. It's also worth considering whether or not this will be binary forever. In any case, it should be informed by any machine names that have already been put into use anywhere else.

cilefen’s picture

Status: Active » Needs work
FileSize
1.28 KB

This is as far as I got but I'm quitting for the day.

cilefen’s picture

Wherever we go with styling this, it is to be determined if we simply let the value pass through to the theme layer like this, or update the update status (even adding a new status constant) as it is passed around.

drumm’s picture

Actually, a bit of text for the message would be good to include in the XML. Links would be good to be able to use; the text should be otherwise well-sanitized so hacking Drupal.org isn't (more of) a risk to individual sites.

With the D6 end of life, we ran into limitations on what we could communicate via update status, which was not a fun to figure out. See #2687957: Improve Update Status messaging for End of Life in Drupal 7(and Drupal 8 and later).

miwayha’s picture

@drumm: Consider this status message or modules and themes not covered by the Security Team's disclosure policy:

Caution: Your site uses contributed modules and / or themes that are not subject to the Drupal Security Team's Responsible Disclosure policy. Production sites should exercise care in using such modules and / or themes. For more information see https://www.drupal.org/security-team.

mj-19’s picture

Issue tags: +MWDS2016

Building upon what @miwayha wrote:

Caution: Your site is at risk. Your site uses modules and / or themes that are not subject to the Drupal Security Team's Responsible Disclosure policy. Use of such modules and / or themes may expose your site to security vulnerabilities. For more information see https://www.drupal.org/security-team.

pwolanin’s picture

Starting on a test fixture change

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drumm’s picture

#2853696: Add security advisory coverage to update status XML added the data to updates.drupal.org. All XML generated after now will have the new elements. In addition to regenerating after commits on a dev release and making a new release, the whole thing is regenerated daily too. So everything should have the new elements tomorrow.

If the new element contains text, that’s the reason something isn’t covered and should show up in the UI. <security covered="1"/> means things are good. For example:

https://updates.drupal.org/release-history/bad_judgement/7.x
has <security>Project has not opted into security advisory coverage!</security> on every release.

https://updates.drupal.org/release-history/api/7.x
has <security>Beta releases are not covered by Drupal security advisories.</security> on beta releases.
and <security covered="1"/> on full releases.

This helps us with #2687957: Improve Update Status messaging for End of Life in Drupal 7(and Drupal 8 and later) by letting arbitrary plain text in:
https://updates.drupal.org/release-history/api/6.x
<security>Drupal 6 and earlier are not covered by Drupal security advisories!</security>

drumm’s picture

Status: Needs work » Needs review
FileSize
8.58 KB
86.12 KB

The attached patch builds on #15:

  • Updated test fixture XML to match what is now in production on updates.drupal.org
  • Flattened the shield even more to keep up with Drupal.org project pages.
  • Added reason for not being covered from update XML.

Screenshot

This only shows the information for versions you might want to update to. I think the goal, wearing my security team hat, is to have some indication on the projects you have installed. We could do any of:

  • Show the shield on covered projects you have installed.
  • Show the reason for not being covered on projects you have installed.
  • Put a warning or error at …/admin/reports/status when you have not covered projects installed.
hestenet’s picture

@mlhess, and @drumm, and myself had a call and reviewed this:

- We like the copy from comment #14 for the non-covered releases.
- We'd like to move the shield icon to the left side of the release information to make it more visible.
- We'd like to add the [covered shield] and the [!shield icon for non-covered releases] in the already installed modules list.
- We could use some design work in terms of how to layout these icons and messages in a sensible way - help welcome

drumm’s picture

Status: Needs review » Needs work
FileSize
8.65 KB

(Simple-reroll of #18 for array code style, and being able to apply to 8.4.x today.)

alexpott’s picture

@mlhess asked me to look at this from a framework manager perspective. There's not much architecture here - I like re-using the shield from d.o - that makes lots of sense. And it'll be good to contrast the security covered modules with those that are not. I definitely think it is worth adding a warning when using modules from d.o not covered by the security policy and #14 looks good in terms of copy. Obviously custom code needs to be excluded. It'd be great to add tests given the importance of putting the shileds on the right modules.

hestenet’s picture

One additional element that would be good here is to include the 'You have projects that are not subject to the drupal security advisory policy' message in same kind of warning message like the 'you have modules with security releases' message - that follows admins around the site.

It should be something that can be disabled in something like a settings.local perhaps though.

mforbes’s picture

+1 to #22 given the overall trend that admins should be made aware of a lack of coverage to the same extent that they are made aware of available security updates.

I'm on the fence about that option to suppress the messages though. I suppose that option would suppress both the existing message (updates available) and the new message (not covered). I feel this could lead to an unscrupulous developer, who is at the tail end of a fixed-price contract and had one of these messages just pop up before delivery with no maintenance contract, enabling this option before the client could notice and ask what gives. Then there's an insecure site in the wild, maybe the client has some internal turnover and there's a new custodian who is then also oblivious to this option instead of figuring out that they should get maintenance, etc. etc.

Anyone using such a thing non-maliciously could just as well ignore the messages, instead.

David_Rothstein’s picture

I would suggest that a message which appears on all admin pages should be discussed in a separate issue. (That's a pretty big user interface change compared to what was originally proposed here - which is limited to the Update Status report only.)

drumm’s picture

Status: Needs work » Needs review
FileSize
9.08 KB
242.87 KB
85.41 KB

This patch:

  • Adds the warning message if you are running uncovered modules from Drupal.org. That won’t show for custom modules not available from Drupal.org.
  • Adds help text explaining the security shield.
  • Moves the shield, or explanation for why something is not covered, from suggested upgrades to what you are currently running. I think we we showed it in both places the page could get quite busy.

Screenshot
Screenshot

To do:
Tests
Design - I grouped security coverage with up to date status
Copyediting - is everything clearly worded and warnings of appropriate severity?
When uncovered projects are running, add the warning to admin/reports/status

Status: Needs review » Needs work

The last submitted patch, 25: 2766491.diff, failed testing.

drumm’s picture

Status: Needs work » Needs review
FileSize
21.36 KB
18.63 KB
11.48 KB

This patch should fix the notices in the last test run and adds either of the following messages to …/admin/reports/status

Screenshot

Screenshot

Personally, I’m not sure we want to put a message on every page. This is important for admins to know, but I’d say it is an order of magnitude less important than running known insecure code, which is the only case we message on every page for admins, that I know of. Conceptually, it is about the same risk, but having an SA with an upgrade to do is a lot more actionable than a project with real potential to have public reports.

hestenet’s picture

Issue summary: View changes
Issue tags: +Needs backport to 8.3.x

Status: Needs review » Needs work

The last submitted patch, 27: 2766491.patch, failed testing.

drumm’s picture

Status: Needs work » Needs review
FileSize
11.56 KB

Fixing notices only. Not all the test fixtures have the new XML elements, it is good to test for that case.

Status: Needs review » Needs work

The last submitted patch, 30: 2766491.patch, failed testing.

hestenet’s picture

Copy suggestions:

Re: #25

The warning message copy at the top seems good to me.

I think the language next to each module/version is concise and informative.

  • Moves the shield, or explanation for why something is not covered, from suggested upgrades to what you are currently running. I think we we showed it in both places the page could get quite busy.

This is a bit tricky, if I'm looking at a module I already have installed, which is one which does *not* have security advisory coverage on the particular release I'm using - I would find it very reassuring to know whether or not the release being recommend to me does. (As in #18). I don't necessarily need that signal for the 'latest version' and the 'development version' just the 'recommended release' - I think.

Re: #27

I think we might want to adjust the language slightly:

Drupal.org Security Advisory coverage All modules and themes from Drupal.org that are currently installed receive coverage. Learn more about security advisory coverage.

I think the 'Not all modules....' message can probably stay as it is.

drumm’s picture

FileSize
12.83 KB

I shortened up the last suggestion:

Currently installed modules and themes from Drupal.org receive coverage.

This patch also starts adding testing.

drumm’s picture

Status: Needs work » Needs review

The last submitted patch, 10: update_status_should-2766491-10.patch, failed testing.

The last submitted patch, 11: update_status_should-2766491-11.patch, failed testing.

The last submitted patch, 15: 2766491-security-coverage-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2766491.patch, failed testing.

drumm’s picture

Status: Needs work » Needs review
FileSize
17 KB

I found the bit of changed markup causing the failed test, and added a test for the shield appearing when a project is covered.

Status: Needs review » Needs work

The last submitted patch, 39: 2766491.patch, failed testing.

drumm’s picture

Status: Needs work » Needs review
FileSize
17 KB

Fixing my failing test.

Status: Needs review » Needs work

The last submitted patch, 41: 2766491.patch, failed testing.

hestenet’s picture

Adding tags just to suggest that a designer's eye would be welcome. I think it looks pretty okay - but I'm sure it could be improved.

drumm’s picture

I got the tag added in ccc_update_test.1_0.xml in the wrong place in the last patch.

xjm’s picture

I don't think "Needs design review" is actually a thing that anyone looks at, but I think what we actually care about is the usability impact of the design, so tagging for those topic maintainers.

It would be best to put current screenshots in the summary, but #25 and #27 are probably accurate.

xjm’s picture

FWIW, to me, the black shield icon looks like a bad thing rather than a good one, like it might be a different kind of bad warning like the yellow triangle. But I am not a usability professional.

drumm’s picture

Issue summary: View changes

It would be best to put current screenshots in the summary, but #25 and #27 are probably accurate.

Done, with the issue summary template. And yes, those are current screenshots.

drumm’s picture

FWIW, to me, the black shield icon looks like a bad thing rather than a good one, like it might be a different kind of bad warning like the yellow triangle. But I am not a usability professional.

I’m happy to change it as needed. I also think some brief explanation to go with the added help text would be good.

On Drupal.org, we use CSS like filter: opacity(0.5) drop-shadow(0 0 0 #fbf03b) brightness(2); to change the color of the icon to match the background. I don’t see filter: used outside of 3rd-party things in core; I’m not sure if it is an appropriate level of cleverness for core.

ifrik’s picture

FileSize
2.51 KB
2.21 KB

About #46

FWIW, to me, the black shield icon looks like a bad thing rather than a good one, like it might be a different kind of bad warning like the yellow triangle. But I am not a usability professional.

On drupal.org, this shield is grey rather than black when it's part of an explanation. So maybe just changing it to the same grey, would make it look less like a "bad thing" and consistent with d.o.