Skip to content

[Developer:Documentation] Update README.md for Github releasing #671

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 9, 2024
Merged

[Developer:Documentation] Update README.md for Github releasing #671

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f1a342
Update README.md
micpap25 Jul 1, 2024
cf6de70
Update README.md
micpap25 Jul 2, 2024
424e0d5
Merge branch 'main' into micpap25-patch-1
micpap25 Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

Question: Does this mean that pdf-annotate won't be published anymore on NPM? If yes, where can I read more about this decision?! 👀

```

## Example
Expand Down Expand Up @@ -73,3 +73,15 @@ $ npm test
# lint the code
$ npm run lint
```

## 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
```
Copy link
Member

Choose a reason for hiding this comment

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

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:

  1. Confirm that within the test workflow that the files generated by npm run build match what's in a given commit
  2. Have a GH action that runs on any push to main by a non-bot user that then does npm run build and 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


Loading