Feat/exclude values#1663
Conversation
656e5c1 to
621cadb
Compare
efef057 to
39c5837
Compare
e8ccb74 to
bc9372b
Compare
7927a9b to
c8a4d2d
Compare
|
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. EDIT 2026-05-07: Since I really want to restrict the exclusions to particular paths, path_filters.go is the most natural place to trigger the key-value excluder. But I separated the logic out to its own files as much as I could. |
|
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 Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
ccojocar
left a comment
There was a problem hiding this comment.
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?
…er issues with it
…ing logic, ensure k-v is always provided
… TODO except-rules
…onfigured and in the issue
c8a4d2d to
8f6972a
Compare
ccojocar
left a comment
There was a problem hiding this comment.
Also make sure to update the https://github.com/securego/gosec/blob/master/RULES.md#g101 with the added functionality when is implemented.
99239e0 to
2cc4ef0
Compare
2cc4ef0 to
9d7c266
Compare
… arg, instead of the file and ruleID
9d7c266 to
a722960
Compare
Done. There should be no breaking changes for the existing filter format. The new feature is only configurable via JSON right now, via a new field in the path filter config. I weighed allowing configs via CLI arguments, but that seemed too complicated at this stage. |
Done! |
| NoSec bool `json:"nosec"` // true if the issue is nosec | ||
| Suppressions []SuppressionInfo `json:"suppressions"` // Suppression info of the issue | ||
| Autofix string `json:"autofix,omitempty"` // Proposed auto fix the issue | ||
| HardcodedCredentialAddenda *HardcodedCredentialAddenda `json:"hardcoded_credential_addenda,omitempty"` // Info for configurable Hardcoded Credential exclusion rules. |
There was a problem hiding this comment.
This file is generic. The name sounds very specific only for hardcoded credentials rule.
Why this information is required to be added into an issue since the goal of the issue is only to add dictional filters on the matching.
Please remove this from the Issue structure. Thanks
There was a problem hiding this comment.
In one of my previous commits, this field was Addenda any. The problem then was the use of any. I got a comment seeming to say that it was too generic then. Should I change this back to Addenda any so that it becomes generic again? EDIT: The way I was envisioning things at that commit, different rules might add different Addenda fulfilling different interfaces than Keyer and Valuer, which I have since removed. Should I go back to that plan with Addenda any? Or should I stay with what I have, and refrain from imposing an opinion on later features which are not fleshed out yet?
(*PathExclusionFilter).ShouldExclude(*issue.Issue) needs some sort of addenda field, because otherwise it would lack key-value information about the violation. It wouldn't be able to call my new rule.hardcodedCredentialsExcluder.ShouldExcludeKV.
If I removed the call to ShouldExcludeKV from ShouldExclude, and instead made the exclusions at the rule level in rules/hardcoded_credentials.go, then the excluded counter for path-based excludes would fail to increment. In concrete terms, this section in cmd/gosec/main.go would not count the new exclusions:
// Apply path-based exclusions first
var pathExcludedCount int
issues, pathExcludedCount = pathFilter.FilterIssues(issues)
if pathExcludedCount > 0 {
logger.Printf("Excluded %d issues by path-based rules", pathExcludedCount)
}Do we want that?
| } | ||
| if r.ignoreEntropy || r.isHighEntropyString(val) { | ||
| iss := ctx.NewIssue(n, r.ID(), r.What, r.Severity, r.Confidence) | ||
| iss.HardcodedCredentialAddenda = &issue.HardcodedCredentialAddenda{ |
There was a problem hiding this comment.
This ensures that all issues reported by this rule include key-value information in a consistent way.
There was a problem hiding this comment.
In which context is the key/value information required in addition to the issue itself?
There was a problem hiding this comment.
I don't see what's the benefit of having this information into an issue.
There was a problem hiding this comment.
We could avoid this, if path-based exclusions for this rule were processed in rules/hardcoded_credentials.go, rather than in path_filter.go.
However, without some data passed down in the Issue, that would mess up the pathExcludedCount statistics.
The existing logic in path_filter.go only has access to the configs and the issue, so that's why I injected the key and value there. I thought it was important for there to be a single place, path_filter.go, which computes path-based exclusions, and tallies them all up.
But maybe that is not so important. I could compute the path exclusion in the rule, and then have it signal in the issue that it was excluded by the exclude-rules.G101 config:
- A new
ExcludedByPath boolfield in the Issue struct could work. That's generic. - Or, could I use the existing
Suppressionsfield? I didn't use that, because I thought this was reserved for#nosecandnolint:gosecdirectives, which include aJustificationstring. If it's ok to use that field, maybe I should add ajustificationfield to the new config file format?
{
"exclude-rules": [{
"path": ".+_test\\.go$",
"G101": {
"justification": "Mocked credentials for tests are not real.",
"values": [
"(?i)^test",
"(?i)^fake"
]
}
}]
}| Rules []string `json:"rules"` // Rule IDs to exclude. Use "*" to exclude all rules | ||
| Path string `json:"path"` // Regex pattern for matching file paths | ||
| Rules []string `json:"rules"` // Rule IDs to exclude. Use "*" to exclude all rules | ||
| G101 HardcodedCredentialExcludeOptions `json:"G101"` // Per-path G101 options |
There was a problem hiding this comment.
This is generic code. Why G101 is here? This should be named in a generic way, not specifically to a rule.
There was a problem hiding this comment.
In the JSON configuration file, users are expected to know the rule codes, as in the Rules field just above. As a UI, I tried to stay consistent in how users will name rule-specific options, so I named the rule-specific path exclude options with json:"G101".
(I did not want to change the type of the Rules field to a map[string]any. That would break compatibility with previous config files.)
I was also trying not to be confusing by naming the field in Go too differently from the JSON name. That's why the field name is also G101. Would you prefer the following?
CredentialExcludeOptions HardcodedCredentialExcludeOptions `json:"G101"`There was a problem hiding this comment.
My point is that this is generic code, not rule specific. You should use generic names for variables, and it should be written in a generic way that it can be reused.
For example this should be:
ExcludeOptions ExcludeOptions
There was a problem hiding this comment.
Right now, in the master branch, this is the best config I can manage for my purpose:
/* Config */ {
"exclude-rules": /* []PathExcludeRule */ [{
"path": ".+_test\\.go$",
"rules": [ "G101" ]
}]
}My idea was to allow being more particular about the exclusions like this:
/* Config */ {
"exclude-rules": /* []PathExcludeRule */ [{
"path": ".+_test\\.go$",
"G101": /* HardcodedCredentialExcludeOptions */ {
"keys": [ "(?i)^test", "(?i)^fake" ],
"values": [ "(?i)^test", "(?i)^fake", "\*{4}" ]
}
}]
}Are you proposing this?
/* Config */ {
"exclude-rules": /* []PathExcludeRule */ [{
"path": ".+_test\\.go$",
"options": /* ExcludeOptions */ {
"G101": /* HardcodedCredentialExcludeOptions */ {
"keys": [ "(?i)^test", "(?i)^fake" ],
"values": [ "(?i)^test", "(?i)^fake", "\*{4}" ]
}
}
}]
}Isn't that more awkward than my idea?
There was a problem hiding this comment.
Or, did you mean this?
/* Config */ {
"exclude-rules": /* []PathExcludeRule */ [{
"path": ".+_test\\.go$",
"options": /* ExcludeOptions */ {
"keys": [ "(?i)^test", "(?i)^fake" ],
"values": [ "(?i)^test", "(?i)^fake", "\*{4}" ]
}
}]
}What other rules would benefit from this?
What if you don't want all the rules to use a shared config? There would be no way to provide different rules with separate configs, unless each config was named uniquely to that rule:
/* Config */ {
"exclude-rules": /* []PathExcludeRule */ [{
"path": ".+_test\\.go$",
"options": /* ExcludeOptions */ {
"G101_keys": [ "(?i)^test", "(?i)^fake" ],
"G101_values": [ "(?i)^test", "(?i)^fake", "\*{4}" ],
"G999_keys": [ "(?i)^test", "(?i)^fake" ],
"G999_values": [ "(?i)^test", "(?i)^fake", "\*{4}" ]
}
}]
}But that would also get awkward and repetitive. Isn't it better to make a separate object for the configs of each rule?
/* Config */ {
"exclude-rules": /* []PathExcludeRule */ [{
"path": ".+_test\\.go$",
"G101": /* HardcodedCredentialExcludeOptions */ {
"keys": [ "(?i)^test", "(?i)^fake" ],
"values": [ "(?i)^test", "(?i)^fake", "\*{4}" ]
},
"G999": /* TBDExcludeOptions */ {
"keys": [ "(?i)^test", "(?i)^fake" ],
"values": [ "(?i)^test", "(?i)^fake", "\*{4}" ]
}
}]
}| // HardcodedCredentialExcludeOptions configures whether | ||
| // a Hardcoded Credentials issue should be excluded | ||
| // from the results, based on options specific to this rule. | ||
| type HardcodedCredentialExcludeOptions struct { |
There was a problem hiding this comment.
This should be named in a generic way, it's not only meant to be used only by hardcoded credentials rule.
There was a problem hiding this comment.
I thought of opening this up to allow other rules to use this. But when I looked at the other rules, they were different enough that either these options were irrelevant, or would increase the PR size by a lot.
Did you find another rule where you want to use this type?
To minimize the PR impact, and to keep type ownership and usage clear, I decided to not imply usefulness beyond "hardcoded credentials" (the name for the rule implied by the filename rules/hardcoded_credentials.go).
If a later PR does want to use this for a different rule, that PR can rename the types as needed.
There was a problem hiding this comment.
See my comment above, use a generic name for the variables.
| Values []string `json:"values"` | ||
| } | ||
|
|
||
| type CompiledHardcodedCredentialsRule struct { |
| original PathExcludeRule // Keep original for error messages | ||
| pathRegex *regexp.Regexp | ||
| ruleSet map[string]bool // Set of rule IDs to exclude | ||
| hardcodedCredentialsExcluder *CompiledHardcodedCredentialsRule |
There was a problem hiding this comment.
ditto, this should be named generically maybe CredentialsRule.
There was a problem hiding this comment.
I like shorter names. Should this be like so?
credentailsRule *CompiledCredentialsRule
Adds
.G101.keysand.G101.valuesfields to the PathExcludeRule 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 thevaluespatterns, or if its key matches any of thekeyspatterns.For example, the following code, common in test files, used to trigger an error:
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$", "G101": { "values": [ "(?i)^test", "(?i)^fake" ] } }] }Variable names can be excluded as well, even if the value looks like a credential:
{ "exclude-rules": [{ "path": ".+_test\\.go$", "G101": { "keys": ["(?i)^test"] } }] }