fix(sync): parse RFC 6266 Content-Disposition (filename*) — WOO-553

[WilcoLouwerse]

Summary

Forward-port the RFC 6266 Content-Disposition parser fix from the WOO stable hotfix (v0.2.24-woo-1, currently live as prerelease) to the development branch. Without this PR the bug re-lands on production as soon as WOO upgrades to a newer main/stable release.

• Parent story: WOO-552
• Sub-task tracking this PR: WOO-553
• Stable hotfix release (already live on openwoo.commonground.nu): https://github.com/ConductionNL/integriq/releases/tag/v0.2.24-woo-1

What changed

SynchronizationService::getFilenameFromHeaders() on development still contained the naïve explode('=', $header) extractor (line 9057-9058 of lib/Service/SynchronizationService.php) that xxllnc's RFC 6266 header shape (filename="…"; filename*=UTF-8''…) breaks: the extractor splits on = inside filename*=, ends up with "…"; filename* in [1], and trim(…, '"') leaves interior punctuation intact. Result: corrupt filename → saveFile() refuses or produces an unfindable file.

Fix:

• Replace the Content-Disposition parsing branch with a call to a new private helper parseContentDispositionFilename() that:
  • splits the header on ; (so filename*=UTF-8''pct-encoded no longer fools the extractor),
  • prefers filename* over filename per RFC 6266 §4.3,
  • accepts case-insensitive parameter names,
  • strips quotes on the plain filename value,
  • decodes the pct-encoded UTF-8 payload of filename* via rawurldecode() (RFC 3986 §2.1),
  • logs and falls back on non-UTF-8 charsets via a second helper decodeRfc5987ExtendedValue().
• Add 10 unit tests via SynchronizationServiceContentDispositionTest, which reaches the private parser through ReflectionClass — same shape as the tests already living on the stable hotfix branch, where they pass 10/10.

The URL-fallback branch is left untouched — the development signature (ObjectEntity $result, request via $result->getObject()['request']) differs from stable, but that branch is outside the scope of this fix.

Test plan

• php -l lib/Service/SynchronizationService.php — no syntax errors.
• CI: PHPUnit runs 10/10 on the new test file (same tests pass locally on the stable hotfix branch; identical private-parser signature here).
• Review by an OpenConnector maintainer.

Why this exists as a separate PR

The WOO stable hotfix (v0.2.24-woo-1) is already prerelease-cut and live on openwoo.commonground.nu. This PR ensures the fix carries forward into the next main-line release rather than getting lost on a hotfix branch that never merges to development.

🤖 Generated with Claude Code

[WilcoLouwerse]

Verdict: REQUEST_CHANGES (Thorough) — self-review posted as COMMENT (GitHub blocks self-APPROVE/REQUEST_CHANGES)

Recommendation: fix the 4 🔴 CI-red blockers before merge — they're all mechanical style rules I violated in my own PR-introduced code.

Findings — 4 🔴, 2 🟡, 2 🟢 (inline for the code-attached ones; the two below are cross-cutting):

🟢 Test coverage — nice-to-have follow-ups

The 10 tests cover the DoD scenarios well. Additional cases worth considering (non-blocking):

