NotificationsYou must be signed in to change notification settings
Fork1.7k
Star22k

Feat/Passing multiple arguments to dir#1750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

recreator66 wants to merge6 commits intogitleaks:master

base:master

Choose a base branch

fromrecreator66:feat/dir-multiple-args

Open

Feat/Passing multiple arguments to dir#1750

recreator66 wants to merge6 commits intogitleaks:masterfromrecreator66:feat/dir-multiple-args

Conversation

Copy link

Contributor

recreator66 commentedFeb 11, 2025•
edited
Loading

Description:

Closes#1727

Checklist:

Does your PR pass tests?
Have you written new tests for your changes?
Have you lint your code locally prior to submission?

recreator66 added3 commits

February 11, 2025 21:38

chore: prevent printing banner multiple times

3f55b13

feat: add logic to handle multiple arguments

b896585

chore: go fmt

08bddf3

recreator66 marked this pull request as draft

February 11, 2025 20:43

Copy link

ContributorAuthor

recreator66 commentedFeb 11, 2025

PR is currently WIP. There is still some testing necessary if the changes have any impact on the core functionalities (e.g. passing config file path, gitleaks-ignore-path etc.)

I am happy for any volunteer that reviews the changes and tests Detector functionalities.

rgmz reviewed

Feb 16, 2025

View reviewed changes

Copy link

Contributor

rgmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a good base.

I have a few suggestions to streamline the approach a bit. You can apply this withgit apply patch.txt:patch.txt

cmd/directory.go Outdated

		var allFindings []report.Finding

		start := time.Now()
		findingsMap := make(map[string]report.Finding)

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What's the purpose of this map? Wouldn't it technically change the behaviour of how things are reported?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

From my point of view the behavior itself would not change. The only reason for creating the map is to remove redundant findings by overriding them with the same fingerprint. Otherwise the report would not be accurate anymore when e.g. scanning a root directory with a child directory.

You can test this by scanning the./testdata directory with./testdata/repos/nogit as additional argument.

cmd/directory.go Outdated

		for _, finding := range findings {
		findingsMap[finding.Fingerprint] = finding
		}
		detector = det

Copy link

Contributor

rgmzFeb 16, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is an unfortunate consequence ofconfiguring report output underDetector. I don't know if there's a better way.

Copy link

ContributorAuthor

recreator66Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I see your point. In general, more testing is required from my end as new Detector structs will be initialized for each directory. This basically has an impact on all cli flag that you pass in.

Will think about a better way, ideally there is only one Detector initialized that stores all flags with e.g. absolute paths for config files etc

UnknownPlatypus mentioned this pull request

Feb 22, 2025

gitleaks does not scan correct files with pre-commit run --files#1409

Open

Copy link

Contributor

rgmz commentedMar 11, 2025

Can you elaborate? It sounds like the results are the same?

Result of gitleaks detect --no-git testdata/expected/git -v and gitleaks detect --no-git . -vis the same.
...
I would have expected that the result ...is equal..

recreator66 added2 commits

March 12, 2025 20:56

refactor: apply patch.txt

9b8be67

refactor: apply patch.txt & unpack findings

4d791ff

Copy link

ContributorAuthor

recreator66 commentedMar 12, 2025

Update

Test Case 1

Compile both gitleaks binaries from source and target branch of this MR.
Execute following command with gitleaks binary frommaster branch
- gitleaks dir testdata/repos/staged - reports 2 leaks
- gitleaks dir testdata/repos/nogit - reports 1 leak
Execute following command with gitleaks binary fromrecreator66:feat/dir-multiple-args branch
- gitleaks dir testdata/repos/staged testdata/repos/nogit - reports 3 leaks

Test Case 2

Compile both gitleaks binaries from source and target branch of this MR.
Create a custom configuration toml file e.g.gitleaks-toml.zip
Execute following command with gitleaks binary frommaster branch
- gitleaks dir testdata/config -c gitleaks-test.toml - reports 1 leak
- gitleaks dir testdata/repos -c gitleaks-test.toml - reports 1 leak
Execute following command with gitleaks binary fromrecreator66:feat/dir-multiple-args branch
- gitleaks dir testdata/config testdata/repos -c gitleaks-test.toml - reports 2 leaks

Even with passing a custom toml I haven't noticed any issues with the changes.
The detailed reports can be compared with verbose mode/ flag.

Since the value of config flag is the only one used to setup the gitleaks config I assume that the changes made within this MR do not have an impact on how the detector is initialized.

To-Do

Test and adjust the logging while initializing new detectors. You can see e.g. indebug mode that gitleaks is logging the construction of additional detectors fromL39. This logging needs to be suppressed.

Output from scanning 2 directories:

gitleaks dir testdata/config testdata/repos -c gitleaks-test.toml -l debug    ○    │╲    │ ○    ○ ░    ░    gitleaks10:38PM DBG using github.com/wasilibs/go-re2 regex engine10:38PM DBG using gitleaks config gitleaks-test.toml from`--config`10:38PM DBG found .gitleaksignore file: .gitleaksignore10:38PM DBG using github.com/wasilibs/go-re2 regex engine10:38PM DBG using gitleaks config gitleaks-test.toml from`--config`10:38PM DBG found .gitleaksignore file: .gitleaksignore10:38PM DBG Skipping symlink path=testdata/repos/symlinks/file_symlink/symlinked_id_ed2551910:38PM INF scanned~17007 bytes (17.01 KB)in 52.9ms10:38PM WRN leaks found: 2

perf: initialize config only once

2462fdb

recreator66 marked this pull request as ready for review

March 17, 2025 21:35

Copy link

ContributorAuthor

recreator66 commentedMar 17, 2025•
edited
Loading

@rgmz I have adopted your approach to initialize the config once while iterating over the args (= directories). In addition, I enhanced the logging within thegitleaks directory command.

PR is ready for review

Copy link

ContributorAuthor

recreator66 commentedApr 15, 2025

@rgmz is this PR still relevant?

Copy link

UnknownPlatypus commentedApr 19, 2025

Does the current state of the pr allows doinggitleaks dir file1 file2 ... ?

If so it's still very relevant to me to be able to fix thepre-commit integration (see#1409 (comment))

pre-commit run tools on a list of files (usually the one your are trying to commit) so I really need this PR behavior to make the pre-commit integration work properly.