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

Package Security Check #72

Open
mitchellsimoens opened this issue Nov 15, 2017 · 27 comments
Open

Package Security Check #72

mitchellsimoens opened this issue Nov 15, 2017 · 27 comments

Comments

@mitchellsimoens
Copy link

I think it'd be quite helpful to check the package and it's dependencies for security issues like nsp and display it in the package details page and may even be a great idea to put in the search result list.

@Haroenv
Copy link
Collaborator

Haroenv commented Nov 15, 2017

That's a great idea, it could be added as another source here, by batching the calls if possible, it would be similar to how the downloads are fetched. Do you want to work on this @mitchellsimoens?

@rdela
Copy link

rdela commented Nov 23, 2017

@mitchellsimoens @Haroenv I really like this idea.
Anything that helps people only install secure packages is a big win to me. Plus I need to get a feel for the code and what is possible with query rules anyway. Happy to give it a try if one of you willing to supervise me and review. What do I need to do to get set up with the right kind of token to test changes locally?

Maybe someone from @nodesecurity would be willing to let us know whether they are on board and possibly review final interface and/or implementation?

@MartinKolarik
Copy link
Collaborator

👍 we previously considered showing an "insecure" badge in the search results at https://www.jsdelivr.com and having this information available directly in the index would definitely make it easier.

@Haroenv
Copy link
Collaborator

Haroenv commented Nov 24, 2017

You can request the data from 200 packages at the same time, I have a small demo I can push where other people can help

@rdela
Copy link

rdela commented Nov 24, 2017

@Haroenv if you already started something I would love to help on that branch, maybe you can split up work and assign me little tasks I can attempt to address in single commits?

@Haroenv
Copy link
Collaborator

Haroenv commented Nov 25, 2017

https://github.com/algolia/npm-search/tree/feat/nsp, but it's pretty late here and there's some bug that needs fixing where the security key doesn't show up in the relevant packages

@rdela
Copy link

rdela commented Nov 26, 2017

Going to take a look at the bug and see if I can get the key to appear in the correct spot. Is there anything extra I need to know about first time dev setup not in readme?

@Haroenv
Copy link
Collaborator

Haroenv commented Nov 26, 2017

You need an Algolia app with api key and index. If you tell me which appid I can give you more records and operations so you don’t need to pay @rdela

@rdela
Copy link

rdela commented Nov 27, 2017

Nice. Signed in with GH earlier and went through onboarding, it was stellar and docs look like they are same caliber.

My first round of questions:

Ok to use this appid made during tour? If so, ok to post that here in issue or should I email/DM it to you somewhere?

Tried to create new app and got…

You cannot create new applications during the trial period

I guess the default appId (OFCNCOG2CU) listed in the readme is the Yarn version?

Looks like I can just clearIndex/deleteIndex before each test?

Do I need to worry about indexName or ok to leave off and
seq=0 apiKey=... yarn dev
will just create npm-search each time if it does not already exist?

Assuming readme Usage > Development production warning…

Be careful to develop on a different index than the production one when necessary.

will not apply because I will not have a “production” index yet?

@Haroenv
Copy link
Collaborator

Haroenv commented Nov 27, 2017

Nice. Signed in with GH earlier and went through onboarding, it was stellar and docs look like they are same caliber.

My first round of questions:

Ok to use this appid made during tour? If so, ok to post that here in issue or should I email/DM it to you somewhere?

Yes that should be good. The appId is not exactly secret so you can send it here if you need more usage

Tried to create new app and got…

You cannot create new applications during the trial period

oh yeah that's right, I think if you pick a (free) plan you can create new apps, just not in trial.

I guess the default appId (OFCNCOG2CU) listed in the readme is the Yarn version?

Yes, that's the appid used in production

Looks like I can just clearIndex/deleteIndex before each test?

yep, both are equivalent for this usage.

Do I need to worry about indexName or ok to leave off and
seq=0 apiKey=... yarn dev

you can put the index name in config.js (where it has a default of npm-search)

will just create npm-search each time if it does not already exist?

it will create or append to that index

Assuming readme Usage > Development production warning…

Be careful to develop on a different index than the production one when necessary.
will not apply because I will not have a “production” index yet?

This is for people at Algolia with access to the production index.

@rdela
Copy link

rdela commented Nov 27, 2017

The appId is not exactly secret so you can send it here if you need more usage

Cool, appid:
ERNLE6UZ7H

Trial seems pretty generous and I am only testing this for now. If you want to wait to uncap til I hit limits or trial expires in 13 days, fine by me, can mention you here then and include key again.

Any reason to get rid of getstarted_actors demo index before I start testing? Won’t bother testing npm-search index somehow will it?

