Fix nil pointer panics in GitHub analyzer gist/repo binding functions (#4864)
* handle gist pointers in github analyzer * address bugbot comments; handle repo without owner * addressed feedback: keep repo binding with nil Parent when Owner is absent
This commit is contained in:
@@ -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...)...)
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user