Bug 40030

Summary: HTML should be escaped when viewing system preferences diff in Log viewer
Product: Koha Reporter: Emily Lamancusa (emlam) <emily.lamancusa>
Component: Architecture, internals, and plumbingAssignee: Emily Lamancusa (emlam) <emily.lamancusa>
Status: Pushed to oldstable --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: dcook, didier.gautheron, fridolin.somers, jonathan.druart, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, phil, philippe.blouin, tomascohen
Version: MainKeywords: release-notes-needed
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.02,24.11.08
Circulation function:
Attachments: Bug 40030: Escape values when displaying action logs comparison
Bug 40030: Escape values when displaying action logs comparison
Bug 40030: Escape values when displaying action logs comparison
Bug 40030: Escape values when displaying action logs comparison

Description Emily Lamancusa (emlam) 2025-05-29 16:33:40 UTC
When viewing the action logs for system preference changes, there is an option to view a comparison between two log lines for the same system preference (i.e. to see how the preference changed over time). If you use this option to view a comparison for IntranetUserJS, and the IntranetUserJS contains HTML markup (for example, to add a link or button to a page), that HTML is actually rendered in the comparison viewer.

At best this can lead to weirdness in the display. I'm not sure if this is specifically a security vulnerability if the library hasn't already introduced a security vulnerability in their IntranetUserJS, but I'm not sure that it isn't, either.
Comment 1 Emily Lamancusa (emlam) 2025-05-29 16:44:47 UTC
Created attachment 182845 [details] [review]
Bug 40030: Escape values when displaying action logs comparison

When viewing the action logs for a system preference change, there is an
option to view a comparison between different log lines for the same
system preference. Escape the string values before displaying the
comparison so that any HTML (e.g. in IntranetUserJS) is not rendered.

To test:
1. Edit IntranetUserJS and add some code that contains HTML markup.
   For example:
$("body").append('<a id="helloworld" class="btn btn-default">Say hi</a>');
2. Save the preference
3. Make another change to IntranetUserJS (for example, add a space
   somewhere) and save again to ensure there are at least 2 action log
   lines for comparison
4. Go to Tools > Log viewer
5. Select System Preferences module and Submit
6. Find the log entries for the above two revisions to IntranetUserJS
7. Check the Compare checkboxes on those entries, and click View
   comparison
--> Note that the HTML button is rendered in the comparison
8. Apply patch
9. Clear browser cache and refresh the page
10. Repeat step 7
--> The button is not rendered and the code is displayed as written
Comment 2 David Cook 2025-05-30 00:57:48 UTC
Sounds like a legitimate bug, and at a glance the code looks sensible, but I'll have to come back to review this.
Comment 3 Phil Ringnalda 2025-05-30 01:16:23 UTC
I'm not a huge fan of the way it double-escapes things in the actual diff (because jsdiff.js escapes those itself). Swap step 3 to be "Change Say hi to be Say hello" and in both the <del> and the <ins> after the changed word you'll have "&lt;/a&gt;" rather than "</a>" displayed.
Comment 4 Emily Lamancusa (emlam) 2025-06-03 15:48:55 UTC
Thanks for catching that, Phil! Ugh...jsdiff escapes the bits of code where a change is marked, but doesn't escape the rest. And we can't just escape the output either, since that would break the display of changes.

I guess the best short-term fix would be to escape the output except for the tags added by the diff?

Generally seems like a bug in the upstream library, though... I couldn't find an upstream repository to see if there's a newer version where it's fixed (or if the repository is still maintained).
Comment 5 Phil Ringnalda 2025-06-05 21:39:41 UTC
Pretty sure there's neither support nor a repo, just the twenty year old blog post linked in the comment of jsdiff.js. I'd say we shouldn't spend too much time on improving it, just fix this for now and then look for a better library. While coming up with a step to see the double-escaping, I hit another bug mentioned in the comments on the blog post, that moving things around doesn't get marked in the diff, though I missed seeing some others also mentioned there.

