Skip to content

Forceer schemavalidatie van projectbestanden voor verwerking#68

Merged
uittenbroekrobbert merged 6 commits into
mainfrom
fix/project-schema-validation
May 28, 2026
Merged

Forceer schemavalidatie van projectbestanden voor verwerking#68
uittenbroekrobbert merged 6 commits into
mainfrom
fix/project-schema-validation

Conversation

@anneschuth

@anneschuth anneschuth commented May 17, 2026

Copy link
Copy Markdown
Member

De kwetsbaarheid

Projectbestanden werden nooit gevalideerd tegen opi/schemas/project_v2.json. De dependency jsonschema was wel gedeclareerd maar nergens gebruikt. Beide ingress-paden laadden het YAML-bestand met ruamel en gebruikten het rechtstreeks:

  • API/wizard: opi/manager/project_manager.py process_project_from_git
  • Directe git-commit, opgepikt door de monitor: opi/core/git_monitor.py file_change_handler

Zonder schemacontrole stroomden vijandige waarden ongehinderd door naar de connectors: een namespace met een newline, een projectnaam met ../, onbekende top-level velden en ongeldige rollen. Dit is de hefboom onder meerdere andere kwetsbaarheden (hostile namespace, ownership, veld-injectie).

De fix

  • Nieuwe module opi/core/project_schema.py: een enkel validatiepunt met een Draft 2020-12 validator. Faalt gesloten, ProjectSchemaError met een Nederlandse, op de gebruiker gerichte foutmelding die het eerste foutieve veldpad en de melding noemt. Bij een schending wordt het project afgewezen en niet verwerkt.
  • Validatie ingebouwd in beide ingress-paden, voor verdere verwerking (in process_project_from_git na de in-memory migratie, in file_change_handler voor check_and_create_namespaces).
  • project_v2.json aangescherpt voor beveiligingsgevoelige velden die voorheen kale {"type": "string"} waren:
    • project name: ^[a-z][a-z0-9-]*$, maxLength 30
    • deployment.namespace, deployment.cluster, component name: k8s DNS-1123 ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$, maxLength 63
    • storage mount-path: absoluut pad ^/[\w./-]+$ (nieuwe storage-entry def, voorheen kale object)
    • component path match/rewrite: ^[A-Za-z0-9/_.\-]*$
    • env-var namen: propertyNames ^[A-Za-z_][A-Za-z0-9_]*$
    • users[].role: enum uitgebreid met developer (komt voor in bestaande projecten en fixtures; zonder deze uitbreiding zou geldige invoer afgewezen worden)

De aanscherpingen zijn getoetst tegen de voorbeeldprojecten in projects/ zodat geldige invoer niet breekt.

Testplan

  • tests/test_project_schema_validation.py (rood-groen): vijandige projecten (newline in namespace, ../ in naam, onbekend top-level veld, ongeldige rol) worden afgewezen; het bestaande projects/simple-example.yaml slaagt. 6/6 geslaagd via uv run pytest tests/test_project_schema_validation.py -q --noconftest.
  • ruff check, ruff format, pyright schoon op alle gewijzigde bestanden (pre-commit hooks geslaagd).
  • Volledige unit-suite kon niet draaien: collectie faalt al op import fastapi met een AssertionError in pydantic onder Python 3.14.0b4 (de enige beschikbare 3.14). Dit is een bestaand omgevingsprobleem, reproduceerbaar op een schone tree zonder deze wijziging, en staat los van deze PR (CLAUDE.md vermeldt dit ook). De validatiemodule importeert alleen jsonschema + stdlib en is daarom los getest.

Opmerking

Dit is een bestaand probleem, onafhankelijk van de andere lopende PR's.

@uittenbroekrobbert

Copy link
Copy Markdown
Contributor

Drie commits toegevoegd vanuit een diepe review (commits 42dfc51, a279b6c, 48085da op deze branch). Allemaal binnen de scope van deze PR (zelfde injectieklasse als waar de PR op gericht is); 16/16 schema-tests groen, bredere suite (3282 tests) ook groen.

