Email templates should always escape headers

Bug #1031335 reported by Dan Scott on 2012-07-31
14
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
2.10
Medium
Unassigned
2.9
Medium
Unassigned

Bug Description

* Evergreen master

OpenILS/Application/Trigger/Reactor.pm provides a helper method, escape_email_header(), that can & should be used to prevent corruption of headers containing Unicode characters -- such as patron names, email addresses, or library names. Like:

"""
To: [%- helpers.escape_email_header(params.recipient_email || user.email) %]
"""

All of the email templates in the seed values should be updated to escape the headers, so that future copy & paste efforts to create new templates magically get the appropriate values.

We might have to skip automatic upgrades in this case because of the high likelihood that we would overwrite customizations -- unless our upgrade script does something very clever like updating the row only where the template matches the previously known template. Sounds like a lot of work, but might be worth it for preventing problems where we can...

Thomas Berezansky (tsbere) wrote :

Possible solution for the "updating existing templates" problem:

http://pastebin.com/14sJWZ3r

Not perfect, but better than nothing (or wiping out templates).

Changed in evergreen:
milestone: 2.3.0-beta1 → 2.3.0-beta2
Ben Shum (bshum) on 2012-08-24
Changed in evergreen:
milestone: 2.3.0-beta2 → 2.3.0-rc1
Changed in evergreen:
milestone: 2.3.0-rc1 → 2.3.0
Changed in evergreen:
milestone: 2.3.0 → 2.3.1
Changed in evergreen:
milestone: 2.3.1 → 2.4.0-alpha
Changed in evergreen:
status: New → Triaged
Thomas Berezansky (tsbere) wrote :

For note: the function I wrote may need to be adjusted to include the SMS reactor too, as it is basically just calling the email one.

Ben Shum (bshum) on 2013-03-03
Changed in evergreen:
milestone: 2.4.0-alpha1 → 2.4.0-beta
Ben Shum (bshum) on 2013-03-17
Changed in evergreen:
milestone: 2.4.0-beta → 2.4.0-rc
Ben Shum (bshum) on 2013-04-27
Changed in evergreen:
milestone: 2.4.0-rc → none
Pasi Kallinen (paxed) on 2013-05-06
tags: added: i18n
Galen Charlton (gmc) wrote :

Thinking aloud, tacking on a lot of helpers.escape_email_header() calls is inelegant, and doesn't convey useful information to a librarian who is tweaking the templates. It just would become another step to follow to make obeisance to the cargo planes that will be coming any day now. (And to be clear, I'm not being dismissive of the actual problem).

Should we instead teach the SendEmail reactor to do some post-processing of the template output? Or at least add support for storing a library of standard template includes so that a user can just set some variables for sender, receiver, and subject and invoke a template to get back a fully fleshed-out set of email headers?

Pasi Kallinen (paxed) wrote :

No idea which would be better, but perhaps a library of std template macros would be more useful ...

Pasi Kallinen (paxed) wrote :

A fix is in http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/paxed/lp1031335

It automatically MIME-encodes the From, To, Subject, Bcc, Cc, Reply-To and Sender -headers.

tags: added: pullrequest
Changed in evergreen:
milestone: none → 2.5.0-alpha1
Remington Steed (rjs7) on 2013-08-12
Changed in evergreen:
milestone: 2.5.0-alpha1 → 2.5.0-alpha2
Dan Wells (dbw2) wrote :

I like Pasi/Galen's solution, but it has at least one flaw: for any templates already using the helper, the headers will get double-encoded.

Possible solutions:
1) decode the header before encoding it (or just check for the encoding marker at the start of the string) - This wouldn't cover every theoretical case, but since it is extremely unlikely for a header to 'appear' MIME encoded, but not actually be, it would be a simple solution for every reasonable case.
2) inform upgraders to remove the 'helper' from their templates - the helper isn't used in any stock templates, so no issue there, but if anyone took the time to add it, they will need to remove it.

Marking as incomplete pending opinions on which route is most viable, or if we might try something else entirely.

Changed in evergreen:
status: Triaged → Incomplete
Dan Wells (dbw2) on 2013-08-26
Changed in evergreen:
milestone: 2.5.0-alpha2 → 2.5.0-beta1
Dan Wells (dbw2) on 2013-09-29
Changed in evergreen:
milestone: 2.5.0-beta1 → 2.5.0-rc
Dan Wells (dbw2) on 2013-10-16
Changed in evergreen:
milestone: 2.5.0-rc → 2.5.1
Mike Rylander (mrylander) wrote :

Extending Dan's thoughts above:

Option 3 -> Make the helper function a no-op AND have the SendEmail handler deal with encoding the headers.

Ben Shum (bshum) on 2013-12-06
Changed in evergreen:
milestone: 2.5.1 → 2.5.2
Dan Wells (dbw2) wrote :

Mike's solution sounds smart to me. I am assigning this to myself to work on when time allows, but if someone else wants it, feel free.

no longer affects: evergreen/2.3
Changed in evergreen:
milestone: 2.5.2 → none
assignee: nobody → Dan Wells (dbw2)
Dan Wells (dbw2) on 2014-02-11
Changed in evergreen:
milestone: none → 2.6.0-beta1
Dan Wells (dbw2) on 2014-02-27
Changed in evergreen:
milestone: 2.6.0-beta1 → 2.6.0-rc1
Changed in evergreen:
milestone: 2.6.0-rc1 → 2.6.1
milestone: 2.6.1 → 2.next
Galen Charlton (gmc) on 2014-09-09
no longer affects: evergreen/2.4
Dan Scott (denials) wrote :

This needs to be fixed. I pushed http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/dbs/lp1031335_encode_email_headers to no-op the helper as suggested. Let's do this.

Changed in evergreen:
status: Incomplete → Confirmed
milestone: 2.next → 2.8-beta
Ben Shum (bshum) on 2015-04-02
Changed in evergreen:
milestone: 2.8-beta → 2.8.1
Changed in evergreen:
assignee: Dan Wells (dbw2) → nobody
milestone: 2.8.1 → 2.8.3
no longer affects: evergreen/2.5
no longer affects: evergreen/2.6
Changed in evergreen:
milestone: 2.8.3 → 2.9.0
Changed in evergreen:
milestone: 2.9.0 → 2.9.1
Changed in evergreen:
milestone: 2.9.1 → 2.next
Kathy Lussier (klussier) on 2015-12-09
Changed in evergreen:
importance: Undecided → Medium
Dan Wells (dbw2) on 2016-04-29
Changed in evergreen:
assignee: nobody → Dan Wells (dbw2)
Dan Wells (dbw2) wrote :

Way overdue, this looks good, and works for me. Can't see a good way for automated testing, given that the only outputs are to email and a no-op. Pushed to master through rel_2_9. Thanks, Dan and Pasi!

Changed in evergreen:
assignee: Dan Wells (dbw2) → nobody
status: Confirmed → Fix Committed
no longer affects: evergreen/2.8
no longer affects: evergreen/2.7
Changed in evergreen:
milestone: 2.next → 2.11-alpha
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers