Bug 40675 - Carriage return in patron note message breaks SIP
Summary: Carriage return in patron note message breaks SIP
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Bernard
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-08-20 10:21 UTC by Bernard
Modified: 2025-10-16 09:54 UTC (History)
5 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
25.11.00,25.05.04,24.11.10
Circulation function:


Attachments
Bug 40675 (1.38 KB, patch)
2025-08-20 10:56 UTC, Bernard
Details | Diff | Splinter Review
Bug 40675 (1.44 KB, patch)
2025-08-26 15:07 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 40675 (1.38 KB, patch)
2025-08-26 15:43 UTC, Bernard
Details | Diff | Splinter Review
Bug 40675 (1.38 KB, patch)
2025-08-26 15:47 UTC, Bernard
Details | Diff | Splinter Review
Bug 40675 (1.38 KB, patch)
2025-08-26 15:51 UTC, Bernard
Details | Diff | Splinter Review
Bug 40675: Added new regex to prevent regression (1.28 KB, patch)
2025-08-26 16:08 UTC, Bernard
Details | Diff | Splinter Review
Bug 40675: Fix SIP message corruption from embedded line endings (2.26 KB, patch)
2025-08-27 15:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40675: Fix SIP message corruption from embedded line endings (2.26 KB, patch)
2025-09-16 09:10 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40675: Add unit tests for SIP line ending normalization (2.68 KB, patch)
2025-09-16 09:10 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 40675: Fix SIP message corruption from embedded line endings (2.40 KB, patch)
2025-09-19 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40675: Add unit tests for SIP line ending normalization (2.71 KB, patch)
2025-09-19 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 40675: Add unit tests for SIP line ending normalization (2.77 KB, patch)
2025-09-19 08:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernard 2025-08-20 10:21:01 UTC
If an OPAC message is pasted into a Koha patron's record which includes a carriage return, it breaks the line and SIP. A user at the Kiosk may find they can no longer log in to the SIP kiosk. The SIP2 specification uses the carriage return as the message terminator.
Comment 1 Bernard 2025-08-20 10:56:44 UTC
Created attachment 185583 [details] [review]
Bug 40675

Removes carriage returns from messages to prevent breaking SIP

