Skip to content

Fix rule extension #1556

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 10, 2024
Merged

Fix rule extension #1556

merged 3 commits into from
Oct 10, 2024

Conversation

9999years
Copy link
Contributor

Description:

PR #1524 broke the ability to specify a [[rules]] with the same ID as a built-in rule in order to extend the default configuration.

This PR restores this ability, but merges the provided configuration with the default rule instead of overwriting the default rule wholesale.

Checklist:

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

@9999years
Reimplement the ability to override built-in rules
785fd3e 
PR gitleaks#1524 broke the ability to specify a `[[rules]]` with the same ID as
a built-in rule in order to extend the default configuration.

This PR restores this ability, but merges the provided configuration
with the default rule instead of overwriting the default rule wholesale.
@9999years
Add tests
91b73bf
@rgmz
Copy link
Contributor

rgmz commented Oct 9, 2024

(Continuing the discussion from #1524.)

@rgmz Just a minimal example. Our actual configuration looks like this:

Ah, I see you want to completely replace the rule. I haven't seen anyone do that before. The impetus for this change was because quite a few people were doing stuff similar to your minimal example.

The [extend] functionality seems to be poorly understood / lack definite clarity on what gets appended versus overwritten. This change makes sense — although it is inconsistent that tags/keywords are overwritten but allowlist is appended.

zricethezav
zricethezav reviewed Oct 9, 2024
View reviewed changes
@zricethezav
Copy link
Collaborator

The [extend] functionality seems to be poorly understood / lack definite clarity on what gets appended versus overwritten. This change makes sense — although it is inconsistent that tags/keywords are overwritten but allowlist is appended.

I think we need to be consistent across the board for the default case. All the things should be appended rather than overwritten. A feature could be included to configure the behavior of extend down the line -- append vs overwrite, or something.

@zricethezav zricethezav merged commit be9d0f8 into gitleaks:master Oct 10, 2024
1 check passed
@9999years 9999years deleted the fix-rule-extension branch October 10, 2024 21:58
@rgmz rgmz mentioned this pull request Oct 14, 2024
Merged
3 tasks
alayne222 pushed a commit to alayne222/gitleaks that referenced this pull request May 28, 2025
@9999years
Fix rule extension (gitleaks#1556)
7ffe234 
* Reimplement the ability to override built-in rules

PR gitleaks#1524 broke the ability to specify a `[[rules]]` with the same ID as
a built-in rule in order to extend the default configuration.

This PR restores this ability, but merges the provided configuration
with the default rule instead of overwriting the default rule wholesale.

* Add tests

* Append keywords and tags
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zricethezav zricethezav zricethezav left review comments

+1 more reviewer

@rgmz rgmz rgmz left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@9999years @rgmz @zricethezav