Scan GitHub wikis #2233

rgmz · 2023-12-16T00:21:21Z

Description:

GitHub wikis are another potential source for secrets.

This PR adds the functionality to scan wikis. It is enabled by default because wikis are just repositories that you can clone, and aren't subject to any rate limits.

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

rosecodym

Thanks for this! I have some minor questions about logging, but my big question is my silliest: Where is the new flag actually checked? I feel like I'm missing something dumb.

pkg/sources/git/git.go

pkg/sources/github/github.go

pkg/engine/github.go

rosecodym

please forgive my persnicketiness about comments - i'm trying to do my future self some favors

pkg/sources/git/git.go

pkg/sources/github/github.go

rgmz · 2023-12-20T22:21:38Z

please forgive my persnicketiness about comments - i'm trying to do my future self some favors

No need to apologize, these are all valid points. Some of these are remnants from prior iterations, such as when I thought that "has_wiki": true meant the repository has a wiki (😂🔫), or when I realized that it makes more sense to check if a wiki exists in repo.go.

I've pushed a few changes that should address those comments.

rgmz · 2024-01-30T13:25:22Z

Hey @rosecodym, do you have any other questions or feedback for this change?

rosecodym

Ah, apologies for letting this get away from me, and thanks for the reminder. The only non-clarifying question I have is about a new flag check (marked inline).

rosecodym · 2024-01-30T15:26:28Z

pkg/sources/github/github.go

-				scanFailed = true
-				return nil
+			// Scan comments, if enabled.
+			if s.includeGistComments || s.includeIssueComments || s.includePRComments {


Were we just ignoring these flags before? This is a surprising thing to not already see here...

I added an explicit check because I think it's more consistent and readable to have if includeWiki { doWiki } ... if includeComments { doComments }.

The s.includeGistComments, s.includeIssueComments, and s.includePRComments weren't being ignored, but they were implemented in a way that's awkward and difficult to understand (imo). Plus I don't think it makes sense to enter a function that can return errors when the flags are disabled.

trufflehog/pkg/sources/github/github.go

Lines 1104 to 1107 in 9867ce8

repoURL, err := git.GitURLParse(repoPath)

if err != nil {

return err

}

trufflehog/pkg/sources/github/github.go

Lines 1179 to 1182 in 9867ce8

// Normal repository URL (https://github.com/<owner>/<repo>).

if len(trimmedURL) < 3 {

return fmt.Errorf("url missing owner and/or repo: '%s'", repoURL.String())

}

Oof, strong agree. Thanks for this.

rosecodym · 2024-01-30T15:27:43Z

pkg/sources/github/github.go

 				}
-				logger.V(2).Info(fmt.Sprintf("scanned %d/%d repos", scannedCount, len(s.repos)), "duration_seconds", time.Since(start).Seconds())


Does this no longer happen if the scan panics? That's a fine change (and your version is more readable), I just want to ensure I understand the diff.

Does this no longer happen if the scan panics?

These changes were made based on how the code looked before #2220 was merged. Now that #2220 has been merged, the diff is a bit awkward.

Your understanding is correct. In fact, since @dustin-decker moved the metric and scannedCount increment to the end of the function, I should move this logging statement as well.

pkg/sources/github/github.go

rosecodym

This looks great, thanks for doing it!

rosecodym · 2024-01-31T15:37:16Z

pkg/sources/github/github.go

