Skip to content

fix(review): authorize review thread blockers#1807

Merged
JSONbored merged 2 commits into
mainfrom
codex/propose-fix-for-review-thread-vulnerability
Jun 30, 2026
Merged

fix(review): authorize review thread blockers#1807
JSONbored merged 2 commits into
mainfrom
codex/propose-fix-for-review-thread-vulnerability

Conversation

@JSONbored

@JSONbored JSONbored commented Jun 30, 2026

Copy link
Copy Markdown
Owner

Motivation

  • No linked issue: this PR resolves a security scanner finding in review-thread blocker authorization.
  • Prevent typosquatted GitHub App bot names and forged public review-thread comments from being treated as trusted scanner blockers.
  • Preserve intended blocking behavior for repository-authorized reviewers and known scanner apps.

Description

  • Add authorAssociation to the GraphQL reviewThreads query and carry it through the review-thread parser in src/github/backfill.ts.
  • Only convert a thread comment into a blocker when the author is explicitly trusted: repository OWNER/COLLABORATOR, a raw MEMBER that verifies to live repository admin/maintain/write permission, or one of the exact known scanner bot logins.
  • Verify raw review-comment MEMBER through the repository collaborator-permission endpoint before trusting it. Raw MEMBER can mean organization membership, which is not by itself repository write or maintain permission.
  • Ignore the app's own bot authors before applying any maintainer/scanner authorization check.
  • Add regression coverage for exact scanner bot allowlisting, scanner typosquats, untrusted public comments, verified and unverified MEMBER authors, permission lookup failures, permission-cache reuse, malformed MEMBER authors, and own-bot precedence with privileged associations.

Testing

  • npx vitest run test/unit/backfill.test.ts -t fetchLiveReviewThreadBlockers
  • npm run test:ci
  • npm audit --audit-level=moderate

@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jun 30, 2026

@superagent-security superagent-security Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Superagent found 1 security concern(s).

Comment thread src/github/backfill.ts
@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jun 30, 2026
@gittensory-orb

gittensory-orb Bot commented Jun 30, 2026

Copy link
Copy Markdown

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-06-30 05:27:27 UTC

2 files · 1 AI reviewer · no blockers · readiness 93/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change tightens review-thread blocker attribution by carrying GraphQL authorAssociation through parsing, ignoring the app's own comments first, exact-matching known scanner bot logins, and verifying raw MEMBER authors against live repository permissions before trusting them. The core authorization order is coherent: trusted scanner bots and OWNER/COLLABORATOR comments remain blocking, while public comments and unverified organization members are filtered out. The added tests exercise the important security cases, including typosquats, permission failures, cache reuse, malformed members, and own-bot precedence.

Nits — 5 non-blocking
  • nit: src/github/backfill.ts:2371 should document why the scanner allowlist contains these exact bot logins, because adding/removing scanner identities is now security-sensitive configuration embedded in code.
  • nit: test/unit/backfill.test.ts:3617 adds broad regression coverage, but the diff does not show a test where a thread contains both an untrusted forged comment and a later authorized maintainer/scanner comment, which is the mixed-comment case most likely to regress the filtering semantics.
  • src/github/backfill.ts:2371: move the trusted scanner login allowlist near other GitHub integration constants or add a short comment pointing to the owning scanner integration so future scanner renames do not silently stop blocking.
  • test/unit/backfill.test.ts:3617: add one mixed-thread fixture with untrusted and authorized comments in the same unresolved thread to lock in that only authorized comment bodies feed buildReviewThreadBlocker.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
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 (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 2 registered-repo PR(s), 2 merged, 263 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 2 PR(s), 263 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 2 PR(s), 263 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Triage stale or unlinked PRs.
  • No action.
  • 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 gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 30, 2026
@JSONbored JSONbored self-assigned this Jun 30, 2026
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.66%. Comparing base (0924f23) to head (4647e12).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1807      +/-   ##
==========================================
+ Coverage   95.62%   95.66%   +0.03%     
==========================================
  Files         214      214              
  Lines       23274    23299      +25     
  Branches     8402     8412      +10     
==========================================
+ Hits        22255    22288      +33     
+ Misses        422      419       -3     
+ Partials      597      592       -5     
Files with missing lines Coverage Δ
src/github/backfill.ts 93.96% <100.00%> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored force-pushed the codex/propose-fix-for-review-thread-vulnerability branch from 25d7aed to 0214bf8 Compare June 30, 2026 04:30
@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jun 30, 2026
@JSONbored JSONbored force-pushed the codex/propose-fix-for-review-thread-vulnerability branch 3 times, most recently from 06694a8 to 3f474d5 Compare June 30, 2026 04:54
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Jun 30, 2026
@superagent-security superagent-security Bot removed the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 30, 2026
@JSONbored JSONbored force-pushed the codex/propose-fix-for-review-thread-vulnerability branch from 3f474d5 to a615498 Compare June 30, 2026 05:10
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 30, 2026
@JSONbored JSONbored force-pushed the codex/propose-fix-for-review-thread-vulnerability branch from a615498 to 4647e12 Compare June 30, 2026 05:24
@JSONbored JSONbored merged commit 65adfe6 into main Jun 30, 2026
17 checks passed
@JSONbored JSONbored deleted the codex/propose-fix-for-review-thread-vulnerability branch June 30, 2026 05:41
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 30, 2026
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 - worth 0.5x multiplier. gittensor Gittensor contributor context size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant