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

Obvious Fix: Added "files" property to only include index.js / index.d.js / properties.json in npm package #83

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MattSidor
Copy link

@MattSidor MattSidor commented Nov 10, 2021

My project uses a CI/CD security scanning tool for our node apps. This tool flagged the encryption keys in the test/ folder of this library and would not allow us to publish the app. Our workaround was to rm -rf node_modules/njwt/test after npm install as part of our build step in the pipeline.

The security scanner is naive to the context of the encryption keys in test/ and cannot see that those files won't actually be used by the apps that import this library.

However, since the test/ files are not necessary to be included for consumers of this library, I believe the best solution is to only declare the files that are necessary. npm allows us to do this via the files property of package.json: https://docs.npmjs.com/cli/v6/configuring-npm/package-json#files

This PR updates the files property of package.json to only include index.js, index.d.ts, and properties.json.

These other files from the library will always be included as part of the npm package, regardless of settings:

  • package.json
  • README.md
  • CHANGELOG.md
  • LICENSE

Copy link

@zzacal zzacal left a comment

Choose a reason for hiding this comment

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

Great work. No reason for test keys to be published.

@oleksandrpravosudko-okta
Copy link
Collaborator

@MattSidor can you also include properties.json into list?

@MattSidor
Copy link
Author

@oleksandrpravosudko-okta Yes! I've updated my PR. Thanks for catching that.

@MattSidor MattSidor changed the title Obvious Fix: Added "files" property to only include index.js / index.d.js in npm package Obvious Fix: Added "files" property to only include index.js / index.d.js / properties.json in npm package Dec 3, 2021
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

4 participants