Two follow-up fixes to the initial ReDoS patch (CodeQL py/polynomial-redos
#1897), raised during code review:
--- Fix 1: _PREFIX_DECL regression — inline prologues and CRLF (#review-1) ---
The first ReDoS fix replaced the ambiguous trailing \s* with [ \t]*(?:\n|$),
but that introduced a behavioral regression:
* Inline prologues — PREFIX ex: <...> SELECT ... on a single line were no
longer stripped because the mandatory (?:\n|$) anchor never matched when
non-whitespace content followed the IRI on the same line.
* CRLF line endings — PREFIX ex: <...>\r\n failed because \r is not in
[ \t]* and the anchor expected a bare \n.
Root cause: the end-of-line anchor was unnecessary; the only thing needed
to eliminate backtracking ambiguity is ensuring the IRI body character class
and the trailing whitespace quantifier are disjoint.
Fix: change the IRI body from <[^>]*> to <[^>\r\n]*>, which:
- excludes CR and LF from the IRI match (semantically correct — SPARQL
IRIs cannot span line boundaries)
- makes [^>\r\n]* and the trailing [ \t]* have zero character overlap,
eliminating all backtracking ambiguity without any end-of-line anchor
No anchor is used, so both inline prologues and CRLF/LF endings work
naturally. ReDoS payloads (base< + !< x 10,000) still complete in <1 ms.
--- Fix 2: oversized-query length guard obscured error (#review-2) ---
The initial patch placed the _SPARQL_MAX_QUERY_LEN guard inside
_is_read_only_query(), which caused execute_sparql() to return the same
generic 'Only SELECT' error for both genuinely disallowed query types and
oversized inputs. Clients could not distinguish the two rejection reasons.
Fix: move the length check out of _is_read_only_query() and into
execute_sparql() as an explicit early gate, alongside the other resource
limits (_SPARQL_MAX_ROWS, _SPARQL_MAX_GRAPH_NODES). Oversized queries now
return a specific message naming the limit, the received length, and the
remediation step. _is_read_only_query() is documented to be length-agnostic.
_SPARQL_MAX_QUERY_LEN is relocated to the resource-limits block with the
other constants.
--- Tests added ---
tests/test_security_regression.py:
- test_inline_prefix_before_select_allowed (Fix 1 regression)
- test_crlf_line_endings_with_prefix (Fix 1 regression)
- test_crlf_multiple_prefixes_then_select (Fix 1 regression)
- test_inline_prefix_before_insert_still_blocked (Fix 1 security check)
- test_long_valid_query_not_rejected_by_is_read_only (Fix 2 separation)
tests/explorer/test_sparql_route.py:
- test_oversized_query_returns_distinct_length_error (Fix 2 error message)
- test_oversized_query_never_touches_the_graph (Fix 2 short-circuit)
- test_query_exactly_at_length_limit_is_accepted (Fix 2 boundary)
All 82 tests pass.
Two issues in the last round of commits:
1. explorer/auth.py added a new, opt-in APIKeyAuthMiddleware
(EXPLORER_API_KEY) and wired it into create_app(), but in doing so
removed the Depends(require_auth) dependency from every router and
deleted the /ws/graph-updates handshake check entirely. The new
middleware also fails OPEN (allows all requests) when its key is
unset, the opposite of require_auth's fail-closed design. Since
GHSA-j4mq-hprp-987v (the unauthenticated-Explorer-API advisory) is
already merged into main via require_auth, this would have reverted
a merged Critical fix the moment this branch merges. Removed
explorer/auth.py, restored the per-router dependencies and the
WebSocket auth check. Kept auth.py's one genuine improvement (adding
X-API-Key to the CORS allow_headers list) by folding it into the
existing CORS middleware config.
2. sparql.py's new _is_read_only_query() hardening (comment/PREFIX
stripping + forbidden-keyword scan) used `#[^\n]*` to strip SPARQL
comments, but a bare '#' also appears inside standard RDF namespace
IRIs (e.g. ".../1999/02/22-rdf-syntax-ns#") — the regex struck
everything after that '#' as a "comment", corrupting the query and
rejecting any legitimate SELECT using rdf:/rdfs:-style PREFIX
declarations. Confirmed by the fact the new hardening's own inlined
test copy failed against two of its own cases. Fixed by only
treating '#' as a comment-start at line-start or after whitespace,
which distinguishes ".../ns#" (preceded by a word character) from an
actual comment (preceded by whitespace/newline in every realistic
case, including the attacker's own comment-hiding PoC). Also fixed
the companion PREFIX/BASE regex, which required a prefix-name token
between the keyword and the IRI even for bare `BASE <...>`
declarations (which have none).
tests/test_security_regression.py's SPARQL section now imports the real
_is_read_only_query instead of maintaining a parallel inlined copy that
had silently drifted from — and shared the same bug as — the real
implementation; removed its TestAPIKeyAuth class (tested the now-deleted
auth.py) since equivalent, more thorough coverage already exists in
tests/explorer/test_explorer_auth.py. Updated tests/explorer/test_sparql_route.py's
multi-statement-injection test to reflect that the keyword scan now
catches "SELECT ... ; DROP ALL" itself rather than relying on rdflib's
parser, and added a new test confirming the parser still catches
multi-statement syntax that doesn't contain any forbidden keyword.
Full explorer/vector_store/security-regression/age_store suite: 543
passed (the only failures are 6 pre-existing, unrelated Pinecone-client
mocking issues).
Sameer6305