ivooskamp/clearview
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
3f225660c8
clearview/docs/code-review-todo.md

9.6 KiB
Raw Blame History

Code Review TODO — Clearview

Aangemaakt: 2026-05-19 Branch bij review: refactor/scanner-package-frontend Scope: Eerste volledige review (~7.100 regels code) Totaal: 13 CRITICAL · 19 HIGH · 14 MEDIUM · 1 LOW

Werkvolgorde: alle CRITICAL eerst (P0), daarna HIGH (P1), dan MEDIUM/LOW (P2). Per item staan severity, bestand(en):regel(s), en de gewenste fix.

P0 — CRITICAL (eerst dichten)

Auth & secrets

  • Geen authenticatie op enig API-endpoint

    • containers/clearview/src/clearview_app/main.py (alle /api/ routes)
    • Fix: API-key via X-API-Key header met FastAPI Security() dependency, of Bearer-token op alle /api/ routes.

  • Client secrets staan plaintext in DB

    • containers/clearview/src/clearview_app/models.py:21 (TenantProfile.client_secret)
    • containers/clearview/src/clearview_app/models.py:45 (ScanJob.auth_client_secret)
    • Fix: encrypt-at-rest met cryptography.fernet; key via env var. Decrypt enkel in geheugen bij gebruik.

  • .env niet in .gitignore

    • .gitignore, stack/.env
    • Fix: voeg stack/.env en **/.env toe aan .gitignore; lever stack/.env.example met placeholders; verifieer dat .env nog niet in git history zit (anders rotate credentials).

  • Hardcoded DB-fallback clearview:clearview

    • containers/clearview/src/clearview_app/config.py:17-19
    • Fix: verwijder default; raise RuntimeError("DATABASE_URL required") als env ontbreekt.

  • Adminer publiek op 0.0.0.0:8081

    • stack/docker-compose.yml:44-46
    • Fix: bind aan 127.0.0.1:${ADMINER_PORT:-8081}:8080 of verwijder uit prod-compose.

Injectie & exfiltratie

  • XSS via ongescapete velden in innerHTML (3 vindplaatsen)

    • containers/clearview/site/app.js:658-676 (job.id, job.source_type, job.items_scanned)
    • containers/clearview/site/app.js:885-894 (job.status, total/processed/successful/failed_targets, items_scanned)
    • containers/clearview/site/app.js:175-184 (statusBadge() zonder escape op status)
    • Fix: consequent escHtml() op alle API-velden, óók ID's en numerieke. Geen uitzonderingen.

  • Open redirect via payload.connect_url

    • containers/clearview/site/app.js:472
    • Fix: valideer new URL(payload.connect_url).protocol === 'https:' + host-allowlist (login.microsoftonline.com).

  • SSRF / token-exfiltratie via @odata.nextLink

    • containers/clearview/src/clearview_app/scanners/sharepoint.py:547-553
    • containers/clearview/src/clearview_app/scanners/entra.py:227-272
    • Fix: vergelijk urlparse(next_url).netloc == urlparse(original_url).netloc; gooi anders een RuntimeError.

  • Header injection in Content-Disposition

    • containers/clearview/src/clearview_app/main.py:701-705
    • Fix: type de route-parameter als uuid.UUID zodat FastAPI job_id valideert; urllib.parse.quote(filename, safe="").

P1 — HIGH

Correctheid

  • Token cache zonder TTL — workers crashen na 1 uur

    • containers/clearview/src/clearview_app/scanners/sharepoint.py:34, 512-543
    • Fix: bewaar expires_at = time.time() + result["expires_in"] - 60; invalideer in _get_token_for_host.

  • MSAL ConfidentialClientApplication per aanroep

    • containers/clearview/src/clearview_app/scanners/sharepoint.py:530
    • Fix: module-level dict (tenant_id, client_id, auth_method) -> app. Hergebruik object.

  • _TOKEN_CACHE zonder lock (race in multi-thread)

    • containers/clearview/src/clearview_app/scanners/sharepoint.py:34
    • Fix: threading.Lock rond check-then-write, of functools.lru_cache + TTL-wrapper.

  • Race condition in worker: niet-atomaire job-claim

    • containers/clearview/src/clearview_app/worker.py:48-68
    • Fix: één UPDATE scan_jobs SET status='running' WHERE id=:id AND status='queued' RETURNING id met FOR UPDATE SKIP LOCKED.

  • Auto-refresh race in frontend

    • containers/clearview/site/app.js:1009-1013 + alle tick() callsites
    • Fix: AbortController per render; vorige request cancelen voordat nieuwe gestart wordt.

  • Event-listener accumulatie / re-render-pattern

    • containers/clearview/site/app.js:238-270, 678-702
    • Fix: event delegation op stabiele container (els.jobsTableBody.addEventListener('click', ...)).

  • Sequentieel awaiten van onafhankelijke calls

    • containers/clearview/site/app.js:554-555, 999, 1281, 1364
    • Fix: await Promise.all([refreshJobs(), refreshSelectedJob()]).

  • Niet-afgehandelde floating promise

    • containers/clearview/site/app.js:1143
    • Fix: .catch(err => showFeedback(...)) op testTargetConnection(...).

  • OAuth state-store is in-memory dict (breekt bij --workers >1)

    • containers/clearview/src/clearview_app/onboarding.py:31, 134-145
    • Fix: state opslaan in DB (tabel oauth_states met created_at, consumed_at) of Redis.

  • scan_type ongevalideerd

    • containers/clearview/src/clearview_app/schemas.py:36, main.py:178, 207
    • Fix: Literal["sharepoint","sharepoint_root","mailbox","entra_groups"] in Pydantic schema.

  • datetime.utcnow() deprecated, timezone-naive (overal)

    • main.py, worker.py, models.py:26-27, cert.py:33
    • Fix: datetime.now(timezone.utc); DateTime(timezone=True) in SQLAlchemy-kolommen.

  • ThreadPoolExecutor(max_workers=1) per target

    • containers/clearview/src/clearview_app/worker.py:307
    • Fix: gedeelde executor; documenteer dat future.cancel() lopende scan niet onderbreekt.

  • Geen throttling-respect bij 429 in item-loop

    • containers/clearview/src/clearview_app/scanners/sharepoint.py:603-619
    • Fix: batching via $expand=RoleAssignments of exponential backoff op item-niveau.