Test
1. Find a user and click "Add message".
2. Choose "Add message for OPAC ... " from dropdown
3. In Word (or similar, type a message adding a carriage return in the middle of it.
4. Copy / paste this text into the message box and save.
5. Run the SIP emulator tool sip_cli_emulator.pl and observe that after the carriage return, the message has broken onto a new line.
6. Install patch
7. restart_all
8. Repeat above 1-5
9. Observe that runaway line is now joined to the previous line (correctly)
Comment 2 Andrew Fuerste-Henry 2025-08-26 15:07:56 UTC
Created attachment 185791 [details] [review]
Bug 40675

Removes carriage returns from messages to prevent breaking SIP

Test
1. Find a user and click "Add message".
2. Choose "Add message for OPAC ... " from dropdown
3. In Word (or similar, type a message adding a carriage return in the middle of it.
4. Copy / paste this text into the message box and save.
5. Run the SIP emulator tool sip_cli_emulator.pl and observe that after the carriage return, the message has broken onto a new line.
6. Install patch
7. restart_all
8. Repeat above 1-5
9. Observe that runaway line is now joined to the previous line (correctly)

Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Comment 3 Bernard 2025-08-26 15:43:21 UTC
Created attachment 185792 [details] [review]
Bug 40675

Removes carriage returns from messages to prevent breaking SIP - new regex to prevent regression.

I am told my original patch could cause a regression and should instead be 
$value =~ s/\r\n|\r|\n/ /g;

"We probably really want to normalise all options to a space as opposed to switching options. My original change switches us from Mac CR characters to Windows combination of CRLF (and unit tends to just stick to LF in the olden days).. so to really fix the bug we should look for all three as above."

Test
1. Find a user and click "Add message".
2. Choose "Add message for OPAC ... " from dropdown
3. In Word (or similar, type a message adding a carriage return in the middle of it.
4. Copy / paste this text into the message box and save.
5. Run the SIP emulator tool sip_cli_emulator.pl and observe that after the carriage return, the message has broken onto a new line.
6. Install patch
7. restart_all
8. Repeat above 1-5
9. Observe that runaway line is now joined to the previous line (correctly)
Comment 4 Bernard 2025-08-26 15:47:01 UTC
Created attachment 185793 [details] [review]
Bug 40675

Removes carriage returns from messages to prevent breaking SIP

Test
1. Find a user and click "Add message".
2. Choose "Add message for OPAC ... " from dropdown
3. In Word (or similar, type a message adding a carriage return in the middle of it.
4. Copy / paste this text into the message box and save.
5. Run the SIP emulator tool sip_cli_emulator.pl and observe that after the carriage return, the message has broken onto a new line.
6. Install patch
7. restart_all
8. Repeat above 1-5
9. Observe that runaway line is now joined to the previous line (correctly)
Comment 5 Bernard 2025-08-26 15:51:16 UTC
Created attachment 185794 [details] [review]
Bug 40675

Removes carriage returns from messages to prevent breaking SIP - new regex to prevent regression.

I am told my original patch could cause a regression and should instead be
$value =~ s/\r\n|\r|\n/ /g;

"We probably really want to normalise all options to a space as opposed to switching options. My original change switches us from Mac CR characters to Windows combination of CRLF (and unit tends to just stick to LF in the olden days).. so to really fix the bug we should look for all three as above."

Test
1. Find a user and click "Add message".
2. Choose "Add message for OPAC ... " from dropdown
3. In Word (or similar, type a message adding a carriage return in the middle of it.
4. Copy / paste this text into the message box and save.
5. Run the SIP emulator tool sip_cli_emulator.pl and observe that after the carriage return, the message has broken onto a new line.
6. Install patch
7. restart_all
8. Repeat above 1-5
9. Observe that runaway line is now joined to the previous line (correctly)
Comment 6 Bernard 2025-08-26 16:08:07 UTC
Created attachment 185802 [details] [review]
Bug 40675: Added new regex to prevent regression

I am told my original patch could cause a regression and should instead be
$value =~ s/\r\n|\r|\n/ /g;

"We probably really want to normalise all options to a space as opposed to switching options. My original change switches us from Mac CR characters to Windows combination of CRLF (and unit tends to just stick to LF in the olden days).. so to really fix the bug we should look for all three as above."
Comment 7 Martin Renvoize (ashimema) 2025-08-27 15:26:54 UTC
Created attachment 185833 [details] [review]
Bug 40675: Fix SIP message corruption from embedded line endings

Patron messages containing carriage returns, line feeds, or CRLF
combinations can break SIP protocol communication since CR is used
as the SIP message terminator. This causes message parsing failures
and protocol errors in self-check systems.

The previous regex only handled single carriage returns (\r),
missing Windows CRLF (\r\n) and Unix LF (\n) sequences.

This patch normalizes all line ending variations to spaces to
prevent SIP message corruption while preserving message content.

Test plan:
1. Go to a patron record and click "Add message"
2. Select "Add message for OPAC..." from dropdown
3. In a text editor (e.g., Word), create a message with line breaks:
   "This is line one[Enter]This is line two[Enter]This is line three"
4. Copy and paste this multi-line text into the message box and save
5. Run the SIP emulator: misc/sip_cli_emulator.pl
6. Execute a patron information request for this patron
7. Before patch: Observe message breaks at line endings, corrupting
   the SIP response
8. Apply patch and restart SIP server
9. Repeat steps 5-6
10. After patch: Verify the message appears as single line with
    spaces replacing line breaks: "This is line one This is line two
    This is line three"
11. Test with different line ending types (Windows, Mac, Unix) to
    ensure all variations are handled correctly

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 8 Marcel de Rooy 2025-08-29 06:35:06 UTC
Please add a test for a module change :)
Comment 9 Martin Renvoize (ashimema) 2025-09-16 09:10:06 UTC
Created attachment 186456 [details] [review]
Bug 40675: Fix SIP message corruption from embedded line endings

Patron messages containing carriage returns, line feeds, or CRLF
combinations can break SIP protocol communication since CR is used
as the SIP message terminator. This causes message parsing failures
and protocol errors in self-check systems.

The previous regex only handled single carriage returns (\r),
missing Windows CRLF (\r\n) and Unix LF (\n) sequences.

This patch normalizes all line ending variations to spaces to
prevent SIP message corruption while preserving message content.

Test plan:
1. Go to a patron record and click "Add message"
2. Select "Add message for OPAC..." from dropdown
3. In a text editor (e.g., Word), create a message with line breaks:
   "This is line one[Enter]This is line two[Enter]This is line three"
4. Copy and paste this multi-line text into the message box and save
5. Run the SIP emulator: misc/sip_cli_emulator.pl
6. Execute a patron information request for this patron
7. Before patch: Observe message breaks at line endings, corrupting
   the SIP response
8. Apply patch and restart SIP server
9. Repeat steps 5-6
10. After patch: Verify the message appears as single line with
    spaces replacing line breaks: "This is line one This is line two
    This is line three"
11. Test with different line ending types (Windows, Mac, Unix) to
    ensure all variations are handled correctly

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 10 Martin Renvoize (ashimema) 2025-09-16 09:10:08 UTC
Created attachment 186457 [details] [review]
Bug 40675: Add unit tests for SIP line ending normalization

This patch adds comprehensive unit tests for the add_field function
to verify that all types of line endings (Unix \n, Windows \r\n,
Mac \r, and mixed) are properly normalized to spaces to prevent
SIP message corruption.

Tests cover:
- Unix line endings (\n)
- Windows line endings (\r\n)
- Mac line endings (\r)
- Mixed line ending combinations
- Normal text without line endings
- Empty values
Comment 11 Marcel de Rooy 2025-09-19 08:28:28 UTC
Looking here
Comment 12 Marcel de Rooy 2025-09-19 08:32:48 UTC
Created attachment 186608 [details] [review]
Bug 40675: Fix SIP message corruption from embedded line endings

Patron messages containing carriage returns, line feeds, or CRLF
combinations can break SIP protocol communication since CR is used
as the SIP message terminator. This causes message parsing failures
and protocol errors in self-check systems.

The previous regex only handled single carriage returns (\r),
missing Windows CRLF (\r\n) and Unix LF (\n) sequences.

This patch normalizes all line ending variations to spaces to
prevent SIP message corruption while preserving message content.

Test plan:
1. Go to a patron record and click "Add message"
2. Select "Add message for OPAC..." from dropdown
3. In a text editor (e.g., Word), create a message with line breaks:
   "This is line one[Enter]This is line two[Enter]This is line three"
4. Copy and paste this multi-line text into the message box and save
5. Run the SIP emulator: misc/sip_cli_emulator.pl
6. Execute a patron information request for this patron
7. Before patch: Observe message breaks at line endings, corrupting
   the SIP response
8. Apply patch and restart SIP server
9. Repeat steps 5-6
10. After patch: Verify the message appears as single line with
    spaces replacing line breaks: "This is line one This is line two
    This is line three"
11. Test with different line ending types (Windows, Mac, Unix) to
    ensure all variations are handled correctly

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Adjusted the comment in Sip.pm
Comment 13 Marcel de Rooy 2025-09-19 08:32:50 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2025-09-19 08:33:57 UTC
Created attachment 186610 [details] [review]
Bug 40675: Add unit tests for SIP line ending normalization

This patch adds comprehensive unit tests for the add_field function
to verify that all types of line endings (Unix \n, Windows \r\n,
Mac \r, and mixed) are properly normalized to spaces to prevent
SIP message corruption.

Tests cover:
- Unix line endings (\n)
- Windows line endings (\r\n)
- Mac line endings (\r)
- Mixed line ending combinations
- Normal text without line endings
- Empty values

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Lucas Gass (lukeg) 2025-09-19 14:45:11 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 16 Paul Derscheid 2025-09-25 16:30:25 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 17 Fridolin Somers 2025-10-16 09:54:03 UTC
Pushed to 24.11.x for 24.11.10