Skip to content
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.

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

New dictation modernazation #818

Closed
wants to merge 21 commits into from

Conversation

juggernautsei
Copy link
Member

Looking for input on what should I do next.

/**
* Drag and Drop file uploader.
*
* Copyright (C) 2017 Sherwin Gaddis sherwingaddis@gmail.com
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$category_id = filter_input(INPUT_GET, 'parent_id');


$sanitize_all_escapes=true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality has been removed, remove 25, 26

$fake_register_globals=false;

require_once("../globals.php");
require_once(dirname(__FILE__) . "/../../library/documents.php");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think '$GLOBALS["srcdir"]' would point to the library folder

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually $scrdir will do the job.


}

?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for closing tag here

@@ -1,29 +1,56 @@
<!-- Form generated from formsWiz -->
<?php
/** Copyright (C) 2016 Sherwin Gaddis <sherwingaddis@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment about docblocks

</head>
<body class="body_top">
<h1><?php echo xlt('Speech Dictation'); ?></h1><br><br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap this in a div with the class page-header

</head>
<body class="body_top">
<h1><?php echo xlt('Speech Dictation'); ?></h1><br><br>

<div class="container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Container should be up by the body, we want most everything contained

<label class="text"><?php echo xlt('Additional Notes:'); ?> </label><br><textarea class="form-control ckeditor" cols=80 rows=8 wrap="virtual" name="additional_notes" ></textarea>
</div>

<a href="javascript:top.restoreSession();document.my_form.submit();" class="link_submit"><button><?php echo xlt('Save'); ?></button></a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the a tag, setup the button with a type=submit and the following classes: btn btn-default btn-save

<a href="<?php echo "$rootdir/patient_file/encounter/$returnurl";?>" class="link"
onclick="top.restoreSession()">[<?php echo xlt('Don\'t Save'); ?>]</a>
onclick="top.restoreSession()"><button><?php echo xlt('Don\'t Save'); ?></button></a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will seem odd, but there's reasons. This element will take the user away from the page so it should be an tag, but there is no need for a button inside. Add the btn btn-cancel and btn-link classes to the anchor. This will make it look like a button while maintaining the proper functionality. Also, I'd stick with "Cancel" as the text, that seems to be the paradigm throughout the rest of the program

</head>
<body class="body_top">
<h1><?php echo xlt('Speech Dictation'); ?></h1><br><br>

<div class="container">
<form method=post action="<?php echo $rootdir;?>/forms/dictation/save.php?mode=new" name="my_form">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at the examples on http://getbootstrap.com/css/#forms for how to layout a form.

@robertdown
Copy link
Member

As a note, buttons in bootstrap get their styling via the btn class, not via the button element. it's common to use btn classes on anchor tags, especially cancel links.

@bradymiller
Copy link
Sponsor Member

delete the 2 unrelated files to make this PR more clear.

<?php Header::setupHeader(); ?>
<script type="text/javascript" src="<?php echo $webroot."/library/custom_template/ckeditor/ckeditor.js" ?>"</script>
<script src="<?php echo $webroot."/library/custom_template/ckeditor/_samples/sample.js" ?>" type="text/javascript"></script>
<link href="<?php echo $webroot."/library/custom_template/ckeditor/_samples.css"; ?>" rel="stylesheet" type="text/css" />
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these things drastically change form? And also, will the form still be backward compatible?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To my limited knowledge, the ckeditor wraps around the text area like styling. I will test to make sure but it does not change the property of the text area.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which two unrelated files

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 unrelated files are:
interface/documents/upload.php
templates/documents/general_upload.html (revert this to file in master by just copying it over)

print "<td><span class=bold>" . xlt($key) . ": </span><span class=text>" .
nl2br(text($value)) . "</span></td>";
print "<td><span class='bold'>" . xlt($key) . ": </span><span class='text'>" .
nl2br($value) . "</span></td>";
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing the text() call is an issue here since it opens the door to cross scripting attacks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But leaving it there causes the output to show the html characters. I have changed the order and it still outputs the line breaks to the screen. After doing some research I found html_entity_decode() works. And should prevent XSS attacks.

print "<td><span class=bold>" . xlt($key) . ": </span><span class=text>" .
nl2br(text($value)) . "</span></td>";
print "<td><span class='bold'>" . xlt($key) . ": </span><span class='text'>" .
html_entity_decode($value, ENT_QUOTES | ENT_HTML5) . "</span></td>";
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

html_entity_decode won't protect from cross scripting attacks.

Can't have raw html (which then allows raw javascript) in fields like this. This could be a potential fatal flaw of ckeditor if that is what requires bypassing html escaping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was doing some reading and it is not the job of the editor but the server side store process. So, reading that, I changed the way the data is stored. I put the input filters in place and then converted any HTML into entities and that is stored in the tables. Now we should be safe from XSS attacks.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't sound like it will protect from xss. Let me look into this a bit.

…erver

side (read an article). So, updated the save function. Now, the system
stores html entities. No html in the database table now.
@juggernautsei
Copy link
Member Author

xss
I entered a script tag in the text box and it comes back to the screen as text.

@bradymiller
Copy link
Sponsor Member

@juggernautsei ,
This is odd. Here's what is happening in the code for the text entries.

  1. Note the filter_input() calls are doing no escaping/filtering/sanitizing
  2. The htmlspecialchars() is escaping the html elements (like < and >)
  3. The html_entity_decode() should then unescape all the html elements.
    (thus the above really shouldn't do anything; ie. 1 big circle :) )

I am guessing that ckeditor has a some basic code built in to escape obvious things like <script> tags.

Would test it on the prior code without all the above escaping stuff.

@juggernautsei
Copy link
Member Author

juggernautsei commented Jun 4, 2017

I put the report file back to the way it was with the text($value) and the save.php is back to the original. Below is what shows in the patient chart. And it is what is in the database table.

xss-report

@bradymiller
Copy link
Sponsor Member

@juggernautsei ,
That is interesting. Just drop the text() call then, and we should be good(ie. don't worry, this wasn't a big exercise in futility since we learned a lot along the way :) ). It looks like ckeditor is escaping the script tags behind the scenes before placing it in the database. Since we are relying on ckeditor to do the html escaping, will be good idea to get most recent version of it in the codebase. What's the best editor out there (this would be a good time to decide on ideal editor to use).

@bradymiller
Copy link
Sponsor Member

@juggernautsei ,
Looks like I can bring in most recent version of ckeditor easily via bower. Just let me know if you want it, and then I'll bring it in.

@juggernautsei
Copy link
Member Author

We still have a problem. When I remove the text the cross-scripting works. And it does not show on the screen so it is not stopping or rendering harmless the XSS injection.
Also, in all my searches for editors ckeditor is always on the list and the current version is 4.7

SOLUTION
htmlpurifier.org
I've tried it and it works. Posting code to show how.

@bradymiller
Copy link
Sponsor Member

The htmlpurifier looks promising. I can bring that in via composer (so it's then autoloaded); just let me know and I'll bring it into the codebase. -brady

@bradymiller
Copy link
Sponsor Member

Another place we can also use this is in pnotes.

@juggernautsei
Copy link
Member Author

Yes, pull the HTML purifier into the code base. It has a symphony plugin.

@bradymiller
Copy link
Sponsor Member

Hi @juggernautsei ,

I just added both packages to the codebase(check out the most recent 2 commit in the main codebase). Note I installed the standard-all package for ckeditor (if needed, can install one of the other packages such as full, if needed in the future).

-brady

@@ -12,6 +12,8 @@

use OpenEMR\Core\Header;
include_once("../../globals.php");
require_once("../../../public/assets/htmlpurifier-4.9.2/library/HTMLPurifier.auto.php");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this come in via bower or composer?

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was brought in via composer in the current codebase

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, is autoloaded

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the ../../../public should be replaced with which global? I looked for it and couldn't find it or remember what it is.

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's autoloaded, no include/require is needed. The class is magically available in all scripts. However, will need to rebase your code on the most current codebase (in order to bring this code in). Also should delete entire public/assets/htmlpurifier-4.9.2 directory from your branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will likely need a usestatement

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use statement was not needed as @bradymiller suggested. It works without the use statement. I will post in a moment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't work without that line. It needs to be added

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am not sure which line you are referring. If you are referring to this line:
require_once("/public/assets/htmlpurifier-4.9.2/library/HTMLPurifier.auto.php");
I am pretty sure it works without it because without that line the page loads. Please correct me if I am wrong but if the page makes a call to a class that it can't find that page should throw an error and not load, right? Everything is working fine without that line.
You can try it out at http://omp.openmedpractice.com/rolebase log in admin pass.

$obj = formFetch("form_dictation", $id);
?>
<form method=post action="<?php echo $rootdir?>/forms/dictation/save.php?mode=update&id=<?php echo attr($id);?>" name="my_form">
<div class="page-header">
<h1><?php echo xlt('Speech Dictation Review/Edit'); ?></h1>
</div>
<div class="form-group">
<label for="dictation"><?php echo xlt('Dictation: '); ?></label><br><textarea class="form-control ckeditor" cols=80 rows=24 wrap="virtual" name="dictation" ><?php echo html_entity_decode(text($obj{"dictation"}));?></textarea>
<label for="dictation"><?php echo xlt('Dictation: '); ?></label><br><textarea class="form-control ckeditor" cols=80 rows=24 wrap="virtual" name="dictation" ><?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest breaking this super long line up. Textarea with "form-control" shouldn't have the colts attribute, CSS takes care of that

</div>
<div class="form-group">
<label for="additional_notes"><?php echo xlt('Additional Notes: '); ?></span><br><textarea class="form-control ckeditor"cols=80 rows=8 wrap="virtual" name="additional_notes" ><?php echo html_entity_decode(text($obj{"additional_notes"}));?></textarea>
<label for="additional_notes"><?php echo xlt('Additional Notes: '); ?></span><br><textarea class="form-control ckeditor"cols=80 rows=8 wrap="virtual" name="additional_notes" ><?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment

@@ -14,9 +14,6 @@
include_once($GLOBALS["srcdir"]."/api.inc");
require_once("../../../public/assets/htmlpurifier-4.9.2/library/HTMLPurifier.auto.php");
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove above line

@@ -0,0 +1,46 @@
<?php
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this file

<?php Header::setupHeader(); ?>
<script type="text/javascript" src="<?php echo $webroot."/library/custom_template/ckeditor/ckeditor.js" ?>"</script>
<script src="<?php echo $webroot."/library/custom_template/ckeditor/_samples/sample.js" ?>" type="text/javascript"></script>
<link href="<?php echo $webroot."/library/custom_template/ckeditor/_samples.css"; ?>" rel="stylesheet" type="text/css" />
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after you rebase your code to the most recent codebase, then you will use the ckeditor that is in public/assets/

include_once(dirname(__FILE__).'/../../globals.php');
include_once($GLOBALS["srcdir"]."/api.inc");
require_once("../../../public/assets/htmlpurifier-4.9.2/library/HTMLPurifier.auto.php");
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you rebase code to most recent codebase, then you can remove the above line.

include_once("../../globals.php");
require_once("../../../public/assets/htmlpurifier-4.9.2/library/HTMLPurifier.auto.php");
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you rebase code to most recent codebase, then you can remove the above line.

<?php Header::setupHeader(); ?>
<script type="text/javascript" src="<?php echo $webroot."/library/custom_template/ckeditor/ckeditor.js" ?>"</script>
<script src="<?php echo $webroot."/library/custom_template/ckeditor/_samples/sample.js" ?>" type="text/javascript"></script>
<link href="<?php echo $webroot."/library/custom_template/ckeditor/_samples.css"; ?>" rel="stylesheet" type="text/css" />
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you rebase code to most recent codebase, then you can use the ckeditor in public/assets/

<link rel="stylesheet" href="<?php echo $css_header;?>" type="text/css">
<html>
<head>
<title>Dictation</title>
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translate:

<title><?php echo xlt('Dictation'); ?></title>

<?php Header::setupHeader(); ?>
<script type="text/javascript" src="<?php echo $webroot."/public/assets/ckeditor-4-7-0/ckeditor.js" ?>"</script>
<script src="<?php echo $webroot."/public/assets/ckeditor-4-7-0/js/sample.js" ?>" type="text/javascript"></script>
<link href="<?php echo $webroot."/public/assets/ckeditor-4-7-0/css/samples.css"; ?>" rel="stylesheet" type="text/css" />
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the above 3 lines, need to use the global for the assets path(this will allow us to migrate that path easily if needed):

<script type="text/javascript" src="<?php echo $GLOBALS['assets_static_relative']; ?>/ckeditor-4-7-0/ckeditor.js"</script>
<script src="<?php echo $GLOBALS['assets_static_relative']; ?>/ckeditor-4-7-0/js/sample.js" type="text/javascript"></script>
<link href="<?php echo $GLOBALS['assets_static_relative']; ?>/ckeditor-4-7-0/css/samples.css" rel="stylesheet" type="text/css" />

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note I made some edits to above post

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the path to samples.* on your bottom 2 lines is incorrect.

<h1><?php echo xlt('Speech Dictation'); ?></h1><br><br>
</div>

<form method=post action="<?php echo $rootdir;?>/forms/dictation/save.php?mode=new" name="my_form">
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forms needs to deal with session leak issue (especially since it was removed from the Save link):

<form method=post action="<?php echo $rootdir;?>/forms/dictation/save.php?mode=new" name="my_form" onsubmit="return top.restoreSession()">

More information on this can be found here:
http://www.open-emr.org/wiki/index.php/OpenEMR_System_Architecture#PHP_Sessions_and_Browser_Windows

<link rel="stylesheet" href="<?php echo $css_header;?>" type="text/css">
<html>
<head>
<title>Review</title>
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translate and use Dictation:

<title><?php echo xlt('Dictation'); ?></title>

<?php Header::setupHeader(); ?>
<script type="text/javascript" src="<?php echo $webroot."/public/assets/ckeditor-4-7-0/ckeditor.js" ?>"</script>
<script src="<?php echo $webroot."/public/assets/ckeditor-4-7-0/js/sample.js" ?>" type="text/javascript"></script>
<link href="<?php echo $webroot."/public/assets/ckeditor-4-7-0/css/samples.css"; ?>" rel="stylesheet" type="text/css" />
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the above 3 lines, need to use the global for the assets path(this will allow us to migrate that path easily if needed):

<script type="text/javascript" src="<?php echo $GLOBALS['assets_static_relative']; ?>/ckeditor-4-7-0/ckeditor.js"</script>
<script src="<?php echo $GLOBALS['assets_static_relative']; ?>/ckeditor-4-7-0/js/sample.js" type="text/javascript"></script>
<link href="<?php echo $GLOBALS['assets_static_relative']; ?>/ckeditor-4-7-0/css/samples.css" rel="stylesheet" type="text/css" />

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the path to samples.* on your bottom 2 lines is incorrect.

<a href="<?php echo "$rootdir/patient_file/encounter/$returnurl";?>" class="link"
onclick="top.restoreSession()">[<?php echo xlt('Don\'t Save Changes'); ?>]</a>
</form>
<form method=post action="<?php echo $rootdir?>/forms/dictation/save.php?mode=update&id=<?php echo attr($id);?>" name="my_form">
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forms needs to deal with session leak issue (especially since it was removed from the Save link):

<form method=post action="<?php echo $rootdir?>/forms/dictation/save.php?mode=update&id=<?php echo attr($id);?>" name="my_form" onsubmit="return top.restoreSession()">

More information on this can be found here:
http://www.open-emr.org/wiki/index.php/OpenEMR_System_Architecture#PHP_Sessions_and_Browser_Windows

</form>
<form method=post action="<?php echo $rootdir?>/forms/dictation/save.php?mode=update&id=<?php echo attr($id);?>" name="my_form">
<div class="page-header">
<h1><?php echo xlt('Speech Dictation Review/Edit'); ?></h1>
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably doesn't makes sense to make this title different on the forms since most forms do not allow this (goal is keeping things as uniform as possible), since usually view.php redirects to new.php on a lot of the forms. Also good to keep the title in head the same as this title which should be the same as the form name. So would use 'Speech Dictation' in all those place.

<form method=post action="<?php echo $rootdir;?>/forms/dictation/save.php?mode=new" name="my_form" onclick="top.restoreSession()">

<div class="form-group">
<label for="dictation"><?php echo xlt('Dictation: '); ?></label><br><textarea class="form-control ckeditor" name="dictation" ></textarea>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bradymiller, correct me if I'm wrong, but shouldn't end-of-line spaces and punctuation like this be avoided? Also, I'm in favor of yanking colons from labels all together

Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robertdown
In this case, the damage was already done about 8 years ago; meaning, this "constant" was already sent to the translation pipeline in it's current form at that time. So, changing these can cause a new constant that needs to be translated. This isn't always the case; for example, here , the Dictation constant already exists, so changing this wouldn't even create a new constant:
https://github.com/openemr/openemr/blob/master/contrib/util/language_translations/currentConstants.txt#L1899

A good general strategy:
If there's a reason(such as not wanting to have the colons), then should change these constants, however, shouldn't do a codebase wide arbitrary fixing of leading/trailing spacing in contants. Hope that makes some sense.

@bradymiller
Copy link
Sponsor Member

Remove the following directory (this will make it much easier to review the code):
public/assets/htmlpurifier-4.9.2

@juggernautsei
Copy link
Member Author

Review Please

@bradymiller
Copy link
Sponsor Member

This branch wasn't working since missing the ckeditor, so I rebased this code into 1 commit and into the most recent codebase in another PR here:
#879

Should start working on the new PR branch and will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants