diff --git a/semantica/explorer/routes/sparql.py b/semantica/explorer/routes/sparql.py index 28d14829..08c8af1d 100644 --- a/semantica/explorer/routes/sparql.py +++ b/semantica/explorer/routes/sparql.py @@ -52,23 +52,27 @@ _FORBIDDEN_KEYWORDS = re.compile( # the IRI (`BASE <...>`, vs. `PREFIX ex: <...>`), so the prefix-name token # is optional. # -# ReDoS note: the original pattern ended with `<[^>]*>\s*` where the -# trailing `\s*` could overlap with the `[^>]*` character class on inputs -# that contain no closing `>`, causing polynomial backtracking (CodeQL -# py/polynomial-redos, issue #1897). The fix replaces the ambiguous `\s*` -# suffix with `[ \t]*(?:\n|$)` which matches only horizontal whitespace -# followed by a hard line boundary, so there is no character-class overlap -# and the engine cannot split the match in multiple ways. +# ReDoS fix (CodeQL py/polynomial-redos, issue #1897): +# +# The original pattern `<[^>]*>\s*` was vulnerable because `\s*` (which +# matches newlines) could overlap with `[^>]*` on inputs that contain no +# closing `>` (e.g. `base\r\n]*>` for the IRI body: excluding CR and LF from +# the character class means the IRI match can never span a line boundary, +# and the disjoint trailing `[ \t]*` (horizontal whitespace only) has zero +# character-class overlap with `[^>\r\n]*`, so the engine has exactly one +# way to match. No end-of-line anchor is needed or used, which correctly +# handles both inline prologues (`PREFIX ex: <...> SELECT ...` on one line) +# and CRLF line endings (`\r\n`) without any special casing. _COMMENT_LINE = re.compile(r"(?:^|(?<=\s))#[^\n]*", re.MULTILINE) _PREFIX_DECL = re.compile( - r"^[ \t]*(?:PREFIX[ \t]+\S+|BASE)[ \t]*<[^>]*>[ \t]*(?:\n|$)", + r"^[ \t]*(?:PREFIX[ \t]+\S+|BASE)[ \t]*<[^>\r\n]*>[ \t]*", re.IGNORECASE | re.MULTILINE, ) -_SPARQL_MAX_QUERY_LEN = 10_000 # chars; guards regex cost on uncontrolled input - - def _is_read_only_query(query: str) -> bool: """Return True only for genuine read-only SPARQL queries. @@ -76,12 +80,11 @@ def _is_read_only_query(query: str) -> bool: checking the first keyword. Also rejects queries containing SPARQL Update keywords anywhere in the body, preventing injection via embedded strings or multi-statement tricks. - """ - # 0. Reject excessively long inputs before any regex work (defense-in-depth - # against ReDoS even if a future regex change reintroduces ambiguity). - if len(query) > _SPARQL_MAX_QUERY_LEN: - return False + Note: callers are responsible for enforcing any input-length limit *before* + calling this function so that an oversized-query rejection can be surfaced + as a distinct, actionable error rather than the generic read-only message. + """ # 1. Remove single-line comments that could hide the real query type cleaned = _COMMENT_LINE.sub("", query) # 2. Remove PREFIX/BASE declarations @@ -175,6 +178,11 @@ _SPARQL_MAX_ROWS = 5_000 # hard cap on returned rows _SPARQL_TIMEOUT_S = 30 # seconds before abandoning the await _SPARQL_MAX_CONCURRENT = 4 # semaphore: max simultaneous executions _SPARQL_MAX_GRAPH_NODES = 50_000 # cap on graph nodes/edges to prevent OOM +# Defense-in-depth against ReDoS: reject inputs longer than this before any +# regex work so that even a future regex regression is bounded. Checked in +# execute_sparql() (not inside _is_read_only_query) so the route can return +# a distinct, actionable error message rather than the generic read-only one. +_SPARQL_MAX_QUERY_LEN = 10_000 # chars # Semaphore caps how many graph.query calls run concurrently so that # timed-out threads (which keep running in the pool) cannot crowd out @@ -203,6 +211,24 @@ async def execute_sparql( req: SparqlRequest, session: GraphSession = Depends(get_session), ): + # Resource-limit check: reject oversized queries before any regex work. + # This is intentionally a separate, earlier check from _is_read_only_query + # so clients receive a specific, actionable message rather than the generic + # read-only rejection, and operators can tune _SPARQL_MAX_QUERY_LEN without + # touching query-semantics code. + if len(req.query) > _SPARQL_MAX_QUERY_LEN: + return SparqlResponse( + columns=[], + rows=[], + total=0, + error=( + f"Query exceeds the maximum allowed length of " + f"{_SPARQL_MAX_QUERY_LEN:,} characters " + f"({len(req.query):,} received). " + f"Please shorten your query." + ), + ) + if not _is_read_only_query(req.query): return SparqlResponse( columns=[], diff --git a/tests/explorer/test_sparql_route.py b/tests/explorer/test_sparql_route.py index 1d8547c6..1f619aa7 100644 --- a/tests/explorer/test_sparql_route.py +++ b/tests/explorer/test_sparql_route.py @@ -317,6 +317,47 @@ def test_oversized_graph_returns_clean_error_not_a_crash(client): assert payload["rows"] == [] +def test_oversized_query_returns_distinct_length_error(client): + """A query exceeding _SPARQL_MAX_QUERY_LEN must be rejected with a + specific, actionable error message — not the generic read-only message. + Clients need to distinguish a size-limit rejection from an actual + non-read-only query rejection to react correctly (e.g. split the query + vs. rewrite it).""" + with patch.object(sparql_mod, "_SPARQL_MAX_QUERY_LEN", 10): + resp = _post(client, "SELECT ?s WHERE { ?s ?p ?o }") # 30 chars > 10 + assert resp.status_code == 200 + payload = resp.json() + assert payload["error"] is not None + # Must mention the limit, not the generic read-only message + assert "length" in payload["error"].lower() or "characters" in payload["error"].lower() + assert "Only SELECT" not in payload["error"] + assert payload["rows"] == [] + assert payload["columns"] == [] + assert payload["total"] == 0 + + +def test_oversized_query_never_touches_the_graph(client): + """An oversized query must be rejected before _build_rdflib_graph is + called — the length guard must short-circuit the entire pipeline.""" + with patch.object(sparql_mod, "_SPARQL_MAX_QUERY_LEN", 10): + with patch.object(sparql_mod, "_build_rdflib_graph") as mock_build: + resp = _post(client, "SELECT ?s WHERE { ?s ?p ?o }") + assert resp.status_code == 200 + assert resp.json()["error"] is not None + mock_build.assert_not_called() + + +def test_query_exactly_at_length_limit_is_accepted(client): + """A query whose length equals the limit exactly must not be rejected — + the guard is strictly greater-than, not greater-than-or-equal.""" + short_query = "ASK {}" + with patch.object(sparql_mod, "_SPARQL_MAX_QUERY_LEN", len(short_query)): + resp = _post(client, short_query) + assert resp.status_code == 200 + payload = resp.json() + assert payload["error"] is None + + # --------------------------------------------------------------------------- # Data-mapping fidelity: does the graph->RDF projection reflect session state? # --------------------------------------------------------------------------- diff --git a/tests/test_security_regression.py b/tests/test_security_regression.py index 18854f0b..56ad46de 100644 --- a/tests/test_security_regression.py +++ b/tests/test_security_regression.py @@ -123,6 +123,43 @@ class TestSparqlReadOnlyValidation: ) assert _is_read_only_query(query) + def test_inline_prefix_before_select_allowed(self): + """PREFIX declaration on the same line as the query keyword (inline + prologue) must be stripped correctly so SELECT is seen first. + Regression for the [ \\t]*(?:\\n|$) anchor that rejected this form.""" + query = "PREFIX ex: SELECT ?s WHERE { ?s ex:p ?o }" + assert _is_read_only_query(query) + + def test_crlf_line_endings_with_prefix(self): + """Windows CRLF line endings (\\r\\n) between PREFIX and SELECT must + be handled correctly. The previous (?:\\n|$) anchor did not allow + the \\r before \\n, causing stripping to fail.""" + query = "PREFIX ex: \r\nSELECT ?s WHERE { ?s ?p ?o }" + assert _is_read_only_query(query) + + def test_crlf_multiple_prefixes_then_select(self): + """Multiple PREFIX lines with CRLF endings should all be stripped.""" + query = ( + "PREFIX rdf: \r\n" + "PREFIX ex: \r\n" + "SELECT ?s WHERE { ?s rdf:type ex:Thing }" + ) + assert _is_read_only_query(query) + + def test_inline_prefix_before_insert_still_blocked(self): + """An inline PREFIX followed by INSERT must not be allowed — the + inline stripping fix must not open a bypass for write operations.""" + query = "PREFIX ex: INSERT DATA { ex:s ex:p ex:o }" + assert not _is_read_only_query(query) + + def test_long_valid_query_not_rejected_by_is_read_only(self): + """_is_read_only_query must not enforce the length limit itself — + that responsibility belongs to execute_sparql() so the route can + return a distinct, actionable error. A syntactically valid but long + SELECT query must still return True from this function.""" + long_query = "SELECT ?s WHERE { ?s ?p ?o } # " + ("x" * 20_000) + assert _is_read_only_query(long_query) + # =================================================================== # 2. Cypher injection prevention