fix(wizard): admin/owner-rolcontrole en bescherming privileged velden bij wizard-bewerken#69
Conversation
|
Twee commits toegevoegd vanuit een diepe review (commits Zelf-correctie eerstEen eerdere review-pass van mij flagde " Wat de twee commits doen1. Inline-comment + future-doc (
|
…paden PR #69 had de role-recheck + protected-key-merge inline in _save_existing_project staan, en de inline allowlist dupliceerde '_require_project_edit_access' uit router_detail_edit. Twee follow-up save-paden waren ongedekt: _modal_do_submit (blind merge, geen TOCTOU) en submit_step in edit-modus (geen role-gate). Nieuw module opi/web/project_edit_security.py: - require_project_edit_access (verhuisd vanuit router_detail_edit, public) - PROTECTED_PROJECT_KEYS constante met per-key reden - merge_preserving_protected_keys helper Toegepast op alle drie de save-paden: - _save_existing_project (wizard-final) -- inline allowlist vervangen - _modal_do_submit (modal-final) -- TOCTOU recheck + protected-merge - submit_step (wizard-step) -- edit-mode role-gate Tests: 8 -> 16 in test_wizard_role_gate.py. Nieuwe dekking voor merge-helper edge-cases (alle keys, drop-when-absent, no-mutate), modal TOCTOU recheck + backup-flow skip, step-submit gate in edit/ create-modus. cluster-editing.md uitgebreid met een 'Form-RBAC: architecturele lacune'-sectie: zolang Editable geen requires_role-attribuut heeft moet elke save-handler beide guards expliciet aanroepen. Dat is een eigen refactor, niet PR #69.
Update — centrale guards + alle save-paden afgedekt (commit f30a321)Tijdens review kwamen drie observaties op tafel:
In één PR oplossen lost het structureel op en voorkomt dat twee follow-ups blijven liggen. Nieuw module
|
| Save-pad | Gate | Merge |
|---|---|---|
_save_existing_project (wizard-final) |
require_project_edit_access |
merge_preserving_protected_keys |
_modal_do_submit (modal-final) |
require_project_edit_access (TOCTOU recheck, skip op backup/restore flows) |
merge_preserving_protected_keys |
submit_step (wizard-step, alleen edit-modus) |
require_project_edit_access |
n.v.t. (alleen sessie-state, geen disk-write) |
Tests: 8 → 16 in test_wizard_role_gate.py
Nieuw:
test_merge_preserves_all_protected_keys— alle 4 keys re-derivedtest_merge_drops_protected_keys_absent_from_existing— geen introductie via submittest_merge_returns_new_dict— geen input-mutatietest_protected_keys_constant_covers_the_documented_set— lock op de set zelftest_modal_do_submit_invokes_role_gate_for_project_edit— TOCTOU recheck wiringtest_modal_do_submit_skips_edit_gate_on_backup_restore_flow— backup-flow gebruikt member-gate, niet edit-gatetest_submit_step_gates_in_edit_mode— step-submit gate in edit-modustest_submit_step_skips_gate_in_create_mode— geen project = geen gate
Documentatie: features/futures/cluster-editing.md uitgebreid
Nieuwe sectie "Form-RBAC: architecturele lacune" legt vast dat Editable (opi/forms/editables/editable.py) géén requires_role-attribuut heeft. Zolang dat zo blijft moet élke save-handler die submitted form data merget zelf beide guards expliciet aanroepen — vergeet er één en de bescherming is daar afwezig. Wenselijke eindstaat (field-level RBAC + form_submission_guard + UI-feedback) is geschetst als eigen toekomstig refactor-traject.
Pre-push pytest equivalent
3403 passed, 31 deselected, 68 warnings in ~30s
Specifiek test_wizard_role_gate.py: 16/16 passed.
Diff
5 files, +382 / -97. Niet meer 7 inline allowlist-regels in router_wizard én een second copy in router_detail_edit; één module met één definitie.
…privileged velden De volledige-pagina wizard-bewerkflow miste de admin/owner-rolcontrole die de detail-bewerkflow wel afdwingt. wizard_edit_page controleerde alleen is_user_authorized_for_project (waar voor elke rol, inclusief member) en _save_existing_project deed een blinde existing_data.update(data). Omdat users/role en het config-blok (api-key, AGE-sleutels) bewerkbare wizard-velden zijn, kon een member van een project zichzelf via de wizard tot owner promoveren of owners verwijderen: project-overname plus mogelijke secret-exfiltratie. - wizard_edit_page (GET) hergebruikt nu _require_project_edit_access en eist daarmee rol admin of owner. - _save_existing_project doet de rolcontrole opnieuw op het muterende verzoek (TOCTOU: GET-sessie kan door een andere gebruiker zijn herspeeld of de rol kan zijn ingetrokken). - De blinde merge is vervangen door een merge die users, config, name en clusters altijd opnieuw afleidt uit het opgeslagen project en nooit uit de ingediende formulierdata. - Regressietest test_wizard_role_gate.py: member geweigerd bij wizard-bewerken en bij opslaan, owner/admin toegestaan, en een member-payload die users/config probeert te herschrijven muteert die sleutels niet.
…aanvallen af De test test_owner_save_cannot_overwrite_protected_keys liep nooit tot de asserties: clear_wizard_state crashte op de fake request omdat die geen sessie heeft, waardoor de kern van de fix (de protected-key merge) in het geheel niet werd geverifieerd. clear_wizard_state wordt nu gemockt zodat het opslagpad volledig wordt doorlopen. Twee aanvalsscenario's toegevoegd: - weglaten van een protected key uit de payload mag die key niet laten verdwijnen (deletie-aanval op users/owners); - het herschrijven van alleen een geneste config-secret (api-key, AGE private key) mag niet blijven hangen.
…r-edit design
The protected_keys list in _save_existing_project lumps four keys together
with one short reason ('escalation/exfiltration'). That covers users/config/
name but not clusters; for clusters the reason is different: it is not even
exposed in the edit-mode wizard (IDENTITY_EDIT_SECTION only renders
display-name and description, the create-mode IDENTITY_SECTION is the one
with the cluster picker), and cluster editing as a feature is not yet
designed - no add/remove cascade logic, no distributed-model coordination
for when two cluster-OPIs see the same project file change.
Two changes, no functional impact:
- Inline comment near protected_keys split per key, with the clusters
rationale and a pointer to the future doc.
- features/futures/cluster-editing.md: explicit open design with the five
things that need to happen before clusters can become editable (add
cascade, remove cascade, deployment-validation, distributed race, UX).
Also lists the two related follow-ups from the same review (modal-edit
blind merge in router_detail_edit, step-POST gating in submit_step).
Self-correction: an earlier review pass flagged 'clusters silently dropped
in edit-flow' as a UX regression. It is not - the field is not rendered in
edit mode, so nothing is dropped.
The existing tests prove _require_project_edit_access rejects a member, and that _save_existing_project rejects a member on the POST. They do not prove that wizard_edit_page (the GET-time entry into the edit flow) calls the gate at all - a future refactor could drop the gate call from the route body without any test failure. This test mocks _require_project_edit_access to raise an HTTPException and verifies that calling wizard_edit_page propagates it. The light pre- gate setup (get_flow / get_current_user / get_templates) is mocked too; the actual assertion is on the gate call.
…paden PR #69 had de role-recheck + protected-key-merge inline in _save_existing_project staan, en de inline allowlist dupliceerde '_require_project_edit_access' uit router_detail_edit. Twee follow-up save-paden waren ongedekt: _modal_do_submit (blind merge, geen TOCTOU) en submit_step in edit-modus (geen role-gate). Nieuw module opi/web/project_edit_security.py: - require_project_edit_access (verhuisd vanuit router_detail_edit, public) - PROTECTED_PROJECT_KEYS constante met per-key reden - merge_preserving_protected_keys helper Toegepast op alle drie de save-paden: - _save_existing_project (wizard-final) -- inline allowlist vervangen - _modal_do_submit (modal-final) -- TOCTOU recheck + protected-merge - submit_step (wizard-step) -- edit-mode role-gate Tests: 8 -> 16 in test_wizard_role_gate.py. Nieuwe dekking voor merge-helper edge-cases (alle keys, drop-when-absent, no-mutate), modal TOCTOU recheck + backup-flow skip, step-submit gate in edit/ create-modus. cluster-editing.md uitgebreid met een 'Form-RBAC: architecturele lacune'-sectie: zolang Editable geen requires_role-attribuut heeft moet elke save-handler beide guards expliciet aanroepen. Dat is een eigen refactor, niet PR #69.
uittenbroekrobbert
force-pushed
the
fix/wizard-role-gate
branch
from
b105a6d to
c24409e
Compare
Following review: silently stripping submitted protected keys is the wrong default. users/config are legitimately editable by admin/owner via MODAL_EDIT_TEAM_FLOW and the *_CONFIG_SECTION flows -- the role gate (require_project_edit_access) is the right boundary for those. Allowlist was over-restrictive and broke legitimate edits. Real reason a top-level field belongs in this list is structural integrity: no form should ever submit it because the system has invariants that depend on it. Two fields qualify: - name = on-disk filename - clusters = no migration logic post-creation (see cluster-editing.md) Renames: - PROTECTED_PROJECT_KEYS -> IMMUTABLE_PROJECT_FIELDS - merge_preserving_protected_keys -> apply_form_data_to_project (raises HTTPException 400 if an immutable field is submitted) Tests rewritten: - owner can now update users/config legitimately - owner submitting name/clusters -> 400 - helper unit tests assert raise + new-dict + immutable set cluster-editing.md + form-field-rbac.md updated for new naming. Drop the inline comment block at the call sites -- the helper name explains it.
2 participants
Privilege-escalation (HIGH)
De volledige-pagina wizard-bewerkflow miste de admin/owner-rolcontrole die de detail-bewerkflow (
opi/web/router_detail_edit.py_require_project_edit_access, rol inadmin/owner) wel afdwingt.wizard_edit_page(GET,opi/web/router_wizard.py) controleerde alleenis_user_authorized_for_project(), dieTrueis voor elke rol inclusiefmember._save_existing_projectdeed een blindeexisting_data.update(data)mass-merge.users/roleen hetconfig-blok (config/api-key,config/age-public-key,config/age-private-key) zijn eersteklas bewerkbare wizard-velden (opi/forms/editables/fields/team.py,config_*.py).Netto: een
membervan een project kon via de wizard deusers-lijst herschrijven om zichzelf owner te maken of owners te verwijderen (project-overname), en hetconfig-blok overschrijven (manipulatie/exfiltratie van api-key en AGE-sleutels).Pre-existing en onafhankelijk van andere openstaande security-bevindingen.
Fix
wizard_edit_pagehergebruikt nu_require_project_edit_accessen eist roladminofownerop de GET-entry diestate.project_namezet._save_existing_projectvoert dezelfde rolcontrole opnieuw uit op het muterende verzoek, gekeyd op de projectnaam (TOCTOU: GET-sessie kan door een andere gebruiker zijn herspeeld of de rol kan zijn ingetrokken).users,config,nameenclustersaltijd opnieuw afleidt uit het opgeslagen project en nooit uit de ingediende formulierdata. Overige velden worden normaal bijgewerkt.Testplan
operations-manager/python/tests/test_wizard_role_gate.py(red-green):membergeweigerd bij wizard-bewerken (GET-gate) -> 403owneren globaleadmintoegestaanmembergeweigerd op het muterende opslaan -> 403, opgeslagen project ongewijzigdowner-payload dieusers/config/name/clustersprobeert te herschrijven muteert die sleutels niet; niet-beschermde velden (display-name,components) worden wel toegepastLet op: de volledige pytest-suite kan in deze omgeving niet draaien door een pre-existing collection-fout (Python 3.14.0b4 + pydantic 2.12.5 laat
import fastapifalen viaconftest.py). Dit is omgevingsbreed en niet door deze wijziging veroorzaakt; de merge- en rol-logica is geverifieerd in isolatie buiten de FastAPI-importketen.ruffenpyrightzijn schoon op de gewijzigde bestanden.