fix(security): remediate 2026-07-11 codebase security audit findings#623
Conversation
Batch fix for the findings from the full-codebase security audit. No Critical findings existed in the single-tenant MVP1 threat model; these are the Medium/Low latent issues, most of which escalate once auth/remote deployment land. - SSRF (#1): re-run the cluster base_url guard on the reuse paths (acquire_adapter / reprobe_cluster / get_or_probe_health), not just at registration, mitigating DNS-rebinding TOCTOU. No-op unless RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False. - Query-DSL injection (#2): require query_text to flow through `| tojson` in query templates (new validator guard) and fix the three shipped templates + demo-seeding bodies that interpolated it raw. Closes a live JSONDecodeError bug too. - Chat confirmation gate (#3): tighten is_affirmative (deferral/inability negations + anchored phrase match) and bind the gate to the immediately-preceding assistant turn so a fresh "yes" can't authorize a stale proposal. - Hardened-posture startup WARNs (#4/#5): warn when ENVIRONMENT=development (destructive _test endpoints live) and when the SSRF guard is disabled on a non-dev deployment. - CORS (#10): disable allow_credentials when a wildcard origin is set. - Frontend CSP (#5): enforce object-src/base-uri/form-action; ship the script-src/style-src policy as report-only (nonce work deferred). - Solr local-params injection (#6): neutralize a leading {! prefix in the user query_text on the `q` param. - git-ref pattern (#7): pattern-lock default_branch/pr_base_branch to block leading-dash git argument injection. - Chat tool-loop budget (#8): peek the daily budget before each completion. - config_diff value validation (#9): reject wrong-typed values for numeric/ bool declared params at PR-open. 30 new regression tests. Also regenerates ui/openapi.json (branch-field pattern). Pre-existing infra gap noted: test_trial_row_shape lacks a Postgres skip gate (cf. chore_studies_chain_recent_contract_db_skip_gate). Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
There was a problem hiding this comment.
Code Review
This pull request implements several security hardening measures addressing recent audit findings, including escaping leading Solr local-params, enforcing per-iteration LLM budget gates, validating git branch patterns to prevent argument injection, requiring | tojson filtering for query text interpolation, and adding SSRF re-validation on cluster reuse paths to mitigate DNS-rebinding. Additionally, the confirmation guard logic is refined, CORS credentials are disabled for wildcard origins, and CSP headers are strengthened. Review feedback suggests avoiding false negatives in the confirmation logic by checking for contractions like "can't" explicitly instead of using "can" as a negation token, and recommends using f-strings or structured logging instead of %s formatting in startup warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Deferral / inability tokens — a reply that contains these is not an | ||
| # authorization even if it also carries an affirmative token ("ok" in | ||
| # "ok thanks", "do it" in "can't do it right now" / "do it later"). | ||
| "can", # can't (apostrophe stripped by the [a-z] regex) / "I can later" | ||
| "cant", | ||
| "cannot", | ||
| "couldn", # couldn't | ||
| "couldnt", | ||
| "maybe", | ||
| "later", |
There was a problem hiding this comment.
Including "can" and "couldn" in _NEGATION_TOKENS causes false negatives for common, valid affirmative responses such as "yes you can" or "I can confirm". Since the [a-z]+ regex splits contractions like "can't" and "couldn't" into separate words, we should remove "can" and "couldn" from the token set and instead handle these contractions explicitly in is_affirmative.
| # Deferral / inability tokens — a reply that contains these is not an | |
| # authorization even if it also carries an affirmative token ("ok" in | |
| # "ok thanks", "do it" in "can't do it right now" / "do it later"). | |
| "can", # can't (apostrophe stripped by the [a-z] regex) / "I can later" | |
| "cant", | |
| "cannot", | |
| "couldn", # couldn't | |
| "couldnt", | |
| "maybe", | |
| "later", | |
| # Deferral / inability tokens — a reply that contains these is not an | |
| # authorization even if it also carries an affirmative token ("ok" in | |
| # "ok thanks", "do it" in "can't do it right now" / "do it later"). | |
| "cant", | |
| "cannot", | |
| "couldnt", | |
| "maybe", | |
| "later", |
| # Affirmative *phrases* must LEAD the message (anchored) so a mid-sentence | ||
| # "do it" in "I'll do it later" doesn't unlock dispatch — the leading | ||
| # negation/deferral tokens above already reject most of those, this closes | ||
| # the residual where no negation token is present. | ||
| if _LEADING_AFFIRMATIVE_PHRASE.match(lowered): | ||
| return True |
There was a problem hiding this comment.
To support removing "can" and "couldn" from _NEGATION_TOKENS (preventing false negatives on valid confirmations), we can explicitly check for "can't" and "couldn't" contractions in is_affirmative before evaluating other rules.
| # Affirmative *phrases* must LEAD the message (anchored) so a mid-sentence | |
| # "do it" in "I'll do it later" doesn't unlock dispatch — the leading | |
| # negation/deferral tokens above already reject most of those, this closes | |
| # the residual where no negation token is present. | |
| if _LEADING_AFFIRMATIVE_PHRASE.match(lowered): | |
| return True | |
| # Since the [a-z]+ regex splits "can't" into "can" and "t", we check for "can't" / "couldn't" explicitly | |
| # to avoid false negatives when "can" is used in a valid affirmative (e.g., "yes you can"). | |
| if any(neg in lowered for neg in ("can't", "cant", "cannot", "couldn't", "couldnt")): | |
| return False | |
| # Affirmative *phrases* must LEAD the message (anchored) so a mid-sentence | |
| # "do it" in "I'll do it later" doesn't unlock dispatch — the leading | |
| # negation/deferral tokens above already reject most of those, this closes | |
| # the residual where no negation token is present. | |
| if _LEADING_AFFIRMATIVE_PHRASE.match(lowered): | |
| return True |
| logger.warning( | ||
| "RELYLOOP_ALLOW_PRIVATE_CLUSTERS=True on a non-development deployment " | ||
| "(ENVIRONMENT=%s): the cluster base_url SSRF guard is disabled, so " | ||
| "internal/metadata endpoints can be registered and probed. Set " | ||
| "RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False to enable the guard.", | ||
| settings.environment, | ||
| event_type="ssrf_guard_disabled_non_dev", | ||
| ) |
There was a problem hiding this comment.
Using %s formatting with structlog loggers is discouraged and may not format correctly depending on the processor configuration. It is cleaner and safer to use an f-string or pass the environment as a structured keyword argument.
logger.warning(
f"RELYLOOP_ALLOW_PRIVATE_CLUSTERS=True on a non-development deployment "
f"(ENVIRONMENT={settings.environment}): the cluster base_url SSRF guard is disabled, so "
f"internal/metadata endpoints can be registered and probed. Set "
f"RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False to enable the guard.",
event_type="ssrf_guard_disabled_non_dev",
)…tlog fmt - confirmation.py: remove bare 'can'/'couldn' from negation tokens (false- rejected valid affirmatives like 'yes you can'); keep whole-word 'cant'/ 'cannot'/'couldnt' + explicit apostrophe-form 'can't'/'couldn't' check. Avoids Gemini's substring approach which would hit 'significant'. - main.py: %s -> f-string for the SSRF-guard structlog WARN. - Regression cases added for both directions. Gemini Code Assist: 3 Medium findings, all accepted. Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Review adjudication (Gemini Code Assist)Fixes landed in
Outcomes
CI noteOne integration test — Cross-model review: Opus self-review (GPT-5.5 unreachable in this environment); Gemini is the live cross-family gate. |
Batch fix for the second security re-audit, which verified the #623 hardening and found three of its fixes were incomplete plus one pre-existing agent-gate weakness. No Critical findings. HIGH - SSRF: #623 added the reuse-path guard to acquire_adapter/reprobe/ get_or_probe_health but NOT to the 5 worker/dispatch paths (trials, baseline, judgments, judgments_ubi, agent dispatch) — the longest-lived cluster-egress paths — which call build_adapter directly. Each now `await assert_base_url_allowed(cluster.base_url)` before build_adapter, so the DNS-rebinding re-check fires on every connection (no-op unless RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False). MEDIUM - Solr local-params: #623 neutralized only `q`; query_text could reach fq/bf/boost/pf via a template. Now neutralized at the source (render entry) so it's safe wherever a template places it. - Confirmation gate F1: bound to tool NAME not args → one "yes" authorized unlimited invocations vs arbitrary targets. New one-mutation-per-turn invariant. - Confirmation gate F2: a NEGATED mention ("I will not cancel_study") no longer counts as a proposal (conservative 3-word negation window). LOW - Apostrophe (F3): fold unicode smart apostrophes (U+2019 etc.). - config_diff value validator handles rich-form declared_params. - tojson guard requires tojson to be the OUTERMOST filter. - template.name path-safe pattern (interpolated into params-file path). - Log scrubber redacts scheme://user:password@host DSN passwords. - CI: fork-PR guard on the smoke job (OPENAI_API_KEY_TEST). - Frontend: digest-panel uses shared MARKDOWN_DISALLOWED_ELEMENTS; report-only connect-src dropped to avoid per-API-call noise. ~30 new regression tests. Regenerates ui/openapi.json (template.name pattern). Repo-settings items (CodeQL results merge rule; PR-approval count) handled separately. Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
* fix(security): remediate 2026-07-12 re-audit findings Batch fix for the second security re-audit, which verified the #623 hardening and found three of its fixes were incomplete plus one pre-existing agent-gate weakness. No Critical findings. HIGH - SSRF: #623 added the reuse-path guard to acquire_adapter/reprobe/ get_or_probe_health but NOT to the 5 worker/dispatch paths (trials, baseline, judgments, judgments_ubi, agent dispatch) — the longest-lived cluster-egress paths — which call build_adapter directly. Each now `await assert_base_url_allowed(cluster.base_url)` before build_adapter, so the DNS-rebinding re-check fires on every connection (no-op unless RELYLOOP_ALLOW_PRIVATE_CLUSTERS=False). MEDIUM - Solr local-params: #623 neutralized only `q`; query_text could reach fq/bf/boost/pf via a template. Now neutralized at the source (render entry) so it's safe wherever a template places it. - Confirmation gate F1: bound to tool NAME not args → one "yes" authorized unlimited invocations vs arbitrary targets. New one-mutation-per-turn invariant. - Confirmation gate F2: a NEGATED mention ("I will not cancel_study") no longer counts as a proposal (conservative 3-word negation window). LOW - Apostrophe (F3): fold unicode smart apostrophes (U+2019 etc.). - config_diff value validator handles rich-form declared_params. - tojson guard requires tojson to be the OUTERMOST filter. - template.name path-safe pattern (interpolated into params-file path). - Log scrubber redacts scheme://user:password@host DSN passwords. - CI: fork-PR guard on the smoke job (OPENAI_API_KEY_TEST). - Frontend: digest-panel uses shared MARKDOWN_DISALLOWED_ELEMENTS; report-only connect-src dropped to avoid per-API-call noise. ~30 new regression tests. Regenerates ui/openapi.json (template.name pattern). Repo-settings items (CodeQL results merge rule; PR-approval count) handled separately. Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai> * fix(security): fold apostrophes before negation regex (Gemini review) _tool_mention_is_negated uses ASCII apostrophe forms (won'?t), but assistant_lower wasn't apostrophe-folded, so a smart-quote 'won't' could bypass the F2 negation check. Fold assistant_lower in _is_authorized_mutation so both tool-matching and negation use the ASCII form. Accepted from Gemini. Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai> --------- Signed-off-by: SoundMindsAI <eric.starr@soundminds.ai>
Summary
Batch remediation of the findings from a full-codebase security audit (six parallel audits: injection/command-exec, SSRF/egress, secrets/auth/webhook, frontend, CI/CD & supply chain, LLM/agent authz). No Critical findings existed in the single-tenant MVP1 threat model — these are the Medium/Low latent issues, most of which escalate once auth / remote deployment land.
acquire_adapter,reprobe_cluster,get_or_probe_health){{ query_text }}in shipped templates (also a liveJSONDecodeError)query_textto flow through| tojson; fixed 3 shipped templates + demo-seeding bodiesis_affirmative(deferral/inability negations + anchored phrases); bound the gate to the immediately-preceding assistant turnENVIRONMENTdefault + SSRF flag off in non-devscript-srcCSP (frontend)object-src/base-uri/form-action; shipscript-src/style-srcas report-only (nonce work deferred to GA){!...}) injection viaquery_textonq{!prefix on the userq(render path only)pr_base_branch/default_branchconfig_diffvalues unvalidated at PR-openallow_credentials=True+ wildcard footgunFindings confirmed by the audits as already secure (webhook HMAC, secret handling, git subprocess argv discipline, no SQLi/SSTI, CI action pinning, React escaping) were left unchanged.
Scope note
Larger than a typical ad-hoc PR (cross-subsystem) — bundled deliberately as one security batch per request. No migration; Alembic head unchanged (
0023).Test plan
make test-unit— 2873 passed (+30 new regression tests)cd ui && pnpm test— 1330 passedmake lint && make typecheck(mypy + tsc clean; ruff format --check clean)bash scripts/regen-generated-artifacts.sh—ui/openapi.jsonrefreshed (branch-field pattern)Notes
JSONDecodeErroron any query containing"(rolled into finding docs: MVP1 architecture + 12 feature specs + GPT-5.5 review fixes #2).🤖 Generated with Claude Code