chinux/theProx
0
0
Fork 0
Code Issues 40 Pull requests Projects Releases Packages Wiki Activity Actions

[P3] Review-Sammelissue: list_scripts ohne Rollenschutz, GUAC_SECRET-Platzhalter, 50× except-pass, Reconcile für ScheduledJob #231

New issue
Open
opened 2026-07-27 10:44:23 +00:00 by chinux · 1 comment
chinux commented 2026-07-27 10:44:23 +00:00
Owner
Copy link

Sammelissue für Befunde aus dem Code-Review, die einzeln je unter 30 Minuten liegen, aber zusammen einen Durchgang wert sind. Keine Funktionsänderung, keine Verhaltensänderung.

1. list_scripts ohne Leseberechtigung

routers/command_router.py:181:

@router.get("/scripts")
async def list_scripts(db=Depends(get_db), cu=Depends(get_current_user)):

Alle anderen Script-Endpunkte nutzen require_role("operator"), die Log-Abfrage require_read(). Hier reicht ein beliebiges authentifiziertes Konto — auch ein reines Mandanten-Lesekonto — um alle gespeicherten Scripts samt vollständigem Inhalt zu lesen. Scripts sind Freitext und enthalten in der Praxis Hostnamen, Pfade, gelegentlich Zugangsdaten.

