From dc10e51bd7f8579dd85dc46ff88f1a2e90a8f739 Mon Sep 17 00:00:00 2001 From: Wilco Louwerse Date: Fri, 4 Sep 2026 10:34:10 +0200 Subject: [PATCH 1/4] =?UTF-8?q?fix(sync):=20parse=20RFC=206266=20Content-D?= =?UTF-8?q?isposition=20(filename*)=20=E2=80=94=20WOO-552?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port the RFC 6266 parser fix from the WOO stable hotfix (hotfix/woo-552-content-disposition-rfc6266, release `v0.2.24-woo-1`) to the development branch, so the next main / stable release does not re-introduce the bug. Same shape as the stable fix: - Replace the naive `explode('=', $header)` in `SynchronizationService::getFilenameFromHeaders()` 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. - Add the second helper `decodeRfc5987ExtendedValue()`. - Add 10 unit tests via `SynchronizationServiceContentDispositionTest`, reaching the private parser through `ReflectionClass` — same tests that cover the stable fix. Method signature on development is `getFilenameFromHeaders(array $response, ObjectEntity $result)` (the URL-fallback branch reads the request via `$result->getObject()['request']`); only the Content-Disposition branch is touched by this commit, the URL-fallback logic is left as-is. Triggered by xxllnc's Zaken API change of 2026-08-19 (started emitting both `filename` and `filename*`, breaking OpenWoo.App document downloads; rolled back but scheduled to re-roll behind a feature toggle). Stable hotfix `v0.2.24-woo-1` is already live as prerelease for direct install on WOO production; this PR ensures the next main-line release does not regress. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/Service/SynchronizationService.php | 97 ++++++++++++- ...onizationServiceContentDispositionTest.php | 132 ++++++++++++++++++ 2 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 tests/Unit/Service/SynchronizationServiceContentDispositionTest.php diff --git a/lib/Service/SynchronizationService.php b/lib/Service/SynchronizationService.php index 2a02a549f..7b770ba35 100644 --- a/lib/Service/SynchronizationService.php +++ b/lib/Service/SynchronizationService.php @@ -9053,10 +9053,10 @@ private function getFilenameFromHeaders(array $response, ObjectEntity $result): if (isset($response['headers']['Content-Disposition']) === true && str_contains($response['headers']['Content-Disposition'][0], 'filename') === true ) { - $explodedContentDisposition = explode('=', $response['headers']['Content-Disposition'][0]); + $filename = $this->parseContentDispositionFilename($response['headers']['Content-Disposition'][0]); + } - $filename = trim(string: $explodedContentDisposition[1], characters: '"'); - } else { + if ($filename === null) { // Otherwise, parse the url and content type header. The CallLog is now // an OpenRegister ObjectEntity; the `request` body lives under // `getObject()['request']` instead of the legacy `getRequest()` getter. @@ -9081,6 +9081,97 @@ private function getFilenameFromHeaders(array $response, ObjectEntity $result): return $filename; }//end getFilenameFromHeaders() + /** + * Parse a Content-Disposition header value and extract the filename per RFC 6266. + * + * Supports both the traditional `filename="…"` parameter and the RFC 5987 + * extended `filename*=charset''pct-encoded-value` form. When both are + * present the extended form wins per RFC 6266 §4.3, with the plain + * `filename` used as fallback when `filename*` is absent or carries an + * unsupported charset. + * + * WOO-552: replaces the naive `explode('=', $header)` that corrupted + * the filename as soon as xxllnc's Zaken API started emitting both + * parameters (release 2026-08-19, temporarily rolled back, feature- + * toggled re-rollout expected). Parameter-name matching is case- + * insensitive; the plain `filename` parameter's surrounding quotes are + * stripped. + * + * @param string $headerValue The raw Content-Disposition header value. + * @return string|null The extracted filename, or null when + * neither `filename*` nor `filename` yielded + * a value. + */ + private function parseContentDispositionFilename(string $headerValue): ?string { + // Split the header into parameter segments on `;`. The first + // segment is the disposition-type (attachment / inline), the rest + // are parameters. Splitting on `;` (instead of `=`) is what the + // naive pre-WOO-552 code got wrong: any `=` inside a value (bv. + // the charset''value shape of filename*) fooled the extractor. + $segments = array_map('trim', explode(';', $headerValue)); + + $filenameStar = null; + $filenamePlain = null; + + foreach ($segments as $segment) { + // Split into name/value on the FIRST `=` only — the value side + // may legitimately contain further `=` characters (RFC 5987 + // extended values, base64-ish payloads). + $eq = strpos($segment, '='); + if ($eq === false) { + continue; + } + $name = strtolower(trim(substr($segment, 0, $eq))); + $value = trim(substr($segment, $eq + 1)); + + if ($name === 'filename*') { + $filenameStar = $this->decodeRfc5987ExtendedValue($value); + } elseif ($name === 'filename') { + // RFC 6266 allows quoted or unquoted `filename`; strip + // matching surrounding double quotes when present. + $filenamePlain = trim($value, '"'); + } + } + + // RFC 6266 §4.3: `filename*` wins when present and decodable. + return $filenameStar ?? $filenamePlain; + }//end parseContentDispositionFilename() + + /** + * Decode an RFC 5987 extended parameter value of shape + * `charset''pct-encoded`. + * + * Only UTF-8 is supported — any other charset (bv. ISO-8859-1) + * triggers a fallback by returning null, causing + * {@see parseContentDispositionFilename()} to use the plain `filename` + * parameter instead. Malformed values also return null. + * + * @param string $value The raw extended value, + * e.g. `UTF-8''na%C3%AFef.pdf`. + * @return string|null The decoded UTF-8 string, or null when + * unsupported. + */ + private function decodeRfc5987ExtendedValue(string $value): ?string { + // RFC 5987 shape: charset ' language ' value-chars + $parts = explode("'", $value, 3); + if (count($parts) !== 3) { + return null; + } + [$charset, $language, $encoded] = $parts; + unset($language); // Language tag is accepted but not used. + + if (strcasecmp($charset, 'UTF-8') !== 0) { + $this->logger->info( + 'Ignoring Content-Disposition filename* with unsupported charset; falling back to plain filename', + ['charset' => $charset] + ); + return null; + } + + // rawurldecode() implements the RFC 3986 §2.1 pct-decode. + return rawurldecode($encoded); + }//end decodeRfc5987ExtendedValue() + /** * Extracts an endpoint from the given data and optionally retrieves a filename and tags. * diff --git a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php new file mode 100644 index 000000000..32101fbc2 --- /dev/null +++ b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php @@ -0,0 +1,132 @@ +newInstanceWithoutConstructor(); + + // Populate the readonly logger property so the unsupported-charset + // fallback in decodeRfc5987ExtendedValue() can call ->info(...). + $loggerProperty = $reflection->getProperty('logger'); + $loggerProperty->setAccessible(true); + $loggerProperty->setValue($service, $this->createMock(LoggerInterface::class)); + + $method = $reflection->getMethod('parseContentDispositionFilename'); + $method->setAccessible(true); + + return $method->invoke($service, $headerValue); + } + + public function testFilenameOnlyAsciiRoundTrips(): void + { + $header = 'attachment; filename="bestand.pdf"'; + $this->assertSame('bestand.pdf', $this->invokeParser($header)); + } + + public function testFilenameStarOnlyUtf8IsDecoded(): void + { + // RFC 5987 extended value form: charset '' language '' pct-encoded. + $header = "attachment; filename*=UTF-8''bestand.pdf"; + $this->assertSame('bestand.pdf', $this->invokeParser($header)); + } + + public function testFilenameStarWinsOverPlainFilenameWhenBothPresent(): void + { + // This is the exact xxllnc post-2026-08-19 shape that broke the + // pre-WOO-552 naive `explode('=', $header)` extractor. + $header = 'attachment; filename="fallback.pdf"; filename*=UTF-8\'\'preferred.pdf'; + $this->assertSame('preferred.pdf', $this->invokeParser($header)); + } + + public function testFilenameStarWithUnicodePctEncodingDecodesToUtf8(): void + { + // "na\xC3\xAFef.pdf" pct-encoded — the diakriet-carrying case that + // motivated xxllnc to adopt filename* in the first place. + $header = "attachment; filename=\"naief.pdf\"; filename*=UTF-8''na%C3%AFef.pdf"; + $this->assertSame('naïef.pdf', $this->invokeParser($header)); + } + + public function testFilenameStarWithUnsupportedCharsetFallsBackToFilename(): void + { + // Charsets other than UTF-8 (e.g. legacy ISO-8859-1) are not + // decoded; RFC 5987 mandates support only for UTF-8, so we fall + // back to the plain `filename` parameter which is guaranteed ASCII. + $header = "attachment; filename=\"safe.pdf\"; filename*=ISO-8859-1''na%EFef.pdf"; + $this->assertSame('safe.pdf', $this->invokeParser($header)); + } + + public function testFilenameStarWithUnsupportedCharsetAndNoFilenameReturnsNull(): void + { + // Defensive: if the extended value is unusable AND there is no + // plain `filename`, the parser must return null so the caller + // knows to fall back to its URL/MIME-based path. + $header = "attachment; filename*=ISO-8859-1''na%EFef.pdf"; + $this->assertNull($this->invokeParser($header)); + } + + public function testFilenameParameterNameIsCaseInsensitive(): void + { + // RFC 6266 explicitly allows case-insensitive parameter names. + $header = 'attachment; FileName="bestand.pdf"'; + $this->assertSame('bestand.pdf', $this->invokeParser($header)); + } + + public function testFilenameStarParameterNameIsCaseInsensitive(): void + { + $header = "attachment; FILENAME*=UTF-8''bestand.pdf"; + $this->assertSame('bestand.pdf', $this->invokeParser($header)); + } + + public function testUnquotedFilenameIsAccepted(): void + { + // Token form (no surrounding quotes) is permitted by RFC 6266 + // when the filename contains no separators — real-world servers + // do emit this shape. + $header = 'attachment; filename=bestand.pdf'; + $this->assertSame('bestand.pdf', $this->invokeParser($header)); + } + + public function testHeaderWithoutAnyFilenameReturnsNull(): void + { + // `inline` disposition with no filename parameter — the parser is + // only called when the caller has already seen the substring + // "filename" in the header, but even so we assert the null path + // to guard against future refactors of the calling contract. + $header = 'inline'; + $this->assertNull($this->invokeParser($header)); + } +} From 8b119afba28e1affaea0a7e830bbdfdf2608d074 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 4 Sep 2026 11:47:57 +0200 Subject: [PATCH 2/4] =?UTF-8?q?fix(sync):=20address=20PR=20#1840=20review?= =?UTF-8?q?=20=E2=80=94=20CI=20style=20+=20quoted-string=20tokenizer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the 8 findings from the self-review on #1840: phpcs / phpmd (CI-red) — SynchronizationService.php: - Line 9056, 9128: use named arguments on internal helper calls. - Line 9120: rename `$eq` → `$eqPos` (phpmd ShortVariable, min 3). - Line 9171: capitalize inline comment ("Decode via rawurldecode()…"). Parser correctness (RFC 6266 §4 quoted-string grammar): - Replace `explode(';', $header)` with a quoted-string-aware tokenizer `splitHeaderParameters()` so a `;` inside `filename="…"` no longer corrupts the value. `filename="foo;bar.pdf"` now returns `foo;bar.pdf` (was `foo`). Test file (SynchronizationServiceContentDispositionTest.php): - Fix namespace: `OCA\OpenConnector\Tests\...` → `OCA\Integriq\Tests\...` to match composer autoload-dev psr-4 rule (was skipped by composer's autoloader with a warning; every other test in tests/Unit/Service/ uses the Integriq namespace). - Add 3 tests: - `testFilenameWithSemicolonInsideQuotedValuePreservesFilename` - `testFilenameWithPathTraversalPayloadIsReturnedVerbatim` (parser contract test — sanitization is downstream's responsibility) - `testFilenameWithWhitespaceAroundEqualsIsAccepted` Refs: WOO-552, WOO-553 Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/Service/SynchronizationService.php | 65 +++++++++++++++---- ...onizationServiceContentDispositionTest.php | 32 ++++++++- 2 files changed, 83 insertions(+), 14 deletions(-) diff --git a/lib/Service/SynchronizationService.php b/lib/Service/SynchronizationService.php index 7b770ba35..e430047cc 100644 --- a/lib/Service/SynchronizationService.php +++ b/lib/Service/SynchronizationService.php @@ -9053,7 +9053,7 @@ private function getFilenameFromHeaders(array $response, ObjectEntity $result): if (isset($response['headers']['Content-Disposition']) === true && str_contains($response['headers']['Content-Disposition'][0], 'filename') === true ) { - $filename = $this->parseContentDispositionFilename($response['headers']['Content-Disposition'][0]); + $filename = $this->parseContentDispositionFilename(headerValue: $response['headers']['Content-Disposition'][0]); } if ($filename === null) { @@ -9103,12 +9103,13 @@ private function getFilenameFromHeaders(array $response, ObjectEntity $result): * a value. */ private function parseContentDispositionFilename(string $headerValue): ?string { - // Split the header into parameter segments on `;`. The first - // segment is the disposition-type (attachment / inline), the rest - // are parameters. Splitting on `;` (instead of `=`) is what the - // naive pre-WOO-552 code got wrong: any `=` inside a value (bv. - // the charset''value shape of filename*) fooled the extractor. - $segments = array_map('trim', explode(';', $headerValue)); + // Split the header into parameter segments on `;`, respecting quoted + // strings so a `;` inside `filename="..."` is treated as part of the + // value (RFC 6266 §4 quoted-string grammar). Splitting on `;` + // (instead of `=`) is what the naive pre-WOO-552 code got wrong: + // any `=` inside a value (bv. the charset''value shape of + // filename*) fooled the extractor. + $segments = $this->splitHeaderParameters(headerValue: $headerValue); $filenameStar = null; $filenamePlain = null; @@ -9117,15 +9118,15 @@ private function parseContentDispositionFilename(string $headerValue): ?string { // Split into name/value on the FIRST `=` only — the value side // may legitimately contain further `=` characters (RFC 5987 // extended values, base64-ish payloads). - $eq = strpos($segment, '='); - if ($eq === false) { + $eqPos = strpos($segment, '='); + if ($eqPos === false) { continue; } - $name = strtolower(trim(substr($segment, 0, $eq))); - $value = trim(substr($segment, $eq + 1)); + $name = strtolower(trim(substr($segment, 0, $eqPos))); + $value = trim(substr($segment, $eqPos + 1)); if ($name === 'filename*') { - $filenameStar = $this->decodeRfc5987ExtendedValue($value); + $filenameStar = $this->decodeRfc5987ExtendedValue(value: $value); } elseif ($name === 'filename') { // RFC 6266 allows quoted or unquoted `filename`; strip // matching surrounding double quotes when present. @@ -9137,6 +9138,44 @@ private function parseContentDispositionFilename(string $headerValue): ?string { return $filenameStar ?? $filenamePlain; }//end parseContentDispositionFilename() + /** + * Split a Content-Disposition header on `;` while preserving semicolons + * that appear INSIDE a quoted-string parameter value. + * + * RFC 6266 §4 uses the HTTP quoted-string grammar for `filename="..."`, + * so a `;` between quotes is part of the value, not a parameter + * separator. A naive `explode(';', $header)` corrupts values like + * `filename="foo;bar.pdf"` to just `foo`. + * + * @param string $headerValue The raw Content-Disposition header value. + * @return array Trimmed parameter segments, starting with + * the disposition-type (attachment/inline). + */ + private function splitHeaderParameters(string $headerValue): array { + $segments = []; + $current = ''; + $inQuotes = false; + $length = strlen($headerValue); + for ($i = 0; $i < $length; $i++) { + $char = $headerValue[$i]; + if ($char === '"') { + $inQuotes = ($inQuotes === false); + $current .= $char; + continue; + } + if ($char === ';' && $inQuotes === false) { + $segments[] = trim($current); + $current = ''; + continue; + } + $current .= $char; + } + if ($current !== '') { + $segments[] = trim($current); + } + return $segments; + }//end splitHeaderParameters() + /** * Decode an RFC 5987 extended parameter value of shape * `charset''pct-encoded`. @@ -9168,7 +9207,7 @@ private function decodeRfc5987ExtendedValue(string $value): ?string { return null; } - // rawurldecode() implements the RFC 3986 §2.1 pct-decode. + // Decode via rawurldecode() — implements the RFC 3986 §2.1 pct-decode. return rawurldecode($encoded); }//end decodeRfc5987ExtendedValue() diff --git a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php index 32101fbc2..8144235f1 100644 --- a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php +++ b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php @@ -1,6 +1,6 @@ assertNull($this->invokeParser($header)); } + + public function testFilenameWithSemicolonInsideQuotedValuePreservesFilename(): void + { + // RFC 6266 §4 quoted-string grammar: a `;` between quotes is part + // of the value, not a parameter separator. A naive + // `explode(';', $header)` corrupts this to just `foo`; the + // quoted-string-aware tokenizer preserves the full filename. + $header = 'attachment; filename="foo;bar.pdf"'; + $this->assertSame('foo;bar.pdf', $this->invokeParser($header)); + } + + public function testFilenameWithPathTraversalPayloadIsReturnedVerbatim(): void + { + // Contract: the parser extracts the filename as declared upstream; + // path-separator / `..` sanitization is the responsibility of the + // downstream FileService::saveFile() writer. Locking this contract + // guards against a future refactor silently sanitizing here (which + // would hide malicious input from the writer's audit surface). + $header = 'attachment; filename="../../etc/passwd"'; + $this->assertSame('../../etc/passwd', $this->invokeParser($header)); + } + + public function testFilenameWithWhitespaceAroundEqualsIsAccepted(): void + { + // Well-behaved servers do not emit whitespace around `=`, but the + // tokenizer's `trim()` handles it gracefully. Locks the behaviour + // so a future refactor does not silently regress it. + $header = 'attachment; filename = "bestand.pdf"'; + $this->assertSame('bestand.pdf', $this->invokeParser($header)); + } } From 205a54e644ffa9e5b2ff1593559b1f2431179e35 Mon Sep 17 00:00:00 2001 From: Wilco Louwerse Date: Fri, 4 Sep 2026 13:17:55 +0200 Subject: [PATCH 3/4] =?UTF-8?q?test(sync):=20lock=20Barry's=20rapport-vers?= =?UTF-8?q?ie=20example=20as=20explicit=20test=20=E2=80=94=20WOO-553?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `testFilenameWithSemicolonAndSpaceInsideQuotedValuePreservesFilename` covering Barry's exact string from his PR #1840 review: attachment; filename="rapport; versie 2.pdf" The existing `testFilenameWithSemicolonInsideQuotedValuePreservesFilename` already exercises the same tokenizer path via `foo;bar.pdf`, but the new test locks the concrete example he raised so the guarantee is explicit in the suite, not only implied. Also mirrored to the WOO stable hotfix branch alongside the tokenizer backport (WOO-552 hotfix cut of v0.2.24-woo-2). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../SynchronizationServiceContentDispositionTest.php | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php index 8144235f1..b2791f7ab 100644 --- a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php +++ b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php @@ -140,6 +140,15 @@ public function testFilenameWithSemicolonInsideQuotedValuePreservesFilename(): v $this->assertSame('foo;bar.pdf', $this->invokeParser($header)); } + public function testFilenameWithSemicolonAndSpaceInsideQuotedValuePreservesFilename(): void + { + // Barry's concrete example on PR #1840. Locks the exact string he + // raised so the guarantee is explicit in the test suite, not only + // implied by the more abstract `foo;bar.pdf` case above. + $header = 'attachment; filename="rapport; versie 2.pdf"'; + $this->assertSame('rapport; versie 2.pdf', $this->invokeParser($header)); + } + public function testFilenameWithPathTraversalPayloadIsReturnedVerbatim(): void { // Contract: the parser extracts the filename as declared upstream; From 4b520ef5fbe418310ad676acfda57a79d9d2a6a4 Mon Sep 17 00:00:00 2001 From: WilcoLouwerse Date: Fri, 4 Sep 2026 13:46:47 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(sync):=20correct=20SynchronizationServi?= =?UTF-8?q?ce=20namespace=20in=20test=20=E2=80=94=20WOO-553?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test imported OCA\OpenConnector\Service\SynchronizationService, but integriq's class lives under OCA\Integriq\Service\SynchronizationService (matching every other test in this directory). ReflectionClass failed on the missing class, producing 14 PHPUnit errors across every PHP/NC matrix cell and cascading into the Quality Report failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../Service/SynchronizationServiceContentDispositionTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php index b2791f7ab..bbbba0bc8 100644 --- a/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php +++ b/tests/Unit/Service/SynchronizationServiceContentDispositionTest.php @@ -2,7 +2,7 @@ namespace OCA\Integriq\Tests\Unit\Service; -use OCA\OpenConnector\Service\SynchronizationService; +use OCA\Integriq\Service\SynchronizationService; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; use ReflectionClass; @@ -22,7 +22,7 @@ * d) `filename*` Unicode pct-decode — diakriet round-trips correctly. * e) `filename*` unsupported charset — falls back to plain `filename`. * - * @package OCA\OpenConnector\Tests\Unit\Service + * @package OCA\Integriq\Tests\Unit\Service */ class SynchronizationServiceContentDispositionTest extends TestCase {