Skip to content

Fix render-state leak and inconsistent extract path handling #146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dereuromark wants to merge 1 commit into from
Closed

Fix render-state leak and inconsistent extract path handling #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 71 additions & 35 deletions src/View/CsvView.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Cake\Datasource\EntityInterface;
use Cake\Utility\Hash;
use Cake\View\SerializedView;
use Stringable;

/**
* A view class that is used for CSV responses.
Expand Down Expand Up @@ -77,11 +78,18 @@ class CsvView extends SerializedView
protected string $subDir = 'csv';

/**
* Whether or not to reset static variables in use
* Aggregated CSV output for the current serialization pass.
*
* @var bool
* @var string
*/
protected string $csv = '';

/**
* Temp stream used by fputcsv() to generate a single row.
*
* @var resource|null
*/
protected bool $_resetStaticVariables = false;
protected $fp = null;

/**
* Iconv extension.
Expand Down Expand Up @@ -201,16 +209,43 @@ public static function contentType(): string
*/
protected function _serialize(array|string $serialize): string
{
$this->resetState();

$this->_renderRow($this->getConfig('header'));
$this->_renderContent();
$this->_renderRow($this->getConfig('footer'));
$content = $this->_renderRow();
$this->_resetStaticVariables = true;
$this->_renderRow();
$content = $this->csv;

$this->resetState();

return $content;
}

/**
* Reset accumulated state so the same view instance can render multiple
* times in a single request (queue worker, multi-file export, etc.).
*/
protected function resetState(): void
{
$this->csv = '';
$this->isFirstBom = true;
if (is_resource($this->fp)) {
fclose($this->fp);
}
$this->fp = null;
}

/**
* @inheritDoc
*/
public function __destruct()
{
if (is_resource($this->fp)) {
fclose($this->fp);
$this->fp = null;
}
}

/**
* Renders the body of the data to the csv
*
Expand Down Expand Up @@ -245,24 +280,35 @@ protected function _renderContent(): void
foreach ($extract as $formatter) {
if (!is_string($formatter) && is_callable($formatter)) {
$value = $formatter($_data);
$pathForError = '<callable>';
} else {
$path = $formatter;
$format = null;
if (is_array($formatter)) {
[$path, $format] = $formatter;
}
$pathForError = (string)$path;

if (!str_contains($path, '.')) {
$value = $_data[$path];
} else {
$value = Hash::get($_data, $path);
}
$value = Hash::get($_data, $path);

if ($format) {
if ($format !== null) {
$value = sprintf($format, $value);
}
}

if (
$value !== null
&& !is_scalar($value)
&& !($value instanceof Stringable)
) {
throw new CakeException(sprintf(
'Extract path `%s` resolved to a non-scalar `%s`. '
. 'Use a callable formatter to flatten it, or adjust the extract path.',
$pathForError,
get_debug_type($value),
));
}

$values[] = $value;
}
$this->_renderRow($values);
Expand All @@ -273,54 +319,44 @@ protected function _renderContent(): void
/**
* Aggregates the rows into a single csv
*
* @param array<string>|null $row Row data
* @param array<scalar|\Stringable|null>|null $row Row data
* @return string CSV with all data to date
*/
protected function _renderRow(?array $row = null): string
{
static $csv = '';
$this->csv .= (string)$this->_generateRow($row);

if ($this->_resetStaticVariables) {
$csv = '';
$this->_resetStaticVariables = false;

return '';
}

$csv .= (string)$this->_generateRow($row);

return $csv;
return $this->csv;
}

