Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Draft] Implement Security Feature #910

Closed
wants to merge 2 commits into from
Closed

[Draft] Implement Security Feature #910

wants to merge 2 commits into from

Conversation

jayfk
Copy link
Contributor

@jayfk jayfk commented Jan 9, 2016

This is a first draft about how a package security feature might look like.

The basic idea is that package maintainers can mark a release as insecure and provide a URL to a CVE or a blog post describing the vulnerability as a first step to implement something like the suggestion in #798

Styles

I've borrowed a lot of the excellent styles that are already in the codebase and added a few new style classes.

If you take a look at the attached screenshots, you'll see that I've added a big red banner on top and a smaller label on the Version History & Changelog tab.

screen shot 2016-01-06 at 10 13 31

screen shot 2016-01-06 at 10 13 16

Workflow

A maintainer that wants to mark a release is insecure would go to a new page where all the packages he or she maintains are listed, select one or multiple releases and provide a URL for further explanation.

screen shot 2016-01-09 at 18 39 07

Implementation

This draft adds two new fields to the packaging.Release model:

  • insecure Boolean
  • insecure_url Text

And a new view to accounts.views that I've called mark_insecure. In its current form, the view does nothing other than passing a dict containing two example projects to the template.

Is this something you would consider to add to the new codebase? I'm open to all feedback for this draft and I'm willing to go down a completely different route if this doesn't fit.

If this is something you'd consider, I'd be happy to continue to work on this to fill the missing parts.

@dstufft
Copy link
Member

dstufft commented Jan 11, 2016

I've not looked at the code itself at all, but the feature itself seems sound to me. I wonder if it makes sense to make it more generic so that it could cover the use case of #345 as well (or maybe not, since it's possible to have an insecure version that hasn't been deprecated or had support dropped).

All design is gated on @nlhkabu so she'd have to speak to any particular design impact and how it should be displayed.

@miohtama
Copy link
Contributor

Instead of insecure boolean I suggest marked_insecure_at datetime, as sometimes you want to track these changes on timeline for audit purposes.

@@ -80,7 +80,16 @@ <h3 class="title"><a href="{{ request.route_path('packaging.project', name=relea
<p class="help-link"><a href="#" {{ l20n("howToInstall") }}>How do I install this?</a></p>
</div>
</section>

{% if release.insecure %}
<section class="horizontal-section -bad -medium">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we try a 'thin' style here instead? It looks a little fat to me - I think the red is distinct enough without taking up the extra screen real estate.

@nlhkabu
Copy link
Contributor

nlhkabu commented Jan 13, 2016

Thanks @jayfk. Both the design and front end code look great - the only thing I'd like to see is the banner size reduced (as per my above comment).

<div class="site-container">
<p {{ l20n("versionInsecure") }}>
This version has been marked insecure by the package maintainers. You can read more
about the vulnerabilites <a href="{{ release.insecure_url }}">here</a>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicks:

  1. "vulnerabilites" ~> "vulnerabilities"
  2. It's not great to have "here" as link text, but since it's an arbitrary link it's hard to have something more descriptive that's always appropriate. Perhaps make the sentence to Read more about the vulnerabilities, and make the whole sentence a link?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On number 2:
You could either do what @alexwlchan suggests, or simply make the whole sentence a link:
<a href="{{ release.insecure_url }}">You can read more about the vulnerabilities here</a>

@jayfk
Copy link
Contributor Author

jayfk commented Jan 31, 2016

Thanks @jayfk. Both the design and front end code look great - the only thing I'd like to see is the banner size reduced (as per my above comment).

Yes! I've played around with it a little (even had the whole banner in red for a while), but I had always the impression that it was a bit too much.

I've not looked at the code itself at all, but the feature itself seems sound to me. I wonder if it makes sense to make it more generic so that it could cover the use case of #345 as well (or maybe not, since it's possible to have an insecure version that hasn't been deprecated or had support dropped).

Maybe we could switch the fields to something like:

  • deprecated_at: datetime (what @miohtama suggested so that we can keep track of it)
  • deprecated_reason: ["insecure", "not maintained", None]

This way a package that is insecure would be a special case of a deprecated package and we have one interface to manage it all. What do you think?

@jayfk
Copy link
Contributor Author

jayfk commented Sep 26, 2016

Implementation in #1462.

@jayfk jayfk closed this Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants