Evergreen

Email templates should always escape headers

Bug #1031335 reported by Dan Scott on 2012-07-31

This bug affects 3 people

	Status	Importance	Assigned to	Milestone
Evergreen	Fix Released	Medium	Unassigned	Evergreen 2.11-alpha
2.10	Fix Released	Medium	Unassigned	Evergreen 2.10.3
2.9	Fix Released	Medium	Unassigned	Evergreen 2.9.5

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...

Tags:

Revision history for this message

Thomas Berezansky (tsbere) wrote on 2012-07-31:

Possible solution for the "updating existing templates" problem:

http://pastebin.com/14sJWZ3r

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

Jason Stephenson (jstephenson) on 2012-08-03

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

Jason Stephenson (jstephenson) on 2012-09-04

Changed in evergreen:
milestone:	2.3.0-rc1 → 2.3.0

Jason Stephenson (jstephenson) on 2012-09-19

Changed in evergreen:
milestone:	2.3.0 → 2.3.1

Jason Stephenson (jstephenson) on 2012-11-08

Changed in evergreen:
milestone:	2.3.1 → 2.4.0-alpha

Jason Stephenson (jstephenson) on 2012-12-13

Changed in evergreen:
status:	New → Triaged

Revision history for this message

Thomas Berezansky (tsbere) wrote on 2013-01-03:

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

Revision history for this message

Galen Charlton (gmc) wrote on 2013-05-06:

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?

Revision history for this message

Pasi Kallinen (paxed) wrote on 2013-05-07:

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

Revision history for this message

Pasi Kallinen (paxed) wrote on 2013-07-24:

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

Revision history for this message

Dan Wells (dbw2) wrote on 2013-08-26:

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

Revision history for this message

Mike Rylander (mrylander) wrote on 2013-10-18:

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

Revision history for this message

Dan Wells (dbw2) wrote on 2014-01-17:

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

Evergreen Bug Maintenance (bugmaster) on 2014-04-07

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

Revision history for this message

Dan Scott (denials) wrote on 2015-01-07:

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

Evergreen Bug Maintenance (bugmaster) on 2015-06-18

Changed in evergreen:
assignee:	Dan Wells (dbw2) → nobody
milestone:	2.8.1 → 2.8.3

Evergreen Bug Maintenance (bugmaster) on 2015-09-04

no longer affects:	evergreen/2.5
no longer affects:	evergreen/2.6
Changed in evergreen:
milestone:	2.8.3 → 2.9.0

Evergreen Bug Maintenance (bugmaster) on 2015-09-17

Changed in evergreen:
milestone:	2.9.0 → 2.9.1

Evergreen Bug Maintenance (bugmaster) on 2015-11-25

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)

Revision history for this message

Dan Wells (dbw2) wrote on 2016-04-29:

#10

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

Evergreen Bug Maintenance (bugmaster) on 2016-08-09

Changed in evergreen:
milestone:	2.next → 2.11-alpha

Evergreen Bug Maintenance (bugmaster) on 2016-11-14

Changed in evergreen:
status:	Fix Committed → Fix Released

Report a bug

This report contains Public information

Everyone can see this information.

You are

Subscribing...

Edit bug mail

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.