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 plumbing | Assignee: | 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: | Main | Keywords: | 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
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 Sounds like a legitimate bug, and at a glance the code looks sensible, but I'll have to come back to review this. 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 "</a>" rather than "</a>" displayed. 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). 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? (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 >_> 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. (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! 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 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> (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? 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> Nice work everyone! Pushed to main for 25.11 Nice work everyone! Pushed to 25.05.x Pushed to 24.11.x for 24.11.08 |