query_graph: a non-numeric LIMIT/SKIP operand (e.g. `LIMIT $limit`) is silently dropped — the query runs unbounded and reports success

[metehanulusoy]

What happens

parse_return_or_with() accepts SKIP/LIMIT only when the next token is a number. When it is not, the clause is discarded and parsing continues as if it were never written. The query then executes with no limit and returns isError: false.

Reproduced on main (3627eff), built from source, against a 26-node fixture repo:

$ cbm cli --json query_graph --project demo --query 'MATCH (n) RETURN n.name LIMIT 1'
rows: 1

$ cbm cli --json query_graph --project demo --query 'MATCH (n) RETURN n.name LIMIT $limit'
rows: 26          # LIMIT silently ignored, isError=false

$ cbm cli --json query_graph --project demo --query 'MATCH (n) RETURN n.name SKIP $offset LIMIT 1'
rows: 26          # the orphaned operand also swallows the LIMIT that follows

$ cbm cli --json query_graph --project demo --query 'MATCH (n) RETURN n.name LIMIT abc'
rows: 26

Expected: a parse error naming the offending token. Actual: a successful response containing the entire result set.

$param is not an exotic input — it is what a Neo4j-shaped client or an LLM agent writes by default. On a real repository the same query returns every matching row up to the 100k ceiling, or dies with result exceeded 100k rows — use narrower filters or add LIMIT, which tells the user to add a LIMIT they did write.

Root cause

src/cypher/cypher.c:1821-1834:

    /* Optional SKIP */
    if (match(p, TOK_SKIP)) {
        const cbm_token_t *num = expect(p, TOK_NUMBER);
        if (num) {
            r->skip = (int)strtol(num->text, NULL, CBM_DECIMAL_BASE);
        }
    }

    /* Optional LIMIT */
    if (match(p, TOK_LIMIT)) {
        const cbm_token_t *num = expect(p, TOK_NUMBER);
        if (num) {
            r->limit = (int)strtol(num->text, NULL, CBM_DECIMAL_BASE);
        }
    }

expect() (cypher.c:492-500) fills p->error and returns NULL, but both call sites only test if (num) and fall through to *out = r; return 0;. r->limit keeps the -1 initialiser set at cypher.c:1770, which cypher.c:4823 reads as "no LIMIT":

    rb_apply_skip_limit(rb, ret->skip, ret->limit >= 0 ? ret->limit : max_rows);

Two further layers hide the failure:

1. cbm_lex() skips unknown characters (cypher.c:410-411, /* Unknown character — skip */ i++;), so $ disappears and the operand lexes as an ordinary token.
2. parse_post_where() does not require TOK_EOF after the RETURN clause, so the orphaned token is dropped as well — which is why SKIP $offset LIMIT 1 loses both clauses.

Because every sub-parse returned 0, cbm_parse() never consults p.error and the caller gets error == NULL.

Why this is the failure mode #1334 already banned

tests/test_cypher.c:410-411 says of the ORDER BY cap fix:

more keys than the modeled maximum is a loud parse error - the old failure mode (ignore the remainder, drop the LIMIT) must never come back.

Same failure mode, different route: #1334 closed the ORDER BY key-list path; the SKIP/LIMIT operand path was left open.

Fix

Propagate the expect() failure in both branches — free_return_clause(r); return CBM_NOT_FOUND; — matching the ORDER BY branch immediately above. cbm_parse() then surfaces the existing expected token type ... got ... at pos N message.

PR follows with four regression tests: three fail on main and pass with the fix, and a control asserting SKIP 2 LIMIT 10 still parses and still carries 2/10. scripts/test.sh is green locally (clean ASan+UBSan build, all suites, contract steps).

Separately, the (int)strtol(..., NULL, ...) truncation at the same lines means a literal ≥ 2^31 also lands on a wrong value, but that needs an absurd literal and is not worth its own issue; the operand check above is the reachable defect.

Environment

• main @ 3627eff, built from source with scripts/build.sh
• macOS 15 (Darwin 25.6.0), arm64, clang

[DeusData]

Thank you for the precise bounded-versus-unbounded comparison.

The source diagnosis is confirmed on current main: both SKIP and LIMIT call expect(TOK_NUMBER) but fall through after a failed expectation, leaving the no-limit sentinel in place. PR #2002 contains the focused propagation fix and reproduce-first coverage, and is already under maintainer review.

This issue will remain open until that fix lands. Thank you for flagging the visible behavior change explicitly rather than hiding it as parser cleanup.