CCHIT-HIPAA De-identification patch submitted

visolveemr wrote on Wednesday, January 27, 2010:

Hi Team,

The patch file for ‘HIPAA-Deidentification’ is submitted in the sourceforge tracker at http://sourceforge.net/tracker/?func=detail&aid=2939409&group_id=60081&atid=1245239

Do share your views.

Thanks
ViCarePlus Team

sunsetsystems wrote on Thursday, January 28, 2010:

I have not had time to try getting this stuff running, do you have a demo?

I skimmed most of the code.  This is good stuff and carefully done.  I’m impressed.  I like the approach of using stored procedures to accomplish background processing.  One concern is, will it work with other databases such as PostgreSQL, or can it be easily adapted to do so?  Also have you tested backup and the companion restore script to make sure they still work?

You don’t get off scott-free with the code, I do have a couple of nits to pick.  :slight_smile:

Regarding de_identification_screen1.php:

* This code and others need a lot more comments.  The goal here is to help the newcomer to your code understand the overall logic of what’s going on.

* Regarding this line and many others like it:

     alert("<?php echo xl(‘End date should be greater than Begin date’);?>");

Be careful when you create a quoted literal that might contain something you have no control over, in this case a translated string.  If the translation includes a quote, the code will crash and someone is going to have a big support headache.  Better would be:

     alert("<?php echo addslashes(xl(‘End date should be greater than Begin date’)); ?>");

(thinking about this, addslashes() is not ideal for this purpose; we probably want a custom function that deals with quotes as well as newline characters).

* Regarding this and other places:

    <table style=“width: 500px;border:1px solid #0000FF;” align=“center” >

Best to avoid specifying widths and heights in pixels.  The problem is that as monitors get better and with higher resolutions, these things will get tinier.  In most cases it’s best to use either points or percentage.

Nice job!

Rod
www.sunsetsystems.com

bradymiller wrote on Thursday, January 28, 2010:

Rod,
That’s good to know regarding the alert coding. Would this be better, or still need custom function:
alert("<?php echo htmlspecialchars(xl(‘End date should be greater than Begin date’),ENT_QUOTES); ?>");
-brady

sunsetsystems wrote on Thursday, January 28, 2010:

No, htmlspecialchars() will translate too much stuff.  For example if the translation includes “<” then you will get “&lt;” in the alert box.

Rod
www.sunsetsystems.com

bradymiller wrote on Thursday, January 28, 2010:

Rod,
This bug will also pertain to translated title within html tags, correct?
wow, there are lots of translated titles and alerts in the openemr codebase….
what would be best way to go about correcting them:
1)a script?
2)a code walk through?
3)would it be a good idea to have a globals.php switch in translation.inc.php  that wiped out all apotrophe/quotes from outputted translations just as an emergency switch?
-brady

visolveemr wrote on Thursday, January 28, 2010:

Hi Rod,

Thanks for your views. Will try to set up a demo server for this feature and audit logging.

Thanks
ViCarePlus Team

sunsetsystems wrote on Thursday, January 28, 2010:

Selvi, that sounds great.

Brady, my concern was mostly about items within JavaScript literals which can crash scripts without the cause being immediately obvious.  But yes, HTML attributes can also be in quotes and break when their string contains a quote, and that does deserve a solution.

So I think we need a suitable function and some developer documentation on how to use it.  They should be used moving forward, and to patch old code as resources are available for that.  In the meantime perhaps the translations could be scanned for single and double quotes?

Rod
www.sunsetsystems.com

sunsetsystems wrote on Thursday, January 28, 2010:

Actually, now that I have my coffee, zapping quotes and newlines in xl() doesn’t sound like a bad idea at all.  There are surely not many of them, and an new function (called, say, xl_raw()) could be added for those cases where zapping is not wanted.

Rod
www.sunsetsystems.com

bradymiller wrote on Thursday, January 28, 2010:

hey,
Could convert all quotes to - character and zap newlines (I don’t let newlines in the official translation sets, but user could always insert them in the admin->language screen); shouldn’t be too intrusive since as I recall all translations are just screen output, and don’t ever need to be reverse translated.
-brady

sunsetsystems wrote on Thursday, January 28, 2010:

I suggest replacing single and double quotes with back-ticks (`).  That way it sort of looks similar.

By the way a quick query on a recent installation shows 66 translations with single quotes, and 215 with double quotes.

Rod
www.sunsetsystems.com

tmccormi wrote on Friday, January 29, 2010:

Back sounds like a bad idea on a unix system from a security perspective to me.
-Tony

tmccormi wrote on Friday, January 29, 2010:

Back tics sounds like a bad idea on a unix system from a security perspective to me.

I hate this damn forum tool.  Needs a edit message and a way to respond from email, it’s such a dang kludge to communicate this way.

bradymiller wrote on Friday, January 29, 2010:

maybe the board should decide this. should i put it on the board meeting itinerary…

sunsetsystems wrote on Friday, January 29, 2010:

Uh, back ticks on the board itinerary?  :slight_smile:

Anyway I don’t know of any translated strings going into unix shell commands.  If there were that would be a security issue all by itself.

Rod
www.sunsetsystems.com

bradymiller wrote on Friday, January 29, 2010:

hey,
The back tick tests well and looks good(replacing single/double quotes; also replacing endline with space and removing and carriage returns). Tony, you ok with this since backticks is only used in labels and such. I don’t see a need for an xl_raw() (function that does not remove the nasty characters) at this time; if ever need this in the future, can create it then.  If all amenable, will commit it and bring it over to 3.2 also.
-brady

sunsetsystems wrote on Friday, January 29, 2010:

Thanks Brady!

Rod
www.sunsetsystems.com

bradymiller wrote on Saturday, January 30, 2010:

hey,
deed is done.

Visolve,

This means that these lines are now safe (no need to use addslashes()):
alert("<?php echo xl(‘End date should be greater than Begin date’);?>");

-brady

visolveemr wrote on Monday, February 15, 2010:

Hi,

The modified de-identification patch is placed in the sourceforge tracker. The changes incorporated in Feb15 patch are:

1. Widths and heights are now converted into %.

2. Styles are placed in css file

3. By default, de-identification screen will not be present in left menu.
It can be made visible by setting the variable “include_de_identification”
in globals.php to 1

Thanks
ViCarePlus Team

bradymiller wrote on Saturday, February 20, 2010:

hey,
Before this gets more review. A couple questions?
Are triggers/procedures absolutely needed? I think most default mysql installations require root privileges to install these (hence the setup.php and sql_upgrade script give access errors when run. Would this be compatible with other databases (at some  point in the future we’d like to be compatible with other databases such as postgresql.
-brady

tmccormi wrote on Sunday, February 21, 2010:

Triggers and Stored procedures are supported by most if not all modern databases, postgresql  does.

These tools are very useful and will allow for much better performance in the long run.    I don’t requiring root privs is big deal, myself.

Tony McCormick
PS: backtic fix good… :slight_smile: