-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Composite rules #1905
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Composite rules #1905
Conversation
I think it's great and will be really helpful for one of my use cases ^_^. In v9 I would like to see |
config/config.go
Outdated
WithinLines *int `mapstructure:"within_lines"` | ||
WithinColumns *int `mapstructure:"within_columns"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to do camel case to match the format of the other settings values? (though I do prefer snake case).
WithinLines *int `mapstructure:"within_lines"` | |
WithinColumns *int `mapstructure:"within_columns"` | |
WithinLines *int `mapstructure:"withinLines"` | |
WithinColumns *int `mapstructure:"withinColumns"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah shoot, you're right. Looks like we have regexTarget
not regex_target
. Consistency wins this one, camel case it is
config/config.go
Outdated
// if _, ok := rulesMap[r.RuleID]; !ok { | ||
// return Config{}, fmt.Errorf("%s: [[rules.required]] rule ID '%s' does not exist", cr.RuleID, r.RuleID) | ||
// } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda like this check. Any reason for commenting it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to happen after all the rules have been seen otherwise it'll flag an err
@@ -360,6 +360,10 @@ func (d *Detector) detectRule(fragment Fragment, newlineIndices [][]int, current | |||
}() | |||
) | |||
|
|||
if r.SkipReport == true && !fragment.InheritedFromFinding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if r.SkipReport == true && !fragment.InheritedFromFinding { | |
if r.SkipReport && !fragment.InheritedFromFinding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should signal to folks that this code is 100% handcrafted by human hands and not generated by an LLM
report/finding.go
Outdated
@@ -68,3 +98,29 @@ func maskSecret(secret string, percent uint) string { | |||
|
|||
return secret[:lth] + "..." | |||
} | |||
|
|||
func (f *Finding) PrintAuxiliaryFindings() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this here instead of util because auxiliaryFindings
is private? Would it make sense for the all the Print*
functions to go in report since it seems like they're more focused on reporting info back to the user instead of the detection itself?
WithinLines *int | ||
WithinColumns *int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish go had an easy way to set defaults for struct values like this. Then you could set it to -1
and wouldn't have to deference pointers to ints.
detect/detect.go
Outdated
inheritedFragment.InheritedFromFinding = true | ||
|
||
// Call detectRule once for each required rule | ||
requiredFindings := d.detectRule(inheritedFragment, newlineIndices, currentRaw, rule, encodedSegments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If two rules have the same required rule, would d.detectRule
get called twice for that rule?
Would something like this save extra scans and passes to see if the reqs are met? (doing it in python/pseudo code):
def detect_with_reqs(rule:Rule, fragment:Fragement, findings_by_rule:dict[str,list[Finding]], to_report:list[Finding]) -> list[Finding]:
# Already processed just return the findings
if rule.id in findings_by_rule:
return findings_by_rule[rule.id]
# Run detections for this rule
findings = detect(rule, fragment, ...)
# If there's nothing left to do, add the findings to the report set and
# return the findings
if not findings or not rule.requires:
if not rule.skip_report:
to_report.extend(findings)
return findings
# Handle the requirements
for req in rule.requires:
req_rule = rules.config[req.id]
# This handles the req tree recursively
req_findings = detect_with_reqs(req_rule, fragment, findings_by_rule, to_report)
# No need to go further if a requirement didn't have findings
if not req_findings:
return []
# Now check if the reqs' findings meet the criteria
for finding in findings:
# Filter the req_findings to the one that meet the proximity requirement
req_findings = within_proximity(req, finding, req_findings)
# No need to go further given a requirement wasn't met
if not req_findings:
return []
# Requirement met! add the findings
finding.add_aux_findings(req_findings)
return findings
def detect(fragment):
to_report = []
findings_by_rule = {}
for rule in config.rules.values():
# These cases will be handled by detect_with_reqs
if rule.skip_report:
continue
_ = detect_with_reqs(rule, fragment, findings_by_rule, to_report)
return to_report
I really like this approach!
What about naming
The
I would say yes, as otherwise reports get flooded. Another alternative is to just use those definitions inline only so that they are only used by the composite rule and not as a regular rule. Could also be beneficial as you typically use those other roles only as part of a composite rule, e.g. like this:
|
whelp in she goes |
Description:
Example composite rule
Fun diagrams for the visual learners (also seen in the readme)
Question for the community:
required
,skipReport
,Auxiliary
?skipReport
?Checklist: