Skip to content
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

SpiderFootCorrelator: Add error handling, annotations, docstrings #1654

Merged
merged 1 commit into from May 1, 2022

Conversation

bcoles
Copy link
Contributor

@bcoles bcoles commented Apr 29, 2022

No description provided.

Comment on lines -59 to -66
# Strip any extra newlines that may have creeped into meta
for rule in self.rules:
for k in rule['meta'].keys():
if isinstance(rule['meta'][k], str):
rule['meta'][k] = rule['meta'][k].strip()
else:
rule['meta'][k] = rule[k]
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this out of the loop as there's no need to check the entirety of self.rules upon every iteration - it can wait until after the loop completes. Also the continue on the last line of the loop was redundant.

Comment on lines -71 to +95
raise SyntaxError("Sanity check of correlation rules failed, aborting.")
raise SyntaxError("Sanity check of correlation rules failed.")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing "aborting" as this isn't the right place to report state. This initialisation function doesn't know where it is being called from and hasn't "started" anything which could be "aborted" (yet). Also, for comparison, the raise above doesn't say "aborting" yet would have the same effect by raising during init.

@@ -332,6 +418,7 @@ def event_keep(self, event: dict, field: str, patterns: str, patterntype: str) -
if value == pattern:
return False
else:
ret = False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure the absence of this line was a bug. Looking at the conditional above fir comparison, the ret needs to be initialised. Otherwise, in theory, ret could be set to True during one iteration of the loop, then fall through here on the next iteration without being reset.

@@ -735,71 +894,97 @@ def create_correlation(self, rule: dict, data: list, readonly=False) -> bool:

return True

# Syntax-check rules
def check_ruleset_validity(self, rules: list) -> bool:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes from here to end of the file are simply splitting the check_ruleset_validity function into two functions:

  • check_ruleset_validity
  • check_rule_validity

The former loops through rules and validates them with the latter.

This remove some of the levels of nesting and offers a function for validating a single rule, rather than all rules, if desired.

@bcoles
Copy link
Contributor Author

bcoles commented Apr 29, 2022

Failed test is related to DNS as usual. Other tests all passed.

Comment on lines +427 to -429
self.__setStatus("FINISHED", None, time.time() * 1000)
self.runCorrelations()
self.__sf.status(f"Scan [{self.__scanId}] completed.")
self.__setStatus("FINISHED", None, time.time() * 1000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scans and correlations are two different things. Once a scan is complete we can run correlations as many times as we want. Updating the scan status to FINISHED before running correlations makes sense, and lets the Correlator validate that the scan has been completed to prevent attempting to run correlations on running scans.

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2022

Codecov Report

Merging #1654 (87e1752) into master (21ce2d9) will decrease coverage by 0.00%.
The diff coverage is 57.69%.

@@            Coverage Diff             @@
##           master    #1654      +/-   ##
==========================================
- Coverage   52.92%   52.91%   -0.01%     
==========================================
  Files         501      501              
  Lines       42896    42923      +27     
==========================================
+ Hits        22702    22714      +12     
- Misses      20194    20209      +15     
Impacted Files Coverage Δ
sf.py 50.12% <37.03%> (-2.06%) ⬇️
spiderfoot/correlation.py 61.12% <58.40%> (-0.03%) ⬇️
spiderfoot/helpers.py 72.90% <86.66%> (+0.87%) ⬆️
sfscan.py 77.34% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@bcoles bcoles changed the title SpiderFootCorrelator: Add annotations and docstrings SpiderFootCorrelator: Add error handling, annotations, docstrings May 1, 2022
@smicallef smicallef merged commit 0494c35 into smicallef:master May 1, 2022
@bcoles bcoles deleted the spiderfoot-correlation branch May 1, 2022 22:50
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.

None yet

3 participants