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: npm audit licenses #182

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

RFC: npm audit licenses #182

wants to merge 4 commits into from

Conversation

bnb
Copy link

@bnb bnb commented Jul 17, 2020

What / Why

It would be ideal to provide license tooling within the CLI itself rather than leaving it to ecosystem OSS tooling or enterprise-grade registry tools/services to implement it.

Edit: Worth noting that much of the discussion until 5/5/2021 is outdated and reflects an older version of this proposal.

@bnb bnb changed the title feat: add npm license RFC npm audit licenses Jul 17, 2020
@bnb bnb changed the title npm audit licenses RFC: \npm audit licenses Jul 17, 2020
@bnb bnb changed the title RFC: \npm audit licenses RFC: npm audit licenses Jul 17, 2020
@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Jul 17, 2020
Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

What about individual package overrides?

while licensee supports using community overrides (which are great), and it'd be important to allow their use here, there'll always be packages that don't have valid licenses, or that are dev-only tools, not distributed, for which the license doesn't matter. How can these be targeted?

Separately, licenses often only need to be enforced on distributed code - ie, runtime dependencies. is there a way to filter those in the config?

@bnb
Copy link
Author

bnb commented Jul 18, 2020

@ljharb I was intentionally vague in values. Once you allow MIT and a few others, you've allowed a majority of the ecosystem. For a few that have incorrect or missing licenses, though, you'd likely either want to add them to the ignore list (under license if they're acceptable but invalid SPDX IDs, or under module if they're entirely missing their license).

Does that answer your question, or are you asking about the potential of using installable license lists?

I would not be opposed and did consider this but didn't want to go overboard with the initial proposal. I would personally love to see installable license lists, since this would enable something like npm i @mcdonalds/license-list as a way to provide large teams a single source of truth to maintain and use. What this API would look like, I am unsure since it shifts the license list out from package.json/audit.json.

@ljharb
Copy link
Contributor

ljharb commented Jul 18, 2020

ah, thanks, i didn't read carefully enough. so "module" is for allowed modules - are there disallowed modules?

@bnb
Copy link
Author

bnb commented Jul 21, 2020

@ljharb this is the structure:

"allow": ["MIT", "ISC"], // licenses that are allowed
"block": ["Unlicense", "WTFPL"], // these are not allowed
"ignore": { // these will all be ignored
  "license": ["BIGCOMPANYINTERNALMODULE"], // when npm encounters a module with this license it will be skipped
  "module": ["@bigco/bigmoneyfrontend"] // ignore this module whenever it's encountered no matter what its license is
}

There is currently not a way to allow a module explicitly through "allow". What is the use case that isn't already covered?

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2020

I think that "module" needs to support an optional version specifier range - for example, maybe v1 doesn't have a specified license (but is MIT) but v2 is MIT and v3 is GPL, you wouldn't want all versions to be allowed, since then v3 would be allowed, but you also wouldn't want to error on v1?

@bnb
Copy link
Author

bnb commented Jul 22, 2020

@ljharb would this work for you?

"allow": ["MIT", "ISC"], // licenses that are allowed
"block": ["Unlicense", "WTFPL"], // these are not allowed
"ignore": { // these will all be ignored
  "license": ["BIGCOMPANYINTERNALMODULE"], // when npm encounters a module with this license it will be skipped
  "module": {
    "@bigco/bigmoneyfrontend": "<semver-expression>"
  } // ignore this module whenever it's encountered no matter what its license is
}

Default could be * and allow you to get more granular from there.

Alternatively (I think this would probably be a bad idea but feel I should propose it):

"allow": ["MIT", "ISC"], // licenses that are allowed
"block": ["Unlicense", "WTFPL"], // these are not allowed
"ignore": { // these will all be ignored
  "license": ["BIGCOMPANYINTERNALMODULE"], // when npm encounters a module with this license it will be skipped
  "module": [ "@bigco/bigmoneyfrontend@semver-expression" ]// ignore this module whenever it's encountered no matter what its license is
}

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2020

To be clear, what I think is ideal is the config format that licensee already has - is there a reason we can't adopt that, since npm already uses licensee?

@bnb
Copy link
Author

bnb commented Jul 22, 2020

If folks would like me to rewrite this with the licensee format, that's fine. I would prefer to get more input that it's the direction folks would want to go before I do that work 👍🏻

@wesleytodd
Copy link

"@bigco/bigmoneyfrontend": ""

Would be anything parsable by npm-package-arg? Same argument which @isaacs made for the expectation of supporting git repos and file system paths for the "parent" feature, right?


