Skip to content

fix(rees): harden SLUG_RE against dot-segment path traversal in 17 analyzers - #4959

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
fix/rees-slug-re-dot-segment-hardening
Jul 11, 2026
Merged

fix(rees): harden SLUG_RE against dot-segment path traversal in 17 analyzers#4959
loopover-orb[bot] merged 1 commit into
mainfrom
fix/rees-slug-re-dot-segment-hardening

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Hardens SLUG_RE in 17 review-enrichment analyzers from the weak /^[A-Za-z0-9._-]+$/ to /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/, matching the two analyzers (duplication-delta.ts, codeowners.ts) that already use the stricter pattern.
  • A bare [A-Za-z0-9._-]+ class lets a slug segment made entirely of dots (e.g. owner="..") pass validation, since every character in ".." is individually allowed by that class. These owner/repo values get spliced into a GitHub Contents-API URL passed to fetch()/new URL(); a leading-dot segment could let a URL parser's dot-segment resolution rewrite the path, sending the request (and the real auth token) somewhere other than the intended owner/repo. Requiring an alphanumeric first character closes that gap with no behavior change for any legitimate slug.
  • Adds a test to each of the 17 files confirming a ".."-shaped owner/repo segment is now rejected (fails safe, no finding, never throws) — extending an existing invalid-slug test where one already existed, adding a new one otherwise.
  • Also normalizes 3 test files (exhaustiveness-drift.test.ts, flaky-test.test.ts, unused-export.test.ts) from pre-existing mixed CRLF/LF line endings (already present on main, unrelated to this change) to plain LF, matching every other file in the package — the mixed endings were tripping git diff --check once these files were touched for this PR.

