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

RFC: Add Audit Policies Support #637

Closed
wants to merge 1 commit into from

Conversation

darcyclarke
Copy link
Contributor

@darcyclarke darcyclarke commented Sep 20, 2022

@darcyclarke darcyclarke changed the title RC: Add Aduit Policies Support RFC: Add Aduit Policies Support Sep 20, 2022
@darcyclarke darcyclarke changed the title RFC: Add Aduit Policies Support RFC: Add Audit Policies Support Sep 20, 2022
{
"name": "Vulnerable",
"type": "error",
"query": ":vulnerable"
Copy link
Contributor

Choose a reason for hiding this comment

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

the query specifies which packages are selected, but what defines how the results are presented?

Vulnerability output is perhaps a special case, but is the output just "error, nonzero packages showed up in the query!"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question - I believe the idea would be to use something like the above but I can put some potential examples of output in the RFC itself

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it won't be nearly as useful if all i can do is control the exit code with a query that produces nonzero results. I suspect people will want to provide their own package that can be a function that receives the query results as an argument, and can do whatever it likes, as long as it returns an exit code, or something.

@naugtur
Copy link

naugtur commented Sep 28, 2022

Hello!

Where does the JSON go? If it's in a policies file, it feels like I could use it for npm-audit-resolver to specify what to ignore too.
Can we consider an ignore condition here? Or setting the policy to error on all vulnerable and then specify exceptions with queries for each?

Do at me ;)

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Sep 28, 2022
@darcyclarke
Copy link
Contributor Author

darcyclarke commented Sep 28, 2022

@naugtur the original idea was to have these policies live in package.json & get picked up by npm audit (they would then also apply during install or update if --audit=true); I can/should expand the RFC to note this & other specifics left out today (including what inclusion/exclusion might look like - although I think it would be fairly straightforward ie.
DSS combinators ex. :vulnerable:not(.dev, ...), but maybe you're hoping for more flexibility in composition of which policies to run[?])

@ljharb
Copy link
Contributor

ljharb commented Sep 28, 2022

I think putting them in package.json, thereby inflating the size of the packument and published packages, is probably not the best idea - a separate JSON file seems ideal.

@naugtur
Copy link

naugtur commented Sep 28, 2022

Yeah, I remember we iterated through all the options of "where to put audit specific information of which we'll need plenty" in the audit-resolver RFC reaching the conclusion that there's no existing file that would accommodate if well.
I'm not sure if I'll make it to the meeting today at all, but happy to help out with this whenever I can.

Also, if these are running before a package is installed to help make a decision, I have a bunch of use-cases for them and there's very little requirements to put in this RFC that'd enable those use-cases. At least it seems so from my initial reading of the PR here.

@bnb
Copy link

bnb commented Feb 1, 2023

I think putting them in package.json, thereby inflating the size of the packument and published packages, is probably not the best idea - a separate JSON file seems ideal.

IMO adding more files is not good. We already have too many files, adding more will only decrease visibility / add the burden of additional knowledge.

@ljharb
Copy link
Contributor

ljharb commented Feb 1, 2023

@bnb "too many files" is a subjective opinion - separate files allow you to have granular CODEOWNERS, as well, beyond being data that doesn't belong in a published package. The burden exists regardless because you still have to google for how to configure the package.json field.

@bnb
Copy link

bnb commented Feb 1, 2023

noting a combination of suggestions from the office hours: if there were a top-level package.json property like npmConfig, having the ability to have audit policies and the ability to point to a file (or a module!) rather than having a pre-determined filename, both options could be supported. If policies as a devDep is an option, there's also so much flexibility for both large-scale open source projects and companies alike.

@ljharb
Copy link
Contributor

ljharb commented Feb 1, 2023

i'd be totally fine with a field that optionally points to a file so that the config is discoverable AND the contents don't have to live in package.json :-)

@darcyclarke darcyclarke closed this Dec 2, 2023
@rondales
Copy link

Rendered RFC

@rondales
Copy link

Rendered RFC

Copy link

@rondales rondales left a comment

Choose a reason for hiding this comment

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

accepted/0000-npm-audit-policies.md

Copy link

@rondales rondales left a comment

Choose a reason for hiding this comment

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

accepted/0000-npm-audit-policies.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda will be discussed at the Open RFC call
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants