Phase 10 — critical & high security fixes (SEC-01, SEC-02, SEC-03, SEC-04, SEC-05, SEC-07, SEC-08, F17) - #8
Merged
Merged
Conversation
The catch-all is the only pre-auth request handler in the app, so an escaping path there is an unauthenticated arbitrary-file read (F1, head of Chain A — it is how an attacker obtains data/ipmideck.db and data/encryption.key). Add a module-level pure _resolve_spa_file(full_path, root) that canonicalises with resolve() and requires is_relative_to(root) + is_file(), so containment does not depend on how the escape is spelled. Empty/over-long paths and an embedded NUL return None (Path.resolve() raises ValueError on a NUL byte, and an over-long/UNC-shaped path can stall resolution on a network lookup); a None result means "serve index.html", so no hostile path becomes a 500. The unmatched-api/ 404 branch is retained and kept FIRST — it is the FIX-04 disabled-module contract and the obvious form of this patch drops it. Verified against a live uvicorn over a raw socket: the three spellings that leaked pyproject.toml at 4322 bytes now all return the 647-byte index.html; /api/nonexistent-xyz and /api/modules/disabled/foo still 404; /%00x is 200, not 500. The SPA still boots in chromium with a mounted root and no page errors. The 58-route contract snapshot is unchanged.
…t never crosses the API
GET /api/system/app-config/{key} served any app_config row by name to any
caller holding a session — real, stolen or forged — session_secret included
(F11, the authenticated variant of the F1 disclosure). The PUT path has had
_ALLOWED_APP_CONFIG_KEYS since 04-W1-01; only the read path lacked it.
Reuse the SAME set rather than introducing a second read-specific list, and
refuse with the shape the PUT path already returns ({success: false,
error: key_not_allowed}) so the frontend error handling and the route-surface
snapshot are both unaffected. The bool-coercion and missing-row value=None
behaviour are untouched — five frontend call sites depend on them.
Verified over HTTP against a live server with a REAL authenticated session:
session_secret / app_secret / auth_enabled / encryption_key are all refused
with key_not_allowed and no value field; all seven allow-listed keys, which
are exactly the ones the SPA reads, still resolve.
…n be evicted Sessions are stateless HMACs, so whoever reads app_config.session_secret out of a copied database mints valid cookies for the real username forever (F2). Closing the read path in SEC-01/SEC-07 stops NEW disclosures but cannot un-disclose a secret already taken — this is the eviction move. AuthManager.rotate_session_secret() generates a fresh secrets.token_hex(32), persists it and assigns self._secret so a running process picks it up immediately. It deliberately does NOT touch self._file_key: the at-rest credential key has a different lifecycle (see the four-case migration in initialize()) and re-keying stored BMC credentials is not this operation. Exposed as an 'ipmideck rotate-session-secret' subcommand that short-circuits in cli() exactly where reset-password does. CLI-only by decision: rotation is incident response, typically run with the app stopped, and a UI control or banner would also be visible to whoever holds a forged session. Zero frontend surface is added. Verified against real servers: a cookie held across the documented stop-rotate-restart sequence is refused (401 on a protected route, authenticated:false on /api/auth/me), and the operator logs straight back in with the same password.
… matched zero rows update_password() ignored the cursor rowcount, so a mistyped username matched no rows and the CLI still printed 'Password updated'. An operator recovering from an incident was told a password had changed when it had not — at the worst possible moment. update_password() now returns whether a row actually changed (rowcount on the UPDATE), and _reset_password() reports that outcome instead of printing success unconditionally. The signal is returned rather than raised so the callers stay simple.
…closed BREAKING: every operator is logged out once on upgrade. Session tokens issued before this version carry no credential-fingerprint claim and are REJECTED. A password change used to revoke nothing (F7): sessions are stateless HMACs and only a *username* change evicted anyone, via the require_auth current-user check. The advertised eviction move did not evict. Session tokens now carry a 'cfp' claim — a truncated sha256 of the stored bcrypt hash. The hash changes on every password change (per-hash salt), so the fingerprint changes with the credentials and no session store is needed. verify_session_token_async() recomputes it from the CURRENT stored hash and compares constant-time. Absence of the claim is REJECTED (D1, fail-closed, maintainer-approved). A forged token minted from a stolen signing secret omits the claim in exactly the same way a pre-upgrade token does, so treating 'absent' as acceptable would leave the forgery path open and make the whole fix cosmetic. Alternatives weighed and rejected: a grace period until natural expiry (keeps alive precisely the tokens this change exists to revoke) and a forced short expiry (more complexity, same window). create_session_token_async() is an async SIBLING; the synchronous create_session_token() is left in place and unchanged because existing tests call it directly and changing its signature is the refactor the minimal-diff directive forbids. The rule is applied at ALL FOUR verification sites, so a rejected cookie is rejected everywhere rather than only on protected routes: require_auth, /api/auth/me, the WebSocket handshake gate, and _require_session_if_active (/configure and /toggle). Verified over real HTTP: after /configure changes the password the retained cookie 401s on a protected route and reports authenticated:false on /api/auth/me, while the freshly issued cookie works; a hand-forged claim-less cookie signed with the secret read straight out of the database is refused at both sites.
An anonymous POST /api/auth/toggle {enabled:false} is accepted on a fresh
instance because the session guard short-circuits while no user exists, and
/api/auth/setup then created the account WITHOUT re-enabling auth. The instance
stayed open permanently, with no UI symptom. Verified live before this change:
anonymous toggle-off -> setup -> anonymous GET /api/servers returned 200 with
the full inventory. It now returns 401.
This closes clause 2 of SEC-04 ("completing first-run setup always leaves
authentication enabled") and deliberately NOT clause 1 ("an anonymous caller
cannot disable authentication on a not-yet-configured instance"), which is
deferred onto SEC-06: before any credential exists nothing over HTTP
distinguishes the legitimate first-run operator from a LAN attacker, and
separating them needs the out-of-band bootstrap secret SEC-06 provides.
The residual is measured and disclosed in README's Security section rather
than being papered over.
toggle_auth is deliberately UNCHANGED. No connection-property gate (the README
tells operators to open http://<their-ip>:3000 and the Dockerfile uses
--network host, so normal first run is not from loopback; and behind a reverse
proxy everything looks like loopback). No servers-emptiness gate (a fresh
instance holds zero servers so it never fires against the attacker, while demo
mode seeds six with no account so it fires against the operator). No flat
"refuse when no user exists" (SetupPage.tsx:101 calls this endpoint anonymously
and auth_enabled defaults to true, so refusing it is a permanent lockout — the
withdrawn attempt's error).
A named regression test pins that the first-run skip still succeeds.
_require_session_if_active checked only the token signature, so a stale-but-signed cookie could rewrite the sole account through /configure — the exact action an incident responder takes to evict an attacker (F6). On an auth-disabled instance that still had an account, no cookie was needed at all (F10). /configure now requires the current password whenever an account exists, and _require_session_if_active and /toggle apply the COMPLETE validator (signature + current user + credential fingerprint) rather than the signature alone, so a cookie refused everywhere else is refused here too. The gate keys on has_user(), NOT on auth_enabled. That keying is load-bearing: keying on auth_enabled would leave the F10 window wide open, and the frontend only shows this form when auth is OFF, so an auth_enabled condition would never fire from the UI. With no account present nothing is required — that is genuine first run, and enabling auth from Settings on a fresh instance still works. On an auth-disabled instance there is no session to name the current user, so verification falls back to the single stored account row (the users table is single-user by design). UI: one new Current password input inside the EXISTING Security enable-auth form — the single UI exception approved for this phase. It renders and is enforced only when an account exists, grafted onto the already-present secCurrentPassword state and currentPassword* i18n keys, so no new component, state hook, i18n key or flow is introduced. Committed component test covers all four cases including the account-less path, where an unconditional guard would have made enabling auth impossible. backend/static rebuilt from source (it is the PyPI wheel's package-data); index.html keeps its original line endings so the diff is the one changed asset hash rather than 12 lines of CRLF noise.
…st-run advisory Four documentation obligations this release depends on. Documentation is the only available mitigation for what code cannot undo — already-disclosed secrets and a deferred finding. CHANGELOG: the forced-logout notice opens the Unreleased section rather than sitting at the bottom. Upgrading invalidates every session and each operator logs in once; the notice says why (tokens are now bound to the credentials, and an unbound token is indistinguishable from a forged one) and points at rotate-session-secret for the incident-response case. README, rotation runbook (SEC-02): the stop / rotate / restart sequence, with the reason the restart matters — the live process keeps serving with the old secret in memory, so rotating against a running app evicts nobody until it restarts. This is the sequence the automated chain test actually replays. README, backup contents (SEC-08 / F4): the archive bundles encryption.key + the database + the config, so it is credential-grade even though a stolen database ALONE decrypts nothing. Adds the recovery the threat model promised: patching is not evicting — rotate the session secret AND the encryption key, re-enter the BMC credentials, change the BMC passwords, delete the old archives. README, first-run advisory (SEC-04 clause 1 + SEC-06): measured, not assumed. Five probes were run against a real non-demo instance and recorded; the advisory states what is delivered (setup re-enables auth; anonymous protected request -> 401), what is not (an anonymous caller can still disable auth and claim a not-yet-configured instance), the exposure window in time (opens at first start, closes when setup completes), and the operator mitigation (first setup on a trusted network, completed promptly). No code changes.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Review of this branch surfaced one real defect and several accuracy problems. No security behaviour changed. - tests: the live attack-chain harness replaced the process environment with a hardcoded POSIX one, so the spawned uvicorn died before any app code ran on Windows (WinError 10106 out of WSAStartup without SystemRoot, then a pyfiglet KeyError without APPDATA). The OS-level variables the interpreter needs are passed through now, matched upper-cased because iterating os.environ yields upper-case keys there. 6 failures and 42 errors become 48 passes; the full suite is 451 passed on Windows, so the pre-commit gate is runnable again. - docs: remove the README security sections. The changelog no longer points at them, and the backup note no longer claims a documentation that does not exist. - docs: drop the emoji from the changelog heading. - auth: rotate_session_secret's docstring claimed a running process picks the new secret up immediately. The CLI action runs in a separate process, so the supported sequence is stop -> rotate -> start. - src/tests: remove internal planning references from public sources (phase and plan numbers, roadmap criteria, decision ids), and rename test_phase10_auth_hardening.py to test_auth_hardening.py. - tests: explain the empty except in the readiness poll.
dev-luigi
marked this pull request as ready for review
September 6, 2026 19:52
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.
Closes the 3 CRITICAL / HIGH findings that form the exploit chain in
SECURITY-AUDIT.md, plus three supporting fixes. Every chain was replayed against a real running server over raw HTTP, not just unit-tested, and each attack test was verified to go red on the vulnerable code before being kept.What each finding closes
app-configread served the session secretreset-passwordlied about unknown usernamesSEC-01 — pre-auth path traversal (head of the chain)
The catch-all is the only pre-auth request handler in the app, so an escaping path there was an unauthenticated arbitrary-file read — this is how an attacker obtained
data/ipmideck.dbanddata/encryption.keyin the first place.New module-level
_resolve_spa_file()canonicalises withresolve()and requiresis_relative_to(root)+is_file(), so containment does not depend on how the escape is spelled. Empty/over-long paths and embedded NULs fall back toindex.htmlrather than 500ing (Path.resolve()raisesValueErroron a NUL byte).Measured against a live server, raw sockets (
index.html= 647 B):/../../pyproject.toml/%2e%2e%2f%2e%2e%2fpyproject.toml/..%2f..%2fpyproject.toml/../../data/encryption.key/api/nonexistent-xyz/%00xThe unmatched-
api/404 branch is retained and kept first — it is the FIX-04 disabled-module contract and the obvious form of this patch drops it.SEC-02 — the secret is now rotatable
New CLI action
ipmideck rotate-session-secret. Previously a secret copied out of a stolen database kept minting valid cookies forever with no supported way to evict them.CLI-only by decision: rotation is incident response, typically run with the app stopped, and a UI control would also be visible to whoever holds a forged session. Zero frontend surface added.
SEC-03 — password change now evicts (⚠️ BREAKING)
Session tokens carry a
cfpclaim — a truncated digest of the stored bcrypt hash, which changes on every password change, so eviction works without adding a session store.A token with no claim is rejected (fail-closed). A forged token omits the claim in exactly the same way a pre-upgrade token does, so accepting "absent" would have left the forgery path open and made the fix cosmetic. Consequence: every operator logs in once after upgrading. Documented prominently at the top of the CHANGELOG's Unreleased section.
Applied at all six verification sites, so a cookie refused anywhere is refused everywhere:
require_auth,/api/auth/me, the WebSocket handshake,_require_session_if_active(/configure+/toggle), and the/togglecurrent-password check.SEC-04 — clause 2 delivered, clause 1 deliberately not
Delivered:
setupnow callsset_auth_enabled(True)unconditionally, so an instance toggled open beforehand no longer stays open forever with no UI symptom. Chain B end-to-end went from 200 with the full inventory to 401.Not delivered, and not claimed to be: an anonymous caller can still disable auth on a not-yet-configured instance. Before any credential exists nothing over HTTP distinguishes the legitimate first-run operator from a LAN attacker — separating them needs the out-of-band bootstrap secret that is SEC-06, which is deferred.
toggle_authis deliberately unchanged. Three substitutes were considered and refuted with evidence rather than reasoning:http://<their-ip>:3000and the Dockerfile uses--network host, so normal first run is not from loopback; and behind any reverse proxy every request looks like loopback (F13, still open);SetupPage.tsx:101calls this endpoint anonymously andauth_enableddefaults totrue, so refusing it is a permanent lockout). A named regression test now pins that the skip still works.ROADMAP criterion 2 is half met, and is reported as half met.
SEC-05 — the account-takeover gate
/configure— the exact endpoint an incident responder uses to evict an attacker — accepted any signature-valid cookie and rewrote the sole account. On an auth-disabled instance no cookie was needed at all (F10).The current password is now required whenever an account exists, keyed on
has_user()and not onauth_enabled. That keying is load-bearing: keying onauth_enabledleaves the F10 window open, and the frontend only shows this form when auth is OFF so the condition would never fire from the UI.UI: exactly one new
Current passwordinput inside the existing Security form — the single UI exception approved for this phase. It renders only when an account exists (an unconditional guard would make it impossible to enable auth on a fresh instance) and grafts onto already-present state and i18n keys. Enforced by an automated gate: the only non-test file changed underfrontend/srcisSecuritySection.tsx.SEC-07, SEC-08, F17
session_secretis refused to everyone, authenticated or not.encryption.key+ database + config, and adds the recovery the threat model promised — patching is not evicting: rotate the session secret AND the encryption key, re-enter BMC credentials, change BMC passwords, delete old archives.reset-passwordno longer prints success for a username that matched zero rows.What this PR does NOT close
Measured residue on a fresh instance: anonymous toggle →
auth_enabled:false, anonymousGET /api/servers→200 {"servers":[]}. The exposure is ownership of an unconfigured appliance, not a path to existing BMC credentials — none exist yet in that state. After setup:auth_enabled:true, anonymousGET /api/servers→ 401.How it was verified
Attack chains replayed against a real
uvicornsubprocess over raw sockets (so percent-encoded escapes arrive un-normalised), committed astests/integration/test_live_attack_chains.pyand running in CI under plainpytest:data/ipmideck.dbanddata/encryption.keyrotate-session-secret→ restart: the retained cookie 401s,/api/auth/mereportsauthenticated:false, and the operator logs back in with the same passwordMutation-tested — every attack test was confirmed to fail on vulnerable code:
Full 8-step CI gate, green locally before pushing:
ruff check backend testspytest --covcheck-i18n-parity.mjsnpm --prefix frontend run testtsc -b --noEmitcheck-spa-built.mjsvite buildTest count 335 → 451 (+116). Coverage 88.14% → 89.02%;
backend/core/auth.pyalone 80% → 84% despite gaining ~90 statements. The 58-route contract snapshot is unchanged — no route was added. The SPA boots clean in headless chromium against the rebuilt bundle (mounted root, no page errors).backend/staticwas rebuilt from source becausefrontend/srcchanged (it is the PyPI wheel's package-data).index.htmlkeeps its original line endings, so that diff is the single changed asset hash rather than 12 lines of CRLF noise.Commits
Atomic, one per finding:
b40b7c2cb42d072b5dc7brotate-session-secret1f7e55dreset-passwordb4e416c75cd45167b2505f95997eDraft pending CI.