Like the Actors example and way it demos searchable attributes and custom ranking formula, might refer to it later on, but could always create another account and leave in free plan for that.

So would you mind elaborating on:

security key doesn't show up in the relevant packages

…the one we map over in nsp.js? Where is it missing? In the index? I have a hunch about introduction of securityRecommendation in chore: spacing and less reduncancy, which does not seem to occur elsewhere?
Should that be plain recommendation?

Want to make sure I know how to tell if/when it is fixed before I dive in though.

@Haroenv
Copy link
Collaborator

Haroenv commented Nov 27, 2017

I think I did something wrong when mapping in the weekend, the data wasn't present in the index, I haven't taken a look at what the cause was yet. It'll be fixed if the records have a securityRecommendation or whatever it's renamed to in the index.

You can remove or keep the getting started index, it won't impact your testing here.

Let us know if you want extended records/operations anytime

@rdela
Copy link

rdela commented Nov 27, 2017

Great news all around, think I am headed in the right direction, will try to PR in next day or so, thanks Haroen!

@rdela
Copy link

rdela commented Dec 1, 2017

@Haroenv thanks for stopping by the Gatsby issue, this is still on my radar and I was not trying to bring this discussion over there, only to ask you about Gatsby plugin index design in progress in case you had ideas from all of your experience implementing and helping others implement search. I am not sure whether we will end up displaying security info in the initial Gatsby index yet even though I personally am heavily in favor.

Part of the reason I mentioned you was the idea to show some sort of indication that a package is not 1.0 yet is interesting, and may be worth me opening a separate issue on the Yarn site repo about. But we can circle back to it once this is merged.

Will let you know how I get on with the code tomorrow and over the weekend, have some time set aside for this so should be able to have something by Monday. Will you be the one implementing the Yarn interface? Is it worth trying to get it displaying in their templates in some form once I get data matched correctly or we would need to work that out later?

@Haroenv
Copy link
Collaborator

Haroenv commented Dec 1, 2017

I or someone else can do it on yarnpkg/website. It should be pretty straightforward to simply display it a little like the deprecated view and some link back to nsp @rdela

@rdela
Copy link

rdela commented Dec 1, 2017

Cool thanks @Haroenv will PR this before I mess with Yarn site

@rdela
Copy link

rdela commented Dec 4, 2017

@Haroenv looking now, thanks for 0a2e39e

@rdela
Copy link

rdela commented Dec 4, 2017

@Haroenv Ok so I am even with you now on my fork, first run fails with:
AlgoliaSearchError: Record at the position 73 objectID=101 is too big size=23909 bytes. Contact us if you need an extended quota

Is this a me still being in trial issue?

We are pulling securityRecommendation data though

@rdela
Copy link

rdela commented Dec 4, 2017

@rdela
Copy link

rdela commented Dec 4, 2017

@Haroenv added both of your emails I saw glancing through commit logs as collaborators on my trial app so you can see firsthand if you like

@Haroenv
Copy link
Collaborator

Haroenv commented Dec 5, 2017

@rdela, yes there will be some records that are too big for a regular index, you can filter those out. Is it the nsp data which is too big? Maybe we should try to find a more data conservative way of writing it 🤔

@jimaek
Copy link

jimaek commented May 26, 2019

Is this being worked on?

@Haroenv
Copy link
Collaborator

Haroenv commented May 27, 2019

Feel free to work on this @jimaek

@rdela
Copy link

rdela commented May 29, 2019

Yes, exactly what Haroen said, feel free to work on this. I have not touched it since comments above from Dec. 2017 but am willing to try to help anyone make progress if they want to attempt to bring it up to date. That said, my time is very limited currently so I may not be of much assistance.

The feat/nsp branch Haroen created is still available on my fork even though it has been deleted from this repo.

NSP/^Lift were swallowed by npm in April 2018. The nsp package was deprecated 7 May 2018 and the
API shut down 30 Sept 2018.

Although the use case is different since it wants a package.json, looks like there may be some prior art to pull ideas from in the npm-auditor package (Github) that uses npm-registry-fetch and npm-audit-report.

@jimaek
Copy link

jimaek commented May 29, 2019

Ok, we will check it out since we want to use this at jsDelivr

@bodinsamuel
Copy link
Contributor

@jimaek do you still need it?
I see that you have a widget in jsdelivr now, so probably not a strong push anymore.
Could be interesting to have it in the index directly though, if any api exists

@jimaek
Copy link

jimaek commented Jul 12, 2021

In our case Snyk sent a PR directly jsdelivr/www.jsdelivr.com#330
I am certain they would be glad for you to integrate their data into your index. So maybe contact Brian to make sure you don't overload their widget :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants