Bug 12123 - HTML notices can break the notice viewer
Summary: HTML notices can break the notice viewer
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
: 20675 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-04-22 13:55 UTC by Kyle M Hall
Modified: 2018-04-29 22:18 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12123 - HTML notices can break the notice viewer (1.02 KB, patch)
2014-04-22 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12123 - HTML notices can break the notice viewer (1.00 KB, patch)
2014-04-22 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12123 - HTML notices can break the notice viewer (1.62 KB, patch)
2014-11-24 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12123 - HTML notices can break the notice viewer (1.76 KB, patch)
2014-11-24 17:45 UTC, Owen Leonard
Details | Diff | Splinter Review
Screenshot (73.97 KB, image/png)
2014-11-24 21:01 UTC, Katrin Fischer
Details
Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (2.20 KB, patch)
2014-11-24 23:06 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (1.67 KB, patch)
2014-11-25 03:53 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (1.84 KB, patch)
2014-11-26 13:30 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 12123 - [Alternative patch] HTML notices can break the notice viewer (1.92 KB, patch)
2014-11-27 07:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12123 - HTML notices can break the notice viewer (1.82 KB, patch)
2018-04-04 15:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12123 - HTML notices can break the notice viewer (1.89 KB, patch)
2018-04-06 09:40 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 12123: Do not render HTML in notice viewer if invalid (2.28 KB, patch)
2018-04-12 16:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12123 - Make iframe large enough to remove scroll bars (1.33 KB, patch)
2018-04-13 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-04-22 13:55:12 UTC
Depending on the content of an html notice, it can cause the notice to either not be collapsible, be uncollapsible, or to be permanently collapsed.
Comment 1 Kyle M Hall 2014-04-22 13:57:51 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-04-22 13:58:33 UTC
I do not consider this patch to be an optimal solution, but it
Comment 3 Kyle M Hall 2014-04-22 13:59:39 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2014-04-22 16:31:44 UTC
Please provide a proper commit message with a test plan.
Comment 5 Kyle M Hall 2014-11-24 15:02:28 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2014-11-24 17:45:13 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2014-11-24 21:01:23 UTC
Created attachment 33882 [details]
Screenshot

I am not sure about this - it seems to work, but should it look like on the screenshot? Do we really need an iframe here?
Comment 8 Liz Rea 2014-11-24 23:06:58 UTC Comment hidden (obsolete)
Comment 9 Liz Rea 2014-11-25 03:53:50 UTC Comment hidden (obsolete)
Comment 10 David Cook 2014-11-25 03:53:57 UTC
Comment on attachment 33885 [details] [review]
Bug 12123 - [Alternative patch] HTML notices can break the notice viewer

Review of attachment 33885 [details] [review]:
-----------------------------------------------------------------

I definitely prefer Liz's patch over Kyle's. I don't think iframes are needed at all. It looks like the problem is that we were trying to stuff HTML into a "p" element which couldn't take them as children.

The only thing stopping my sign off would be the inline CSS. I find the CSS isn't necessary when I test it, and I suppose if it is necessary for other browsers, I would just prefer it to be in a stylesheet somewhere else so we can control our styles better.

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt
@@ +41,4 @@
>  <h1>Sent notices for [% INCLUDE 'patron-title.inc' %]</h1>
>  
>  [% IF ( QUEUED_MESSAGES ) %]
> +    <table id="noticestable" style="table-layout: fixed">

Using Chrome on Windows, I don't find this CSS does anything. If anything, it makes the presentation slightly worse.

However, if we do include it, my only request would be to not use inline CSS and to put this in a separate stylesheet somewhere like staff-global.css or something like that.

@@ +55,4 @@
>  	    <tr>
>  		<td>
>              <a class="notice-title" href="#">[% QUEUED_MESSAGE.subject %]</a>
> +            <div class="notice">

Switching to a div fixes this problem nicely.
Comment 11 Owen Leonard 2014-11-26 13:30:52 UTC Comment hidden (obsolete)
Comment 12 David Cook 2014-11-27 01:51:31 UTC
Thinking again... I can see the merit of using an iframe instead of a div. 

After all, the div only works because "p" and "blockquote" can be children of a "div".

If a person put "html" and "body" tags into the message, those the HTML would be invalid.

*does a test*

Mmm, instead of busting the notice viewer, Chrome removes the "html" and "body" tags. (So the viewer continues to work fine, although you'll still notice the "html" and "body" tags in the message_queue.)

I've noticed libraries put "html" and "body" tags in their notices before, which is why this comes into my mind.

Chrome is pretty rad at cleaning up tag soup, but I'm not sure about Firefox or Internet Explorer...

Microsoft Outlook seems capable of handling a HTML email with "html" and "body" tags inside of a "body" tag. I suppose most email clients are pretty flexible when it comes to invalid HTML markup.

In any case, I think Liz's solution works. If any problems arise in the future, I suppose we can address them then. (Or possibly strip "body" and "html" tags when saving notice templates.)
Comment 13 Katrin Fischer 2014-11-27 07:25:26 UTC
Created attachment 33976 [details] [review]
[PASSED QA] Bug 12123 - [Alternative patch] HTML notices can break the notice viewer

Depending on the content of an html notice, it can cause the notice to
either not be collapsible, be uncollapsible, or to be permanently
collapsed.

Test Plan:
1) Set your CHECKOUT notice to the following ( with HTML Message checked ) :
<p>The following items have been checked out:</p>

----
<blockquote> <<biblio.title>> </blockquote>
----

<p>Thank you for visiting the <<branches.branchname>> of HMCPL.</p>
2) Check out some items to a patron
3) View the patrn's notices
4) Note the notice viewer is broken ( message is not collapsed, and
   con't be collapsed ).
5) Apply this patch
6) Reload the page
7) Note the notice viewer is no longer broken

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

This patch appears to fix the problem with the minimum required change.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works nicely, no problems found.
Passes tests and QA script.
Comment 14 Tomás Cohen Arazi 2014-12-03 14:55:15 UTC
Patch pushed to master.

Thanks Kyle and Liz!
Comment 15 Chris Cormack 2014-12-03 20:17:54 UTC
Pushed to 3.18.x will be in 3.18.01
Comment 16 Kyle M Hall 2018-04-04 15:53:47 UTC
This is still a problem if the html of a notice is malformed. For example, an html notice missing a closing tag or quote can break all the things.
Comment 17 Kyle M Hall 2018-04-04 15:59:46 UTC
Created attachment 73650 [details] [review]
Bug 12123 - HTML notices can break the notice viewer

Depending on the content of an html notice, it can cause the notice to
either not be collapsible, be uncollapsible, or to be permanently
collapsed.

Test Plan:
1) Set your CHECKOUT notice to the following ( with HTML Message checked ) :
<p>The following items have been checked out:</p>

----
<blockquote> <<biblio.title>> </blockquote>
----

<p>Thank you for visiting the <<branches.branchname>> of HMCPL.</p>
2) Check out some items to a patron
3) View the patrn's notices
4) Note the notice viewer is broken ( message is not collapsed, and
   con't be collapsed ).
5) Apply this patch
6) Reload the page
7) Note the notice viewer is no longer broken

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

I followed the test plan successfully.
Comment 18 Marcel de Rooy 2018-04-06 09:38:09 UTC
Note: srcdoc seems not to be supported by IE/Edge.
Ignoring that fact since this is a staff page.
Comment 19 Marcel de Rooy 2018-04-06 09:40:01 UTC
Created attachment 73758 [details] [review]
Bug 12123 - HTML notices can break the notice viewer

Depending on the content of an html notice, it can cause the notice to
either not be collapsible, be uncollapsible, or to be permanently
collapsed.

Test Plan:
1) Set your CHECKOUT notice to the following ( with HTML Message checked ) :
<p>The following items have been checked out:</p>

----
<blockquote> <<biblio.title>> </blockquote>
----

<p>Thank you for visiting the <<branches.branchname>> of HMCPL.</p>
2) Check out some items to a patron
3) View the patrn's notices
4) Note the notice viewer is broken ( message is not collapsed, and
   con't be collapsed ).
5) Apply this patch
6) Reload the page
7) Note the notice viewer is no longer broken

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
I followed the test plan successfully.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Jonathan Druart 2018-04-06 17:22:24 UTC
I do not recreate the problem, the message is collapsed and the link behaves as expected.

