[Developer:Documentation] Update README.md for Github releasing #671
Conversation
README.md
Outdated
|
|
||
| ## Building | ||
| Commit your built files when contributing. | ||
| If on Windows, change the `build` script in `package.json` to `webpack && SET MINIFY=1&webpack`. | ||
| ```bash | ||
| # switch to node v14 or earlier | ||
| $ nvm use 14 | ||
|
|
||
| # build the dist files | ||
| $ npm run build | ||
| ``` |
There was a problem hiding this comment.
Given that the dist/ files are minified, relying purely on PR authors for updating the dist files themselves opens the door to potential exploits that are hard to detect. The attack would be that a malicious actor writes some fix for a bug and also their attack, creates the dist/ files, removes their attack, and then creates the PR. The PR reviewer will (understandably) not look over the built minified file as it's basically impossible to, and instead evaluate just the src/ change, and then go, yup, looks good, PR gets merged, and then if that build gets released (likely as there's not that many releases), the attack then gets the main Submitty repo.
My suggestion would be that you use GH actions to either:
- Confirm that within the
testworkflow that the files generated bynpm run buildmatch what's in a given commit - Have a GH action that runs on any push to
mainby a non-bot user that then doesnpm run buildand that then commits the built files (if any changes).
My personal preference would be (2) as it lowers the barrier to making PRs as it's one less thing to worry about within a PR, and is also not having the dist files show up in the files changed pane always, and that generating the build is a perfect thing for automation. GH actions are also already setup such that if you have on: push, if you push a branch using GITHUB_TOKEN, it would not trigger a subsequent workflow run, so it's pretty straight forward to implement.
There was a problem hiding this comment.
Thank you for the advice! See Submitty/Submitty#10667. We very infrequently touch this repository but we'll keep all this in mind in case a contributor rolls around.
| @@ -19,7 +19,7 @@ To report issues for pdf-annotate.js, please file them under the [Submitty/Submi | |||
| ## Installation | |||
|
|
|||
| ```bash | |||
| npm install @submitty/pdf-annotate.js | |||
| npm install Submitty/pdf-annotate.js | |||
There was a problem hiding this comment.
Question: Does this mean that pdf-annotate won't be published anymore on NPM? If yes, where can I read more about this decision?! 👀
No description provided.