Bug 25381

Summary: XSLTs should not define entities
Product: Koha Reporter: David Cook <dcook>
Component: Architecture, internals, and plumbingAssignee: Didier Gautheron <didier.gautheron>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: aleisha, andrewfh, caroline.cyr-la-rose, didier.gautheron, fridolin.somers, jonathan.druart, victor
Version: MainKeywords: Academy
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25373
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23290
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.03,20.05.09
Attachments: Bug 25381: XSLTs should not define entities
Bug 25381: XSLTs should not define entities
Bug 25381: XSLTs should not define entities
Bug 25381: XSLTs should not define entities
Bug 25381: XSLTs should not define entities
Bug 25381: (QA follow-up) Fix occurence in the one file that escaped

Description David Cook 2020-05-05 23:25:39 UTC
Bug 23290 explicitly disabled entity expansion during XML parsing for security reasons.

However, many XSLTs define the following entity:
<!ENTITY nbsp "&#160;" >

They don't use the entity &nbsp, but its presence could lead to confusion (as noted in Bug 25373).

These entities should be removed from the XSLTs.
Comment 1 Didier Gautheron 2020-05-08 14:07:13 UTC
Created attachment 104599 [details] [review]
Bug 25381: XSLTs should not define entities

Bug 23290 explicitly disabled entity expansion during XML parsing for security reasons.

However, many XSLTs define the following entity:
<!ENTITY nbsp "&#160;" >

They don't use the entity &nbsp, but its presence could lead to confusion.
Comment 2 Didier Gautheron 2020-05-08 14:09:51 UTC
Output of:

for i in $(git grep -lF '<!DOCTYPE stylesheet [<!ENTITY nbsp "&#160;" >]>' ) ; do sed -i 's/ \[<!ENTITY nbsp "\&#160;" >\]//' $i ; done

In Koha source tree
Comment 3 Aleisha Amohia 2021-01-20 23:52:33 UTC
Is this ready for testing? If yes can you please adjust the bug status accordingly?
Comment 4 Katrin Fischer 2021-01-21 19:53:48 UTC
I have tested the patch appleis still, switching status.
Comment 5 Aleisha Amohia 2021-01-21 21:36:15 UTC
Cool! Test plan would be to simply eyeball the patch and check that the entity definition has been removed correctly and the code still looks OK!
Comment 6 Eden 2021-01-21 22:46:49 UTC
Created attachment 115632 [details] [review]
Bug 25381: XSLTs should not define entities

Bug 23290 explicitly disabled entity expansion during XML parsing for security reasons.

However, many XSLTs define the following entity:
<!ENTITY nbsp "&#160;" >

They don't use the entity &nbsp, but its presence could lead to confusion.
Comment 7 Eden 2021-01-21 22:49:55 UTC
Created attachment 115635 [details] [review]
Bug 25381: XSLTs should not define entities

Bug 23290 explicitly disabled entity expansion during XML parsing for security reasons.

However, many XSLTs define the following entity:
<!ENTITY nbsp "&#160;" >

They don't use the entity &nbsp, but its presence could lead to confusion.

Signed-off-by: Eden Bacani <eden.bacani@gmail.com>
Comment 8 Katrin Fischer 2021-01-21 23:43:15 UTC
Created attachment 115650 [details] [review]
Bug 25381: XSLTs should not define entities

Bug 23290 explicitly disabled entity expansion during XML parsing for security reasons.

However, many XSLTs define the following entity:
<!ENTITY nbsp "&#160;" >

They don't use the entity &nbsp, but its presence could lead to confusion.

Signed-off-by: Eden Bacani <eden.bacani@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2021-01-21 23:45:10 UTC
Created attachment 115651 [details] [review]
Bug 25381: XSLTs should not define entities

Bug 23290 explicitly disabled entity expansion during XML parsing for security reasons.

However, many XSLTs define the following entity:
<!ENTITY nbsp "&#160;" >

They don't use the entity &nbsp, but its presence could lead to confusion.

Signed-off-by: Eden Bacani <eden.bacani@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2021-01-21 23:45:14 UTC
Created attachment 115652 [details] [review]
Bug 25381: (QA follow-up) Fix occurence in the one file that escaped

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Jonathan Druart 2021-01-22 13:54:00 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 12 Fridolin Somers 2021-01-29 13:18:21 UTC
Pushed to 20.11.x for 20.11.03
Comment 13 Andrew Fuerste-Henry 2021-02-03 22:07:02 UTC
Pushed to 20.05.x for 20.05.09
Comment 14 Victor Grousset/tuxayo 2021-02-03 23:10:50 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.