Repository navigation
Add maxissues threshold support for CI/CD rule enforcement - #15
Conversation
- Add MaxIssues field to RuleMetadataOverride (Rule.go) - Add GetRuleMaxIssues method to Config to read the configured threshold (config.go) - Add checkThresholdViolations in output.go to compare findings against thresholds and exit non-zero when a rule exceeds its configured maxissues in CI/CD mode
- Add maxissues property description and usage to README and index.md - Update .asist.example.yaml with maxissues example and explanatory comments
| for ruleId, count := range findingsPerRule { | ||
| maxIssuesAllowed := configFile.GetRuleMaxIssues(ruleId) | ||
|
|
||
| if count > maxIssuesAllowed { |
There was a problem hiding this comment.
This may be considered a breaking change if we really want CI/CD to only print the issues.
This just prints when you go over, default behaviour is 0 allowed in CI/CD
Could be prettified too, it shows just after printing issues, so probably need to add a linebreak before if we do decide to keep it.
Eg. example
{
"ID": "XSSMergeField",
"Name": "Potential XSS with mergefield",
"Description": "Using a merge field inside a script or style tag may introduce an XSS issue. Consider adding an encode method for the merge field; otherwise, ensure that all user inputs are sanitized properly.",
"Severity": "High",
"RuleCategory": "Security",
"Occurrence": {
"File": "<redacted>",
"Line": " <redacted>",
"LineNumber": 13,
"ColumnRange": [
52,
83
]
}
}
]
}
Threshold violation: Rule XSSMergeField has 50 issues (max allowed: 5)
There was a problem hiding this comment.
Prettified it a little bit more, I added a space
... JSON
}
Threshold violations:
Rule XSSDomHtml has 40 issues (max allowed: 0)
Rule XSSMergeField has 36 issues (max allowed: 5)
2 rule(s) exceed their maxissues threshold.
| 2. Enable the `Custom Binary Enabled` setting | ||
| 3. Provide the path to your ASIST binary in `Custom Binary Path` | ||
| 1. Enable the `Custom Binary Enabled` setting | ||
| 1. Provide the path to your ASIST binary in `Custom Binary Path` |
There was a problem hiding this comment.
Context: 1. 1. 1 etc is valid for MD, it auto-increments.
|
|
||
|
|
||
| ### 🫣 .gitignore and .forceignore files | ||
| ## 🫣 .gitignore and .forceignore files |
There was a problem hiding this comment.
I did some nitpicking, a H1 -> H3.
Arguably this should have stayed a H3 and CI/CD should be a H2 but...
Happy to change it up 😅
There was a problem hiding this comment.
Future enhancement I might do, is make the index/readme copy from a central file with these details so I don't need to copy-paste across except for the headers/footers. Maybe it does and I just didn't see it?
| * Exclude files or directories for specific rules | ||
| * Add custom regex rules | ||
| * Define rules to be run in CI/CD mode | ||
| - Enable/disable all the standard rules |
There was a problem hiding this comment.
| - Enable/disable all the standard rules | |
| - Enable/disable all the standard rules |
| cicdrules: | ||
| - "XSSLabel" | ||
| - "XSSMergeField" | ||
| - "XSSDomHtml" |
There was a problem hiding this comment.
Should this list be alphabetical?
|
|
||
| /** | ||
| * Checks if any rule exceeds its configured maxissues. | ||
| * Default maxissues is 0 (no issues allowed) if not explicitly set. |
There was a problem hiding this comment.
Default and 'not explicitly set' mean the same. So there is redundancy in this sentence.
- Alphabetising cicd rules - Removing rogue spaces at the end of list
- Ordering the rules to ensure they are deterministic for testing - Removed unintentional repetition in comment - Added padding between the violations - Added a message for saying how many violated rules there are
Added: - Integration tests - Output tests - config tests
15621c4
ajaincertinia
left a comment
There was a problem hiding this comment.
Hi @B1naryVision, we had internal discussion regarding the --list-rule case, and decided that maxissues should be renamed to cicdmaxissues (as per your earlier suggestion), and --list-rule should print this property.
- Rename RuleMetadataOverride.MaxIssues → CicdMaxIssues; update YAML/JSON deserialization key from `maxissues` to `cicdmaxissues` accordingly - Add CicdMaxIssues int field to RuleMetadata so --list-rules output includes the configured threshold per rule (default 0) - Populate CicdMaxIssues on RuleMetadata via Override() at rule load time - Update all tests, fixtures, docs, example config, and changelog
| } | ||
| } | ||
|
|
||
| func TestCheckThresholdViolations_WhenMaxIssuesNotSet_DefaultsToZero(t *testing.T) { |
There was a problem hiding this comment.
| func TestCheckThresholdViolations_WhenMaxIssuesNotSet_DefaultsToZero(t *testing.T) { | |
| func TestCheckThresholdViolations_WhenCicdMaxIssuesNotSet_DefaultsToZero(t *testing.T) { |
|
|
||
| //Then | ||
| if !result { | ||
| t.Errorf("Expected violation when maxissues defaults to 0 and rule has 1 finding") |
There was a problem hiding this comment.
| t.Errorf("Expected violation when maxissues defaults to 0 and rule has 1 finding") | |
| t.Errorf("Expected violation when cicdmaxissues defaults to 0 and rule has 1 finding") |
| * Returns the max allowed issues for a specific rule ID. | ||
| * Returns ruleoverrides.<rule>.cicdmaxissues if set, otherwise 0. | ||
| */ | ||
| func (c *Config) GetRuleMaxIssues(ruleId rules.RuleID) int { |
There was a problem hiding this comment.
Not sure but should GetRuleMaxIssues also be renamed to GetRuleCicdMaxIssues?
There was a problem hiding this comment.
I don't mind it, I think the method is clear. But happy to change it if you feel like it should be renamed
Co-authored-by: Govardhan <13658588+gkumarcertinia@users.noreply.github.com>
- Add CicdMaxIssues *int field to CustomRegexRule config struct - Propagate it through NewCustomRule into RuleMetadata.CicdMaxIssues - Extend GetRuleCicdMaxIssues to fall back to CustomRegexRules after checking RuleOverrides, so custom rule thresholds are respected in CI/CD mode (-j) - Update config testdata fixtures (YAML + JSON) with cicdmaxissues - Update CustomRule_test.go and config_test.go with new tests - Update README, docs/index.md, .asist.example.yaml, and changelog
|
I meant to merge, accidentally closed. I just realised I don't see a merge button |
Currently, asist fails if any issues are detected (equivalent to a hard cap of 0). This change allows users to define a custom limit per rule.
💡 Why is this needed?
On large or legacy repositories, "perfect" compliance is often impossible to achieve overnight. By allowing a maxIssues threshold, teams can:
Grandfather in existing issues: Set the cap to the current issue count to prevent new regressions.
Incremental Improvement: Gradually lower the cap over time as technical debt is cleared.
CI Adoption: Enable the analyzer in CI/CD pipelines without immediately breaking the build for existing debt.
🛠 Proposed Changes
Added a maxIssues configuration option per rule.
Default Behavior: If no cap is defined, it defaults to 0, maintaining the current strict enforcement behavior.
Logic: The build fails only if actualIssues > maxIssues.
Why only CI/CD?: It only exits with error codes if it's in CI/CD mode, otherwise it simply prints out the issues.
Perhaps we would also like to Print a warning outside of CI/CD? Would that mess with baseline or anything though?
🧪 WIP Notes (Seeking Early Input)
I am raising this as a Draft PR to get feedback on the implementation logic before I finalize the test suite. Specifically:
Does the flag naming (maxIssues) align with the project's naming conventions?
Here's some other options I considered.
Are there any edge cases in the threshold logic I should account for before I write the unit tests?
I added a printing log in CI/CD mode, is this a breaking change as it now no longer ONLY prints the issues?