Untested, but could we just remove the escaping inside the <ins> and <del>, and escape all of both arguments as the first thing the function does, before diffing them?
Comment 6 David Cook 2025-06-06 01:09:46 UTC
(In reply to Phil Ringnalda from comment #5)
> Pretty sure there's neither support nor a repo, just the twenty year old
> blog post linked in the comment of jsdiff.js. I'd say we shouldn't spend too
> much time on improving it, just fix this for now and then look for a better
> library. While coming up with a step to see the double-escaping, I hit
> another bug mentioned in the comments on the blog post, that moving things
> around doesn't get marked in the diff, though I missed seeing some others
> also mentioned there.
> 
> Untested, but could we just remove the escaping inside the <ins> and <del>,
> and escape all of both arguments as the first thing the function does,
> before diffing them?

This is all sounding very familiar. I feel like I've looked at this a long time ago...

I know I keep delaying this one, but at some point I am going to look at this again >_>
Comment 7 Phil Ringnalda 2025-06-06 15:55:13 UTC
D'oh, I completely missed seeing that my first thought, just escape() the unchanged text where it's output in line 62, didn't work because we don't actually use jsdiff.js, we use jsdiff.min.js.

So, that instead but patching both jsdiff.js and jsdiff.min.js, and we don't have to think about whether or not a diff of pre-escaped text would cut things in the middle that we didn't want cut.
Comment 8 Emily Lamancusa (emlam) 2025-06-25 14:44:54 UTC
(In reply to Phil Ringnalda from comment #7)
> D'oh, I completely missed seeing that my first thought, just escape() the
> unchanged text where it's output in line 62, didn't work because we don't
> actually use jsdiff.js, we use jsdiff.min.js.
> 
> So, that instead but patching both jsdiff.js and jsdiff.min.js, and we don't
> have to think about whether or not a diff of pre-escaped text would cut
> things in the middle that we didn't want cut.

Whoops, this fell off my radar for a bit! That sounds reasonable to me - I'll upload a new patch. Thanks for the feedback!
Comment 9 Emily Lamancusa (emlam) 2025-06-25 15:05:56 UTC
Created attachment 183510 [details] [review]
Bug 40030: Escape values when displaying action logs comparison

When viewing the action logs for a system preference change, there is an
option to view a comparison between different log lines for the same
system preference. However, if a system preference value contains HTML
markup (e.g. in IntranetUserJS), the diff text is only escaped on
sections where a change is marked, so unchanged lines containing HTML
markup result in the HTML being rendered.

Escape the string values for unchanged sections as well, to ensure that
HTML markup in the diff text will not be rendered.

To test:
1. Edit IntranetUserJS and add some code that contains HTML markup.
   For example:
$("body").append('<a id="helloworld" class="btn btn-default">Say hi</a>');
2. Save the preference
3. Make another change to IntranetUserJS that does not affect the line
   you just added, and save again to ensure there are at least 2 action
   log lines for comparison
4. Go to Tools > Log viewer
5. Select System Preferences module and Submit
6. Find the log entries for the above two revisions to IntranetUserJS
7. Check the Compare checkboxes on those entries, and click View
   comparison
--> Note that the HTML button is rendered in the comparison
8. Apply patch
9. Clear browser cache and refresh the page
10. Repeat step 7
--> The button is not rendered and the code is displayed as written
11. Make a change to the line you added in step 1 (for example, replace
    "Say hi" with "Say hello")
12. Refresh the page
13. Check the checkboxes for the newest 2 log entries and click View
    comparison to view the diff for that last change
--> Confirm the change is shown correctly - the button is not rendered
    and nothing has been double-escaped
Comment 10 Phil Ringnalda 2025-06-26 22:55:05 UTC
Created attachment 183562 [details] [review]
Bug 40030: Escape values when displaying action logs comparison

When viewing the action logs for a system preference change, there is an
option to view a comparison between different log lines for the same
system preference. However, if a system preference value contains HTML
markup (e.g. in IntranetUserJS), the diff text is only escaped on
sections where a change is marked, so unchanged lines containing HTML
markup result in the HTML being rendered.

Escape the string values for unchanged sections as well, to ensure that
HTML markup in the diff text will not be rendered.

To test:
1. Edit IntranetUserJS and add some code that contains HTML markup.
   For example:
$("body").append('<a id="helloworld" class="btn btn-default">Say hi</a>');
2. Save the preference
3. Make another change to IntranetUserJS that does not affect the line
   you just added, and save again to ensure there are at least 2 action
   log lines for comparison
4. Go to Tools > Log viewer
5. Select System Preferences module and Submit
6. Find the log entries for the above two revisions to IntranetUserJS
7. Check the Compare checkboxes on those entries, and click View
   comparison
--> Note that the HTML button is rendered in the comparison
8. Apply patch
9. Clear browser cache and refresh the page
10. Repeat step 7
--> The button is not rendered and the code is displayed as written
11. Make a change to the line you added in step 1 (for example, replace
    "Say hi" with "Say hello")
12. Refresh the page
13. Check the checkboxes for the newest 2 log entries and click View
    comparison to view the diff for that last change
--> Confirm the change is shown correctly - the button is not rendered
    and nothing has been double-escaped

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 11 Marcel de Rooy 2025-06-27 07:12:59 UTC
(In reply to Phil Ringnalda from comment #5)
> Pretty sure there's neither support nor a repo, just the twenty year old
> blog post linked in the comment of jsdiff.js. I'd say we shouldn't spend too
> much time on improving it, just fix this for now and then look for a better
> library. While coming up with a step to see the double-escaping, I hit
> another bug mentioned in the comments on the blog post, that moving things
> around doesn't get marked in the diff, though I missed seeing some others
> also mentioned there.

So we patched this library here. Did we open a new report for replacing this (old) library?
Comment 12 Marcel de Rooy 2025-06-27 07:13:26 UTC
Created attachment 183581 [details] [review]
Bug 40030: Escape values when displaying action logs comparison

When viewing the action logs for a system preference change, there is an
option to view a comparison between different log lines for the same
system preference. However, if a system preference value contains HTML
markup (e.g. in IntranetUserJS), the diff text is only escaped on
sections where a change is marked, so unchanged lines containing HTML
markup result in the HTML being rendered.

Escape the string values for unchanged sections as well, to ensure that
HTML markup in the diff text will not be rendered.

To test:
1. Edit IntranetUserJS and add some code that contains HTML markup.
   For example:
$("body").append('<a id="helloworld" class="btn btn-default">Say hi</a>');
2. Save the preference
3. Make another change to IntranetUserJS that does not affect the line
   you just added, and save again to ensure there are at least 2 action
   log lines for comparison
4. Go to Tools > Log viewer
5. Select System Preferences module and Submit
6. Find the log entries for the above two revisions to IntranetUserJS
7. Check the Compare checkboxes on those entries, and click View
   comparison
--> Note that the HTML button is rendered in the comparison
8. Apply patch
9. Clear browser cache and refresh the page
10. Repeat step 7
--> The button is not rendered and the code is displayed as written
11. Make a change to the line you added in step 1 (for example, replace
    "Say hi" with "Say hello")
12. Refresh the page
13. Check the checkboxes for the newest 2 log entries and click View
    comparison to view the diff for that last change
--> Confirm the change is shown correctly - the button is not rendered
    and nothing has been double-escaped

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Lucas Gass (lukeg) 2025-06-30 14:42:44 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 14 Paul Derscheid 2025-07-28 09:51:27 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 15 Fridolin Somers 2025-07-28 09:53:35 UTC
Pushed to 24.11.x for 24.11.08