1. Regex-anchors $\Z (commit 42dfc514)

jsonschema valideert pattern via Python re.search. Daarin matcht $ óók voor een afsluitende \n, dus elke pattern met $-anchor accepteerde stilzwijgend een trailing newline. Empirisch geverifieerd op origin/main schema vs. fixed:

  • namespace: "valid-project\n" — origin ACCEPT, fixed REJECT
  • name: "valid-project\n" — origin ACCEPT, fixed REJECT

Alle patterns op name, namespace, cluster, component-name, mount-path, match, rewrite en env-vars-keys zijn nu \Z-anchored.

2. Validatie-volgorde rond auto-migratie (commit a279b6c0)

process_project_from_git riep save_project_file + commit_and_push aan voor de auto-migratie vóór validate_project_schema. Een vijandig project dat schoon migreert werd dus eerst gecommit naar zad-projects met de ops-manager-identiteit, en pas dáárna geweigerd. Validatie loopt nu als eerste; bij een schema-violation wordt niets naar disk of git geschreven. git_monitor.file_change_handler had de juiste volgorde al — daar geen wijziging.

3. Scope-gap: env-var-values + mount-path traversal (commit 42dfc514)

Twee categorieën strings die de oorspronkelijke versie schema-vrij liet:

  • env-vars values: keys hadden een pattern, values waren elke string. Een newline in een env-var-value rendert naar value: "<...>\n injectedKey: ..." in deployment.yaml.jinja — exact het primitief dat PR fix: YAML-injectie via authorization-wall banner (HIGH) #63 op templateniveau dichtte. Pattern toegevoegd dat CR, LF en NUL weigert. Normale waarden (spaces, =, /, @, etc.) blijven geldig — empirisch getest.
  • mount-path: pattern ^/[\w./-]+$ accepteerde /var/../etc/passwd omdat .. als substring niet werd geblokkeerd. Negatieve lookahead (?!.*\.\.) toegevoegd; normale dot-bestandsnamen zoals /data/v1.0/files blijven valid — ook empirisch getest.

image en repositories[].url heb ik bewust niet aangeraakt — die zijn moeilijker te restrictief patroon-bepalen en hebben downstream filtering. Graag jouw inschatting of we die in deze PR meenemen of als follow-up doen.

Tests (commit 48085dab)

9 regressie-tests toegevoegd in test_project_schema_validation.py voor elk van bovenstaande gevallen plus een positieve sanity-test per pattern. Alle 16 tests in het bestand passen, en de bredere unit-suite (3282 tests) is groen.


Audit: dit commentaar is achteraf gegenereerd nadat de drie commits direct op de PR-branch zijn gepushed. Reviewproces beschreven in de SECURITY.md die we parallel aan deze PR-stroom opbouwen.

anneschuth and others added 6 commits May 28, 2026 10:02
Projectbestanden werden nooit gevalideerd tegen opi/schemas/project_v2.json.
Zowel de API/wizard-route (process_project_from_git) als directe git-commits
(git_monitor.file_change_handler) laadden het bestand met ruamel en gebruikten
het rechtstreeks, zonder schemacontrole. Hierdoor konden vijandige waarden
(namespace met newline, projectnaam met ../, onbekende top-level velden,
ongeldige rollen) ongehinderd doorstromen naar de connectors.

Wijzigingen:
- Nieuwe module opi/core/project_schema.py met een enkel validatiepunt
  (Draft 2020-12). Faalt gesloten: bij een schemaschending wordt het project
  afgewezen met een duidelijke Nederlandse foutmelding en niet verwerkt.
- Validatie ingebouwd in beide ingress-paden, voor verdere verwerking.
- project_v2.json aangescherpt met patronen/limieten voor
  beveiligingsgevoelige velden: project name (^[a-z][a-z0-9-]*$, max 30),
  deployment namespace/cluster en component name (DNS-1123, max 63),
  storage mount-path (absoluut pad), component path match/rewrite,
  env-var namen, en role-enum uitgebreid met de in projecten gebruikte
  waarde 'developer'.
