Skip to content

fix: add workflow permissions and npm provenance #16

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
evansantos merged 4 commits into from
Feb 19, 2026
Merged

fix: add workflow permissions and npm provenance #16

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be372f9
fix: add workflow permissions and npm provenance for publish
evansantos Feb 19, 2026
f7b6043
fix: make npm audit non-fatal in CI
evansantos Feb 19, 2026
316b686
fix: address GitSniff review warnings
evansantos Feb 19, 2026
7e42f54
fix: implement GitSniff suggestions properly
evansantos Feb 19, 2026
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
9 changes: 6 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ on:
pull_request:
branches: [ main, develop ]
Comment on lines 6 to 7
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

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

🟡 Warning

The current CI workflow for ci.yml is triggered only on pull_request events for main and develop branches. This means that direct pushes to these branches, or any other branch, will not trigger the CI workflow. This can lead to situations where code is pushed directly to the main or develop branch without being fully tested by the CI pipeline, potentially introducing regressions or untested code.

Fix: Add push events as a trigger for the CI workflow to ensure that all pushes to the specified branches (main, develop) also run the CI checks. This provides more comprehensive testing and prevents untested code from being merged or deployed. It also provides immediate feedback on the health of the branch after a direct push.

🤖 Prompt for AI Agents 
In .github/workflows/ci.yml around line 6:

Issue: The current CI workflow for `ci.yml` is triggered only on `pull_request` events for `main` and `develop` branches. This means that direct pushes to these branches, or any other branch, will not trigger the CI workflow. This can lead to situations where code is pushed directly to the main or develop branch without being fully tested by the CI pipeline, potentially introducing regressions or untested code.

Add `push` events as a trigger for the CI workflow to ensure that all pushes to the specified branches (main, develop) also run the CI checks. This provides more comprehensive testing and prevents untested code from being merged or deployed. It also provides immediate feedback on the health of the branch after a direct push.

Replace with:
on:
  push:
    branches: [ main, develop ]
  pull_request:
    branches: [ main, develop ]


Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

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

⚠️ Security

The ci.yml workflow specifies permissions for pull_request events, but not for push events. While contents: read is a sensible default, it's good practice to explicitly define permissions for all trigger events if they differ or if default permissions are insufficient for certain jobs. This ensures consistent and explicit permission management.

Fix: Add a permissions: block directly under the on:push: event or at the top-level on: block to cover all events where permissions might be required. In this case, contents: read is likely sufficient for the push event as well.

permissions:
contents: read

jobs:
test:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -42,7 +45,7 @@ jobs:
run: npm run build

- name: Check for vulnerabilities
run: npm audit --omit=dev
run: npm audit --omit=dev --audit-level=high || echo "::warning::npm audit found vulnerabilities (see above)"
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

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

🟡 Warning

The npm audit command is modified to issue a warning for all vulnerabilities that are not of 'high' severity. While this prevents CI from failing on lower severity issues, it might obscure visibility into 'moderate' and 'low' severity vulnerabilities, which could still pose risks or accumulate over time if not addressed. It's important to understand the project's policy on these.

Fix: Consider explicitly setting an --audit-level that aligns with the project's security policy. For example, --audit-level=moderate would warn on moderate and high issues, providing better visibility while still allowing builds to pass for low-severity findings. Alternatively, add a comment explaining the rationale for ignoring non-high vulnerabilities.

Suggested change
run: npm audit --omit=dev --audit-level=high || echo "::warning::npm audit found vulnerabilities (see above)"
run: npm audit --omit=dev --audit-level=moderate || echo "::warning::npm audit found vulnerabilities (see above)"

Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

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

⚠️ Security

The npm audit command is configured to emit a warning for any vulnerabilities found, but it doesn't fail the job directly. This can lead to vulnerabilities being overlooked, especially if the warning is missed in the logs. It's generally better to fail the build on high-severity vulnerabilities to enforce security.

Fix: Consider making the npm audit step fail the job if high-severity vulnerabilities are found. This ensures that security issues are addressed promptly. If a warning is acceptable for certain levels, configure the --audit-level accordingly.


- name: Upload coverage to Codecov (if exists)
uses: codecov/codecov-action@v3
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

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

ℹ️ Info

The condition if: matrix.node-version == '22.x' for uploading coverage to Codecov is now redundant. With the removal of the build matrix and a single Node.js version set in env.NODE_VERSION, this condition will always evaluate to true (or the equivalent of always running), and it is no longer necessary to explicitly check the Node.js version.

Fix: Remove the redundant if: condition from the Codecov action, as it no longer serves a purpose with the simplified Node.js versioning.

Suggested change
uses: codecov/codecov-action@v3
uses: codecov/codecov-action@v3

Expand All @@ -60,10 +63,10 @@ jobs:
steps:
- uses: actions/checkout@v4

- name: Use Node.js 20.x
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
with:
node-version: '20.x'
node-version: '22.x' # matches test job
Copy link
Copy Markdown

@gitsniff gitsniff bot Feb 19, 2026

Choose a reason for hiding this comment

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

ℹ️ Info

The comment # matches test job on line 69 is redundant given the explicit node-version: '22.x' directly above it. The purpose of this change is to ensure consistency, and the comment doesn't add significant value beyond what the code itself conveys.

Fix: Remove the redundant comment # matches test job as the intention is clear from the adjacent code, adhering to the principle of self-documenting code and minimizing comment cruft.

Suggested change
node-version: '22.x' # matches test job
node-version: '22.x'

cache: 'npm'

- name: Install dependencies
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/publish.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ jobs:
runs-on: ubuntu-latest
permissions:
contents: write
id-token: write

steps:
- uses: actions/checkout@v4
Expand All @@ -48,7 +49,7 @@ jobs:

- name: Publish to npm
if: ${{ github.event.inputs.dry_run != 'true' }}
run: npm publish --access public
run: npm publish --access public --provenance
env:
NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}

Expand Down