Skip to content

fix(github): reject malformed repoFullName in app.ts and comments.ts - #8402

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-repofullname-guard-8311
Jul 24, 2026
Merged

fix(github): reject malformed repoFullName in app.ts and comments.ts#8402
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-repofullname-guard-8311

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • src/github/app.ts's three GitHub-write call sites did a bare two-variable destructure with only a truthiness check, so two failure modes every sibling module in src/github/ already rejects got through: "owner/repo/extra" silently dropped the extra segment and issued the call against a different repo than the caller named, and a padded "owner/ repo" / " owner/repo" was encodeURIComponent-ed straight into a GitHub API URL.
    • getRepositoryCollaboratorPermission (:446), cancelInFlightWorkflowRunsForHeadSha (:620), createOrUpdateNamedCheckRun (:911 — the function backing every createOrUpdate*GateCheckRun/createOrUpdateCheckRun, i.e. the gate-check-posting path).
  • src/github/comments.ts (createOrUpdateIssueCommentWithMarker) had the segment-count guard but was missing the /\s/ whitespace check that pr-actions.ts/assignees.ts/labels.ts gained under pr-actions.ts splitRepo misses per-segment whitespace padding (owner/ repo slips through) #6613.
  • Adds a small local parseRepoFullNameStrict helper inside app.ts, reused by its own three call sites, per this directory's stated house convention of a per-module copy of this tiny pure check rather than a shared cross-file export (issues.ts:5-15's own comment). It returns null so each call site maps a malformed value to its existing, unchanged failure contract:
    • getRepositoryCollaboratorPermission → returns null
    • cancelInFlightWorkflowRunsForHeadSha → returns { kind: "error", warning: … }
    • createOrUpdateNamedCheckRun → throws Invalid repository full name: …
    • createOrUpdateIssueCommentWithMarker → throws (already did; only the whitespace condition was added)
  • No behavior change for any well-formed owner/repo value.
  • Regression tests at all four call sites (test/unit/github-app.test.ts, test/unit/github-comments.test.ts) assert "owner/repo/extra" and whitespace-padded slugs are rejected exactly like the existing no-slash "invalid" case, and collectively exercise all four operands of the guard (parts.length !== 2, !owner, !repo, /\s/) plus the valid-input path.

Closes #8311

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • 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
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • This diff touches only src/github/app.ts, src/github/comments.ts and their two root test suites, so actionlint (no workflow change), build:mcp/test:mcp-pack (no MCP change), ui:openapi:check/ui:lint/ui:typecheck/ui:build (no UI or OpenAPI/route change), test:workers (no worker change), and npm audit (no dependency change) are not exercised by it.
  • Diff coverage verified at 100%, lines and branches, via the scoped simulation CI actually runs (vitest run --coverage --coverage.all=false --changed=origin/main): the resulting coverage/lcov.info is non-empty, contains both changed source files, and every executable changed line and branch in them is hit (0 uncovered). test/unit/github-app.test.ts + test/unit/github-comments.test.ts pass in full (113 tests). Root tsc --noEmit is clean.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — backend-only input-validation hardening in src/github/; no visible UI, frontend, docs, or extension change.

Notes

  • This is a defense-in-depth tightening, not a bug with a known live trigger: every in-repo caller already passes a well-formed owner/repo, so no existing behavior changes. The value is closing the last inconsistency in this boundary — pr-actions.ts, assignees.ts, labels.ts, issues.ts, and milestones.ts all already reject these shapes, and app.ts carries the heaviest GitHub-write traffic (installation tokens, check-run creation, workflow-run cancellation).
  • Per the issue's required pattern, no new shared cross-module helper was introduced in client.ts or elsewhere; the guard is a local helper within app.ts (its three call sites share a file) and an inline condition in comments.ts, matching the five existing per-module copies.

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 24, 2026 11:39
@superagent-security

Copy link
Copy Markdown
Contributor

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

