Make the Chart Name field html 'clean'. #239

jpwhite4 · 2017-09-14T02:05:32Z

The chart name field could have been used to inject html into the page. This
pull request addresses this issue.

Notes:

The 'New Chart' dialog already htmlEncoded the chart name so any charts that we created
using this mechanism using the old code will be displayed exactly the same (except of course the
display bug in the chart menu is fixed).

The code change on the server will prevent any existing saved charts with embedded html
from being executed. These charts will obviously have the name appear different in this
new code, by design!

Testing details

1a) Create a chart name with injected img and script tags using old code
2a) update to new code and confirm that the injected script is not run and the raw html gets displayed instead.

1b) Try to use the Name text box to enter html code. Confirm that the text entered is displayed verbatim in

The text box itself
The chart button on the top right
The tooltip for the chart button on the top right
The "Load Chart" grid entry
The tooltip for the "Load Chart" grid entry

The chart name field could have been used to inject html into the page. This pull request addresses this issue. Notes: The 'New Chart' dialog already htmlEncoded the chart name so any charts that we created using this mechanism using the old code will be displayed exactly the same (except of course the display bug in the chart menu is fixed). The code change on the server will prevent any existing saved charts with embedded html from being executed. These charts will obviously have the name appear different in this new code, by design!

jpwhite4 · 2017-09-14T02:06:14Z

Note that this is my attempt to fix the issue described in #233

jpwhite4 · 2017-09-14T02:58:17Z

You may ask why I didn't use ENT_HTML5 | ENT_QUOTES as the flags to htmlspecialchars. The reason is that the ENT_HTML5 flag is not in php5.3.3 and the ENT_QUOTES causes the single quote to be replaced, but the ExtJs htmlEncode / htmlDecode functions do not translate the single quote char. The non-translation of the single quote does not appear to be an issue since the data is never inserted into unquoted dom attributes.

smgallo · 2017-09-14T03:00:32Z

In this case, would it make sense to also apply a patch to 7.1 (which supports PHP 5.4) to use ENT_HTML5 | ENT_QUOTES?

ryanrath · 2017-09-14T12:55:14Z

I see that you've updated getQueries and getQueryById. What are your thoughts on updating createQuery and updateQueryById.

ryanrath · 2017-09-14T13:03:56Z

Not strictly related to 'Chart Name' but per our discussion yesterday, were you able to test whether or not the Report Manager can properly handle a Chart Title with html content?

ryanrath

I believe there may be additional changes in MetricExplorer.js:createQueryHandler required.

ryanrath · 2017-09-14T12:52:46Z

classes/NewRest/Controllers/MetricExplorerControllerProvider.php