Closes #4955

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused: a single mechanical hardening pass across review-enrichment/src/analyzers/** and its tests only — no backend/UI/MCP/docs/dependency/deploy changes mixed in.
  • This follows CONTRIBUTING.md and does not touch GitHub Pages, VitePress, site/, or CNAME.
  • Linked open issue: Closes Harden SLUG_RE against dot-segment confusion in 17 REES analyzers #4955.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage — passes (13910 tests, coverage thresholds met); note none of the 17 changed analyzer/test files are in vitest's coverage include scope (only the untouched review-enrichment/src/analyzers/codeowners.ts is), so codecov/patch has no lines to grade on this diff. REES's own gate, npm run rees:test, is the relevant bar here and is fully green (1308/1308, including all 17 new dot-segment tests).
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New/changed behavior has tests for every new branch: all 17 touched analyzers each get a dedicated test confirming the new first-character requirement rejects a dot-segment slug.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • N/A — no public GitHub comment/text content changed.
  • N/A — no auth/cookie/CORS/GitHub App/Cloudflare/session changes.
  • N/A — no API/OpenAPI/MCP behavior changed.
  • N/A — no UI changes.
  • N/A — no visible UI change (see UI Evidence below).
  • N/A — no docs/changelog changes needed for a REES-internal hardening pass.

UI Evidence

Not applicable — this PR only touches review-enrichment/src/analyzers/** and review-enrichment/test/**, a backend service package with no UI surface.

Notes

  • Mirrors the exact hardened pattern and trailing comment (// rejects \..` and other path-traversal segments) already used by codeowners.ts`, kept byte-identical across all 17 files for consistency rather than 17 different phrasings.
  • Deliberately a single pass across all 17 files at once, per the issue's explicit request — this was previously deferred twice (PR refactor(rees): migrate three analyzers off hand-rolled bounded-fetch onto boundedFetchText #4821, PR refactor(rees): migrate three more analyzers off hand-rolled bounded-fetch onto boundedFetchText #4947), each of which touched 3 of these same files but left this fix for one consistent pass instead of a partial one.
  • Four files (churn-hotspot.ts, coverage-delta.ts, flaky-test.ts, revert-recurrence.ts) guard with !owner || !repo || !SLUG_RE.test(owner) || !SLUG_RE.test(repo) but without an explicit parts.length !== 2 check that the other 13 files have. That's a separate, pre-existing gap unrelated to the dot-segment character-class issue this PR fixes (a 3+ segment repoFullName still resolves to the correct first two parts; it isn't a path-traversal risk), so it's left out of this mechanical, single-purpose pass — flagging it here as a possible small follow-up rather than silently leaving it unmentioned.

…alyzers

Change SLUG_RE from the weak /^[A-Za-z0-9._-]+$/ to /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/
in all 17 review-enrichment analyzers still using the old pattern, matching
duplication-delta.ts and codeowners.ts (the only two files already hardened).
A bare [A-Za-z0-9._-]+ class lets a slug segment made entirely of dots (e.g.
owner="..") pass validation, since every character in ".." is individually
allowed. These owner/repo values get spliced into GitHub Contents-API URLs
passed to fetch()/new URL(); a leading-dot segment could let a URL parser's
dot-segment resolution rewrite the path, sending the auth token somewhere
other than the intended owner/repo. Requiring an alphanumeric first character
closes that gap without any behavior change for a legitimate slug (GitHub
owner/repo names always start with an alphanumeric character).

Each changed analyzer gets a test confirming a ".."-shaped owner/repo segment
is rejected (fails safe, no finding, never throws) — extending an existing
invalid-slug test where one already existed, adding a new one otherwise.

Also normalizes three test files (exhaustiveness-drift, flaky-test,
unused-export) from pre-existing mixed CRLF/LF line endings to plain LF,
matching every other file in the package; the mixed endings were already
present on main and were tripping git diff --check on these files now that
they're touched.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 11, 2026
@loopover-orb

loopover-orb Bot commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-11 03:22:12 UTC

34 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a clean, mechanical security hardening pass: it tightens SLUG_RE from a bare `[A-Za-z0-9._-]+` character class (which a dot-only segment like ".." satisfies character-by-character) to `[a-zA-Z0-9][a-zA-Z0-9._-]*`, requiring an alphanumeric first character, across all 17 analyzers that interpolate owner/repo into GitHub API URLs. The fix correctly mirrors the already-stricter pattern in duplication-delta.ts and codeowners.ts, and each file's full content confirms the regex is applied identically and the vulnerable `SLUG_RE.test(owner) || SLUG_RE.test(repo)` guard now genuinely rejects a `..`/`.` segment before any fetch. Every changed analyzer gets a real test asserting the fetch function is never called for a `../evil`-shaped repoFullName, which exercises the actual guarded code path rather than a fabricated scenario. The oversized test-file diffs (e.g. flaky-test.test.ts +178/-164) are explained as pre-existing CRLF→LF normalization incidental to touching those files, not hidden logic changes.

Nits — 4 non-blocking
  • The CRLF→LF normalization of 3 unrelated test files is bundled into a security-fix PR; even though it's explained and low-risk, a separate whitespace-only commit would have made the diff easier to audit at a glance.
  • The inline comment `// rejects \`..\` and other path-traversal segments` is repeated verbatim in all 17 files — fine for consistency, but consider centralizing SLUG_RE as a shared exported constant in a common module given it's now duplicated in ~19 files with identical semantics.
  • Consider extracting SLUG_RE (and the owner/repo split+validate logic) into a shared helper in a common module (e.g. github-headers.js or a new slug.ts) so future hardening only needs to happen once instead of being copy-pasted across 19 analyzer files.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #4955
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 312 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 312 issue(s).
Gate result ✅ Passing No configured blocker found.
Linked issue satisfaction

Addressed
The diff updates SLUG_RE to the hardened `/^[a-zA-Z0-9][a-zA-Z0-9._-]*$/` pattern with the dot-segment-risk comment in all 17 listed files, and adds/extends dot-segment rejection tests in each corresponding test file, matching the issue's stated fix and acceptance criteria in a single pass.

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: Python, TypeScript, Ruby, Go, JavaScript, MDX, Shell, Solidity
  • Official Gittensor activity: 48 PR(s), 312 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 4a662c8 into main Jul 11, 2026
11 checks passed
@loopover-orb
loopover-orb Bot deleted the fix/rees-slug-re-dot-segment-hardening branch July 11, 2026 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden SLUG_RE against dot-segment confusion in 17 REES analyzers

1 participant