From abc10bc8e0761ec09c6262812d63affee624cc5f Mon Sep 17 00:00:00 2001 From: KaifAhmad1 Date: Tue, 11 Aug 2026 15:36:49 +0530 Subject: [PATCH] fix(security): restore GHSA-j4mq auth enforcement, fix SPARQL comment-regex bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- semantica/explorer/app.py | 63 ++++++++++------ semantica/explorer/auth.py | 102 -------------------------- semantica/explorer/routes/sparql.py | 28 +++++-- tests/explorer/test_sparql_route.py | 23 ++++-- tests/test_security_regression.py | 110 ++++++++++------------------ 5 files changed, 119 insertions(+), 207 deletions(-) delete mode 100644 semantica/explorer/auth.py diff --git a/semantica/explorer/app.py b/semantica/explorer/app.py index 3d175ed2..68bab729 100644 --- a/semantica/explorer/app.py +++ b/semantica/explorer/app.py @@ -8,16 +8,16 @@ from contextlib import asynccontextmanager from pathlib import Path from typing import Optional -from fastapi import FastAPI, HTTPException, Request, WebSocket, WebSocketDisconnect +from fastapi import Depends, FastAPI, HTTPException, Request, WebSocket, WebSocketDisconnect from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import FileResponse, HTMLResponse, JSONResponse from fastapi.staticfiles import StaticFiles from .. import __version__ from ..context.context_graph import ContextGraph +from .dependencies import anonymous_access_allowed, get_expected_api_key, is_valid_api_key, require_auth from .session import GraphSession from .ws import ConnectionManager -from .auth import APIKeyAuthMiddleware, warn_if_unauthenticated def _read_int_env(name: str, default: int) -> int: @@ -98,6 +98,22 @@ def create_app( @asynccontextmanager async def lifespan(app: FastAPI): + import logging as _lifespan_logging + _lifespan_logger = _lifespan_logging.getLogger(__name__) + if anonymous_access_allowed(): + _lifespan_logger.warning( + "Explorer is running with SEMANTICA_ALLOW_ANONYMOUS=true — " + "all API routes are unauthenticated. Do not expose this " + "process beyond localhost." + ) + elif get_expected_api_key(): + _lifespan_logger.info("Explorer API authentication: enabled (SEMANTICA_API_KEY set).") + else: + _lifespan_logger.warning( + "Explorer API authentication: NOT CONFIGURED. All protected " + "routes will return 503 until SEMANTICA_API_KEY is set." + ) + app.state.event_loop = asyncio.get_running_loop() app.state.ws_manager = ConnectionManager() app.state.session = active_session @@ -114,10 +130,10 @@ def create_app( app.state.explorer_settings = settings # allow_credentials lets browsers send cookies/auth headers cross-origin. - # The Explorer has no authentication, so credentials serve no purpose and - # enabling them when origins are broadened creates cross-site request risk. - # Set EXPLORER_CORS_CREDENTIALS=true explicitly to opt in (e.g. for a - # reverse-proxy setup that injects its own auth layer). + # Credentials aren't needed for the X-API-Key auth scheme below, and + # enabling them when origins are broadened creates cross-site request + # risk. Set EXPLORER_CORS_CREDENTIALS=true explicitly to opt in (e.g. + # for a reverse-proxy setup that injects its own cookie-based auth). _allow_credentials = os.environ.get("EXPLORER_CORS_CREDENTIALS", "false").lower() == "true" app.add_middleware( CORSMiddleware, @@ -128,10 +144,6 @@ def create_app( max_age=600, ) - # API key authentication (opt-in via EXPLORER_API_KEY env var) - app.add_middleware(APIKeyAuthMiddleware) - warn_if_unauthenticated() - import logging as _logging _logger = _logging.getLogger(__name__) @@ -164,22 +176,31 @@ def create_app( from .routes.temporal import router as temporal_router from .routes.vocabulary import router as vocabulary_router - app.include_router(graph_router) - app.include_router(analytics_router) - app.include_router(decisions_router) - app.include_router(temporal_router) - app.include_router(enrich_router) - app.include_router(export_import_router) - app.include_router(annotations_router) - app.include_router(sparql_router) - app.include_router(provenance_router) - app.include_router(vocabulary_router) - app.include_router(ontology_router) + _auth = [Depends(require_auth)] + app.include_router(graph_router, dependencies=_auth) + app.include_router(analytics_router, dependencies=_auth) + app.include_router(decisions_router, dependencies=_auth) + app.include_router(temporal_router, dependencies=_auth) + app.include_router(enrich_router, dependencies=_auth) + app.include_router(export_import_router, dependencies=_auth) + app.include_router(annotations_router, dependencies=_auth) + app.include_router(sparql_router, dependencies=_auth) + app.include_router(provenance_router, dependencies=_auth) + app.include_router(vocabulary_router, dependencies=_auth) + app.include_router(ontology_router, dependencies=_auth) _WS_MAX_MESSAGE_BYTES = 64 * 1024 # 64 KB — control messages only @app.websocket("/ws/graph-updates") async def websocket_endpoint(websocket: WebSocket): + # Browsers can't set custom headers on a WebSocket handshake, so + # accept the key via header (non-browser clients) or query param + # (browser clients), same SEMANTICA_API_KEY the REST routes check. + candidate = websocket.headers.get("x-api-key") or websocket.query_params.get("api_key") + if not is_valid_api_key(candidate): + await websocket.close(code=4401) # unauthorized + return + manager: ConnectionManager = app.state.ws_manager await manager.connect(websocket) await manager.send_personal(websocket, "connection_ack", {"connected": True}) diff --git a/semantica/explorer/auth.py b/semantica/explorer/auth.py deleted file mode 100644 index 9785b1b5..00000000 --- a/semantica/explorer/auth.py +++ /dev/null @@ -1,102 +0,0 @@ -""" -Semantica Explorer : Authentication Middleware - -Provides opt-in API key authentication for all Explorer API routes. - -Enable by setting the ``EXPLORER_API_KEY`` environment variable. When set, -every request to ``/api/*`` must include either: - -- An ``Authorization: Bearer `` header, or -- An ``X-API-Key: `` header. - -When ``EXPLORER_API_KEY`` is not set, authentication is disabled and the -Explorer operates in open/development mode (with a startup warning). -""" - -import hmac -import logging -import os -from typing import Optional - -from fastapi import HTTPException, Request, status -from starlette.middleware.base import BaseHTTPMiddleware, RequestResponseEndpoint -from starlette.responses import Response - -_logger = logging.getLogger(__name__) - - -def _get_api_key() -> Optional[str]: - """Read the configured API key from the environment.""" - return os.environ.get("EXPLORER_API_KEY") - - -def _extract_token(request: Request) -> Optional[str]: - """Extract the API key from the request headers.""" - # Check Authorization: Bearer - auth_header = request.headers.get("Authorization", "") - if auth_header.startswith("Bearer "): - return auth_header[7:].strip() - - # Check X-API-Key: - api_key_header = request.headers.get("X-API-Key", "") - if api_key_header: - return api_key_header.strip() - - return None - - -class APIKeyAuthMiddleware(BaseHTTPMiddleware): - """ - Middleware that enforces API key authentication on ``/api/*`` routes. - - Skips authentication for: - - Non-API routes (static files, health checks, WebSocket, docs) - - OPTIONS requests (CORS preflight) - - When ``EXPLORER_API_KEY`` is not configured (open mode) - """ - - async def dispatch( - self, request: Request, call_next: RequestResponseEndpoint - ) -> Response: - api_key = _get_api_key() - - # If no API key is configured, allow all requests (open mode) - if not api_key: - return await call_next(request) - - # Skip authentication for non-API paths - path = request.url.path - if not path.startswith("/api/"): - return await call_next(request) - - # Skip CORS preflight - if request.method == "OPTIONS": - return await call_next(request) - - # Validate the token - token = _extract_token(request) - if not token: - raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, - detail="Missing API key. Provide via 'Authorization: Bearer ' or 'X-API-Key: ' header.", - headers={"WWW-Authenticate": "Bearer"}, - ) - - # Constant-time comparison to prevent timing attacks - if not hmac.compare_digest(token, api_key): - raise HTTPException( - status_code=status.HTTP_403_FORBIDDEN, - detail="Invalid API key.", - ) - - return await call_next(request) - - -def warn_if_unauthenticated() -> None: - """Log a warning at startup if no API key is configured.""" - if not _get_api_key(): - _logger.warning( - "EXPLORER_API_KEY is not set. The Explorer API is running WITHOUT " - "authentication. Set EXPLORER_API_KEY to enable API key protection " - "for all /api/* endpoints." - ) diff --git a/semantica/explorer/routes/sparql.py b/semantica/explorer/routes/sparql.py index 0a0abee4..8271d38d 100644 --- a/semantica/explorer/routes/sparql.py +++ b/semantica/explorer/routes/sparql.py @@ -3,11 +3,16 @@ SPARQL routes backed by an in-memory rdflib projection of the current graph. Security contract ----------------- -* Only SELECT, ASK, CONSTRUCT, and DESCRIBE are accepted (allowlist enforced - before graph construction so rejected queries never touch the session). -* Multi-statement injections that start with an allowed keyword (e.g. - ``SELECT ... ; DROP ALL``) pass the prefix check and reach rdflib, which - rejects non-SELECT/ASK/CONSTRUCT/DESCRIBE update syntax in the parser. +* Only SELECT, ASK, CONSTRUCT, and DESCRIBE are accepted, and the query + body is scanned for SPARQL Update keywords (INSERT/DELETE/DROP/LOAD/ + CLEAR/CREATE/COPY/MOVE/ADD) after stripping comments and PREFIX/BASE + declarations — both enforced before graph construction, so rejected + queries never touch the session. A multi-statement injection appended + after an allowed keyword (e.g. ``SELECT ... ; DROP ALL``) is caught by + the keyword scan itself, not left to rdflib's parser. +* rdflib's parser remains a second line of defense for malformed multi- + statement syntax that doesn't contain any forbidden keyword (e.g. + ``SELECT ... ; ASK ...``), which SPARQL 1.1 Query doesn't permit. * The in-memory rdflib graph is a read-only projection — the live ``GraphSession`` is never mutated by this route. """ @@ -38,9 +43,16 @@ _FORBIDDEN_KEYWORDS = re.compile( re.IGNORECASE, ) -# Matches SPARQL single-line comments (# ...) and PREFIX declarations -_COMMENT_LINE = re.compile(r"#[^\n]*", re.MULTILINE) -_PREFIX_DECL = re.compile(r"^\s*(?:PREFIX|BASE)\s+\S+\s*<[^>]*>\s*", re.IGNORECASE | re.MULTILINE) +# Matches SPARQL single-line comments (# ...) and PREFIX/BASE declarations. +# The comment regex only treats '#' as a comment-starter at line-start or +# after whitespace — not mid-token — since RDF namespace IRIs commonly +# contain a literal '#' (e.g. ".../1999/02/22-rdf-syntax-ns#"), and a naive +# `#[^\n]*` would truncate every such PREFIX declaration's IRI, corrupting +# the query. BASE declarations have no prefix name between the keyword and +# the IRI (`BASE <...>`, vs. `PREFIX ex: <...>`), so the prefix-name token +# is optional. +_COMMENT_LINE = re.compile(r"(?:^|(?<=\s))#[^\n]*", re.MULTILINE) +_PREFIX_DECL = re.compile(r"^\s*(?:PREFIX\s+\S+|BASE)\s*<[^>]*>\s*", re.IGNORECASE | re.MULTILINE) def _is_read_only_query(query: str) -> bool: diff --git a/tests/explorer/test_sparql_route.py b/tests/explorer/test_sparql_route.py index 77cc5b71..1d8547c6 100644 --- a/tests/explorer/test_sparql_route.py +++ b/tests/explorer/test_sparql_route.py @@ -199,14 +199,27 @@ def test_allowlist_rejected_query_never_touches_the_graph(client, query): mock_build.assert_not_called() -def test_multi_statement_injection_reaches_graph_but_fails_in_parser(client): - """Confirms the distinction between allowlist rejection and parser rejection: - a string starting with SELECT passes _is_read_only_query and builds a graph, - but rdflib.Graph.query() rejects the trailing '; DROP ALL' syntax.""" +def test_multi_statement_injection_is_rejected_by_forbidden_keyword_check(client): + """A string starting with an allowed keyword (SELECT) but containing a + forbidden Update keyword later in the body ('; DROP ALL') is now + rejected by _is_read_only_query's keyword scan itself, before a graph + is ever built — a stronger, earlier rejection than relying solely on + rdflib's parser to reject the syntax.""" + with patch.object(sparql_mod, "_build_rdflib_graph") as mock_build: + resp = _post(client, "SELECT ?s WHERE { ?s ?p ?o } ; DROP ALL") + assert resp.status_code == 200 + assert resp.json()["error"] is not None + mock_build.assert_not_called() + + +def test_malformed_syntax_without_forbidden_keywords_still_fails_in_parser(client): + """The parser remains a real second line of defense for malformed + queries that don't contain any forbidden keyword — these pass + _is_read_only_query and reach rdflib, which rejects the syntax.""" with patch.object( sparql_mod, "_build_rdflib_graph", wraps=sparql_mod._build_rdflib_graph ) as spy_build: - resp = _post(client, "SELECT ?s WHERE { ?s ?p ?o } ; DROP ALL") + resp = _post(client, "SELECT ?s WHERE { ?s ?p ?o } ; ASK { ?x ?y ?z }") assert resp.status_code == 200 assert resp.json()["error"] is not None spy_build.assert_called_once() diff --git a/tests/test_security_regression.py b/tests/test_security_regression.py index cab6cf9e..18854f0b 100644 --- a/tests/test_security_regression.py +++ b/tests/test_security_regression.py @@ -2,48 +2,29 @@ Regression tests for security fixes in PR #898. Covers: - 1. API Key Authentication (Explorer) - 2. Cypher Injection Prevention (AGE Store) - 3. SPARQL Injection Prevention (read-only query validation) - 4. XXE Protection (rdf_parser fail-closed) - 5. Vector save numpy serialization - 6. SPARQL graph cap error handling - 7. SSRF redirect handling (relative URLs, resp.close) + 1. Cypher Injection Prevention (AGE Store) + 2. SPARQL Injection Prevention (read-only query validation) + 3. XXE Protection (rdf_parser fail-closed) + 4. Vector save numpy serialization + 5. SPARQL graph cap error handling + 6. SSRF redirect handling (relative URLs, resp.close) + +Explorer API-key authentication (GHSA-j4mq-hprp-987v) has its own, more +thorough test suite at tests/explorer/test_explorer_auth.py — it isn't +duplicated here. """ -import re import pytest - -# =================================================================== -# 1. SPARQL read-only query validation (injection prevention) -# =================================================================== - -# Inline the validation logic so tests don't require full app context -_ALLOWED_QUERY_TYPES = re.compile( - r"^(SELECT|ASK|CONSTRUCT|DESCRIBE)\b", - re.IGNORECASE, -) -_FORBIDDEN_KEYWORDS = re.compile( - r"\b(INSERT|DELETE|DROP|LOAD|CLEAR|CREATE|COPY|MOVE|ADD)\b", - re.IGNORECASE, -) -_COMMENT_LINE = re.compile(r"#[^\n]*", re.MULTILINE) -_PREFIX_DECL = re.compile( - r"^\s*(?:PREFIX|BASE)\s+\S+\s*<[^>]*>\s*", - re.IGNORECASE | re.MULTILINE, -) - - -def _is_read_only_query(query: str) -> bool: - cleaned = _COMMENT_LINE.sub("", query) - cleaned = _PREFIX_DECL.sub("", cleaned) - cleaned = cleaned.strip() - if not _ALLOWED_QUERY_TYPES.match(cleaned): - return False - if _FORBIDDEN_KEYWORDS.search(cleaned): - return False - return True +# Import the real implementation rather than re-declaring the regexes here: +# an earlier version of this file inlined a copy that silently drifted from +# semantica/explorer/routes/sparql.py's actual behavior (the inlined +# _COMMENT_LINE regex stripped '#' mid-token, corrupting any PREFIX +# declaration using a namespace IRI with a literal '#', e.g. the standard +# rdf:/rdfs: namespaces) and neither the code nor this test caught it, +# since both had the same bug. Importing the real function makes that class +# of drift impossible. +from semantica.explorer.routes.sparql import _is_read_only_query class TestSparqlReadOnlyValidation: @@ -122,6 +103,26 @@ class TestSparqlReadOnlyValidation: query = "BASE \nSELECT ?s WHERE { ?s ?p ?o }" assert _is_read_only_query(query) + def test_namespace_iri_with_hash_fragment_not_treated_as_comment(self): + """A '#' inside a PREFIX declaration's IRI (standard for RDF/RDFS/OWL + namespaces) must not be mistaken for a comment-start — a naive + `#[^\\n]*` strip corrupts the IRI and truncates the rest of the + query with it.""" + query = ( + "PREFIX rdf: \n" + "SELECT ?s WHERE { ?s rdf:type ?o }" + ) + assert _is_read_only_query(query) + + def test_real_comment_after_namespace_iri_still_stripped(self): + """A genuine trailing comment must still be recognized even on a + line that also contains a '#'-bearing IRI earlier in the query.""" + query = ( + "PREFIX rdf: \n" + "SELECT ?s WHERE { ?s rdf:type ?o } # trailing comment INSERT DATA" + ) + assert _is_read_only_query(query) + # =================================================================== # 2. Cypher injection prevention @@ -270,38 +271,5 @@ class TestSSRFRedirectHandling: assert result == "https://example.com/api/data.ttl" -# =================================================================== -# 6. API Key Auth -# =================================================================== - -class TestAPIKeyAuth: - """Regression tests for API key authentication.""" - - def test_auth_module_importable(self): - from semantica.explorer.auth import APIKeyAuthMiddleware - assert APIKeyAuthMiddleware is not None - - def test_extract_bearer_token(self): - from semantica.explorer.auth import _extract_token - from unittest.mock import MagicMock - req = MagicMock() - req.headers = {"Authorization": "Bearer test-key-123"} - assert _extract_token(req) == "test-key-123" - - def test_extract_api_key_header(self): - from semantica.explorer.auth import _extract_token - from unittest.mock import MagicMock - req = MagicMock() - req.headers = {"X-API-Key": "my-secret-key", "Authorization": ""} - assert _extract_token(req) == "my-secret-key" - - def test_extract_no_token(self): - from semantica.explorer.auth import _extract_token - from unittest.mock import MagicMock - req = MagicMock() - req.headers = {} - assert _extract_token(req) is None - - if __name__ == "__main__": pytest.main([__file__, "-v"])