Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.

feat(resources): SVG sanitiser plugged into upload pipeline#51

Merged
rubenvdlinde merged 1 commit into
developmentfrom
feature/impl-svg-sanitisation
Apr 30, 2026
Merged

feat(resources): SVG sanitiser plugged into upload pipeline#51
rubenvdlinde merged 1 commit into
developmentfrom
feature/impl-svg-sanitisation

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Summary

Implements REQ-RES-009..013 from the svg-sanitisation change. Adds a server-side DOM-based whitelist sanitiser for uploaded SVG bytes and plugs it into the resource-uploads pipeline.

Builds on top of #46 (now merged) — reuses the ResourceException base class so ResourceController::errorResponse() automatically renders the 400 invalid_svg envelope without any controller-level change.

What changed

  • lib/Service/SvgSanitiser.php (new) — pure-PHP DOM-based sanitiser. No Nextcloud dependencies. Conservative whitelist of 24 element types (REQ-RES-010) and 50 attribute types (REQ-RES-011). Strips:
    • any element / attribute not on the whitelist (children removed with the parent);
    • all on* attributes regardless of whitelist (defence in depth, in case a future whitelist edit accidentally adds one);
    • href / xlink:href values starting with javascript: or data: (REQ-RES-012);
    • style values containing expression(, javascript:, or url(data:.
  • lib/Exception/InvalidSvgException.php (new) — extends the existing ResourceException base from PR feat(resources): admin-only resource upload endpoint #46 with stable error code invalid_svg and HTTP 400.
  • lib/Service/ResourceService.php — injects SvgSanitiser; for the SVG branch, sanitises before the size check; throws InvalidSvgException on null; persists the sanitised bytes (NOT the original).
  • l10n/en + l10n/nl — translation entry for the invalid_svg display message.
  • DEVELOPMENT.md — security-review checkbox documenting that adding to the SVG whitelist is a deliberate code change, not an editorial one.

Notes for future contributors

  • Size cap is measured AFTER sanitisation. A 5.5 MB SVG that sanitises down to 4.5 MB is accepted; the 5 MB REQ-RES-003 cap applies to what gets persisted, not what was uploaded. Spec scenario Size cap measured after sanitisation covers this.
  • Whitelist is conservative on purpose — 24 elements + 50 attributes covers every common SVG drawing primitive without permitting <script>, <foreignObject>, <iframe>, <embed>, <object>, or any of the 60+ exotic SVG elements that carry executable surface. Adding any element / attribute is a security review checkbox per DEVELOPMENT.md (Security review checkboxes section).
  • XXE protection via LIBXML_NONET | LIBXML_NOENT parse flags. The LIBXML_NONET flag prevents the parser from fetching external DTDs / entities; LIBXML_NOENT substitutes entities so they do not amplify recursively. Modern libxml internal expansion limits also protect against billion-laughs (verified in tests).
  • null return → 400 invalid_svg is reserved for unparseable XML or fully-stripped documents. Disallowed elements alone do not fail the upload — they are stripped silently and the request proceeds.

Test plan

  • composer phpcs passes (42 / 42 files clean)
  • composer lint passes (no syntax errors)
  • PHPUnit suite: 145 tests, 394 assertions, 0 failures (16 new sanitiser unit + 6 new integration + 123 pre-existing)
  • Sanitiser unit covers all 16 spec scenarios (clean round-trip, script / foreignObject / on* / javascript: / data: / expression() / url(data:) stripped; safe https preserved; geometry preserved; data-* stripped; external DTD does not fetch; billion-laughs bounded; unparseable → null; empty → null; non-svg root → null)
  • Integration covers tasks 4.1–4.5 (malicious upload sanitised, garbage rejected, oversize-but-sanitises-down accepted, near-cap clean accepted, persisted bytes ≠ original)
  • InvalidSvgException carries the stable error code invalid_svg + HTTP 400 (asserted in integration test)

Pre-existing issues observed (NOT touched by this PR)

  • lib/Service/UserAttributeResolver.php:59 — Psalm UndefinedInterfaceMethod on IUser::getLanguage()
  • lib/Service/ImageMimeValidator.php:109 — PHPStan offset-on-array notice

These predate this change and live in files this PR does not modify; left for a separate fix-in-passing pass on those files.

Spec

  • openspec/changes/svg-sanitisation/proposal.md
  • openspec/changes/svg-sanitisation/tasks.md
  • openspec/changes/svg-sanitisation/specs/resource-uploads/spec.md (REQ-RES-009..013)

@rubenvdlinde rubenvdlinde added the ready-for-code-review Build complete — awaiting code reviewer label Apr 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/mydash @ 7ffc0d9

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 103/103
npm ✅ 342/342
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-04-30 12:25 UTC

Download the full PDF report from the workflow artifacts.

Adds a server-side DOM-based whitelist sanitiser for uploaded SVG bytes
and plugs it into the resource-uploads pipeline before the 5 MB size
cap (so the cap is measured against the persisted, sanitised bytes).

- lib/Service/SvgSanitiser.php (new): 24-element / 50-attribute
  whitelist (REQ-RES-010 / REQ-RES-011); strips ALL on* attributes
  defence-in-depth; rejects javascript: / data: hrefs (REQ-RES-012);
  removes style attributes containing expression() / javascript: /
  url(data:); parses with LIBXML_NONET | LIBXML_NOENT for XXE
  protection (REQ-RES-013); returns null on parse failure or
  fully-stripped result.
- lib/Exception/InvalidSvgException.php (new): extends ResourceException
  with stable error code 'invalid_svg' and HTTP 400. ResourceController
  already maps any ResourceException through errorResponse() so no
  controller change is needed.
- lib/Service/ResourceService.php: injects SvgSanitiser; for the SVG
  branch, sanitises BEFORE the size check; throws InvalidSvgException
  on null; persists the sanitised bytes (NOT the original).
- l10n/en + nl: translation entry for the invalid_svg display message.
- DEVELOPMENT.md: security-review checkbox documenting that adding to
  the whitelist is a deliberate code change.
- tests: 16 unit tests for SvgSanitiser (clean / script / foreignObject
  / on* / javascript: / data: / expression() / url(data:) / safe https
  / geometry preserved / data-* stripped / external DTD bounded /
  billion-laughs bounded / unparseable / empty / non-svg root) +
  6 integration tests for ResourceService that wire the real
  sanitiser end-to-end (malicious upload sanitised, garbage rejected,
  oversize-but-sanitises-down accepted, near-cap clean accepted,
  persisted bytes != original).
@rubenvdlinde rubenvdlinde force-pushed the feature/impl-svg-sanitisation branch from 4784de7 to b6a6fd7 Compare April 30, 2026 12:35
@rubenvdlinde rubenvdlinde merged commit e010e20 into development Apr 30, 2026
31 of 32 checks passed
@rubenvdlinde rubenvdlinde deleted the feature/impl-svg-sanitisation branch April 30, 2026 12:35
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/mydash @ 5030310

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 103/103
npm ✅ 342/342
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-04-30 12:37 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde added a commit that referenced this pull request May 3, 2026
#51)

Adds a server-side DOM-based whitelist sanitiser for uploaded SVG bytes
and plugs it into the resource-uploads pipeline before the 5 MB size
cap (so the cap is measured against the persisted, sanitised bytes).

- lib/Service/SvgSanitiser.php (new): 24-element / 50-attribute
  whitelist (REQ-RES-010 / REQ-RES-011); strips ALL on* attributes
  defence-in-depth; rejects javascript: / data: hrefs (REQ-RES-012);
  removes style attributes containing expression() / javascript: /
  url(data:); parses with LIBXML_NONET | LIBXML_NOENT for XXE
  protection (REQ-RES-013); returns null on parse failure or
  fully-stripped result.
- lib/Exception/InvalidSvgException.php (new): extends ResourceException
  with stable error code 'invalid_svg' and HTTP 400. ResourceController
  already maps any ResourceException through errorResponse() so no
  controller change is needed.
- lib/Service/ResourceService.php: injects SvgSanitiser; for the SVG
  branch, sanitises BEFORE the size check; throws InvalidSvgException
  on null; persists the sanitised bytes (NOT the original).
- l10n/en + nl: translation entry for the invalid_svg display message.
- DEVELOPMENT.md: security-review checkbox documenting that adding to
  the whitelist is a deliberate code change.
- tests: 16 unit tests for SvgSanitiser (clean / script / foreignObject
  / on* / javascript: / data: / expression() / url(data:) / safe https
  / geometry preserved / data-* stripped / external DTD bounded /
  billion-laughs bounded / unparseable / empty / non-svg root) +
  6 integration tests for ResourceService that wire the real
  sanitiser end-to-end (malicious upload sanitised, garbage rejected,
  oversize-but-sanitises-down accepted, near-cap clean accepted,
  persisted bytes != original).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

ready-for-code-review Build complete — awaiting code reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant