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

feat: adding security badge #330

Merged
merged 4 commits into from
Jun 2, 2020
Merged

Conversation

bmvermeer
Copy link
Contributor

#143
Creating a badge based on a call to a reactive endpoint to feed the number of vulns a package has and link to a page that has more information on it

@jimaek jimaek temporarily deployed to jsdelivr-com-master-q2rtvyjjbv May 27, 2020 14:55 Inactive
- don't load vulnerability info in homepage search results
- only load for npm packages
- minor style and naming changes
@jimaek jimaek temporarily deployed to jsdelivr-com-master-q2rtvyjjbv May 27, 2020 15:23 Inactive
@jimaek jimaek temporarily deployed to jsdelivr-com-master-q2rtvyjjbv May 27, 2020 15:24 Inactive
@MartinKolarik
Copy link
Member

MartinKolarik commented May 27, 2020

Hi @bmvermeer, thanks for the PR. I made a few changes:

  • don't load this directly in search results, as it results in too many requests and backend errors,
  • minor style changes.

A few more issues that I found and that should be addressed:

@bmvermeer
Copy link
Contributor Author

@MartinKolarik I can make a change to the backend that prevents this too many requests error.

@MartinKolarik
Copy link
Member

MartinKolarik commented May 27, 2020

Even then I'm not sure we want to fire 10 (currently 20 because of preflight) requests on every keystroke. Maybe if there was at least 1 second debounce and the endpoint also used non-zero max-age for client-side caching.

@MartinKolarik
Copy link
Member

MartinKolarik commented May 27, 2020

non-zero max-age for client-side caching

  • that might be a good idea, regardless, at least for 1 hour

@bmvermeer
Copy link
Contributor Author

@MartinKolarik I will take a look tomorrow on the server-side to check the grunt page and the max-age thing.
At this time I cannot change the token to a query param, unfortunately. If that changes I will let you know.

@MartinKolarik
Copy link
Member

At this time I cannot change the token to a query param, unfortunately. If that changes I will let you know.

Ok, for now, it doesn't make that big of a difference, but if we were to include it on the homepage, it would need to be changed. 10 extra requests are too many.

@bmvermeer
Copy link
Contributor Author

@MartinKolarik I deployed a new backend that fixed the mismatch you pointed out with grunt.

@jimaek jimaek temporarily deployed to jsdelivr-com-master-q2rtvyjjbv May 28, 2020 14:16 Inactive
@MartinKolarik
Copy link
Member

@bmvermeer can we also do something about the max-age thing?

@MartinKolarik MartinKolarik merged commit ff7a252 into jsdelivr:master Jun 2, 2020
@MartinKolarik
Copy link
Member

@bmvermeer
Copy link
Contributor Author

Great!, I see that in the current version the badge will not be shown if the package has 0 vulns.
IMO it would be good to show explicitly that a package has no known vulns.
WDYT?

@MartinKolarik
Copy link
Member

We discussed with @jimaek and agreed to show a badge with a checkmark in that case, check now.

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.

3 participants