Can I get more details to recreate?
Comment 21 Katrin Fischer 2018-04-10 05:53:50 UTC
I've tried it with the example from the commit message in Firefox and Chromium and no problems found (without patch). Maybe something is missing from the test plan?
Comment 22 Kyle M Hall 2018-04-10 12:42:57 UTC
(In reply to Katrin Fischer from comment #21)
> I've tried it with the example from the commit message in Firefox and
> Chromium and no problems found (without patch). Maybe something is missing
> from the test plan?

The original example doesn't break the page html anymore. Try leaving an open anchor tag or some other badly malformed html in a notice.
Comment 23 Jonathan Druart 2018-04-10 16:23:41 UTC
I tried that:

The following items have been checked out:
----
<<biblio.title>>
----
<div>
Thank you for visiting <<branches.branchname>>.
<div>

And it looks ok.
Comment 24 Kyle M Hall 2018-04-11 17:18:08 UTC
Try something like:

----
<<biblio.title>>
----
<div>
Thank you for <a ref='#'>visiting <<branches.branchname>>.
<div>
Comment 26 Kyle M Hall 2018-04-11 18:31:11 UTC
(In reply to Jonathan Druart from comment #25)
> Still works:
> https://screenshots.firefox.com/XkVBMvtHEg4P3sfT/pro.kohadev.org
> https://screenshots.firefox.com/cmfWvx4WeOHNkELn/pro.kohadev.org

Try adding this to the end of you notice:
<a href="

Also make sure the notice is set to HTML.
Comment 27 Jonathan Druart 2018-04-11 19:32:06 UTC
Still working.

SO is from 2014, Marcel did not confirm it recreates the issues.

Switch back to NSO.
Comment 28 Marcel de Rooy 2018-04-12 08:21:37 UTC
(In reply to Jonathan Druart from comment #27)
> Still working.
> 
> SO is from 2014, Marcel did not confirm it recreates the issues.

Will have another look at this report tomorrow.
Comment 29 Kyle M Hall 2018-04-12 11:07:55 UTC
(In reply to Jonathan Druart from comment #27)
> Still working.
> 
> SO is from 2014, Marcel did not confirm it recreates the issues.
> 
> Switch back to NSO.

Demonstration of bug with and without patch: https://www.youtube.com/watch?v=IAr-ht3b5kw
Comment 30 Jonathan Druart 2018-04-12 13:40:09 UTC
hum, I recreate it now, with the same notice as yesterday.
Sounds like PEBCAK...
Let me try again.
Comment 31 Jonathan Druart 2018-04-12 16:16:32 UTC
Created attachment 74113 [details] [review]
[ALTERNATIVE-PATCH] Bug 12123: Do not render HTML in notice viewer if invalid

If a notice contains malformed HTML it should not be displayed.
Comment 32 Jonathan Druart 2018-04-12 16:16:58 UTC
I'd like your opinions on this alternative patch.
Comment 33 Kyle M Hall 2018-04-12 17:21:40 UTC
(In reply to Jonathan Druart from comment #32)
> I'd like your opinions on this alternative patch.

Your patch will definitely fix any issues with malformed html, but it doesn't check to see if the notices *is* html, does it? Also, it feels like just another band-aid to me. Using an iframe completely sandboxes the message html so it will protect the page from problems that we haven't anticipated. It also adds more overhead since it is not doing xml processing on each notice a patron has.
Comment 34 Marcel de Rooy 2018-04-13 05:57:02 UTC
(In reply to Jonathan Druart from comment #32)
> I'd like your opinions on this alternative patch.

Can we add that test to saving the letter in tools/letter.pl ?
I would rather test once if we saved valid HTML and not check every message in the queue. (In the assumption that we do not add invalid constructs during processing.) To be safe, we present it here in an iframe.
Comment 35 Marcel de Rooy 2018-04-13 06:09:52 UTC
The example Kyle suggested in comment26 is sufficient to demonstrate that this patch makes a difference. I recommend to restore PQA status and move the html check to a new report.
Comment 36 Katrin Fischer 2018-04-13 07:09:33 UTC
Be careful with the check - HTML5 is not valid XML. Unclosed <p> tags are perfectly valid. Often browsers are forgiving and probably mail clients too. We don't want to break display in more cases than now, that will annoy the libraries.
Comment 37 David Cook 2018-04-13 07:22:58 UTC
(In reply to Katrin Fischer from comment #36)
> Be careful with the check - HTML5 is not valid XML. Unclosed <p> tags are
> perfectly valid. Often browsers are forgiving and probably mail clients too.
> We don't want to break display in more cases than now, that will annoy the
> libraries.

Good call.

In PHP, I used to use http://php.net/manual/en/book.tidy.php but that was pre-HTML5 I think. 

Might be worth looking for a HTML specific module?

Also, I'm pretty sure XML::Simple is a bad choice of XML modules. The author of it even discourages its use. No one should ever use it again I reckon.
Comment 38 Kyle M Hall 2018-04-13 11:10:52 UTC
It seems like the consensus is that the new alternative patch is not acceptable.
Comment 39 Kyle M Hall 2018-04-13 12:38:48 UTC
(In reply to Marcel de Rooy from comment #35)
> The example Kyle suggested in comment26 is sufficient to demonstrate that
> this patch makes a difference. I recommend to restore PQA status and move
> the html check to a new report.

Agreed
Comment 40 Jonathan Druart 2018-04-13 13:03:57 UTC
We are going to use iframe with a weird display (scrollbars) to catch possible malformed notices.
I still do not like the solution.
Something else to suggest?
Comment 41 Kyle M Hall 2018-04-13 13:57:50 UTC
Created attachment 74169 [details] [review]
Bug 12123 - Make iframe large enough to remove scroll bars
Comment 42 Jonathan Druart 2018-04-19 15:42:19 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 43 Katrin Fischer 2018-04-29 22:18:00 UTC
*** Bug 20675 has been marked as a duplicate of this bug. ***