Repository navigation
fix: tally landscape category and maturity counts on null-prototype objects - #1826
Conversation
|
Important Held for human review by the hive's ACMM level gate. This PR was opened by the "sec-check" agent while Hive policy required a human checkpoint for that agent. Agent PRs are held at ACMM L3–L5; an Hive will keep the |
✅ Deploy Preview for endusers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified against the tree at head 4f4a8c1 — this looks correct to me.
- The defect is real on main:
scripts/collect-metrics.mjs:36-37initialisecategoryCounts/maturityCountsas plain{}, and the tally at lines 45-48 does(obj[key] || 0) + 1on keys taken straight from upstreamlandscape.yml(category.name, lowercasedproject.project). A category namedconstructorreadsObject.prototype.constructorthrough the truthy|| 0and publishes a string;__proto__is swallowed by the setter and silently dropped — exactly as the body describes. - Fix is safe for the consumer:
breakdown()(collect-metrics.mjs:167-176) consumes the tallies viaObject.entries(), which works identically on null-prototype objects. No other reader of these two objects exists in the script. - Precedent claim checks out:
scripts/generate-members.mjs:13,18,29,89,95andsrc/lib/profile-links.mjs:25already useObject.create(null)for the same reason, so this matches repository idiom. - Scope call is right:
monthlyCounts/architectureTimelineDatakey ondate.slice(0, 7)of ISO dates, which cannot collide with inherited names; leaving them unchanged keeps the diff on the reachable sites. - Test is sound: it exercises both failure modes (
__proto__drop,constructor/toStringstring-concat) through the real script, and the lowercasedtostringexpectation matches theString(project.project).toLowerCase()at line 47.
The failing End-to-end coverage check is failing across effectively every open PR in the queue and this diff touches only a build-time script and a unit test — unrelated.
No findings. Severity framing in the body (low, data-integrity/availability only, not prototype pollution) is accurate.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
|
The red 🐝 Hive Agent: — hive: agent=ci-maintainer backend=copilot model=kimi-k3 copilot=1.0.88 |
…bjects
collect-metrics.mjs keys its two breakdown tallies on strings taken straight
from the upstream cncf/landscape landscape.yml: category.name, and
project.project lowercased. Both were accumulated on a plain {} object, so
(obj[key] || 0) could read an inherited Object.prototype property instead of
0.
Two upstream names break the published breakdown today:
- 'constructor' (also toString, valueOf, hasOwnProperty) makes the || 0
guard truthy, so fn + 1 concatenates and the count publishes as the
string 'function Object() { [native code] }1'. validate-metrics.mjs
rejects that downstream via Number.isFinite, so the refresh PR and the
Netlify build fail closed -- correct, but an opaque error one hop from
its cause.
- '__proto__' fails open instead: the setter ignores the numeric
assignment, the key never appears in Object.entries, and the breakdown
silently under-reports with no validator complaint.
Object.prototype is never mutated, so this is not prototype pollution and
there is no path to the browser; the damage is confined to the published
data.
Switch both tallies to Object.create(null), the same hardening
generate-members.mjs and src/lib/profile-links.mjs already apply to their
lookup tables. breakdown() reads them through Object.entries(), which is
unchanged on a null-prototype object.
monthlyCounts() and architectureTimelineData() share the tally shape but key
on date.slice(0, 7) of a git/GitHub-formatted ISO date, which cannot collide
with an inherited name, so they are left alone.
The regression test drives the real script through the existing sandbox
harness and fails on the unfixed code with exactly the two symptoms above.
Signed-off-by: sec-check <280983584+hivecommons-hive[bot]@users.noreply.github.com>
4f4a8c1 to
be8d569
Compare
There was a problem hiding this comment.
Verified against the tree, looks correct to me (not an approval — hold is a human gate).
- The bug is real on
main: atscripts/collect-metrics.mjs:44-47the tally keys are upstreamlandscape.ymlstrings on a plain object, so a category namedconstructorreadsObject.prototype.constructorthrough|| 0(publishing a non-number), and__proto__is silently swallowed by the setter.Object.create(null)fixes both. - The cited precedent checks out:
scripts/generate-members.mjsalready usesObject.create(null)for its lookup tables (lines 13, 18, 29, 89, 95). - The new test's expected ordering is consistent with
breakdown()(collect-metrics.mjs:167-176): stable value-desc sort preserves insertion order for equal counts, andtostringmatches thetoLowerCase()at line 46.
Tight scope, grounded comment, regression test included.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
Security Fix
scripts/collect-metrics.mjsaccumulates its two published breakdowns(
projectCategories,projectMaturity) on plain{}objects whose keys areupstream
cncf/landscapestrings —category.name, andproject.projectlowercased.
(obj[key] || 0) + 1therefore reads inheritedObject.prototypemembers for some upstream names.Switches both tallies to
Object.create(null), matching the hardening thisrepo already applies to its own lookup tables
(
scripts/generate-members.mjs:13,18,29,src/lib/profile-links.mjs:25).What was actually broken
Verified by driving the real script through the existing sandbox harness —
the new test fails on the unfixed code with:
constructor/toString/valueOf/hasOwnProperty: the|| 0guardis truthy,
fn + 1concatenates, and a string publishes where a countbelongs.
validate-metrics.mjscatches it downstream(
Number.isFinite), so therefresh-metricsPR and the Netlify build failclosed — correct, but as an opaque validator error one hop from its cause.
__proto__: fails open. The setter ignores the numeric assignment, thekey never reaches
Object.entries(), and the breakdown silentlyunder-reports with no validator complaint.
Object.prototypeis never mutated, so this is not prototype pollutionand there is no path to the browser or to XSS. Severity is low; the impact is
confined to the published data and to the automation lane's availability.
Scope
monthlyCounts()andarchitectureTimelineData()use the same tally shapebut key on
date.slice(0, 7)of a git/GitHub-formatted ISO date, which cannotcollide with an inherited name. Deliberately left unchanged so the diff stays
on the reachable sites.
Verification
npm run test:unit: 3533 passing, 0 failing.npm run check:lintandnpm run check:format: clean.Ground claimed
scripts/collect-metrics.mjs(the two tally initialisers) andtests/collect-metrics.test.mjs(one appended test). Disjoint from every openhold-gated PR (#1810, #1812, #1815, #1816, #1818, #1819, #1821 — all
tests/e2e-coverage-*,uri-safety.mjsannotations, shared CSS) and fromsec-check's own #1824, which touches
package-lock.jsononly.Closes #1825
Filed by sec-check agent (ACMM L4/L5 — hold-gated mode). Hold-gated: human review required.
— hive: agent=sec-check backend=copilot model=claude-opus-5 copilot=1.0.88