Is there anything we can take from the audit resolve spec which can be applied here? As I mentioned on the call, it seems like a "snooze for 1 month" style is really what is effective to reduce noise. If you add to "ignore" it will rely on you remembering to check again once the update from the transitive 3 levels deep gets propagated up to you, this will lead to stale info here I think.

@bnb
Copy link
Author

bnb commented Jul 29, 2020

@isaacs would genuinely appreciate your feedback on this in text, if you have any, before I start editing so I can make sure I don't miss anything - I know you had some thoughts on the RFCs call but my brain did not adequately capture them in a way that I can turn them into a checklist.

And no rush, just wanted to make sure I get the request in and share the reason I've not continued to update.

@ruyadorno
Copy link
Contributor

Action item from the OpenRFC Call:

  • Explore ways we could potentially use licensee

@bnb
Copy link
Author

bnb commented Aug 24, 2020

@ruyadorno it would be nice to hear expanded thoughts on that. Do the project's maintainers want to use the JSON format of licensee? Literally just pull in Licensee as a dep and provide a command as an alias for it? Something else?

@ruyadorno
Copy link
Contributor

I'm really sorry about the vague comment @bnb but it was very intentional 😬 I was very stressed out running the call and I honestly don't remember exactly what were the points, just commented here for the sake of not losing track of it (also remembered you mentioning you'd like textual comments for actionable things)

@ruyadorno ruyadorno removed the Agenda will be discussed at the Open RFC call label Aug 26, 2020
@bnb
Copy link
Author

bnb commented Mar 17, 2021

Any ideas on how we could progress this? Is just adding a dep on licensee a thing we could do? Or read that format?

Happy to work on this, I'm just not sure what next steps requests are from the npm team.

@darcyclarke darcyclarke added the Agenda will be discussed at the Open RFC call label Mar 17, 2021
@darcyclarke
Copy link
Contributor

Adding agenda label just so we make sure to give feedback or next steps by next week

@bnb
Copy link
Author

bnb commented Apr 14, 2021

next step discussed in today's meeting:

  • figure out the specific DX / command strucutre.

It was suggested that an independent command or npm ls output would make sense.

Addressing those two, respectively:

  • my guess would be that specifically for controlling incoming dependencies, this specific feature set under npm audit does make sense since it is technically management / control of your dependencies. It would also be something you'd want to have run automatically when you're installing and periodically.
  • while I'm not opposed to npm ls output of licenses, I think this proposal is distinct from npm ls since it provides actions / filtering that would apply to npm install and related commands.

@bnb
Copy link
Author

bnb commented May 5, 2021

I've updated the RFC to reflect using licensee while retaining the originally proposed DX.

@bnb
Copy link
Author

bnb commented May 5, 2021

Updated further with the npm audit --fix and related changes that were discussed in today's RFC meeting.

@bnb
Copy link
Author

bnb commented May 5, 2021

remaining unresolved question:

  • package.json/audit.json/both

are we okay with introducing an audit.json configuration file? are we okay shoving this configuration into package.json? are we okay with doing both and having one take priority?

@bnb
Copy link
Author

bnb commented May 5, 2021

worth noting, I've also opened an issue that looks like it'll result in a PR from me this week to licensee to replace whitelist with allowlist, with +1's to that path from the current maintainers: jslicense/licensee.js#72

@ljharb
Copy link
Contributor

ljharb commented May 5, 2021

I'd prefer having "only not package.json" - it avoids inflating the size of the packument/published package, and it avoids adding more stuff into package.json.

However, some folks might prefer avoiding "another config file", so I'm not sure what would be best.

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Aug 25, 2021
@settings settings bot removed the Proposal label Sep 21, 2021
@bnb
Copy link
Author

bnb commented Mar 30, 2022

depending on how #564 goes, it might be worth simply waiting for that to be implemented and the subsequent command mapping feature, so as not to waste time on implementing "normally" and then migrating.

@ljharb
Copy link
Contributor

ljharb commented Mar 30, 2022

Given that licensee already exists, I’m not sure what the value is of waiting?

@bnb
Copy link
Author

bnb commented Mar 30, 2022

Given that licensee already exists, I’m not sure what the value is of waiting?

I explored doing it with licensee in a pairing session with @izs and the conclusion we came to was that doing so was massively limiting, given that it would preclude a lot of the benefits you get from doing such a thing with Arborist, a number of which are baseline expectations folks would have of an npm CLI command.

@ljharb
Copy link
Contributor

ljharb commented Mar 31, 2022

Ah, that's new information, thanks.

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.

5 participants