Skip to content

Commit ae1a0d7

Browse files
committed
Reject unsafe direct inline expectation edits
Require generated expectation atoms to round-trip through the inline grammar before emitting an edit. Let language implementations reject unsafe end-of-line appends, and conservatively disable Python appends where explicit continuation lines cannot be distinguished at this seam. Cover unsafe hash, whitespace, newline, and Python continuation cases through the public learnEdits relation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1adfa307-c48e-43f5-94e3-8286e4a8c083
1 parent 0728c0a commit ae1a0d7

7 files changed

Lines changed: 85 additions & 13 deletions

File tree

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
| Test.java | 3 | append | 0 | 0 | // $ java-direct=safe |
2+
| Test.java | 4 | replace | 25 | 37 | // $ foreign java-direct=safe |
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
11
class Test {
22
void target() {} // $ java-direct
3+
void safeAppend() {}
4+
void safeReplace() {} // $ foreign
5+
void unsafeHash() {}
6+
void unsafeSpace() {} // $ foreign
7+
void unsafeNewline() {} // $ foreign
38
}

‎java/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,34 @@ import utils.test.InlineExpectationsTest
44
module DirectTest implements TestSig {
55
string getARelevantTag() { result = "java-direct" }
66

7+
predicate tagIsIgnored(string expectedTag) { expectedTag = "foreign" }
8+
79
predicate hasActualResult(Location location, string element, string tag, string value) {
810
exists(Method method |
9-
method.getName() = "target" and
11+
method.getName() =
12+
["target", "safeAppend", "safeReplace", "unsafeHash", "unsafeSpace", "unsafeNewline"] and
1013
method.fromSource() and
1114
location = method.getLocation() and
1215
element = method.toString() and
1316
tag = "java-direct" and
14-
value = ""
17+
if method.getName() = "unsafeHash"
18+
then value = "a#b"
19+
else
20+
if method.getName() = "unsafeSpace"
21+
then value = "hello world"
22+
else
23+
if method.getName() = "unsafeNewline"
24+
then value = "hello\nworld"
25+
else
26+
if method.getName() = ["safeAppend", "safeReplace"]
27+
then value = "safe"
28+
else value = ""
1529
)
1630
}
1731
}
1832

19-
import MakeTest<DirectTest>
33+
private module Test = MakeTest<DirectTest>;
34+
35+
from string file, int line, string operation, int startColumn, int endColumn, string text
36+
where Test::learnEdits(file, line, operation, startColumn, endColumn, text)
37+
select file, line, operation, startColumn, endColumn, text

‎python/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,8 @@ module Impl implements InlineExpectationsTestSig {
3030
relativePath.regexpMatch(".*\\.(py|pyi)") and
3131
result = "#"
3232
}
33+
34+
// Python source lines are not available here, so an explicit `\` continuation cannot be excluded.
35+
bindingset[relativePath, line]
36+
predicate canAppendCommentAtEndOfLine(string relativePath, int line) { none() }
3337
}

