[8.0] Only escape callable output of add and edit column. #1852

sharifzadesina · 2018-09-22T11:03:23Z

Fixes #1849
P.S. bro I think you need to rewrite whole package again, you use nesting extremely.

yajra · 2018-09-25T00:43:26Z

Thanks, can you please fix the failing tests?

you use nesting extremely

Yeah, will try to refactor more when I got the chance. :)

-- Edit --
I just recalled, I think the nesting was due to support for eager loading.

sharifzadesina · 2018-09-25T09:41:15Z

@yajra Done.
About "extremely nesting" I mean, "nesting code extremely" in the whole of the project.
for example this function here.
you can combine it with escapeRow and make code cleaner.

yajra · 2018-09-29T03:19:32Z

OIC, thanks for pointing that out. It seems the tests are still failing though.

sharifzadesina · 2018-09-29T08:11:07Z

@yajra I doubt if they are cause of my code.

yajra · 2018-10-05T02:24:57Z

@sharifzadesina ok thanks, will to review this later.

yajra · 2018-10-05T05:46:17Z

Just tested this with my app and it breaks my DT with the same error on the tests. Please fix. Thanks!

exception: "Symfony\Component\Debug\Exception\FatalThrowableError"
file: "/www/yajra/dataTables/core/src/Processors/DataProcessor.php"
line: 241
message: "Argument 1 passed to Yajra\DataTables\Processors\DataProcessor::escapeRow() must be of the type array, integer given, called in /www/yajra/dataTables/core/src/Processors/DataProcessor.php on line 223"

sharifzadesina · 2018-10-12T20:19:18Z

@yajra Bro I fixed the error, and improved the package a lot.
please test the PR and merge it.
this will make the life for developers easier.
also I hope you will work on the package and improve other parts too.
thank you

yajra · 2018-10-17T07:17:47Z

@sharifzadesina thanks but it's failing on the indexColumn. I think it should be the last item on row manipulations as per the original code before PR.

yajra · 2018-10-17T07:19:42Z

src/Processors/DataProcessor.php

-
+            $data           = $this->escapeRow(Helper::convertToArray($row));
+            $value          = $this->addColumns($data, $row);
+            $value          = $this->addIndexColumn($value, $row);


I think this should be the last item in row processing or maybe second to the last then escape the row.

sharifzadesina · 2018-10-17T18:50:00Z

@yajra Fixed, but it was cause of an other error actually.

yajra · 2018-10-18T03:02:38Z

The changes looks good. However, there are some columns on my project that should be raw but are being escaped. It's not blade but a closure that returns a view.

->rawColumns(['filename_html', 'deleted_html', 'cooked_html']);

Need to review this further. I think we are missing some tests on raw columns.

sharifzadesina · 2018-10-18T06:08:32Z

@yajra This should be fixed now.

yajra · 2018-10-18T06:58:50Z

Still not working and I think I found where the issue was:

escapeRow should be last one to be called since added & edited columns are not yet processed.

        foreach ($this->results as $row) {
            $data  = Helper::convertToArray($row);
            $value = $this->addColumns($data, $row);
            $value = $this->editColumns($value, $row);
            $value = $this->setupRowVariables($value, $row);
            $value = $this->selectOnlyNeededColumns($value);
            $value = $this->removeExcessColumns($value);
            $value = $this->addIndexColumn($value);
            $value = $this->escapeRow($value);

            $this->output[] = $object ? $value : $this->flatten($value);
        }

Since our goal is to not escape blade string from add/edit column, we can modify our checker to something like:

    protected function shouldEscapeColumn($key)
    {
        foreach ($this->appendColumns as $column) {
            if ($column['name'] == $key && is_string($column['content'])) {
                return false;
            }
        }

        foreach ($this->editColumns as $column) {
            if ($column['name'] == $key && is_string($column['content'])) {
                return false;
            }
        }

        if ($this->escapeColumns === '*') {
            return ! in_array($key, $this->rawColumns); // escape if is not a raw column
        } elseif (is_array($this->escapeColumns)) {
            return in_array($key, array_diff($this->escapeColumns, $this->rawColumns));
        } else {
            return true;
        }
    }

yajra · 2018-10-18T06:59:25Z

src/Processors/DataProcessor.php

-                $value[$indexColumn] = ++$this->start;
-            }
-
+            $data           = $this->escapeRow(Helper::convertToArray($row));


Should be the last process in order to capture added / edited data.

yajra · 2018-10-18T07:00:27Z

src/Processors/DataProcessor.php