- Test test_project_schema_validation.py: rood-groen bewijs dat vijandige
  invoer wordt afgewezen en het bestaande voorbeeldproject blijft slagen.

Dit is een bestaand probleem, los van de andere lopende PR's.
De aangescherpte fail-closed schemavalidatie wees elk project af dat
deployments[].backup gebruikt. Dat veld is een bestaande productiefeature:
de backup-scheduler leest deployments[].backup.schedule (RRULE) en het
detail-edit-formulier schrijft schedule:time/day/monthday in dezelfde map.
Zonder dit zou elke deployment met geplande backups stoppen met werken.

Voegt een deployment-backup definitie toe met getypeerde bekende velden en
alleen scalaire extra waarden, zodat de schedule-formuliervelden geaccepteerd
worden zonder het schema open te zetten voor objectinjectie. Regressietest
toegevoegd die een deployment met geplande backup laat slagen.
….. in mount-path

Three review findings on the schema patterns:

1. End-of-string anchor $ allowed a trailing newline under Python re
   (jsonschema regex engine), so values like 'valid-project<NL>' passed.
   That is exactly the injection class the schema is meant to stop. All
   patterns switched to \Z (strict end-of-string).

2. env-vars map: keys were pattern-restricted but values were any string.
   A newline in an env-var value is the same primitive that PR #63 closed
   at the deployment.yaml.jinja template layer; this closes it at the
   source. Added pattern rejecting CR, LF and NUL on the value side, plus
   \Z anchor on the key pattern.

3. mount-path pattern allowed .. as substring. Negative lookahead now
   rejects any .. while keeping normal filename dots like /data/v1.0/files
   valid.
process_project_from_git ran the auto-migration save_project_file +
git commit_and_push **before** calling validate_project_schema. A hostile
project that happens to migrate cleanly was therefore written to
zad-projects with our ops-manager identity, and only rejected afterwards.

The validator now runs first; a schema violation aborts before any side-
effect touches disk or git. git_monitor.file_change_handler already
ordered validation first, so no change needed there.
Nine additional tests pinning the augmentations from the previous two
commits:

- Trailing-newline rejection (namespace, project name) - validates
  the \Z anchor change.
- env-var value with CR, LF, or NUL rejected; plain string with spaces /
  special chars still accepted.
- mount-path with .. anywhere in the string rejected; normal dotted
  filenames still accepted.

All 16 tests in the file pass.
Follow-up to the env-vars-value and mount-path tightening in 42dfc51:
also restrict the other two free-form string fields that land in
shell- or template-rendered contexts.

- components[].image and deployment-components[].image: pattern requires
  alnum first char and image-safe chars only (A-Za-z0-9._:/@-). Rejects
  spaces, shell metacharacters, newlines, quotes. Allows everything in
  real-world refs incl. registry+port, tag, digest.
- repositories[].url, registries[].url and helmfiles[].url: must start
  with https/http/ssh/git scheme; body excludes whitespace, NUL and
  double-quote. Rejects file:// and other unexpected schemes, plus the
  obvious injection payloads (semicolon-based RCE, newline-based YAML
  injection).

Empirically verified against projects/simple-example.yaml (still
accepted) and the bredere unit-suite (still green). 8 additional
regression tests; 24/24 tests in test_project_schema_validation.py
pass.
@uittenbroekrobbert uittenbroekrobbert force-pushed the fix/project-schema-validation branch from 4669263 to a38c3c3 Compare May 28, 2026 08:16
@uittenbroekrobbert uittenbroekrobbert merged commit 891ffc5 into main May 28, 2026
19 of 20 checks passed
@uittenbroekrobbert uittenbroekrobbert deleted the fix/project-schema-validation branch May 28, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants