fix(attest-npmjs-com): compute integrity from actual tarball bytes#78
fix(attest-npmjs-com): compute integrity from actual tarball bytes#78
Conversation
The old code re-packed every input tarball with `npm pack --json`, which produces different bytes than pnpm-packed tarballs (different gzip params, tar metadata). This caused a checksum mismatch when publishing pnpm-packed packages to npm with the attestation. Now the action reads the tarball as-is and computes sha512 integrity directly with openssl, matching what npm registry will compute on the same bytes. Also adds a `package-manager` input (npm/pnpm) for directory packing and a `tarball-path` output so callers can publish the exact attested tarball. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a checksum mismatch issue when publishing pnpm-packed packages to npm with attestations. Previously, the action re-packed every tarball using npm pack, which produced different bytes than pnpm-packed tarballs due to different gzip parameters and tar metadata. The fix reads tarballs as-is and computes SHA512 integrity directly from the actual bytes.
Changes:
- Compute integrity hash directly from tarball bytes using OpenSSL instead of re-packing with npm
- Add
package-managerinput to support both npm and pnpm for directory packing - Add
tarball-pathoutput to ensure callers publish the exact attested tarball - Enhanced documentation with workflow examples and important notes about tarball identity
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| actions/attest-for-npmsjs-com/action.yml | Core logic changes: removed npm re-packing, added direct tarball reading with tar/openssl, added package-manager validation, improved path resolution, and added tarball-path output |
| actions/attest-for-npmsjs-com/README.md | Documentation updates: added workflow examples, explained tarball identity importance, documented new inputs/outputs, and added "How It Works" section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "Filename: $PACKAGE_FILENAME" | ||
| echo "Integrity: $PACKAGE_INTEGRITY" | ||
|
|
||
| echo "TARBALL_PATH=$(realpath "$PACKAGE_TARBALL")" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Calling realpath on $PACKAGE_TARBALL when it may already be an absolute path (from lines 53, 56) is redundant but harmless. However, in the third case (line 62), $PACKAGE_TARBALL is constructed using ls -1t which returns an absolute path only if $SUBJECT_ABS is absolute. Since $SUBJECT_ABS is already absolute from line 48, this works, but it would be clearer to ensure $PACKAGE_TARBALL is always absolute in all three branches or document this assumption.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif [ -d "$SUBJECT_ABS" ] && [ -f "$SUBJECT_ABS/package.json" ]; then | ||
| echo "Found package.json in $SUBJECT_ABS, packing with $PACK_MANAGER..." | ||
| if [ "$PACK_MANAGER" = "pnpm" ]; then | ||
| (cd "$SUBJECT_ABS" && "$PACK_MANAGER" pack --pack-destination .) |
There was a problem hiding this comment.
Using ls with glob patterns in command substitution can fail when there are spaces in filenames or when the glob doesn't match any files. The current code handles the no-match case with the subsequent check, but filenames with spaces could still cause issues. Consider using a more robust approach like find "$SUBJECT_ABS" -maxdepth 1 -name "*.tgz" -type f -print0 | sort -z | head -z -n 1 | tr -d '\0' for better reliability.
| if [ "$PACK_MANAGER" = "pnpm" ]; then | ||
| (cd "$SUBJECT_ABS" && "$PACK_MANAGER" pack --pack-destination .) | ||
| else | ||
| (cd "$SUBJECT_ABS" && "$PACK_MANAGER" pack) | ||
| fi |
There was a problem hiding this comment.
The --pack-destination flag is not supported in npm versions before npm v7. Since the code already changes to the target directory with cd "$SUBJECT_ABS", consider either: 1) removing --pack-destination . (as packing in the current directory is the default), or 2) adding version-specific logic to only use this flag when needed. This ensures compatibility with older npm versions that may still be in use.
| if [ "$PACK_MANAGER" = "pnpm" ]; then | |
| (cd "$SUBJECT_ABS" && "$PACK_MANAGER" pack --pack-destination .) | |
| else | |
| (cd "$SUBJECT_ABS" && "$PACK_MANAGER" pack) | |
| fi | |
| (cd "$SUBJECT_ABS" && "$PACK_MANAGER" pack) |
- Use delimiter-based pattern (heredoc with uuidgen) for GITHUB_OUTPUT writes to prevent injection via newlines or special characters - Remove unnecessary npm/pnpm branching for pack command since cd already sets the working directory Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…and pnpm 9.6.0 Tested all 8 combinations of npm/pnpm pack and publish against JFrog, including end-to-end column for npmjs.com republish via npm publish. Added warning about .tgz files inside tarballs triggering wrapper extraction in api-oss-software-supply-chain. Co-authored-by: Cursor <cursoragent@cursor.com>
- Updated the description for `subject-path` and `package-manager` inputs to clarify behavior when both .tgz files and package.json are present. - Improved error handling for non-existent subject paths and added warnings for multiple .tgz files found. - Refactored tarball selection logic to prioritize existing .tgz files over packing from package.json. - Enhanced README documentation to reflect these changes and provide clearer usage instructions. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,67 +1,214 @@ | |||
| name: "[Ledger Security] Attest a npm package for npmjs.com distribution" | |||
| description: "" | |||
| description: "Attest a npm package for npmjs.com distribution. Supports pre-packed tarballs or directories with package.json." | |||
There was a problem hiding this comment.
Grammar nit: use “Attest an npm package …” instead of “Attest a npm package …”.
|
|
||
| <!-- action-docs-description source="action.yml" --> | ||
|
|
||
| Attest a npm package for npmjs.com distribution. Supports pre-packed tarballs or directories with package.json. |
There was a problem hiding this comment.
Grammar nit: use “Attest an npm package …” instead of “Attest a npm package …”.
| Attest a npm package for npmjs.com distribution. Supports pre-packed tarballs or directories with package.json. | |
| Attest an npm package for npmjs.com distribution. Supports pre-packed tarballs or directories with package.json. |
| EOF_MARKER=$(uuidgen || true) | ||
| if [ -z "$EOF_MARKER" ]; then | ||
| echo "::error::Failed to generate a safe delimiter for GITHUB_OUTPUT." | ||
| exit 1 | ||
| fi | ||
|
|
||
| TARBALL_ABS=$(realpath "$PACKAGE_TARBALL") |
There was a problem hiding this comment.
uuidgen is used to generate the delimiter for multi-line GITHUB_OUTPUT, but it isn’t guaranteed to exist on all runners. Consider using a portable fallback when uuidgen is missing (e.g., derive a delimiter from $RANDOM/timestamp with a collision check) so the action doesn’t fail before producing outputs.
| EOF_MARKER=$(uuidgen || true) | |
| if [ -z "$EOF_MARKER" ]; then | |
| echo "::error::Failed to generate a safe delimiter for GITHUB_OUTPUT." | |
| exit 1 | |
| fi | |
| TARBALL_ABS=$(realpath "$PACKAGE_TARBALL") | |
| TARBALL_ABS=$(realpath "$PACKAGE_TARBALL") | |
| # Generate a safe, unique delimiter for multi-line GITHUB_OUTPUT values. | |
| if command -v uuidgen >/dev/null 2>&1; then | |
| EOF_MARKER="$(uuidgen)" | |
| else | |
| EOF_MARKER="EOF_$(date +%s)_$$_${RANDOM}" | |
| fi | |
| OUTPUT_CONTENT="$TARBALL_ABS | |
| $PACKAGE_FILENAME | |
| $PACKAGE_NAME | |
| $PACKAGE_VERSION | |
| $PACKAGE_INTEGRITY" | |
| # Ensure the delimiter does not appear in the output content. | |
| ATTEMPTS=0 | |
| while printf '%s\n' "$OUTPUT_CONTENT" | grep -q "$EOF_MARKER"; do | |
| ATTEMPTS=$((ATTEMPTS + 1)) | |
| if [ "$ATTEMPTS" -ge 5 ]; then | |
| echo "::error::Failed to generate a safe delimiter for GITHUB_OUTPUT after multiple attempts." | |
| exit 1 | |
| fi | |
| EOF_MARKER="${EOF_MARKER}_${RANDOM}" | |
| done | |
| if [ -z "$EOF_MARKER" ]; then | |
| echo "::error::Failed to generate a safe delimiter for GITHUB_OUTPUT." | |
| exit 1 | |
| fi |
| github_token: | ||
| description: 'GitHub Token (to be able to upload the attestation to the GitHub Attestation API).' | ||
| default: ${{ github.token }} | ||
| required: true |
There was a problem hiding this comment.
github_token is declared as an input but isn’t used anywhere in the composite action. This makes the documented input misleading and prevents callers from providing a PAT when GITHUB_TOKEN permissions are restricted. Either wire it into actions/github-script via with: github-token: ${{ inputs.github_token }} (and/or export GITHUB_TOKEN accordingly) or remove the input/documentation.
| ```yaml | ||
| permissions: | ||
| id-token: write | ||
| ``` |
There was a problem hiding this comment.
The README’s required permissions list only id-token: write, but this action uploads attestations via the GitHub Attestations API (and typically requires attestations: write). Please update the permissions snippet and the workflow examples to include attestations: write so users don’t hit auth failures.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| EOF_MARKER=$(uuidgen || true) | ||
| if [ -z "$EOF_MARKER" ]; then | ||
| echo "::error::Failed to generate a safe delimiter for GITHUB_OUTPUT." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
uuidgen is required to create the GITHUB_OUTPUT delimiter, but uuidgen isn’t guaranteed to exist on all GitHub-hosted runners/shell environments. Consider using a more portable delimiter generator (e.g., openssl rand -hex, python -c 'import uuid', or date +%s%N with PID) or detect missing uuidgen and provide an actionable error message about required tooling/runner OS.
| @@ -1,67 +1,214 @@ | |||
| name: "[Ledger Security] Attest a npm package for npmjs.com distribution" | |||
| description: "" | |||
| description: "Attest a npm package for npmjs.com distribution. Supports pre-packed tarballs or directories with package.json." | |||
There was a problem hiding this comment.
The action description uses “Attest a npm package”; grammatically this should be “Attest an npm package”. This text is surfaced in the GitHub Actions marketplace and README generation, so it’s worth correcting.
The old code re-packed every input tarball with
npm pack --json, which produces different bytes than pnpm-packed tarballs (different gzip params, tar metadata). This caused a checksum mismatch when publishing pnpm-packed packages to npm with the attestation.Now the action reads the tarball as-is and computes sha512 integrity directly with openssl, matching what npm registry will compute on the same bytes. Also adds a
package-managerinput (npm/pnpm) for directory packing and atarball-pathoutput so callers can publish the exact attested tarball.