Repository navigation
docs: advertise supply-chain hardening in READMEs and correct SECURITY.md - #407
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
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 |
Preview DeploymentPreview URL: https://ef1f7ef9.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| full commit SHA, not a movable tag. | ||
| - **Socket.dev** — the Socket GitHub App reviews every pull request for | ||
| malware, install scripts, obfuscated code, and privilege escalation in | ||
| dependency changes, and posts two required checks. Its policy is managed in |
There was a problem hiding this comment.
Socket checks are not "required" checks — 🟡 Minor · Correctness
What: This new bullet says Socket "posts two required checks", but the Socket checks are not part of main's branch protection. The live required_status_checks on main are exactly four — Build and Test, React 18 compatibility, React 19 compatibility, Dependency Review — none of them Socket.
Why it matters: SECURITY.md is the canonical, precise statement of the gate. Calling the Socket checks "required" claims a merge gate that does not exist — exactly the "advertising a gate that isn't configured" failure that #405 was filed to stop. #405 even enumerates the two Socket Security checks under "Checks deliberately NOT required". Socket posts the checks and surfaces alerts on every PR, but it does not block merge.
Evidence — live branch protection on main
"required_status_checks": {
"contexts": ["Build and Test", "React 18 compatibility",
"React 19 compatibility", "Dependency Review"]
}No Socket context present. rulesets is [].
Fix:
| dependency changes, and posts two required checks. Its policy is managed in | |
| dependency changes, and posts two status checks on every PR. Its policy is managed in |
There was a problem hiding this comment.
Deep review — 1 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Correctness | Socket checks described as "required" — they are not in main's required_status_checks; only the four CI checks are |
SECURITY.md:49 |
| 2 | 🔵 Advisory | Correctness | SECURITY.md states .github/workflows/scorecard.yml scores the repo weekly, and 4 READMEs add Scorecard badges — but that file is absent from main and PR #406 (issue #404) is still open. Copy/badges are false until #406 merges. Author documented "merge #406 first." |
SECURITY.md:55, README badges |
| 3 | 🔵 Advisory | Correctness | "an approving review" is required at merge — I confirmed required_status_checks (4 CI contexts) but could not read required_pull_request_reviews (token 403 on the protection endpoint) and #405 is still open. Owner should confirm a 1-approval rule is actually configured. |
SECURITY.md:62, README.md:198 |
Overall: This is a well-researched, accurate docs PR — I chased each supply-chain claim to its source and the great majority hold up (signed provenance on all three packages incl. bestax-migrate provenance:true, OIDC/no-NPM_TOKEN, SHA-pinned actions, install-scripts-blocked, 3-day cooldown with the prettier exclusion, frozen-lockfile + pnpm audit --audit-level=high gate, CodeQL default-setup with no codeql.yml, .github/** write-lock for AI agents, required-signatures + no-force-push + no-deletion, and the four required CI checks all verified live). The riskiest part is timing and precision around gates that aren't fully in place yet: the "required" Socket wording overstates the merge gate (blocking), and the Scorecard workflow the doc names doesn't exist on main yet. The human should fix the one word on line 49 and honor the stated merge-after-#406 ordering.
Residual risk: the failure class here is documenting a security gate that isn't actually enforced (the exact concern of #405):
- Required-checks overstatement — confirmed once: Socket's two checks are posted but not in branch protection (finding #1). The four CI checks are genuinely required (verified against the live protection payload), so the green-CI claim is sound.
- Scorecard forward-reference — confirmed:
scorecard.ymlis absent frommainand #406/#404 are open; mitigated only by the PR body's explicit "merge #406 first" note, which a human merge must honor (finding #2). - Approving-review requirement — unverifiable from here (protection endpoint 403'd); refuted only partially since the sibling
required_status_checkshalf of #405 is demonstrably applied. Owner-confirmable in seconds (finding #3).
🏄 Solid, honest write-up, dude — the security story mostly checks out clean against the live config. Just one gnarly little word ("required" on the Socket line) paddling out ahead of the actual gate, and a couple badges waiting on the #406 set to roll in. Fix the word, mind the merge order, and this one's good to ride.
Socket posts two checks on every PR, but they are not in main's required_status_checks — only Build and Test, the React 18/19 matrix, and Dependency Review are. Caught by deep review on #407.
|
Good catch on #1 — that one was a real error. All three addressed. Finding 1 (🟡 Minor, "required" overstates the gate) — fixed in c9fb21cCorrect and mine. Socket posts two checks on every PR, but they are not in Finding 2 (🔵 Advisory, Scorecard forward-reference) — acknowledged, merge order standsAccurate. Finding 3 (🔵 Advisory, unverified approval rule) — resolved: it is configuredThe 403 was a token-scope limit on the review's side, not an absent rule. Live payload: Applied under #405 before this PR was opened. Independent confirmation: both #406 and #407 currently report
|
Preview DeploymentPreview URL: https://05fbcc74.bestax.pages.dev |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…Y.md Adds Socket.dev, OpenSSF Scorecard, npm provenance, and security-policy badges plus a 'Hardened by default' section to all four READMEs. The two npm-published package READMEs are the highest-leverage place to say this and previously said nothing. Package READMEs use absolute URLs so links survive rendering on npmjs.com. SECURITY.md corrections: - Socket.dev and CodeQL were invisible in-repo (both dashboard-configured) - cooldown exceptions described as prettier-only; there are more - bestax-migrate missing from the supported-versions table - provenance said 'both packages'; there are three Closes #403
Socket posts two checks on every PR, but they are not in main's required_status_checks — only Build and Test, the React 18/19 matrix, and Dependency Review are. Caught by deep review on #407.
c9fb21c to
180bd6b
Compare
Preview DeploymentPreview URL: https://39717241.bestax.pages.dev |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
deep-review: RE-REVIEW — verify the prior finding was properly addressed, and re-verify every public security claim. Head is rebased; note CodeRabbit has NEVER successfully reviewed this PR (rate-limited), so you are the only reviewer on it. Your previous review flagged (🟡 Minor) that SECURITY.md called Socket’s checks "required" when they are not in main’s required_status_checks. The author claims this is fixed. Verify independently — this PR publishes security claims to npmjs.com, so an overstatement ships to consumers:
|
|
@claude Please re-review this PR to verify prior review feedback was addressed, and independently re-verify every public security claim in it. Two reasons this needs a careful pass: CodeRabbit never successfully reviewed this PR (rate-limited every attempt), and the content ships to npmjs.com, so an overstatement reaches consumers. Your earlier deep review flagged (🟡 Minor) that SECURITY.md called Socket’s checks "required" when they are not in Please verify independently, treating claims in the PR body and comments as unverified:
Report what is actually wrong. If a claim checks out, say so briefly rather than manufacturing findings. |
|
Claude finished @allxsmith's task in 3m 47s —— View job Re-review — verifying prior fix + every public security claim
|
| Claim | Verified against | Result |
|---|---|---|
| Install scripts blocked by default | pnpm-workspace.yaml:12-24 (allowBuilds, default-deny) |
✅ matches |
| 3-day cooldown | pnpm-workspace.yaml:29 minimumReleaseAge: 4320 (min) |
✅ matches (4320 min = 3 days) |
| Cooldown exceptions reworded from "prettier-only" | pnpm-workspace.yaml:33-34 — only prettier currently listed |
✅ generic wording is accurate; no fast-uri entry exists in this checkout (matches the PR's own "follow-up not done here" note that it's still pending removal elsewhere) |
| Isolated node_modules | pnpm-workspace.yaml:68 nodeLinker: isolated |
✅ matches |
| Frozen lockfile in CI | ci.yml:39,204 pnpm install --frozen-lockfile; React matrix job explicitly uses --no-frozen-lockfile (ci.yml:153) — matches the doc's stated exception |
✅ matches |
publishConfig.provenance on all three packages |
bulma-ui/package.json:205, create-bestax/package.json:77, bestax-migrate/package.json:72 — all true |
✅ "three" is correct |
OIDC publishing, no NPM_TOKEN |
ci.yml:224-235 — publish job comment explicitly documents no NPM_TOKEN, only id-token: write |
✅ matches |
| SHA-pinned actions | grepped every uses: line in .github/workflows/*.yml — 100% pinned to 40-char SHAs; the one non-SHA hit is a local composite action (./.github/actions/verified-commit), correctly out of scope |
✅ matches |
| Dependency Review blocks on high severity | dependency-review.yml:23 fail-on-severity: high, job named exactly Dependency Review |
✅ matches |
| Dependabot weekly, grouped | .github/dependabot.yml — interval: weekly, dev-dependencies/production-dependencies groups |
✅ matches |
| Scorecard weekly + badge | scorecard.yml exists on this checkout (schedule + push-to-main triggers, publish_results: true) — no longer a forward reference now that #406 is merged |
✅ matches |
| CodeQL / Socket dashboard-configured, no workflow file | confirmed no codeql.yml in .github/workflows/ |
✅ matches (can't verify the dashboard toggle itself — see #3) |
Package README SECURITY.md/docs-guide links are absolute |
all three package READMEs link https://github.com/allxsmith/bestax/blob/main/SECURITY.md and https://bestax.io/docs/guides/security |
✅ matches — resolves correctly on npmjs.com |
3. Branch protection — could not verify, saying so explicitly
git fetch, gh api, curl, and WebFetch all require approval this sandbox doesn't grant, so I cannot independently confirm required_status_checks, required_approving_review_count, or required_signatures on main right now. The four CI context names SECURITY.md cites (Build and Test, React 18 compatibility, React 19 compatibility, Dependency Review) do match the actual job names in ci.yml — so the doc isn't inventing checks that don't exist in the workflow — but whether they're wired into branch protection as required, and whether the 1-approval/signed-commits rules are live, is unverified by me. The author's pasted gh api payload in the thread may well be accurate, but I have no independent confirmation of it.
4. Independent adversarial review, different model — confirmed true
.github/workflows/claude-review.yml:267 pins --model claude-opus-4-8. .github/workflows/claude-implement.yml:116 pins --model claude-sonnet-5. Different models, confirmed directly in the workflow files — this claim is accurate.
5. bestax-migrate advertised while uninstallable — confirmed real, not fixed by this PR
bestax-migrate/package.json:51 still declares "@allxsmith/bestax-bulma": "workspace:^" in dependencies at the commit this PR merged into. A fix exists (fix(bestax-migrate): resolve workspace: specifiers before publishing, commit 782829a) but only on the still-open branch fix/412-migrate-workspace-protocol — git merge-base --is-ancestor 782829a 92e94e0 confirms it is not an ancestor of this PR's merge commit, i.e. not merged. So as of this PR, bestax-migrate 1.0.0 is genuinely what #412 describes: published with an unresolved workspace specifier, meaning npm install bestax-migrate pulls a dependency npm can't resolve.
This PR adds four Socket/Scorecard/provenance/security-policy badges and a supported-versions row for a package in that state. That's not a factual error — every individual badge/claim is still true (it does have provenance, Socket does scan it) — but the overall effect is advertising install-readiness for a package that currently can't be installed. Worth a one-line caveat in the table or a follow-up, though I'd call this advisory, not blocking: it doesn't misstate any security control, and it's arguably out of scope for a docs-only PR whose job is describing the pipeline, not the artifact.
Summary
The one prior blocking finding (Socket "required" wording) is genuinely fixed, and I found no instance of it recurring elsewhere in the diff. Every checkable claim in the new "Hardened by default" sections and SECURITY.md matches the actual repo/workflow state — I did not find any new overstatement. Two residual items are worth the author's attention, neither of which is a defect introduced by this PR:
- Branch protection (required checks/approvals/signatures) remains unverifiable from here — I have no network access in this environment, so I can't independently confirm it the way the author's pasted
gh apioutput claims. This isn't a finding against the PR; it's a limitation of this review pass. bestax-migrateis advertised as hardened/supported while#412(unresolvedworkspace:^) is still open on an unmerged branch — every claim about it is individually true, but the overall impression (safe to install) is currently inaccurate for that one package. Advisory-level, not blocking, and arguably belongs in a follow-up rather than this PR.
Since this PR is already merged, there's nothing to implement here — this is a verification pass only, as requested.
|
🎉 This PR is included in version 4.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.8.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Closes #403. Also lands the badge half of #404 and the documentation half of #405.
What
Badges + a Hardened by default section in all four READMEs, and four factual corrections to
SECURITY.md.README.md## 🔒 Hardened by defaultbulma-ui/README.md@allxsmith/bestax-bulma## 📦 NPM Packagecreate-bestax/README.mdcreate-bestax## Publishingbestax-migrate/README.md## LicenseWhy
bulma-ui/README.mdandcreate-bestax/README.mdare the npmjs.com package pages — the highest-leverage place to answer "is this dependency safe to add" — and they said nothing about provenance, scanning, or review. Socket.dev in particular was completely invisible: it's configured in the Socket dashboard, so no file in this repo mentioned it.SECURITY.md corrections found while auditing
These were wrong before this PR, independent of the new copy:
prettier".pnpm-workspace.yaml:33-38also excludesfast-uri. Reworded to describe the actual policy rather than enumerate a list that drifts.bestax-migratemissing from the supported-versions table. It's published (1.0.0) and setspublishConfig.provenance.Wording note
The review layer is described as AI bot reviews — CodeRabbit plus an independent adversarial Claude review on a deliberately different model — followed by required green CI, an approving review, and a human merge. It does not claim multiple human reviewers.
Verification
prettier --checkpasses on all five filespnpm check:conformance— all 9 checks greenSECURITY.mdand the docs guide, so links work when rendered on npmjs.com rather than resolving against a repo pathci.yml,pnpm-workspace.yaml,.coderabbit.yaml,dependency-review.yml, branch protection API)t3-oss/t3-envandccusage/ccusage. Please eyeball the rendered README here on GitHub, and the npm pages after the next release. Fallback if npm strips Socket-hosted images: swap to a shields.io badge linking to the Socket package page — shields.io is definitively rendered by npm, as the existing badges prove.Merge order
Merge #406 first. The Scorecard badge 404s until
scorecard.ymlhas run onmainat least once.Follow-up not done here
pnpm-workspace.yamlstill excludesfast-urifrom the cooldown, past its own stated removal date of 2026-07-22. Pruning it is a dependency-resolution change, not a docs change, so it's deliberately out of scope — worth a small separate PR (related: #391).