@@ -120,6 +120,7 @@ public function getQueries(Request $request, Application $app)

                foreach ($data as &$query) {
                    $this->removeRoleFromQuery($user, $query);
+                    $query['name'] = htmlspecialchars($query['name'], ENT_COMPAT, 'UTF-8', false);


Quick question, was there a problem when including the default ENT_HTML401 flag?

ryanrath · 2017-09-14T13:06:03Z

classes/NewRest/Controllers/MetricExplorerControllerProvider.php

@@ -171,6 +172,7 @@ public function getQueryById(Request $request, Application $app, $id)

                if (isset($query)) {
                    $payload['data'] = $query;
+                    $payload['data']['name'] = htmlspecialchars($query['name'], ENT_COMPAT, 'UTF-8', false);


Same as the one above re: ENT_HTML401.

ryanrath · 2017-09-14T13:09:47Z

Well that review makes no sense ( structure wise )... My additional comments to follow

ryanrath · 2017-09-14T13:18:00Z

Have you tested whether or not a long ( > 73 characters ) name w/ embedded html has it's entry in the 'Load Chart' grid display it's tooltip as expected?

plessbd · 2017-09-14T13:31:33Z

If you could at least run the automated UI tests, that should at least cover some of the questions asked by @ryanrath specifically the long title (https://github.com/ubccr/xdmod/blob/xdmod7.1/open_xdmod/modules/xdmod/automated_tests/test/specs/xdmod/metricExplorer.js#L412-L423)

ryanrath · 2017-09-14T13:41:14Z

@plessbd This test would need to be updated to include some raw html. The fragment that I've been using is <img src='x' onerror'alert(1)'> but I would imagine there are others.

plessbd · 2017-09-14T13:47:11Z

@ryanrath good point
@jpwhite4 add automated tests :)

ryanrath · 2017-09-14T13:47:34Z

Full disclosure, the reason for my comments is that I actually worked on answering these questions yesterday and held off on submitting the PR because I had to run before I could add new tests. I wanted to make sure that everything was in place before submitting. Honestly, In the grand scheme of things don't care who puts the PR in so long as the identified issues are resolved.

jpwhite4 · 2017-09-14T14:47:42Z

Re the flags: The ENT_HTML5 flag doesn't make any difference if you are not using ENT_QUOTES and I think we don't need to use ENT_QUOTES for this use case. We may, however need to use it in other places, and, if so, we will need to make sure the single quote character is converted appropriately for display in text boxes (the Ext.util.Format.htmlDecode does not support either the single quote html5 or single quote html4.01 encoding).

jpwhite4 · 2017-09-14T14:49:17Z

No need to update createQuery and updateQueryById since we always ensure correct conversion on output. See e.e. https://security.stackexchange.com/questions/95325/input-sanitization-vs-output-sanitization

jpwhite4 · 2017-09-14T14:49:33Z

Have you tested whether or not a long ( > 73 characters ) name w/ embedded html has it's entry in the 'Load Chart' grid display it's tooltip as expected?

Yes

jpwhite4 · 2017-09-14T14:50:22Z

Quick question, was there a problem when including the default ENT_HTML401 flag?

It is not needed as it is the default. In any case it does nothing unless you are using ENT_QUOTES which we are not.

ryanrath · 2017-09-14T15:17:31Z

Maybe you can help me out as I'm sure I'm just missing something. But when I use this branch as the basis for a docker rpm build ( like shippable ), login as 'admin', navigate to Metric Explorer, Create a new chart with a title of asdfkja;sldkfjas;ldkfja;sdlkfja;slkdfja;slkdfja;lskdfja;lskdfja;slkfja;lskdfjas;lkdfjas;lkdfj <img src='x' onerror='alert(1)'>, Click the 'Load Charts' button and hover over the newly created chart entry I get a browser alert with the contents of 1. If I save it and then hover over the entry the alert still occurs, and if I force refresh, and hover over the chart entry I still get an alert.

jpwhite4 · 2017-09-14T15:33:13Z

Are you sure you installed this code? When I try that on my system I do not get the alert. I'm using chrome OSX. What browser are you using?

ryanrath · 2017-09-14T15:34:07Z

Chrome / Linux

ryanrath · 2017-09-14T15:40:10Z

Here's the docker command I used : docker run --privileged -p 8383:8080 -v /media/ryanrath/Data/data:/root/data -v /media/ryanrath/Data/workspace/ccr/xdmod/jpwhite4:/root/src/github.com/jpwhite4/xdmod -v /sys/fs/cgroup:/sys/fs/cgroup:ro -it tas-tools-ext-01.ccr.xdmod.org/centos7-xdmod6.6.0:version5 /bin/bash

Here is the workspace/ccr/xdmod/jpwhite4 directory:

Here is a screenshot of the alert:

Once in the container I issued the following commands:

cd ~/src/github.com/jpwhite4/xdmod
export XDMOD_TEST_MODE=fresh_install
~/bin/buildrpm xdmod && ./open_xdmod/modules/xdmod/integration_tests/scripts/bootstrap.sh && xdmod-setup
Set up the database

On the host system:

opened chrome and navigated to 'localhost:8383'
etc.

ryanrath · 2017-09-14T15:45:53Z

Here's FF:

jpwhite4 · 2017-09-14T15:46:40Z

Did you make sure to clear the browser cache?

jpwhite4 · 2017-09-14T15:51:24Z

jpwhite4 · 2017-09-14T15:52:32Z

After a save

After a ctrl-shift-reload

ryanrath · 2017-09-14T15:55:08Z

Note to self, clearing your cache will log you out of Github....
Clearing the cache exhibits the same behavior, I have a screenshot of incognito mode as well

jpwhite4 · 2017-09-14T16:00:53Z

Can you put a breakpoint in line 4972 and dump the contents of metaData.attr

jpwhite4 · 2017-09-14T16:07:52Z

ryanrath · 2017-09-14T16:46:32Z

Ahhh, now I see the discrepancy. Line 4972, you have Ext.util.Format.htmlEncode(name) but I don't see that in the changeset.

ryanrath · 2017-09-14T16:47:31Z

~~I believe that should take care of it.~~ Errr... adding that should take care of the it.

jpwhite4 · 2017-09-14T16:50:09Z

Sorry about that. I made all of the changes in the installed location and then manually copied them back into the source tree. I should have double checked that I caught all of the changes.

ryanrath · 2017-09-14T16:52:52Z

No worries :) Just glad we figured it out. If you'd like I can toss the set of manual tests I came up with here so that we can refer to them when creating the automated tests ( if they + yours are sufficient of course). ( I also have all the changes to take care of Chart Title but I can just do that in a different PR ).

jpwhite4 · 2017-09-14T16:58:57Z

Sound like a good plan. I'm in the process of writing some auto tests now.

ryanrath · 2017-09-14T17:19:47Z

Here's what I had ( granted I was also testing the Title so Test 1.5 can be left out I think )

Manual Testing was performed, they were as follows:

Test 1

Log in to XDMoD
Select the 'Metric Explorer' Tab
Select the menu options: 'New Chart' -> 'Timeseries' -> 'Line' to create a new chart
Enter 'a & b' for the chart name
Click 'Ok' to finish creation of the chart
Ensure that the text of the 'Chart Options' button has been updated and that it shows a & b not a & b
Click the 'Load Chart' button
Ensure that the first entry correctly displays a & b not a & b
Ensure that the first entry has a red font color
Click the 'Chart Options' button
ensure that the value in the 'Name' text box displays a & b not a & b
ensure that the value in the 'Title' text box displays a & b not a & b
Click the 'Save' -> 'Save Changes' button
Click the 'Load Chart' button
ensure that the first entry is still displayed correctly and that it's color is now black.

Test 1.5:

force refresh XDMoD
click the 'Load Chart' button
ensure that this charts entry is still displayed correctly
select this charts entry
Add data to the chart if it does not already contain some
Click the 'Save' -> 'Save Changes' button
Click the 'Available For Report' checkbox
Select the 'Report Generator' tab
Ensure that this chart is listed in the Chart Pool and that the picture displayed is representative of what it looked like in Metric Explorer.
Create a new Report if there is not one already, or open an existing report
Add the new chart to the report
Again ensure that the picture of the chart is representative of what it looked like in Metric Explorer.
click the 'Save' button
ensure that this completes successfully ( there will be a prompt )
Click 'Preview'
ensure that the chart title displays as intended. i.e. a & b not a & b
click 'Return to the Report Editor'
click 'Download' -> 'PDF'
ensure that the pdf is generated successfully
open the pdf
ensure that the chart is representative of what it looked like in Metric Explorer and that the title displays correctly.
click the 'Download' -> 'As Word Doc' button
ensure that the word document is generated successfully
open the document
ensure that the chart is representative of what it looked like in Metric Explorer and that the title displays correctly.

Test 2: depends on Test 1

force refresh XDMoD
click the 'Load Chart' button
ensure that the first entry still correctly displays a & b not a & b
ensure that the font color for the first entry is still black
hover your mouse over the 'Chart Options' button
ensure that the value displayed in the tooltip is the same as the text of the button
Click the 'Chart Options' button
Ensure that the value displayed in the 'Name' text box is the same as the text of the 'Chart Options' button
Ensure that the value displayed in the 'Title' text box has not changed
Update the value in the 'Name' text box to include some additional content ( it's not important at this point what that is, we just want to see that the value changes in all the appropriate places and that it's displayed correctly )
Ensure that the 'Chart Options' button has had it's text updated appropriately
Click the 'Load Chart' button and ensure that this chart's entry has been updated appropriately.
Click 'Save' -> 'Save Changes'

Test 3: Depends on Test 2

force refresh XDMoD
click the 'Load Chart' button
Ensure that this chart's entry is still displayed correctly
select this chart's entry
Ensure that the 'Chart Options' text is still correctly updated
Click the 'Chart Options' button
Ensure the value in the 'Name' text box is still displayed correctly
Ensure the value in the 'Title' text box is still displayed correctly
Update the value in the 'Title' text box to include additional content
Click the 'Save' -> 'Save Changes' button
Conduct Test 1.5 and ensure that the new title is displayed correctly.

Test 4:

Conduct Tests 1-3 with chart name: <img src='x' onerror='alert(1)'> and title:<img src='x' onerror='alert(2)'>
for all tests ensure that no 'alert's are triggered in addition to any other assertions the test require

ryanrath

Looks good to me.

jpwhite4 added 3 commits September 13, 2017 17:22

Style fixes for linter

0207e86

Whoops missed change from the installed code

40ac55f

jpwhite4 requested a review from ryanrath September 14, 2017 02:05

jpwhite4 force-pushed the htmldecode branch 3 times, most recently from eff6de1 to 0207e86 Compare September 14, 2017 02:54

ryanrath reviewed Sep 14, 2017

View reviewed changes

Added integration test for the queries endpoint

c0a5139

plessbd approved these changes Sep 15, 2017

View reviewed changes

ryanrath approved these changes Sep 18, 2017

View reviewed changes

jpwhite4 merged commit a169575 into ubccr:xdmod7.0 Sep 18, 2017

jpwhite4 deleted the htmldecode branch September 18, 2017 14:02

tyearke added this to the v7.0.0 milestone Sep 21, 2017

tyearke mentioned this pull request Sep 21, 2017

Merge 7.0 work into 7.1 #253

Merged

tyearke added the bug Bugfixes label Sep 22, 2017

Make the Chart Name field html 'clean'. #239

Make the Chart Name field html 'clean'. #239

Conversation

jpwhite4 commented Sep 14, 2017 • edited Loading

Testing details

jpwhite4 commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

smgallo commented Sep 14, 2017 • edited Loading

ryanrath commented Sep 14, 2017

ryanrath commented Sep 14, 2017

ryanrath left a comment

Choose a reason for hiding this comment

ryanrath Sep 14, 2017

Choose a reason for hiding this comment

ryanrath Sep 14, 2017

Choose a reason for hiding this comment

ryanrath commented Sep 14, 2017

ryanrath commented Sep 14, 2017

plessbd commented Sep 14, 2017

ryanrath commented Sep 14, 2017 • edited Loading

plessbd commented Sep 14, 2017

ryanrath commented Sep 14, 2017 • edited Loading

jpwhite4 commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017 • edited Loading

jpwhite4 commented Sep 14, 2017 • edited Loading

ryanrath commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

ryanrath commented Sep 14, 2017

ryanrath commented Sep 14, 2017 • edited Loading

ryanrath commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

ryanrath commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

ryanrath commented Sep 14, 2017 • edited Loading

ryanrath commented Sep 14, 2017 • edited Loading

jpwhite4 commented Sep 14, 2017

ryanrath commented Sep 14, 2017

jpwhite4 commented Sep 14, 2017

ryanrath commented Sep 14, 2017 • edited Loading

ryanrath left a comment

Choose a reason for hiding this comment

jpwhite4 commented Sep 14, 2017 •

edited

Loading

smgallo commented Sep 14, 2017 •

edited

Loading

ryanrath commented Sep 14, 2017 •

edited

Loading

ryanrath commented Sep 14, 2017 •

edited

Loading

jpwhite4 commented Sep 14, 2017 •

edited

Loading

jpwhite4 commented Sep 14, 2017 •

edited

Loading

ryanrath commented Sep 14, 2017 •

edited

Loading

ryanrath commented Sep 14, 2017 •

edited

Loading

ryanrath commented Sep 14, 2017 •

edited

Loading

ryanrath commented Sep 14, 2017 •

edited

Loading