diff --git a/.snyk b/.snyk index c481b7339..212a26fe5 100644 --- a/.snyk +++ b/.snyk @@ -13,3 +13,7 @@ exclude: global: - internal/azure/azure_apps.go - cmd/kosli/root.go + # False positive: Snyk Code flags the GitHub username in this test fixture + # (a graphql `Login` field) as a hardcoded credential. It is a public + # identifier in test data, not a secret. See kosli-dev/server#5479. + - internal/github/build_pr_evidence_test.go diff --git a/internal/azure/azure.go b/internal/azure/azure.go index 6f6628484..b83cf56ab 100644 --- a/internal/azure/azure.go +++ b/internal/azure/azure.go @@ -167,20 +167,24 @@ func (c *AzureConfig) GetPullRequestCommits(pr git.GitPullRequest) ([]types.Comm } for _, commit := range prCommitsResponse.Value { - commits = append(commits, types.Commit{ - SHA: *commit.CommitId, - Message: *commit.Comment, - Committer: *commit.Author.Name, - Timestamp: commit.Committer.Date.Time.Unix(), - URL: *commit.Url, - Branch: *pr.SourceRefName, - CommitterUsername: *commit.Committer.Name, - }) + commits = append(commits, commitFromAzureCommit(commit, *pr.SourceRefName)) } return commits, nil } +// commitFromAzureCommit maps an Azure DevOps API commit to a types.Commit. +func commitFromAzureCommit(commit git.GitCommitRef, branch string) types.Commit { + return types.Commit{ + SHA: *commit.CommitId, + Message: *commit.Comment, + Author: fmt.Sprintf("%s <%s>", *commit.Author.Name, *commit.Author.Email), + Timestamp: commit.Author.Date.Time.Unix(), + URL: *commit.Url, + Branch: branch, + } +} + // PullRequestsForCommit returns a list of pull requests for a specific commit func (c *AzureConfig) PullRequestsForCommit(commit string) ([]git.GitPullRequest, error) { ctx := context.Background() diff --git a/internal/azure/commit_mapping_test.go b/internal/azure/commit_mapping_test.go new file mode 100644 index 000000000..5343334cd --- /dev/null +++ b/internal/azure/commit_mapping_test.go @@ -0,0 +1,54 @@ +package azure + +import ( + "testing" + "time" + + "github.com/microsoft/azure-devops-go-api/azuredevops" + "github.com/microsoft/azure-devops-go-api/azuredevops/git" + "github.com/stretchr/testify/require" +) + +func azStr(s string) *string { return &s } + +func azTestCommit() git.GitCommitRef { + authorDate := azuredevops.Time{Time: time.Unix(1772630000, 0)} + committerDate := azuredevops.Time{Time: time.Unix(1772635812, 0)} + return git.GitCommitRef{ + CommitId: azStr("abc1230000000000000000000000000000000000"), + Comment: azStr("Apply suggestions from code review"), + Url: azStr("https://dev.azure.com/kosli/_git/x/commit/abc123"), + Author: &git.GitUserDate{ + Name: azStr("Steve Tooke"), + Email: azStr("tooky@kosli.com"), + Date: &authorDate, + }, + Committer: &git.GitUserDate{ + Name: azStr("GitHub"), + Email: azStr("noreply@github.com"), + Date: &committerDate, + }, + } +} + +// TestCommitFromAzureCommit_RecordsAuthorIdentity is a regression test for +// server#5479. Azure recorded the author display name only, and populated +// author_username from the committer. Record the author as "Name ", +// and drop author_username (Azure commits carry no login). +func TestCommitFromAzureCommit_RecordsAuthorIdentity(t *testing.T) { + c := commitFromAzureCommit(azTestCommit(), "my-branch") + + require.Equal(t, "Steve Tooke ", c.Author, + "author must be name of the git author") + require.Empty(t, c.AuthorUsername, + "Azure commits carry no login; author_username must be omitted, not the committer name") +} + +// TestCommitFromAzureCommit_UsesAuthorDate is a regression test for server#5479: +// the timestamp must match the recorded author identity. +func TestCommitFromAzureCommit_UsesAuthorDate(t *testing.T) { + c := commitFromAzureCommit(azTestCommit(), "my-branch") + + require.Equal(t, int64(1772630000), c.Timestamp, + "timestamp must be the author date, not the committer date") +} diff --git a/internal/bitbucket/bitbucket.go b/internal/bitbucket/bitbucket.go index 40cb9cc7a..e535d0010 100644 --- a/internal/bitbucket/bitbucket.go +++ b/internal/bitbucket/bitbucket.go @@ -243,7 +243,7 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e allCommits = append(allCommits, types.Commit{ SHA: commit["hash"].(string), Message: commit["message"].(string), - Committer: commit["author"].(map[string]any)["raw"].(string), + Author: commit["author"].(map[string]any)["raw"].(string), Timestamp: timestamp, URL: commit["links"].(map[string]any)["html"].(map[string]any)["href"].(string), }) diff --git a/internal/github/build_pr_evidence_test.go b/internal/github/build_pr_evidence_test.go new file mode 100644 index 000000000..b15dd7f63 --- /dev/null +++ b/internal/github/build_pr_evidence_test.go @@ -0,0 +1,107 @@ +package github + +import ( + "testing" + "time" + + "github.com/shurcooL/graphql" + "github.com/stretchr/testify/require" +) + +// TestBuildPREvidence_RecordsAuthorNotCommitter is a regression test for +// server#5479. PR commit attestations were recording the git committer in the +// "author" field. For GitHub web-flow commits (applied suggestions, bot +// commits) the committer is "GitHub ", distinct from the +// real author — so the true author was being lost and the author_username +// dropped entirely (the committer has no associated GitHub user). +func TestBuildPREvidence_RecordsAuthorNotCommitter(t *testing.T) { + node := graphqlCommitNode{} + node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" + node.Commit.MessageHeadline = "Apply suggestions from code review" + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.URL = "https://github.com/kosli-dev/cli/commit/0e723254516c841126e81f76100be57258ff1386" + + // Author is the real person who wrote the change. The query no longer + // fetches the committer at all (it would be GitHub's web-flow identity for + // applied-suggestion / bot commits), so only the author is recorded. + node.Commit.Author.Name = "Steve Tooke" + node.Commit.Author.Email = "tooky@kosli.com" + node.Commit.Author.User = &struct { + Login graphql.String + }{Login: "tooky"} + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", + "tooky", + "2026-03-01T11:00:00Z", + "", + "Introduce kosli evaluate", + "introduce-kosli-evaluate", + []graphqlCommitNode{node}, + nil, + ) + require.NoError(t, err) + require.Len(t, evidence.Commits, 1) + + c := evidence.Commits[0] + require.Equal(t, "Steve Tooke ", c.Author, + "the commit author (wire field author) must be the git author, not the committer") + require.Equal(t, "tooky", c.AuthorUsername, + "author_username must be the author's GitHub login, not the committer's (absent) one") +} + +// TestBuildPREvidence_UsesAuthoredDate is a regression test for server#5479. +// Now that the recorded identity is the author, the timestamp should be the +// author date too. For rebased / applied-suggestion commits the authored and +// committed dates differ. +func TestBuildPREvidence_UsesAuthoredDate(t *testing.T) { + node := graphqlCommitNode{} + node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" + node.Commit.MessageHeadline = "Apply suggestions from code review" + node.Commit.AuthoredDate = "2026-03-01T10:00:00Z" + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.Author.Name = "Steve Tooke" + node.Commit.Author.Email = "tooky@kosli.com" + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "Introduce kosli evaluate", "introduce-kosli-evaluate", + []graphqlCommitNode{node}, nil, + ) + require.NoError(t, err) + require.Len(t, evidence.Commits, 1) + + wantAuthored, _ := time.Parse(time.RFC3339, "2026-03-01T10:00:00Z") + require.Equal(t, wantAuthored.Unix(), evidence.Commits[0].Timestamp, + "timestamp must be the author date, not the committer date") +} + +// TestBuildPREvidence_FallsBackToCommittedDate ensures the timestamp falls back +// to the committed date when the GraphQL response omits the authored date. +func TestBuildPREvidence_FallsBackToCommittedDate(t *testing.T) { + node := graphqlCommitNode{} + node.Commit.Oid = "0e723254516c841126e81f76100be57258ff1386" + node.Commit.MessageHeadline = "msg" + node.Commit.AuthoredDate = "" + node.Commit.CommittedDate = "2026-03-01T12:00:00Z" + node.Commit.Author.Name = "Steve Tooke" + node.Commit.Author.Email = "tooky@kosli.com" + + evidence, err := buildPREvidence( + "https://github.com/kosli-dev/cli/pull/671", + "0e723254516c841126e81f76100be57258ff1386", + "MERGED", "tooky", "2026-03-01T09:00:00Z", "", + "title", "branch", + []graphqlCommitNode{node}, nil, + ) + require.NoError(t, err) + require.Len(t, evidence.Commits, 1) + + wantCommitted, _ := time.Parse(time.RFC3339, "2026-03-01T12:00:00Z") + require.Equal(t, wantCommitted.Unix(), evidence.Commits[0].Timestamp, + "timestamp must fall back to the committed date when authored date is absent") +} diff --git a/internal/github/github.go b/internal/github/github.go index b9f295420..973b24a41 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -263,11 +263,11 @@ type graphqlCommitNode struct { Oid graphql.String MessageHeadline graphql.String CommittedDate graphql.String + AuthoredDate graphql.String URL graphql.String - Committer struct { + Author struct { Name graphql.String Email graphql.String - Date graphql.String User *struct { Login graphql.String } @@ -319,22 +319,28 @@ func buildPREvidence( } for _, n := range commitNodes { - timestamp, err := time.Parse(time.RFC3339, string(n.Commit.CommittedDate)) + // Use the author date to match the recorded author identity; fall back to + // the committed date if the API omits it (server#5479). + dateStr := string(n.Commit.AuthoredDate) + if dateStr == "" { + dateStr = string(n.Commit.CommittedDate) + } + timestamp, err := time.Parse(time.RFC3339, dateStr) if err != nil { return nil, err } - committerUsername := "" - if n.Commit.Committer.User != nil { - committerUsername = string(n.Commit.Committer.User.Login) + authorUsername := "" + if n.Commit.Author.User != nil { + authorUsername = string(n.Commit.Author.User.Login) } evidence.Commits = append(evidence.Commits, types.Commit{ - SHA: string(n.Commit.Oid), - Message: string(n.Commit.MessageHeadline), - Committer: fmt.Sprintf("%s <%s>", string(n.Commit.Committer.Name), string(n.Commit.Committer.Email)), - CommitterUsername: committerUsername, - Timestamp: timestamp.Unix(), - Branch: headRef, - URL: string(n.Commit.URL), + SHA: string(n.Commit.Oid), + Message: string(n.Commit.MessageHeadline), + Author: fmt.Sprintf("%s <%s>", string(n.Commit.Author.Name), string(n.Commit.Author.Email)), + AuthorUsername: authorUsername, + Timestamp: timestamp.Unix(), + Branch: headRef, + URL: string(n.Commit.URL), }) } diff --git a/internal/gitlab/commit_mapping_test.go b/internal/gitlab/commit_mapping_test.go new file mode 100644 index 000000000..15554426a --- /dev/null +++ b/internal/gitlab/commit_mapping_test.go @@ -0,0 +1,72 @@ +package gitlab + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + gitlab "gitlab.com/gitlab-org/api/client-go" +) + +// TestCommitFromGitlabCommit_RecordsAuthorNotCommitter is a regression test for +// server#5479. PR commit attestations were recording the git committer in the +// author field. They must record the git author. +func TestCommitFromGitlabCommit_RecordsAuthorNotCommitter(t *testing.T) { + created := time.Unix(1772635812, 0) + commit := &gitlab.Commit{ + ID: "abc1230000000000000000000000000000000000", + Message: "Apply suggestions from code review", + AuthorName: "Steve Tooke", + AuthorEmail: "tooky@kosli.com", + CommitterName: "GitHub", + CommitterEmail: "noreply@github.com", + CreatedAt: &created, + WebURL: "https://gitlab.com/kosli/x/-/commit/abc123", + } + + c := commitFromGitlabCommit(commit, "my-branch") + + require.Equal(t, "Steve Tooke ", c.Author, + "author must be the git author, not the committer") +} + +// TestCommitFromGitlabCommit_UsesAuthoredDate is a regression test for +// server#5479: the timestamp must match the recorded author identity. +func TestCommitFromGitlabCommit_UsesAuthoredDate(t *testing.T) { + authored := time.Unix(1772630000, 0) + created := time.Unix(1772635812, 0) + commit := &gitlab.Commit{ + ID: "abc1230000000000000000000000000000000000", + Message: "msg", + AuthorName: "Steve Tooke", + AuthorEmail: "tooky@kosli.com", + AuthoredDate: &authored, + CreatedAt: &created, + WebURL: "https://gitlab.com/kosli/x/-/commit/abc123", + } + + c := commitFromGitlabCommit(commit, "my-branch") + + require.Equal(t, int64(1772630000), c.Timestamp, + "timestamp must be the authored date, not created_at") +} + +// TestCommitFromGitlabCommit_FallsBackToCreatedAt ensures the timestamp falls +// back to created_at when the API omits the authored date. +func TestCommitFromGitlabCommit_FallsBackToCreatedAt(t *testing.T) { + created := time.Unix(1772635812, 0) + commit := &gitlab.Commit{ + ID: "abc1230000000000000000000000000000000000", + Message: "msg", + AuthorName: "Steve Tooke", + AuthorEmail: "tooky@kosli.com", + AuthoredDate: nil, + CreatedAt: &created, + WebURL: "https://gitlab.com/kosli/x/-/commit/abc123", + } + + c := commitFromGitlabCommit(commit, "my-branch") + + require.Equal(t, int64(1772635812), c.Timestamp, + "timestamp must fall back to created_at when authored date is absent") +} diff --git a/internal/gitlab/gitlab.go b/internal/gitlab/gitlab.go index a58226763..bb901ac67 100644 --- a/internal/gitlab/gitlab.go +++ b/internal/gitlab/gitlab.go @@ -174,14 +174,25 @@ func (c *GitlabConfig) GetMergeRequestCommits(mr *gitlab.BasicMergeRequest) ([]t return commits, err } for _, commit := range glCommits { - commits = append(commits, types.Commit{ - SHA: commit.ID, - Message: commit.Message, - Committer: fmt.Sprintf("%s <%s>", commit.CommitterName, commit.CommitterEmail), - Timestamp: commit.CreatedAt.Unix(), - Branch: mr.SourceBranch, - URL: commit.WebURL, - }) + commits = append(commits, commitFromGitlabCommit(commit, mr.SourceBranch)) } return commits, nil } + +// commitFromGitlabCommit maps a GitLab API commit to a types.Commit. +func commitFromGitlabCommit(commit *gitlab.Commit, branch string) types.Commit { + // Use the authored date to match the recorded author identity; fall back to + // created_at if the API omits it (server#5479). + timestamp := commit.CreatedAt.Unix() + if commit.AuthoredDate != nil { + timestamp = commit.AuthoredDate.Unix() + } + return types.Commit{ + SHA: commit.ID, + Message: commit.Message, + Author: fmt.Sprintf("%s <%s>", commit.AuthorName, commit.AuthorEmail), + Timestamp: timestamp, + Branch: branch, + URL: commit.WebURL, + } +} diff --git a/internal/types/types.go b/internal/types/types.go index cc84f2173..84d42f9f3 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -20,13 +20,13 @@ type PRApprovals struct { } type Commit struct { - SHA string `json:"sha1"` - Message string `json:"message"` - Committer string `json:"author"` - CommitterUsername string `json:"author_username,omitempty"` - Timestamp int64 `json:"timestamp"` - Branch string `json:"branch"` - URL string `json:"url,omitempty"` + SHA string `json:"sha1"` + Message string `json:"message"` + Author string `json:"author"` + AuthorUsername string `json:"author_username,omitempty"` + Timestamp int64 `json:"timestamp"` + Branch string `json:"branch"` + URL string `json:"url,omitempty"` } type PRRetriever interface {