Skip to content

fix(checkouts): secure git auth and protect mirror acquisition - #326

Merged
smith-xyz merged 3 commits into
developmentfrom
fix/checkout-merged-review-gaps
Oct 31, 2026
Merged

smith-xyz merged 3 commits into
developmentfrom
fix/checkout-merged-review-gaps

Conversation

@chubirka

Copy link
Copy Markdown
Member

What

Fix outstanding checkout defects from merged PRs #311, #312, #313 and #318:

  • Use Git-compatible HTTP Basic token authentication for GitHub/GitHub Enterprise; preserve REST auth and GitLab's oauth2 username.
  • Fetch a requested commit when it exists only under a PR/MR ref, for both checkout and refresh, without importing the full review-ref namespace. Missing commits remain not-found; transport failures are not relabeled as missing commits.
  • Coordinate eviction with fetch/acquisition through worktree registration and with release. Preserve active-worktree protection and refresh's existing idle-clock semantics.
  • Accept full SHA-256 commit IDs for local checkouts and history reads, while retaining existing SHA-1/short-SHA behavior.

Review disposition

The reviewed #311/#312 heads merged unchanged. #313/#318 received other fixes, but these outstanding defects remained on the actual merged development tree. #318 already corrected the process allowlist/timeout finding; this PR leaves that correction intact. The export-ignored regression-pair issue is addressed separately in #325.

No changes to the Java ownership work in #323. No public API or dependency-pin changes.

Verification

  • Before the fix, the current merged baseline failed the GitHub/GHE authenticated Git regressions and the SHA-256 checkout regression; the GitLab control passed.

  • Real-Git baseline smoke reproduced rejection of a commit retained only by a PR ref and deletion of an actively fetching mirror.

  • Focused checkout/provider suite: 103 passed.

  • Standalone production-provider smoke: PR-only checkout/refresh, synchronized eviction and release, SHA-256 checkout/history all succeeded.

  • Real read-only private GitHub ls-remote with ForgeApi.git_headers matched the independently authenticated forge API HEAD; no credential was printed or written.

  • Full unit gate: 8,395 passed, 1 existing skip.

  • Mandatory pre-push lint/coverage gates passed: 8,395 passing cases, 1 existing skip, 96.20% coverage (80% required).

  • Native PostgreSQL 18.6 integration suite: 3,025 cases passed, 3 existing skips, across the complete invocation and a targeted rerun after setting the isolated cluster to the UTC timezone expected by the existing freshness test. No unrelated production behavior was changed.

  • Replaced brittle filename-substring leak assertions with actual workspace-lifecycle checks; temporary roots are outside the workstation's parent Git repository. No hooks or gates were bypassed.

Related: #311, #312, #313, #318. Continuous operations / XWING-2440.

This PR is for review only; auto-merge is not requested.

@smith-xyz

Copy link
Copy Markdown
Contributor

/agentic_review

@smith-xyz

Copy link
Copy Markdown
Contributor

@qodo is this duplicating what #327 does?

@qodo-for-traust-security

Copy link
Copy Markdown

is this duplicating what #327 does?

I can’t inspect pull request 327 from this environment, so I can’t confirm duplication from the link alone. This PR adds three main changes: authenticated Git HTTP via Basic auth, direct fetching of missing SHA-1/SHA-256 or PR/MR-only commits, and locking around mirror acquisition/eviction. If you paste PR 327’s description or diff, I can compare them precisely and identify any duplicated or conflicting changes.

@qodo-for-traust-security

qodo-for-traust-security Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Code Review by Qodo


Resolved findings
1. Older review commits fail checkout ✓ Resolved
Description
MirrorCheckout._fetch selects a review ref only when its advertised tip exactly equals the
requested SHA, rather than checking whether the ref contains that commit. If a pull or merge-request
ref advances from commit C to descendant D while C remains outside the mirrored heads and tags,
checkout and refresh for C both raise NotFoundError even though the review ref still retains it.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
An older commit retained in the history of a current review ref is rejected because the review-ref fallback matches only the ref's tip. Checkout and refresh must still reject commits outside the repository's currently advertised refs.

## Fix Focus Areas
- src/traust_core/v1/providers/mirror.py[144-178]
- tests/v1/providers/test_mirror.py[196-242]

