Skip to content

chore: enforce supply chain age gating#16

Merged
justin-layerv merged 4 commits intomainfrom
chore/enforce-supply-chain-age-gating
Apr 11, 2026
Merged

chore: enforce supply chain age gating#16
justin-layerv merged 4 commits intomainfrom
chore/enforce-supply-chain-age-gating

Conversation

@Kevin-layerV
Copy link
Copy Markdown
Contributor

Summary

  • Adds .npmrc with min-release-age=7 to reject npm packages published less than 7 days ago
  • Enables ignore-scripts=true to block arbitrary install scripts from packages

Why

Newly published or hijacked packages are typically detected and removed within days. A 7-day quarantine ensures compromised packages never enter our dependency tree.

Override for critical updates

npm install <pkg>@<version> --min-release-age=0

Test plan

  • npm config get min-release-age --location=project returns 7
  • npm install still resolves existing dependencies from lock file
  • npm install --min-release-age=0 <pkg> bypasses the restriction

Supply chain hardening — reject npm packages published less than
7 days ago and disable install scripts by default. Override for
emergency updates: npm install <pkg> --min-release-age=0

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 1, 2026

PR Review: chore: enforce supply chain age gating

Good supply-chain hardening initiative. A couple of items to consider before merging:

1. ignore-scripts=true suppresses prepublishOnly during publish

package.json defines "prepublishOnly": "npm run build" as a safety net to ensure dist/ is fresh before publishing. With ignore-scripts=true, this lifecycle hook is silently skipped during npm publish.

In CI this is fine — the release workflow explicitly runs npm run build before npm publish (line 53-54 of release-please.yml). However, if anyone ever publishes locally, prepublishOnly won't fire, potentially shipping stale or missing artifacts.

Worth documenting this trade-off in the PR description or as a comment in .npmrc so future maintainers understand why the safety net is disabled.

2. min-release-age requires npm ≥ 11 (Node ≥ 22)

This config option was introduced in npm 11. The project's engines field allows Node ≥ 18, and contributors on Node 18 or 20 (which ship npm 10 / npm 10) will see min-release-age silently ignored — no error, no protection.

CI uses Node 22 so it's enforced there, but local development won't be gated on older Node versions. Consider either:

  • Bumping engines.node to >=22 if the team is ready, or
  • Adding a note that this protection is CI-enforced only, or
  • Adding an .nvmrc with 22 to nudge contributors toward the right version.

3. Minor: add a comment to .npmrc

Since .npmrc config is easy to overlook or accidentally remove, a brief comment explaining the rationale helps future maintainers:

# Supply-chain hardening: reject packages published <7 days ago (npm 11+)
min-release-age=7
# Block arbitrary install/lifecycle scripts from dependencies
ignore-scripts=true

Overall

The change is small, well-scoped, and the security rationale is sound. The two items above are non-blocking but worth addressing. The PR description is clear and includes a useful override escape hatch.

Verdict: Approve with minor suggestions.

CR feedback: document that min-release-age requires npm 11+ (CI-enforced
only on older Node), and that ignore-scripts suppresses prepublishOnly
during local publish (CI runs build explicitly).

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 10, 2026

PR Review: chore: enforce supply chain age gating

Overall Assessment

This is a well-scoped, high-value security hardening change. The .npmrc adds two meaningful defenses against supply-chain attacks, and the second commit does a good job documenting the trade-offs flagged in the earlier review. A few observations:

Positives

  • min-release-age=7 is a strong defense against typosquatting and package hijacking — most malicious publishes are detected and yanked within days.
  • ignore-scripts=true eliminates an entire class of install-time attacks. Good default.
  • Comments are thorough — the Node 22/npm 11 requirement, the prepublishOnly suppression, and the CI-only enforcement caveat are all clearly documented.
  • CI is already aligned — both ci.yml and release-please.yml use Node 22 and run npm run build explicitly before publish, so neither setting causes breakage there.
  • Lock file compatibilitynpm ci installs from the lock file, so min-release-age doesn't interfere with existing installs; it only gates adding or updating dependencies.

Minor Considerations (non-blocking)

  1. Future native dependencies: ignore-scripts=true suppresses postinstall hooks from dependencies. The current dependency set (eslint, prettier, typescript, vitest) is pure JS and unaffected. However, if a future dependency requires a postinstall step (e.g., a native module using node-gyp), the install will silently succeed but the package won't work. This is fine to accept as a trade-off — just worth knowing. The comment in .npmrc could optionally mention this: "if a dep needs postinstall, override per-package with --ignore-scripts=false."

  2. .nvmrc consideration: Since min-release-age silently no-ops on npm <11, contributors on Node 18/20 get no protection locally. CI catches it, but an .nvmrc with 22 would nudge local dev toward the enforced version. Low priority since CI is the real gate.