Hardening

  • Container draait als root

    • containers/clearview/Dockerfile
    • Fix: RUN adduser --system --ingroup clearview clearview + USER clearview.

  • .deb van packages.microsoft.com zonder checksum

    • containers/clearview/Dockerfile:17-19
    • Fix: hardcoded SHA256 + sha256sum --check, of officiële GPG-key via signed-by.

  • Install-Module ExchangeOnlineManagement zonder versie-pin

    • containers/clearview/Dockerfile:24-26
    • Fix: -RequiredVersion 3.7.0 (of huidige geteste versie).

  • Graph-foutberichten gelekt richting frontend

    • containers/clearview/src/clearview_app/onboarding.py:170, 188
    • Fix: volledig log naar server-side DEBUG; aan client alleen generieke code + HTTP-status.

  • OData-filter injection via displayName/mail

    • containers/clearview/src/clearview_app/scanners/entra.py:178-196
    • Fix: urllib.parse.quote(cleaned.replace("'", "''"), safe="").

  • PowerShell-args zonder UPN-validatie

    • containers/clearview/src/clearview_app/scanners/mailbox.py:181-190
    • containers/clearview/src/clearview_app/scanners/exo_scripts/get-permissions.ps1:107
    • Fix: re.fullmatch(r"[^@\s]{1,64}@[^@\s]{1,255}", upn) vóór _run_pwsh.

Architectuur

  • main.py is 1139 regels — splitsen

    • Fix: routers/{tenants,jobs,onboarding}.py, services/job_service.py, export.py.

  • sharepoint.py is 722 regels — splitsen

    • Fix: _auth.py, _http.py, sharepoint.py (publieke scanfuncties), sharing_links.py.

  • _ensure_schema_columns() met 18 raw ALTER TABLE

    • containers/clearview/src/clearview_app/main.py:1115-1139
    • Fix: vervang door Alembic; commit baseline-migratie + history.

P2 — MEDIUM / LOW

  • app.js 1586 regels, geen build step — splits in ES-modules + esbuild/rollup; verwijder IIFE-wrapper.
  • Focus management bij route-wissel ontbreekt — app.js:1529-1551 — focus naar <h2> van nieuwe route na navigatie.
  • Focus-outline 38% opacity voldoet niet aan WCAG 3:1 — styles.css:292-296 — outline: 2px solid var(--cv-accent).
  • Geen debouncing/abort op jobSiteFilter — app.js:1158-1172 — AbortController per fetch.
  • els.submitFeedback gebruikt voor niet-SharePoint feedback — app.js:682, 711 — generiek feedback-element of context-specifiek.
  • Magic string '__manual__' — app.js:412, 424 — named constant.
  • Icon-knoppen missen aria-label — app.js:229-231 — aria-label toevoegen.
  • scanner.py is shim zonder waarschuwing — DeprecationWarning of verwijderen.
  • except Exception: pass zonder logging (meerdere) — minimaal log.warning(..., exc_info=True).
  • _resolve_credentials(db, ...) mist type-hint — main.py:935 — db: Session.
  • CreateScanAppResponse lekt secret zonder waarschuwing — schemas.py:149-155 — show_once: bool veld + log-suppression voor dit endpoint.
  • Deviations hard-capped op 1000 — main.py:728 — voeg total_deviations_count + truncated: bool toe.
  • Geen logging in scanners-package — import logging; logger = logging.getLogger(__name__) overal.
  • list-mailboxes.ps1 laadt alles in geheugen — -ResultSize Unlimited → paginering of cap.
  • cert.py private key zonder encryptie — documenteer aanname dat caller encryptie-at-rest doet.
  • build-and-push.sh doet git add -A — build-and-push.sh:294 — expliciete file-lijst of bevestigingsprompt.
  • README build-instructies kloppen niet (1/2/3 vs t/r) — README.md:80-84.

Werkwijze

  1. Werk per checkbox; verwijder geen items maar zet [x] als done.
  2. Bij oplevering van een batch: append entry in docs/changelog-develop.md.
  3. CRITICAL items vereisen handmatige verificatie (auth-tests, secret-rotation, XSS-payload checks).
  4. Na P0 + P1: hertest met deze TODO als checklist voordat een nieuwe review wordt aangevraagd.