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

Not does not apply to all operators #58

Closed
jmgnc opened this issue Jul 5, 2017 · 7 comments
Closed

Not does not apply to all operators #58

jmgnc opened this issue Jul 5, 2017 · 7 comments

Comments

@jmgnc
Copy link

jmgnc commented Jul 5, 2017

https://github.com/oasis-open/cti-stix2-json-schemas/blob/master/pattern_grammar/STIXPattern.g4#L45

Not should apply to = and != as well as the ordering operators.

Though their use here does not make sense, there is nothing in the spec that prevents their use. I don't see a good reason to restrict them either.

@gtback
Copy link
Contributor

gtback commented Jul 6, 2017

A good (though not great) reason to restrict them is that the use of NOT in front of =, !=, <, <=, >, or >= amounts to more than one way to do something, which we try to avoid.

I agree that, as written, the spec does not prohibit NOT != (or the others), but in my opinion we should strongly discourage their use. That said, the pattern grammar is somewhat lax, in that it doesn't (on its own) validate correct object types or property names, so there is something to be said for allowing NOT != in the grammar, but raising warnings/errors in the STIX-Validator.

Since this is an open repository, we can't change/influence the specs here; we just need to implement what is specified.

@chisholm @clenk @ikiril01 @treyka What do you think?

@treyka
Copy link

treyka commented Jul 6, 2017

I would disagree that we should restrict the use of NOT before any operators permitted by the Patterning specification. It's not two different ways of doing the same thing; that's just how boolean logic works.

Which is to say that it's not(not(not))) two different ways of doing the same thing. ;-)

@chisholm
Copy link
Contributor

chisholm commented Jul 6, 2017

I'd say boolean logic allows multiple ways of doing the same thing. E.g. A=>B <=> !A | B, A <=> !!A <=> !!!!A, !(A|B) <=> !A & !B, etc, etc...

I'd be all for allowing the full range of propositional logic expression. But let's face it, NOT != isn't exactly a standard notation. In fact it's really weird, and surely anyone looking at it would wonder what the author was thinking when he wrote it.

In this case, we have lots of custom operators (i.e. beyond those of propositional logic), and inserting NOT before the operator in some cases actually reads better to humans. A NOT LIKE B reads more like a sentence, so it makes sense in terms of human understandability. A NOT != B is going make people squint and scratch their heads. I guess that's why I wrote the grammar that way. It was making me squint and scratch my head :-P

So yeah, it's not consistent across all operators, but I guess it made more sense to me to be inconsistent. I guess we gotta be totally spec-compliant, but I do think the spec allows some weird patterns :)

@gtback
Copy link
Contributor

gtback commented Jul 6, 2017

that's just how boolean logic works.

Unless I'm misunderstanding/misremembering the spec, NOT really (pun intended 😁 ). The spec doesn't allow <comparison expression A> AND NOT (<comparison expression B> OR <comparison expression C>). In effect, we can only apply NOT to "leaf nodes", which is equivalent to using the opposite operator in the comparison.

That does raise the issue (which I thought we resolved but now can't find in the spec) of whether a object that is missing property will ever match a comparison expression using that property. Thus, given a foo object {bar: 42}, the pattern [foo:bar = 42] would match. But what about [foo:qux != 37]? Should that be interpreted differently than [foo:qux NOT = 37] ? There's also the issue of incompatible types. [foo:bar > 'tacos'] is False, and [foo:bar <= 'tacos'] is also False, so does that mean that [foo:bar NOT > 'tacos'] is True?

Regardless of the answers to the above questions, I agree that the grammar needs to be updated to support what's in the spec (and the matcher updated accordingly), and if we want to discourage diabolical uses of NOT, we should do that elsewhere.

@jmgnc
Copy link
Author

jmgnc commented Jul 7, 2017

While I agree that NOT != is not friendly, as is pointed out, you can do similar things in many languages, even python allows not (a == b). We do have a bit more context due to where we put the NOT though than other languages.

This is correct that NOT only applies to the results of the comparison. We have not defined what NOT would result in, and is invalid.

Re missing properties. I'm pretty sure we had text in the document, but I do not see it. IIRC, not present is somewhat like the empty set, so it's never be equal, but always !=. and in that case NOT = and != I would expect to return the same.

It is known that we do not have a function to test the presence of a missing property. The real question is, does missingprop MATCHES '.*' return true or false? (and what happens when missingprop is present but not a string?) And we do not have this defined in the spec. I thought we had resolved this too and added text, but it is not present.

@chisholm
Copy link
Contributor

chisholm commented Jul 7, 2017

you can do similar things in many languages, even python allows not (a == b).

Actually, I was thinking of the customary unary prefix NOT operator as being the contrasting approach, not the similar, comparable one. not (a != b) actually seems more reasonable to me, perhaps because it's negating a whole sub-expression, as opposed to the juxtaposed NOT != which looks like a confusing and unnecessary double-negation. Unary prefix NOT seems like a general purpose tool for logically negating arbitrary sub-expressions, which makes sense to have. NOT != just looks like a clumsy confusing syntax.

As far as missing properties, the philosophy embodied in the matcher is that an object path is basically a selector of values from cyber observable objects, which will participate in the comparison. Having a non-existent property means that no values are selected, which means that no comparison can happen. There is nothing to compare. Therefore, there are no "root" cyber observable objects, and the ramifications flow from that. E.g. if that was the only comparison expression within the observation expression (e.g. within the square brackets), then it won't match the observation; the comparison operator is irrelevant (even if it was '!=').

If the spec were to say the result was true when the operator was '!=' and the property was not found, it would effectively be saying there is a match without a matching cyber observable object. How do you resolve that idea against the common root cyber observable constraint? It seems contradictory. Consider a compound comparison expression:

[foo:missingprop != 1 AND foo:name = 'alice']

The first evals to true, and let's say the second does too, because the prop exists with the given value. Since they're ANDed, one might think the whole thing must eval to true, and match. But what is the common root cyber observable here? The first comparison expression didn't match any. So there can't be a common cyber observable. That suggests that there can be no match. So is it a match or not? If a match, then what do you do with the common root cyber observable constraint?

As far as type mismatches, 4.2.1 does state that if the types are incompatible, '!=' results in true, all other operators result in false. The matcher follows this: if the selected value isn't a string, the MATCHES operator will yield false.

@gtback
Copy link
Contributor

gtback commented Jul 12, 2017

Fixed by #59

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

No branches or pull requests

4 participants