Repository navigation
fix(audit): close the five PR #84 review follow-ups as a five-layer stack - #90
Conversation
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.
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.
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.
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.
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.
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. |
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.
![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: d3d3e958cc
ℹ️ 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".
| or "" | ||
| ) | ||
| raw_redis = (env.get("REDIS_URL") or "").strip() | ||
| redis_url = apply_redis_password(raw_redis, env.get("REDIS_PASSWORD")) |
There was a problem hiding this comment.
When REDIS_PASSWORD is set and REDIS_URL contains a malformed IPv6 authority such as redis://[broken, apply_redis_password() raises ValueError in urlsplit() before _parse_url() can report the malformed URL. The guard then exits with a traceback instead of returning the promised actionable validation error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 713735b.
apply_redis_password() does split the URL before the audit ever reaches _parse_url(), so the finding was real: redis://[broken with REDIS_PASSWORD set killed the guard with ValueError: Invalid IPv6 URL.
_resolve_redis_url()now wraps the call and turns a split failure intoREDIS_URL is not a valid connection URL.- A failed split also suppresses the follow-on "has no password" line, since there is no URL left to inspect.
test_malformed_redis_url_is_reported_instead_of_raisingcovers bothredis://[broken(split error) andredis://host:notaport/0(parse error).- By hand: the guard exits
1with that single problem line and no traceback.
All four CI jobs are green on 713735b.
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.
|
Review fix pushed as What Codex caught: with What changed:
Verification on CI: Lint & Type Check, Test Hermetic (3.11, 3.12) and Test Against Live PostgreSQL and Redis all pass. PR body updated with the review-fix row. @codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
docs(progress): correct the PR #90 stack record
Summary
Closes the five follow-up issues filed from the PR #84 review (#85–#89) as a linear five-layer stack, one commit per layer, plus a progress-bookkeeping commit that records the stack in
docs/PROGRESS.mdand a review-fix commit for the finding Codex raised on this PR. This PR is the tip, so reviewing commit-by-commit walks the layers in dependency order and merging lands all six commits onmainat once.fix/deploy-credential-url-passwords3242d6dDATABASE_URL/DATABASE_READONLY_URL/REDIS_URLwith the same policy as components, requires a password in an explicit URL, reports malformed URLs instead of raising, and stops demanding component variables an explicit URL does not use.refactor/deploy-credential-logging1eefc8blogginginstead ofprint(), with handler setup confined to__main__.test/real-infra-provisioning-failures06c20dddocs/audit-history-consistencya202693docs/AUDIT.mdrecords the canonical 12-layer / 14-commit history, and the stale "Strict SELECT AST validation" row indocs/PROGRESS.mdis corrected; both are guarded by the docs-accuracy test.chore/repo-uv-lockd3d3e95uv.lockis ignored instead of appearing as untracked noise; CI installs frompyproject.tomlwith pip, so a lockfile is not part of the current build.59c1499docs/PROGRESS.mdgains one row per follow-up pointing at this PR and a combined Detailed Task Notes section.713735bredis://[brokenmadeurlsplit()raise before validation, so the guard crashed instead of reporting it.What changed, per issue
audit_credentials()still resolves URLs throughresolve_postgres_url()/apply_redis_password(), so it audits exactly whatSettingsuses. URL-supplied secrets now go through_is_weak_password()(defaults, placeholders, <12 chars), an explicit URL with no password is reported, andmake_url()failures become "<VAR>is not a valid connection URL." without echoing SQLAlchemy's parse error or the secret. Component passwords are validated when present but only required when they are the source of the resolved URL.main()emits the header and oneERRORrecord per problem;basicConfig()lives in theif __name__ == "__main__"block. Exit code and message text are unchanged.readonly_enginecallsprovision_readonly_role()directly.real_database_urlalready applies migrations and fails with a clear message on an unreachable database, so the skip was masking real defects.tests/unit/test_docs_accuracy.pypins both..gitignoregainsuv.lockwith a comment explaining that the pip-based CI does not consume it. Adopting uv properly remains a separate decision.713735b) —_resolve_redis_url()wrapsapply_redis_password(), which callsurlsplit()before the audit parses anything. A malformed IPv6 authority (redis://[broken) now yields "REDIS_URL is not a valid connection URL." instead of a traceback, and a failed split suppresses the follow-on "no password" message because there is no URL to inspect. Parametrised tests cover the unsplittable authority and a bad port; the guard was also run by hand and exits 1 with the problem line only.Test plan
Run on the tip, all green:
The guard was also run by hand against a mixed weak configuration and exits 1 with one line per problem; the live PostgreSQL/Redis job exercises the changed #85 fixture.
Merge note
Use a merge commit, not squash, so the five layer commits (plus the bookkeeping and review commits) stay individually reviewable in
mainhistory.Closes #85
Closes #86
Closes #87
Closes #88
Closes #89