Skip to content

Commit c0a9671

Browse files
fix(cypher): refuse a query the parser did not fully read
cbm_parse built a query and returned success without checking that it had read every token. The grammar accepts at most one WITH and treats RETURN as optional, so the parser stopped at the first thing it did not understand and reported success anyway. The dropped tail took the filter and the RETURN with it. The engine then answered from the fragment it had parsed, using its default projection. It reported success and returned wrong rows, which is worse than a refusal, because nothing tells the caller to look. Two shapes hit this: MATCH (f:Function) WHERE f.name = 'x' RETURN f.name AS n BANANA SPLIT 99 -> one row, no error, the trailing words silently dropped MATCH (f:Function) OPTIONAL MATCH (a)-[:CALLS]->(f) WITH f, count(a) AS calls OPTIONAL MATCH (b)-[:USAGE]->(f) WITH f, calls, count(b) AS usages WHERE calls = 0 AND usages = 0 RETURN f.name AS n -> every function, unfiltered, under the default column names cbm_parse now checks that the cursor sits on the end-of-input token before it returns success, and names the leftover token when it does not. The message also says that only one WITH clause is supported, because that is the limit people actually meet. The check exposed a second defect. parse_post_where parses the branch after UNION by calling cbm_parse on a slice of the same tokens, using a separate parser. The outer parser's cursor never moved past the UNION keyword, so a valid UNION query looked unfinished. That cursor was already wrong; nothing caught it, because nothing checked where the cursor ended up. The UNION branch now moves the cursor to the end after the sub-parse succeeds, which is sound because that sub-parse no longer returns success with tokens left over. Three tests cover both directions. Removing only the guard turns the two rejection tests red with rc == 0 -- the parser reporting success on input it never finished reading -- while the acceptance test stays green, so the guard is load-bearing for exactly these two behaviours. Verified on macOS with Apple clang: make -f Makefile.cbm test-focused TEST_SUITES=cypher -> 186 passed guard removed, tests kept -> 184 passed, 2 failed make -f Makefile.cbm cbm -> exit 0, no warnings The full suite reports 7623 passed, 2 failed. Both failures are in tests/test_cli.c (lines 1748 and 6723) and reproduce on a clean tree with this change stashed out. They depend on the coding agents installed on the machine, not on this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
1 parent 4149c50 commit c0a9671

2 files changed

Lines changed: 82 additions & 0 deletions

File tree

‎src/cypher/cypher.c‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1992,6 +1992,13 @@ static int parse_post_where(parser_t *p, cbm_query_t *q, // NOLINT(misc-no-recur
19921992
q->union_next = sub.query;
19931993
sub.query = NULL;
19941994
cbm_parse_free(&sub);
1995+
/* The branch after UNION was parsed by a SEPARATE parser over a slice
1996+
* of these tokens, so this parser's cursor never moved past the UNION
1997+
* keyword. That sub-parse now refuses to succeed with anything left
1998+
* over, so everything from here to the end is accounted for. Move the
1999+
* cursor to the end to say so, or cbm_parse's end-of-input check reads
2000+
* a fully parsed UNION query as unfinished. */
2001+
p->pos = p->count;
19952002
}
19962003
return 0;
19972004
}
@@ -2055,6 +2062,23 @@ int cbm_parse(const cbm_token_t *tokens, int token_count, // NOLINT(misc-no-recu
20552062
return CBM_NOT_FOUND;
20562063
}
20572064

