Skip to content

WIP: Feat/exclude values#1663

Draft
chaimleib wants to merge 8 commits intosecurego:masterfrom
chaimleib:feat/exclude-values
Draft

WIP: Feat/exclude values#1663
chaimleib wants to merge 8 commits intosecurego:masterfrom
chaimleib:feat/exclude-values

Conversation

@chaimleib
Copy link
Copy Markdown

@chaimleib chaimleib commented May 1, 2026

Adds keys and values fields to the (formerly Path-)ExcludeRule type. If provided with a slice of regex patterns, G101 "Possible hardcoded credentials" issues will also be excluded if the hardcoded-value matches any of the values patterns, or if its key matches any of the keys patterns.

For example, the following code, common in test files, used to trigger an error:

const token = "fakeToken"

With this PR, a config can be set to exclude that issue when encountered in test files, based on the hardcoded value matching a pattern:

{
  "exclude-rules": [{
    "path": ".+_test\\.go$",
    "values": [
      "(?i)^test",
      "(?i)^fake"
    ],
    "rules": [ "G101" ]
  }]
}

@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 00:13 — with GitHub Actions Inactive
@chaimleib chaimleib force-pushed the feat/exclude-values branch from 656e5c1 to 621cadb Compare May 1, 2026 00:29
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 00:29 — with GitHub Actions Inactive
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 22:45 — with GitHub Actions Inactive
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 22:46 — with GitHub Actions Inactive
@chaimleib chaimleib force-pushed the feat/exclude-values branch from efef057 to 39c5837 Compare May 1, 2026 22:59
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 22:59 — with GitHub Actions Inactive
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 23:17 — with GitHub Actions Inactive
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

🤖 Barry Security Review

Comment thread exclusion_filter.go Outdated
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 23:25 — with GitHub Actions Inactive
@chaimleib chaimleib force-pushed the feat/exclude-values branch from e8ccb74 to bc9372b Compare May 1, 2026 23:26
@chaimleib chaimleib temporarily deployed to security-review May 1, 2026 23:26 — with GitHub Actions Inactive
@chaimleib chaimleib temporarily deployed to security-review May 2, 2026 00:11 — with GitHub Actions Inactive
@chaimleib chaimleib force-pushed the feat/exclude-values branch from 7927a9b to c8a4d2d Compare May 2, 2026 01:09
@chaimleib chaimleib temporarily deployed to security-review May 2, 2026 01:09 — with GitHub Actions Inactive
@ccojocar
Copy link
Copy Markdown
Member

ccojocar commented May 4, 2026

Thanks for this contribution. This seems AI generated.

Why was required to rename the methods/functions? Can you reduce the size of the changes only to the hardcode credentials?

@chaimleib
Copy link
Copy Markdown
Author

chaimleib commented May 4, 2026

Thanks for this contribution. This seems AI generated.

Why was required to rename the methods/functions? Can you reduce the size of the changes only to the hardcode credentials?

This was not AI generated. I did not even use AI for autocomplete.

I renamed some symbols because they mentioned "Path Exclusion", but I was adding functionality not related to paths. To avoid the misleading implication that PathExclusion symbols dealt only with paths, I removed "Path" from the names.

This is still WIP, and I'm looking into whether I should push the config deeper to be more tightly coupled to G101. I also want to write tests.

@ccojocar
Copy link
Copy Markdown
Member

ccojocar commented May 4, 2026

Please do first the minimum change required to incorporate the functionality you mentioned without refactoring. You can follow up with refactoring if it is needed afterwards. This will make reviewing the change much easier. Thanks

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

❌ Patch coverage is 69.95885% with 73 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.25%. Comparing base (5f4eec9) to head (c8a4d2d).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
exclusion_filter.go 63.44% 49 Missing and 4 partials ⚠️
rules/hardcoded_credentials.go 83.72% 13 Missing and 1 partial ⚠️
cmd/gosec/main.go 25.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1663      +/-   ##
==========================================
- Coverage   80.57%   80.25%   -0.32%     
==========================================
  Files         109      109              
  Lines       10181    10260      +79     
==========================================
+ Hits         8203     8234      +31     
- Misses       1495     1541      +46     
- Partials      483      485       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

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

Can you please add some info in the README regarding the usage of this exclusion filter?

Is this introducing any breaking changes for the existing filter format?

Comment thread exclusion_filter.go
// In other words, this method will further limit the exlude rule's scope,
// if the issue has addenda matching either keys or values
// which were specified in the rule.
func (r *compiledExcludeRule) ExcludesAddenda(addenda any) bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would use a type for addenda instead of any. It can be a join between Keyer and Valuer.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The idea (which I'm reconsidering) was to allow the different rules to provide different types of addenda. I imagined that some rules might only provide a key, for example, and not a value. And some rules might provide addenda with completely different methods, or none at all.

This would go away if I tightly couple the new configs to G101.

Comment thread exclusion_filter.go

// ShouldExclude returns true if the given issue should be excluded based on
// its file path, rule ID, and addenda.
func (f *ExclusionFilter) ShouldExclude(filePath, ruleID string, addenda any) bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto: addenda with type.

Comment thread exclusion_filter.go
}

pathPattern := strings.TrimSpace(part[:colonIdx])
rulesPart := strings.TrimSpace(part[colonIdx+1:])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the part can be only of len == 1? This indexing will generate a crash in that case.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is not my code. It was carried over from when file had a different name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants