Release: npm publish should not fail when registry propagation-check times out - #273
google-labs-jules[bot] wants to merge 1 commit into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: fortemate/dicechess-engine/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
| echo "warning: could not read the published integrity for $PACKAGE_SPEC from $REGISTRY_URL after $attempt attempts" >&2 | ||
| if [[ -n "${GITHUB_ACTIONS:-}" ]]; then | ||
| echo "::warning::could not read the published integrity for $PACKAGE_SPEC from $REGISTRY_URL after $attempt attempts" | ||
| fi | ||
| sed 's/^/ /' "$VIEW_ERROR" >&2 | ||
| return 1 | ||
| return 0 |
There was a problem hiding this comment.
When --expected-integrity is supplied, verify_integrity only returns success after comparing a registry-provided dist.integrity value with the expected SHA-512 digest. However, after all npm view ... dist.integrity attempts fail or return no digest, the final branch emits a warning and returns 0 instead of failing. Because callers invoke this function before declaring an existing package verified, after a successful publish, and on the concurrent-publication path, CI can treat a package as integrity-verified without obtaining any registry digest. The release bundle publisher always supplies --expected-integrity from its manifest, and the workflows use it for both mirror verification and canonical npm publication, so registry metadata unavailability can bypass the intended digest-verification gate.
Steps to Reproduce
- Invoke
.mise/lib/npm-publish-if-missing.shwith a valid package source, a registry URL, and--expected-integrity sha512-.... - Configure
npm(or the registry path it contacts) so everynpm view <package>@<version> dist.integrityrequest fails or returns an empty value; setNPM_VERIFY_MAX_ATTEMPTS=1andNPM_VERIFY_SLEEP_SECONDS=0to make the test immediate. - Observe that
verify_integrityprints the warning at.mise/lib/npm-publish-if-missing.sh:105-109and returns success at line 110 without comparing a digest. - Observe that the caller proceeds as successful through the existing-package, successful-publish, or concurrent-publication paths.
set -e
TMP=$(mktemp -d)
trap 'rm -rf "$TMP"' EXIT
mkdir -p "$TMP/package" "$TMP/bin"
printf '{"name":"@example/pkg","version":"1.0.0"}\n' > "$TMP/package/package.json"
cat > "$TMP/bin/npm" <<'EOF'
#!/usr/bin/env bash
if [[ " $* " == *" dist.integrity "* ]]; then
echo 'registry metadata unavailable' >&2
exit 1
fi
if [[ " $* " == *" version "* ]]; then
printf '1.0.0\n'
exit 0
fi
exit 0
EOF
chmod +x "$TMP/bin/npm"
PATH="$TMP/bin:$PATH" NPM_VERIFY_MAX_ATTEMPTS=1 NPM_VERIFY_SLEEP_SECONDS=0 \
bash ./dicechess-engine/.mise/lib/npm-publish-if-missing.sh \
"$TMP/package" https://registry.example \
--expected-integrity sha512-YWJj
# The command exits 0 and prints the timeout warning despite never receiving a digest.Fix with AI
A security vulnerability was found by Hacktron.
File: .mise/lib/npm-publish-if-missing.sh
Lines: 105-110
Severity: medium
Vulnerability: Integrity verification fails open when registry metadata cannot be read
Description:
When `--expected-integrity` is supplied, `verify_integrity` only returns success after comparing a registry-provided `dist.integrity` value with the expected SHA-512 digest. However, after all `npm view ... dist.integrity` attempts fail or return no digest, the final branch emits a warning and returns `0` instead of failing. Because callers invoke this function before declaring an existing package verified, after a successful publish, and on the concurrent-publication path, CI can treat a package as integrity-verified without obtaining any registry digest. The release bundle publisher always supplies `--expected-integrity` from its manifest, and the workflows use it for both mirror verification and canonical npm publication, so registry metadata unavailability can bypass the intended digest-verification gate.
Proof of Concept:
**Steps to Reproduce**
1. Invoke `.mise/lib/npm-publish-if-missing.sh` with a valid package source, a registry URL, and `--expected-integrity sha512-...`.
2. Configure `npm` (or the registry path it contacts) so every `npm view <package>@<version> dist.integrity` request fails or returns an empty value; set `NPM_VERIFY_MAX_ATTEMPTS=1` and `NPM_VERIFY_SLEEP_SECONDS=0` to make the test immediate.
3. Observe that `verify_integrity` prints the warning at `.mise/lib/npm-publish-if-missing.sh:105-109` and returns success at line 110 without comparing a digest.
4. Observe that the caller proceeds as successful through the existing-package, successful-publish, or concurrent-publication paths.
```bash
set -e
TMP=$(mktemp -d)
trap 'rm -rf "$TMP"' EXIT
mkdir -p "$TMP/package" "$TMP/bin"
printf '{"name":"@example/pkg","version":"1.0.0"}\n' > "$TMP/package/package.json"
cat > "$TMP/bin/npm" <<'EOF'
#!/usr/bin/env bash
if [[ " $* " == *" dist.integrity "* ]]; then
echo 'registry metadata unavailable' >&2
exit 1
fi
if [[ " $* " == *" version "* ]]; then
printf '1.0.0\n'
exit 0
fi
exit 0
EOF
chmod +x "$TMP/bin/npm"
PATH="$TMP/bin:$PATH" NPM_VERIFY_MAX_ATTEMPTS=1 NPM_VERIFY_SLEEP_SECONDS=0 \
bash ./dicechess-engine/.mise/lib/npm-publish-if-missing.sh \
"$TMP/package" https://registry.example \
--expected-integrity sha512-YWJj
# The command exits 0 and prints the timeout warning despite never receiving a digest.
```
Affected Code:
if [[ $attempt -ge $max_attempts ]]; then
echo "warning: could not read the published integrity for $PACKAGE_SPEC from $REGISTRY_URL after $attempt attempts" >&2
if [[ -n "${GITHUB_ACTIONS:-}" ]]; then
echo "::warning::could not read the published integrity for $PACKAGE_SPEC from $REGISTRY_URL after $attempt attempts"
fi
sed 's/^/ /' "$VIEW_ERROR" >&2
return 0
fi
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), !accepted_risk <reason>, or !fixed (resolved). Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
|
1 participant
Redesigned
verify_integrityin.mise/lib/npm-publish-if-missing.shso that registry propagation timeouts emit a warning (and GitHub Actions::warning::annotation) instead of causing release failure, ensuring remaining packages and downstream release steps complete. Updated.mise/lib/test-npm-release-handoff.shto verify propagation timeout behavior. (Ref #132, #266)Fixes #272
PR created automatically by Jules for task 876042609893764583 started by @rabestro