Zusätzlich fehlt jede Mandantenfilterung: Scripts sind global, was vermutlich beabsichtigt ist, in Kombination mit dem fehlenden Rollenschutz aber bedeutet, dass ein Kundenkonto (#224) sie sähe.

Fix: require_role("operator") wie bei den Schreibpfaden.

2. Widersprüchlicher GUAC_SECRET-Default

routers/deploy_router.py:26:

GUAC_SECRET = os.getenv("GUAC_SECRET", "theprox-guac-secret-change-me")

main.py:72 bricht den Start ab, wenn GUAC_SECRET genau dieser Platzhalter oder kürzer als 32 Zeichen ist. rdp_router.py:19 nutzt korrekt os.getenv("GUAC_SECRET", "").

Praktisch nicht ausnutzbar, weil main.py beim Import scheitert. Aber der Platzhalter im Code lädt dazu ein, die Fail-Fast-Prüfung später „aufzuweichen", und dann ist der schwache Default plötzlich wirksam. Fix: gleiche Form wie in rdp_router.py.

3. except Exception: pass — 50 Vorkommen

140 except Exception im Server, davon 50 mit reinem pass. Ein Teil ist legitim (Best-Effort-Notifications, WS-Close in Aufräumpfaden). Der Rest verschluckt Fehler ohne Spur.

Kein Pauschalumbau. Vorgehen: jedes Vorkommen einzeln ansehen und in eine der drei Kategorien einordnen:

  • legitim → Kommentar dazu, warum der Fehler ignoriert werden darf
  • sollte geloggt werden → logger.debug/warning mit Kontext
  • sollte behandelt werden → als eigenes Issue melden, hier nicht ändern

Ergebnis ist Nachvollziehbarkeit, keine Verhaltensänderung.

4. Routergrößen

admin_router.py     1756
deploy_router.py    1460
security_router.py  1214
main.py             1029
agent_ws.py          918

main.py enthält neben dem App-Setup auch Enrollment, Install-Script-Erzeugung und Binary-Auslieferung — Fachlogik, die dort nicht hingehört.

Hier NICHT anfassen. Die Extraktion ist #95 und wird durch #212 (services/guest_view.py) teilweise eingelöst. Nach #212 gehört #95 neu bewertet. Dieser Punkt steht hier nur zur Vollständigkeit des Reviews.

5. ScheduledJob.last_status bleibt nach Absturz auf „running"

reconcile_running_tasks() (admin_router.py:1020) räumt nach einem Neustart verwaiste Task-Zeilen auf, aber nicht ScheduledJob.last_status.

Funktional harmlos — next_run_at wurde vor dem Lauf gesetzt, der Job läuft also wieder. Rein kosmetisch in der Anzeige. Fix: im bestehenden Reconcile-Durchgang last_status="running" auf "unknown" setzen, mit Hinweis im last_output.

Prompt für Claude Code

Arbeite die fünf Punkte aus dem Sammelissue ab. Reine Hygiene — keine
Funktions- oder Verhaltensänderung. Jeder Punkt ein eigener Commit.

1. command_router.py:181 -> list_scripts von get_current_user auf
   require_role("operator") umstellen. Prüfen, ob das Frontend den Endpunkt
   aus einem Kontext aufruft, in dem der Nutzer nur viewer ist — wenn ja,
   im Issue melden statt die Rolle zu senken.

2. deploy_router.py:26 -> os.getenv("GUAC_SECRET", "") wie in
   rdp_router.py:19. Prüfen, ob GUAC_SECRET in deploy_router überhaupt
   verwendet wird; wenn nicht, die Zeile entfernen.

3. Alle 50 "except Exception: ... pass" durchgehen. Pro Fall:
   legitim -> Begründungskommentar; unklar -> logger.debug/warning mit
   Kontext; behandlungsbedürftig -> NICHT hier ändern, sondern als neues
   Issue vorschlagen. Am Ende eine Liste der drei Kategorien in den
   Issue-Kommentar schreiben.

4. Routergrößen: NICHTS tun. Gehört zu #95, wird nach #212 neu bewertet.

5. reconcile_running_tasks() um ScheduledJob erweitern: Zeilen mit
   last_status="running" auf "unknown" setzen, last_output mit
   "Backend-Neustart während der Ausführung — Ergebnis unbekannt".
   next_run_at NICHT anfassen.

Nach jedem Commit: server/tests/ laufen lassen.

Definition of Done

  • list_scripts rollengeschützt
  • Kein Platzhalter-Secret mehr im Code
  • Alle 50 Stellen eingeordnet, Liste im Issue-Kommentar
  • Reconcile deckt ScheduledJob mit ab
  • Punkt 4 unangetastet
Sammelissue für Befunde aus dem Code-Review, die einzeln je unter 30 Minuten liegen, aber zusammen einen Durchgang wert sind. Keine Funktionsänderung, keine Verhaltensänderung. ## 1. `list_scripts` ohne Leseberechtigung `routers/command_router.py:181`: ```python @router.get("/scripts") async def list_scripts(db=Depends(get_db), cu=Depends(get_current_user)): ``` Alle anderen Script-Endpunkte nutzen `require_role("operator")`, die Log-Abfrage `require_read()`. Hier reicht ein beliebiges authentifiziertes Konto — auch ein reines Mandanten-Lesekonto — um **alle gespeicherten Scripts samt vollständigem Inhalt** zu lesen. Scripts sind Freitext und enthalten in der Praxis Hostnamen, Pfade, gelegentlich Zugangsdaten. Zusätzlich fehlt jede Mandantenfilterung: Scripts sind global, was vermutlich beabsichtigt ist, in Kombination mit dem fehlenden Rollenschutz aber bedeutet, dass ein Kundenkonto (#224) sie sähe. Fix: `require_role("operator")` wie bei den Schreibpfaden. ## 2. Widersprüchlicher `GUAC_SECRET`-Default `routers/deploy_router.py:26`: ```python GUAC_SECRET = os.getenv("GUAC_SECRET", "theprox-guac-secret-change-me") ``` `main.py:72` bricht den Start ab, wenn `GUAC_SECRET` genau dieser Platzhalter oder kürzer als 32 Zeichen ist. `rdp_router.py:19` nutzt korrekt `os.getenv("GUAC_SECRET", "")`. Praktisch nicht ausnutzbar, weil `main.py` beim Import scheitert. Aber der Platzhalter im Code lädt dazu ein, die Fail-Fast-Prüfung später „aufzuweichen", und dann ist der schwache Default plötzlich wirksam. Fix: gleiche Form wie in `rdp_router.py`. ## 3. `except Exception: pass` — 50 Vorkommen 140 `except Exception` im Server, davon **50 mit reinem `pass`**. Ein Teil ist legitim (Best-Effort-Notifications, WS-Close in Aufräumpfaden). Der Rest verschluckt Fehler ohne Spur. Kein Pauschalumbau. Vorgehen: jedes Vorkommen einzeln ansehen und in eine der drei Kategorien einordnen: - **legitim** → Kommentar dazu, warum der Fehler ignoriert werden darf - **sollte geloggt werden** → `logger.debug/warning` mit Kontext - **sollte behandelt werden** → als eigenes Issue melden, hier nicht ändern Ergebnis ist Nachvollziehbarkeit, keine Verhaltensänderung. ## 4. Routergrößen ``` admin_router.py 1756 deploy_router.py 1460 security_router.py 1214 main.py 1029 agent_ws.py 918 ``` `main.py` enthält neben dem App-Setup auch Enrollment, Install-Script-Erzeugung und Binary-Auslieferung — Fachlogik, die dort nicht hingehört. **Hier NICHT anfassen.** Die Extraktion ist #95 und wird durch #212 (`services/guest_view.py`) teilweise eingelöst. Nach #212 gehört #95 neu bewertet. Dieser Punkt steht hier nur zur Vollständigkeit des Reviews. ## 5. `ScheduledJob.last_status` bleibt nach Absturz auf „running" `reconcile_running_tasks()` (`admin_router.py:1020`) räumt nach einem Neustart verwaiste `Task`-Zeilen auf, aber nicht `ScheduledJob.last_status`. Funktional harmlos — `next_run_at` wurde vor dem Lauf gesetzt, der Job läuft also wieder. Rein kosmetisch in der Anzeige. Fix: im bestehenden Reconcile-Durchgang `last_status="running"` auf `"unknown"` setzen, mit Hinweis im `last_output`. --- ## Prompt für Claude Code ``` Arbeite die fünf Punkte aus dem Sammelissue ab. Reine Hygiene — keine Funktions- oder Verhaltensänderung. Jeder Punkt ein eigener Commit. 1. command_router.py:181 -> list_scripts von get_current_user auf require_role("operator") umstellen. Prüfen, ob das Frontend den Endpunkt aus einem Kontext aufruft, in dem der Nutzer nur viewer ist — wenn ja, im Issue melden statt die Rolle zu senken. 2. deploy_router.py:26 -> os.getenv("GUAC_SECRET", "") wie in rdp_router.py:19. Prüfen, ob GUAC_SECRET in deploy_router überhaupt verwendet wird; wenn nicht, die Zeile entfernen. 3. Alle 50 "except Exception: ... pass" durchgehen. Pro Fall: legitim -> Begründungskommentar; unklar -> logger.debug/warning mit Kontext; behandlungsbedürftig -> NICHT hier ändern, sondern als neues Issue vorschlagen. Am Ende eine Liste der drei Kategorien in den Issue-Kommentar schreiben. 4. Routergrößen: NICHTS tun. Gehört zu #95, wird nach #212 neu bewertet. 5. reconcile_running_tasks() um ScheduledJob erweitern: Zeilen mit last_status="running" auf "unknown" setzen, last_output mit "Backend-Neustart während der Ausführung — Ergebnis unbekannt". next_run_at NICHT anfassen. Nach jedem Commit: server/tests/ laufen lassen. ``` ## Definition of Done - [ ] `list_scripts` rollengeschützt - [ ] Kein Platzhalter-Secret mehr im Code - [ ] Alle 50 Stellen eingeordnet, Liste im Issue-Kommentar - [ ] Reconcile deckt `ScheduledJob` mit ab - [ ] Punkt 4 unangetastet
chinux commented 2026-07-27 10:44:25 +00:00
Author
Owner
Copy link

Aus dem Code-Review vom 2026-07-27 (Commit 4152c4d). Gesamtblock: #226, #227, #228, #229, #230, #231.

Abarbeitung niedrig → hoch: #231 → #230 → #229 → #228 → #227 → #226.

Aus dem Code-Review vom 2026-07-27 (Commit 4152c4d). Gesamtblock: #226, #227, #228, #229, #230, #231. Abarbeitung niedrig → hoch: #231 → #230 → #229 → #228 → #227 → #226.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
fix/vm-agent-update-error
fix/bulk-update-autoclose
security/tenant-scope-108
refactor/dashboard-page-split
fix/rust-agent-parity-features
feat/rust-agent-parity
archive/python-node-agent
Labels
Clear labels   
agent
Node-/VM-Agent

  
bug
Funktionaler Defekt

  
frontend
Svelte/TS Frontend

  
refactor
Umbau ohne Verhaltensaenderung

  
tech-debt
Altlast / Aufraeumen

No labels
agent
bug
frontend
refactor
tech-debt
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
chinux/theProx#231
No description provided.