Bug 37918 - Properly encode email subjects in mailto: links
Summary: Properly encode email subjects in mailto: links
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Martin Renvoize (ashimema)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-09-13 14:05 UTC by Katrin Fischer
Modified: 2025-01-24 11:13 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 37918: Add email_header template toolkit filter (4.92 KB, patch)
2024-09-16 09:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37918: Add email_header template toolkit filter (4.97 KB, patch)
2024-09-26 15:56 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 37918: Remove the new dependency (1.51 KB, patch)
2025-01-15 15:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37918: Add filter to existing email subjects (12.51 KB, patch)
2025-01-21 15:57 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37918: Add email_header template toolkit filter (5.03 KB, patch)
2025-01-21 16:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37918: Remove the new dependency (1.56 KB, patch)
2025-01-21 16:08 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37918: Add email_header template toolkit filter (5.03 KB, patch)
2025-01-21 16:32 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37918: Remove the new dependency (1.56 KB, patch)
2025-01-21 16:32 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37918: Add filter to existing email subjects (12.56 KB, patch)
2025-01-21 16:32 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37918: Correct POD (1.12 KB, patch)
2025-01-24 11:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 37918: Correct usage (11.34 KB, patch)
2025-01-24 11:12 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2024-09-13 14:05:38 UTC
There are some places in Koha where we use mailto: links with a subject line. 
An example is the email link added by bug 36595.

I believe we need to take a closer look at making sure we are encoding the subject line correctly.

Also need to make sure to not include untranslatable and therefore hardcoded bits into the subject messages.
Comment 1 Martin Renvoize (ashimema) 2024-09-13 14:13:15 UTC
So, on my quick research I reckon we need a new TT filter to make sure subjects are properly RFC2047 encoded.

We should be able to add that fairly trivially using Encode::MIME::Header I think.
Comment 2 Martin Renvoize (ashimema) 2024-09-16 09:31:51 UTC
Created attachment 171518 [details] [review]
Bug 37918: Add email_header template toolkit filter

This patch adds a new email_header TT filter to properly encode strings
according to RFC2047 as per the email specification.  This allows
developers to set default subjects in mailto links with the correct
encoding.
Comment 3 Shi Yao Wang 2024-09-26 15:56:27 UTC
Created attachment 171994 [details] [review]
Bug 37918: Add email_header template toolkit filter

This patch adds a new email_header TT filter to properly encode strings
according to RFC2047 as per the email specification.  This allows
developers to set default subjects in mailto links with the correct
encoding.

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Comment 4 Shi Yao Wang 2024-09-26 15:58:49 UTC
Assuming I only needed to make sure the test passes for the sign-off.

Should we include refactoring of mailto with subject in this bug?
Comment 5 Martin Renvoize (ashimema) 2025-01-07 14:14:00 UTC
Yes indeed, that was the next step here.
Comment 6 Jonathan Druart 2025-01-15 15:20:29 UTC
Created attachment 176576 [details] [review]
Bug 37918: Remove the new dependency
Comment 7 Jonathan Druart 2025-01-15 15:20:40 UTC
Do we really need the new dep?
Comment 8 Jonathan Druart 2025-01-15 15:21:33 UTC
24 use Encode::MIME::Header;