‎python/ql/test/library-tests/InlineExpectationsTest/learn-edits/Test.ql‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ module DirectTest implements TestSig {
1313

1414
predicate hasActualResult(Location location, string element, string tag, string value) {
1515
exists(Name name |
16-
name.getId() = "target" and
16+
name.getId() = ["target", "target_continuation"] and
1717
location = name.getLocation() and
1818
element = name.toString() and
1919
tag = "direct" and
@@ -22,4 +22,8 @@ module DirectTest implements TestSig {
2222
}
2323
}
2424

25-
import MakeTest<DirectTest>
25+
private module Test = MakeTest<DirectTest>;
26+
27+
from string file, int line, string operation, int startColumn, int endColumn, string text
28+
where Test::learnEdits(file, line, operation, startColumn, endColumn, text)
29+
select file, line, operation, startColumn, endColumn, text
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
11
target = 1 # $ direct[alias] foreign
2+
target_continuation = 1 \
3+
+ 1

‎shared/util/codeql/util/test/InlineExpectationsTest.qll‎

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,12 @@ signature module InlineExpectationsTestSig {
124124
bindingset[relativePath]
125125
string getStartCommentMarker(string relativePath);
126126

127+
/**
128+
* Holds if a comment can safely be appended at the end of `line` in `relativePath`.
129+
*/
130+
bindingset[relativePath, line]
131+
default predicate canAppendCommentAtEndOfLine(string relativePath, int line) { any() }
132+
127133
/**
128134
* Gets the marker that ends a comment (for example `"-->"`) in the source file with the
129135
* given `relativePath`. Has no result by default, which is correct for languages whose
@@ -318,6 +324,7 @@ module Make<InlineExpectationsTestSig Impl> {
318324
parseRelativeUrl(Impl::getRelativeUrl(comment.getLocation()), relativePath, resultLine, _,
319325
_, _)
320326
) and
327+
Impl::canAppendCommentAtEndOfLine(relativePath, resultLine) and
321328
file = relativePath and
322329
line = resultLine and
323330
operation = "append" and
@@ -626,7 +633,8 @@ module Make<InlineExpectationsTestSig Impl> {
626633
) and
627634
parseRelativeUrl(Impl::getRelativeUrl(actualResult.getLocation()), relativePath, line, _,
628635
line, _) and
629-
text = actualResult.getExpectationText()
636+
text = actualResult.getExpectationText() and
637+
expectationAtomRoundTrips(actualResult.getTag(), actualResult.getValue(), text)
630638
)
631639
}
632640

@@ -919,6 +927,23 @@ private module ExpectationParser<InlineExpectationsTestSig Impl> {
919927
}
920928
}
921929

930+
bindingset[tag, value, text]
931+
private predicate expectationAtomRoundTrips(string tag, string value, string text) {
932+
exists(string content, string parsedTag, string parsedValue |
933+
content = ("$ " + text).regexpCapture(expectationCommentPattern(), 1).trim() and
934+
content = text and
935+
content.regexpMatch(expectationPattern()) and
936+
parsedTag = content.regexpCapture(expectationPattern(), 1) and
937+
parsedTag = tag and
938+
(
939+
parsedValue = content.regexpCapture(expectationPattern(), 2)
940+
or
941+
not exists(content.regexpCapture(expectationPattern(), 2)) and parsedValue = ""
942+
) and
943+
parsedValue = value
944+
)
945+
}
946+
922947
/**
923948
* Provides logic for creating a `@kind test-postprocess` query that checks
924949
* inline test expectations using `$ Alert` markers.
@@ -1564,21 +1589,29 @@ module TestPostProcessing {
15641589
}
15651590

15661591
/**
1567-
* Renders a current-query atom in its canonical cohort form: bare when every member produces
1568-
* it, otherwise qualified with the current query ID.
1592+
* Renders a current-query tag in its canonical cohort form: bare when every member produces
1593+
* the atom, otherwise qualified with the current query ID.
15691594
*/
15701595
bindingset[relativePath, line, tag, value]
1571-
private string renderCohortAtom(string relativePath, int line, string tag, string value) {
1596+
private string renderCohortTag(string relativePath, int line, string tag, string value) {
15721597
(
15731598
not inCohortMode()
15741599
or
15751600
isCollapsedAtom(relativePath, line, tag, value)
15761601
) and
1577-
result = renderAtom(tag, value)
1602+
result = tag
15781603
or
15791604
inCohortMode() and
15801605
not isCollapsedAtom(relativePath, line, tag, value) and
1581-
result = renderAtom(tag + "[" + getQueryId() + "]", value)
1606+
result = tag + "[" + getQueryId() + "]"
1607+
}
1608+
1609+
/**
1610+
* Renders a current-query atom in its canonical cohort form.
1611+
*/
1612+
bindingset[relativePath, line, tag, value]
1613+
private string renderCohortAtom(string relativePath, int line, string tag, string value) {
1614+
result = renderAtom(renderCohortTag(relativePath, line, tag, value), value)
15821615
}
15831616

15841617
/**
@@ -1699,8 +1732,12 @@ module TestPostProcessing {
16991732
) and
17001733
parseRelativeUrl(actualResult.getLocation().getRelativeUrl(), relativePath, endLine, _,
17011734
endLine, _) and
1702-
text =
1703-
renderCohortAtom(relativePath, endLine, actualResult.getTag(), actualResult.getValue())
1735+
exists(string renderedTag |
1736+
renderedTag =
1737+
renderCohortTag(relativePath, endLine, actualResult.getTag(), actualResult.getValue()) and
1738+
text = renderAtom(renderedTag, actualResult.getValue()) and
1739+
expectationAtomRoundTrips(renderedTag, actualResult.getValue(), text)
1740+
)
17041741
)
17051742
}
17061743

0 commit comments

Comments
 (0)