@@ -125,13 +126,28 @@ public function process($object = false)
    protected function addColumns($data, $row)
    {
        foreach ($this->appendColumns as $key => $value) {
-            $value['content'] = Helper::compileContent($value['content'], $data, $row);
+            $value['content'] = Helper::compileContent($value['content'], $data, $row, $this->shouldEscapeColumn($key));


I think the escape flag here is not needed since escaping will be done on escapeRow process.

yajra · 2018-10-18T07:00:48Z

src/Processors/DataProcessor.php

@@ -142,7 +158,7 @@ protected function addColumns($data, $row)
    protected function editColumns($data, $row)
    {
        foreach ($this->editColumns as $key => $value) {
-            $value['content'] = Helper::compileContent($value['content'], $data, $row);
+            $value['content'] = Helper::compileContent($value['content'], $data, $row, $this->shouldEscapeColumn($key));


Not needed like on addColumns.

yajra · 2018-10-18T07:01:27Z

src/Utilities/Helper.php

    {
        if (is_string($content)) {
            return static::compileBlade($content, static::getMixedValue($data, $param));
        } elseif (is_callable($content)) {
-            return $content($param);
+            return $escape ? e($content($param)) : $content($param);


Should be reverted back to original since all data here is callable.

yajra · 2018-10-18T07:02:16Z

src/Processors/DataProcessor.php

+     */
+    protected function shouldEscapeColumn($key)
+    {
+        if ($this->escapeColumns === '*') {


We can add the logic here to figure out if added/edited columns are to be escaped.

foreach ($this->appendColumns as $column) { if ($column['name'] == $key && is_string($column['content'])) { return false; } } foreach ($this->editColumns as $column) { if ($column['name'] == $key && is_string($column['content'])) { return false; } }

sharifzadesina · 2018-10-18T07:05:45Z

@yajra we are not doing escaping at "escapeRow", cause we don't know the content was a blade string or a callable, we need to escape output of callables, that's why we only escape database data using escapeRow.

yajra · 2018-10-18T07:08:52Z

@sharifzadesina we can identify the content by checking the append columns structure:

foreach ($this->appendColumns as $column) {
            if ($column['name'] == $key && is_string($column['content'])) {
                return false;
            }
        }

that's why we only escape database data

Good point on this one.

yajra · 2018-10-18T07:11:28Z

But I think we should still escape added/edited columns since we can still directly return a database value from a closure.

sharifzadesina · 2018-10-18T07:12:21Z

@yajra
Imagine I've added this callable using addColumn:

function () {
    return '<b>XSS</b>';
}

The content will be string so we won't escape it. this should not happen.
Can you please send your full code? I think it should work with the changes I made.

yajra · 2018-10-18T07:17:49Z

For closure like that, I would still prefer to be explicit and add it on the rawColumns.

For a straight blade string, It's ok to be raw.

addColumn('xss', function ($model) {
    return '<b>XSS</b>'; // should be added to rawColumns
})

addColumn('xss', function ($model) {
    return $model->xss; // should be added to rawColumns
})

addColumn('xss', 'path.to.view'); // can bypass escape
addColumn('xss', '<a href="#">{{ $xss }}</a>'); // can bypass escape

yajra · 2018-10-18T07:20:26Z

See this gist for my debugged processor. It's not yet optimized though but my project does not seem to break.

sharifzadesina · 2018-10-18T07:23:25Z

@yajra The problem is if you add it to rawColumns it won't escape and the XSS bug will happen.
We want to escape callables by default so if they wanted to not escape callables they can add it to rawColumns.
Can you send your script?

yajra · 2018-10-18T07:31:57Z

Already created the gist.

Yes, we should by default escape all rows for xss protection unless explicitly set as raw column.That is why I agreed of allowing blade templates to be raw since the argument is we are using {{ }} on our template. It's a different case for closure though.

sharifzadesina · 2018-10-18T07:32:49Z

@yajra no I mean the script you use to test my PR

yajra · 2018-10-18T07:36:25Z

It's just a standard dataTable with multiple addColumns. Anyways, I added it on the gist.

sharifzadesina · 2018-10-19T09:21:14Z

@yajra Bro I tested the PR on my own apps, it works without problem, are you sure you are using whole pull request?
cause there were also some changes at Helper.php file.

sharifzadesina · 2018-11-06T14:25:48Z

@yajra Any opinion on this? Why we don't merge it?

yajra · 2018-11-07T00:51:49Z

It's still failing the last time I checked. For now I think the safest implementation would be:

Allow raw columns on the following condition:

String template

->addColumn('extra', 'some string')
->editColumn('extra', 'some string')

Blade template

->addColumn('extra', 'path.to.blade')
->editColumn('extra', 'path.to.blade')

Do not allow raw columns on closure:

->addColumn('extra', function($model) {
   return 'html';
})
->rawColumns(['extra'])

sharifzadesina · 2018-11-07T11:56:53Z

@yajra It is exactly like that, that's why I told "this is not a BC".

yajra · 2018-11-08T06:23:23Z

Ok, will try to do an actual test again when I got the chance.

yajra · 2018-11-23T03:14:45Z

It seems to work well now but it breaks the index column. Reverting, please resubmit the PR and fix the index column part. Thanks!

yajra · 2018-11-23T03:15:57Z

Or you can alternatively send another PR to fix the index column? Will hold the revert and release of this feature until it's fixed. Thanks!

Introduced on #1852.

yajra · 2018-11-23T03:45:17Z

Fixed the index column and added some tests. Released on v8.13.0 🚀

If you can, please also submit a PR on docs. Thanks!

yajra · 2018-11-23T07:17:59Z

Resume discussion on #1916.

Only escape callable output

55a8cfb

Bug fix

d4bf989

yajra added the Hacktoberfest label Oct 5, 2018

Sina added 2 commits October 12, 2018 23:30

Trying to fix failed test

feb034a

Fix CS, Fix Bug

b1e98d5

Merge branch '8.0' into 8.0

194255a

yajra requested changes Oct 17, 2018

View reviewed changes

sharifzadesina added 2 commits October 17, 2018 22:06

Move index column to last

9a2bca4

Removes the raw param

416d64b

sharifzadesina added 3 commits October 17, 2018 22:26

Fix bug

75349dc

Improve

c13af4b

Use old index column name

93ed4e3

yajra approved these changes Oct 18, 2018

View reviewed changes

Fix escape operation

afc4b4e

yajra requested changes Oct 18, 2018

View reviewed changes

Merge branch '8.0' into 8.0

530f264

yajra approved these changes Nov 23, 2018

View reviewed changes

yajra changed the title ~~Only escape callable output~~ [8.0] Only escape callable output of add and edit column. Nov 23, 2018

yajra merged commit 040eef2 into yajra:8.0 Nov 23, 2018

yajra mentioned this pull request Nov 23, 2018

Revert "[8.0] Only escape callable output of add and edit column." #1914

Closed

yajra added a commit that referenced this pull request Nov 23, 2018

Fix adding of index column.

3a34cf7

Introduced on #1852.

yajra mentioned this pull request Nov 23, 2018

Fix adding of index column. #1915

Merged

yajra added a commit that referenced this pull request Nov 23, 2018

Add tests for #1852.

6018db3

Repository owner locked as resolved and limited conversation to collaborators Nov 23, 2018

[8.0] Only escape callable output of add and edit column. #1852

[8.0] Only escape callable output of add and edit column. #1852

Conversation

sharifzadesina commented Sep 22, 2018

yajra commented Sep 25, 2018 • edited Loading

sharifzadesina commented Sep 25, 2018 • edited Loading

yajra commented Sep 29, 2018

sharifzadesina commented Sep 29, 2018

yajra commented Oct 5, 2018

yajra commented Oct 5, 2018

sharifzadesina commented Oct 12, 2018

yajra commented Oct 17, 2018

yajra Oct 17, 2018

Choose a reason for hiding this comment

sharifzadesina commented Oct 17, 2018 • edited Loading

yajra commented Oct 18, 2018

sharifzadesina commented Oct 18, 2018

yajra commented Oct 18, 2018

yajra Oct 18, 2018

Choose a reason for hiding this comment

yajra Oct 18, 2018

Choose a reason for hiding this comment

yajra Oct 18, 2018

Choose a reason for hiding this comment

yajra Oct 18, 2018

Choose a reason for hiding this comment

yajra Oct 18, 2018

Choose a reason for hiding this comment

sharifzadesina commented Oct 18, 2018

yajra commented Oct 18, 2018

yajra commented Oct 18, 2018

sharifzadesina commented Oct 18, 2018 • edited Loading

yajra commented Oct 18, 2018 • edited Loading

yajra commented Oct 18, 2018

sharifzadesina commented Oct 18, 2018 • edited Loading

yajra commented Oct 18, 2018

sharifzadesina commented Oct 18, 2018

yajra commented Oct 18, 2018

sharifzadesina commented Oct 19, 2018

sharifzadesina commented Nov 6, 2018

yajra commented Nov 7, 2018

sharifzadesina commented Nov 7, 2018

yajra commented Nov 8, 2018

yajra commented Nov 23, 2018

yajra commented Nov 23, 2018

yajra commented Nov 23, 2018

yajra commented Nov 23, 2018

yajra commented Sep 25, 2018 •

edited

Loading

sharifzadesina commented Sep 25, 2018 •

edited

Loading

sharifzadesina commented Oct 17, 2018 •

edited

Loading

sharifzadesina commented Oct 18, 2018 •

edited

Loading

yajra commented Oct 18, 2018 •

edited

Loading

sharifzadesina commented Oct 18, 2018 •

edited

Loading