Conversation
| column = TDefaultColumn() and | ||
| exists(int end | | ||
| end = getEndOfColumnPosition(0, content) and | ||
| not exists(string name | exists(TNamedColumn(name)) and content.indexOf(name + ":") < end) and |
There was a problem hiding this comment.
Doesn't this check belong in getEndOfColumnPosition? That predicate already does a very similar thing that checks the start position.
There was a problem hiding this comment.
It seems to me that getEndOfColumnPosition is doing the right thing -- it's identifying that the next column is the next column keyword or the end of the string.
The problem is that MISSING: abc could previously match both this disjunct (column = TDefaultColumn(), end = EOS, expectation = abc) and the other disjunct (column = TNamedColumn("MISSING"), start = len("MISSING: "), end = EOS, expectation = abc)
There was a problem hiding this comment.
I suppose alternatively we could replace regexpFind with a regexpMatch consuming only whitespace up front, so MISSING: abc would fail to match due to the : character
There was a problem hiding this comment.
Ah, right. I see what you mean. I think replacing regexpFind with regexpMatch would be the more "right" thing to do, but I don't dislike this solution enough to argue against your fix. 👍
There was a problem hiding this comment.
I tried using regexpMatch, but it's awkward because it's using regexpFind in order to bind more than one chunk that matches expectationRegexp() -- for example, in // $ a=b c=d that regexpFind is supposed to match twice, once starting at a and again starting at c.
| column = TDefaultColumn() and | ||
| exists(int end | | ||
| end = getEndOfColumnPosition(0, content) and | ||
| not exists(string name | exists(TNamedColumn(name)) and content.indexOf(name + ":") < end) and |
There was a problem hiding this comment.
I haven't looked too far into the machinery of InlineExpectationsTest.qll, but did this turn out to be a problem for your test? I couldn't see anything with MISSING: in it 😕 so if you have a motivating example, that would be great 👍
There was a problem hiding this comment.
Sure, here's an example that produces spurious missing-value warnings at every use of // $ MISSING::
public class Test {
public static void main1() {} // $MISSING: abc
public static void main2() {} //$MISSING:abc
public static void main3() {} //$MISSING: abc
public static void main4() {} // $MISSING:abc
} // $MISSING: abcThere was a problem hiding this comment.
Oh, I see this has been fixed by #6096 now 👍 I'm not sure if you still want this change to go in for the inline-tests? (this is more in the category of nitpicks I guess)
There was a problem hiding this comment.
This line is now superfluous and the fix commit on this PR can be removed.
There was a problem hiding this comment.
👍 rebased and dropped
d80e739 to
c8695d6
Compare
This covers the cases currently exercised in https://github.com/github/codeql-securitylab/blob/main/java/ql/src/pwntester/security/RestXSS.ql
Classes without RestController and methods without GetMapping or similar were never going to be detected.
the Optional type has now been modelled
This is required to use them in annotations
a5c602a to
7819d32
Compare
Based on #6042
These cover the cases currently exercised in https://github.com/github/codeql-securitylab/blob/main/java/ql/src/pwntester/security/RestXSS.ql particularly for Spring.
Auxiliary changes: