Skip to content

fix(security): parameterize SQL, redact sensitive logs, harden ReDoS regexes#2239

Merged
kovtcharov merged 1 commit into
mainfrom
fix/codeql-sqli-logging-redos
Jul 18, 2026
Merged

fix(security): parameterize SQL, redact sensitive logs, harden ReDoS regexes#2239
kovtcharov merged 1 commit into
mainfrom
fix/codeql-sqli-logging-redos

Conversation

@kovtcharov

Copy link
Copy Markdown
Collaborator

Closes five open high-severity Python CodeQL alerts. Before: a memory-browser sort parameter was interpolated into SQL after only a silent-fallback check, the EMR agent wrote patient names into its plain-text success log, and three schedule-parsing regexes could be driven into pathological backtracking by hostile input from the schedules API. After: invalid sort columns fail loudly and never touch the SQL, the success log carries only the patient ID (identity fields still display in the details panel right after), and schedule parsing is length-bounded with linear-time patterns.

Alerts addressed (all fixed, none dismissed):

Alert Rule Resolution
#351 py/sql-injection get_all_knowledge maps sort_by through a literal column allowlist; unknown values raise ValueError instead of silently falling back
#350 py/clear-text-logging-sensitive-data EMR success line redacts the patient name (source of the flagged flow into print_success); on-screen details panel unchanged
#214 py/polynomial-redos Bounded digit quantifier (\d{1,9}) + 256-char input cap in parse_interval
#215 py/polynomial-redos Removed ambiguous `\s*(?:am
#216 py/polynomial-redos Bounded digit quantifier + input cap on the bare Xh/Xm pattern

Test plan

  • pytest tests/unit/test_scheduler.py tests/unit/test_memory_store.py — 374 passed (includes new tests: SQLi sort_by raises, oversized/hostile schedule inputs rejected, all natural-language schedule formats still parse)
  • pytest tests/unit/test_memory_router.py tests/unit/test_scheduler_api.py tests/unit/test_daemon_scheduler.py tests/unit/test_schedule_daemon.py — one failure reproduces identically on pristine origin/main (pre-existing, unrelated)
  • EMR agent tests — 23 passed
  • python util/lint.py --all — exit 0
  • Hostile-input timing check: 10k-char digit/tab payloads now rejected in <1 ms

…regexes

Resolves five high-severity CodeQL alerts:

- py/sql-injection (alert 351): MemoryStore.get_all_knowledge now maps
  sort_by through a literal column allowlist and raises ValueError on
  unknown values instead of silently falling back — no caller-supplied
  string ever reaches the SQL.
- py/clear-text-logging-sensitive-data (alert 350): the EMR agent's
  success line no longer prints the patient's name; identity fields
  remain visible in the on-screen details panel shown right after.
- py/polynomial-redos (alerts 214/215/216): schedule/interval parsing
  now bounds input length to 256 chars before any regex work, bounds
  the digit quantifiers, and removes the ambiguous whitespace overlap
  (`\s*(?:am|pm)?` followed by `\s+`) in the from/to window regex.
@github-actions github-actions Bot added tests Test changes agents agent::emr Medical-intake (EMR) agent changes labels Jul 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

This PR closes five high-severity CodeQL alerts with tightly-scoped, correct fixes: it parameterizes the memory-store sort column against a literal allowlist, drops the patient name from the EMR success log (still shown on-screen right after), and length-caps + de-ambiguates three schedule-parsing regexes so hostile input can't drive pathological backtracking. Each fix ships with a matching test, and the behavior changes are safe — nothing downstream relied on the old silent fallbacks. Nice, focused security work.

🔍 Technical details

Issues Found

None blocking.

Verification notes

  • SQL allowlist change is safe end-to-end (memory_store.py:2087-2092). get_all_knowledge now raises ValueError on an unknown sort_by instead of silently defaulting — correct per the "No Silent Fallbacks" rule. I checked the one HTTP caller: src/gaia/ui/routers/memory.py:337-340 constrains sort_by with Query(pattern="^(updated_at|confidence|created_at|category|context|content|use_count)$"), so FastAPI rejects invalid values with a 422 before they reach the store — the new raise can't turn into an uncaught 500. The other internal callers (memory.py:2396/2427/2453) pass hardcoded literals. So this is defense-in-depth, not a breaking change.
  • EMR redaction preserves the on-screen data (agent.py:631-638). The name is removed only from the persisted success log; _print_patient_details immediately renders identity fields to the console, so the operator still sees them. Claim in the description holds.
  • ReDoS regexes are correctly bounded. \d{1,9} + the 256-char _MAX_SCHEDULE_INPUT_LEN cap kill the polynomial blowup, and rewriting \s*(?:am|pm)?(?:\s*(?:am|pm))? removes the whitespace overlap with the trailing \s+ that CodeQL flagged (scheduler.py:130, 144-145). All natural-language forms ("9:30 am to 5 pm", "9pm", bare "6h") still match. The inline NOTE at scheduler.py:139-140 explaining why the group is nested is a good one-line WHY comment.

Strengths

  • Every fix has a corresponding assertion, including the negative cases: SQLi sort_by now asserts pytest.raises(ValueError) (tightening the old "raise or fall back" test), and the new TestScheduleInputBounds class exercises oversized input, absurd digit runs, and confirms legitimate window/time parsing still works.
  • Fixes stay minimal and scope-clean — no drive-by refactors, and the changes align with the repo's fail-loudly convention rather than papering over bad input.

@kovtcharov
kovtcharov added this pull request to the merge queue Jul 18, 2026
Merged via the queue into main with commit b9391ec Jul 18, 2026
46 checks passed
@kovtcharov
kovtcharov deleted the fix/codeql-sqli-logging-redos branch July 18, 2026 18:12
kovtcharov added a commit that referenced this pull request Jul 18, 2026
…#2238/#2239, OAuth forward-out #2203) into refactor/hub-agent-first-layout-2019
@itomek itomek mentioned this pull request Jul 22, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent::emr Medical-intake (EMR) agent changes agents tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants