fix: stop validation errors reflecting submitted secrets (credential/password echo) + auth foundation - #40
Merged
Merged
Conversation
…, not enforcement) Builds the authentication machinery and proves it on a deliberately small set of endpoints. Every /engagements route is untouched and still open; applying the gate across them is the next change, and docs/STATUS.md lists which routes are which. The deliverable is that a password and a raw token are one-way and never recoverable. Passwords are argon2id (argon2-cffi, MIT, PX-FREE clean) at OWASP's 64 MiB/t=3/p=4 baseline, deliberately not built on the reversible evidence-crypto layer next door. Session tokens are 256 random bits stored as SHA-256 only, so a stolen auth_token row cannot be replayed as a bearer token. UserRead has no password_hash field at all, so there is no serializer that could publish it (B-FA-07). Bootstrap is one-shot and closes permanently: the guard is a fixed-primary-key marker row rather than a COUNT(admins)==0 check, which loses a concurrent first-run race safely and survives deletion of the admin it created. Roles are admin/operator/viewer with the whole policy encoded as a capability table in app/api/deps.py, plus require_role and require_capability dependencies (B-FA-03). Enforced on GET /auth/me, POST /auth/logout, and the admin-only /users routes only. auth_event is append-only and records every action including a failed login, with no password, digest, or token value in it (PX-SECRETS, PX-EVIDENCE, D-05). Tests: the no-leak detector searches response bodies, response headers, and log records through record.__dict__ rather than getMessage(), closing the gap an extra= payload slips through - and the detector is itself proven to fire on planted leaks. The 403 is attributed by mounting the same handler behind a no-op dependency. Three by-hand mutations (leak the hash, no-op require_role, store the raw token) were each run and each failed the suite. Migration up and down, plus model-vs-migration column comparison for all four tables (W-03). Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
A first-run path and a session the CLI can keep. New provx_cli/store.py is stdlib only, so test_cli_no_bypass.py's assertion that httpx lives in exactly two modules still holds. Tokens land in $XDG_CONFIG_HOME/provx/credentials.json keyed by server URL, the file created 0600 before it is written rather than chmod'd after - a chmod that follows the write leaves a window where the token is world-readable on disk. api.py:_headers() stays the single place a token becomes a header; precedence is $PROVX_TOKEN then the stored token, so the pre-existing environment path keeps working and keeps winning, for CI. No password flag exists on any command (PX-SECRETS), and the guard was widened to match: test_cli_secrets.py now walks the entire subparser tree rather than just credential set, with a control asserting the walk actually reaches login and admin create - otherwise it would pass vacuously. read_secret gained a prompt argument rather than being copied (Q-11). --json login output omits the token: that stream is routinely redirected into a file, and a token there outlives the shell that made it. Logout clears the local token even when the server refuses. Proven unstubbed end to end in test_integration_cli.py: admin create, login, an authenticated call, a refused second bootstrap, and logout invalidating the token. Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
STATUS.md gains an Authentication section whose load-bearing rows are the two lists: the four enforced endpoints, and every engagement route that is deliberately still open, enumerated rather than gestured at. The header, the roadmap phase table, the issues table, and the testing ladder are updated to match. KNOWN_ISSUES.md: authentication moves off the declared-scaffolding list in the established 'was on this list until <branch>' form, and gains a deliberate-limits section for the things a reader would otherwise assume shipped - no failed-login lockout (A-06), no refresh rotation (A-05), no password denylist (S-12), no self-service password change or deactivation endpoint. KI-003 gains an update paragraph: its urgency trigger is not reached by this change, because enforcement does not cover the route that accepts a scope and there is no non-admin operator yet. Both stop being true in the next change, which makes it the top item then. README and QUICKSTART had a prominent 'the API has no authentication' warning. It is now half wrong, which is worse than wholly wrong, so both say precisely which routes are gated and which are not, and still say do not expose this server. QUICKSTART gains the first-run walkthrough; the CLI README documents the token store and the $PROVX_TOKEN precedence. Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
…redential
FastAPI's default RequestValidationError handler returns Pydantic's error list verbatim,
and a Pydantic error carries the value that failed in `input`. For a `missing` error that
value is the whole request object, so a malformed request to any endpoint accepting a
secret handed the secret straight back:
POST /auth/login {"password": "<SECRET>"} -> 422 input.password
POST /engagements/{id}/credential {"value": "<SECRET>"} -> 422 input.value
api_spec_inline echoed the same way. Three shapes leaked: a sibling field missing, a
non-object body, a list body.
This was live and predates the auth work. No RequestValidationError handler existed on
main either, so the credential case has been reflecting submitted scanning credentials for
as long as the endpoint has existed (PX-SECRETS). What comes back is the caller's own
input, not stored state - validation runs before the handler, so nothing is read from the
database. It still matters: a secret in a response body ends up in proxy logs, error
trackers, and CI output (CWE-209). The auth branch added three more
password-bearing fields to an already-leaking surface; its planted-secret discipline is
what found it.
One handler on the app, not per-router, because every endpoint taking a password,
credential, or token had the same exposure. type/loc/msg survive - they are what makes a
422 actionable and none carries caller input. input and ctx are dropped because both can;
url is a docs link. FastAPI's {"detail": [...]} shape is kept rather than moved to
ErrorResponse: the CLI branches on the envelope first, so changing shape would discard the
per-field message it renders, and a fix that degrades the error UX invites being reverted.
Tests send bodies CONTAINING a planted secret across all five secret-bearing request
models and assert absence through the detector the auth tests already use, with a control
proving that detector fires against the pre-fix shape. The malformed shapes also ride
along in the no-leak walkthrough, so its existing absence assertions cover them.
Also closes the one gap a pre-merge audit found: the password needle had no demonstrated
mutation. A permanent control now mounts a deliberately leaky handler behind the
production 500 handler and asserts the detector refuses it, and two by-hand mutations
against the real login handler (log the password via extra=, echo it in a response header)
each failed the assertion before being reverted.
Residual: msg is retained for usability. No validator interpolates its input into a
message today; the guard against a future one is the planted-secret scan, not the text.
Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
KI-010: what leaked, for how long, why the existing credential no-leak test did not catch it (it drives the happy path, and nobody had asked what a malformed request returns), and what closed it. Marked RESOLVED with the residual stated. STATUS leads with the finding rather than with the feature, because a live password-and-credential disclosure found during a pre-merge audit is the more important fact about this branch. The mutation tally goes from three to five, and now says which were run against production code. CHANGELOG gets a Fixed entry ahead of the Added one. Also files the bootstrap-guard precision gap as issue #39 (marker row vs 'admin exists' - a re-bootstrap path that is unreachable today only because /users requires an admin) with a one-line pointer from the auth deliberate-limits section. Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
The first write-up of KI-010 said the credential endpoint 'returned the stored scanning credential'. That is wrong, and it overstates the flaw. Validation runs before the route handler, so nothing is read from the database: what comes back is the caller's own submitted value, reflected. There is no path here for one party to read another's stored credentials. It is still a defect worth fixing and worth an advisory - CWE-209, a secret placed into a response body, which then comes to rest in proxy access logs, error trackers, CI job output, and browser devtools. But it is Moderate, not critical, and a security tool that overstates its own vulnerabilities is not being more careful, it is being less accurate. Corrected at all five sites plus the fix commit's message: KI-010, STATUS, CHANGELOG, the handle_validation_error docstring, and the validation-test docstrings. KI-010 gains an explicit 'what this is, and what it is not' paragraph so the next reader does not have to re-derive the distinction. SECURITY.md's supported-versions table still claimed only main was supported 'until the first tagged release' - two tags exist. It now names v0.1.1, which is the version the advisory has to point at. Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
snad1
added a commit
that referenced
this pull request
Jul 31, 2026
…password echo) + auth foundation (#40) Closes #39. SECURITY FIX (pre-existing, affects v0.1.1): no RequestValidationError handler existed, so 422 responses reflected the submitted request body. POST /engagements/{id}/credential has been echoing submitted scanning credentials since it shipped (da25679, v0.1.1); the auth branch's planted-secret discipline surfaced it. The reflected value is the caller's own input, not stored state (CWE-209). Fixed globally by one handler that strips input/ctx/url, keeping type/loc/msg and the CLI's per-field rendering. AUTH FOUNDATION: argon2id passwords (one-way, not the reversible evidence-crypto layer), tokens stored SHA-256 only, roles admin/operator/viewer with the policy as a capability table, append-only auth_event, one-shot admin bootstrap, provx login/logout/admin create. Enforced on /auth/me, /auth/logout, /users only; every engagement route deliberately unchanged (next PR). Password-never-recoverable is mutation-proven. Zero stubs on the backend auth path. See KI-010 and GHSA-58x3-ph7q-vfvf. Signed-off-by: Solomon Nii Amu Darku <snad1@users.noreply.github.com>
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
1 participant
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.
SECURITY FIX (pre-existing, affects v0.1.1). No
RequestValidationErrorhandler existed, so validation errors (422) reflected the submitted request body.POST /engagements/{id}/credentialhas been echoing submitted scanning credentials in 422 responses since that endpoint shipped (da25679, tagged in v0.1.1). This branch adds password fields to that same surface; the planted-secret discipline is what surfaced it.Impact (CWE-209): the reflected secret is the caller's own input, not stored server-side state — no path for one party to read another's credentials. The harm is where it comes to rest: reverse-proxy and gateway access logs, error trackers, CI job output, browser devtools. Moderate. See KI-010 for the "what this is, and what it is not" distinction.
main.py, 24 lines) stripsinput,ctx, andurlfrom every validation error across every endpoint. 10 probe cases + credential + inline-spec confirmed clean.msgretained for usability; a future input-interpolating validator would reintroduce an echo — guarded by the planted-secret scan (KI-010), not by trusting message text.AUTH FOUNDATION (the branch's original purpose) — argon2id (one-way; not the reversible evidence-crypto layer, boundary documented at the site); tokens stored SHA-256 so a stolen row can't be replayed; roles admin/operator/viewer with
require_role/require_capabilityand a 403 attributed by an isolating control test; append-onlyauth_event; one-shot admin bootstrap (#39 tracks a marker-vs-admin invariant hardening);provx login/logout/admin createwith an on-disk token store and no password flag anywhere in the command tree.Enforced on
/auth/me,/auth/logout,/usersonly — engagement/scan endpoints UNCHANGED (enforcement is the next PR; STATUS enumerates every still-open route).test_api_engagements.pyis untouched and passes unmodified — proof the scan surface didn't move. Four pre-existing test files were extended additively (conftest.py,test_integration_cli.py,test_migrations.py,test_cli_secrets.py).Password-never-recoverable is MUTATION-PROVEN: two planted leaks in the real login handler each failed the assertion, an anti-vacuity guard confirms the probe leaked first, and a permanent CI control keeps it proven. Zero stubs on the backend auth path.
Gates: ruff, mypy --strict (62), pytest 631 (+11), DCO. Accuracy runs in CI (no scanning path touched). Advisory for v0.1.1 filed alongside.
What & why
Closes #39 (bootstrap invariant hardening, filed from this work). The validation-echo disclosure has no tracking issue: it is pre-existing, found here, and fixed here. The auth foundation is the branch's original purpose; the disclosure fix rides along because "a password is never recoverable" is the branch's headline promise and a 422 that hands it back is a direct failure of it.
Definition of Done
passive(read-only, safe in test/production). No adapter or detection logic changed.make accuracyruns in CI unchanged).docs/STATUS.mdgains an Authentication section and a validation-echo row.### Fixedand### Addedentries under## [Unreleased].auth_event.Safety classification
passive(read-only, safe in test/production)intrusive(Active mode only)How I tested
No lab target is relevant — no scanning path is touched. The auth and validation suites drive the real app on a real SQLite database with zero stubs on the backend auth path. The validation-echo fix is proven by sending bodies containing a planted secret to every secret-bearing endpoint and asserting absence. The password-never-recoverable claim is mutation-proven (two planted leaks in the real login handler each failed the assertion; a permanent CI control keeps it proven).
make accuracyruns in CI; locally, Docker is unavailable so it was not run here, and no scanning-path file changed.Closes #39.