/**
* Generates a single row in a csv from an array of
* data by writing the array to a temporary file and
* returning its contents
*
* @param array<string|null>|null $row Row data
* @param array<scalar|\Stringable|null>|null $row Row data
* @return string|false String with the row in csv-syntax, false on fputscv failure
*/
protected function _generateRow(?array $row = null): string|false
{
static $fp = false;

if (!$row) {
return '';
}

if ($fp === false) {
if ($this->fp === null) {
$stream = 'php://temp';
$fp = fopen($stream, 'r+');
if ($fp === false) {
throw new CakeException(sprintf('Cannot open stream `%s`', $stream));
}
$this->fp = $fp;

$setSeparator = $this->getConfig('setSeparator');
if ($setSeparator) {
fwrite($fp, 'sep=' . $setSeparator . "\n");
fwrite($this->fp, 'sep=' . $setSeparator . "\n");
}
} else {
ftruncate($fp, 0);
ftruncate($this->fp, 0);
}

$null = $this->getConfig('null');
Expand All @@ -340,21 +376,21 @@ protected function _generateRow(?array $row = null): string|false
/** @phpstan-ignore-next-line */
$row = str_replace(["\r\n", "\n", "\r"], $newline, $row);
if ($enclosure === '') {
// fputcsv does not supports empty enclosure
if (fputs($fp, implode($delimiter, $row) . "\n") === false) {
// fputcsv does not support empty enclosure
if (fputs($this->fp, implode($delimiter, $row) . "\n") === false) {
return false;
}
} else {
if (fputcsv($fp, $row, $delimiter, $enclosure, $escape) === false) {
if (fputcsv($this->fp, $row, $delimiter, $enclosure, $escape) === false) {
return false;
}
}

rewind($fp);
rewind($this->fp);
unset($row);

$csv = '';
while (($buffer = fgets($fp, 4096)) !== false) {
while (($buffer = fgets($this->fp, 4096)) !== false) {
$csv .= $buffer;
}

Expand Down
78 changes: 78 additions & 0 deletions tests/TestCase/View/CsvViewTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

namespace CsvView\Test\TestCase\View;

use Cake\Core\Exception\CakeException;
use Cake\Http\Response;
use Cake\Http\ServerRequest as Request;
use Cake\I18n\DateTime;
Expand Down Expand Up @@ -519,4 +520,81 @@ public function testInvalidViewVarThrowsException()
$this->view->setConfig('serialize', 'data');
$this->view->render();
}

/**
* Rendering the same instance twice must produce clean output both times
* (no stale BOM state, no leftover writer state).
*
* @return void
*/
public function testRenderTwiceWithSameInstance()
{
$data = [['a', 'b'], ['c', 'd']];
$this->view->set(['data' => $data])
->setConfig(['serialize' => 'data', 'bom' => true, 'csvEncoding' => 'UTF-8']);

$bom = chr(0xEF) . chr(0xBB) . chr(0xBF);
$expected = $bom . 'a,b' . PHP_EOL . 'c,d' . PHP_EOL;

$this->assertSame($expected, $this->view->render());
$this->assertSame($expected, $this->view->render());
}

/**
* Simple (non-dotted) extract paths must fall through Hash::get() so a
* missing key resolves to null instead of triggering an undefined-key
* warning.
*
* @return void
*/
public function testRenderViaExtractMissingSimpleKey()
{
$data = [
['name' => 'alice', 'email' => 'a@example.com'],
['name' => 'bob'], // missing 'email'
];
$this->view->set(['users' => $data])
->setConfig([
'serialize' => 'users',
'extract' => ['name', 'email'],
]);

$expected = 'alice,a@example.com' . PHP_EOL . 'bob,' . PHP_EOL;
$this->assertSame($expected, $this->view->render());
}

/**
* An extract path that resolves to an array (e.g. a hasMany association)
* must throw a clear exception instead of silently producing "Array to
* string conversion" notices and corrupted CSV. Regression for #131.
*
* @return void
*/
public function testRenderViaExtractArrayValueThrows()
{
$data = [
[
'id' => 1,
'tags' => [['name' => 'php'], ['name' => 'cakephp']],
],
];
$this->view->set(['rows' => $data])
->setConfig([
'serialize' => 'rows',
'extract' => ['id', 'tags'],
]);

try {
$this->view->render();
$this->fail('Expected exception for array-valued extract path.');
} catch (Exception $e) {
// SerializedView wraps our CakeException in SerializationFailureException.
$previous = $e->getPrevious() ?? $e;
$this->assertInstanceOf(CakeException::class, $previous);
$this->assertStringContainsString(
'Extract path `tags` resolved to a non-scalar `array`',
$previous->getMessage(),
);
}
}
}
Loading