feat(vault-key-proof): require a master-password proof for destructive vault operations (#673)

[rjzondervan]

What this changes

Closes the session-only vault-lockout reported in #395 (and the two further vectors found while tracing it). Under ADR-003 the server holds only ciphertext, so reading the vault needs the master password — but destroying key material needed only a Nextcloud session. This makes the destructive operations require a vault-key proof: a signature, made with the owner's suite private key, over a server-issued challenge bound to the operation's parameters. Because that private key is recoverable only by decrypting its envelope with the master password, a verified proof is a server-verifiable proof of the master password. A stolen cookie, leaked app password, or XSS in an unlocked tab no longer suffices.

Backend

• #[VaultKeyProofRequired(binds, subject, purpose)] + VaultKeyProofMiddleware + VaultKeyProofService (stateless HMAC challenge; RSASSA-PKCS1-v1_5 SHA-256 signature; no ICacheFactory).
• Challenge endpoint GET /api/v1/suites/{id}/proof-challenge (ungated).
• Guard applied to compromiseRecovery, updatePrivateKey, complete, and the emergency-contact destroy. VaultKeyProofAttributesTest fails the build if a destructive route drops the attribute.
• New abort route (POST /api/v1/migrations/{id}/abort) — the remedy compromiseRecovery already promised. Non-destructive; refuses once any record has moved.

Frontend

• proveMasterPassword + a shared challenge/header helper; all four flows send the proof. Emergency delete gained a master-password confirm dialog; the recovery form re-prompts on a key_proof_required refusal; the resume banner gained an abort control.

Design points worth reviewing (documented in docs/ARCHITECTURE.md §4.2): sign-not-decrypt (the session key is decrypt-only and can't sign); the attribute carries the binding because the middleware can't read the body; complete proves the old key; abort is deliberately unguarded.

Testing

• Full PHP unit suite green (1244); dedicated tests for the service crypto, middleware dispatch, attribute coverage, a JS→PHP cross-impl round-trip, and abort.
• Frontend green: proveMasterPassword round-trip, the session-key decrypt-only pin, the emergency confirm dialog, store and component tests.
• phpmd clean; prettier + eslint 0 errors. Frontend rebuilt locally and verified to contain the proof wiring.

Not yet done (tracked in the OpenSpec change)

A live without-proof 403 / finding-2 end-to-end regression needs the running request pipeline (the guard is middleware-enforced, so isolated PHPUnit can't produce a real 403 — same rationale as RateLimitAttributesTest), and the independent human reproduction (pre-fix → refused post-fix). These are the remaining §6.5/6.6/7.6 items.

Tracking

Closes #673 (OpenSpec change harden-vault-key-material-guards) and closes #395 (the reported vulnerability). The §3 abort route is independently mergeable if you'd prefer to split it out.

Security disclosure

This fixes an exploitable vulnerability. #395 was filed publicly by the contributor deliberately (the app is pre-production); this PR and its linked issue therefore discuss it in the open. If that is not intended, per the project security policy the report path is HackerOne rather than a public PR — the contributor's call.

AI assistance disclosure

Assisted by Claude Code (claude-opus-5): drafted the guard middleware/service/attribute, the abort route, the client wiring, the OpenSpec change and tests, and reproduced #395 on a local instance. All changes to be reviewed and independently verified by the human contributor before merge.

[github-actions]

Quality Report — ConductionNL/keepiq @ 76695c2

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		❌
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 111/111
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	✅
PHPUnit					❌
Newman					✅
Playwright					✅
Hydra gates					❌

---

Quality workflow — 2026-09-11 08:36 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/keepiq @ 2af8a0c

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		❌
check-schema-l10n		✅
composer			✅	✅ 111/111
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	✅
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					❌

---

Quality workflow — 2026-09-11 09:03 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/keepiq @ 4792535

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 111/111
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	✅
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					✅

---

Quality workflow — 2026-09-11 11:56 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/keepiq @ 7f54672

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 111/111
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	✅
PHPUnit					✅
Newman					✅
Playwright					✅
Hydra gates					✅

---

Quality workflow — 2026-09-11 12:20 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

Inline findings from a Strict-mode review. The protocol construction is sound — hash_equals on the MAC, openssl_verify !== 1, enforced 300s expiry, server-resolved verifying key, and a test that genuinely pins the session key as non-extractable/decrypt-only. The two blockers below are about coverage and binding completeness, not construction.

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Strict mode)

The protocol construction is genuinely good. I checked the things that usually go wrong in a scheme like this, and they're all right:

• hash_equals() on the MAC — no timing leak, no ==/=== anywhere on it
• openssl_verify checked as !== 1, so the -1 error case refuses instead of passing (the classic truthiness bug — avoided)
• expiry genuinely enforced on verify (300s), with an injected ITimeFactory and a real test
• the verifying public key comes from the server-resolved subject suite, never from request input
• purpose is inside the MAC'd payload and the nonce is line 1 of the signed message, so cross-purpose replay is closed
• the middleware is registered (PlatformIntegrationRegistrar.php:70) — not inert — and fails closed on every path I traced: missing/empty header, malformed base64, missing certificate, suite-resolution Throwable re-wrapped rather than swallowed
• sign-not-decrypt is load-bearing and actually pinned: proveMasterPassword.spec.js:134-135 asserts extractable === false and usages === ['decrypt'] — a real assertion, not a comment
• no master password or raw PEM in localStorage/sessionStorage anywhere in src/crypto/
• the cross-impl test uses a fixed fixture and a tampered-bound-value negative case

On item 17 of my own checklist — the "primary control is untested" worry — that's largely unfounded: VaultKeyProofMiddlewareTest exercises the refusal paths directly. The missing live 403 is a genuine but narrow gap, and your rationale for it (same as RateLimitAttributesTest) holds.

Where the implementation doesn't match the design is coverage and binding completeness, not construction.

Two blockers

1. complete binds only id — acceptUnrecoverable and hasErrors are unbound and re-aimable inside the 300s window, and the 409 body hands the caller the exact count needed. A replayed proof can finalise the migration with unacknowledged permanent loss. docs/ARCHITECTURE.md:630's "a replay only ever re-authorises the byte-identical operation" is not true for this route. Your own design.md:160 already argues the parameter is security-relevant.

2. revoke is left entirely outside the guard — and outside the attributes test. Session-only, hard-deletes ShareTargets, promotes delegations irreversibly, blocks every secret read, and recovery is admin-only so the victim can't undo it. That is the #395 lockout shape this PR exists to close, still reachable with nothing but a stolen cookie.

There are exactly four #[VaultKeyProofRequired] sites in lib/; revoke at :286 isn't one of them.

Guarded-route audit

Route	Attribute	binds	Params	Gap
compromise-recovery	✅	publicKey, encryptedPrivateKey	same	complete
suites/{id}/private-key	✅	encryptedPrivateKey	id, encryptedPrivateKey	id selects the verifying key, so re-aiming breaks the signature — safe
migrations/{id}/complete	✅	id	id, hasErrors, acceptUnrecoverable	🔴 two unbound
emergency-access/contacts/{id}	✅	id	id	complete
suites/{id}/revoke	❌	—	id, reason	🔴 unguarded
migrations/{id}/abort	❌ deliberate	—	id	accepted — gate verified
suites/{id}/proof-challenge	❌ deliberate	—	id, purpose	accepted

Two 🟡s

3. The attributes test's route list is hardcoded — a good regression pin, but it can't catch a new destructive route. revoke is the live proof it already failed once.
4. validateOwnership passes for any non-user-owned suite, so the new proofChallenge accepts an application suite id. Nothing is granted — the middleware's own assertOwned() is the real control — but the check the endpoint advertises in its docblock isn't the check the code performs. (PR #676 fixes this same helper; worth coordinating.)

🟢 Notes

• The challenge HMAC key defaults to '' when secret is unset (VaultKeyProofService.php:241). Not exploitable on its own — a forged challenge still can't produce a signature — but a key-material control shouldn't silently degrade. Throw on an empty secret.
• Abort's gate is correct. I verified your claim: it refuses whenever countCommitted() > 0, is idempotent on non-in_progress, deletes rather than revokes the successor, and dispatches the aborted event so the teardown cascades don't fire. The reasoning that a hostile session can never have committed a record holds. The only residual is a rotation-restart nuisance, which is the right trade against a wedged vault.

On the disclosure note — filing publicly was the contributor's call to make and the reasoning is sound for a pre-production app; no objection from me.

[github-actions]

Quality Report — ConductionNL/keepiq @ e4e8f7e

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 111/111
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	✅
PHPUnit					✅
Newman					✅
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test.
Hydra gates					✅

---

Quality workflow — 2026-09-14 08:25 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

Inline findings from a Strict-mode re-review of the three fix commits. Both prior blockers were taken seriously and complete's binding is properly closed; the revoke guard is the right shape but is inert as landed — one missing allow-list entry. Verdict follows separately.

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Strict mode) — re-review of 96470c1…cc63def

Both blockers were taken on directly, and the reasoning in the new commit messages and docblocks is good. One of them is properly closed; the other landed a correct guard that can't fire.

Blocker 1 — complete binding: ✅ closed

binds: ['id', 'hasErrors', 'acceptUnrecoverable'], matching boundValues at all three client call sites, provider row and docs table updated. I checked the part that could have failed silently: the middleware's (string)getParam($name, '') and the new boundParam() helper agree on every case that matters (true→'1', false/null/absent→''), and completeMigration posts hasErrors as a real boolean. A proof captured on a clean completion no longer verifies once acceptUnrecoverable is set. Thread resolved.

Blocker 2 — revoke: ⚠️ guarded on paper, inert in practice

The attribute is the right shape — dedicated purpose, subject: 'routeParam:id', binds: ['reason'], provider row, client wiring, a master-password field in the confirm dialog, and store tests asserting the headers ride the request. But PURPOSE_REVOKE_SUITE was never added to ALLOWED_PURPOSES, so proofChallenge answers 400 Unknown or missing proof purpose, no proof can be built, and the middleware refuses every revoke.

revoke therefore went from reachable by any session to reachable by nobody — and it's the self-service "my laptop was stolen, lock my vault" control, with revokeSuite()'s single caller being this method and reinstate being admin-only. One line fixes it.

Guarded-route audit (updated)

Route	Attribute	binds	Params	Status
compromise-recovery	✅	publicKey, encryptedPrivateKey	same	complete
suites/{id}/private-key	✅	encryptedPrivateKey	id, encryptedPrivateKey	complete (id selects the verifying key)
migrations/{id}/complete	✅	id, hasErrors, acceptUnrecoverable	same	✅ fixed this round
emergency-access/contacts/{id}	✅	id	id	complete
suites/{id}/revoke	✅	reason	id, reason	🔴 purpose not issuable → route dead
migrations/{id}/abort	❌ deliberate	—	id	accepted
suites/{id}/proof-challenge	❌ deliberate	—	id, purpose	accepted

Also this round

• 🟡 Nothing pins "a guarded purpose is issuable" — the root cause of the above, and why 30 green checks didn't catch it. VaultKeyProofAttributesTest asserts the attribute's purpose constant; EncryptionSuiteControllerTest only exercises proofChallenge with two purposes that happen to be in the list. A one-line assertContains($purpose, ALLOWED_PURPOSES) over the existing provider closes it. My earlier note about the provider being hardcoded stays open — it still derives nothing from appinfo/routes.php.
• 🟡 The master password is now the only key to a revoke — a design consequence worth recording rather than a defect, and it's where @rjzondervan's Slack point lands. Short version: admin revocation rights are a fair constraint and my finding never argued against them (the concern was #[NoAdminRequired] admitting any session); but on this branch no admin revoke exists to preserve, and reinstateSuite() restores only the status flag — the ShareTarget DELETE and the delegation promotion are not undone, so the cascade isn't reversible even though the status is. An admin-only proof-exempt forceRevoke would close both halves; good follow-up issue, not this PR.
• 🟢 validateOwnership — fixed, via the development merge bringing in the ownerType !== 'user' || ownerId !== $userId form. My earlier note about coordinating with #676 is moot; it landed.
• 🟢 Still open from last time: the challenge HMAC key defaults to '' when secret is unset (VaultKeyProofService.php:242). Not exploitable on its own, but a key-material control shouldn't silently degrade — throw on an empty secret.

Blocking set

Just the one line in ALLOWED_PURPOSES. I'd take the test cross-check with it, since it is what turns that class of mistake from green to red, but only the first is blocking.

CI is green across all 30 checks on cc63def, which is worth stating plainly: this failure mode is invisible to every gate the repo has.

[github-actions]

Quality Report — ConductionNL/keepiq @ 7c7ce04

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
check-manifest		✅
test-l10n		✅
format		✅
check-l10n-js		✅
check-schema-l10n		✅
composer			✅	✅ 111/111
npm			✅	✅ 543/543
app:check-code	⏭️
info.xml	✅
REUSE	✅
PHPUnit					✅
Newman					✅
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test.
Hydra gates					✅

---

Quality workflow — 2026-09-14 09:26 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

Verdict: APPROVE (Standard mode) — re-review of cc63def…716b8b6

Both things I blocked on last round are fixed, and I verified each at the head SHA rather than from the diff:

• The revoke guard is now live. PURPOSE_REVOKE_SUITE is in ALLOWED_PURPOSES, so proofChallenge issues for it and the attribute added in aef04a7 actually takes effect instead of refusing every revoke.
• The gap that let an inert guard ship green is closed. The attribute test now cross-checks each guarded purpose against the issuable set — the same suite that went green on a dead guard would now turn red.

One change in 716b8b6 I didn't ask for and think is a good catch: mac() throws when the instance secret is empty instead of computing an HMAC with an empty key. I traced the failure path before treating it as harmless — verify() isn't wrapped in a catch, and afterException re-throws anything that isn't a KeyProofRequiredException, so an unset secret produces a 500 rather than a forgeable challenge. That's fail-closed in the right direction, and it removes a real (if unlikely) fail-open: with an empty key, anyone could have minted a valid nonce.

CI is green across all 30 checks on 716b8b6.

Two 🟡s stay open, neither blocking:

• guardedMethodsProvider() is still a literal array — the routes that exist are pinned, but one added tomorrow without the attribute still passes silently. Follow-up issue, not this PR.
• The master password is now the only key to a revoke — a decision I'd like recorded in docs/ARCHITECTURE.md §4.2 rather than inherited. Either answer is fine; I'd just like it written down.

The two items your description lists as not yet done — the live without-proof 403 and the independent human reproduction — are still outstanding, and they're the ones that matter most for a security fix. Approving on the protocol and the guard coverage; the end-to-end confirmation is yours to sign off before merge.