-				scanFailed = true
-				return nil
+			// Scan comments, if enabled.
+			if s.includeGistComments || s.includeIssueComments || s.includePRComments {


Oof, strong agree. Thanks for this.

This reverts commit 8e90c4e.

The new functionality introduced by #2233 runs very slowly; this commits causes the new functionality to not run by default.

rgmz requested review from a team as code owners December 16, 2023 00:21

rosecodym reviewed Dec 19, 2023

View reviewed changes

pkg/sources/git/git.go Outdated Show resolved Hide resolved

pkg/sources/github/github.go Outdated Show resolved Hide resolved

pkg/engine/github.go Show resolved Hide resolved

rgmz force-pushed the feat/github-scan-wikis branch 2 times, most recently from adc5962 to e59837d Compare December 19, 2023 21:12

rosecodym reviewed Dec 20, 2023

View reviewed changes

pkg/sources/git/git.go Outdated Show resolved Hide resolved

pkg/sources/github/github.go Show resolved Hide resolved

pkg/sources/github/github.go Outdated Show resolved Hide resolved

pkg/sources/github/github.go Outdated Show resolved Hide resolved

rgmz force-pushed the feat/github-scan-wikis branch from e59837d to 03a819d Compare December 20, 2023 22:18

rgmz force-pushed the feat/github-scan-wikis branch 4 times, most recently from 3d22af5 to c5b5875 Compare January 30, 2024 13:22

rosecodym reviewed Jan 30, 2024

View reviewed changes

rgmz force-pushed the feat/github-scan-wikis branch 2 times, most recently from a4f948a to d5ff649 Compare January 30, 2024 23:32

feat(github): scan wikis

6b7f030

rgmz force-pushed the feat/github-scan-wikis branch from d5ff649 to 6b7f030 Compare January 30, 2024 23:34

rosecodym approved these changes Jan 31, 2024

View reviewed changes

rosecodym merged commit 8e90c4e into trufflesecurity:main Jan 31, 2024
9 of 10 checks passed

rgmz deleted the feat/github-scan-wikis branch January 31, 2024 15:52

rgmz mentioned this pull request Feb 3, 2024

Enumerate GitHub wikis concurrently #2371

Closed

rosecodym added a commit that referenced this pull request Feb 5, 2024

Revert "Scan GitHub wikis #2233"

78e33f2

This reverts commit 8e90c4e.

This was referenced Feb 5, 2024

Revert "Scan GitHub wikis #2233" #2385

Closed

Disable GitHub wiki scanning by default #2386

Merged

rosecodym added a commit that referenced this pull request Feb 5, 2024

Disable GitHub wiki scanning by default (#2386)

95616b0

The new functionality introduced by #2233 runs very slowly; this commits causes the new functionality to not run by default.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scan GitHub wikis #2233

Scan GitHub wikis #2233

rgmz commented Dec 16, 2023

rosecodym left a comment

rosecodym left a comment

rgmz commented Dec 20, 2023 •

edited

Loading

rgmz commented Jan 30, 2024

rosecodym left a comment

rosecodym Jan 30, 2024

rgmz Jan 30, 2024 •

edited

Loading

rosecodym Jan 31, 2024

rosecodym Jan 30, 2024

rgmz Jan 30, 2024

rosecodym left a comment

rosecodym Jan 31, 2024

	repoURL, err := git.GitURLParse(repoPath)
	if err != nil {
	return err
	}

	// Normal repository URL (https://github.com/<owner>/<repo>).
	if len(trimmedURL) < 3 {
	return fmt.Errorf("url missing owner and/or repo: '%s'", repoURL.String())
	}

		}
		logger.V(2).Info(fmt.Sprintf("scanned %d/%d repos", scannedCount, len(s.repos)), "duration_seconds", time.Since(start).Seconds())

Scan GitHub wikis #2233

Scan GitHub wikis #2233

Conversation

rgmz commented Dec 16, 2023

Description:

Checklist:

rosecodym left a comment

Choose a reason for hiding this comment

rosecodym left a comment

Choose a reason for hiding this comment

rgmz commented Dec 20, 2023 • edited Loading

rgmz commented Jan 30, 2024

rosecodym left a comment

Choose a reason for hiding this comment

rosecodym Jan 30, 2024

Choose a reason for hiding this comment

rgmz Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

rosecodym Jan 31, 2024

Choose a reason for hiding this comment

rosecodym Jan 30, 2024

Choose a reason for hiding this comment

rgmz Jan 30, 2024

Choose a reason for hiding this comment

rosecodym left a comment

Choose a reason for hiding this comment

rosecodym Jan 31, 2024

Choose a reason for hiding this comment

rgmz commented Dec 20, 2023 •

edited

Loading

rgmz Jan 30, 2024 •

edited

Loading