app.ts's three GitHub-write call sites (getRepositoryCollaboratorPermission,
cancelInFlightWorkflowRunsForHeadSha, createOrUpdateNamedCheckRun) did a bare
two-variable destructure with only a truthiness check, so "owner/repo/extra"
silently dropped the extra segment and issued a call against a different repo,
and a padded "owner/ repo" was encodeURIComponent-ed straight into a GitHub
URL. comments.ts had the segment-count guard but not the whitespace one.

Adds a local parseRepoFullNameStrict helper in app.ts (per this directory's
house convention of a small per-module copy rather than a shared export) used
by all three call sites, each preserving its existing failure contract, and
adds the whitespace condition to comments.ts's existing check. Regression tests
cover the extra-segment and whitespace-padded shapes at all four call sites.

Closes JSONbored#8311
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.71%. Comparing base (4a0b136) to head (1e24a28).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8402      +/-   ##
==========================================
- Coverage   92.17%   89.71%   -2.47%     
==========================================
  Files         791       99     -692     
  Lines       79249    23044   -56205     
  Branches    23947     3993   -19954     
==========================================
- Hits        73048    20673   -52375     
+ Misses       5062     2188    -2874     
+ Partials     1139      183     -956     
Flag Coverage Δ
shard-1 66.86% <75.00%> (+8.10%) ⬆️
shard-2 55.91% <81.25%> (+5.38%) ⬆️
shard-3 89.94% <87.50%> (+36.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/github/app.ts 97.94% <100.00%> (+0.05%) ⬆️
src/github/comments.ts 100.00% <100.00%> (ø)

... and 692 files with indirect coverage changes

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

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-24 12:06:13 UTC

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

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This diff adds a local parseRepoFullNameStrict helper to app.ts, applied at its three GitHub-write call sites, and fills a missing whitespace check into comments.ts's existing segment-count guard — both bringing repoFullName validation in line with the pattern already used in assignees.ts/labels.ts/issues.ts. The fix is correct and traced end-to-end: each call site's malformed-input branch (null return, error outcome, or throw) preserves its existing contract, and the new tests exercise all four guard operands (segment count, missing owner, missing repo, whitespace) at each of the four call sites without fabricating unreachable states. This closes the linked issue #8311, which explicitly calls out app.ts/comments.ts as missing the guard other sibling modules already have.

Nits — 3 non-blocking
  • src/github/app.ts:446-462 duplicates the same parseRepoFullNameStrict logic already present nearly verbatim in issues.ts/assignees.ts/labels.ts — the PR description defends this as house convention (issues.ts:5-15's own comment), which is a reasonable call given the existing pattern, but it's worth confirming that's still the desired direction rather than finally extracting a shared helper.
  • test/unit/github-comments.test.ts's diff content wasn't available in full (file-content omitted for budget) so its added test block couldn't be independently verified beyond the diff hunk shown, though the hunk itself looks consistent with the other three test additions.
  • If a fourth or fifth call site needing this guard shows up, consider promoting parseRepoFullNameStrict to a shared src/github/repo-full-name.ts export rather than adding a fifth local copy.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8311
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: 354 registered-repo PR(s), 137 merged, 37 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 354 PR(s), 37 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds a local parseRepoFullNameStrict helper in app.ts reused by all three named call sites, preserving each site's original failure contract (null / error object / throw), and adds the missing /\s/ whitespace check to comments.ts's existing segment-count guard, matching the required per-module pattern.

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, Ruby, JavaScript, Svelte, TypeScript, Cuda, Markdown, MDX
  • Official Gittensor activity: 354 PR(s), 37 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver 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.

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

@loopover-orb
loopover-orb Bot merged commit 00b7f8c into JSONbored:main Jul 24, 2026
12 checks passed
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.

fix(github): app.ts/comments.ts repoFullName parsing is missing the segment-count/whitespace guard every sibling GitHub-write module has

1 participant