Repository navigation
fix(audit): remediate all 18 AUDIT findings as a ten-branch stack - #84
Merged
Merged
Conversation
The tool connected with the application DATABASE_URL, so its entire read-only guarantee rested on a keyword regex that dollar quotes and E'' escapes could blind, while file-reading and side-effecting built-ins were never denied. Connect with a dedicated non-superuser role instead, fail closed when it is not configured, enforce read-only at the session and refuse a superuser role. Extend the statement masker to both PostgreSQL quoting forms, deny the privileged built-ins, and clamp LIMIT ALL / oversized LIMIT in SQL so the row cap cannot be widened by the caller. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The stack published Postgres on 5432 with postgres/postgres and Redis on 6379 with no authentication at all, so every run goal, answer and session history was reachable by anything that could route to those ports. Bind both datastores to loopback by default, make all three passwords required rather than defaulted, authenticate Redis and its healthcheck, and add a credential guard that stops the migrations service on image defaults or placeholders. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
create_run, update_run and save_messages were all wrapped in a silent contextlib.suppress, and the route then synthesised a 200 from the in-memory result. A run that was never stored was reported as succeeded, a failed final update left the row in 'running' forever, and PostgresTraceSink fabricated a placeholder Run with a lost goal and session id. Raise PersistenceError (503) on create/read-back failure, retry a failed final update once as failed, and let the trace sink drop a step rather than invent its parent. Bound session_id at the API to the column width so an over-long id is a 422 rather than an invisible insert error.
…istory A single non-UTF-8 thought_signature made model_dump(mode="json") raise, which aborted the write of the whole conversation while the agent suppressed the error. Reading a corrupt payload returned [] and the next run then overwrote the real history with a context-free one, and truncation was never wired to a setting - its naive tail cut could also start a history on an orphaned tool result. Encode signatures as an explicitly tagged base64 object and decode them in a ToolCall validator (legacy UTF-8 values still read). Raise SessionHistoryError instead of pretending the session is new, and bound histories with SESSION_MAX_MESSAGES, advancing the cut point past leading tool results.
Every tool result was sent as its own user Content, so Gemini rejected any parallel call because the response count did not match the call count. The provider FunctionCall.id was replaced by a random UUID and FunctionResponse.id was never set, making identical parallel calls uncorrelatable. Group consecutive results into one turn with one function_response per call, preserve the provider id end to end, and stop concatenating thought=True parts into the answer (including via the raw_response.text fallback) while still returning the function call a thought part carries. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
A blocked prompt, a content filter and an empty completion all arrive as an ordinary 200 with no content and no tool calls. The agent only asked whether there were tool calls, so each was recorded as SUCCEEDED with a blank final_answer and nothing named the provider's decision. Carry the provider verdict on LLMResponse.refusal_reason, add RunStatus.REFUSED, and fail a run that produced neither text nor tool calls. A successful run now always carries a non-empty answer. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Pydantic emits nested models and enums as $refs into a sibling $defs block, which the schema builder dropped, leaving unresolvable references. Provider clients were also built per request with no close path, leaking a transport and connection pool each time, and TOOL_OUTPUT_MAX_CHARS was never read. Carry $defs and the other schema keys through, build the provider once on app.state and close it in the lifespan (aio.aclose for Gemini), and wire the truncation limit plus every documented setting into the agent and compose. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…stant time The duplicate detector only remembered the previous call signature, so an alternating A, B, A, B pattern reset the counter every step and the run burned max_steps instead of being stopped by the guard. API keys were matched with a short-circuiting list membership test, a timing oracle, and the raw header was compared against stripped configured keys. Count repeats inside a configurable sliding window, and compare every configured key with hmac.compare_digest over normalised values so rate limiting and logs see the same key. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
The README described sql_readonly as AST-validated, the audit table recorded unqualified PASS verdicts for components that were broken, ADR-006 claimed Pydantic base64-encodes bytes and named a model the code does not use, and the architecture doc never stated the real default model. Describe the actual lexer and dedicated-role controls, qualify the PASS rows, annotate ADR-006 as superseded and correct its false claims, and guard the corrected statements with a docs-accuracy test. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Each layer was squashed to a single atomic commit, so the per-row commit counts now match the branch history instead of the original work-in-progress step count. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
…edis Compose hardening made Redis start with --requirepass but nothing outside compose consumed REDIS_PASSWORD, so the live tier fell back to a credential-less URL and all 18 Redis fixtures failed with "HELLO must be called with the client already authenticated". REDIS_PASSWORD is now a documented setting that completes a credential-less REDIS_URL for the application and the real suite, keeping the secret out of the URL string while an explicit credential still wins.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7311ee9098
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+71
to
+72
| - DATABASE_URL=${DATABASE_URL:-postgresql+asyncpg://${POSTGRES_USER:-postgres}:${POSTGRES_PASSWORD}@postgres:5432/${POSTGRES_DB:-agentkit}} | ||
| - DATABASE_READONLY_URL=${DATABASE_READONLY_URL:-postgresql+asyncpg://${POSTGRES_READONLY_USER:-agentkit_readonly}:${POSTGRES_READONLY_PASSWORD}@postgres:5432/${POSTGRES_DB:-agentkit}} |
There was a problem hiding this comment.
When Compose synthesizes these URLs from the documented openssl rand -base64 24 passwords, it inserts the values raw. A generated password containing / splits the URL authority and can make the migration or API connection fail; the same fallback pattern is used for both database URLs. Percent-encode credentials or construct the URLs with SQLAlchemy's URL API.
Useful? React with 👍 / 👎.
Comment on lines
+109
to
+112
| verb = "ALTER" if exists else "CREATE" | ||
| await _run_formatted( | ||
| conn, | ||
| f"{verb} ROLE %I WITH LOGIN PASSWORD %L", |
There was a problem hiding this comment.
When DATABASE_READONLY_URL names an existing role, this branch changes its login and password but leaves existing memberships intact. A role inheriting pg_read_server_files can still read host files: _enforce_read_only_session rejects only superusers, and the SQL validator masks quoted identifiers, so SELECT "pg_read_file"(...) passes its denylist. Revoke inherited privileges or refuse to reuse a role that is not already restricted.
Useful? React with 👍 / 👎.
Compose interpolated POSTGRES_PASSWORD and POSTGRES_READONLY_PASSWORD straight into DATABASE_URL, and REDIS_PASSWORD into REDIS_URL. The documented generator is `openssl rand -base64 24`, whose output contains '/', '+' and '='; an RFC 3986 parser ends the authority at the first '/', so the remainder of the password becomes the path and the connection resolves to a database that does not exist. SQLAlchemy's parser is lenient about '/' but truncates the secret at an '@'. Credentials are now configured as components (POSTGRES_HOST/PORT/USER/ PASSWORD/DB plus POSTGRES_READONLY_*), and build_postgres_url() renders the URL through SQLAlchemy's URL.create() so each component is escaped. resolve_postgres_url() is the single precedence rule - an explicit DATABASE_URL/DATABASE_READONLY_URL wins verbatim, then the components, then the documented development default - and both Settings and the deploy credential guard call it, so the guard audits exactly the URLs the application uses. Compose passes components only and pins the datastore host/port to the service names, so a host-side .env pointing at localhost cannot leak into a container. Verified against live PostgreSQL 16 and Redis 7 started with a password containing '/', '+' and '=': both connect, and coverage, ruff, mypy and the full suite stay green. Closes AUDIT-19; recorded in ADR-019.
Provisioning changed only the password of a role that already existed, so an existing SUPERUSER, CREATEDB, CREATEROLE, REPLICATION or BYPASSRLS role kept those attributes, and a member of pg_read_server_files kept the ability to call pg_read_file, no matter what the SELECT-only grant says. An existing role is now treated as untrusted input: restriction_statements() revokes those attributes and every privilege on the database, the public schema, tables, sequences and functions; _revoke_memberships() drops each direct membership; only then are CONNECT/USAGE/SELECT granted back. A role that owns the database is refused (an owner can grant itself anything) and _assert_restricted() verifies the outcome, raising ReadonlyRoleConfigError instead of reporting a privileged role as provisioned. The tool no longer trusts the provisioner alone. _enforce_read_only_session checks rolsuper, rolbypassrls and membership of the privileged predefined roles in one round trip (pg_has_role, so indirect membership counts) and refuses before the query runs. The statement lexer re-exposes plain quoted identifiers to the function denylist, because PostgreSQL resolves SELECT "pg_read_file"(...) to the same built-in; keyword scans keep masking identifiers so SELECT "delete" FROM t stays valid. Verified against the live stack: the new real test grants CREATEDB plus pg_read_server_files and provisioning strips both, and the full real-infra suite passes. Closes AUDIT-20; recorded in ADR-020.
getdownandcode
force-pushed
the
fix/real-infra-redis-auth
branch
from
October 26, 2026 16:29
660aaf1 to
e131dfb
Compare
This was referenced Oct 26, 2026
This was referenced Oct 26, 2026
getdownandcode
added a commit
that referenced
this pull request
Oct 26, 2026
The five PR #84 follow-ups (#85-#89) landed as a stacked PR but were missing from the execution log, so the table still ended at AUDIT-18-FU and the detailed notes stopped at AUDIT-20. Add one row per follow-up pointing at #90 and a combined Detailed Task Notes section that records what each layer changed and why.
getdownandcode
added a commit
that referenced
this pull request
Oct 26, 2026
fix(audit): close the five PR #84 review follow-ups as a five-layer stack
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses every open audit issue (
#58–#75, AUDIT-01…18), one follow-up regression the compose hardening introduced, and the two findings raised by review on this PR (AUDIT-19, AUDIT-20). The work is a linear stack of twelve branches, one commit each; this PR is the tip, so reviewing commit-by-commit walks the layers in dependency order and merging lands all twelve atomic commits onmainat once.Layers
fix/sql-readonly-isolationfix(sql): isolate the read-only SQL tool from the application superuserchore/compose-hardeningchore(deploy): harden Postgres and Redis exposure in composefix/persistence-integrityfix(persistence): fail loudly instead of returning phantom successesfix/session-historyfix(memory): persist Gemini signatures losslessly and bound session historyfix/gemini-adapterfix(gemini): group parallel function responses and drop thought partsfix/provider-refusal-statusfix(llm): surface refusals, blocks and empty responses as failuresfix/tools-resources-configfix(config): preserve tool schema $defs and share one provider clientfix/agent-loop-and-authfix(agent): detect non-consecutive tool loops and compare keys in constant timedocs/correct-audit-claimsdocs(audit): correct guarantees the code does not providefix/real-infra-redis-authtest(real-infra): authenticate the suite against password-protected Redisfix/pg-url-credentialsfix(config): build datastore URLs from escaped componentsfix/readonly-role-privilegesfix(sql): restrict a reused read-only role instead of trusting itWhat changed, per finding
LIMIT ALL/ huge limits bypass the row cap #59 (SQL isolation, row cap) —sql_readonlyconnects with a dedicated non-superuser role (DATABASE_READONLY_URL,SELECT-only,default_transaction_read_only), refuses a superuser connection and fails closed when the role is unprovisioned. The literal masker now understands dollar quotes andE'…'escapes, a function denylist blockspg_read_file,nextval,dblink_execand friends, and the row cap is clamped in SQL (LIMIT ALL/NULL/oversized), leaving subquery bounds alone.POSTGRES_BIND/REDIS_BIND),POSTGRES_PASSWORD/POSTGRES_READONLY_PASSWORD/REDIS_PASSWORDare required by interpolation, Redis runs with--requirepassand an authenticating healthcheck, andscripts/check_deploy_credentials.pyfails the stack closed before migrations.PostgresTraceSinkfabricates a placeholderRunrow #60 / AUDIT-08 — Medium-High — Persistence errors are swallowed; phantom successes and stalerunning#65 / AUDIT-09 — Medium-High —session_idunbounded at the API,VARCHAR(255)in the DB #66 (persistence) —PersistenceError(503) instead of phantom 200s, no fabricated run row in the trace sink, failed-state retry after a failed final update, andsession_idbounded at the API boundary.thought_signaturecannot be persisted; session history silently dropped #61 / AUDIT-10 — Medium-High — Session-history load failure is silent and destructive #67 / AUDIT-11 — Medium — Session truncation is never wired; when enabled it orphans tool messages #68 (session history) — tagged base64 thought-signature codec with aToolCallvalidator,SessionHistoryError(503) instead of overwriting unreadable history, andSESSION_MAX_MESSAGEStruncation that never orphans a tool result.thought=Trueparts excluded from the answer and fromfinal_answer.SUCCEEDED#64 (refusals) —LLMResponse.refusal_reason,RunStatus.REFUSED, and an empty completion failing instead of succeeding.TOOL_OUTPUT_MAX_CHARSignored; compose omits documented settings #69 / AUDIT-13 — Medium — Tool JSON schemas drop$defs(unresolvable$ref) #70 / AUDIT-14 — Medium — LLM provider clients are created per request and never closed #71 —TOOL_OUTPUT_MAX_CHARSwired (and every documented setting forwarded by compose),$defspreserved so$refs resolve, one application-owned provider client closed on shutdown.hmac.compare_digestover normalised keys.REDIS_PASSWORDcompletes a credential-lessREDIS_URLfor the API and the live suite.${POSTGRES_PASSWORD},${POSTGRES_READONLY_PASSWORD}or${REDIS_PASSWORD}into a connection URL. Credentials travel as components (POSTGRES_HOST/PORT/USER/PASSWORD/DB,POSTGRES_READONLY_*,REDIS_PASSWORD) andbuild_postgres_url()renders the URL through SQLAlchemy'sURL.create()so each part is percent-encoded;resolve_postgres_url()is the single precedence rule (explicit URL wins, then components, then the development default) and bothSettingsand the credential guard call it. Matters because the documented generator isopenssl rand -base64 24, whose/ends the URL authority under RFC 3986 parsing and whose@truncates the password in SQLAlchemy's parser.SUPERUSER/CREATEDB/BYPASSRLS/memberships (pg_read_server_filesallowspg_read_fileeven though the lexer blanks quoted identifiers). Provisioning now revokes those attributes, every direct membership and all non-SELECTobject privileges before grantingCONNECT/USAGE/SELECTback, refuses a role that owns the database, and verifies the result. Per session, the tool also refusesrolsuper,rolbypassrlsand membership of the privileged predefined roles viapg_has_role, and the lexer now seesSELECT "pg_read_file"(…)as the call it is.Test plan
Run on the tip, all green:
docker compose configwas rendered with a generatedopenssl rand -base64 24password and shows no credential inside any URL. The escaped-credential path was additionally exercised against live PostgreSQL 16 and Redis 7 started withaB3/xY+9zQ==wE7andca/Ch+e9==xY.Fresh regression tests live in
tests/unit/test_sql_isolation.py,test_session_history.py,test_gemini_wire_format.py,test_provider_refusals.py,test_tools_resources_config.py,test_loop_detector_and_auth.py,test_real_infra_urls.py,test_database_url_components.py,test_readonly_role_provisioning.py,test_docs_accuracy.py,tests/integration/test_persistence_integrity.pyand the live tier undertests/real/(including a role that is grantedCREATEDBandpg_read_server_filesbefore provisioning strips both).Merge note
Use a merge commit, not squash, so the twelve layer commits stay individually reviewable in
mainhistory.Closes #58
Closes #59
Closes #60
Closes #61
Closes #62
Closes #63
Closes #64
Closes #65
Closes #66
Closes #67
Closes #68
Closes #69
Closes #70
Closes #71
Closes #72
Closes #73
Closes #74
Closes #75