Security

No concerns — this strictly tightens the security posture with no new attack surface.

Verdict

Approve. Clean, well-documented, and the CI workflows already handle the ignore-scripts implications correctly. The non-blocking items above are suggestions for future resilience, not merge blockers.

Both reviews flagged that `min-release-age=7` silently no-ops on npm <11
(Node <22), so contributors on older Node versions get no local
enforcement. CI already runs Node 22 and is the real gate, but an
`.nvmrc` nudges local dev toward the enforced version.

Also extends the `ignore-scripts` comment in `.npmrc` with the
per-package override escape hatch (`npm install <pkg>
--ignore-scripts=false`), documenting how to handle future
dependencies that legitimately need a postinstall hook.

Deferred from the review: bumping `engines.node` from `>=18` to
`>=22`. That is a user-facing breaking change for a published SDK
(consumers would see EBADENGINE warnings / hard errors with
engine-strict) and belongs in its own PR with explicit intent, not
buried in a supply-chain hardening change.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@justin-layerv
Copy link
Copy Markdown
Contributor

Review feedback addressed — commit 3b8cdf2

Both reviews' remaining non-blocking items are in.

Fixed

  1. Added .nvmrc pinning Node 22. Both reviews flagged that min-release-age=7 silently no-ops on npm <11 (Node <22), so contributors on older versions get no local protection. CI already runs Node 22 and is the real gate — .nvmrc just nudges local dev to match without any compatibility breakage for anyone.

  2. Extended the ignore-scripts comment in .npmrc to document the per-package override escape hatch:

    # If a dependency ever legitimately requires a postinstall hook (e.g. a
    # native module using node-gyp), re-enable per-package with
    # `npm install <pkg> --ignore-scripts=false`.
    

    Directly addresses reviewer 2's "Future native dependencies" nit so a future contributor hitting a native-module install doesn't have to go hunt the npm docs.

