Enable descriptions for statement nodes #209
Conversation
capa/engine.py
Outdated
| super(And, self).__init__() | ||
| self.children = list(children) | ||
| def __init__(self, children, description=None): | ||
| super(And, self).__init__(description) |
There was a problem hiding this comment.
since description is a kwarg, i prefer specifying this via description=description
There was a problem hiding this comment.
why is it a kward? From Python documentation:
**kwargs allows you to pass keyworded variable length of arguments to a function
Isn't this used to allow the function to receive an arbitrary number of parameters? This has a fixed number of parameters, but one is optional. Or has I understood something wrong? 😕
There was a problem hiding this comment.
kwarg is the name for a parameter like foo=bar, aka a parameter with a default value. kwargs don't have to be passed in order (but typically are) when they have their keyword provided. when a function supports arbitrary kwargs, that is, when not all of the possible keywords are pre-declared, then it accepts a positional argument **kwargs that collects all of these into a dict. but, thats not whats happening here. I'm just suggesting that we use the description= prefix so that its clear we're referencing the kwarg, not a position arg (which cannot be referenced by key, at least in py2.7).
There was a problem hiding this comment.
I think I get it now. I should used the key=value format when calling parameters with a default value, right? Should we also update this in other places for consistency? I am concretely thinking about the description implementation in features (which looks the same as here).
| "type": "subscope", | ||
| "subscope": statement.scope, | ||
| } | ||
| else: |
There was a problem hiding this comment.
nice simplification here, should we keep the RuntimeError though?
There was a problem hiding this comment.
I don't think it is needed. We would have to add an if with the rest Statement subclasses (or checking that statement is an instance of Statement), but this function is called at https://github.com/Ana06/capa/blob/statement-description/capa/render/__init__.py#L94 after checking that node is an instance of Statement. So, I don't see how the error could be trigger. It would be anyway a bug as there is no input the user can provide that should run in an error like this.
|
Please also add 1-2 sentences in the documentation, e.g. here https://github.com/fireeye/capa-rules/blob/master/doc/format.md#features-block |
I'll do 😉 I though I had written this in the PR description... It seems I forgot... 👀 |
Avoid repeating code and make easier to modify.
They are not needed and complicate the code and make more difficult to add more parameters to the initialization of Statements. This produces many changes in the tests. The alternative would be to add a parameter None in all of them, which are also a lot of changes.
Enable descriptions for statement nodes such as and and or. Use of case in: mandiant/capa-rules/pull/51 Documentation should be added in capa-rules.
Avoid code repetition to make modifying this code easier.
Implement it similarly as how it is rendered for features.
Add statement descriptions to `test_rule_yaml_descriptions` to ensure rules with statement descriptions are parsed and extracted correctly.
|
Documentation in: mandiant/capa-rules#57 |
Enable descriptions for statement nodes such as
andandor. Includes some refactoring to facilitate the implementation.Use of case in: mandiant/capa-rules/pull/51
Closes #194