• filename*=UTF-8'' (empty pct-encoded value) — asserts that the extended value returning empty falls back to plain filename.
• Whitespace around = (filename = "foo.pdf") — the current trim(substr(...)) handles this, but a test locks the behaviour.
• Path-traversal probe (filename="../../../etc/passwd") — asserts the parser returns the raw string verbatim (sanitization is downstream's responsibility).

🟢 Path-traversal / disk-write sanitization surface (pre-existing)

$filename from getFilenameFromHeaders() flows directly into FileService::saveFile(fileName: $filename, ...) at line 8823 with no sanitization at this layer. Both the old explode('=') extractor and the new RFC 6266 parser pass raw filenames through, so this PR does not change the attack surface — it's not a regression. But: if OpenRegister's FileService::saveFile() doesn't reject ../ sequences or path separators, a hostile upstream could write files outside the intended location. Worth a follow-up ticket to audit FileService::saveFile()'s filename sanitization, tracked separately from WOO-552.

CI status

• 🔴 quality / PHP Quality (phpcs) — 3 errors on new code (findings #1, #3, #4 below).
• 🔴 quality / PHP Quality (phpmd) — 1 violation on new code (finding #2 below).
• 🟡 quality / PHP Quality (phpstan) — still running when I checked.
• ✅ All other checks passing (lint, psalm, PHPUnit test matrix, license, security, CodeQL, coverage baseline).

All four CI-red findings are trivial 5-minute fixes in my own new helpers.

[WilcoLouwerse]

Verdict: APPROVE (Quick) — self-review posted as COMMENT (GitHub blocks self-APPROVE)

All 6 prior inline findings addressed by 8b119af, verified by grep/inspection:

Prior	Finding	Fix in 8b119af
🔴	Named arg on parseContentDispositionFilename	line 9056 → headerValue: $...
🔴	Named arg on decodeRfc5987ExtendedValue	line 9129 → value: $value
🔴	$eq → $eqPos (phpmd ShortVariable)	line 9120 (and refs)
🔴	Inline comment capitalization	line 9210 → // Decode via rawurldecode() — …
🟡	Test namespace psr-4 mismatch	OCA\Integriq\Tests\Unit\Service
🟡	Quoted-string ; breaks the parse	new splitHeaderParameters() tokenizer

Beyond the findings:

• 3 new tests locking behaviour: testFilenameWithSemicolonInsideQuotedValuePreservesFilename, testFilenameWithPathTraversalPayloadIsReturnedVerbatim, testFilenameWithWhitespaceAroundEqualsIsAccepted. Total 13 tests (was 10).

CI status on the new SHA: still queued when this review was written. Mechanical rules (named args, ShortVariable, comment capitalization) are satisfied by inspection; if phpcs/phpmd re-emit any of these on the new SHA, it will be a rule I missed rather than a rule I ignored. Will monitor and post a follow-up if CI stays red.

Resolving all 6 prior inline threads (all self-authored).

[github-actions]

Quality Report — ConductionNL/integriq @ ddc020a

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	❌
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 141/141
npm			✅	✅ 537/537
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					❌
Playwright					⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates					❌

---

Quality workflow — 2026-09-04 10:00 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/integriq @ 44ef80d

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 141/141
npm			✅	✅ 537/537
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates					✅

---

Quality workflow — 2026-09-04 10:38 UTC

Download the full PDF report from the workflow artifacts.

[bbrands02]

explode(';', $headerValue) also splits semicolons that are inside a quoted filename. For example, attachment; filename="rapport; versie 2.pdf" would now return rapport instead of rapport; versie 2.pdf. Could we parse parameter separators while respecting quoted strings, and add a test for this case?

[WilcoLouwerse]

Goed gezien, Barry — die exact case. Wilco had 'm eerder al 🟡-gevlagd in zijn tweede self-review en de fix + tokenizer in commit 8b119af doorgevoerd:

• Nieuwe private helper splitHeaderParameters(string): array op SynchronizationService — quoted-string-aware tokenizer die "-open state tracked en alleen op ; buiten quotes splitst (per RFC 6266 §4).
• parseContentDispositionFilename() roept nu die helper aan i.p.v. explode(';', ...).
• Test testFilenameWithSemicolonInsideQuotedValuePreservesFilename op foo;bar.pdf (regel 133).

Nu in commit 205a54e een extra test testFilenameWithSemicolonAndSpaceInsideQuotedValuePreservesFilename met jouw exacte string attachment; filename="rapport; versie 2.pdf" toegevoegd — zo staat de garantie letterlijk in de suite, niet alleen impliciet via het foo;bar.pdf geval. 13 → 14 tests.

Zelfde fix + 4 tests (waaronder jouw voorbeeld) ook geport naar de WOO-stable hotfix branch hotfix/woo-552-content-disposition-rfc6266 — een fresh prerelease v0.2.24-woo-2 wordt nu gecut die v0.2.24-woo-1 op openwoo.commonground.nu opvolgt.

Thanks voor de scherpe review!

[github-actions]

Quality Report — ConductionNL/integriq @ 2aa915d

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 141/141
npm			✅	✅ 537/537
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					❌
Newman					✅
Playwright					⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates					✅

---

Quality workflow — 2026-09-04 11:35 UTC

Download the full PDF report from the workflow artifacts.

[github-actions]

Quality Report — ConductionNL/integriq @ fa308af

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
build		✅
build		✅
check-specs		✅
test-l10n		✅
format		✅
check-schema-l10n		✅
check-l10n-js		✅
composer			✅	✅ 141/141
npm			✅	✅ 537/537
app:check-code	⏭️
info.xml	✅
REUSE	❌
PHPUnit					✅
Newman					✅
Playwright					⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates					✅

---

Quality workflow — 2026-09-04 11:56 UTC

Download the full PDF report from the workflow artifacts.