Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions rag_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ def close_server_mode():
client.close()
logger.info("Closed Milvus client: %s", path)
except Exception as e:
logger.warning("Error closing Milvus client %s: %s", path, e)
logger.warning("Error closing Milvus client %s: %s", path, _scrub_exception(e))
_persistent_clients.clear()
_fts.close_all()
# Nil the semaphore and lock FIRST so any coroutine that wakes up while we
Expand All @@ -416,7 +416,7 @@ def _get_persistent_client(db_path: str) -> MilvusClient:
_persistent_clients[db_path].has_collection(COLLECTION_NAME)
return _persistent_clients[db_path]
except Exception as e:
logger.warning("Stale Milvus client for %s: %s — reconnecting", db_path, e)
logger.warning("Stale Milvus client for %s: %s — reconnecting", db_path, _scrub_exception(e))
try:
_persistent_clients[db_path].close()
except Exception:
Expand All @@ -441,7 +441,7 @@ def _get_persistent_client(db_path: str) -> MilvusClient:
)
logger.info("Opened client: %s", db_path)
except Exception as e:
logger.error("Failed to connect to Milvus at %s: %s", db_path, e)
logger.error("Failed to connect to Milvus at %s: %s", db_path, _scrub_exception(e))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM RISK

Suggestion: While the local log is now scrubbed, re-raising the exception e without modification means any upstream logger will still record the sensitive data. Additionally, db_path may contain credentials that should be redacted before logging. Update the logging to redact the db_path argument and mutate e.args to redact secrets before re-raising.

raise
return _persistent_clients[db_path]

Expand Down Expand Up @@ -834,7 +834,7 @@ def add_turns(turns: List[Dict], db_path: Optional[str] = None) -> int:
if results:
existing_ids.add(doc_id)
except Exception as e:
logger.warning("Dedup check failed for doc_id %s: %s", doc_id, e)
logger.warning("Dedup check failed for doc_id %s: %s", doc_id, _scrub_exception(e))

new_turns = [t for t in turns if t["doc_id"] not in existing_ids]
if not new_turns:
Expand Down Expand Up @@ -1537,7 +1537,7 @@ def _passes(entry: Dict) -> bool:
filters=fts_filters or None, db_path=db_path,
)
except Exception as e:
logger.warning("FTS fallback failed for issue timeline %s (non-fatal): %s", canonical, e)
logger.warning("FTS fallback failed for issue timeline %s (non-fatal): %s", canonical, _scrub_exception(e))
fts_hits = []

for row in fts_hits:
Expand Down Expand Up @@ -1951,7 +1951,7 @@ def delete_by_session(session_id: str, db_path: Optional[str] = None) -> int:
_fts.delete(conn, "session_id", session_id)
_fts.close_ephemeral(conn)
except Exception as e:
logger.warning("FTS delete by session failed (non-fatal): %s", e)
logger.warning("FTS delete by session failed (non-fatal): %s", _scrub_exception(e))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

The ephemeral FTS connection is not closed if an exception occurs during deletion, resulting in a resource leak. The block should use a finally clause to ensure the connection is always released. Refactor delete_by_session to ensure _fts.close_ephemeral(conn) is called in a finally block.


return len(results)

Expand Down Expand Up @@ -2217,7 +2217,7 @@ def delete_by_branch(git_branch: str, db_path: Optional[str] = None) -> int:
_fts.delete(conn, "git_branch", git_branch)
_fts.close_ephemeral(conn)
except Exception as e:
logger.warning("FTS delete by branch failed (non-fatal): %s", e)
logger.warning("FTS delete by branch failed (non-fatal): %s", _scrub_exception(e))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

The ephemeral FTS connection is not closed if an exception occurs during deletion, resulting in a resource leak. Use a finally block to ensure _fts.close_ephemeral(conn) is called regardless of success or failure.


return len(results)

Expand Down Expand Up @@ -2256,7 +2256,7 @@ def delete_older_than(max_age_days: int, db_path: Optional[str] = None) -> int:
_fts.delete_where(conn, "timestamp < ? AND timestamp != ''", (cutoff_str,))
_fts.close_ephemeral(conn)
except Exception as e:
logger.warning("FTS delete older_than failed (non-fatal): %s", e)
logger.warning("FTS delete older_than failed (non-fatal): %s", _scrub_exception(e))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH RISK

The ephemeral FTS connection is not closed if an exception occurs during deletion, resulting in a resource leak. Use a finally block to ensure _fts.close_ephemeral(conn) is called regardless of success or failure.


return len(results)

Expand Down
31 changes: 31 additions & 0 deletions tests/test_secret_redaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,37 @@ def boom(conn, records):
assert AWS_KEY not in logged # FTS failure is non-fatal AND scrubbed


@pytest.mark.parametrize(
"invoke",
[
lambda rag: rag.delete_by_session("sess-1", db_path=DB),
lambda rag: rag.delete_by_branch("feature/x", db_path=DB),
lambda rag: rag.delete_older_than(30, db_path=DB),
],
ids=["by_session", "by_branch", "older_than"],
)
def test_ac17_fts_delete_error_is_scrubbed(add_turns_harness, monkeypatch, caplog, invoke):
"""AC-17: a secret in any FTS delete failure message is scrubbed from the log.

Covers the three cleanup entry points (``delete_by_session`` / ``delete_by_branch``
use ``_fts.delete``; ``delete_older_than`` uses ``_fts.delete_where``). A SQLite
error string can echo bound parameters or row content, so the non-fatal handler
must route ``e`` through ``_scrub_exception`` rather than logging it raw.
"""
rag_engine, _ = add_turns_harness

def boom(*args, **kwargs):
raise RuntimeError(f"delete failed near {AWS_KEY}")

monkeypatch.setattr(rag_engine._fts, "delete", boom, raising=False)
monkeypatch.setattr(rag_engine._fts, "delete_where", boom, raising=False)
with caplog.at_level("WARNING"):
invoke(rag_engine) # non-fatal: the forced FTS error must not propagate
logged = " ".join(r.getMessage() for r in caplog.records)
assert AWS_KEY not in logged # delete failure is non-fatal AND scrubbed
assert "[REDACTED:AWS]" in logged # positive control: the scrub actually fired


def test_ac17_embedding_error_is_scrubbed(add_turns_harness, monkeypatch):
"""AC-17: a secret in a raised embedding error is scrubbed before re-raise."""
rag_engine, _ = add_turns_harness
Expand Down
Loading