2065+
/* Every token must be consumed. The grammar accepts at most one WITH and
2066+
* treats RETURN as optional, so a query with a second WITH stage — or any
2067+
* typo after RETURN — used to stop parsing there and succeed anyway. The
2068+
* dropped tail took the filter and the RETURN with it, and the engine
2069+
* answered from the fragment it had parsed, using its default projection.
2070+
* That reported success and returned wrong rows, which is worse than a
2071+
* refusal because nothing tells the caller to look. Refuse instead. */
2072+
if (peek(&p)->type != TOK_EOF) {
2073+
snprintf(p.error, sizeof(p.error),
2074+
"unexpected input at pos %d ('%s') — the query was not fully "
2075+
"parsed. Note that only one WITH clause is supported.",
2076+
peek(&p)->pos, peek(&p)->text ? peek(&p)->text : "");
2077+
out->error = heap_strdup(p.error);
2078+
cbm_query_free(q);
2079+
return CBM_NOT_FOUND;
2080+
}
2081+
20582082
out->query = q;
20592083
return 0;
20602084
}

‎tests/test_cypher.c‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,61 @@ TEST(cypher_parse_simple_node) {
209209
PASS();
210210
}
211211

212+
/* Trailing input must be an error, never a silent drop. The parser used to
213+
* stop at the first thing it did not understand and report success, so the
214+
* engine answered from the fragment it had parsed. */
215+
TEST(cypher_parse_rejects_trailing_tokens) {
216+
cbm_query_t *q = NULL;
217+
char *err = NULL;
218+
int rc = cbm_cypher_parse("MATCH (f:Function) RETURN f.name AS n BANANA SPLIT 99", &q, &err);
219+
ASSERT_NEQ(rc, 0);
220+
ASSERT_NOT_NULL(err);
221+
ASSERT_NULL(q);
222+
223+
free(err);
224+
PASS();
225+
}
226+
227+
/* Only one WITH is supported. A second one used to take the rest of the
228+
* query with it — the filter and the RETURN both vanished, and every row
229+
* came back unfiltered under the default projection. */
230+
TEST(cypher_parse_rejects_second_with_clause) {
231+
cbm_query_t *q = NULL;
232+
char *err = NULL;
233+
int rc = cbm_cypher_parse("MATCH (f:Function) "
234+
"OPTIONAL MATCH (a)-[:CALLS]->(f) "
235+
"WITH f, count(a) AS calls "
236+
"OPTIONAL MATCH (b)-[:USAGE]->(f) "
237+
"WITH f, calls, count(b) AS usages "
238+
"WHERE calls = 0 AND usages = 0 "
239+
"RETURN f.name AS n",
240+
&q, &err);
241+
ASSERT_NEQ(rc, 0);
242+
ASSERT_NOT_NULL(err);
243+
ASSERT_NULL(q);
244+
245+
free(err);
246+
PASS();
247+
}
248+
249+
/* The guard must not reject a query that is simply finished. One WITH, a
250+
* WHERE after it and a RETURN is the shape the grammar does support. */
251+
TEST(cypher_parse_accepts_single_with_clause) {
252+
cbm_query_t *q = NULL;
253+
char *err = NULL;
254+
int rc = cbm_cypher_parse("MATCH (f:Function) "
255+
"WITH f, f.name AS n "
256+
"WHERE n = 'buildTree' "
257+
"RETURN n",
258+
&q, &err);
259+
ASSERT_EQ(rc, 0);
260+
ASSERT_NULL(err);
261+
ASSERT_NOT_NULL(q);
262+
263+
cbm_query_free(q);
264+
PASS();
265+
}
266+
212267
TEST(cypher_parse_relationship_outbound) {
213268
cbm_query_t *q = NULL;
214269
char *err = NULL;
@@ -4044,6 +4099,9 @@ SUITE(cypher) {
40444099
RUN_TEST(cypher_lex_full_query);
40454100
/* Parser */
40464101
RUN_TEST(cypher_parse_simple_node);
4102+
RUN_TEST(cypher_parse_rejects_trailing_tokens);
4103+
RUN_TEST(cypher_parse_rejects_second_with_clause);
4104+
RUN_TEST(cypher_parse_accepts_single_with_clause);
40474105
RUN_TEST(cypher_parse_relationship_outbound);
40484106
RUN_TEST(cypher_parse_relationship_inbound);
40494107
RUN_TEST(cypher_parse_relationship_any);

0 commit comments

Comments
 (0)