Support SHA-256 Git object hashes (64-char OIDs)#3872
Conversation
Agent-Logs-Url: https://github.com/github/codeql-action/sessions/e39d1fb6-4ce3-47c3-9113-e41b111fc8fb Co-authored-by: oscarsj <1410188+oscarsj@users.noreply.github.com>
| // 100644 6b792ea543ce75d7a8a03df591e3c85311ecb64f 0\tsrc/git-utils.ts | ||
| // The fields are: <mode> <oid> <stage>\t<path> | ||
| const regex = /^[0-9]+ ([0-9a-f]{40}) [0-9]+\t(.+)$/; | ||
| const regex = /^[0-9]+ ([0-9a-f]{40,64}) [0-9]+\t(.+)$/; |
There was a problem hiding this comment.
I think we usually prefer a regex that only allows 40 or 64 character SHAs, something like this:
| const regex = /^[0-9]+ ([0-9a-f]{40,64}) [0-9]+\t(.+)$/; | |
| const regex = /^[0-9]+ ([0-9a-f]{40}(?:[a-f0-9]{24})?) [0-9]+\t(.+)$/; |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
A few minor comments, mainly about some of the added tests that cover functions which don't perform any validation and are unchanged in this PR. I am not greatly opposed to adding these tests, since they could protect against accidentally breaking this in the future, but I want to be sure that this is an intentional goal here and not just Copilot adding tests it doesn't understand.
If we do want these tests, then it might make sense to just reuse the existing test implementations and put them in a loop rather than duplicating the implementations.
| } as const satisfies Record<string, string>; | ||
|
|
||
| /** A 64-character SHA-256 Git OID for use in SHA-256 repository test scenarios. */ | ||
| export const SHA256_GITHUB_SHA = "0".repeat(64); |
There was a problem hiding this comment.
Minor: This is only used in upload-lib.test.ts so I don't think it needs to be in testing-utils.ts.
Additionally, the name is misleading. There's nothing GitHub-specific about a SHA256 hash.
| `${__dirname}/../src/testdata/pull_request.json`; | ||
|
|
||
| const sha256MergeBase = "b".repeat(64); | ||
| const prMergePayload: any = uploadLib.buildPayload( |
There was a problem hiding this comment.
The addition of this test doesn't make sense: buildPayload doesn't perform any validation. Indeed, this test passes without any of the changes to git-utils.ts.
| const callback = sinon.stub(gitUtils, "getCommitOid"); | ||
| callback.withArgs("HEAD").resolves(currentSha); | ||
|
|
||
| const actualRef = await gitUtils.getRef(); |
There was a problem hiding this comment.
getRef() doesn't perform any validation, so the four new tests for getRef() here pass without the changes in this PR.
Git is transitioning to SHA-256 as the default object hash (64 hex chars vs SHA-1's 40). Two production code paths hard-coded assumptions about 40-char OIDs, causing silent failures or errors in SHA-256 repositories.
Production fixes (
src/git-utils.ts)getFileOidsUnderPath(): Regex{40}→{40,64}— previously all entries failed to match in SHA-256 repos, causing an error throw.determineBaseBranchHeadCommitOid(): Length checks=== 40→(=== 40 || === 64)— previously the condition was always false for 64-char OIDs, causing silentundefinedreturn and fallback to server-side base SHA calculation.Test coverage (
src/git-utils.test.ts,src/upload-lib.test.ts,src/testing-utils.ts)SHA256_GITHUB_SHA = "0".repeat(64)constant totesting-utils.tsgetRef()tests (merge PR ref, checkout@v1 path, head PR ref, explicit input)determineBaseBranchHeadCommitOidreturning the correct base OID from a 64-char merge commitgetFileOidsUnderPath(pure 64-char OIDs and mixed SHA-1/SHA-256)upload-lib.test.tsRisk assessment
Which use cases does this change impact?
dynamicworkflows (Default Setup, Code Quality, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.upload-sarifaction.Environments:
github.comand/or GitHub Enterprise Cloud with Data Residency.How did/will you validate this change?
.test.tsfiles).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist