Repository navigation
fix(gitlab): never crash on a null field in a GitLab reply - #1287
AlexKantor87 wants to merge 12 commits into
Conversation
Part of #1283. Provider tests use these to send their evidence code a realistic reply once per field with that field made null, or a list emptied, and to name the outcome: ok, error or panic, plus which evidence fields came back changed or missing.
The evidence code read times, users and list entries from GitLab replies without nil checks, so a null merged_at (an MR that has not merged) panicked assert pullrequest gitlab and attest pullrequest gitlab. Null times now give 0 and null users an empty name: that part of the evidence is empty and the rest is kept. A null entry in a list (MRs, approved_by, reviewers, commits) is an error naming the list and index, because it is missing data, not an empty entry. A null approvals or signature reply reads as none. The new test feeds both command entry points a realistic reply with each value made null, or each list emptied, and asserts that rule. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A null signature reply now reads as unsigned, like a 404, so the comment saying only a 404 does was wrong. Drops a comment that restated its function name. Part of #1283.
|
Claude finished @AlexKantor87's task in 3m 10s —— View job Follow-up review complete
Summary comment posted via |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
# Conflicts: # internal/testHelpers/oddReplies.go # internal/testHelpers/oddReplies_test.go
Snyk fails every branch on CVE-2026-111943, a high-severity flaw in Go's crypto/x509 fixed in 1.26.10; go.mod declared 1.26.7. The Dockerfile built from golang:1.26-alpine3.23, so each build could pull a different toolchain. It now names golang:1.26.10-alpine3.23 by digest. Dependabot can't update a FROM built from ARGs, so the line is literal, and a docker ecosystem entry (golang only) keeps the digest current.
An approved_by entry with no user was recorded as an approval by nobody, which a policy counting approvers would accept: that fails open. A reviewer with no user can't be matched to its approval either, and a commit with no id was looked up at commits//signature and recorded as unsigned with no SHA. Each is now an error naming the list entry, the same rule as a null entry, checked in one place (noUnreadableItems). A user without a username counts as no user, since the username is what attributes and matches the entry. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dependabot bumps the pinned golang image on its own, and none of the gomod updates touch go.mod's go directive. Snyk reads go.mod, so an image-only bump would leave the next Go stdlib CVE flagged even though the image is fixed. The test makes that bump red until go.mod moves too.
…error GitLab sends a review whose account was deleted with no user. Erroring on it would fail attestation for every MR ever approved by someone who has since left, with no workaround. Such a review is now kept in Reviews with no name and its state, but never counted as an approver, so approval counting still fails closed. Nameless entries are never matched to each other, so a nameless change request can't hide a nameless approval. A commit with no id still errors. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This branch now includes #1291's branch, so the Go 1.26.10 change clears the Snyk check here. That's why no search: answers no review finding; it brings in #1291's reviewed files unchanged ( |
…283-gitlab # Conflicts: # .github/dependabot.yml
Follow-up review: commits 964a6fc..337d7a4The only change in this range is Status of previous findings (all already resolved by earlier commits 9b4460c / 1002181, which predate this range; GitHub review threads already marked resolved):
No new issues found — the new commits don't touch any of the previously-reviewed code. |
The comment on
|
Policies read Reviews as well as Approvers, so an approval or comment kept there with no name still counted as evidence from nobody: the same fail-open as an anonymous approver. A review with no account is now kept in Reviews only when it requests changes, as dropping that would read as none given. Same rule as Bitbucket's on main. Never an error. The empty-username guards on matching are gone: with this rule nothing an unnamed match could hide is ever kept. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reviewer and approval maps now key every user with no account as "". Say that this is safe only because add drops every nameless entry except a change request, so a later change to add doesn't hide an approval silently. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @mbevc1, you're right on both. My earlier reply was wrong: the rule wasn't the same on all four, and the Bitbucket PR had also dropped 1. Unnamed reviews. Fixed on all four with
The search: grep -nE 'PRApprovals{|Reviews = append|approvers = append|Approvers = ' internal/{github,gitlab,bitbucket,azure}/*.go (non-test) Every hit is either an approver append that needs a name, or a review append that goes through the change-request-only rule. mutation: drop the change-request exception in The other three providers have the same two mutations, and each one fails their tests too. 2. Description. Updated for all four PRs to match what the code does now. That covers the change-request note you mentioned, unnamed approvers being dropped, and the commit |
Follow-up review: commits 337d7a4..c617515The new commits ( Status of the 4 inline findings from the Oct 21 review round (all anchored outside the lines these commits touch, already resolved by earlier commits 9b4460c/1002181a, GitHub threads already marked resolved): no change — none of their locations were touched by this diff.
mbevc1's follow-up finding (nameless No new bugs or regressions found in the changed lines. 0 new inline comments. |
Part of #1283.
kosli attest pullrequest gitlabandkosli assert pullrequest gitlabcrashed when GitLab sent a field as null where the code expected a value. The most common case:merged_atis null on every open merge request, so attesting a commit that belongs to one crashed. For now we treat every field as optional (#1284 will decide which fields are required).What changed
unixOrZero, replaces the direct.Unix()calls on the merge request's created and merged times and the commit'screated_at.reviewswe keep one only if it requests changes, because dropping it would read as no objection. This is the same rule Bitbucket has onmain.noNullItemsreturns an error that names the list and index when merge requests, approvals, reviewers or commits contain a null entry. Skipping it could hide a "changes requested" review.iid(the next request needs it) or a commit with noidreturns a readable error.Tests
odd_replies_test.gosends realistic replies through V2 and Hybrid (whatassertandattestcall) once per field, with that field made null. It checks there's never a crash, and never an error except the two cases above. Before the fix, 32 cases crashed.TestOddReplies_ReviewByNoOnecovers reviews by deleted accounts.Full CLI suite against a local server: 3,934 tests passed.
🤖 Generated with Claude Code