Merge pull request DeusData#1875 from CaptainMittens/fix/cypher-reject-trailing-tokens

fix(cypher): refuse a query the parser did not fully read

Author: DeusData

src/cypher/cypher.c

@@ -1992,6 +1992,13 @@ static int parse_post_where(parser_t *p, cbm_query_t *q, // NOLINT(misc-no-recur
         q->union_next = sub.query;
         sub.query = NULL;
         cbm_parse_free(&sub);
+        /* The branch after UNION was parsed by a SEPARATE parser over a slice
+         * of these tokens, so this parser's cursor never moved past the UNION
+         * keyword. That sub-parse now refuses to succeed with anything left
+         * over, so everything from here to the end is accounted for. Move the
+         * cursor to the end to say so, or cbm_parse's end-of-input check reads
+         * a fully parsed UNION query as unfinished. */
+        p->pos = p->count;
     }
     return 0;
 }
@@ -2055,6 +2062,36 @@ int cbm_parse(const cbm_token_t *tokens, int token_count, // NOLINT(misc-no-recu
         return CBM_NOT_FOUND;
     }
 
+    /* Every token must be consumed. The grammar accepts at most one WITH and
+     * treats RETURN as optional, so a query with a second WITH stage — or any
+     * typo after RETURN — used to stop parsing there and succeed anyway. The
+     * dropped tail took the filter and the RETURN with it, and the engine
+     * answered from the fragment it had parsed, using its default projection.
+     * That reported success and returned wrong rows, which is worse than a
+     * refusal because nothing tells the caller to look. Refuse instead. */
+    if (peek(&p)->type != TOK_EOF) {
+        /* Only mention the one-WITH limit when a standalone WITH really is
+         * sitting in the part we could not read. Saying it every time points
+         * a reader at WITH when the problem is a typo. A WITH straight after
+         * STARTS is the STARTS WITH operator rather than a clause, the same
+         * guard parse_post_where uses. */
+        const char *hint = "";
+        for (int i = p.pos; i < p.count; i++) {
+            if (p.tokens[i].type == TOK_WITH &&
+                (i == 0 || p.tokens[i - SKIP_ONE].type != TOK_STARTS)) {
+                hint = " Note that only one WITH clause is supported.";
+                break;
+            }
+        }
+        snprintf(p.error, sizeof(p.error),
+                 "unexpected input at pos %d ('%s') — the query was not fully "
+                 "parsed.%s",
+                 peek(&p)->pos, peek(&p)->text ? peek(&p)->text : "", hint);
+        out->error = heap_strdup(p.error);
+        cbm_query_free(q);
+        return CBM_NOT_FOUND;
+    }
+
     out->query = q;
     return 0;
 }

tests/test_cypher.c

@@ -209,6 +209,71 @@ TEST(cypher_parse_simple_node) {
     PASS();
 }
 
+/* Trailing input must be an error, never a silent drop. The parser used to
+ * stop at the first thing it did not understand and report success, so the
+ * engine answered from the fragment it had parsed. */
+TEST(cypher_parse_rejects_trailing_tokens) {
+    cbm_query_t *q = NULL;
+    char *err = NULL;
+    int rc = cbm_cypher_parse("MATCH (f:Function) RETURN f.name AS n BANANA SPLIT 99", &q, &err);
+    ASSERT_NEQ(rc, 0);
+    ASSERT_NOT_NULL(err);
+    ASSERT_NULL(q);
+
+    /* The message must name what actually stopped the parse, and must not
+     * mention WITH: this query has no WITH in it anywhere. */
+    ASSERT(strstr(err, "BANANA") != NULL);
+    ASSERT(strstr(err, "WITH") == NULL);
+
+    free(err);
+    PASS();
+}
+
+/* Only one WITH is supported. A second one used to take the rest of the
+ * query with it — the filter and the RETURN both vanished, and every row
+ * came back unfiltered under the default projection. */
+TEST(cypher_parse_rejects_second_with_clause) {
+    cbm_query_t *q = NULL;
+    char *err = NULL;
+    int rc = cbm_cypher_parse("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",
+                              &q, &err);
+    ASSERT_NEQ(rc, 0);
+    ASSERT_NOT_NULL(err);
+    ASSERT_NULL(q);
+
+    /* Here the note earns its place. The parse stops at OPTIONAL, and the
+     * reason is the second WITH further along, which the reader cannot see
+     * from the stopping point alone. */
+    ASSERT(strstr(err, "only one WITH clause is supported") != NULL);
+
+    free(err);
+    PASS();
+}
+
+/* The guard must not reject a query that is simply finished. One WITH, a
+ * WHERE after it and a RETURN is the shape the grammar does support. */
+TEST(cypher_parse_accepts_single_with_clause) {
+    cbm_query_t *q = NULL;
+    char *err = NULL;
+    int rc = cbm_cypher_parse("MATCH (f:Function) "
+                              "WITH f, f.name AS n "
+                              "WHERE n = 'buildTree' "
+                              "RETURN n",
+                              &q, &err);
+    ASSERT_EQ(rc, 0);
+    ASSERT_NULL(err);
+    ASSERT_NOT_NULL(q);
+
+    cbm_query_free(q);
+    PASS();
+}
+
 TEST(cypher_parse_relationship_outbound) {
     cbm_query_t *q = NULL;
     char *err = NULL;
@@ -4102,6 +4167,9 @@ SUITE(cypher) {
     RUN_TEST(cypher_lex_full_query);
     /* Parser */
     RUN_TEST(cypher_parse_simple_node);
+    RUN_TEST(cypher_parse_rejects_trailing_tokens);
+    RUN_TEST(cypher_parse_rejects_second_with_clause);
+    RUN_TEST(cypher_parse_accepts_single_with_clause);
     RUN_TEST(cypher_parse_relationship_outbound);
     RUN_TEST(cypher_parse_relationship_inbound);
     RUN_TEST(cypher_parse_relationship_any);