Deferred

  • Bumping engines.node from >=18 to >=22 (reviewer 1's first alternative). This is a user-facing breaking change for the published @layerv/qurl SDK — consumers installing on Node 18/20 would see EBADENGINE warnings (or hard errors with engine-strict). Making downstream consumers upgrade Node is a product decision that deserves its own PR with explicit intent and a major-version bump, not bundled into a supply-chain hardening change. The .nvmrc covers the contributor-facing concern without touching the consumer contract.

Already addressed in earlier commits

  • prepublishOnly suppression trade-off — documented in .npmrc comments from commit fcc4061.
  • CI-only enforcement caveat — same commit documents that local dev on older Node is not gated.
  • Rationale comment in .npmrc — same commit added the full comment block.

All earlier suggestions are now either landed or explicitly deferred with reasoning.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 11, 2026

PR Review — chore: enforce supply chain age gating

Overall Assessment

Clean, well-motivated PR. The two .npmrc hardening settings and the .nvmrc pin are sensible supply-chain defenses with minimal risk. The comments in .npmrc are thorough and cover the key edge cases. Approve with a few minor observations below.


What looks good

  • min-release-age=7 — Solid defense against typosquatting and account-takeover attacks on npm. The 7-day quarantine gives the ecosystem time to flag compromised packages before they enter your tree.
  • ignore-scripts=true — Blocks the most common vector for malicious packages (postinstall scripts). Good default for a pure-TypeScript SDK with no native dependencies.
  • Excellent inline comments — The .npmrc file explains why each setting exists, documents the prepublishOnly interaction, and provides the per-package override escape hatch. This is the kind of documentation that saves future contributors from accidentally reverting the setting.
  • CI compatibility is solid — Both ci.yml and release-please.yml explicitly run npm run build before publish/test, so the suppressed prepublishOnly lifecycle hook is not a problem. Verified this directly.
  • .nvmrc aligns with CI — Both workflows already pin node-version: "22", so the .nvmrc brings local dev into alignment.

Minor observations (non-blocking)

  1. Local npm publish will skip prepublishOnly
    The comment in .npmrc (line 7–8) correctly notes this, and CI handles it. Just worth calling out: if anyone ever does a manual npm publish locally, the build step won't run. The existing workflow (merge to main → Release Please → CI publish) makes this a non-issue in practice, but a quick note in CONTRIBUTING.md or the README about "always publish via CI" could prevent a future footgun.

  2. min-release-age is silently ignored on npm <11 (Node <22)
    The comment acknowledges this too. Since engines.node is >=18, contributors on Node 18/20 won't get the age-gating protection locally. CI enforces it, which is the important boundary. If you want belt-and-suspenders, you could tighten engines.node to >=22 — but that would be a separate decision with broader implications for the SDK's compatibility surface, and the current approach (enforce in CI, best-effort locally) is reasonable.

  3. No changes to package-lock.json
    Since ignore-scripts=true changes how npm ci/npm install behaves, it's worth confirming that a fresh npm ci still completes cleanly with this .npmrc in place. The test plan covers this (npm install still resolves from lock file), just flagging it for the reviewer running the checks.


Security

No concerns. This PR strictly improves the supply-chain security posture. The two settings complement each other well: min-release-age gates on package freshness, ignore-scripts gates on execution. Both are appropriate defaults for a library with only dev dependencies and no native modules.

Tests / CI

No code changes, so no new tests needed. The test plan in the PR body is appropriate — the three manual verification steps cover the key behaviors.


Verdict: Looks good to merge. 👍

🤖 Generated with Claude Code

The .npmrc sets ignore-scripts=true as a supply-chain defense, which
also suppresses prepublishOnly during `npm publish`. CI explicitly runs
`npm run build` before publishing so the release workflow is safe, but
a local publish would ship whatever is in dist/ without re-running the
build.

Called out in the Releases section of CONTRIBUTING.md so contributors
don't trip over this footgun, and pointed at `npm run build` as the
alternative for local artifact inspection.

Addresses the reviewer's footgun note on #16. Verified `npm ci` still
completes cleanly with the .npmrc in place.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@justin-layerv
Copy link
Copy Markdown
Contributor

Additional round addressed — commit 5b0dd00

New review came back clean-approve with three minor non-blocking observations. Picked up the two actionable ones.

Fixed

  1. Added a "Always publish via CI" note to CONTRIBUTING.md. The reviewer flagged that the ignore-scripts=true / prepublishOnly suppression could be a footgun if anyone ever runs npm publish locally. The .npmrc comment already covers it, but CONTRIBUTING.md is a more visible home. The new section:

    • Explains why a local publish would ship stale dist/
    • Points at CI (.github/workflows/release-please.yml) as the safe path
    • Suggests npm run build directly for local artifact inspection
  2. Verified npm ci completes cleanly with the .npmrc. Ran rm -rf node_modules && npm ci && npm run build && npm test on this branch:

    • npm ci — ✅ 439 packages installed (from the existing lock file, so no min-release-age gating)
    • npm run build — ✅ clean
    • npm test — ✅ 46 tests passing

    Confirms reviewer observation chore(main): release qurl 0.2.0 #3 (test plan item "npm install still resolves existing dependencies from lock file").

Deferred (explicit, reviewer agreed)

  • Bumping engines.node from >=18 to >=22 — reviewer's own words: "the current approach (enforce in CI, best-effort locally) is reasonable". Same rationale as before: this is a user-facing breaking change for the published SDK, not an internal concern. The .nvmrc covers contributor UX without touching the consumer contract. Belongs in its own major-version PR.

All three items from the latest review are now either landed or explicitly deferred with reviewer-agreed reasoning.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 11, 2026

PR Review: chore: enforce supply chain age gating

Summary

This PR adds two npm supply-chain hardening settings via .npmrc, pins Node 22 in .nvmrc, and documents the trade-offs in CONTRIBUTING.md. Four commits, three new/modified files, zero code changes.


Code Quality & Best Practices

Strengths:

  • Excellent inline documentation. The .npmrc comments explain not just what each setting does, but why it's there, what breaks without it, and how to override it. This is exactly the right level of documentation for security-sensitive config that a future contributor might be tempted to remove.
  • Clean commit history. Each commit is atomic and well-scoped: the initial hardening, then docs, then .nvmrc, then CONTRIBUTING.md. The commit messages follow the repo's conventional-commit format and include rationale in the body.
  • Good separation of concerns. The deliberate deferral of bumping engines.node to >=22 is the right call — that's a consumer-facing breaking change that doesn't belong in a supply-chain hardening PR.

One nit:

  • The .nvmrc contains just 22 (a major version). This is fine for nvm, but worth noting that some Node version managers (fnm, volta) may interpret this differently. Since the team is already on nvm (implied by the file choice), this is a non-issue in practice.

Potential Bugs or Issues

  • ignore-scripts=true applies globally, including to npm publish. The prepublishOnly script in package.json (npm run build) will not fire on local publish. This is thoroughly documented in both .npmrc and CONTRIBUTING.md, and CI's release-please.yml runs npm run build explicitly at line 35 (test job) and line 53 (publish job) before npm publish. No bug here — just confirming the safety net is in place.

  • min-release-age is a no-op on npm <11. Documented in .npmrc line 2-3. CI enforces it (Node 22 = npm 11+). Local dev on Node 18/20 silently skips the check. The .nvmrc mitigates this by nudging contributors to Node 22. Acceptable trade-off.

  • No risk to existing npm ci installs. min-release-age only gates new package resolution, not installs from an existing lock file. npm ci (which CI uses) reads from package-lock.json directly, so this setting won't cause spurious failures on existing dependencies.


Performance Considerations

No performance impact. These are install-time config settings that don't affect runtime behavior or build times.


Security

This PR strictly improves the security posture:

  1. min-release-age=7 — Defends against typosquatting, account takeover, and malicious package publishes. Most compromised packages are detected and removed within days; a 7-day quarantine keeps them out of the dependency tree.
  2. ignore-scripts=true — Eliminates the most common attack vector for malicious npm packages (postinstall scripts executing arbitrary code). The current dependency tree is pure TypeScript/JS tooling with no native modules, so nothing breaks.

The override escape hatches are documented for both settings (--min-release-age=0 and --ignore-scripts=false), which is important for operational flexibility without weakening the defaults.

No new attack surface introduced.


Test Coverage

No code changes, so no new tests are needed. The test plan in the PR description covers the three key verification points:

  1. npm config get min-release-age --location=project returns 7
  2. npm install still resolves from lock file
  3. Per-package override works

The contributor confirmed all checks pass: npm ci + npm run build + npm test (46 tests passing).


Final Notes

This PR has been through multiple review rounds and all feedback has been addressed or explicitly deferred with sound reasoning. The changes are minimal, well-documented, and the CI workflows are already aligned.

Verdict: Approve. Ready to merge.


🤖 Generated with Claude Code

@justin-layerv
Copy link
Copy Markdown
Contributor

Review acknowledged — no changes needed

Read the latest review carefully. It's a clean Approve. Ready to merge. verdict with zero actionable items — every section is either (a) confirming existing choices are correct or (b) positive observations. The one explicit "nit" is flagged as "a non-issue in practice" by the reviewer themselves.

Verified the one nit

The .nvmrc contains just 22 (a major version). This is fine for nvm, but worth noting that some Node version managers (fnm, volta) may interpret this differently.

Cross-checked:

  • .nvmrc content: 22
  • .github/workflows/ci.yml: node-version: "22"
  • .github/workflows/release-please.yml: node-version: "22" (both instances)

All three are intentionally aligned at the major-version level. Pinning further (e.g., 22.11.0) would create drift with CI's unpinned "22" and produce noise when the GitHub Actions runner image updates its Node 22 patch version. Keeping the major-version pin is the right call.

On version manager compatibility: nvm, fnm, volta, and asdf-nodejs all handle a plain major version in .nvmrc the same way (select latest installed 22.x). This is well-documented in each tool's docs and widely used across the JS ecosystem. The reviewer's "may interpret this differently" was speculative, and as they noted, it's "a non-issue in practice".

Verified across the rest of the review

  • ignore-scripts=true / prepublishOnly trade-off — reviewer: "No bug here — just confirming the safety net is in place." CI runs npm run build at release-please.yml:35 and :53 before publish; CONTRIBUTING.md warns against local publish.
  • min-release-age no-op on npm <11 — reviewer: "Acceptable trade-off." .nvmrc pins Node 22 (npm 11+).
  • No risk to existing npm ci installs — reviewer: confirmed. Verified in the previous round.
  • Security"strictly improves the security posture".
  • Performance"No performance impact."
  • Test coverage"No code changes, so no new tests are needed."

Nothing to change. Ready to merge whenever you pull the trigger.

@justin-layerv justin-layerv merged commit 6f23382 into main Apr 11, 2026
3 checks passed
@justin-layerv justin-layerv deleted the chore/enforce-supply-chain-age-gating branch April 11, 2026 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants