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/ 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/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/docs/PROGRESS.md b/docs/PROGRESS.md index 51251fe..e1a0f30 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 | @@ -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. diff --git a/scripts/check_deploy_credentials.py b/scripts/check_deploy_credentials.py index 69bbdf0..7cd8232 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. @@ -17,11 +22,13 @@ from __future__ import annotations +import logging import os 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, @@ -30,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( { @@ -53,12 +62,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 +88,54 @@ 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 _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. @@ -85,10 +147,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,51 +184,91 @@ def audit_credentials(env: Mapping[str, str]) -> list[str]: ) or "" ) + raw_redis = (env.get("REDIS_URL") or "").strip() + redis_url, redis_problems = _resolve_redis_url(raw_redis, env.get("REDIS_PASSWORD")) + problems.extend(redis_problems) 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." ) + 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." + ) - redis_url = apply_redis_password(env.get("REDIS_URL", ""), env.get("REDIS_PASSWORD")) - if not redis_url or not make_url(redis_url).password: - problems.append("REDIS_URL has no password; Redis must not be reachable unauthenticated.") + if not redis_url: + 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) + 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 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/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: diff --git a/tests/unit/test_deploy_credentials.py b/tests/unit/test_deploy_credentials.py index 01fee4a..5e51e4a 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 @@ -39,7 +40,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 +83,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,13 +99,106 @@ def test_redis_password_completes_a_credential_less_url() -> None: assert problems == [] -def test_main_reports_problems_on_stderr(capsys: pytest.CaptureFixture[str]) -> None: +@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 + + +@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" + 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_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: 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")