diff --git a/pkg/analyzer/analyzers/github/github.go b/pkg/analyzer/analyzers/github/github.go index 8d3e34a91..360123871 100644 --- a/pkg/analyzer/analyzers/github/github.go +++ b/pkg/analyzer/analyzers/github/github.go @@ -83,11 +83,11 @@ func secretInfoToUserBindings(info *common.SecretInfo) []analyzers.Binding { } func userToResource(user *gh.User) *analyzers.Resource { - name := *user.Login + name := user.GetLogin() return &analyzers.Resource{ Name: name, FullyQualifiedName: fmt.Sprintf("github.com/%s", name), - Type: strings.ToLower(*user.Type), // "user" or "organization" + Type: strings.ToLower(user.GetType()), // "user" or "organization" } } @@ -114,11 +114,17 @@ func secretInfoToRepoBindings(info *common.SecretInfo) []analyzers.Binding { } var bindings []analyzers.Binding for _, repo := range repos { + // A repo has identity independent of its owner (name/full_name); if the + // owner is absent we still emit the repo but leave Parent unset. + var parent *analyzers.Resource + if owner := repo.GetOwner(); owner != nil { + parent = userToResource(owner) + } resource := analyzers.Resource{ - Name: *repo.Name, - FullyQualifiedName: fmt.Sprintf("github.com/%s", *repo.FullName), + Name: repo.GetName(), + FullyQualifiedName: fmt.Sprintf("github.com/%s", repo.GetFullName()), Type: "repository", - Parent: userToResource(repo.Owner), + Parent: parent, } bindings = append(bindings, analyzers.BindAllPermissions(resource, perms...)...) } @@ -128,11 +134,17 @@ func secretInfoToRepoBindings(info *common.SecretInfo) []analyzers.Binding { func secretInfoToGistBindings(info *common.SecretInfo) []analyzers.Binding { var bindings []analyzers.Binding for _, gist := range info.Gists { + // A gist without an owner cannot be attributed to a user, so we cannot + // build a meaningful resource or FQN for it. Skip it rather than + // producing silently corrupt output (e.g. "gist.github.com//id"). + if gist.GetOwner() == nil { + continue + } resource := analyzers.Resource{ - Name: *gist.Description, - FullyQualifiedName: fmt.Sprintf("gist.github.com/%s/%s", *gist.Owner.Login, *gist.ID), + Name: gist.GetDescription(), + FullyQualifiedName: fmt.Sprintf("gist.github.com/%s/%s", gist.GetOwner().GetLogin(), gist.GetID()), Type: "gist", - Parent: userToResource(gist.Owner), + Parent: userToResource(gist.GetOwner()), } bindings = append(bindings, analyzers.BindAllPermissions(resource, info.Metadata.OauthScopes...)...) } diff --git a/pkg/analyzer/analyzers/github/github_test.go b/pkg/analyzer/analyzers/github/github_test.go index a3b9a9ee2..6ac3d3b65 100644 --- a/pkg/analyzer/analyzers/github/github_test.go +++ b/pkg/analyzer/analyzers/github/github_test.go @@ -6,11 +6,173 @@ import ( "time" "github.com/google/go-cmp/cmp" + gh "github.com/google/go-github/v67/github" "github.com/trufflesecurity/trufflehog/v3/pkg/analyzer/analyzers" + githubcommon "github.com/trufflesecurity/trufflehog/v3/pkg/analyzer/analyzers/github/common" "github.com/trufflesecurity/trufflehog/v3/pkg/common" "github.com/trufflesecurity/trufflehog/v3/pkg/context" ) +func strPtr(s string) *string { return &s } + +func makeOwner(login, userType string) *gh.User { + return &gh.User{Login: &login, Type: &userType} +} + +// TestSecretInfoToGistBindings exercises nil-field edge cases that caused a +// production panic (nil Description and nil Owner from the GitHub API). +func TestSecretInfoToGistBindings(t *testing.T) { + owner := makeOwner("truffle-sandbox", "User") + scope := []analyzers.Permission{{Value: "gist"}} + + tests := []struct { + name string + gists []*gh.Gist + wantLen int + wantFQNs []string + wantNames []string // parallel to wantFQNs; checked unconditionally (including empty string) + }{ + { + name: "nil description produces empty name, gist is still included", + gists: []*gh.Gist{ + {ID: strPtr("abc123"), Description: nil, Owner: owner}, + }, + wantLen: 1, + wantFQNs: []string{"gist.github.com/truffle-sandbox/abc123"}, + wantNames: []string{""}, + }, + { + name: "nil owner: gist is skipped entirely", + gists: []*gh.Gist{ + {ID: strPtr("abc123"), Description: strPtr("my gist"), Owner: nil}, + }, + wantLen: 0, + }, + { + name: "mix of valid and nil-owner gists: only valid gist is included", + gists: []*gh.Gist{ + {ID: strPtr("abc123"), Description: strPtr("valid gist"), Owner: owner}, + {ID: strPtr("def456"), Description: strPtr("no owner"), Owner: nil}, + }, + wantLen: 1, + wantFQNs: []string{"gist.github.com/truffle-sandbox/abc123"}, + wantNames: []string{"valid gist"}, + }, + { + name: "empty gist list: no bindings", + gists: nil, + wantLen: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + info := &githubcommon.SecretInfo{ + Metadata: &githubcommon.TokenMetadata{OauthScopes: scope}, + Gists: tt.gists, + } + got := secretInfoToGistBindings(info) + if len(got) != tt.wantLen { + t.Errorf("got %d bindings, want %d", len(got), tt.wantLen) + } + for i, fqn := range tt.wantFQNs { + if i >= len(got) { + break + } + if got[i].Resource.FullyQualifiedName != fqn { + t.Errorf("binding[%d].FullyQualifiedName = %q, want %q", i, got[i].Resource.FullyQualifiedName, fqn) + } + } + for i, name := range tt.wantNames { + if i >= len(got) { + break + } + if got[i].Resource.Name != name { + t.Errorf("binding[%d].Name = %q, want %q", i, got[i].Resource.Name, name) + } + } + }) + } +} + +// TestSecretInfoToRepoBindings verifies that a repo with a nil Owner is still +// emitted (with Parent left unset) rather than skipped, since a repo has +// identity independent of its owner. +func TestSecretInfoToRepoBindings(t *testing.T) { + owner := makeOwner("truffle-sandbox", "User") + scope := []analyzers.Permission{{Value: "repo"}} + + tests := []struct { + name string + repos []*gh.Repository + wantLen int + wantFQNs []string + wantParentNil []bool // parallel to wantFQNs + }{ + { + name: "valid repo produces binding with populated parent", + repos: []*gh.Repository{ + {Name: strPtr("my-repo"), FullName: strPtr("truffle-sandbox/my-repo"), Owner: owner}, + }, + wantLen: 1, + wantFQNs: []string{"github.com/truffle-sandbox/my-repo"}, + wantParentNil: []bool{false}, + }, + { + name: "nil owner: repo is still included with nil parent", + repos: []*gh.Repository{ + {Name: strPtr("my-repo"), FullName: strPtr("truffle-sandbox/my-repo"), Owner: nil}, + }, + wantLen: 1, + wantFQNs: []string{"github.com/truffle-sandbox/my-repo"}, + wantParentNil: []bool{true}, + }, + { + name: "mix of valid and nil-owner repos: both included, parent populated only for valid", + repos: []*gh.Repository{ + {Name: strPtr("my-repo"), FullName: strPtr("truffle-sandbox/my-repo"), Owner: owner}, + {Name: strPtr("other"), FullName: strPtr("truffle-sandbox/other"), Owner: nil}, + }, + wantLen: 2, + wantFQNs: []string{"github.com/truffle-sandbox/my-repo", "github.com/truffle-sandbox/other"}, + wantParentNil: []bool{false, true}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + info := &githubcommon.SecretInfo{ + Metadata: &githubcommon.TokenMetadata{ + Type: githubcommon.TokenTypeClassicPAT, + OauthScopes: scope, + }, + Repos: tt.repos, + } + got := secretInfoToRepoBindings(info) + if len(got) != tt.wantLen { + t.Errorf("got %d bindings, want %d", len(got), tt.wantLen) + } + for i, fqn := range tt.wantFQNs { + if i >= len(got) { + break + } + if got[i].Resource.FullyQualifiedName != fqn { + t.Errorf("binding[%d].FullyQualifiedName = %q, want %q", i, got[i].Resource.FullyQualifiedName, fqn) + } + } + for i, wantNil := range tt.wantParentNil { + if i >= len(got) { + break + } + gotNil := got[i].Resource.Parent == nil + if gotNil != wantNil { + t.Errorf("binding[%d].Parent nil = %v, want %v", i, gotNil, wantNil) + } + } + }) + } +} + func TestAnalyzer_Analyze(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) defer cancel()