mirror of
https://github.com/semantica-agi/semantica.git
synced 2026-08-29 04:26:20 +00:00
fix(security): address Qodo review findings on the disclosure-fix PR
- export_table_data() re-raises ValidationError instead of masking it as ProcessingError via the blanket except Exception. - _apply_connection_pin() restores the session's original Host header state on an unpinned hop instead of unconditionally clearing it, which was dropping a caller-supplied session's own Host override. - SQL fragment blocklist now masks quoted string/identifier literal contents before matching, so legitimate data containing a blocked keyword (e.g. status = 'union') no longer false-positives; a malformed/unterminated quote stays unmasked and still scrutinized.
This commit is contained in:
@@ -185,6 +185,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
- Also hardened in the same pass: Apache AGE's `create_index()` `index_type` parameter is now allowlisted (was interpolated raw into a `USING` clause); Neo4j's `limit` is now explicitly validated (raises `ValidationError` for non-integer input instead of falling through to a generic `ProcessingError`); the `ffprobe` metadata-extraction subprocess call is guarded against a filename starting with `-` being parsed as an option; the MCP server no longer echoes raw exception text to JSON-RPC clients, logging full details server-side and returning a generic message plus the exception class name instead
|
||||
- **Fixed during review** (@KaifAhmad1): the SSRF IP-pinning change introduced a connection-pool leak of its own — `requests.Session.mount()` silently drops whatever adapter it replaces without closing it, so a multi-hop redirect chain on a reused session leaked one pooled connection per hop. Pinned adapters are now tagged and explicitly closed before being replaced, both per-hop and on final restore
|
||||
- **Fixed during review** (@KaifAhmad1): mounting a pinned adapter and setting a Host header on a caller-supplied `Session` is not inherently thread-safe — two guarded calls sharing the same session from different threads could interleave their mount/restore cycles. Added a per-session lock (`_get_session_lock`) so concurrent guarded calls on the same session now serialize instead of racing; verified with a two-thread test showing correct serialization and zero cross-contamination of per-request Host headers
|
||||
- **Fixed during automated PR review** (Qodo): `export_table_data()`'s new identifier/fragment validation raised `ValidationError` from inside a `try` whose blanket `except Exception` re-wrapped it as `ProcessingError`, masking the distinction between "bad input" and "the export itself failed" that callers rely on elsewhere in this module. Added the `except ValidationError: raise` guard already used by its sibling methods
|
||||
- **Fixed during automated PR review** (Qodo): on a hop where IP pinning doesn't apply (`allow_private_ips=True`), `_apply_connection_pin()` unconditionally popped the session's `Host` header instead of restoring whatever it was before pinning touched it — a caller-supplied session carrying its own legitimate `Host` override (e.g. fronting a private endpoint under a different name) had that override silently dropped for the in-flight request, only reappearing afterward via the outer `finally` restore. It now restores the session's own pre-call header state (set back if present, popped only if it was truly absent) instead of always popping
|
||||
- **Fixed during automated PR review** (Qodo): the `where`/`order_by` blocklist matched keywords/punctuation inside properly quoted string literals and identifiers too, so legitimate data like `status = 'union'` or `name = 'a--b'` was rejected as if it were SQL syntax. The blocklist now runs against a copy with quoted-literal contents masked out (`_mask_sql_literals`) — a malformed/unterminated quote sequence doesn't match the masking pattern and is left fully exposed to the blocklist, so this closes false positives without opening a masking-based bypass; the fragment actually used in the query is unchanged
|
||||
- Re-ran each finding's proof-of-concept (or an equivalent adversarial test) against the fix and confirmed it is blocked: tar path/symlink traversal (both lexical and resolved-path forms), SQL UNION exfiltration and identifier breakout, DNS-rebinding TOCTOU (including under a configured `HTTP_PROXY`, which the pinning adapter also rejects outright since a proxy would resolve DNS itself), stored XSS, and the AnzoStore SPARQL injection
|
||||
- `pytest tests/ingest/`: 266 passed, 2 skipped (10 pre-existing failures unrelated to this change — identical failure set confirmed on unmodified `main`); full regression sweep across `graph_store`, `export`, `triplet_store`, `parse`, and backup/restore: 313 passed
|
||||
|
||||
|
||||
@@ -53,6 +53,34 @@ _SQL_FRAGMENT_BLOCKLIST_RE = re.compile(
|
||||
re.IGNORECASE,
|
||||
)
|
||||
|
||||
# SQL single-quoted string literals ('' is the standard escaped-quote) and
|
||||
# double-quoted identifiers ("" likewise) — matched only when properly
|
||||
# closed, so a malformed/unterminated quote sequence is left alone and
|
||||
# still hits the blocklist above rather than being treated as "inside a
|
||||
# literal" and skipped.
|
||||
_SQL_STRING_LITERAL_RE = re.compile(r"'(?:[^']|'')*'")
|
||||
_SQL_QUOTED_IDENTIFIER_RE = re.compile(r'"(?:[^"]|"")*"')
|
||||
|
||||
|
||||
def _mask_sql_literals(fragment: str) -> str:
|
||||
"""Blank the contents of quoted literals so they can't trip the blocklist.
|
||||
|
||||
A legitimate value or quoted identifier that happens to contain a
|
||||
blocked word or character as *data* — e.g. ``status = 'union'`` or
|
||||
``"my--column" = 1`` — is not SQL syntax and shouldn't be rejected as
|
||||
if it were. Only the quoted span's interior is replaced (with `?`,
|
||||
keeping the surrounding quotes and the fragment's length/positions
|
||||
intact for the error message); text outside any properly closed quote
|
||||
is passed through unchanged and still fully scrutinized.
|
||||
"""
|
||||
fragment = _SQL_STRING_LITERAL_RE.sub(
|
||||
lambda m: "'" + "?" * (len(m.group(0)) - 2) + "'", fragment
|
||||
)
|
||||
fragment = _SQL_QUOTED_IDENTIFIER_RE.sub(
|
||||
lambda m: '"' + "?" * (len(m.group(0)) - 2) + '"', fragment
|
||||
)
|
||||
return fragment
|
||||
|
||||
|
||||
def _validate_sql_identifier(name: str, kind: str) -> str:
|
||||
"""Validate a table/schema name used as a raw SQL identifier.
|
||||
@@ -90,7 +118,11 @@ def _validate_sql_fragment(fragment: str, kind: str) -> str:
|
||||
"""
|
||||
if not isinstance(fragment, str):
|
||||
raise ValidationError(f"Invalid {kind}: must be a string")
|
||||
if _SQL_FRAGMENT_BLOCKLIST_RE.search(fragment):
|
||||
# Check the blocklist against literal-masked text so a blocked word
|
||||
# appearing only as quoted data (not as SQL syntax) doesn't false-
|
||||
# positive; the original, unmodified fragment is still what's returned
|
||||
# and used in the query.
|
||||
if _SQL_FRAGMENT_BLOCKLIST_RE.search(_mask_sql_literals(fragment)):
|
||||
raise ValidationError(
|
||||
f"Invalid {kind}: {fragment!r} contains disallowed SQL "
|
||||
"keywords or statement-boundary characters"
|
||||
@@ -417,6 +449,8 @@ class DataExporter:
|
||||
schema=schema,
|
||||
)
|
||||
|
||||
except ValidationError:
|
||||
raise
|
||||
except Exception as e:
|
||||
self.logger.error(f"Failed to export table {table_name}: {e}")
|
||||
raise ProcessingError(f"Failed to export table: {e}") from e
|
||||
|
||||
@@ -408,6 +408,8 @@ def _apply_connection_pin(
|
||||
pinned_ips: Optional[List[str]],
|
||||
orig_http_adapter: "requests.adapters.HTTPAdapter",
|
||||
orig_https_adapter: "requests.adapters.HTTPAdapter",
|
||||
had_host_header: bool,
|
||||
orig_host_header: Optional[str],
|
||||
) -> None:
|
||||
"""Mount (or remove) IP pinning on *active_session* for the next hop."""
|
||||
# Session.mount() silently drops whatever adapter it replaces without
|
||||
@@ -435,7 +437,15 @@ def _apply_connection_pin(
|
||||
else:
|
||||
active_session.mount("http://", orig_http_adapter)
|
||||
active_session.mount("https://", orig_https_adapter)
|
||||
active_session.headers.pop("Host", None)
|
||||
# Restore the session's own pre-call Host header state rather than
|
||||
# unconditionally clearing it — a caller-supplied session may carry
|
||||
# a legitimate Host override (e.g. a private/internal endpoint
|
||||
# fronted by a name that differs from the connection host), which
|
||||
# a hop that happens not to need pinning must not silently drop.
|
||||
if had_host_header:
|
||||
active_session.headers["Host"] = orig_host_header
|
||||
else:
|
||||
active_session.headers.pop("Host", None)
|
||||
|
||||
|
||||
_SESSION_LOCK_ATTR = "_semantica_ssrf_lock"
|
||||
@@ -605,6 +615,8 @@ def request_with_ssrf_guard(
|
||||
current_pinned_ips,
|
||||
_orig_http_adapter,
|
||||
_orig_https_adapter,
|
||||
_had_host_header,
|
||||
_orig_host_header,
|
||||
)
|
||||
response = requester(
|
||||
current_method,
|
||||
|
||||
Reference in New Issue
Block a user