📊 Quantitative test results for language: eng, year: 2023, size: 10K, paranoia level: 1:
🚀 Quantitative testing did not detect new false positives

Well, i'm not really familiar with the .ra format.

Here is what i would like :

(?i)[\"'`][\[\{][\s\S]*?[\]\}][\"'`][\s\S]*?(?:::(/\*.*?\*/)?jsonb?)?(?:(?:@|->?)>|<@|\?[&\|]|#>>?|[<>]|<-)|(?:(?:@|->?)>|<@|\?[&\|]|#>>?|[<>]|<-)?[\"'`][\[\{][^#\]\}]*[\]\}]+[\"'`]|\bjson_extract\b[^\(]*\([^\)]*\)

@theseion Would you find some time to look at this? You are the best with regex assembly among us.

We haven't seen any FP reports for this rules so far (that I'm aware of). I do agree with your assessment though.

According to the Postgres docs, ::jsonb is not a required prefix. For example, to_jsonb() could be used to achieve the same thing. So we cannot simply make ::jsonb a required prefix.

I fail to see how adding < and > to the operators really helps here. The result would be a boolean and the syntax would likely be detected by other rules (e.g. libinjection). The boolean could be used to mask && / || uses, of course, but I don't see that as an issue.

I propose to remove < and > from the list of operators, since they are not specific to JSON use.

Hi Max.

We hadn’t seen any false positives on this rule before the change introduced in PR #3767 (https://github.com/coreruleset/coreruleset/pull/3767/files#diff-ffff196fd301a0b25ec989e607586c033a21de733d88f24c6c31267ea2b8ed24L608).

The FP only appeared afterwards, which makes sense: the primary/anchor trigger in the chain was removed, so the remaining operator sequence now evaluates on its own and can fire in cases it previously wouldn’t. That’s not acceptable in my view.

It’s not just a < vs > issue. This rule should not trigger on any payload that contains:

(?:(?:@|->?)>|<@|\?[&\|]|#>>?|[<>]|<-)

IMO, the primary group from the previous regex should be preserved:

["'`][\[\{].*[\]\}]["'`].*

Or an optimized version :

["'`][\[\{][\s\S]*?[\]\}]["'`][\s\S]*?

@touchweb-vincent Can you add some tests for the false positives you encountered?

I've personally found a few false positives as well with the recent changes to this rule but I didn't end up getting around to fix this one specifically.

Thanks @touchweb-vincent. The problematic change in that PR was the addition of the alternation {{non_greedy_jsonb}}{{operators}}. I didn't catch that in my review. The effect is indeed that operators are now matched without any prefix or suffix. The fix would be to simply remove this additional alternation (line 38), and the obsolete definition of non_greedy_jsonb (line 34).

Adding at least one test for a naked operator would certainly be a good idea.

@fzipi Do you recall why you had added that alternation?

I don't remember now, but as long as it covers https://github.com/touchweb-vincent/coreruleset/blob/be985322b26610b24ce40ac810e0bdacdaecfa52/regex-assembly/942550.ra#L7-L22 we are probably good.

I went through the regex to make sure it matches the examples. There are a couple of things that I had to fix. I'll open a PR soon.

@touchweb-vincent Can you add some tests for the false positives you encountered?

I've personally found a few false positives as well with the recent changes to this rule but I didn't end up getting around to fix this one specifically.

Hello,

I have added two unit tests we encountered.

I've updated the PR with my changes. I've extended the list of operators, added an alternation for matching SQLite JSON path expressions and added tests for all the operators. I've also removed your change to the regex @touchweb-vincent, as I had described above, removing the newly introduced alternation already fixes the issue.

Since I've now committed to the branch, I can no longer review. @fzipi, @azurit, could you?

Ping @fzipi