## Recommended Fix
Fetch a currently advertised review ref and verify that the requested commit is an ancestor of its fetched tip before accepting it. Preserve rejection of deleted or unadvertised refs. Add checkout and refresh regressions in which a review ref advances from C to descendant D and the request still names C.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Checkouts accept commits from outside the repo ✓ Resolved
Description
When a SHA isn't reachable from the mirrored heads or tags, _fetch now runs `git fetch <repo>
<sha> with the bare SHA. A non-zero cat-file -e` afterwards is the only check that the commit
belongs to the repository. On GitHub, objects are shared across the fork network and dangling
commits are kept, so a commit that exists only in someone's fork, or on a deleted or force-pushed
branch, is fetched and checked out as if it were request.repo's. The scan results, fleet-fix
worktrees and refreshes are then attributed to that repository, and the old `commit … is not in
<repo>` guard no longer stops them.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The fallback `git fetch <repo> <sha>` accepts any object the forge serves for that SHA. On GitHub that includes commits that exist only in a fork, and dangling commits. Only an object-existence check follows the fetch, so a commit from outside the repository passes as the repo's commit.

## Fix Focus Areas
- src/traust_core/v1/providers/mirror.py[141-156]

## Recommended Fix
Only accept the SHA if the repository's own review refs contain it. One way: run `git ls-remote <repo> refs/pull/* refs/merge-requests/*` (with the same env) and confirm that a review ref points at the SHA, or contains it, before fetching. Another: fetch the matching review ref into a temporary namespace, check ancestry with `merge-base --is-ancestor`, then delete the temporary ref. Return False (not found) when no ref of the repo contains the SHA. Add a regression test for an unreferenced or fork-only SHA.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. SHA-256 assessment baselines are rejected ✓ Resolved
Description
GitCheckout.reachable now accepts 64-character commit IDs, but DiffRequest.since still validates
explicit baseline commits against a pattern capped at 40 characters. When an assessment names a
SHA-256 baseline for a local checkout, model validation rejects the request before
AssessmentService.prepare can pass that commit to the Git reader.
Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Explicit SHA-256 assessment baselines fail validation even though the checkout Git reader now accepts them.

## Fix Focus Areas
- src/traust_core/v1/models/assessment.py[614-621]
- src/traust_core/v1/providers/checkout.py[14-14]

## Recommended Fix
Extend `DiffRequest.since` validation to accept 64-character lowercase commit IDs while retaining its existing SHA-1 and short-SHA behavior. Add regression coverage that supplies a SHA-256 `since` value through the assessment request.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



@smith-xyz

Copy link
Copy Markdown
Contributor

@qodo here is the #327's description:

Fix-forward for the two findings in Michele's review of traust#336 (chubirka, CHANGES_REQUESTED) — both independently verified before fixing.

1. Git transport auth (her P1). Verified read-only against a real GitHub upload-pack advertisement with a valid token: Authorization: Bearer → 401 invalid credentials (even for public repos); Authorization: Basic x-access-token:<token> → 200. The merged ForgeApi.git_headers sent Bearer for GitHub hosts (the earlier Qodo fix covered GitLab's PRIVATE-TOKEN case only), so every authenticated deployment's mirror fetch failed. git_headers now sends Basic for every forge kind: x-access-token for GitHub kinds, oauth2 for GitLab.

2. A checkout names its origin (her P2). Verified: mirror worktrees carried no remote.origin (the bare mirror is fetched by explicit URL), while session run --checkout dispatches only the workspace path and vuln-scan/threat-model derive repository identity from git remote get-url origin in the checkout — a regression from the clone-based flow, which had origin. The mirror now records remote.origin.url, which every worktree shares, restoring the identity contract for sessions, skills and hand runs.

Changed tests

  • tests/v1/providers/test_forge_api.py::test_git_transport_auth… · old: github expected Bearer · new: expects Basic x-access-token · why: Bearer is refused by git smart-HTTP
  • tests/v1/providers/test_mirror.py token test · old: raw token asserted in the env value · new: asserts the exact Basic header · why: the token now rides base64-encoded
  • new: test_a_checkout_names_its_origin_the_skills_derive_identity_from
  • A person reviewed each changed test and the behaviour change is intended

@qodo-for-traust-security

Copy link
Copy Markdown

here is the #327's description:

Fix-forward for the two findings in Michele's review of traust#336 (chubirka, CHANGES_REQUESTED) — both independently verified before fixing.

1. Git transport auth (her P1). Verified read-only against a real GitHub upload-pack advertisement with a valid token: Authorization: Bearer → 401 invalid credentials (even for public repos); Authorization: Basic x-access-token:<token> → 200. The merged ForgeApi.git_headers sent Bearer for GitHub hosts (the earlier Qodo fix covered GitLab's PRIVATE-TOKEN case only), so every authenticated deployment's mirror fetch failed. git_headers now sends Basic for every forge kind: x-access-token for GitHub kinds, oauth2 for GitLab.

2. A checkout names its origin (her P2). Verified: mirror worktrees carried no remote.origin (the bare mirror is fetched by explicit URL), while session run --checkout dispatches only the workspace path and vuln-scan/threat-model derive repository identity from git remote get-url origin in the checkout — a regression from the clone-based flow, which had origin. The mirror now records remote.origin.url, which every worktree shares, restoring the identity contract for sessions, skills and hand runs.

Changed tests

  • tests/v1/providers/test_forge_api.py::test_git_transport_auth… · old: github expected Bearer · new: expects Basic x-access-token · why: Bearer is refused by git smart-HTTP
  • tests/v1/providers/test_mirror.py token test · old: raw token asserted in the env value · new: asserts the exact Basic header · why: the token now rides base64-encoded
  • new: test_a_checkout_names_its_origin_the_skills_derive_identity_from
  • A person reviewed each changed test and the behaviour change is intended

Thanks—this provides context for the earlier transport-auth and checkout-origin fixes in #327. It does not correspond to the two findings in the current review:

  • finding 1: SHA-256 assessment baselines are still rejected by DiffRequest.since validation.
  • finding 2: mirror fallback fetching can accept a commit not reachable from the requested repository’s own refs.

The authentication change appears intended and is covered by the new Git smart-HTTP test; the origin-restoration change is separate from these active findings.

@smith-xyz

Copy link
Copy Markdown
Contributor

@qodo I am saying that PR #327 seems to add the same changes that this PR adds relative to github basic auth

@qodo-for-traust-security

Copy link
Copy Markdown

I am saying that PR #327 seems to add the same changes that this PR adds relative to github basic auth

Yes—the current PR changes GitHub/GitHub Enterprise Git transport authentication from Bearer <token> to HTTP Basic using x-access-token:<token> in src/traust_core/v1/providers/forge_api.py, with corresponding tests. If PR #327 already introduces that exact change, this portion is overlapping/duplicated and should be coordinated or removed from one PR. I can’t inspect PR #327’s diff from the available PR context, so I can’t determine which PR should retain it.

@smith-xyz

Copy link
Copy Markdown
Contributor

Cross-checked against #327 rather than relying on Qodo’s comparison: both change ForgeApi.git_headers to HTTP Basic (x-access-token for GitHub/GHE, oauth2 for GitLab), so that hunk is duplicate. #327 uniquely sets remote.origin.url on the mirror, required by session/skill identity; #326 uniquely adds PR-only commit fetch, eviction/acquisition locking, and SHA-256 acceptance. The mirror _fetch rewrites overlap and will need integration without dropping origin. I have not validated Qodo’s new #326 concern about accepting an unreferenced/fork-only SHA; please resolve that trust boundary before merging the fetch expansion. Neither PR is merged or approved yet.

@smith-xyz
smith-xyz self-requested a review as a code owner October 31, 2026 15:56
@smith-xyz

Copy link
Copy Markdown
Contributor

Addressed both Qodo findings in aa323c8. Mirror checkout now accepts a commit only when reachable from current repository heads/tags or equal to an advertised pull/merge-request head; it fetches that named review ref, never a bare SHA. Previously fetched objects and deleted review refs no longer grant authorization. The new regressions failed before the fix and pass now. DiffRequest also accepts a 64-character SHA-256 since (while retaining 7–40-character SHA-1 inputs). Local make lint-fix, make test (8,398 passed), and pre-push coverage (96.70%) passed. This changes checkout trust behavior and test fixtures; please review the new security boundary before merging.

@smith-xyz

Copy link
Copy Markdown
Contributor

Follow-up for Qodo’s new older-review-commit finding: c9badc8 resolves the SHA to PR/MR IDs through the target repository’s forge API, checks each candidate against currently advertised review refs, fetches only the named ref and proves the requested SHA is an ancestor of its fetched tip. A stale cached commit, unrelated fork commit or deleted/force-pushed ref cannot authorize checkout. The new old-commit regression failed on aa323c8 and passes now; GitHub’s commit-to-PR endpoint was also checked against an actual older core#322 commit. Local gates: 8,403 tests and 96.69% pre-push coverage. Michele’s review and fresh CI remain required.

@smith-xyz
smith-xyz merged commit 27b2861 into development Oct 31, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants