Skip to content

fix(miner): reject unsafe telemetry metric names#4487

Merged
JSONbored merged 2 commits into
mainfrom
codex/fix-telemetry-metrics-keys-to-prevent-leakage
Jul 10, 2026
Merged

fix(miner): reject unsafe telemetry metric names#4487
JSONbored merged 2 commits into
mainfrom
codex/fix-telemetry-metrics-keys-to-prevent-leakage

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The telemetry normalizer validated only metric values and then JSON.stringify-ed the whole metrics object, which preserved unvalidated metric keys and could leak raw repo/issue identifiers or free text into metricsJson.

Description

  • Add a metricNamePattern and update normalizeMetrics to validate both metric names and values so keys must be low-cardinality identifier-style names before serialization.
  • Update the normalizeMetrics JSDoc to document that free-text or raw-identifier metric names are rejected and absent metrics normalize to {}.
  • Add a unit test that asserts unsafe metric names (e.g. containing /, whitespace, punctuation, empty string or dashed names) are rejected with invalid_metrics.

Testing

  • Ran the unit suite for the change with npx vitest run test/unit/miner-telemetry.test.ts, and the new/updated tests passed.
  • Verified TypeScript build/typecheck with npm run build:miner and npm run typecheck, both of which succeeded.
  • Attempted coverage with npm run test:coverage for the modified test file; tests passed but coverage remapping failed in this environment with TypeError: jsTokens is not a function.
  • Attempted the full gate with npm run test:ci, but the run did not complete in this environment due to unrelated pre-existing checks (cf-typegen reporting stale runtime types) and an npm audit network error (registry returned 403).

Codex Task

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 9, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui fe42405 Commit Preview URL

Branch Preview URL
Jul 10 2026, 07:01 AM

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@gittensory-orb gittensory-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. label Jul 9, 2026
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.12%. Comparing base (63d5a0f) to head (fe42405).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4487   +/-   ##
=======================================
  Coverage   94.12%   94.12%           
=======================================
  Files         430      430           
  Lines       38181    38183    +2     
  Branches    13922    13922           
=======================================
+ Hits        35939    35941    +2     
  Misses       1585     1585           
  Partials      657      657           
Files with missing lines Coverage Δ
packages/gittensory-engine/src/miner-telemetry.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb

gittensory-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-10 07:17:35 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · unstable

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This closes a real anti-leak gap: normalizeMetrics previously validated only metric values and JSON.stringify'd the object as-is, so an unvalidated key (e.g. a raw `owner/repo#123` string or free text) could reach metricsJson untouched. The added metricNamePattern (`^[A-Za-z][A-Za-z0-9_]{0,63}$`) is checked in the same loop as the value check and throws the existing `invalid_metrics` error, matching the module's fail-closed convention (mirrors normalizeOptionalHash's anti-leak guard). The new test exercises the actual reachable path — a real caller supplying a leaking key — not a fabricated/unreachable state, and existing valid camelCase keys (candidatesReturned, rankMs) still pass, so this isn't a symptom-layer patch; it's the correct source-layer fix.

Blockers

  • The PR description does not link or close any open issue in this repo, which this repo's contributor policy requires even for a well-scoped fix like this.
Nits — 4 non-blocking
  • packages/gittensory-engine/src/miner-telemetry.ts:63 — the pattern's max length (64 via `{0,63}`) and starting-char requirement are undocumented magic constants; a one-line comment noting why 64 (or referencing an existing convention) would help the next reader.
  • test/unit/miner-telemetry.test.ts — no test asserts a valid underscore-containing name like `rank_ms` or a name at the 64-char boundary, which would pin down the pattern's edges (currently only invalid names and previously-existing camelCase names are exercised).
  • Consider extracting `64` in the regex into a named constant (e.g. `MAX_METRIC_NAME_LENGTH`) alongside `MAX_ITEM_LENGTH`/`MAX_LIST_ITEMS` in miner-goal-spec.ts for consistency with this repo's existing pattern of named length constants.
  • Add a boundary test (63/64/65-char name) to pin the length limit explicitly.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 334 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 334 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 334 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added the manual-review Gittensor contributor context label Jul 9, 2026
@JSONbored JSONbored merged commit c074efd into main Jul 10, 2026
13 checks passed
@JSONbored JSONbored deleted the codex/fix-telemetry-metrics-keys-to-prevent-leakage branch July 10, 2026 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensor:bug Gittensor-scored bug fix — scores a 0.5x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant