Merge pull request #45 from crashappsec/nettrino/nilfixes

Handle null responses in case of TPS throttling

README.md

@@ -104,7 +104,6 @@ You can see available options via the `--help` flag.
   $GOPATH/bin/github-analyzer \
     --organization crashappsec \
     --token "$GH_SECURITY_AUDITOR_TOKEN" \
-    --enableStats
   ```
 
 ### Running using Docker
@@ -121,7 +120,6 @@ You can see available options via the `--help` flag.
           --organization crashappsec \
           --output output \
           --token "$GH_SECURITY_AUDITOR_TOKEN" \
-          --enableStats
   ```
 
 ## Permissions
@@ -147,7 +145,7 @@ needed. Example usage:
 github-analyzer \
     --organization crashappsec \
     --token "$GH_SECURITY_AUDITOR_TOKEN" \
-    --enableStats \
+    --userPermissionStats \
     --enableScraping \
     --username "$GH_SECURITY_AUDITOR_USERNAME" \
     --password "$GH_SECURITY_AUDITOR_PASSWORD" \

cmd/github-analyzer/main.go

@@ -51,8 +51,7 @@ func runCmd() {
 			config.ViperEnv.Username,
 			config.ViperEnv.Password,
 			config.ViperEnv.OtpSeed,
-			config.ViperEnv.Organization,
-			config.ViperEnv.EnableStats)
+			config.ViperEnv.Organization)
 		if err != nil {
 			log.Logger.Error(err)
 		}
@@ -68,7 +67,7 @@ func runCmd() {
 			log.Logger.Error(err)
 			return
 		}
-		results, execStatus, err := auditor.AuditOrg(config.ViperEnv.Organization, config.ViperEnv.EnableStats)
+		results, execStatus, err := auditor.AuditOrg(config.ViperEnv.Organization, config.ViperEnv.UserPermissionStats)
 		if err != nil {
 			log.Logger.Error(err)
 		}
