Skip to content

fix(review): verify PR head before E2E test commits - #4359

Merged
JSONbored merged 2 commits into
mainfrom
codex/propose-fix-for-fork-pr-vulnerability
Jul 10, 2026
Merged

fix(review): verify PR head before E2E test commits#4359
JSONbored merged 2 commits into
mainfrom
codex/propose-fix-for-fork-pr-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent a write-scope escalation where an unqualified PR headRef could be used to update the base repo branch when delivering generated E2E tests.
  • Ensure commit delivery only proceeds when the live PR head is confirmed to be the same-repository branch matching the cached ref and SHA.

Description

  • Add a live-PR verification step in commitE2eTestToPrBranch (src/github/e2e-test-commit.ts) that fetches the live PR and declines delivery unless live.head.repo.full_name === args.repoFullName and the live ref/sha match the cached values.
  • Retain the existing git tree/commit/ref update flow for same-repo PRs after the verification passes.
  • Add unit tests in test/unit/e2e-test-commit.test.ts that assert fork PRs are declined before any git write and that stale live-head data causes a decline.
  • Update test/unit/queue.test.ts stubs to account for the new live-PR lookup paths used by the commit delivery flow.

Testing

  • Ran npx vitest run test/unit/e2e-test-commit.test.ts, which passed (all new/modified tests in that file succeeded).
  • Ran a combined vitest invocation targeting both the e2e commit tests and queue suite, which failed to import test/unit/queue.test.ts due to a pre-existing duplicate identifier error (maybeProcessReviewCommand) already present in the codebase; this prevented the full queue suite from running in this environment.
  • Ran npm run typecheck, which surfaced pre-existing type errors unrelated to the change (duplicate implementations in src/queue/processors.ts and a missing aws4fetch type); no new type errors were introduced by the modified helper and tests.
  • Ran git diff --check with no whitespace/conflict issues reported for the modified files.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

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

@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 (5bd8615).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4359   +/-   ##
=======================================
  Coverage   94.12%   94.12%           
=======================================
  Files         430      430           
  Lines       38181    38187    +6     
  Branches    13922    13924    +2     
=======================================
+ Hits        35939    35945    +6     
  Misses       1585     1585           
  Partials      657      657           
Files with missing lines Coverage Δ
src/github/e2e-test-commit.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.

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

loopover-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

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

3 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 PR adds a live-PR verification step in `commitE2eTestToPrBranch` (src/github/e2e-test-commit.ts:65-72) that re-fetches the PR before any git write and declines delivery if the live head's repo doesn't match `args.repoFullName` or if ref/sha have drifted from the cached values — a real fix for the write-scope escalation described (a fork or stale cached ref could otherwise be used to push a commit onto an unintended branch). The security logic itself is sound and the new/updated tests (fork-PR decline, stale-head decline, queue.test.ts stub updates) exercise the real code path rather than fabricating an unreachable state. However, the second guard accesses `liveHead.ref`/`liveHead.sha` without narrowing `liveHead` past `undefined`, which is very likely the cause of the reported `validate-code` CI failure on this commit.

Blockers

  • src/github/e2e-test-commit.ts:66-70: after `if (liveHead?.repo?.full_name !== args.repoFullName) return ...`, TypeScript's control-flow analysis does not narrow `liveHead` to non-undefined (the comparison isn't against `undefined`, so optional-chaining narrowing doesn't apply), so the very next line's `liveHead.ref !== args.headRef || liveHead.sha !== args.headSha` reads `.ref`/`.sha` off a value still typed `{...} | undefined`, producing a TS2532 'Object is possibly undefined' compile error — consistent with the `validate-code` check failing on this commit; fix by adding an explicit `if (!liveHead) return { status: "declined", reason: ... }` before the repo-name check, or by using `liveHead?.ref !== args.headRef || liveHead?.sha !== args.headSha`.
Nits — 4 non-blocking
  • src/github/e2e-test-commit.ts:65: the new `GET .../pulls/{pull_number}` call adds a mandatory extra GitHub API request to every commit-delivery attempt (on top of the existing git/commits, git/trees, git/commits, and PATCH refs calls); worth a one-line comment noting this is the deliberate cost of closing the TOCTOU gap, since a future reader might otherwise try to 'optimize' it away.
  • test/unit/e2e-test-commit.test.ts: the fork-PR and stale-head tests are only added for the two new decline paths but there's no test asserting the happy path still fetches `/pulls/42` and proceeds when ref/sha match with mismatched-but-still-truthy fields (e.g. `sha` matches but `ref` doesn't) — consider a matrix test or at least a comment noting the OR condition's second arm is covered only implicitly by the 'stale head' test using both a different ref and sha simultaneously.
  • Add the `if (!liveHead) return ... ;` guard called out in the blocker so TS narrows correctly and the two field checks can safely drop back to direct property access without duplicating optional chaining.
  • Consider asserting in the 'stale live head' test that no `/git/` write calls occur either (mirroring the fork-PR test's `calls.some(...)` assertion), to guard against a future regression where the ref/sha check is moved after a write.

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

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 9, 2026
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 10, 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 5bd8615 Commit Preview URL

Branch Preview URL
Jul 10 2026, 07:02 AM

@JSONbored
JSONbored merged commit 41596a2 into main Jul 10, 2026
12 checks passed
@JSONbored
JSONbored deleted the codex/propose-fix-for-fork-pr-vulnerability branch July 10, 2026 07:20
JSONbored added a commit that referenced this pull request Jul 10, 2026
…it-delivery test (#4627)

Main-red: PR #4600 added this test before #4359 (merged same day) started
verifying the live PR head/ref/sha inside commitE2eTestToPrBranch before ever
writing. #4600 merged on top of #4359 without picking up the stub every
sibling commit-delivery test already got in #4359's own diff, so this one
test 404s on the live-PR fetch and silently declines the commit -- zero
git-write calls, gitWrites stays empty. Adds the same GET /pulls/{n} stub
the passing sibling tests already use.
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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant