Repository navigation
fix(bitbucket): never crash on a null field in a Bitbucket reply - #1286
AlexKantor87 wants to merge 10 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.
Every field was read with an unchecked type assertion, so a null or missing value (an approver with no state, a PR with no author) panicked with a Go stack trace. Fields are now read through one set of safe getters that give the zero value, and empty timestamps parse to 0. A null item in a list we read (values, participants) is an error naming the list and index, not a silent skip. The only null fields that still fail are the PR's links.self.href and id, which the next request needs. The test nulls each value of realistic replies in turn through both PREvidenceForCommitV1 (assert) and V2 (attest) and asserts the rule. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Claude finished @AlexKantor87's task in 1m 11s —— View job Follow-up review complete
Summary: The new commits ( None of the three previous findings' locations were touched by the new commits:
No new bugs or regressions found in the changed lines. Full details posted as a PR comment. |
![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.
… advance An approved participant with no user or display_name was counted as an approval by nobody, which records approval evidence that fails open. It is now an entry we can't read and errors, naming participants[i], for both assert (V1) and attest (V2). Commit paging followed next even when it pointed at the page just fetched, so such a reply looped forever. It now errors instead, matching the gitlab and github "did not advance" guards. 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.
…in reviews The previous commit made an approval with no user or display_name an error. A deleted account is a legal reply with no identity, so any pull request it ever approved could never be attested again, with no workaround. Now an approval with no identity is left out of the approvers, so it never counts as an approval, and is kept in the reviews with an empty name and its state, so the record still shows an unknown account reviewed. One helper, approvedBy, decides it for V1 and V2. Part of #1283 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The earlier guard only stopped a next link that was the page just fetched. Two pages pointing at each other still looped forever, and the earlier commit was wrong to say it matched GitLab: GitLab also caps the number of pages. Bitbucket's next is an opaque URL, so there is no page number to check for going up; a cap is the guard that ends every cycle. Commit paging now stops with an error after defaultMaxPages (1000) pages. The same-page check is dropped: the cap covers it. 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-bitbucket # Conflicts: # .github/dependabot.yml
Follow-up review: BEFORE
|
Part of #1283. Builds on #1285 (test helpers), so this PR's base is that branch. It moves to
mainonce #1285 merges.kosli attest pullrequest bitbucketandkosli assert pullrequest bitbucketcrashed when Bitbucket sent a field as null where the code expected a value. A crash prints a Go stack trace that doesn't say which field caused it. For now we treat every field as optional (#1284 will decide which fields are required).What changed
str,obj,booleanandobjectsread a field and give an empty value when it's missing, null or a different type. They replace every unchecked read inbitbucket.go, on both the V1 path (assert) and the V2 path (attest).valuesorparticipantsreturns an error that names the list and index. Skipping it could hide a "changes requested" review.links.self.href(the next request's URL) or noid(needed to fetch commits) now returns a readable error.Tests
odd_replies_test.gosends realistic replies through V1 and V2 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, 62 cases crashed. Now all 340 pass.Not changed
nextlink points back to the same page.Full CLI suite against a local server: 3,989 tests passed.
🤖 Generated with Claude Code