@@ -172,7 +171,7 @@ func NewRootCommand() *cobra.Command {
 	rootCmd.Flags().
 		BoolVarP(&config.ViperEnv.Version, "version", "", false, "print version and exit")
 	rootCmd.Flags().
-		BoolVarP(&config.ViperEnv.EnableStats, "enableStats", "", false, "enable user permission statistics (might be slow due to throttling limits)")
+		BoolVarP(&config.ViperEnv.UserPermissionStats, "userPermissionStats", "", false, "enable user permission statistics (might be slow in large orgs due to throttling limits)")
 
 	rootCmd.Flags().
 		BoolVarP(&config.ViperEnv.EnableScraping, "enableScraping", "", false, "enable experimental checks that rely on screen scraping")

pkg/config/config.go

@@ -7,18 +7,18 @@ const (
 )
 
 type ViperEnvVars struct {
-	CfgFile        string `mapstructure:"CFG_FILE"`
-	EnableScraping bool   `mapstructure:"ENABLE_SCRAPING"`
-	EnableStats    bool   `mapstructure:"ENABLE_STATS"`
-	Version        bool   `mapstructure:"VERSION"`
-	Organization   string `mapstructure:"ORGANIZATION"`
-	OtpSeed        string `mapstructure:"OTP_SEED"`
-	OutputDir      string `mapstructure:"OUTPUT_DIR"`
-	Password       string `mapstructure:"PASSWORD"`
-	Port           int    `mapstructure:"PORT"`
-	ScmURL         string `mapstructure:"SCM_URL"`
-	Token          string `mapstructure:"TOKEN"`
-	Username       string `mapstructure:"USERNAME"`
+	CfgFile             string `mapstructure:"CFG_FILE"`
+	EnableScraping      bool   `mapstructure:"ENABLE_SCRAPING"`
+	UserPermissionStats bool   `mapstructure:"USER_PERMISSION_STATS"`
+	Version             bool   `mapstructure:"VERSION"`
+	Organization        string `mapstructure:"ORGANIZATION"`
+	OtpSeed             string `mapstructure:"OTP_SEED"`
+	OutputDir           string `mapstructure:"OUTPUT_DIR"`
+	Password            string `mapstructure:"PASSWORD"`
+	Port                int    `mapstructure:"PORT"`
+	ScmURL              string `mapstructure:"SCM_URL"`
+	Token               string `mapstructure:"TOKEN"`
+	Username            string `mapstructure:"USERNAME"`
 }
 
 var ViperEnv ViperEnvVars

pkg/github/auditor/auditor.go

@@ -49,12 +49,12 @@ func NewGithubAuditor(token string) (*GithubAuditor, error) {
 // or yielded in an error), and a generic overall error if audit fails overall
 func (gs GithubAuditor) AuditOrg(
 	name string,
-	enableStats bool,
+	enableUserPermissionStats bool,
 ) ([]issue.Issue, map[issue.IssueID]error, error) {
 	ctx := context.Background()
 	back := &backoff.Backoff{
-		Min:    10 * time.Second,
-		Max:    1 * time.Hour,
+		Min:    30 * time.Second,
+		Max:    30 * time.Minute,
 		Jitter: true,
 	}
 	org, err := org.NewOrganization(ctx, gs.client, back, name)
@@ -63,5 +63,5 @@ func (gs GithubAuditor) AuditOrg(
 		return nil, nil, err
 	}
 
-	return org.Audit(ctx, enableStats)
+	return org.Audit(ctx, enableUserPermissionStats)
 }

pkg/github/org/org.go

@@ -543,6 +543,8 @@ func (org *Organization) AuditMemberPermissions(
 	var issues []issue.Issue
 	execStatus := make(map[issue.IssueID]error, 1)
 
+	log.Logger.Infof("[slow] Fetching user permissions for %s", *org.info.Login)
+
 	// userRepoPermissions holds a list of all repos with a given permission for a given user
 	type userRepoPermissions map[string]([]types.RepoName)
 	// permission summary is the list of different permissions for a user
@@ -620,7 +622,7 @@ func (org *Organization) AuditMemberPermissions(
 
 func (org *Organization) Audit(
 	ctx context.Context,
-	enableStats bool,
+	enableUserPermissionStats bool,
 ) ([]issue.Issue, map[issue.IssueID]error, error) {
 	var allIssues []issue.Issue
 	execStatus := map[issue.IssueID]error{}
@@ -634,7 +636,7 @@ func (org *Organization) Audit(
 	org.GetInstalls(ctx)
 	org.GetActionRunners(ctx)
 
-	if enableStats {
+	if enableUserPermissionStats {
 		auditHooks = append(auditHooks, org.AuditMemberPermissions)
 	}
 	for _, hook := range auditHooks {

pkg/github/repo/repo.go

@@ -157,6 +157,7 @@ func (repo *Repository) GetCollaborators(ctx context.Context) (
 		log.Logger.Error(err)
 	}
 
+	repo.backoff.Reset()
 	collaborators := make(map[types.UserLogin]types.User, len(users))
 	for _, u := range users {
 		collaborators[types.UserLogin(*u.Login)] = u

pkg/github/utils/utils.go

@@ -90,24 +90,51 @@ func WorkflowsAggregator(workflows *github.Workflows) []types.Workflow {
 
 func GetPaginatedResult[T any, K any](
 	ctx context.Context,
-	backoff *backoff.Backoff,
+	globalBackoff *backoff.Backoff,
 	callOpts *github.ListOptions,
 	githubCall func(opts *github.ListOptions) (K, *github.Response, error),
 	aggregator func(K) []T,
 ) ([]T, error) {
 
 	var results []T
+	retries := 0
+
+	var back *backoff.Backoff
+	if globalBackoff != nil {
+		back = &backoff.Backoff{
+			Min:    globalBackoff.Min,
+			Max:    globalBackoff.Max,
+			Jitter: globalBackoff.Jitter,
+		}
+	} else {
+		back = &backoff.Backoff{
+			Min:    30 * time.Second,
+			Max:    30 * time.Minute,
+			Jitter: true,
+		}
+	}
+
 	for {
 		raw, resp, err := githubCall(callOpts)
 
-		if _, ok := err.(*github.RateLimitError); ok {
-			d := backoff.Duration()
-			log.Logger.Infoln("Hit rate limit, sleeping for %d", d)
+		_, ok := err.(*github.RateLimitError)
+		if ok || resp == nil {
+			d := back.Duration()
+			log.Logger.Infof("Hit rate limit, sleeping for %v", d)
 			time.Sleep(d)
+			if resp == nil {
+				retries += 1
+				if retries > 10 {
+					return results, fmt.Errorf(
+						"Aborting after 5 failed retries",
+					)
+				}
+			}
 			continue
 		}
 
-		if err != nil {
+		retries = 0
+		if err != nil && resp != nil {
 			if resp.StatusCode == 403 {
 				log.Logger.Debugf(
 					"It appears the token being used doesn't have access to call %v",
@@ -123,7 +150,7 @@ func GetPaginatedResult[T any, K any](
 			return results, err
 		}
 
-		backoff.Reset()
+		back.Reset()
 		for _, res := range aggregator(raw) {
 			results = append(results, res)
 		}

pkg/output/html/html.go

@@ -342,6 +342,9 @@ func Serve(
 		"Server with HTML summary starting at 0.0.0.0:%d\n",
 		port,
 	)
+	fmt.Println(
+		"\n\n\t Analysis complete! Visit localhost:3000 using your browser to see results",
+	)
 	err = http.ListenAndServe(fmt.Sprintf(":%d", port), nil)
 	if err != nil {
 		log.Logger.Error(err)

pkg/output/html/templates/index.html.tmpl

@@ -102,7 +102,7 @@
       </div>
     </div>
     <div>
-      The following checks had errors - does token you used have the appropriate permissions?
+      The following checks had errors - does the token you used have the appropriate permissions?
       <ul>
         {{range $issue := $failed}}
         <li>

pkg/scraping/scraping.go

@@ -12,7 +12,7 @@ import (
 )
 
 func AuditScraping(
-	username, password, otpseed, org string, enableStats bool,
+	username, password, otpseed, org string,
 ) ([]issue.Issue, map[issue.IssueID]error, error) {
 	var issues []issue.Issue
 	execStatus := make(map[issue.IssueID]error, 1)
@@ -33,10 +33,6 @@ func AuditScraping(
 		issues = append(issues, issue.ApplicationRestrictionsDisabled(org))
 	}
 
-	if !enableStats {
-		return issues, execStatus, nil
-	}
-
 	apps, err := client.ListOAuthApps(org)
 	execStatus[issue.STATS_OAUTH_PERMS] = err
 	if err != nil {