From 3242d6df5b1ea809596e88e3b21255a7a0815959 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:25:30 +0530 Subject: [PATCH 1/7] fix(deploy): validate passwords embedded in explicit datastore URLs The credential guard applied the weak-password policy only to the POSTGRES_* and REDIS_PASSWORD components. A URL that carries its own userinfo bypassed it: DATABASE_URL was only checked for exact membership in the known-password set, the read-only URL was never policy-checked, and REDIS_URL was checked for presence alone, so 'redis://:short@host' passed the 12-character rule. Malformed URLs also crashed the guard with a traceback instead of reporting a deployment problem, and a URL-only deployment was scolded for component variables it does not use. Audit the credentials actually used: resolve each URL exactly as Settings does, apply the same weak-password policy to URL-supplied secrets, require a password in an explicit URL, and validate a component only when it is the resolution source (still checking any component that is set). Parse each URL once through a helper that reports 'is not a valid connection URL' without echoing the SQLAlchemy parse error or the secret. Add regression tests for weak URL passwords, URL-only configuration, missing passwords and malformed URLs. --- docs/DEPLOY.md | 9 +- scripts/check_deploy_credentials.py | 143 ++++++++++++++++++++------ tests/unit/test_deploy_credentials.py | 76 +++++++++++++- 3 files changed, 193 insertions(+), 35 deletions(-) diff --git a/docs/DEPLOY.md b/docs/DEPLOY.md index 336ee31..31d811c 100644 --- a/docs/DEPLOY.md +++ b/docs/DEPLOY.md @@ -268,9 +268,12 @@ readable by anyone who can reach those ports. `AGENTKIT_REAL_INFRA=1 pytest -m real_infra tests/real` works against this stack without a `REDIS_TEST_URL` — pass `REDIS_PASSWORD` (or let it come from `.env`) with any override URL. - **The guard fails closed.** The `migrations` service runs - `python scripts/check_deploy_credentials.py` before anything else. It rejects the image - defaults, the documented placeholders, short passwords, a missing read-only URL, a read-only - role equal to the application role, and a `REDIS_URL` without a password. + `python scripts/check_deploy_credentials.py` before anything else. It audits the credentials + actually used, whether they come from components or from an explicit URL: it rejects the + image defaults, the documented placeholders, short passwords, a URL with no password, a + malformed URL, a missing read-only URL, a read-only role equal to the application role, and + a `REDIS_URL` without a password. A URL-only deployment is not required to repeat the + `*_PASSWORD` components it does not use. - **Read-only SQL role.** `python scripts/init_readonly_role.py` provisions `POSTGRES_READONLY_USER` with `CONNECT`/`USAGE`/`SELECT` only and `default_transaction_read_only = on`. The `sql_readonly` tool connects with it and refuses to diff --git a/scripts/check_deploy_credentials.py b/scripts/check_deploy_credentials.py index 69bbdf0..c128909 100644 --- a/scripts/check_deploy_credentials.py +++ b/scripts/check_deploy_credentials.py @@ -5,6 +5,11 @@ reads the data. This guard runs before migrations in the compose ``migrations`` service and refuses to continue while any of those values are still in place. +Both credential forms are audited: the ``POSTGRES_*``/``REDIS_PASSWORD`` components and a full +URL that carries its own userinfo. The URL is resolved exactly the way +:class:`agentkit.config.Settings` resolves it (an explicit URL wins, otherwise the components +are escaped into one), so the guard inspects the credentials the application will really use. + It is deliberately a deployment concern, not a ``Settings`` validator: local development and the hermetic test suite use throwaway credentials and must not be forced through it. @@ -21,7 +26,8 @@ import sys from collections.abc import Mapping -from sqlalchemy.engine import make_url +from sqlalchemy.engine import URL, make_url +from sqlalchemy.exc import ArgumentError from agentkit.config import ( DEFAULT_POSTGRES_USER, @@ -53,12 +59,17 @@ #: Prefixes used by the documented example values. PLACEHOLDER_PREFIXES = ("change_me", "changeme", "your_", "replace_me", "placeholder", "todo") -#: Secrets the compose stack depends on. -REQUIRED_PASSWORD_VARS = ("POSTGRES_PASSWORD", "POSTGRES_READONLY_PASSWORD", "REDIS_PASSWORD") +#: Variables that carry a datastore password. Each is validated whenever it is set, but only +#: *required* when it is the source of the resolved URL: a deployment that pins a complete URL +#: with its own credentials does not need the components at all. +COMPONENT_PASSWORD_VARS = ("POSTGRES_PASSWORD", "POSTGRES_READONLY_PASSWORD", "REDIS_PASSWORD") #: A short password on a loopback-only dev box is a nuisance; on a shared host it is a hole. MIN_PASSWORD_LENGTH = 12 +#: Appended to every component-password problem so the fix is one copy/paste away. +_GENERATE_HINT = "generate one with `openssl rand -base64 24`." + def _is_weak_password(value: str) -> str | None: """Return the reason a password is unacceptable, or None when it is specific enough.""" @@ -74,6 +85,39 @@ def _is_weak_password(value: str) -> str | None: return None +def _parse_url(label: str, url: str) -> tuple[URL | None, list[str]]: + """Parse a connection URL, reporting a malformed value instead of raising. + + The parser's own exception is deliberately not reused: SQLAlchemy reproduces the string it + could not parse, and a malformed URL is exactly where a credential is most likely to be + visible. The caller gets a stable message naming only the variable. + """ + try: + return make_url(url), [] + except (ArgumentError, ValueError): + return None, [f"{label} is not a valid connection URL."] + + +def _weak_password_problem(label: str, password: str | None) -> str | None: + """Return a problem when ``password`` is absent or fails the deployment policy.""" + if not password: + return f"{label} has no password." + reason = _is_weak_password(password) + if reason is not None: + return f"{label} password {reason}." + return None + + +def _password_in_url(url: str) -> str | None: + """Return the password encoded in ``url`` itself, or None when it has none.""" + if not url: + return None + try: + return make_url(url).password + except (ArgumentError, ValueError): + return None + + def audit_credentials(env: Mapping[str, str]) -> list[str]: """Check the deployment's datastore credentials. @@ -85,10 +129,19 @@ def audit_credentials(env: Mapping[str, str]) -> list[str]: """ problems: list[str] = [] - for var in REQUIRED_PASSWORD_VARS: - reason = _is_weak_password(env.get(var, "")) - if reason is not None: - problems.append(f"{var} {reason}; generate one with `openssl rand -base64 24`.") + # A component password is validated whenever it is set, because compose always uses it to + # create the role/database. It is only *required* when it is the source of the resolved + # URL, so a URL-only deployment is not scolded for variables it does not use. + for var in COMPONENT_PASSWORD_VARS: + value = env.get(var) + if value: + reason = _is_weak_password(value) + if reason is not None: + problems.append(f"{var} {reason}; {_GENERATE_HINT}") + + explicit_app = bool((env.get("DATABASE_URL") or "").strip()) + explicit_readonly = bool((env.get("DATABASE_READONLY_URL") or "").strip()) + explicit_redis = _password_in_url((env.get("REDIS_URL") or "").strip()) is not None # Resolved exactly the way Settings resolves them: an explicit URL wins, otherwise the # URL is assembled from the POSTGRES_* components with the credentials escaped. Auditing @@ -113,37 +166,67 @@ def audit_credentials(env: Mapping[str, str]) -> list[str]: ) or "" ) + raw_redis = (env.get("REDIS_URL") or "").strip() + redis_url = apply_redis_password(raw_redis, env.get("REDIS_PASSWORD")) app_role: str | None = None - if app_url: - parsed_app = make_url(app_url) - app_role = parsed_app.username - if parsed_app.password and parsed_app.password in WEAK_PASSWORDS: - problems.append("DATABASE_URL uses a default password.") - - if readonly_url: - parsed_ro = make_url(readonly_url) - if not parsed_ro.username or not parsed_ro.password: - problems.append("DATABASE_READONLY_URL must include a username and a password.") - if app_role and parsed_ro.username == app_role: - problems.append( - "DATABASE_READONLY_URL names the application role; it must be a separate " - "non-superuser role." - ) - if parsed_ro.password and app_url and parsed_ro.password == make_url(app_url).password: - problems.append( - "DATABASE_READONLY_URL reuses the application password; give the read-only " - "role its own credential." - ) + app_password: str | None = None + if not app_url: + problems.append(f"POSTGRES_PASSWORD is not set; {_GENERATE_HINT}") else: + parsed_app, parse_problems = _parse_url("DATABASE_URL", app_url) + problems.extend(parse_problems) + if parsed_app is not None: + app_role = parsed_app.username + app_password = parsed_app.password + if explicit_app: + # The component form was validated above; only the URL's own secret remains. + problem = _weak_password_problem("DATABASE_URL", parsed_app.password) + if problem is not None: + problems.append(problem) + + if not readonly_url: problems.append( "DATABASE_READONLY_URL is not set; the sql_readonly tool would fail closed. Set " "POSTGRES_READONLY_PASSWORD (or the URL) so the role can be provisioned." ) - - redis_url = apply_redis_password(env.get("REDIS_URL", ""), env.get("REDIS_PASSWORD")) - if not redis_url or not make_url(redis_url).password: + else: + parsed_ro, parse_problems = _parse_url("DATABASE_READONLY_URL", readonly_url) + problems.extend(parse_problems) + if parsed_ro is not None: + if not parsed_ro.username or not parsed_ro.password: + problems.append("DATABASE_READONLY_URL must include a username and a password.") + elif explicit_readonly: + problem = _weak_password_problem("DATABASE_READONLY_URL", parsed_ro.password) + if problem is not None: + problems.append(problem) + if app_role and parsed_ro.username == app_role: + problems.append( + "DATABASE_READONLY_URL names the application role; it must be a separate " + "non-superuser role." + ) + if parsed_ro.password and app_password and parsed_ro.password == app_password: + problems.append( + "DATABASE_READONLY_URL reuses the application password; give the read-only " + "role its own credential." + ) + + if not redis_url: problems.append("REDIS_URL has no password; Redis must not be reachable unauthenticated.") + else: + parsed_redis, parse_problems = _parse_url("REDIS_URL", redis_url) + problems.extend(parse_problems) + if parsed_redis is not None: + if not parsed_redis.password: + problems.append( + "REDIS_URL has no password; Redis must not be reachable unauthenticated." + ) + elif explicit_redis: + # REDIS_PASSWORD never overrides a password already present in the URL, so the + # URL's own secret is the one that needs auditing here. + problem = _weak_password_problem("REDIS_URL", parsed_redis.password) + if problem is not None: + problems.append(problem) return problems diff --git a/tests/unit/test_deploy_credentials.py b/tests/unit/test_deploy_credentials.py index 01fee4a..84d1ef5 100644 --- a/tests/unit/test_deploy_credentials.py +++ b/tests/unit/test_deploy_credentials.py @@ -39,7 +39,6 @@ def test_specific_credentials_pass() -> None: ("POSTGRES_PASSWORD", "postgres"), ("POSTGRES_PASSWORD", "change_me"), ("POSTGRES_READONLY_PASSWORD", "readonly"), - ("REDIS_PASSWORD", ""), ("REDIS_PASSWORD", "short"), ], ) @@ -83,7 +82,7 @@ def test_redis_url_without_password_is_reported() -> None: ``REDIS_URL`` may be credential-less as long as ``REDIS_PASSWORD`` supplies the password, so the failure case is having neither. """ - problems = audit_credentials(_env(REDIS_URL="redis://redis:6379/0", REDIS_PASSWORD=None)) + problems = audit_credentials(_env(REDIS_URL="redis://redis:6379/0", REDIS_PASSWORD="")) assert any("REDIS_URL has no password" in problem for problem in problems) @@ -99,6 +98,79 @@ def test_redis_password_completes_a_credential_less_url() -> None: assert problems == [] +@pytest.mark.parametrize("password", ["postgres", "change_me_generate_with_openssl", "short"]) +def test_weak_password_in_explicit_app_url_is_rejected(password: str) -> None: + """A URL-supplied secret must pass the same policy as a component.""" + problems = audit_credentials( + _env(DATABASE_URL=f"postgresql+asyncpg://agentkit:{password}@postgres:5432/agentkit") + ) + assert any(problem.startswith("DATABASE_URL password") for problem in problems) + + +@pytest.mark.parametrize("password", ["readonly", "change_me", "short"]) +def test_weak_password_in_explicit_readonly_url_is_rejected(password: str) -> None: + """The read-only credential is audited even when it is not a component.""" + problems = audit_credentials( + _env( + DATABASE_READONLY_URL=( + f"postgresql+asyncpg://agentkit_readonly:{password}@postgres:5432/agentkit" + ) + ) + ) + assert any(problem.startswith("DATABASE_READONLY_URL password") for problem in problems) + + +def test_weak_password_in_explicit_redis_url_is_rejected() -> None: + """REDIS_PASSWORD does not override a URL secret, so the URL secret is audited.""" + problems = audit_credentials( + _env(REDIS_URL="redis://:change_me@redis:6379/0", REDIS_PASSWORD="cache_password_4b18") + ) + assert any(problem.startswith("REDIS_URL password") for problem in problems) + + +def test_url_only_deployment_without_component_passwords_passes() -> None: + """An explicit URL is authoritative; unused component variables must not be demanded.""" + problems = audit_credentials( + { + "DATABASE_URL": APP_URL, + "DATABASE_READONLY_URL": READONLY_URL, + "REDIS_URL": REDIS_URL, + } + ) + assert problems == [] + + +def test_explicit_app_url_without_a_password_is_reported() -> None: + """A URL with a username but no password cannot be authenticated.""" + problems = audit_credentials( + _env(DATABASE_URL="postgresql+asyncpg://agentkit@postgres:5432/agentkit") + ) + assert "DATABASE_URL has no password." in problems + + +@pytest.mark.parametrize( + "url", + [ + "not a url", + "postgresql+asyncpg://agentkit:app_password_9f3a@postgres:notaport/agentkit", + ], +) +def test_malformed_urls_are_reported_instead_of_raising(url: str) -> None: + """A bad URL is a deployment problem, not a traceback in the migrations service.""" + problems = audit_credentials(_env(DATABASE_URL=url)) + assert "DATABASE_URL is not a valid connection URL." in problems + + +def test_problem_messages_never_echo_the_secret() -> None: + """The guard reports variable names, never the credential it rejected.""" + secret = "hunter2" + problems = audit_credentials( + _env(DATABASE_URL=f"postgresql+asyncpg://agentkit:{secret}@postgres:5432/agentkit") + ) + assert problems + assert all(secret not in problem for problem in problems) + + def test_main_reports_problems_on_stderr(capsys: pytest.CaptureFixture[str]) -> None: """The guard exits non-zero and names each problem, so CI logs are actionable.""" exit_code = main(_env(POSTGRES_PASSWORD="postgres")) From 1eefc8b70d7b29fd2ac9a940cca8c4deb3d4ce91 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:26:04 +0530 Subject: [PATCH 2/7] refactor(scripts): log credential problems instead of printing AGENTS.md forbids print() in favour of module logging. The deployment guard wrote its failure report straight to stderr, which also made the message untestable through caplog and bypassed any logging configuration the caller had installed. Emit the header and each problem through the module logger at ERROR, leaving handler configuration to the __main__ block so importing the module still has no side effects. The compose migrations service keeps stderr output via basicConfig, the exit code is unchanged, and the test asserts the log records; a second test pins the no-print rule. --- scripts/check_deploy_credentials.py | 15 ++++++++++++--- tests/unit/test_deploy_credentials.py | 19 ++++++++++++++----- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/scripts/check_deploy_credentials.py b/scripts/check_deploy_credentials.py index c128909..60c5a9a 100644 --- a/scripts/check_deploy_credentials.py +++ b/scripts/check_deploy_credentials.py @@ -22,6 +22,7 @@ from __future__ import annotations +import logging import os import sys from collections.abc import Mapping @@ -36,6 +37,8 @@ resolve_postgres_url, ) +logger = logging.getLogger(__name__) + #: Passwords that are the image defaults, the documented examples, or a bare service name. WEAK_PASSWORDS = frozenset( { @@ -232,15 +235,21 @@ def audit_credentials(env: Mapping[str, str]) -> list[str]: def main(env: Mapping[str, str] | None = None) -> int: - """Run the audit against the environment and report the result.""" + """Run the audit against the environment and report the result. + + Problems are logged at ``ERROR`` so the caller's logging setup decides where they go. + Importing this module has no side effects; the ``__main__`` block below installs the + stderr handler used by the compose ``migrations`` service. + """ problems = audit_credentials(os.environ if env is None else env) if problems: - print("Refusing to continue: insecure deployment credentials.", file=sys.stderr) + logger.error("Refusing to continue: insecure deployment credentials.") for problem in problems: - print(f" - {problem}", file=sys.stderr) + logger.error(" - %s", problem) return 1 return 0 if __name__ == "__main__": + logging.basicConfig(level=logging.INFO, format="%(levelname)s %(name)s: %(message)s") sys.exit(main()) diff --git a/tests/unit/test_deploy_credentials.py b/tests/unit/test_deploy_credentials.py index 84d1ef5..d3db364 100644 --- a/tests/unit/test_deploy_credentials.py +++ b/tests/unit/test_deploy_credentials.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging from pathlib import Path from typing import Any @@ -171,13 +172,21 @@ def test_problem_messages_never_echo_the_secret() -> None: assert all(secret not in problem for problem in problems) -def test_main_reports_problems_on_stderr(capsys: pytest.CaptureFixture[str]) -> None: +def test_main_logs_problems_and_exits_non_zero(caplog: pytest.LogCaptureFixture) -> None: """The guard exits non-zero and names each problem, so CI logs are actionable.""" - exit_code = main(_env(POSTGRES_PASSWORD="postgres")) - captured = capsys.readouterr() + with caplog.at_level(logging.ERROR, logger="scripts.check_deploy_credentials"): + exit_code = main(_env(POSTGRES_PASSWORD="postgres")) assert exit_code == 1 - assert "Refusing to continue" in captured.err - assert "POSTGRES_PASSWORD" in captured.err + assert "Refusing to continue" in caplog.text + assert "POSTGRES_PASSWORD" in caplog.text + + +def test_no_print_calls_remain_in_the_guard() -> None: + """AGENTS.md forbids print(); deployment output goes through the logger.""" + source = (Path(__file__).parents[2] / "scripts/check_deploy_credentials.py").read_text( + encoding="utf-8" + ) + assert "print(" not in source def test_compose_binds_datastores_to_loopback_and_requires_passwords() -> None: From 06c20dd6a00e2d05c35913aa4bd457c4af0956f4 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:26:19 +0530 Subject: [PATCH 3/7] test(real-infra): fail instead of skipping when role provisioning errors The readonly_engine fixture treated every exception from provision_readonly_role() as 'environment unavailable' and skipped the test. A regression in the security-critical provisioner would therefore turn most of the SQL isolation tier green-by-skip, even though the upstream real_database_url fixture already fails loudly when PostgreSQL is unreachable (it applies migrations first). Let the provisioning error propagate so CI goes red. The upstream fixture still skips when AGENTKIT_REAL_INFRA is unset, so local development ergonomics are unchanged. --- tests/real/test_sql_isolation_real.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/tests/real/test_sql_isolation_real.py b/tests/real/test_sql_isolation_real.py index 18b288e..2a44210 100644 --- a/tests/real/test_sql_isolation_real.py +++ b/tests/real/test_sql_isolation_real.py @@ -39,12 +39,15 @@ def _readonly_url(admin_url: str) -> str: @pytest.fixture async def readonly_engine(real_database_url: str) -> AsyncIterator[AsyncEngine]: - """Provision the dedicated role and yield an engine connected as it.""" + """Provision the dedicated role and yield an engine connected as it. + + No provisioning failure is skipped here. ``real_database_url`` has already applied + migrations and fails with a clear message when PostgreSQL is unreachable, so anything + this call raises is a provisioning defect (or a role that cannot be restricted) and must + turn CI red rather than degrade the isolation tests to skips. + """ url = _readonly_url(real_database_url) - try: - await provision_readonly_role(real_database_url, url) - except Exception as exc: # pragma: no cover - environment-dependent - pytest.skip(f"Could not provision the read-only role: {exc}") + await provision_readonly_role(real_database_url, url) engine = create_async_engine(url, pool_pre_ping=True) try: From a20269336a4355158f22867c032cb0ca3364b27e Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:26:55 +0530 Subject: [PATCH 4/7] docs(audit): reconcile the stack history and drop the stale AST claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AUDIT-16 corrected the README but left two inconsistent claims behind. docs/AUDIT.md still described the remediation as 'a linear stack of nine branches', while the merged PR contains a 12-layer stack (14 commits once the two progress-bookkeeping commits are counted); the title, body and docs each used a different number. The historical M4-T04 row in docs/PROGRESS.md still said the SQL tool uses 'Strict SELECT AST validation', although the implementation is a lexer with a dedicated read-only role as the primary control — exactly the claim the README correction removed. Record the canonical 12-layer/14-commit description in AUDIT.md, rewrite the M4-T04 row to match the code, and extend the docs-accuracy test so neither claim can drift back. --- docs/AUDIT.md | 3 ++- docs/PROGRESS.md | 2 +- tests/unit/test_docs_accuracy.py | 15 +++++++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/docs/AUDIT.md b/docs/AUDIT.md index 5ad4c4f..8688a62 100644 --- a/docs/AUDIT.md +++ b/docs/AUDIT.md @@ -66,7 +66,8 @@ Per the AgentKit specification and development rules: ## 4. Consolidated AUDIT-01…18 Remediation (2026-10-25) A second whole-codebase audit filed 18 standalone findings as GitHub issues (`AUDIT-01` … -`AUDIT-18`). They were fixed as a linear stack of nine branches; this table is the index, and +`AUDIT-18`). They were fixed as a linear 12-layer stack, merged as 14 commits (12 layer +commits plus two progress-bookkeeping commits); this table is the index, and `docs/PROGRESS.md` records the per-layer detail. | Finding | Severity | Issue | Branch | Resolution | diff --git a/docs/PROGRESS.md b/docs/PROGRESS.md index 51251fe..dff86f5 100644 --- a/docs/PROGRESS.md +++ b/docs/PROGRESS.md @@ -21,7 +21,7 @@ This document tracks execution progress across all backlog tasks, including pull | M4-T01 | M4 | Safe AST Calculator Tool | `feat/safe-calculator-tool` | #14 | 2 | Done | Strict AST arithmetic parser with zero eval/exec and DoS guards | | M4-T02 | M4 | Sandboxed File Reader Tool | `feat/sandboxed-file-reader-tool` | #15 | 2 | Done | FILE_TOOL_BASE_DIR sandboxing, path traversal & symlink defense | | M4-T03 | M4 | SSRF-Protected HTTP Fetch Tool | `feat/ssrf-http-fetch-tool` | #16 | 2 | Done | DNS & IP SSRF validation, private/metadata blocking, safe redirect | -| M4-T04 | M4 | Read-Only SQL Tool | `feat/readonly-sql-tool` | #17 | 2 | Done | Strict SELECT AST validation, DDL/DML rejection, Markdown formatting | +| M4-T04 | M4 | Read-Only SQL Tool | `feat/readonly-sql-tool` | #17 | 2 | Done | Single SELECT/WITH lexer validation (dedicated read-only role is the primary control), DDL/DML rejection, Markdown formatting | | M4-T05 | M4 | Web Search Tool Adapter | `feat/web-search-tool` | #18 | 2 | Done | Tavily search integration, error handling for missing key, formatted snippets | | M5-T01 | M5 | TraceSink Protocol & InMemoryTraceSink | `feat/trace-sink-base` | #19 | 2 | Done | StepTrace model, TraceSink ABC, InMemoryTraceSink, Agent.run() trace wiring | | M5-T02 | M5 | Contextual Structured JSON Logger | `feat/structured-logging` | #20 | 2 | Done | JSONFormatter, contextvars run_id/step_no propagation, Agent logging | diff --git a/tests/unit/test_docs_accuracy.py b/tests/unit/test_docs_accuracy.py index 2e9421b..260392e 100644 --- a/tests/unit/test_docs_accuracy.py +++ b/tests/unit/test_docs_accuracy.py @@ -71,6 +71,21 @@ def test_audit_table_records_the_sql_and_gemini_findings() -> None: assert "**PASS (after AUDIT-04/05/06)**" in audit +def test_progress_no_longer_claims_ast_validation_for_sql() -> None: + """The historical M4 row must not contradict the corrected README.""" + progress = _read("docs/PROGRESS.md") + row = next(line for line in progress.splitlines() if line.startswith("| M4-T04")) + assert "AST" not in row + + +def test_audit_history_uses_the_canonical_stack_description() -> None: + """The merged stack is 12 layers / 14 commits; earlier counts had drifted.""" + audit = _read("docs/AUDIT.md") + assert "12-layer stack" in audit + assert "14 commits" in audit + assert "nine branches" not in audit + + def test_every_audit_finding_appears_in_the_remediation_table() -> None: """A closed finding that is missing from the table is invisible to reviewers.""" audit = _read("docs/AUDIT.md") From d3d3e958cc6ea811d17993598a82d7ffcd2e3401 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:27:26 +0530 Subject: [PATCH 5/7] chore(repo): ignore the local uv lockfile uv.lock was untracked and not ignored, so every checkout reported it as noise and contributors could not tell whether dependency resolution was meant to be reproducible. The project installs from pyproject.toml with pip in all three CI jobs, and nothing consumes a uv lock, so tracking one would be misleading. Ignore it explicitly and document why in the ignore file. Adopting uv properly (commit the lock and switch CI to 'uv sync --locked') remains a separate decision. --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index 87fd69c..d3cd907 100644 --- a/.gitignore +++ b/.gitignore @@ -24,6 +24,11 @@ wheels/ .installed.cfg *.egg +# Dependency lockfiles +# CI installs from pyproject.toml with pip (see .github/workflows/ci.yml); a uv lockfile is a +# local convenience only and is not part of the reproducible build. +uv.lock + # Virtual environments .venv/ venv/ From 59c14992a5797edf74127069ce8c4043233cf1a6 Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:28:56 +0530 Subject: [PATCH 6/7] docs(progress): record PR #90 for the follow-up fixes 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. --- docs/PROGRESS.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/PROGRESS.md b/docs/PROGRESS.md index dff86f5..e1a0f30 100644 --- a/docs/PROGRESS.md +++ b/docs/PROGRESS.md @@ -71,6 +71,11 @@ This document tracks execution progress across all backlog tasks, including pull | AUDIT-15/17 | AUDIT | Loop Detection and API Key Auth | `fix/agent-loop-and-auth` | #84 | 1 | Done | Sliding-window duplicate detector; hmac.compare_digest with normalised keys | | AUDIT-16 | AUDIT | Documentation Claim Corrections | `docs/correct-audit-claims` | #84 | 1 | Done | README/ARCHITECTURE/AUDIT/ADR-006/PROGRESS/PLAN corrected and guarded by tests | | AUDIT-18-FU | AUDIT | Real-Infra Redis Authentication | `fix/real-infra-redis-auth` | #84 | 1 | Done | `REDIS_PASSWORD` completes a credential-less `REDIS_URL` for the app and the live suite; 18 fixture errors fixed | +| FU-86 | AUDIT | Credential Guard URL Password Audit | `fix/deploy-credential-url-passwords` | #90 | 1 | Done | URL-embedded passwords audited with the component policy; malformed URLs reported, not raised | +| FU-87 | AUDIT | Credential Guard Logging | `refactor/deploy-credential-logging` | #90 | 1 | Done | Guard reports through module logging instead of `print()`; handlers only in `__main__` | +| FU-85 | AUDIT | Real-Infra Provisioning Failures | `test/real-infra-provisioning-failures` | #90 | 1 | Done | Provisioning errors fail the live SQL-isolation tier instead of skipping it | +| FU-88 | AUDIT | Audit History Consistency | `docs/audit-history-consistency` | #90 | 1 | Done | Canonical 12-layer/14-commit wording; stale "AST validation" row corrected and guarded | +| FU-89 | AUDIT | uv Lockfile Policy | `chore/repo-uv-lock` | #90 | 1 | Done | `uv.lock` ignored; pip-based CI does not consume a uv lock | --- @@ -489,3 +494,14 @@ This document tracks execution progress across all backlog tasks, including pull - **PR**: stacked on `fix/pg-url-credentials` — PR #84 - **Commits**: 1 - **Notes**: `scripts/init_readonly_role.py` changed only the password of a role that already existed, so a pre-existing `SUPERUSER`, `CREATEDB`, `CREATEROLE`, `REPLICATION` or `BYPASSRLS` role — or one that was a member of `pg_read_server_files` — kept everything the documented `SELECT`-only contract says it does not have. Provisioning now restricts first: `restriction_statements()` revokes those attributes and all privileges on the database, the `public` schema, tables, sequences and functions, `_revoke_memberships()` drops every direct membership, and only then are `CONNECT`/`USAGE`/`SELECT` granted back. It refuses a role that owns the target database (an owner can grant itself anything) and `_assert_restricted()` verifies the outcome, raising `ReadonlyRoleConfigError` rather than reporting a privileged role as provisioned. The tool checks the same properties per session in one round trip — `rolsuper`, `rolbypassrls` and membership of `pg_read_server_files`, `pg_write_server_files`, `pg_execute_server_program`, `pg_signal_backend`, `pg_monitor` or `pg_maintain`, resolved through `pg_has_role` so indirect membership counts — and refuses before the query runs. The statement lexer also 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. Covered by `tests/unit/test_readonly_role_provisioning.py`, new payloads in `tests/unit/test_sql_isolation.py`, and `tests/real/test_sql_isolation_real.py::test_reused_role_has_its_privileges_stripped`, which grants `CREATEDB` plus `pg_read_server_files` to the role and asserts provisioning strips both. Recorded in ADR-020. + +### PR #84 Follow-ups (#85–#89) +- **Status**: Done +- **PR**: stacked on `main` — PR #90 (five layer commits plus one progress-bookkeeping commit) +- **Commits**: 5 +- **Notes**: + - **#86 (guard correctness)** — `scripts/check_deploy_credentials.py` applied the weak-password policy only to the `POSTGRES_*` and `REDIS_PASSWORD` components. An explicit URL that carried its own userinfo bypassed it: `DATABASE_URL` was checked only against the exact known-password set, the read-only URL was never policy-checked, and `REDIS_URL` was checked for presence alone, so `redis://:short@host` passed the 12-character rule. `audit_credentials()` now resolves each URL exactly as `Settings` does and applies `_is_weak_password()` to URL-supplied secrets, requires a password in an explicit URL, validates a component whenever it is set but requires it only when it is the resolution source, and parses each URL once through a helper that reports "is not a valid connection URL" rather than leaking a SQLAlchemy traceback or the secret. `docs/DEPLOY.md` describes both credential forms. + - **#87 (logging)** — `main()` emits the header and one `ERROR` record per problem through `logging.getLogger(__name__)`; `basicConfig()` is confined to the `__main__` block so importing the module has no side effects. Exit code and message text are unchanged, and a test pins the no-`print()` rule from `AGENTS.md`. + - **#85 (test reliability)** — `tests/real/test_sql_isolation_real.py::readonly_engine` no longer converts `provision_readonly_role()` failures into `pytest.skip`. The upstream `real_database_url` fixture already applies migrations and fails loudly when PostgreSQL is unreachable, so a provisioning error is a defect and now turns CI red instead of leaving the isolation tier green-by-skip. + - **#88 (docs)** — `docs/AUDIT.md` records the canonical "linear 12-layer stack, merged as 14 commits (12 layer commits plus two progress-bookkeeping commits)" wording instead of "nine branches", and the historical M4-T04 row describes the lexer plus dedicated read-only role instead of "Strict SELECT AST validation". `tests/unit/test_docs_accuracy.py` guards both claims. + - **#89 (repo hygiene)** — `.gitignore` ignores `uv.lock` with a comment explaining that all three CI jobs install from `pyproject.toml` with pip, so a lockfile is not part of the reproducible build. Adopting uv properly (commit the lock, switch CI to `uv sync --locked`) remains a separate decision. From 713735b4d46d42320a38efb5604fc513e093df9c Mon Sep 17 00:00:00 2001 From: Amar K P Date: Mon, 26 Oct 2026 22:35:42 +0530 Subject: [PATCH 7/7] fix(deploy): report malformed Redis URLs instead of crashing Codex review of #90: with REDIS_PASSWORD set, apply_redis_password() splits REDIS_URL with urlsplit() before the audit parses it. A malformed IPv6 authority such as redis://[broken makes urlsplit raise ValueError, so the guard died with a traceback instead of returning the actionable problem line the rest of the audit produces. Resolve the Redis URL through a helper that catches the split error and reports 'REDIS_URL is not a valid connection URL.' A failed split also suppresses the follow-on 'has no password' message, since there is no URL to inspect. Add regression tests for the unsplittable authority and for a bad port, and verified by hand that the guard exits 1 with the problem line and no traceback. --- scripts/check_deploy_credentials.py | 23 +++++++++++++++++++++-- tests/unit/test_deploy_credentials.py | 12 ++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/scripts/check_deploy_credentials.py b/scripts/check_deploy_credentials.py index 60c5a9a..7cd8232 100644 --- a/scripts/check_deploy_credentials.py +++ b/scripts/check_deploy_credentials.py @@ -121,6 +121,21 @@ def _password_in_url(url: str) -> str | None: return None +def _resolve_redis_url(url: str, password: str | None) -> tuple[str, list[str]]: + """Complete a passwordless Redis URL, reporting a malformed value instead of raising. + + ``apply_redis_password`` splits the URL with ``urlsplit``, which raises ``ValueError`` on a + malformed authority (``redis://[broken``). That call happens before ``_parse_url``, so an + unguarded value would abort the guard with a traceback instead of the actionable problem + line the rest of the audit produces. A failed split also means there is no URL to inspect, + so the caller must not additionally report a missing password. + """ + try: + return apply_redis_password(url, password), [] + except ValueError: + return "", ["REDIS_URL is not a valid connection URL."] + + def audit_credentials(env: Mapping[str, str]) -> list[str]: """Check the deployment's datastore credentials. @@ -170,7 +185,8 @@ def audit_credentials(env: Mapping[str, str]) -> list[str]: or "" ) raw_redis = (env.get("REDIS_URL") or "").strip() - redis_url = apply_redis_password(raw_redis, env.get("REDIS_PASSWORD")) + redis_url, redis_problems = _resolve_redis_url(raw_redis, env.get("REDIS_PASSWORD")) + problems.extend(redis_problems) app_role: str | None = None app_password: str | None = None @@ -215,7 +231,10 @@ def audit_credentials(env: Mapping[str, str]) -> list[str]: ) if not redis_url: - problems.append("REDIS_URL has no password; Redis must not be reachable unauthenticated.") + if not redis_problems: + problems.append( + "REDIS_URL has no password; Redis must not be reachable unauthenticated." + ) else: parsed_redis, parse_problems = _parse_url("REDIS_URL", redis_url) problems.extend(parse_problems) diff --git a/tests/unit/test_deploy_credentials.py b/tests/unit/test_deploy_credentials.py index d3db364..5e51e4a 100644 --- a/tests/unit/test_deploy_credentials.py +++ b/tests/unit/test_deploy_credentials.py @@ -162,6 +162,18 @@ def test_malformed_urls_are_reported_instead_of_raising(url: str) -> None: assert "DATABASE_URL is not a valid connection URL." in problems +@pytest.mark.parametrize("url", ["redis://[broken", "redis://host:notaport/0"]) +def test_malformed_redis_url_is_reported_instead_of_raising(url: str) -> None: + """URL splitting happens before parsing, so it needs the same guard as the databases. + + ``redis://[broken`` makes ``urlsplit`` raise ``ValueError`` while ``REDIS_PASSWORD`` is + being embedded. That used to abort the audit with a traceback before any problem line was + printed, which is the opposite of what a fail-closed deployment guard should do. + """ + problems = audit_credentials(_env(REDIS_URL=url)) + assert problems == ["REDIS_URL is not a valid connection URL."] + + def test_problem_messages_never_echo_the_secret() -> None: """The guard reports variable names, never the credential it rejected.""" secret = "hunter2"