but you are not using it?
Comment 9 Paul Derscheid 2025-01-20 17:46:35 UTC
Would PQA this, if it weren't for these 2 lines:
> Should we include refactoring of mailto with subject in this bug?
> Yes indeed, that was the next step here.
Comment 10 Martin Renvoize (ashimema) 2025-01-21 15:21:16 UTC
(In reply to Jonathan Druart from comment #8)
> 24 use Encode::MIME::Header;
> 
> but you are not using it?

That module is what adds the 'MIME-Header' support to 'encode'.. without it we're not encoding in MIME-Header format... see line 55 of the patch.
Comment 11 Martin Renvoize (ashimema) 2025-01-21 15:23:09 UTC
If someone wants to follow-up here and apply the filter to all the current email subjects that would be great.. I'm unlikely to have a chance to continue here myself any time soon.
Comment 12 Paul Derscheid 2025-01-21 15:26:42 UTC
So should we change the bug description and scope this bug to the filter itself? Then we at least have it.

Also quickly looked again at: https://metacpan.org/pod/Encode::MIME::Header

I don't like it when libraries work that way, but it is what it is.
Comment 13 Martin Renvoize (ashimema) 2025-01-21 15:57:24 UTC
Created attachment 176869 [details] [review]
Bug 37918: Add filter to existing email subjects
Comment 14 Paul Derscheid 2025-01-21 16:07:59 UTC
Created attachment 176870 [details] [review]
Bug 37918: Add email_header template toolkit filter

This patch adds a new email_header TT filter to properly encode strings
according to RFC2047 as per the email specification.  This allows
developers to set default subjects in mailto links with the correct
encoding.

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 15 Paul Derscheid 2025-01-21 16:08:02 UTC
Created attachment 176871 [details] [review]
Bug 37918: Remove the new dependency

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 16 Paul Derscheid 2025-01-21 16:09:07 UTC
Sent a keyboard interrupt by accident.. Fixing.
Comment 17 Martin Renvoize (ashimema) 2025-01-21 16:29:33 UTC
Hopefully that's all the cases covered.. must admit, I've not actually tested that last patch
Comment 18 Paul Derscheid 2025-01-21 16:32:14 UTC
Created attachment 176873 [details] [review]
Bug 37918: Add email_header template toolkit filter

This patch adds a new email_header TT filter to properly encode strings
according to RFC2047 as per the email specification.  This allows
developers to set default subjects in mailto links with the correct
encoding.

Signed-off-by: Shi Yao Wang <shi-yao.wang@inlibro.com>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 19 Paul Derscheid 2025-01-21 16:32:16 UTC
Created attachment 176874 [details] [review]
Bug 37918: Remove the new dependency

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 20 Paul Derscheid 2025-01-21 16:32:19 UTC
Created attachment 176875 [details] [review]
Bug 37918: Add filter to existing email subjects

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 21 Marcel de Rooy 2025-01-24 09:07:10 UTC
This patch set raises a few questions:

[1] How is [% something | email_header %] supposed to work? Where does email_header link to the EmailHeader module? I think this just returns nothing.
I would expect [% something| $EmailHeader %]
Since this was tested by two people, please explain. (No test plan in the third patch.)

[2] Are you sure that you are doing the right thing here? We are in an HTML context, not working with email headers directly but representing them in html.
Please clarify.
Why =?UTF-8?B?SGVsbG8gV29ybGQ=?= instead of Hello%20World for instance?
Should the script sending a mail subject to the html template be responsible?

[3] How do you prevent double encoding?
Comment 22 Paul Derscheid 2025-01-24 10:53:04 UTC
You are correct Marcel, I was careless especially with that last patch.
I had a conversation with Martin during QA and didn't run the qa script when quickly signing off that last patch (which would've caught the incorrect usage).

Thanks for catching my mistake.

In regard to the reasoning:
1) Since the tests passed (and there was no usage at that point during QA), I didn't even look at the usage in the POD. You are correct, the correct usage should be [% something | $EmailHeader %].
2) This needs further investigation, I can't even reproduce the root issue that this bug was about with unfiltered korean.
3) Valid concern.
Comment 23 Paul Derscheid 2025-01-24 11:12:28 UTC
Created attachment 177103 [details] [review]
Bug 37918: Correct POD
Comment 24 Paul Derscheid 2025-01-24 11:12:31 UTC
Created attachment 177104 [details] [review]
Bug 37918: Correct usage
Comment 25 Paul Derscheid 2025-01-24 11:13:39 UTC
Here's at least the usage corrected, but it would be nice to see a failing example.